[PATCH] NSS: Print FQDN for groups with mixed domain membership

[Jakub Hrozek]

Hi,

The first patch adds a unit test for getgrnam. I wanted to make sure we
don't break anything this close to the 1.11.2 release.

The second patch is a workaround until
https://fedorahosted.org/sssd/ticket/2129 is fixed properly.

Consider a group entry such as:
 cn: subgroup@subdom
 ghost: someuser
 ghost: anotheruser@subdom

Currently in order to print all group members as FQDN (which is the default
for AD provider), the code needs to iterate over the ghost attributes and
parse them into (name,domain) and optionally re-add the domain.

The proper fix would be to store always just the FQDN in the hardcoded
form of user@domain

[Sumit Bose]

On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  Hi,
>  
>  The first patch adds a unit test for getgrnam. I wanted to make sure we
>  don't break anything this close to the 1.11.2 release.
>  
>  The second patch is a workaround until
>  https://fedorahosted.org/sssd/ticket/2129 is fixed properly.
>  
>  Consider a group entry such as:
>   cn: subgroup@subdom
>   ghost: someuser
>   ghost: anotheruser@subdom
>  
>  Currently in order to print all group members as FQDN (which is the default
>  for AD provider), the code needs to iterate over the ghost attributes and
>  parse them into (name,domain) and optionally re-add the domain.
>  
>  The proper fix would be to store always just the FQDN in the hardcoded
>  form of user@domain 
sorry, but nss-srv-tests fails for me after applying the patch to master.
See http://paste.fedoraproject.org/50054/13829949/ for the short and
http://paste.fedoraproject.org/50056/29949861/ for the full debug
output.

bye,
Sumit

[Jakub Hrozek]

On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  > Hi,
>  > 
>  > The first patch adds a unit test for getgrnam. I wanted to make sure we
>  > don't break anything this close to the 1.11.2 release.
>  > 
>  > The second patch is a workaround until
>  > https://fedorahosted.org/sssd/ticket/2129 is fixed properly.
>  > 
>  > Consider a group entry such as:
>  >  cn: subgroup@subdom
>  >  ghost: someuser
>  >  ghost: anotheruser@subdom
>  > 
>  > Currently in order to print all group members as FQDN (which is the default
>  > for AD provider), the code needs to iterate over the ghost attributes and
>  > parse them into (name,domain) and optionally re-add the domain.
>  > 
>  > The proper fix would be to store always just the FQDN in the hardcoded
>  > form of user@domain
>  
>  sorry, but nss-srv-tests fails for me after applying the patch to master.
>  See http://paste.fedoraproject.org/50054/13829949/ for the short and
>  http://paste.fedoraproject.org/50056/29949861/ for the full debug
>  output.
>  
>  bye,
>  Sumit 
I should have explicitly said that these patches must be applied on top
of those in "[PATCH] ad: support cross domain membership", do they still
fail for you?

[Sumit Bose]

On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  > > Hi,
>  > > 
>  > > The first patch adds a unit test for getgrnam. I wanted to make sure we
>  > > don't break anything this close to the 1.11.2 release.
>  > > 
>  > > The second patch is a workaround until
>  > > https://fedorahosted.org/sssd/ticket/2129 is fixed properly.
>  > > 
>  > > Consider a group entry such as:
>  > >  cn: subgroup@subdom
>  > >  ghost: someuser
>  > >  ghost: anotheruser@subdom
>  > > 
>  > > Currently in order to print all group members as FQDN (which is the default
>  > > for AD provider), the code needs to iterate over the ghost attributes and
>  > > parse them into (name,domain) and optionally re-add the domain.
>  > > 
>  > > The proper fix would be to store always just the FQDN in the hardcoded
>  > > form of user@domain
>  > 
>  > sorry, but nss-srv-tests fails for me after applying the patch to master.
>  > See http://paste.fedoraproject.org/50054/13829949/ for the short and
>  > http://paste.fedoraproject.org/50056/29949861/ for the full debug
>  > output.
>  > 
>  > bye,
>  > Sumit
>  
>  I should have explicitly said that these patches must be applied on top
>  of those in "[PATCH] ad: support cross domain membership", do they still
>  fail for you? 
I'm sorry but yes. Am I missing any other patch? This is the top of the
branch I test with on a 32bit system:

