On Mon, Aug 26, 2013 at 11:02:56AM +0200, Jakub Hrozek wrote: > On Fri, Aug 16, 2013 at 06:25:02PM +0200, Sumit Bose wrote: > > Hi, > > > > this series of patches contains improvements for the PAC responder > > related to the support of UIDs and GIDs managed by AD. > > > > The first patch is a fix for https://fedorahosted.org/sssd/ticket/1996. > > The original idea in the ticket was to modify an existing user entry > > instead of deleting and recreating it. But since the PAC does not > > contain any useful information which would improve the entry I decided to > > not touch existing user entries at all and only update the group > > memberships. > > > > Please find details about the other patches in the commit messages. > > > > bye, > > Sumit > > Hi, > > The patches work well with one addition I sent to the list separately. I > don't have any change requests, just some questions before the patches > are applied, see below: > > > From 1e54d2061d72d2f72c30d3fced9b43d4ec369a28 Mon Sep 17 00:00:00 2001 > > From: Sumit Bose <[email protected]> > > Date: Thu, 1 Aug 2013 12:40:24 +0200 > > Subject: [PATCH 1/6] PAC: if user entry already exists keep it > > > > Currently the PAC responder deletes a user entry and recreates it if > > some attributes seems to be different. > > > > Two of the attributes where the home directory and the shell of the > > user. Those two attributes are not available from the PAC but where > > generates by the PAC responder. The corresponding ID provider might have > > better means to determine those attributes, e.g. read them from LDAP, so > > we shouldn't change them here. > > > > The third attribute is the user name. Since the PAC responder does > > lookups only based on the UID we can wait until the ID provider updates > > the entry. > > The current logic doesn't detect if any of the extra attributes we put > into the "_attrs" variable of get_pwd_from_pac() changed. I think this > can only apply to UPN as the alias is derived from the name and the SID > should never change. Can this be a problem?
I think not. If it is the generated UPN, i.e. username + realm, they are the same and if the UPN in the cache was extracted from the Kerberos ticket during password authentication it would be the more correct one. > > Also maybe we should lowercase the alias explicitly as we do in the IPA > extdom handler code. But this patch is good as-is, I think, if there are > any issues, they can be solved separately. I agree this is a good idea. Maybe we should consider to introduce a new sysdb call like sysdb_attrs_add_lower_case_string()? Would you mind to open a suitable ticket? Thank you for the review. bye, Sumit > > > From: Sumit Bose <[email protected]> > > Date: Tue, 6 Aug 2013 11:10:10 +0200 > > Subject: [PATCH 2/6] PAC: do not create users with missing GID > ACK > > > From: Sumit Bose <[email protected]> > > Date: Thu, 8 Aug 2013 12:35:12 +0200 > > Subject: [PATCH 3/6] PAC: handle non-POSIX groups in cache > ACK > > > From: Sumit Bose <[email protected]> > > Date: Thu, 8 Aug 2013 14:09:42 +0200 > > Subject: [PATCH 4/6] PAC: read user DN instead of constructing it > ACK. > > > From 966390e5436d2eed565774adddf33f6ce3f85b24 Mon Sep 17 00:00:00 2001 > > From: Sumit Bose <[email protected]> > > Date: Thu, 8 Aug 2013 16:56:06 +0200 > > Subject: [PATCH 5/6] PAC: do not fail if a single group cannot be > > added/removed > ACK > > > From: Sumit Bose <[email protected]> > > Date: Thu, 8 Aug 2013 18:29:48 +0200 > > Subject: [PATCH 6/6] PAC: use SID instead of GID to search for groups > ACK > > _______________________________________________ > sssd-devel mailing list > [email protected] > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
