On 07/20/2014 10:05 PM, Jakub Hrozek wrote:
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@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.
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 ;_)
Sorry, the patch dependence somehow slipped my mind, I'll try to be more careful next time.