d703220 NSS: Print FQDN for groups with mixed domain membership
de24e2d TEST: Test getgrnam with emphasis on members
bdd6b7e NSS: Fix parenthesis
267adcb LDAP: Check all search bases during nested group processing
b0fc582 nested groups: pick correct domain for cache lookups
0335a23 sdap_fill_memberships: pick correct domain for every member
1e3112f ghosts: pick correct domain for every member
8c6d1a4 sdap: add sdap_domain_get_by_dn()
c6360d8 sdap: store base dn in sdap_domain
3226d5b ad: shortcut if possible during get object by ID or SID
da34cf4 ad: destroy ptasks when subdomain is removed
44e8e96 ipa: destroy cleanup task when subdomain is removed

bye,
Sumit
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
>  On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  > > > Hi,
>  > > > 
>  > > > The first patch adds a unit test for getgrnam. I wanted to make sure we
>  > > > don't break anything this close to the 1.11.2 release.
>  > > > 
>  > > > The second patch is a workaround until
>  > > > https://fedorahosted.org/sssd/ticket/2129 is fixed properly.
>  > > > 
>  > > > Consider a group entry such as:
>  > > >  cn: subgroup@subdom
>  > > >  ghost: someuser
>  > > >  ghost: anotheruser@subdom
>  > > > 
>  > > > Currently in order to print all group members as FQDN (which is the
> default
>  > > > for AD provider), the code needs to iterate over the ghost attributes and
>  > > > parse them into (name,domain) and optionally re-add the domain.
>  > > > 
>  > > > The proper fix would be to store always just the FQDN in the hardcoded
>  > > > form of user@domain
>  > > 
>  > > sorry, but nss-srv-tests fails for me after applying the patch to master.
>  > > See http://paste.fedoraproject.org/50054/13829949/ for the short and
>  > > http://paste.fedoraproject.org/50056/29949861/ for the full debug
>  > > output.
>  > > 
>  > > bye,
>  > > Sumit
>  > 
>  > I should have explicitly said that these patches must be applied on top
>  > of those in "[PATCH] ad: support cross domain membership", do they still
>  > fail for you?
>  
>  I'm sorry but yes. Am I missing any other patch? This is the top of the
>  branch I test with on a 32bit system:
>  
>  d703220 NSS: Print FQDN for groups with mixed domain membership
>  de24e2d TEST: Test getgrnam with emphasis on members
>  bdd6b7e NSS: Fix parenthesis
>  267adcb LDAP: Check all search bases during nested group processing
>  b0fc582 nested groups: pick correct domain for cache lookups
>  0335a23 sdap_fill_memberships: pick correct domain for every member
>  1e3112f ghosts: pick correct domain for every member
>  8c6d1a4 sdap: add sdap_domain_get_by_dn()
>  c6360d8 sdap: store base dn in sdap_domain
>  3226d5b ad: shortcut if possible during get object by ID or SID
>  da34cf4 ad: destroy ptasks when subdomain is removed
>  44e8e96 ipa: destroy cleanup task when subdomain is removed 
Ah, I will test on a 32bit system, the code works for me on 64bits.

[Jakub Hrozek]

