[Freeipa-devel] [PATCH 42-47][tests] RFE: Allow users to authenticate with alternative names

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky