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

[Martin Babinsky]

On 07/28/2016 01:44 PM, Milan Kubík wrote:
> 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
>

Thanks, ACK.

-- 
Martin^3 Babinsky