On Tue, Oct 29, 2013 at 10:12:34AM +0100, Jakub Hrozek wrote:
>  On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
>  > On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  > > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  > > > > Hi,
>  > > > > 
>  > > > > The first patch adds a unit test for getgrnam. I wanted to make sure
> we
>  > > > > don't break anything this close to the 1.11.2 release.
>  > > > > 
>  > > > > The second patch is a workaround until
>  > > > > https://fedorahosted.org/sssd/ticket/2129 is fixed properly.
>  > > > > 
>  > > > > Consider a group entry such as:
>  > > > >  cn: subgroup@subdom
>  > > > >  ghost: someuser
>  > > > >  ghost: anotheruser@subdom
>  > > > > 
>  > > > > Currently in order to print all group members as FQDN (which is the
> default
>  > > > > for AD provider), the code needs to iterate over the ghost attributes
> and
>  > > > > parse them into (name,domain) and optionally re-add the domain.
>  > > > > 
>  > > > > The proper fix would be to store always just the FQDN in the
> hardcoded
>  > > > > form of user@domain
>  > > > 
>  > > > sorry, but nss-srv-tests fails for me after applying the patch to master.
>  > > > See http://paste.fedoraproject.org/50054/13829949/ for the short and
>  > > > http://paste.fedoraproject.org/50056/29949861/ for the full debug
>  > > > output.
>  > > > 
>  > > > bye,
>  > > > Sumit
>  > > 
>  > > I should have explicitly said that these patches must be applied on top
>  > > of those in "[PATCH] ad: support cross domain membership", do they
> still
>  > > fail for you?
>  > 
>  > I'm sorry but yes. Am I missing any other patch? This is the top of the
>  > branch I test with on a 32bit system:
>  > 
>  > d703220 NSS: Print FQDN for groups with mixed domain membership
>  > de24e2d TEST: Test getgrnam with emphasis on members
>  > bdd6b7e NSS: Fix parenthesis
>  > 267adcb LDAP: Check all search bases during nested group processing
>  > b0fc582 nested groups: pick correct domain for cache lookups
>  > 0335a23 sdap_fill_memberships: pick correct domain for every member
>  > 1e3112f ghosts: pick correct domain for every member
>  > 8c6d1a4 sdap: add sdap_domain_get_by_dn()
>  > c6360d8 sdap: store base dn in sdap_domain
>  > 3226d5b ad: shortcut if possible during get object by ID or SID
>  > da34cf4 ad: destroy ptasks when subdomain is removed
>  > 44e8e96 ipa: destroy cleanup task when subdomain is removed
>  
>  Ah, I will test on a 32bit system, the code works for me on 64bits. 
You were right, one test (that checked gr_mem pointer for NULL for empty
groups) relied on undefined behaviour. I simply removed the code,
it was bogus, we are able to detect empty groups in the packet by the
number of members, which is also what the sss_client does.

[Sumit Bose]

