Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SYSDB: Removing of sysdb_try_to_find_expected_dn() #85

Closed
wants to merge 4 commits into from

Conversation

@celestian
Copy link
Collaborator

commented Nov 23, 2016

Currently in order to match multiple LDAP search results we
use two different functions - we have sysdb_try_to_find_expected_dn()
but also sdap_object_in_domain().

This patch removes sysdb_try_to_find_expected_dn() and add new
sdap_search_initgr_user_in_batch() based on sdap_object_in_domain().
This function covers necessary logic.

Resolves:
https://fedorahosted.org/sssd/ticket/3230

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 23, 2016

Reproducer:

We need AD domain and it's AD subdomain.
If we type in SSSD box connected to AD domain:

id Administrator@<domain>

it resolves between Administrator@ and Administrator@

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 28, 2016

So, I will rewrite tests for sysdb_try_to_find_expected_dn() to suitable form for sdap_object_in_domain().

@celestian celestian force-pushed the celestian:t3230_gc_results_master branch from f26af5f to 18f36c1 Jan 10, 2017

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 10, 2017

WIP of tests added. I have issue with proeprly setting up test environment.
This call (at line 95):

test_ctx->initgr_state->opts = mock_sdap_options_ldap(...

doesn't prepare valid options. Could anybody help me, please?

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 10, 2017

Solved, thanks to @lslebodn .
I will prepare new version.

@celestian celestian force-pushed the celestian:t3230_gc_results_master branch from 18f36c1 to 7b97263 Jan 13, 2017

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2017

There is new version if somebody would like to look how I fight.
The positive test case test_user_is_on_batch is ready,
the negative test case test_user_is_on_batch needs changes in env. setup
(it is copied from the first case).

@celestian celestian force-pushed the celestian:t3230_gc_results_master branch from 7b97263 to 369d258 Jan 16, 2017

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 16, 2017

I pushed new version. Let me update the situation:

There are three commits:

[1] SYSDB: Removing of sysdb_try_to_find_expected_dn()
[2] TEST: create_multidom_test_ctx() extending 
[3] TESTS: Tests for sdap_search_initgr_user_in_batch

The patch [1] is refactor which is requested by https://fedorahosted.org/sssd/ticket/3230.

The patch [2] extends function create_multidom_test_ctx(). We need different search bases so there is array of params instead of one set of params.

The patch [3] adds tests for [1]. The core of [1] is new function sdap_search_initgr_user_in_batch() which calls sdap_object_in_domain() internally. We can see three tests in [3]:

a) test_user_is_on_batch
b) test_user_is_from_subdomain
c) test_user_is_from_another_domain

The tests a), b) works how expected. The test c) doesn't work. I am afraid we have bug on
https://github.com/SSSD/sssd/blob/master/src/providers/ldap/sdap.c#L1695
In my opinion, there should be:

    sdmatch = sdap_domain_get_by_dn(opts, original_dn);
    if (sdmatch == NULL) {
        DEBUG(SSSDBG_FUNC_DATA,
              "The group has no original DN, assuming our domain\n");
        return false;
    }

What do you think about it, @jhrozek? Or anybody else?

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

I think sdap_object_in_domain() and sdap_domain_get_by_dn() are working as expected, only the debug message in the code-block you cited should be corrected to some thing like "The original DN of the group cannot be related to any search base".

sdap_object_in_domain() assumes by default that the given object belongs to the given group which can be seen in the handling of the missing DN. So it makes sense that if the DN cannot be matched to any search bases to assume the same, i.e. 'return true;'.

When test_user_is_from_another_domain() is run there is only one domain, "domain.test.com", available in opts->sdom when sdap_domain_get_by_dn() is called. The search base does not match to the DN of the object from "another_domain.test.com" and NULL is returned. If you setup the test so that there is at least "another_domain.test.com" in the opt->sdom list as well sdap_domain_get_by_dn() can return the domain and in sdap_object_in_domain() false can be returned because the domains are not the same.

HTH

bye,
Sumit

celestian added 3 commits Jan 4, 2017
SYSDB: Removing of sysdb_try_to_find_expected_dn()
Currently in order to match multiple LDAP search results we
use two different functions - we have sysdb_try_to_find_expected_dn()
but also sdap_object_in_domain().

This patch removes sysdb_try_to_find_expected_dn() and add new
sdap_search_initgr_user_in_batch() based on sdap_object_in_domain().
This function covers necessary logic.

Resolves:
https://fedorahosted.org/sssd/ticket/3230
TEST: create_multidom_test_ctx() extending
Function create_multidom_test_ctx() prepares test environment for
multidomains. This patch enables setting of different params for
each domain.

Resolves:
https://fedorahosted.org/sssd/ticket/3230

@celestian celestian force-pushed the celestian:t3230_gc_results_master branch from 369d258 to 47fc4ac Jan 19, 2017

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 19, 2017

I pushed new version of the patch set. I addressed @sumit-bose notes, I hope in right manner.

Unfortunately test_user_is_from_another_domain() doesn't work in expected way. My opinion is that user from another_domain shouldn't be selected. I would like to test negative case.

I found out that function sdap_domain_get_by_dn() doesn't return right domain even other_domain is in opts.

TESTS: Tests for sdap_search_initgr_user_in_batch
This patch provides tests for core logic of
sdap_search_initgr_user_in_batch() function. This function replaces
old approach with sysdb_try_to_find_expected_dn() function.

Resolves:
https://fedorahosted.org/sssd/ticket/3230

@celestian celestian force-pushed the celestian:t3230_gc_results_master branch from 47fc4ac to 2e1245d Jan 20, 2017

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 20, 2017

New version is pushed. Thanks, @sumit-bose

@celestian

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 1, 2017

bump

@jhrozek jhrozek self-assigned this Feb 7, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

If @sumit-bose agrees the use-case he brought up in one of the previous comments is covered, then I think the patches can be pushed. I tested the following cases:

  • id of a user who exists in both the joined domain and a nested domain (administrator can be used here)
  • the same in case the domain is named differently than the joined domain. I find a bug in subdomains code here and opened PR #149 to address this
  • resolving a user from a non-default OU in parent and child domains

All these cases were working as expected. Therefore I'm adding the accepted label and if Sumit agrees, I'll push the patch later.

@jhrozek jhrozek added the Accepted label Feb 7, 2017

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

I'm fine with the patches, please go ahead.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

@jhrozek jhrozek added Pushed and removed Accepted labels Feb 8, 2017

@jhrozek jhrozek closed this Feb 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.