[sssd PR#464][opened] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: opened

PR body:
"""
When using name/gid override together with domain resolution order the
mpg name/gid may be returned instead of the overridden one.

In order to avoid that, let's add a check in case the domain supports
mpg so we can ensure that the originalADname and originalADgidNumber
attributes are the very same as the ones searched and then normally
proceed with the current flow in the code. In case those are not the
same, we *must* follow the code path for the non-mpg domains and then
return the proper values.

Resolves: https://pagure.io/SSSD/sssd/issue/3595

Signed-off-by: Fabiano Fidêncio 
"""

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
Coverity: passed without issues!
Internal CI: waiting for rawhide results, but all the other passed without issues!
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
Coverity: passed without issues!
Internal CI: also passed without issues ... http://vm-031.$%7Babc%7D/logs/job/82/16/summary.html
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

lslebodn commented:
"""
Please write a unit tests for new change in sysdb functions.
"""

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

[sssd PR#464][+Changes requested] SYSDB: Properly handle name/gid override when using domain resolution order

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

Label: +Changes requested

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
Sure thing. I'd appreciate some guidance on how to write a valid test for this case, considering it involves an AD-IPA trust setup.
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

lslebodn commented:
"""
On (08/12/17 16:53), fidencio wrote:
> Sure thing. I'd appreciate some guidance on how to write a valid test for this case, considering it involves an AD-IPA trust setup.

Unit test cover execution of function and therefore does not require any
"AD-IPA trust setup". Unit test != integration test.
You pass some arguments to function and expect some result.
+fail if expected result is different.

Unit testing sometimes requires preparing mock object. It is a simulated
objects that mimic the behavior of real objects in controlled ways[1]
It is a huge benefit because sometimes it is complicated to reproduce some
state with integration tests.

There is lot of resources on net about unit testing[2,3,4]
And we have quite hight code coverage in sysdb (73.8 % of functions)

BTW the function `sysdb_getgrgid` is already used in more than one unit test
test_nested_groups.c, test_sysdb_ts_cache, sysdb-tests.c.

LS

[1] https://en.wikipedia.org/wiki/Mock_object
[2] https://en.wikipedia.org/wiki/Unit_testing
[3] https://www.youtube.com/watch?time_continue=3&v=wEhu57pih5w
[4] http://softwaretestingfundamentals.com/unit-testing/

"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
@lslebodn, wow, thanks for the very good explanation.

I actually misread your first comment as "tests" instead of "unit tests". I'll write it down and update the patch set soon.
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

jhrozek commented:
"""
Hi @fidencio given that there is a downstream BZ where the support person mentioned they might need the fix in downstream I was wondering if you had time to add the test so we can push this patch upstream?
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
@jhrozek, sorry, no.

I'd strongly prefer if someone else could take it over from now.
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
Before I get back to this and spend some time fighting windmills ...

I'd like to know:
- Is the approach valid? Can someone make a review of the approach **before** I start digging into the tests?
- About the tests ... testing this scenario is not the simplest thing to do (or I may be just over complicating it). I'd appreciate some guidance to understand how to prepare a sysdb containing valid data for both mpg and non-mpg case, then the rest should be easy enough to test.

@SSSD/developers, could someone give me a help here?
"""

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
@sumit-bose, did you have the chance to take a look at https://github.com/SSSD/sssd/pull/464#issuecomment-373682283 ?
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

sumit-bose commented:
"""
@fidencio, yes, I think the approach is valid.

About the patch, I would set

fmt_filter = SYSDB_GRNAM_FILTER;
         base_dn = sysdb_group_base_dn(tmp_ctx, domain);

before 'if (domain->mpg)' and only set it in the if block to the mpg related values is all conditions are met.

The check 'if (base_dn == NULL)' should be done after the 'if (domain->mpg)' block to cover all assignments.
"""

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

sumit-bose commented:
"""
Ok, I agree that the readability is not improved a lot by my suggestion.

I run some manual tests and didn't found issues and the original issue is solved by the patch as well. CI went well as well http://vm/logs/job/88/98/summary.html.

Unfortunately I came across a case which might need some improvement. For the pure mpg case where SYSDB_GRNAM_MPG_FILTER is the right filter the new version runs the same ldb_search two times (if I see it correctly). Can you add some checks so that the second ldb_search is skipped for this case?
"""

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][-Changes requested] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

Label: -Changes requested

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
@sumit-bose, thanks for the review.

I have updated the patches according to your suggestions.
"""

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][synchronized] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: synchronized

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

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

sumit-bose commented:
"""
Thanks, my manual testing was successful and CI still passes http://vm/logs/job/89/03/summary.html and Coverity didn't found a new issue either, so ACK,

"""

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

[sssd PR#464][+Accepted] SYSDB: Properly handle name/gid override when using domain resolution order

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

Label: +Accepted

[sssd PR#464][comment] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

fidencio commented:
"""
master:
 cf4f5e0
"""

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

[sssd PR#464][+Pushed] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

Label: +Pushed

[sssd PR#464][-Accepted] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order

Label: -Accepted

[sssd PR#464][closed] SYSDB: Properly handle name/gid override when using domain resolution order

[fidencio]

URL: https://github.com/SSSD/sssd/pull/464
Author: fidencio
 Title: #464: SYSDB: Properly handle name/gid override when using domain resolution order
Action: closed

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