On 07/25/2016 02:05 PM, Milan Kubík wrote:
On 07/25/2016 01:53 PM, Milan Kubík wrote:

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


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:

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

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

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.

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



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.


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


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

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.

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

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to