[Freeipa-devel] [PATCH 0029][Tests] Adding authentication test to trust test suite

[Lenka Doudova]

On 07/20/2016 02:28 PM, Martin Babinsky wrote:
> On 07/19/2016 10:41 AM, Lenka Doudova wrote:
>> Hi,
>>
>>
>> this patch adds authentication test (specifically "kinit -E
>> ipauser at IPADOMAIN") to basic trust test suite, as requested by Sumit.
>>
>> Intended to be applied after my patches 25.4 and 26.3 (already waiting
>> to be pushed).
>>
>>
>> Lenka
>>
>>
>>
>
> Hi Lenka,
>
> Code review:
>
> 1.) You have several PEP8 transgressions in the patch, please fix them:
> """
> $ git show -U0 | pep8 --diff
> ./ipatests/test_integration/test_trust.py:172:34: E127 continuation 
> line over-indented for visual indent
> ./ipatests/test_integration/test_trust.py:176:35: E128 continuation 
> line under-indented for visual indent
> ./ipatests/test_integration/test_trust.py:180:27: E231 missing 
> whitespace after ','
> """
>
> 2.)
> +
> +
> +def unlock_principal_password(user, oldpw, newpw, master):
> +    container_user = "cn=users,cn=accounts"
> +    basedn = master.domain.basedn
> +
> +    userdn = "uid={},{},{}".format(user, container_user, basedn)
> +
> +    args = [paths.LDAPPASSWD, '-D', userdn, '-w', oldpw, '-a', oldpw,
> +            '-s', newpw, '-x']
> +    return run(args)
>
> there is already a function with the same name in other module:
>
> """
> git grep -ni 'def unlock_principal_password' ipatests
> ipatests/test_integration/util.py:82:def 
> unlock_principal_password(user, oldpw, newpw, master):
> ipatests/util.py:676:def unlock_principal_password(user, oldpw, newpw):
> """
>
> Having functions with the same names in different modules makes for 
> poor coding practice IMHO. Please rename the function to something 
> like "ldappasswd_user" or something like that so that we have a 
> distinction.
>
> Also, you should call ldappasswd directly on master (since you pass it 
> as an argument anyway) using "master.run_command(args)". You should 
> *not* run any in-test code on the controller unless absolutely necessary.
>
> You can then remove the ipautil.run import from the beginning of the 
> module.
>
> Commit message woes:
>
> 1.) vague summary is vague, I would rather see something like:
>
> """
> test that IPA user can kinit using enterprise principal with IPA domain
> """
>
> 2.) Commit message body is longer than 78 characters.
>
> 3.) there is no ticket URL, I think you can link 
> https://fedorahosted.org/freeipa/ticket/6036 or create a new ticket 
> for the change.
>
Hi,

thanks for review, fixed (and renamed) patch attached.

Lenka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ldoudova-0029.2-Tests-IPA-user-can-kinit-using-enterprise-principal.patch
Type: text/x-patch
Size: 2914 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160722/a466357b/attachment.bin>