[PATCH] Retain group members in setups where id_provider=ad and ignore_group_members=True

[Jakub Hrozek]

Hi,

the attached patch fixes https://fedorahosted.org/sssd/ticket/2646. See 
the commit message for the problem description. The bug was found by 
Lukas (thanks!), I just wrote the patch.

I was not really happy with some parts of the patch myself, but I 
couldn't find a better way to fix the ticket. Here are some parts of the 
patch that I think need careful review:
- is_fake_tgs_group() uses heuristics based on the AD schema, RDN value 
and and POSIX/non-POSIX flag. Is that OK or too much of a hack?
- sdap_get_tokengroup_members() plays a bit of a trick on the memory 
hierarchy. I wanted the API of this function to only return a single 
member (a ldb_message_element) but at the same time, the cache search 
returns only the top-level ldb_message and the first element as talloc 
pointers. So the functions returns the element, which points to the 
message, but the element owns the message. I checked with valgrind that 
there are no memory errors. I really want to avoid copying all the 
members, because that number can go to thousands. The only alternative 
that comes to mind is to run a second cache search only for the member 
attribute, then the member would be a talloc pointer -- but this way 
seems to rely a bit on ldb implementation which I don't like either.
- sdap_save_grpmem() and sdap_save_groups() now use a saved_ldap_group 
structure. This means we now need to do an extra allocation per group, 
but the number of groups is usually not that big (max. hundreds, 
definitely not thousands like members).

Thanks for review!

[Sumit Bose]

On Fri, May 08, 2015 at 03:24:29PM +0200, Jakub Hrozek wrote:
>  Hi,
>  
>  the attached patch fixes https://fedorahosted.org/sssd/ticket/2646. See the
>  commit message for the problem description. The bug was found by Lukas
>  (thanks!), I just wrote the patch. 
Hi Jakub,

thank you for the patch, I haven't checked it closely yet, because I
would like to discuss some alternative approaches first.

The scheme to save the groups with the SID in the DN was introduced to
speed up initgroups request when id-mapping is used. Iirc this path is
currently only used for the parent domain if id-mapping is enabled. I do
not have any statistics but I would expect that there are already many
cases where this code path is not used at all either because POSIX
attributes in AD are used or the user is from a subdomain. This makes me
wonder if we cannot pay the price and always lookup the group and skip
the SID-as-name path completely? Maybe only if ignore_group_members=True
is set?

As a second alternative I was thinking if it is necessary to rename the
group at all? Historically there were some sysdb calls which generate
the DN of a group object for lookups with the aim to speed things up
(assuming a base search is faster then an indexed sub-tree search).
With all the short-name/fully-qualified-name/id-view changes I wonder if
this is used for lookups anymore. If not, I think there is no need to
rename the group at all. If there are still such lookups (iirc Michal is
currently working on the sysdb code, maybe he knows?) we might want to
replace them by proper searches and eliminate the need to rename the
group. Here I do not have any data either but I would expect that there
is no big difference in ldb between a base search and an indexed
sub-tree search.



bye,
Sumit

>  
>  I was not really happy with some parts of the patch myself, but I couldn't
>  find a better way to fix the ticket. Here are some parts of the patch that I
>  think need careful review:
>  - is_fake_tgs_group() uses heuristics based on the AD schema, RDN value and
>  and POSIX/non-POSIX flag. Is that OK or too much of a hack?
>  - sdap_get_tokengroup_members() plays a bit of a trick on the memory
>  hierarchy. I wanted the API of this function to only return a single member
>  (a ldb_message_element) but at the same time, the cache search returns only
>  the top-level ldb_message and the first element as talloc pointers. So the
>  functions returns the element, which points to the message, but the element
>  owns the message. I checked with valgrind that there are no memory errors. I
>  really want to avoid copying all the members, because that number can go to
>  thousands. The only alternative that comes to mind is to run a second cache
>  search only for the member attribute, then the member would be a talloc
>  pointer -- but this way seems to rely a bit on ldb implementation which I
>  don't like either.
>  - sdap_save_grpmem() and sdap_save_groups() now use a saved_ldap_group
>  structure. This means we now need to do an extra allocation per group, but
>  the number of groups is usually not that big (max. hundreds, definitely not
>  thousands like members).
>  
>  Thanks for review!

[Jakub Hrozek]

