On Wed, Jan 29, 2014 at 05:40:56PM +0100, Lukas Slebodnik wrote:
> On (28/01/14 23:10), Jakub Hrozek wrote:
> >On Mon, Jan 27, 2014 at 09:46:26AM +0100, Lukas Slebodnik wrote:
> >> ehlo,
> >>
> >> two patches are attached.
> >> The 1st one is almost he same like patch:
> >> commit 16b27fcceebcbbaeefaf5b9bdf2dec3065adba4a
> >> LDAP: Don't fail if subdomain cannot be found by sid
> >>
> >> I didn't notice that similar change was done in two separeted patches.
> >>
> >>
> >> I am not sure if 2nd is right solution, because some conditions are litle
> >> bit
> >> confusing for me.
> >>
> >> LS
> >
> >> From 6fb4a41e7340f48565d5d81f079e9861b5b3c71e Mon Sep 17 00:00:00 2001
> >> From: Lukas Slebodnik <lsleb...@redhat.com>
> >> Date: Fri, 24 Jan 2014 17:03:27 +0100
> >> Subject: [PATCH 1/2] LDAP: store group if subdomain cannot be found by sid
> >>
> >> Domain needn't contain sid if id_provider is ldap.
> >> With enabled id mapping, group couldn't be stored, because domain
> >> couldn't be found by sid.
> >>
> >> Resolves:
> >> https://fedorahosted.org/sssd/ticket/2172
> >> ---
> >> src/providers/ldap/sdap_async_groups.c | 10 ++++++----
> >> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/providers/ldap/sdap_async_groups.c
> >> b/src/providers/ldap/sdap_async_groups.c
> >> index
> >> 4ae772636863acf4c1fd59a20a20d1ad3a28ace0..55813c0fe4ffe864e1725959ad2a6ff51b1ffc65
> >> 100644
> >> --- a/src/providers/ldap/sdap_async_groups.c
> >> +++ b/src/providers/ldap/sdap_async_groups.c
> >> @@ -450,6 +450,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> >> bool posix_group;
> >> bool use_id_mapping;
> >> char *sid_str;
> >> + struct sss_domain_info *subdomain;
> >
> >I know that the function is called 'find_subdomain_by_sid' but if I read
> >its code right, it can return also the main domain and is actually used
> >this way on a couple of places. So I suggest we rename the function in a
> >separate patch. Similarly, this variable shouldn't be called
> >'subdomain', but maybe 'sid_dom' or 'group_dom' ?
> >
> >(the same misnomer seems to be in save_user)
> >
> >> int32_t ad_group_type;
> >>
> >> tmpctx = talloc_new(NULL);
> >> @@ -488,11 +489,12 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> >> /* If this object has a SID available, we will determine the correct
> >> * domain by its SID. */
> >> if (sid_str != NULL) {
> >> - dom = find_subdomain_by_sid(get_domains_head(dom), sid_str);
> >> - if (dom == NULL) {
> >> - DEBUG(SSSDBG_OP_FAILURE, ("SID %s does not belong to any
> >> known "
> >> + subdomain = find_subdomain_by_sid(get_domains_head(dom), sid_str);
> >> + if (subdomain) {
> >> + dom = subdomain;
> >> + } else {
> >> + DEBUG(SSSDBG_TRACE_FUNC, ("SID %s does not belong to any
> >> known "
> >> "domain\n", sid_str));
> >
> >This change is correct. But I checked the other calls to
> >find_subdomain_by_sid() and there are some other places where a similar
> >change might be needed:
> > src/providers/simple/simple_access_check.c:614
> >and several places in src/providers/ldap/sdap_async_initgroups_ad.c
> >
> >But I admit I only checked the code visually and didn't do any testing.
> >
> >Because there is a deadline coming up, we can commit this patch, but
> >then we should file a ticket to inspect the other usages
> >
> >And one related idea -- some code in the LDAP provider really depends on
> >having data like domain SID available. I wonder if we could add a new
> >LDAP schema (SDAP_SCHEMA_AD_BE) that would only be used by the AD
> >provider and code that depends on having the AD data be only called if
> >this schema is selected. The current AD schema could be still used for
> >cases where the LDAP ID provider is used.
> >
> >An alternative might be to split the code better during the upcoming
> >refactoring of the LDAP provider.
> >
> I would prefer to file a ticket, because proposed change can break something.
>
> >> - return ERR_DOMAIN_NOT_FOUND;
> >> }
> >> }
> >>
> >> --
> >> 1.8.5.3
> >>
> >
> >> From 8ee3656c5a24abf5520ea5c636935ff9b0c5fb2d Mon Sep 17 00:00:00 2001
> >> From: Lukas Slebodnik <lsleb...@redhat.com>
> >> Date: Fri, 24 Jan 2014 19:11:57 +0100
> >> Subject: [PATCH 2/2] LDAP: Fix group handling with AD schema
> >>
> >> regression introduced in commit 8280c5213094a72fcaa499dda2f8647246185d45
> >
> >Could we set the ldap_group_type in the gen_ad2008r2_group_map instead?
> >Something like:
> > - { "ldap_group_type", NULL, SYSDB_GROUP_TYPE, NULL },
> > + { "ldap_group_type", "groupType", SYSDB_GROUP_TYPE, NULL },
>
> This change is sufficient and patch look nicer :-)
>
> Updated patches are attached.
>
> LS
These patches work for me and resolve the issue. Retrieving groups also
still works fine with AD backend. So ACK to the code.
I still think there are issues to address, at least we should check the
usage of find_domain_by_XXX. I think that searching for a domain should
be only used in the generic LDAP code if there's no other way:
https://fedorahosted.org/sssd/ticket/2216
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
