On 20.10.2015 10:00, Milan Kubík wrote:
On 10/19/2015 01:38 PM, Martin Basti wrote:


On 16.10.2015 15:43, Milan Kubík wrote:
On 09/30/2015 02:47 PM, Martin Basti wrote:









On 09/24/2015 02:49 PM, Milan Kubík
wrote:


Hi
all,




an update for CA ACL tests!




I, with help from M. Babinsky, managed to find a way how to change
the identity during acceptance cest run, which allows


to test CA ACLs (and perhaps other areas with some form of access
controll).




This allowed me to write a test for CA ACLs and certificate
profiles that checks if the ACL/profile is being used and
enforced.


The first several tests are based on Fraser's blogpost using SMIME
profile [1].




The master and ipa-4-2 branches diverged a bit, so I had to change
two commits when rebasing to ipa-4-2 branch.




Commits should be applied in the order (including rebased patches
I sent in an earlier email):




master:


    * 12 - 17




ipa-4-2:


    * 18, 13 - 15, 19, 17




For convenience:


patches on top of master:
https://github.com/apophys/freeipa/tree/acl-profile-functional


patches on top of ipa-4-2:
https://github.com/apophys/freeipa/tree/acl-42






[1]:
https://blog-ftweedal.rhcloud.com/2015/08/user-certificates-and-custom-profiles-with-freeipa-4-2/



Cheers,


Milan










NACK



0)

rpm file does not contain test_xmlrpc/data directory, please modify
setup.py.in.



1)

Code contains to much todo for my taste.



2)

Please do not use filter function, use dict comprehension.








Hi,

updated patches and the numbering mess somehow curbed. The patches are rebased on top of current master and ipa-4-2.

0) fixed by 0021

1) docs for tracker extended, added more test cases

2) changed


--
Milan Kubik
I have a few comments:

1)
+# TODO: rewrite these into Tracker instances
+@pytest.fixture(scope='class')
+def smime_user(request):
+    api.Command.user_add(uid=u'alice', givenname=u'Alice', sn=u'SMIME',
+                         userpassword=u'Change123')
+
+    unlock_principal_password('alice', 'Change123', 'Secret123')
+
+    def fin():
+        api.Command.user_del(u'alice')
+    request.addfinalizer(fin)
+
+    return u'alice'

I do not like hardcoded password value, as this password is used in many places in the test, I sugest to use a module variable
Done

2)
+class TestSignWithChangedProfile(XMLRPC_test):
+    """ Test to verify that the updated profile is used."""
+    pass  # import invalid profile, try to sign, expect fail

IMO something is missing here, a test maybe?

Done by using profile with constraint that CSR cannot meet.
3)
# noqa
Please remove "# noqa" commets from commits

Done.

--
Milan Kubik

NACK

1)
I still see many hardcoded passwords in the code
with change_principal(smime_user, "Secret123"):

2)
Also the 'alice' username can be extracted to module variable instead hardcoding

3)
File alice.conf.tmpl can be generalized to be used for more users, replace alice in template to {username} and in code replace this variable with alice, also do not forgot rename template to something more general


-- 
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

Reply via email to