On Mon, May 11, 2015 at 11:31:43AM +0200, Sumit Bose wrote:
>  On Fri, May 08, 2015 at 03:24:29PM +0200, Jakub Hrozek wrote:
>  > Hi,
>  > 
>  > the attached patch fixes https://fedorahosted.org/sssd/ticket/2646. See the
>  > commit message for the problem description. The bug was found by Lukas
>  > (thanks!), I just wrote the patch.
>  
>  Hi Jakub,
>  
>  thank you for the patch, I haven't checked it closely yet, because I
>  would like to discuss some alternative approaches first.
>  
>  The scheme to save the groups with the SID in the DN was introduced to
>  speed up initgroups request when id-mapping is used. Iirc this path is
>  currently only used for the parent domain if id-mapping is enabled. I do
>  not have any statistics but I would expect that there are already many
>  cases where this code path is not used at all either because POSIX
>  attributes in AD are used or the user is from a subdomain. This makes me
>  wonder if we cannot pay the price and always lookup the group and skip
>  the SID-as-name path completely? 
This could work, but then there would be some performance impact.

Currently the initgroups operation with ID mapping AD domain is precisely
one LDAP lookup. With this change, it would turn into N LDAP lookups for
N groups (if none of the groups is cached) I would prefer not to decrease
performance of the default setup for a corner case (even if often used)
like ignore_group_members.

>  Maybe only if ignore_group_members=True
>  is set? 
Yes, perhaps..maybe as a short-term before we implement what you propose
below?

>  
>  As a second alternative I was thinking if it is necessary to rename the
>  group at all? Historically there were some sysdb calls which generate
>  the DN of a group object for lookups with the aim to speed things up
>  (assuming a base search is faster then an indexed sub-tree search).
>  With all the short-name/fully-qualified-name/id-view changes I wonder if
>  this is used for lookups anymore. If not, I think there is no need to
>  rename the group at all. If there are still such lookups (iirc Michal is
>  currently working on the sysdb code, maybe he knows?) we might want to
>  replace them by proper searches and eliminate the need to rename the
>  group. Here I do not have any data either but I would expect that there
>  is no big difference in ldb between a base search and an indexed
>  sub-tree search. 
I like with this direction more. I was even thinking we should not use name
in RDN at all with the AD domain and always use objectSID. Then we would
also have fewer problems changing numerical IDs..

My only concern is that this is an invasive solution that might bring
bugs. Currently there is around 40 occurences of LDB_SCOPE_BASE in the
code. At least those in sysdb_views.c and sysdb_asq_search(),
sysdb_initgroups() and sysdb_initgroups_with_views() look like they
would need to be converted. And currently the ignore_group_members bug
is hitting users..

[Jakub Hrozek]

On Mon, May 11, 2015 at 12:12:00PM +0200, Jakub Hrozek wrote:
>  On Mon, May 11, 2015 at 11:31:43AM +0200, Sumit Bose wrote:
>  > On Fri, May 08, 2015 at 03:24:29PM +0200, Jakub Hrozek wrote:
>  > > Hi,
>  > > 
>  > > the attached patch fixes https://fedorahosted.org/sssd/ticket/2646. See the
>  > > commit message for the problem description. The bug was found by Lukas
>  > > (thanks!), I just wrote the patch.
>  > 
>  > Hi Jakub,
>  > 
>  > thank you for the patch, I haven't checked it closely yet, because I
>  > would like to discuss some alternative approaches first.
>  > 
>  > The scheme to save the groups with the SID in the DN was introduced to
>  > speed up initgroups request when id-mapping is used. Iirc this path is
>  > currently only used for the parent domain if id-mapping is enabled. I do
>  > not have any statistics but I would expect that there are already many
>  > cases where this code path is not used at all either because POSIX
>  > attributes in AD are used or the user is from a subdomain. This makes me
>  > wonder if we cannot pay the price and always lookup the group and skip
>  > the SID-as-name path completely?
>  
>  This could work, but then there would be some performance impact.
>  
>  Currently the initgroups operation with ID mapping AD domain is precisely
>  one LDAP lookup. With this change, it would turn into N LDAP lookups for
>  N groups (if none of the groups is cached) I would prefer not to decrease
>  performance of the default setup for a corner case (even if often used)
>  like ignore_group_members.
>  
>  > Maybe only if ignore_group_members=True
>  > is set?
>  
>  Yes, perhaps..maybe as a short-term before we implement what you propose
>  below?
>  
>  > 
>  > As a second alternative I was thinking if it is necessary to rename the
>  > group at all? Historically there were some sysdb calls which generate
>  > the DN of a group object for lookups with the aim to speed things up
>  > (assuming a base search is faster then an indexed sub-tree search).
>  > With all the short-name/fully-qualified-name/id-view changes I wonder if
>  > this is used for lookups anymore. If not, I think there is no need to
>  > rename the group at all. If there are still such lookups (iirc Michal is
>  > currently working on the sysdb code, maybe he knows?) we might want to
>  > replace them by proper searches and eliminate the need to rename the
>  > group. Here I do not have any data either but I would expect that there
>  > is no big difference in ldb between a base search and an indexed
>  > sub-tree search.
>  
>  I like with this direction more. I was even thinking we should not use name
>  in RDN at all with the AD domain and always use objectSID. Then we would
>  also have fewer problems changing numerical IDs..
>  
>  My only concern is that this is an invasive solution that might bring
>  bugs. Currently there is around 40 occurences of LDB_SCOPE_BASE in the
>  code. At least those in sysdb_views.c and sysdb_asq_search(),
>  sysdb_initgroups() and sysdb_initgroups_with_views() look like they
>  would need to be converted. And currently the ignore_group_members bug
>  is hitting users.. 
Oh and one more argument against renaming -- it is actually performance
intensive, because the memberof plugin needs to establish all the links
again..

