[sssd PR#85][opened] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Author: celestian
 Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()
Action: opened

PR body:
"""
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
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/85/head:pr85
git checkout pr85

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
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@<domain> and Administrator@<subdomain>
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-262537432

[sssd PR#85][+Changes requested] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

Label: +Changes requested

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
So, I will rewrite tests for sysdb_try_to_find_expected_dn() to suitable form for sdap_object_in_domain().
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-263226837

[sssd PR#85][synchronized] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Author: celestian
 Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/85/head:pr85
git checkout pr85

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
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?
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-271571357

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
Solved, thanks to @lslebodn .
I will prepare new version.
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-271581697

[sssd PR#85][synchronized] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Author: celestian
 Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/85/head:pr85
git checkout pr85

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
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).
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-272460868

[sssd PR#85][synchronized] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Author: celestian
 Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/85/head:pr85
git checkout pr85

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
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?
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-272900707

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

sumit-bose commented:
"""
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
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-273496307

[sssd PR#85][synchronized] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Author: celestian
 Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/85/head:pr85
git checkout pr85

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
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```. 
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-273763997

[sssd PR#85][synchronized] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Author: celestian
 Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/85/head:pr85
git checkout pr85

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
New version is pushed. Thanks, @sumit-bose 
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-274036719

[sssd PR#85][-Changes requested] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

Label: -Changes requested

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

celestian commented:
"""
bump
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-276602905

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

jhrozek commented:
"""
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.
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-277966837

[sssd PR#85][+Accepted] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

Label: +Accepted

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

sumit-bose commented:
"""
I'm fine with the patches, please go ahead.
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-277989609

[sssd PR#85][comment] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

jhrozek commented:
"""
master:
 * 0b7ded15e53b3f31f1570c366f04bc41e5761929
 * f1e3364a72eb75673d10cf8c97ba8f1d7a385405
 * 3ee411625aee19afda7477bb10b52c3da378b6fb
 * c3593f06da54315c88a08a46cfc0def366acad43
"""

See the full comment at https://github.com/SSSD/sssd/pull/85#issuecomment-278282561

[sssd PR#85][-Accepted] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

Label: -Accepted

[sssd PR#85][+Pushed] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/85
Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()

Label: +Pushed

[sssd PR#85][closed] SYSDB: Removing of sysdb_try_to_find_expected_dn()

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/85
Author: celestian
 Title: #85: SYSDB: Removing of sysdb_try_to_find_expected_dn()
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/85/head:pr85
git checkout pr85