On Tue, Oct 29, 2013 at 11:41:52AM +0100, Jakub Hrozek wrote:
>  On Tue, Oct 29, 2013 at 10:12:34AM +0100, Jakub Hrozek wrote:
>  > On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
>  > > On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  > > > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > > > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  > > > > > Hi,
>  > > > > > 
>  > > > > > The first patch adds a unit test for getgrnam. I wanted to make
> sure we
>  > > > > > don't break anything this close to the 1.11.2 release.
>  > > > > > 
>  > > > > > The second patch is a workaround until
>  > > > > > https://fedorahosted.org/sssd/ticket/2129 is fixed properly.
>  > > > > > 
>  > > > > > Consider a group entry such as:
>  > > > > >  cn: subgroup@subdom
>  > > > > >  ghost: someuser
>  > > > > >  ghost: anotheruser@subdom
>  > > > > > 
>  > > > > > Currently in order to print all group members as FQDN (which is
> the default
>  > > > > > for AD provider), the code needs to iterate over the ghost
> attributes and
>  > > > > > parse them into (name,domain) and optionally re-add the domain.
>  > > > > > 
>  > > > > > The proper fix would be to store always just the FQDN in the
> hardcoded
>  > > > > > form of user@domain
>  > > > > 
>  > > > > sorry, but nss-srv-tests fails for me after applying the patch to
> master.
>  > > > > See http://paste.fedoraproject.org/50054/13829949/ for the short and
>  > > > > http://paste.fedoraproject.org/50056/29949861/ for the full debug
>  > > > > output.
>  > > > > 
>  > > > > bye,
>  > > > > Sumit
>  > > > 
>  > > > I should have explicitly said that these patches must be applied on top
>  > > > of those in "[PATCH] ad: support cross domain membership", do
> they still
>  > > > fail for you?
>  > > 
>  > > I'm sorry but yes. Am I missing any other patch? This is the top of the
>  > > branch I test with on a 32bit system:
>  > > 
>  > > d703220 NSS: Print FQDN for groups with mixed domain membership
>  > > de24e2d TEST: Test getgrnam with emphasis on members
>  > > bdd6b7e NSS: Fix parenthesis
>  > > 267adcb LDAP: Check all search bases during nested group processing
>  > > b0fc582 nested groups: pick correct domain for cache lookups
>  > > 0335a23 sdap_fill_memberships: pick correct domain for every member
>  > > 1e3112f ghosts: pick correct domain for every member
>  > > 8c6d1a4 sdap: add sdap_domain_get_by_dn()
>  > > c6360d8 sdap: store base dn in sdap_domain
>  > > 3226d5b ad: shortcut if possible during get object by ID or SID
>  > > da34cf4 ad: destroy ptasks when subdomain is removed
>  > > 44e8e96 ipa: destroy cleanup task when subdomain is removed
>  > 
>  > Ah, I will test on a 32bit system, the code works for me on 64bits.
>  
>  You were right, one test (that checked gr_mem pointer for NULL for empty
>  groups) relied on undefined behaviour. I simply removed the code,
>  it was bogus, we are able to detect empty groups in the packet by the
>  number of members, which is also what the sss_client does. 
Thank you, now the unit test do not fail anymore. Nevertheless I think
it would be good to remove this undefined behaviour and properly
initialized the packet. If you agree would you mind to open a ticket?

The patches look good so ACK. I only have a minor nitpick:

>  +
>  +    ret = sss_parse_name(mem_ctx, group_dom->names, member, &domname,
> &username);
>  +    if (ret != EOK) {
>  +        DEBUG(SSSDBG_MINOR_FAILURE, ("Could not parse [%s] into "
>  +              "name-value components.\n", member)); 
I think this should read name-domain components ?

>  +        return ret;
>  +    }
>  + 
bye,
Sumit

[Jakub Hrozek]

