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

[Milan Kubík]

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

-- 
Milan Kubik