So I would prefer to not use the name as RDN value at all, but the scope
seems out of question for 1.12 and even for 1.13 looks like a stretch
goal.

[Jakub Hrozek]

On Mon, May 11, 2015 at 12:27:03PM +0200, Jakub Hrozek wrote:
>  > I like with this direction more. I was even thinking we should
> not use name
>  > in RDN at all with the AD domain and always use objectSID. Then we would
>  > also have fewer problems changing numerical IDs..
>  > 
>  > My only concern is that this is an invasive solution that might bring
>  > bugs. Currently there is around 40 occurences of LDB_SCOPE_BASE in the
>  > code. At least those in sysdb_views.c and sysdb_asq_search(),
>  > sysdb_initgroups() and sysdb_initgroups_with_views() look like they
>  > would need to be converted. And currently the ignore_group_members bug
>  > is hitting users..
>  
>  Oh and one more argument against renaming -- it is actually performance
>  intensive, because the memberof plugin needs to establish all the links
>  again..
>  
>  So I would prefer to not use the name as RDN value at all, but the scope
>  seems out of question for 1.12 and even for 1.13 looks like a stretch
>  goal. 
As also discussed elsewhere with Sumit, we need to first stop using the
DN for searches in our cache and then stop using the name attribute in
RDN -- at this point we won't need to rename the groups at all.

But that's a future goal, so as a short-term fix I implemented what
Sumit proposed earlier -- if ignore_group_members is set to True, we
fetch the group entry as if POSIX attributes were in use or as if the
group was located in a subdomain.

[Sumit Bose]

