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

[Rob Crittenden]

Ondrej Hamada wrote:
> On 12/09/2011 08:46 PM, Rob Crittenden wrote:
>> 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
> Corrected and modified according to rob's proposals
>
> Ondra
>

ACK, pushed to master. I fixed up a couple of white space issues (some 
intentions were 8, not 4 spaces) and expanded the commit message to ~72 
chars/line.

rob