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
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
