On Fri, Dec 03, 2010 at 08:11:39AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/03/2010 06:56 AM, Sumit Bose wrote:
> On Thu, Dec 02, 2010 at 09:00:37AM -0500, Stephen Gallagher wrote:
> On 11/25/2010 08:25 AM, Sumit Bose wrote:
>>>> Hi,
>>>>
>>>> these two patches aim to fix trac ticket #672. While the first patch
>>>> only makes a utility function public the second adds some new layers to
>>>> the LDAP access provider. I've tried to make the changes in a way
to
>>>> make it easy to add new rules (#670) and new expire policies (#673,
>>>> #674, #690).
>>>>
>>>> I would like to ask the reviewers to check if the new code is really as
>>>> flexible as I think and if it enough to evaluate the shadow expire
>>>> attribute here. TIA
>>>>
>>>> bye,
>>>> Sumit
>>>>
>
>
> Patch 0001: Ack.
>
> Patch 0002: Nack.
> The manpage entry for ldap_account_expire_policy should list the valid
> values for this option. The patch comment notes that this is only
> "shadow" at present.
>
>> done
>
>
> sssm_ldap_access_init() should throw an error (or at least warn and
> ignore it) if we get a duplicate access rule. e.g.:
> ldap_access_order = filter, filter
>
>> I prefer the error, because it might not be clear which order was
>> intended.
>
>
> Please rename sdap_access_decide_offline(), sdap_access_retry(),
> sdap_access_connect_done() and sdap_access_get_access_done() to be clear
> that they apply to the access_filter.
>
>> done
>
>
> I'm not sure we want to deny on a missing password expiration attribute.
> Some users might be lacking this attribute with the intent that it means
> "no expiration". Maybe we should add a strictness option here.
>
>> I checked you pam_ldap handles this. A missing attribute or a value of 0
>> means "no expiration".
>
>
> next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just
> PAM_PERM_DENIED.
>
>> done
>
>> Thanks for the review, new version attached.
Nack.
Typo in sssd-ldap.5.xml (tat)
typo in ldap_init.c (dublicate)
fixed
check_list_for_duplicates() could be made more scalable by doing (in
pseudocode):
diff_string_lists(list, NULL, &nodupes, NULL, NULL); /* Pass in one
string, and get the list of entries that were only in that string */
if (array_len(nodupes) < array_len(list)) {
/* list had duplicates */
}
Internally, this just plays some tricks with a hash addition so that if
the same key is used, it will be dropped. So the resulting array
returned to *_list1_only will contain no duplicates. If the number of
entries in the _list1_only array is fewer than the number of entries in
the input list, duplicates were dropped.
We discussed this on irc and agreed that the order list is too short to
justify the use of a hash. Additionally it is not necessary here to get
a new list without duplicates, because it is sufficient to return an
error after the first duplicate is found. To avoid an unintended use of
this call I moved it frim util.c to ldap_init.c
New version attached.
bye,
Sumit
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkz47IgACgkQeiVVYja6o6PovwCfW8a0+X3KKPIJ1rRMm52yov0i
xzMAniR20aYjU5uJqHKlpF8AOe2Zx4eZ
=RXH6
-----END PGP SIGNATURE-----
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel