On Thu, Jul 17, 2014 at 05:41:23PM +0200, Pavel Reichl wrote: > On Thu, 2014-07-17 at 07:59 +0200, Jakub Hrozek wrote: > > On 10 Jul 2014, at 16:38, Pavel Reichl <prei...@redhat.com> wrote: > > > > > Hello, > > > > > > please see attached patches. > > > > > > I found out that if we go with approach introduced in previous version > > > (in case of LDAP provider assume SID comes from default domain) this can > > > lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix > > > groups which in case of disabled id mapping leads to failure which will > > > end request prematurely. 2nd patch should make SID resolution more > > > resilient to handle this. > > > > > > > The only complaint I have about the code is that the domain NULL check is > > not needed, I actually think we should fail if there is a NULL domain in > > the provider code, the provider handler should already take care of finding > > the right domain. If not, we’re in big trouble anyway. > > > > Otherwise the code solves the problem — I can’t say it looks great. That’s > > not your fault Pavel, just yet another reminder that we need to work on the > > LDAP provider refactoring no later than 1.13. > > > > I tested with both id_provider=ldap and =ad using both POSIX attributes and > > ID mapping. Both worked fine in my testing. > > > > Can you re-send the patches without the domain check? Then I’ll ack. > > Sure, attached patches address your and Lukas' comments.
They indeed do, ACK > > [snip] > > >>>>>> 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. > > >> > > >> ah, sorry, now I see your point. I agree that the name misleading but I > > >> think this can be fixed after the release. > > > > > Would 's/find_subdomain_by_sid/find_domain_by_sid/' be a sufficient > solution? Yes, but in a separate patch, please. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
