[Freeipa-devel] [PATCH 42-44, 48-51][tests] RFE: Allow users to authenticate with alternative names

Thu Jul 28 11:44:49 UTC 2016

On 07/28/2016 12:51 PM, Martin Babinsky wrote:
> On 07/27/2016 11:54 AM, Milan Kubík wrote:
>>
>>>>
>>>>
>>>
>>> Hi Milan,
>>>
>>> the tests seem to work as expected except
>>> `test_enterprise_principal_UPN_overlap_without_additional_suffix`
>>> which crashes on #6099. I have a few comments, however:
>> This is a test that hits a known bug. I have added an expected fail
>> marker for it.
>>>
>>> Patch 42:
>>>
>>> 1.)
>>> +class KerberosAliasMixin:
>>>
>>> make sure the class inherits from 'object' so that it behaves like
>>> new-style class in Python 2.
>>>
>>> Also, I think that 'check_for_tracker' method is slightly redundant.
>>> If somebody would use the mixin directly, then he will still get
>>> "NotImplementedError" exceptions and see that he probably did
>>> something wrong.
>>>
>>> If you really really want to treat to prevent the instantiation of the
>>> class, then use ABC module to turn it into proper abstract class.
>>>
>>> But in this case it should IMHO be enough that you explained the class
>>> usage in the module docstring.
>> Ok. Fixed the inheritance and removed the check for Tracker. The check
>> for krbprincipalname attribute has been kept.
>>>
>>> 2.)
>>> +    def _make_add_alias_cmd(self):
>>> +        raise RuntimeError("The _make_add_alias_cmd method "
>>> +                           "is not implemented.")
>>> +
>>> +    def _make_remove_alias_cmd(self):
>>> +        raise RuntimeError("The _make_remove_alias_cmd method "
>>> +                           "is not implemented."
>>>
>>> Abstract methods should raise "NotImplementedError" exception.
>>>
>> Fixed.
>>> 3.)
>>> is the 'options=None' kwarg in {add,remove}_principals methods
>>> necessary? I didn't see it anywhere in the later commits so I guess
>>> you can safely remove it. Better yet, you can just replace it with
>>> '**options' since all it does is to pass options to the
>>> `*-add/remove-principal` commands.
>>>
>> Fixed to **options.
>>> 4.)
>>> a nitpick but IIRC the mixin class should be listed before the actual
>>> hierarchy base so that MRO works as expected.
>>>
>>> So instead
>>>
>>> +class UserTracker(Tracker, KerberosAliasMixin):
>>>
>>> It should say
>>> +class UserTracker(KerberosAliasMixin, Tracker):
>> Fixed in all three classes.
>>>
>>> PATCH 43: LGTM
>>>
>>> PATCH 44:
>>>
>>> Please do not create any more utility modules. Either put it to
>>> existing 'ipatests/util.py' or better yet, rename the module to
>>> something like 'mock_trust' since the scope of it is to provide mocked
>>> trust entries.
>> Moved the functions from test_range_plugin.py module to a new mock_trust
>> module. The fix for the range plugin test introduced a new commit here.
>>>
>>> PATCH 45:
>>>
>>> It would be nice if you could add '-E' option in the same way to
>>> indicate enterprise principal name resolution.
>> Done.
>>>
>>> PATCH 46: LGTM
>>> PATCH 47:
>>>
>>> 1.)
>>> +from ipatests.test_xmlrpc.test_range_plugin import (
>>> +    get_trust_dn, get_trusted_dom_dict, encode_mockldap_value)
>>>
>>> Since you already introduced a module grouping all trust-mocking
>>> machinery in patch 44, you could extract these functions and put it
>>> there to improve code reuse.
>> Fixed.
>>>
>>> 2.)
>>> I am not a big fan of duplicate code in 'trusted_domain' and
>>> 'trusted_domain_with_suffix' fixtures. Use some module-level mapping
>>> for common attributes and add more specific attributes per fixture.
>> Fixed
>>>
>>> 3.)
>>> I would like to expand the test cases for AD realm namespace overlap
>>> checks. We should reject enterprise principals with suffixes
>>> coinciding with realm names, NETBIOS names, and UPN suffixes and I
>>> would like to test all three cases to get a full coverage.
>>>
>> Extended with explicit checks fot rhe AD realm and NETBIOS name by
>> constructing the enterprise principal from corresponding LDAP 
>> attributes.
>>
>> The fixed and rebased version of the commits is in my repo [1].
>>
>> [1]: https://github.com/apophys/freeipa/tree/krb5-principal-aliases-test
>>
>> Regards
>>
>
> Tests works and code is ok, however you will need to create a separate 
> ticket to your patches, since it is not allowed to add fixes to 
> tickets in closed milestones. Sorry that I didn't notice it earlier.
>
> cond-ACK if you create ticket and remove the xfail from 
> `test_enterprise_principal_overlap_with_AD_realm` test case as the fix 
> for #6099 was pushed to master.
>

Hi,

thanks for the review. The xfail marker was removed. The commit messages 
now reffer to new ticket [1].
Since the changes during review introduced new commit in the sequence, I 
have abandoned the patches 45 to 47 and renumbered them (with the new 
one) from 48 onwards.

[1]: https://fedorahosted.org/freeipa/ticket/6142

Regards

-- 
Milan Kubik

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0042-1-ipatests-Add-tracker-class-for-kerberos-principal-al.patch
Type: text/x-patch
Size: 10049 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/5d318e4d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0043-1-ipatests-Extend-the-MockLDAP-utility-class.patch
Type: text/x-patch
Size: 1340 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/5d318e4d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0044-1-ipatests-Provide-a-context-manager-for-mocking-a-tru.patch
Type: text/x-patch
Size: 2798 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/5d318e4d/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0048-ipatests-Move-trust-mock-helper-functions-to-a-separ.patch
Type: text/x-patch
Size: 5253 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/5d318e4d/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0049-ipapython-Extend-kinit_password-to-support-principal.patch
Type: text/x-patch
Size: 1836 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/5d318e4d/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0050-ipatests-Allow-change_principal-context-manager-to-u.patch
Type: text/x-patch
Size: 1587 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/5d318e4d/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkubik-0051-ipatests-Add-kerberos-principal-alias-tests.patch
Type: text/x-patch
Size: 10839 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/5d318e4d/attachment-0006.bin>