On Fri, Jul 18, 2014 at 05:10:53PM +0200, Jakub Hrozek wrote:
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 <preichl(a)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
> > >>>>>> 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
> > >>
> > >> ah, sorry, now I see your point. I agree that the name misleading but
> > >> think this can be fixed after the release.
> > >
> Would 's/find_subdomain_by_sid/find_domain_by_sid/' be a sufficient
Yes, but in a separate patch, please.
Sorry Pavel, I pushed your rename patch, because it was simple and
applied on origin/master and didn't realize this one should took
precedence...can you rebase it one last time?
btw it's fine to say patch A depends on patch B, especially if B is
significantly more complex than A ;_)