On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
> On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
> > On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> > > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see attached patch.
> > > >
> > > > Regards,
> > > >
> > > > PR
> > >
> > > The patch solves the problem, but I think one part should be improved:
> > >
> > > > @@ -875,7 +893,13 @@ static void
> > > > sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> > > > domain =
> > > > find_subdomain_by_sid(get_domains_head(state->domain), sid);
> > > > if (domain == NULL) {
> > > > DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID
> > > > %s\n", sid);
> > > > - continue;
> > > > + if (state->domain->parent == NULL &&
> > > > + state->domain->subdomains == NULL) {
> > > > + domain = state->domain;
> > > > + DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n",
> > > > domain->name);
> > > > + } else {
> > > > + continue;
> > > > + }
> > > > }
> > >
> > > I think this is a bit dangerous. I wonder if we should have some
> > > modification of find_subdomain_by_sid that would return the first
> > > configured domain if no subdomain provider was configured or if no
> > > domains had a SID. This could be a separate function.
> >
> > This sounds even more dangerous to me.
> >
> > >
> > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > to find the primary domain.
> >
> > I think find_subdomain_by_sid() does what the name says and of course it
> > can return the primary domain as long as the SID of the domain is know
^^^^^^
fwiw, this was my concern, the function is named "find_subdomain" yet it
can find both main domain and subdomain. But I won't bikeshed any further.
> > which is the case for the IPA and AD provider.
> >
>
> > What about adding an explicit check if the running id provider is the
> > plain LDAP provider?
> Would that be acceptable with you Jakub?
It's a bit hackish but I don't see any other way.
>
> > As an alternative the LDAP provider can add a
> > special value in the id member of the sss_dom_info struct and then
> > find_subdomain_by_sid can handle this case specially?
> >
> > bye,
> > Sumit
> >
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel@lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel@lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
