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.
[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?
[snip]