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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky