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
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? 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
