[Freeipa-devel] [PATCH] 5 User-add random password support

[Rob Crittenden]

Ondrej Hamada wrote:
> On 11/29/2011 10:31 AM, Martin Kosek wrote:
>> On Thu, 2011-11-24 at 17:51 +0100, Ondrej Hamada wrote:
>>> On 11/24/2011 03:54 PM, Ondrej Hamada wrote:
>>>> https://fedorahosted.org/freeipa/ticket/1979
>>>>
>>>> I've used code from ipalib/plugins/host.py to add support for
>>>> random
>>>> password generation. The '--random' option is now available in
>>>> user-add and user-mod commands. If both the 'password' and 'random'
>>>> options are used the 'random' option will be ignored.
>>>>
>>>>
>> Functionally, it works OK. I would just like to propose few
>> improvements:
>>
>> 1) Minor API version in VERSION file should be bumped since you add a
>> new option
>> 2) We should add some tests exercising this new functionality so that we
>> can detect regressions early
>> 3) (optional) I am thinking if the passwords we generate are not very
>> user friendly. I would love to see user's face when he is told that his
>> new password is 5QU;8l2%]y"? .
>>
>> While this is may be OK for hosts bulk passwords which are only
>> manipulated by admins, we may want to develop more user friendly
>> passwords in the user plugin.
>>
>> Martin
>>
> https://fedorahosted.org/freeipa/ticket/1979
>
> I've used code from ipalib/plugins/host.py to add
> support for random password generation. The
> '--random' option is now available in user-add and
> user-mod commands. If both the 'password' and 'random'
> options are used the 'random' option will be ignored.
>
> Two test cases were added to unit test's module
> test_user_plugin.py - they test creating and modifying
> user with random password. Two fuzzy tests were added:
> test for password(string that doesn't start or end with
> whitespace and doesn't containt other whitespace than
> ' ') and for whatever string(because of krbextradata).
>
> I've slightly modified ipa_generate_password in order
> to make passwords for users more user-friendly(reduce
> number of non-letters). It has two optional parameters
> now - first one is string of characters that should be
> used for generating the passwd and second one is length
> of password. If none parameter is set default values will
> be used so there's no need to modify other plugins that
> use random password generator.

This is very, very close. I just have a couple of observations:

1. Would it be clearer if in user.py you set the character set to: 
string.digits + string.ascii_letters + '_,.@' ? I gather you are 
excluding most of the characters that would cause the shell grief on 
purpose, right? You can probably add in +, - and = though.

2. Indention in ipa_generate_password() is a bit off.

3. Rather than fuzzy_string() I think you should define something like 
fuzzy_dergeneralizedtime() that specifies the time format in those 
kerberos time attributes. krbextradata is probably fine as a string.

rob