[PR PATCH] idmap_rid: default group always set to "Domain Users"

[Uri Simchoni]

On 04/06/2018 11:43 PM, Github bot account via samba-technical wrote:
> There is a new pull request by dmulder against master on the Samba Samba Github repository
> 
> https://github.com/dmulder/samba Bug1087931
> https://github.com/samba-team/samba/pull/159
> 
> idmap_rid: default group always set to "Domain Users"
> https://bugzilla.samba.org/show_bug.cgi?id=13371
> 
> I don't know whether I've taken the right approach to fixing this, but essentially this forward ports some of the code removed in [this commit](https://github.com/samba-team/samba/commit/241c81b2763392439043261cf179cd2c8793faed) which broke primary groups for the rid backend (gecos was broken also).
> 
> A patch file from https://github.com/samba-team/samba/pull/159.patch is attached
> 
tl;dr - Ask Volker, but I'm still providing my 2c so that this doesn't
get accidentally pushed.

Whew, something looks awfully wrong about this, with algorithmic
id-mapping code making LDAP calls :(. There's no denying the bug though.

As far as correctness goes, any query to the DC is to be made only if
the information is not in the netsamlogon cache - that has been the
behavior before the breaking commit and it doesn't appear to be the case
with the fix. So NAK at least until this is fixed. It's enough to verify
the user doesn't exist, because if it exists, we already get all correct
data from there.

Beyond correctness, perhaps there's a better fix that would not have an
algorithmic ID-mapping backend make LDAP queries to AD (do it in the
central place). I would try making an RPC via the domain child to
determine the primary group SID/user name if the user is not in the
netsamlogon cache and the id-mapping backend has no clue, and then have
the id-mapping translate that into a gid. Volker would probably be the
person to lay out the best solution. I wouldn't NAK a bug fix just
because it's not clean enough to my taste without me suggesting a
concrete alternative though.

Background - the patch set that broke things had a premise that returned
user info is correct only for users who logged on, and this principle
was agreed-upon (acked and not nacked) on the list before submitting,
reviewing, and pushing this patch set. In a sense this bug is
as-designed. Then it turned out users cared about this breakage and part
of it was reverted but apparently this part was missed.

Thanks,
Uri.