On Tue, Jul 23, 2013 at 10:15:47AM +0300, Alexander Bokovoy wrote:
> On Tue, 23 Jul 2013, Nalin Dahyabhai wrote:
> >Apologies for the delay.
> Thanks for the review!
> 
> One short comment -- PAM code is from PAM pass-through plugin from
> 389-ds. That's the reason why its code doesn't follow slapi-nis way and
> why it has that license. I tried to keep it mostly intact to share
> changes but looking at git log it gets roughly two commits per year so
> maybe it is better to rework it completely.

That'd be my preference.  Other than knowing how to map specific
PAM error codes to LDAP-level errors, there doesn't seem to be a lot of
magic that needs to be preserved in there.

> I'll address other comments and will send updated version for the
> review today. This was my first sizable SLAPI code so errors are
> inevitable.

No worries.  I think there's already a lot in there that's right.

I've been thinking about the patch some more, and I need to revise a
couple of my comments.

[snip]
> >>+   slapi_entry_add_string(entry,
> >>+                          "uid", user_name);
> >
> >If is_uid is true, this is a numeric string.  Intentional?

Given that group entries are being constructed using group member
information, which is always login names, I guess it isn't.

[snip]
> >>+           for (i=0; grp.gr_mem[i]; i++) {
> >>+                   slapi_entry_add_string(entry, "memberUid",
> >>+                           slapi_ch_smprintf("uid=%s,%s", grp.gr_mem[i], 
> >>sdn));
> >>+           }
> >
> >The "memberUid" attribute doesn't typically contain DNs.  Did you mean
> >to use "member" here?  Or to just use the user login name for the value?

This probably wants to set "memberUid" to the grp.gr_mem element's
value, because if we construct a "member" DN and expect the plugin's
configured logic to dereference it and pull out the UID value, and the
plugin attempts to read the entry with that DN by doing a search with
scope=base to find the entry, I don't think it'll trigger the logic that
would create that entry in the cache.

That, and in compat trees we're generally in the business of unrolling
group memberships anyway.

Cheers,

Nalin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to