On Tue, Oct 29, 2013 at 12:37:11PM +0100, Sumit Bose wrote:
>  On Tue, Oct 29, 2013 at 11:41:52AM +0100, Jakub Hrozek wrote:
>  > On Tue, Oct 29, 2013 at 10:12:34AM +0100, Jakub Hrozek wrote:
>  > > On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
>  > > > On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  > > > > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > > > > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  > > > > > > Hi,
>  > > > > > > 
>  > > > > > > The first patch adds a unit test for getgrnam. I wanted to
> make sure we
>  > > > > > > don't break anything this close to the 1.11.2 release.
>  > > > > > > 
>  > > > > > > The second patch is a workaround until
>  > > > > > > https://fedorahosted.org/sssd/ticket/2129 is fixed
> properly.
>  > > > > > > 
>  > > > > > > Consider a group entry such as:
>  > > > > > >  cn: subgroup@subdom
>  > > > > > >  ghost: someuser
>  > > > > > >  ghost: anotheruser@subdom
>  > > > > > > 
>  > > > > > > Currently in order to print all group members as FQDN
> (which is the default
>  > > > > > > for AD provider), the code needs to iterate over the ghost
> attributes and
>  > > > > > > parse them into (name,domain) and optionally re-add the
> domain.
>  > > > > > > 
>  > > > > > > The proper fix would be to store always just the FQDN in
> the hardcoded
>  > > > > > > form of user@domain
>  > > > > > 
>  > > > > > sorry, but nss-srv-tests fails for me after applying the patch
> to master.
>  > > > > > See http://paste.fedoraproject.org/50054/13829949/ for the short
> and
>  > > > > > http://paste.fedoraproject.org/50056/29949861/ for the full
> debug
>  > > > > > output.
>  > > > > > 
>  > > > > > bye,
>  > > > > > Sumit
>  > > > > 
>  > > > > I should have explicitly said that these patches must be applied on
> top
>  > > > > of those in "[PATCH] ad: support cross domain membership",
> do they still
>  > > > > fail for you?
>  > > > 
>  > > > I'm sorry but yes. Am I missing any other patch? This is the top of
> the
>  > > > branch I test with on a 32bit system:
>  > > > 
>  > > > d703220 NSS: Print FQDN for groups with mixed domain membership
>  > > > de24e2d TEST: Test getgrnam with emphasis on members
>  > > > bdd6b7e NSS: Fix parenthesis
>  > > > 267adcb LDAP: Check all search bases during nested group processing
>  > > > b0fc582 nested groups: pick correct domain for cache lookups
>  > > > 0335a23 sdap_fill_memberships: pick correct domain for every member
>  > > > 1e3112f ghosts: pick correct domain for every member
>  > > > 8c6d1a4 sdap: add sdap_domain_get_by_dn()
>  > > > c6360d8 sdap: store base dn in sdap_domain
>  > > > 3226d5b ad: shortcut if possible during get object by ID or SID
>  > > > da34cf4 ad: destroy ptasks when subdomain is removed
>  > > > 44e8e96 ipa: destroy cleanup task when subdomain is removed
>  > > 
>  > > Ah, I will test on a 32bit system, the code works for me on 64bits.
>  > 
>  > You were right, one test (that checked gr_mem pointer for NULL for empty
>  > groups) relied on undefined behaviour. I simply removed the code,
>  > it was bogus, we are able to detect empty groups in the packet by the
>  > number of members, which is also what the sss_client does.
>  
>  Thank you, now the unit test do not fail anymore. Nevertheless I think
>  it would be good to remove this undefined behaviour and properly
>  initialized the packet. If you agree would you mind to open a ticket?
>  
>  The patches look good so ACK. I only have a minor nitpick: 
Sorry, I didn't express the problem clearly.

The packet *is* initialized correctly[1], just the code in the unit test
that parses the packet into struct group didn't initialize gr_mem to NULL
when it processed a packet that described an empty group didn't work the
same as in the client code.

If you prefer, I can amend the unit test parsing so that it behaves the
same as the client code..

[Jakub Hrozek]

