Hi Martin,
Thanks for the review. The updated patches are attached. Please, see my
comments below
On 08/30/2016 01:58 PM, Martin Basti wrote:
On 22.08.2016 13:18, Oleg Fayans wrote:
ping for review
On 08/02/2016 01:11 PM, Oleg Fayans wrote:
Hi Martin,
I did! Thank you!
On 08/02/2016 12:31 PM, Martin Basti wrote:
On 01.08.2016 22:46, Oleg Fayans wrote:
The test was redesigned so that it actually tests against an AD user.
cleanly applies, passes lint and passes
https://paste.fedoraproject.org/399504/00843641/
Okay
Did you forget to send patches?
Martin^2
On 06/28/2016 01:40 PM, Oleg Fayans wrote:
Patch-0050 rebased against latest upstream branch
On 06/28/2016 10:45 AM, Oleg Fayans wrote:
Passing test output:
https://paste.fedoraproject.org/385774/71035231/
NACK for 0049.1
1)
PEP8: you must use 2 empty lines between functions
Fixed
2)
+ new_args = " ".join(new_args + args)
you don't need this, run_command takes list as argument too
new_args.extend(args)
The list-based approach does not work with shell redirects which are
heavily used in the certs_id_idoverrides test. Thus, this trick is
really needed
3)
To make it more usable you should add raiseonerr as kwarg to
run_certutil (True as default)
Done
NACK for 0050.2
1)
+ tasks.run_certutil(master, ['-L', '-n', cls.adcert1, '-a', '>',
+ cls.adcert1_file], cls.reqdir)
+ tasks.run_certutil(master, ['-L', '-n', cls.adcert2, '-a', '>',
+ cls.adcert2_file], cls.reqdir)
IMO thus should raise an error if failed, but previously you set
raiseonerr=False (multiple times)
Agreed. Done
2)
+ cls.ad = cls.ad_domains[0].ads[0]
+ cls.ad_domain = cls.ad.domain.name
+ cls.aduser = "testuser@%s" % cls.ad_domain
+ cls.adcert1 = 'MyCert1'
+ cls.adcert2 = 'MyCert2'
+ cls.adcert1_file = cls.adcert1 + '.crt'
+ cls.adcert2_file = cls.adcert2 + '.crt'
New definitions of variables/constants should be directly in class not
in install method, adding new class variables in classmethod is the same
evil as adding instance variables outside __init__
Fair point. Fixed
3)
I have question, why do you need AD for this test? AFAIK you can use ID
overrides without AD
Correct. You can, but the workflow would be slightly different. For
example, you can not issue and sign cert requests for AD-users the way
you would do it for local users. We want to have tests that can be taken
by end-users as example how to use our software, that's why it is better
to be as close to real-world use-cases as it is possible.
Martin^3
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 867c603183d792b0056c0f8895f52577bc67d7b0 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Tue, 6 Sep 2016 12:39:45 +0200
Subject: [PATCH] Added interface to certutil
---
ipatests/test_integration/tasks.py | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index c60d43699d6577abe930ac8d6ab696feea837331..0e329f4ad5d754fd61a9ca911488230677daad77 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -1187,6 +1187,13 @@ def run_server_del(host, server_to_delete, force=False,
return host.run_command(args, raiseonerr=False)
+def run_certutil(host, args, reqdir, stdin=None, raiseonerr=True):
+ new_args = [paths.CERTUTIL, "-d", reqdir]
+ new_args = " ".join(new_args + args)
+ return host.run_command(new_args, raiseonerr=raiseonerr,
+ stdin_text=stdin)
+
+
def assert_error(result, stderr_text, returncode=None):
"Assert that `result` command failed and its stderr contains `stderr_text`"
assert stderr_text in result.stderr_text, result.stderr_text
--
1.8.3.1
From fb0591407a64dcf84eda1a28a06d1ead2fa7ab0d Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Tue, 6 Sep 2016 12:41:06 +0200
Subject: [PATCH] Automated test for certs in idoverrides feature
https://fedorahosted.org/freeipa/ticket/6005
---
.../test_integration/test_certs_in_idoverrides.py | 120 +++++++++++++++++++++
1 file changed, 120 insertions(+)
create mode 100644 ipatests/test_integration/test_certs_in_idoverrides.py
diff --git a/ipatests/test_integration/test_certs_in_idoverrides.py b/ipatests/test_integration/test_certs_in_idoverrides.py
new file mode 100644
index 0000000000000000000000000000000000000000..d72fc1e898f0574015c6b7dd5f601cec8e4350d6
--- /dev/null
+++ b/ipatests/test_integration/test_certs_in_idoverrides.py
@@ -0,0 +1,120 @@
+#
+# Copyright (C) 2016 FreeIPA Contributors see COPYING for license
+#
+
+import os
+import re
+import string
+from ipatests.test_integration import tasks
+from ipatests.test_integration.base import IntegrationTest
+from ipatests.test_integration.tasks import assert_error
+from ipatests.test_integration.env_config import get_global_config
+config = get_global_config()
+
+
+class TestCertsInIDOverrides(IntegrationTest):
+ topology = "line"
+ service_certprofile = 'caIPAserviceCert'
+ num_ad_domains = 1
+ user_certprofile = 'caIPAuserCert'
+ adview = 'Default Trust View'
+ cert_re = re.compile('Certificate: (?P<cert>.*?)\\s+.*')
+ ad = config.ad_domains[0].ads[0]
+ ad_domain = ad.domain.name
+ aduser = "testuser@%s" % ad_domain
+ adcert1 = 'MyCert1'
+ adcert2 = 'MyCert2'
+ adcert1_file = adcert1 + '.crt'
+ adcert2_file = adcert2 + '.crt'
+
+ @classmethod
+ def uninstall(cls, mh):
+ cls.master.run_command(['rm', '-rf', cls.reqdir], raiseonerr=False)
+
+ @classmethod
+ def install(cls, mh):
+ super(TestCertsInIDOverrides, cls).install(mh)
+ master = cls.master
+
+ # AD-related stuff
+ tasks.install_adtrust(master)
+ tasks.sync_time(master, cls.ad)
+ tasks.establish_trust_with_ad(cls.master, cls.ad_domain,
+ extra_args=['--range-type',
+ 'ipa-ad-trust'])
+
+ tasks.sync_time(cls.master, cls.ad)
+ master.run_command(['ipa', 'certprofile-show', cls.service_certprofile,
+ "--out=%s.txt" % cls.user_certprofile])
+ master.run_command("sed -i \"s/profileId=%s/profileId=%s/\" %s.txt" % (
+ cls.service_certprofile, cls.user_certprofile,
+ cls.user_certprofile)
+ )
+ master.run_command(['ipa', 'certprofile-import', cls.user_certprofile,
+ "--file=%s.txt" % cls.user_certprofile,
+ '--store=true', '--desc="User Certs"'])
+
+ cls.reqdir = os.path.join(master.config.test_dir, "certs")
+ cls.reqfile1 = os.path.join(cls.reqdir, "test1.csr")
+ cls.reqfile2 = os.path.join(cls.reqdir, "test2.csr")
+ cls.pwname = os.path.join(cls.reqdir, "pwd")
+
+ # Create a NSS database folder
+ master.run_command(['mkdir', cls.reqdir], raiseonerr=False)
+ # Create an empty password file
+ master.run_command(["touch", cls.pwname], raiseonerr=False)
+
+ # Initialize NSS database
+ tasks.run_certutil(master, ["-N", "-f", cls.pwname], cls.reqdir)
+ # Now generate self-signed certs for a windows user
+ stdin_text = string.digits+string.letters[2:] + '\n'
+ tasks.run_certutil(master, ['-S', '-s',
+ "cn=%s,dc=ad,dc=test" % cls.adcert1, '-n',
+ cls.adcert1, '-x', '-t', 'CT,C,C', '-v',
+ '120', '-m', '1234'],
+ cls.reqdir, stdin=stdin_text)
+ tasks.run_certutil(master, ['-S', '-s',
+ "cn=%s,dc=ad,dc=test" % cls.adcert2, '-n',
+ cls.adcert2, '-x', '-t', 'CT,C,C', '-v',
+ '120', '-m', '1234'],
+ cls.reqdir, stdin=stdin_text)
+
+ # Export the previously generated cert
+ tasks.run_certutil(master, ['-L', '-n', cls.adcert1, '-a', '>',
+ cls.adcert1_file], cls.reqdir)
+ tasks.run_certutil(master, ['-L', '-n', cls.adcert2, '-a', '>',
+ cls.adcert2_file], cls.reqdir)
+ cls.cert1_base64 = cls.master.run_command(
+ "openssl x509 -outform der -in %s | base64 -w 0" % cls.adcert1_file
+ ).stdout_text
+ cls.cert2_base64 = cls.master.run_command(
+ "openssl x509 -outform der -in %s | base64 -w 0" % cls.adcert2_file
+ ).stdout_text
+
+ def test_certs_in_idoverrides_ad_users(self):
+ master = self.master
+ master.run_command(['ipa', 'idoverrideuser-add',
+ self.adview, self.aduser])
+ master.run_command(['ipa', 'idoverrideuser-add-cert',
+ self.adview, self.aduser,
+ "--certificate=%s" % self.cert1_base64])
+ result = master.run_command(['ipa', 'idoverrideuser-add-cert',
+ self.adview, self.aduser,
+ "--certificate=%s" % self.cert1_base64],
+ raiseonerr=False)
+ assert_error(result, "already contains one or more values")
+
+ result1 = master.run_command(['ipa', 'idoverrideuser-show',
+ self.adview, self.aduser])
+ assert(self.cert1_base64 in result1.stdout_text), (
+ "idoverrideuser-show does not show the user certificate")
+ master.run_command(['ipa', 'idoverrideuser-add-cert',
+ self.adview, self.aduser,
+ "--certificate=%s" % self.cert2_base64])
+ result2 = master.run_command(['ipa', 'idoverrideuser-show',
+ self.adview, self.aduser])
+ assert(self.cert2_base64 in result2.stdout_text), (
+ "idoverrideuser-show does not show all user certificates")
+ master.run_command(['ipa', 'idoverrideuser-remove-cert',
+ self.adview, self.aduser,
+ "--certificate=%s" % self.cert2_base64])
--
1.8.3.1
--
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
