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

[Martin Basti]

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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151020/f84836e8/attachment.htm>