On Tue, Oct 29, 2013 at 02:46:22PM +0100, Jakub Hrozek wrote:
>  On Tue, Oct 29, 2013 at 12:37:11PM +0100, Sumit Bose wrote:
>  > On Tue, Oct 29, 2013 at 11:41:52AM +0100, Jakub Hrozek wrote:
>  > > On Tue, Oct 29, 2013 at 10:12:34AM +0100, Jakub Hrozek wrote:
>  > > > On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
>  > > > > On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  > > > > > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > > > > > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek
> wrote:
>  > > > > > > > Hi,
>  > > > > > > > 
>  > > > > > > > The first patch adds a unit test for getgrnam. I
> wanted to make sure we
>  > > > > > > > don't break anything this close to the 1.11.2
> release.
>  > > > > > > > 
>  > > > > > > > The second patch is a workaround until
>  > > > > > > > https://fedorahosted.org/sssd/ticket/2129 is fixed
> properly.
>  > > > > > > > 
>  > > > > > > > Consider a group entry such as:
>  > > > > > > >  cn: subgroup@subdom
>  > > > > > > >  ghost: someuser
>  > > > > > > >  ghost: anotheruser@subdom
>  > > > > > > > 
>  > > > > > > > Currently in order to print all group members as FQDN
> (which is the default
>  > > > > > > > for AD provider), the code needs to iterate over the
> ghost attributes and
>  > > > > > > > parse them into (name,domain) and optionally re-add
> the domain.
>  > > > > > > > 
>  > > > > > > > The proper fix would be to store always just the FQDN
> in the hardcoded
>  > > > > > > > form of user@domain
>  > > > > > > 
>  > > > > > > sorry, but nss-srv-tests fails for me after applying the
> patch to master.
>  > > > > > > See http://paste.fedoraproject.org/50054/13829949/ for the
> short and
>  > > > > > > http://paste.fedoraproject.org/50056/29949861/ for the full
> debug
>  > > > > > > output.
>  > > > > > > 
>  > > > > > > bye,
>  > > > > > > Sumit
>  > > > > > 
>  > > > > > I should have explicitly said that these patches must be applied
> on top
>  > > > > > of those in "[PATCH] ad: support cross domain
> membership", do they still
>  > > > > > fail for you?
>  > > > > 
>  > > > > I'm sorry but yes. Am I missing any other patch? This is the top
> of the
>  > > > > branch I test with on a 32bit system:
>  > > > > 
>  > > > > d703220 NSS: Print FQDN for groups with mixed domain membership
>  > > > > de24e2d TEST: Test getgrnam with emphasis on members
>  > > > > bdd6b7e NSS: Fix parenthesis
>  > > > > 267adcb LDAP: Check all search bases during nested group processing
>  > > > > b0fc582 nested groups: pick correct domain for cache lookups
>  > > > > 0335a23 sdap_fill_memberships: pick correct domain for every member
>  > > > > 1e3112f ghosts: pick correct domain for every member
>  > > > > 8c6d1a4 sdap: add sdap_domain_get_by_dn()
>  > > > > c6360d8 sdap: store base dn in sdap_domain
>  > > > > 3226d5b ad: shortcut if possible during get object by ID or SID
>  > > > > da34cf4 ad: destroy ptasks when subdomain is removed
>  > > > > 44e8e96 ipa: destroy cleanup task when subdomain is removed
>  > > > 
>  > > > Ah, I will test on a 32bit system, the code works for me on 64bits.
>  > > 
>  > > You were right, one test (that checked gr_mem pointer for NULL for empty
>  > > groups) relied on undefined behaviour. I simply removed the code,
>  > > it was bogus, we are able to detect empty groups in the packet by the
>  > > number of members, which is also what the sss_client does.
>  > 
>  > Thank you, now the unit test do not fail anymore. Nevertheless I think
>  > it would be good to remove this undefined behaviour and properly
>  > initialized the packet. If you agree would you mind to open a ticket?
>  > 
>  > The patches look good so ACK. I only have a minor nitpick:
>  
>  Sorry, I didn't express the problem clearly.
>  
>  The packet *is* initialized correctly[1], just the code in the unit test
>  that parses the packet into struct group didn't initialize gr_mem to NULL
>  when it processed a packet that described an empty group didn't work the
>  same as in the client code.
>  
>  If you prefer, I can amend the unit test parsing so that it behaves the
>  same as the client code.. 
grr, I forgot to add the footnote -- I actually verified that with a
simple C program. getgr* and getpw* result is correct.

[Sumit Bose]

