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 Doudova <[email protected]>
Date: 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 = dict(DEFAULT_CONFIG)['container_user']
+ basedn = master.domain.basedn
+
+ userdn = "uid={},{},{}".format(user, container_user, basedn)
+
+ args = [paths.LDAPPASSWD, '-D', userdn, '-w', oldpw, '-a', oldpw,
+ '-s', newpw, '-x']
+ master.run_command(args)
--
2.7.4
--
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
