On 28.07.2016 18:10, Martin Babinsky wrote:
On 07/28/2016 01:44 PM, Milan Kubík wrote:
On 07/28/2016 12:51 PM, Martin Babinsky wrote:
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.


Hi,

thanks for the review. The xfail marker was removed. The commit messages
now reffer to new ticket [1].
Since the changes during review introduced new commit in the sequence, I
have abandoned the patches 45 to 47 and renumbered them (with the new
one) from 48 onwards.

[1]: https://fedorahosted.org/freeipa/ticket/6142

Regards


Thanks, ACK.


master:
* 5582d1df3213a0727e313d543ee560a09d0cdff8 ipatests: Add tracker class for kerberos principal aliases * dde1240f5d71f3a8c50226a720af6f1000a35be1 ipatests: Extend the MockLDAP utility class * 7c03708734ad7cb8f1a6edd39817212794b5aabd ipatests: Provide a context manager for mocking a trust in RPC tests * ddb7a08084d69a119abdd39a3c82113bb8586db6 ipatests: Move trust mock helper functions to a separate module * 8e83b9715a04fab8d7864b6e02e1210df885119c ipapython: Extend kinit_password to support principal canonicalization * e17ec08daef521b33dcc5db131f36f4269edcdb2 ipatests: Allow change_principal context manager to use canonicalization * dd2e3a5547f7305cb40d94b00f7b8b14b9a73885 ipatests: Add kerberos principal alias tests

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

Reply via email to