On Tue, Oct 29, 2013 at 02:46:22PM +0100, Jakub Hrozek wrote:
>  On Tue, Oct 29, 2013 at 12:37:11PM +0100, Sumit Bose wrote:
>  > On Tue, Oct 29, 2013 at 11:41:52AM +0100, Jakub Hrozek wrote:
>  > > On Tue, Oct 29, 2013 at 10:12:34AM +0100, Jakub Hrozek wrote:
>  > > > On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
>  > > > > On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  > > > > > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > > > > > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek
> wrote:
>  > > > > > > > Hi,
>  > > > > > > > 
>  > > > > > > > The first patch adds a unit test for getgrnam. I
> wanted to make sure we
>  > > > > > > > don't break anything this close to the 1.11.2
> release.
>  > > > > > > > 
>  > > > > > > > The second patch is a workaround until
>  > > > > > > > https://fedorahosted.org/sssd/ticket/2129 is fixed
> properly.
>  > > > > > > > 
>  > > > > > > > Consider a group entry such as:
>  > > > > > > >  cn: subgroup@subdom
>  > > > > > > >  ghost: someuser
>  > > > > > > >  ghost: anotheruser@subdom
>  > > > > > > > 
>  > > > > > > > Currently in order to print all group members as FQDN
> (which is the default
>  > > > > > > > for AD provider), the code needs to iterate over the
> ghost attributes and
>  > > > > > > > parse them into (name,domain) and optionally re-add
> the domain.
>  > > > > > > > 
>  > > > > > > > The proper fix would be to store always just the FQDN
> in the hardcoded
>  > > > > > > > form of user@domain
>  > > > > > > 
>  > > > > > > sorry, but nss-srv-tests fails for me after applying the
> patch to master.
>  > > > > > > See http://paste.fedoraproject.org/50054/13829949/ for the
> short and
>  > > > > > > http://paste.fedoraproject.org/50056/29949861/ for the full
> debug
>  > > > > > > output.
>  > > > > > > 
>  > > > > > > bye,
>  > > > > > > Sumit
>  > > > > > 
>  > > > > > I should have explicitly said that these patches must be applied
> on top
>  > > > > > of those in "[PATCH] ad: support cross domain
> membership", do they still
>  > > > > > fail for you?
>  > > > > 
>  > > > > I'm sorry but yes. Am I missing any other patch? This is the top
> of the
>  > > > > branch I test with on a 32bit system:
>  > > > > 
>  > > > > d703220 NSS: Print FQDN for groups with mixed domain membership
>  > > > > de24e2d TEST: Test getgrnam with emphasis on members
>  > > > > bdd6b7e NSS: Fix parenthesis
>  > > > > 267adcb LDAP: Check all search bases during nested group processing
>  > > > > b0fc582 nested groups: pick correct domain for cache lookups
>  > > > > 0335a23 sdap_fill_memberships: pick correct domain for every member
>  > > > > 1e3112f ghosts: pick correct domain for every member
>  > > > > 8c6d1a4 sdap: add sdap_domain_get_by_dn()
>  > > > > c6360d8 sdap: store base dn in sdap_domain
>  > > > > 3226d5b ad: shortcut if possible during get object by ID or SID
>  > > > > da34cf4 ad: destroy ptasks when subdomain is removed
>  > > > > 44e8e96 ipa: destroy cleanup task when subdomain is removed
>  > > > 
>  > > > Ah, I will test on a 32bit system, the code works for me on 64bits.
>  > > 
>  > > You were right, one test (that checked gr_mem pointer for NULL for empty
>  > > groups) relied on undefined behaviour. I simply removed the code,
>  > > it was bogus, we are able to detect empty groups in the packet by the
>  > > number of members, which is also what the sss_client does.
>  > 
>  > Thank you, now the unit test do not fail anymore. Nevertheless I think
>  > it would be good to remove this undefined behaviour and properly
>  > initialized the packet. If you agree would you mind to open a ticket?
>  > 
>  > The patches look good so ACK. I only have a minor nitpick:
>  
>  Sorry, I didn't express the problem clearly.
>  
>  The packet *is* initialized correctly[1], just the code in the unit test
>  that parses the packet into struct group didn't initialize gr_mem to NULL
>  when it processed a packet that described an empty group didn't work the
>  same as in the client code. 
ah, I see, sorry for the noise.

