Re: [Freeipa-devel] [PATCH 0029][Tests] Adding authentication test to trust test suite
On 07/22/2016 11:20 AM, Lenka Doudova wrote: 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@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 Thanks, ACK. Pushed to master: 648b5afa2f2d01d99c1cf2d1f4a87a5da4461246 -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0029][Tests] Adding authentication test to trust test suite
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@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 From f93dafb858d55701f52b6f316a20818855a6e180 Mon Sep 17 00:00:00 2001 From: Lenka DoudovaDate: Mon, 18 Jul 2016 14:38:18 +0200 Subject: [PATCH] Tests: IPA user can kinit using enterprise principal with IPA domain Providing missing test case verifying authentication as IPA user, namely: "kinit -E ipauser@IPADOMAIN". https://fedorahosted.org/freeipa/ticket/6036 --- ipatests/test_integration/test_trust.py | 20 ipatests/test_integration/util.py | 13 + 2 files changed, 33 insertions(+) diff --git a/ipatests/test_integration/test_trust.py b/ipatests/test_integration/test_trust.py index e3fe9c89e9cd1357c8913d8fcd98699906f07f85..d0b8e58b75551976bcd2739d6811ad7b4b6faad8 100644 --- a/ipatests/test_integration/test_trust.py +++ b/ipatests/test_integration/test_trust.py @@ -161,6 +161,26 @@ class TestBasicADTrust(ADTrustBase): assert re.search(testuser_regex, result.stdout_text) +def test_ipauser_authentication(self): +ipauser = u'tuser' +original_passwd = 'Secret123' +new_passwd = 'userPasswd123' + +# create an ipauser for this test +self.master.run_command(['ipa', 'user-add', ipauser, '--first=Test', + '--last=User', '--password'], +stdin_text=original_passwd) + +# change password for the user to be able to kinit +util.ldappasswd_user_change(ipauser, original_passwd, new_passwd, +self.master) + +# try to kinit as ipauser +self.master.run_command( +['kinit', '-E', '{0}@{1}'.format(ipauser, + self.master.domain.name)], +stdin_text=new_passwd) + def test_remove_nonposix_trust(self): tasks.remove_trust_with_ad(self.master, self.ad_domain) tasks.clear_sssd_cache(self.master) diff --git a/ipatests/test_integration/util.py b/ipatests/test_integration/util.py index 594737b6d753d476cd06aeb0d5cd376b7ca46467..179f6727eae2d17f81f6204a504481784c4fa33c 100644 --- a/ipatests/test_integration/util.py +++ b/ipatests/test_integration/util.py @@ -20,6 +20,8 @@ import time import re +from ipaplatform.paths import paths +from ipalib.constants import DEFAULT_CONFIG def run_repeatedly(host, command, assert_zero_rc=True, test=None, timeout=30, **kwargs): @@ -75,3 +77,14 @@ def get_host_ip_with_hostmask(host): if match: return match.group('full_ip') + + +def ldappasswd_user_change(user, oldpw, newpw, master): +container_user =
Re: [Freeipa-devel] [PATCH 0029][Tests] Adding authentication test to trust test suite
On 07/19/2016 10:41 AM, Lenka Doudova wrote: Hi, this patch adds authentication test (specifically "kinit -E ipauser@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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code