On Fri, May 22, 2015 at 03:52:20PM +0200, Jakub Hrozek wrote:
>  On Mon, May 11, 2015 at 12:27:03PM +0200, Jakub Hrozek wrote:
>  > > I like with this direction more. I was even thinking we should not use name
>  > > in RDN at all with the AD domain and always use objectSID. Then we would
>  > > also have fewer problems changing numerical IDs..
>  > > 
>  > > My only concern is that this is an invasive solution that might bring
>  > > bugs. Currently there is around 40 occurences of LDB_SCOPE_BASE in the
>  > > code. At least those in sysdb_views.c and sysdb_asq_search(),
>  > > sysdb_initgroups() and sysdb_initgroups_with_views() look like they
>  > > would need to be converted. And currently the ignore_group_members bug
>  > > is hitting users..
>  > 
>  > Oh and one more argument against renaming -- it is actually performance
>  > intensive, because the memberof plugin needs to establish all the links
>  > again..
>  > 
>  > So I would prefer to not use the name as RDN value at all, but the scope
>  > seems out of question for 1.12 and even for 1.13 looks like a stretch
>  > goal.
>  
>  As also discussed elsewhere with Sumit, we need to first stop using the
>  DN for searches in our cache and then stop using the name attribute in
>  RDN -- at this point we won't need to rename the groups at all.
>  
>  But that's a future goal, so as a short-term fix I implemented what
>  Sumit proposed earlier -- if ignore_group_members is set to True, we
>  fetch the group entry as if POSIX attributes were in use or as if the
>  group was located in a subdomain. 
>  From dd1c23e5c0b197819eea793a118f89bc03fe9798 Mon Sep 17 00:00:00
> 2001
>  From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  Date: Fri, 22 May 2015 15:19:31 +0200
>  Subject: [PATCH] Download complete groups if ignore_group_members is set with
>   tokengroups
>  
>  Resolves:
>      https://fedorahosted.org/sssd/ticket/2644
>  
>  When tokenGroups are enabled, we save groups using their SID as the RDN
>  attribute during initgroups() and later, if the groups is requested and saved
>  again with the full name, remove the original and save the new group entry.
>  
>  Saving the new group entry would break if ignore_group_members is also
>  set, because the new group entry would lack the "member" attribute, so the
>  member/memberof links between the new group and the user entry wouldn't
>  be established again.
>  
>  This patch changes the initgroups processing so that the full group
>  object is fetched when initgroups is enabled but together with
>  ignore_group_members.
>  
>  To reproduce the bug, set: ignore_group_members = True with a
>  backend that uses:
>      id_provider = ad
>  Then run:
>      $ id aduser(a)ad_domain.com
>      $ id aduser(a)ad_domain.com
>  ---
>   src/providers/ldap/sdap_async_initgroups_ad.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>  
>  diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
> b/src/providers/ldap/sdap_async_initgroups_ad.c
>  index 9915f1863f172d5d3f59afe03abbbfb87fdf3409..1db07049b21a65eb33ed5fb096ce606453504f38
> 100644
>  --- a/src/providers/ldap/sdap_async_initgroups_ad.c
>  +++ b/src/providers/ldap/sdap_async_initgroups_ad.c
>  @@ -1445,7 +1445,9 @@ sdap_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
>       state->use_id_mapping = use_id_mapping;
>       state->domain = domain;
>   
>  -    if (state->use_id_mapping && !IS_SUBDOMAIN(state->domain)) {
>  +    if (state->use_id_mapping
>  +            && !IS_SUBDOMAIN(state->domain)
>  +            && state->domain->ignore_group_members == false) { 
Since this is a workaround until a more general solution is available I
would recommend to add a comment with an explanation and maybe even a
link to a ticket to make sure that the workaround is not 'optimized'
away accidentally and that it is removed when the solution is available.

bye,
Sumit

>           subreq = sdap_ad_tokengroups_initgr_mapping_send(state, ev,
> opts,
>                                                            sysdb, domain, sh,
>                                                            name, orig_dn,
>  @@ -1485,7 +1487,9 @@ static void sdap_ad_tokengroups_initgroups_done(struct tevent_req
> *subreq)
>       req = tevent_req_callback_data(subreq, struct tevent_req);
>       state = tevent_req_data(req, struct sdap_ad_tokengroups_initgroups_state);
>   
>  -    if (state->use_id_mapping && !IS_SUBDOMAIN(state->domain)) {
>  +    if (state->use_id_mapping
>  +            && !IS_SUBDOMAIN(state->domain)
>  +            && state->domain->ignore_group_members == false) {
>           ret = sdap_ad_tokengroups_initgr_mapping_recv(subreq);
>       } else {
>           ret = sdap_ad_tokengroups_initgr_posix_recv(subreq);
>  -- 
>  2.1.0
>   
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, May 22, 2015 at 04:56:31PM +0200, Sumit Bose wrote:
>  On Fri, May 22, 2015 at 03:52:20PM +0200, Jakub Hrozek wrote:
>  > On Mon, May 11, 2015 at 12:27:03PM +0200, Jakub Hrozek wrote:
>  > > > I like with this direction more. I was even thinking we should not use
> name
>  > > > in RDN at all with the AD domain and always use objectSID. Then we would
>  > > > also have fewer problems changing numerical IDs..
>  > > > 
>  > > > My only concern is that this is an invasive solution that might bring
>  > > > bugs. Currently there is around 40 occurences of LDB_SCOPE_BASE in the
>  > > > code. At least those in sysdb_views.c and sysdb_asq_search(),
>  > > > sysdb_initgroups() and sysdb_initgroups_with_views() look like they
>  > > > would need to be converted. And currently the ignore_group_members bug
>  > > > is hitting users..
>  > > 
>  > > Oh and one more argument against renaming -- it is actually performance
>  > > intensive, because the memberof plugin needs to establish all the links
>  > > again..
>  > > 
>  > > So I would prefer to not use the name as RDN value at all, but the scope
>  > > seems out of question for 1.12 and even for 1.13 looks like a stretch
>  > > goal.
>  > 
>  > As also discussed elsewhere with Sumit, we need to first stop using the
>  > DN for searches in our cache and then stop using the name attribute in
>  > RDN -- at this point we won't need to rename the groups at all.
>  > 
>  > But that's a future goal, so as a short-term fix I implemented what
>  > Sumit proposed earlier -- if ignore_group_members is set to True, we
>  > fetch the group entry as if POSIX attributes were in use or as if the
>  > group was located in a subdomain.
>  
>  > From dd1c23e5c0b197819eea793a118f89bc03fe9798 Mon Sep 17 00:00:00 2001
>  > From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  > Date: Fri, 22 May 2015 15:19:31 +0200
>  > Subject: [PATCH] Download complete groups if ignore_group_members is set with
>  >  tokengroups
>  > 
>  > Resolves:
>  >     https://fedorahosted.org/sssd/ticket/2644
>  > 
>  > When tokenGroups are enabled, we save groups using their SID as the RDN
>  > attribute during initgroups() and later, if the groups is requested and saved
>  > again with the full name, remove the original and save the new group entry.
>  > 
>  > Saving the new group entry would break if ignore_group_members is also
>  > set, because the new group entry would lack the "member" attribute, so
> the
>  > member/memberof links between the new group and the user entry wouldn't
>  > be established again.
>  > 
>  > This patch changes the initgroups processing so that the full group
>  > object is fetched when initgroups is enabled but together with
>  > ignore_group_members.
>  > 
>  > To reproduce the bug, set: ignore_group_members = True with a
>  > backend that uses:
>  >     id_provider = ad
>  > Then run:
>  >     $ id aduser(a)ad_domain.com
>  >     $ id aduser(a)ad_domain.com
>  > ---
>  >  src/providers/ldap/sdap_async_initgroups_ad.c | 8 ++++++--
>  >  1 file changed, 6 insertions(+), 2 deletions(-)
>  > 
>  > diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
> b/src/providers/ldap/sdap_async_initgroups_ad.c
>  > index
> 9915f1863f172d5d3f59afe03abbbfb87fdf3409..1db07049b21a65eb33ed5fb096ce606453504f38 100644
>  > --- a/src/providers/ldap/sdap_async_initgroups_ad.c
>  > +++ b/src/providers/ldap/sdap_async_initgroups_ad.c
>  > @@ -1445,7 +1445,9 @@ sdap_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
>  >      state->use_id_mapping = use_id_mapping;
>  >      state->domain = domain;
>  >  
>  > -    if (state->use_id_mapping && !IS_SUBDOMAIN(state->domain)) {
>  > +    if (state->use_id_mapping
>  > +            && !IS_SUBDOMAIN(state->domain)
>  > +            && state->domain->ignore_group_members == false) {
>  
>  Since this is a workaround until a more general solution is available I
>  would recommend to add a comment with an explanation and maybe even a
>  link to a ticket to make sure that the workaround is not 'optimized'
>  away accidentally and that it is removed when the solution is available.
>  
>  bye,
>  Sumit 
Sure, good idea.

[Sumit Bose]

On Fri, May 22, 2015 at 05:35:49PM +0200, Jakub Hrozek wrote:
>  On Fri, May 22, 2015 at 04:56:31PM +0200, Sumit Bose wrote:
>  > On Fri, May 22, 2015 at 03:52:20PM +0200, Jakub Hrozek wrote:
>  > > On Mon, May 11, 2015 at 12:27:03PM +0200, Jakub Hrozek wrote:
>  > > > > I like with this direction more. I was even thinking we should not
> use name
>  > > > > in RDN at all with the AD domain and always use objectSID. Then we
> would
>  > > > > also have fewer problems changing numerical IDs..
>  > > > > 
>  > > > > My only concern is that this is an invasive solution that might
> bring
>  > > > > bugs. Currently there is around 40 occurences of LDB_SCOPE_BASE in
> the
>  > > > > code. At least those in sysdb_views.c and sysdb_asq_search(),
>  > > > > sysdb_initgroups() and sysdb_initgroups_with_views() look like they
>  > > > > would need to be converted. And currently the ignore_group_members
> bug
>  > > > > is hitting users..
>  > > > 
>  > > > Oh and one more argument against renaming -- it is actually performance
>  > > > intensive, because the memberof plugin needs to establish all the links
>  > > > again..
>  > > > 
>  > > > So I would prefer to not use the name as RDN value at all, but the scope
>  > > > seems out of question for 1.12 and even for 1.13 looks like a stretch
>  > > > goal.
>  > > 
>  > > As also discussed elsewhere with Sumit, we need to first stop using the
>  > > DN for searches in our cache and then stop using the name attribute in
>  > > RDN -- at this point we won't need to rename the groups at all.
>  > > 
>  > > But that's a future goal, so as a short-term fix I implemented what
>  > > Sumit proposed earlier -- if ignore_group_members is set to True, we
>  > > fetch the group entry as if POSIX attributes were in use or as if the
>  > > group was located in a subdomain.
>  > 
>  > > From dd1c23e5c0b197819eea793a118f89bc03fe9798 Mon Sep 17 00:00:00 2001
>  > > From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  > > Date: Fri, 22 May 2015 15:19:31 +0200
>  > > Subject: [PATCH] Download complete groups if ignore_group_members is set with
>  > >  tokengroups
>  > > 
>  > > Resolves:
>  > >     https://fedorahosted.org/sssd/ticket/2644
>  > > 
>  > > When tokenGroups are enabled, we save groups using their SID as the RDN
>  > > attribute during initgroups() and later, if the groups is requested and saved
>  > > again with the full name, remove the original and save the new group entry.
>  > > 
>  > > Saving the new group entry would break if ignore_group_members is also
>  > > set, because the new group entry would lack the "member" attribute,
> so the
>  > > member/memberof links between the new group and the user entry wouldn't
>  > > be established again.
>  > > 
>  > > This patch changes the initgroups processing so that the full group
>  > > object is fetched when initgroups is enabled but together with
>  > > ignore_group_members.
>  > > 
>  > > To reproduce the bug, set: ignore_group_members = True with a
>  > > backend that uses:
>  > >     id_provider = ad
>  > > Then run:
>  > >     $ id aduser(a)ad_domain.com
>  > >     $ id aduser(a)ad_domain.com
>  > > ---
>  > >  src/providers/ldap/sdap_async_initgroups_ad.c | 8 ++++++--
>  > >  1 file changed, 6 insertions(+), 2 deletions(-)
>  > > 
>  > > diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
> b/src/providers/ldap/sdap_async_initgroups_ad.c
>  > > index
> 9915f1863f172d5d3f59afe03abbbfb87fdf3409..1db07049b21a65eb33ed5fb096ce606453504f38 100644
>  > > --- a/src/providers/ldap/sdap_async_initgroups_ad.c
>  > > +++ b/src/providers/ldap/sdap_async_initgroups_ad.c
>  > > @@ -1445,7 +1445,9 @@ sdap_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
>  > >      state->use_id_mapping = use_id_mapping;
>  > >      state->domain = domain;
>  > >  
>  > > -    if (state->use_id_mapping && !IS_SUBDOMAIN(state->domain))
> {
>  > > +    if (state->use_id_mapping
>  > > +            && !IS_SUBDOMAIN(state->domain)
>  > > +            && state->domain->ignore_group_members == false) {
>  > 
>  > Since this is a workaround until a more general solution is available I
>  > would recommend to add a comment with an explanation and maybe even a
>  > link to a ticket to make sure that the workaround is not 'optimized'
>  > away accidentally and that it is removed when the solution is available.
>  > 
>  > bye,
>  > Sumit
>  
>  Sure, good idea. 
Hi,

I did a couple of tests and didn't found an issue (cause by the patch).
CI passes as well:

http://sssd-ci.duckdns.org/logs/job/16/38/summary.html

so ACK.

bye,
Sumit

[Jakub Hrozek]

On Fri, May 29, 2015 at 06:24:06PM +0200, Sumit Bose wrote:
>  Hi,
>  
>  I did a couple of tests and didn't found an issue (cause by the patch).
>  CI passes as well:
>  
>  http://sssd-ci.duckdns.org/logs/job/16/38/summary.html
>  
>  so ACK. 
master: ee44aac95e42c3cb634876286a2aa4960ac69a2b
sssd-1-12: 44f35a0f32785bf460b5d05424f5e9a15f4f4028