[Freeipa-devel] [PATCH 0012-0019] CA ACL tracker and functional test

[Martin Basti]

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

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?

3)
# noqa
Please remove "# noqa" commets from commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151019/9afb6a82/attachment.htm>