>  
>  If you prefer, I can amend the unit test parsing so that it behaves the
>  same as the client code.. 
no, I think it is not needed.

bye,
Sumit
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Tue, Oct 29, 2013 at 12:37:11PM +0100, Sumit Bose wrote:
>  On Tue, Oct 29, 2013 at 11:41:52AM +0100, Jakub Hrozek wrote:
>  > On Tue, Oct 29, 2013 at 10:12:34AM +0100, Jakub Hrozek wrote:
>  > > On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
>  > > > On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
>  > > > > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
>  > > > > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek wrote:
>  > > > > > > Hi,
>  > > > > > > 
>  > > > > > > The first patch adds a unit test for getgrnam. I wanted to
> make sure we
>  > > > > > > don't break anything this close to the 1.11.2 release.
>  > > > > > > 
>  > > > > > > The second patch is a workaround until
>  > > > > > > https://fedorahosted.org/sssd/ticket/2129 is fixed
> properly.
>  > > > > > > 
>  > > > > > > Consider a group entry such as:
>  > > > > > >  cn: subgroup@subdom
>  > > > > > >  ghost: someuser
>  > > > > > >  ghost: anotheruser@subdom
>  > > > > > > 
>  > > > > > > Currently in order to print all group members as FQDN
> (which is the default
>  > > > > > > for AD provider), the code needs to iterate over the ghost
> attributes and
>  > > > > > > parse them into (name,domain) and optionally re-add the
> domain.
>  > > > > > > 
>  > > > > > > The proper fix would be to store always just the FQDN in
> the hardcoded
>  > > > > > > form of user@domain
>  > > > > > 
>  > > > > > sorry, but nss-srv-tests fails for me after applying the patch
> to master.
>  > > > > > See http://paste.fedoraproject.org/50054/13829949/ for the short
> and
>  > > > > > http://paste.fedoraproject.org/50056/29949861/ for the full
> debug
>  > > > > > output.
>  > > > > > 
>  > > > > > bye,
>  > > > > > Sumit
>  > > > > 
>  > > > > I should have explicitly said that these patches must be applied on
> top
>  > > > > of those in "[PATCH] ad: support cross domain membership",
> do they still
>  > > > > fail for you?
>  > > > 
>  > > > I'm sorry but yes. Am I missing any other patch? This is the top of
> the
>  > > > branch I test with on a 32bit system:
>  > > > 
>  > > > d703220 NSS: Print FQDN for groups with mixed domain membership
>  > > > de24e2d TEST: Test getgrnam with emphasis on members
>  > > > bdd6b7e NSS: Fix parenthesis
>  > > > 267adcb LDAP: Check all search bases during nested group processing
>  > > > b0fc582 nested groups: pick correct domain for cache lookups
>  > > > 0335a23 sdap_fill_memberships: pick correct domain for every member
>  > > > 1e3112f ghosts: pick correct domain for every member
>  > > > 8c6d1a4 sdap: add sdap_domain_get_by_dn()
>  > > > c6360d8 sdap: store base dn in sdap_domain
>  > > > 3226d5b ad: shortcut if possible during get object by ID or SID
>  > > > da34cf4 ad: destroy ptasks when subdomain is removed
>  > > > 44e8e96 ipa: destroy cleanup task when subdomain is removed
>  > > 
>  > > Ah, I will test on a 32bit system, the code works for me on 64bits.
>  > 
>  > You were right, one test (that checked gr_mem pointer for NULL for empty
>  > groups) relied on undefined behaviour. I simply removed the code,
>  > it was bogus, we are able to detect empty groups in the packet by the
>  > number of members, which is also what the sss_client does.
>  
>  Thank you, now the unit test do not fail anymore. Nevertheless I think
>  it would be good to remove this undefined behaviour and properly
>  initialized the packet. If you agree would you mind to open a ticket?
>  
>  The patches look good so ACK. I only have a minor nitpick: 
Pushed to master and sssd-1-11