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

[Martin Babinsky]

On 07/25/2016 02:05 PM, Milan Kubík wrote:
> On 07/25/2016 01:53 PM, Milan Kubík wrote:
>> Hi,
>>
>> I'm sending the tests for kerberos principal aliases rfe. The tests
>> are implemented according to test plan [1] sent earlier.
>> Some of the patches implement modifications and extensions to previous
>> code to allow implement the tests themselves.
>>
>> The patches can be cloned also from github [2].
>>
>> [1]: http://www.freeipa.org/page/V4/Kerberos_principal_aliases/Test_Plan
>> [2]: https://github.com/apophys/freeipa/tree/krb5-principal-aliases-test
>>
>> Cheers,
>>
>>
>>
>
> Self nack for 0047, the ldapconn fixture is not needed. New patch attached.
> Git repo updated (force-push).
>
> --
> Milan Kubik
>
>
>

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:

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.

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.

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.

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

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.

PATCH 45:

It would be nice if you could add '-E' option in the same way to 
indicate enterprise principal name resolution.

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.

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.

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.

-- 
Martin^3 Babinsky