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

2016-07-28 Thread Martin Babinsky 

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.


--
Martin^3 Babinsky

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

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

2016-07-27 Thread 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

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

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

2016-07-26 Thread 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

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

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

2016-07-25 Thread Milan Kubík 

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

From 5dd5fe5d0ccc921949dedb2f3e2497344f87e493 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= 
Date: Fri, 22 Jul 2016 17:25:06 +0200
Subject: [PATCH] ipatests: Add kerberos principal alias tests

Add tests for alias manipulation, tests authentication and several
error scenarios.

https://fedorahosted.org/freeipa/ticket/3864
https://fedorahosted.org/freeipa/ticket/5413
https://fedorahosted.org/freeipa/ticket/6099
---
 .../test_xmlrpc/test_kerberos_principal_aliases.py | 261 +
 1 file changed, 261 insertions(+)
 create mode 100644 ipatests/test_xmlrpc/test_kerberos_principal_aliases.py

diff --git a/ipatests/test_xmlrpc/test_kerberos_principal_aliases.py b/ipatests/test_xmlrpc/test_kerberos_principal_aliases.py
new file mode 100644
index ..11a69e6664a219c6f6b682266ff1b75327ae0046
--- /dev/null
+++ b/ipatests/test_xmlrpc/test_kerberos_principal_aliases.py
@@ -0,0 +1,261 @@
+# coding: utf-8
+#
+# Copyright (C) 2016  FreeIPA Contributors see COPYING for license
+#
+import ldap
+import pytest
+
+from ipalib import errors, api
+from ipapython import ipautil
+from ipaplatform.paths import paths
+
+from ipatests.util import MockLDAP
+from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test
+from ipatests.test_xmlrpc.tracker.user_plugin import UserTracker
+from ipatests.test_xmlrpc.tracker.host_plugin import HostTracker
+from ipatests.test_xmlrpc.tracker.service_plugin import ServiceTracker
+from ipatests.test_xmlrpc.test_range_plugin import (
+get_trust_dn, get_trusted_dom_dict, encode_mockldap_value)
+from ipatests.test_xmlrpc.utils import mocked_trust_containers
+from ipatests.util import unlock_principal_password, change_principal
+
+
+@pytest.yield_fixture
+def trusted_domain():
+"""Fixture providing mocked AD trust entries
+
+The fixture yields after creating a mock of AD trust
+entries in the directory server. After the test, the entries
+are deleted from the directory.
+"""
+trusted_dom = u'trusted.domain.net'
+trusted_dom_dn = get_trust_dn(trusted_dom)
+trusted_dom_sid = u'S-1-5-21-2997650941-1802118864-3094776726'
+
+trusted_dom_add = get_trusted_dom_dict(trusted_dom, trusted_dom_sid)
+
+# Write the changes
+with mocked_trust_containers(), MockLDAP() as ldap:
+ldap.add_entry(trusted_dom_dn, trusted_dom_add)
+yield trusted_dom
+ldap.del_entry(trusted_dom_dn)
+
+
+@pytest.yield_fixture
+def trusted_domain_with_suffix():
+"""Fixture providing mocked AD trust entries
+
+The fixture yields after creating a mock of AD trust
+entries in the directory server. After the test, the entries
+are deleted from the directory.
+"""
+trusted_dom = u'trusted.domain.net'
+trusted_dom_dn = get_trust_dn(trusted_dom)
+trusted_dom_sid = u'S-1-5-21-2997650941-1802118864-3094776726'
+
+trusted_dom_add = get_trusted_dom_dict(trusted_dom, trusted_dom_sid)
+trusted_dom_add['ipaNTAdditionalSuffixes'] = (
+encode_mockldap_value(trusted_dom))
+
+# Write the changes
+with mocked_trust_containers(),  MockLDAP() as ldap:
+ldap.add_entry(trusted_dom_dn, trusted_dom_add)
+yield trusted_dom
+ldap.del_entry(trusted_dom_dn)
+
+
+@pytest.fixture(scope='function')
+def krbalias_user(request):
+tracker = UserTracker(u'krbalias_user', u'krbalias', u'test')
+
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='function')
+def krbalias_user_c(request):
+tracker = UserTracker(u'krbalias_user_conflict', u'krbalias', u'test')
+
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='function')
+def krbalias_host(request):
+tracker = HostTracker(u'testhost-krb')
+
+return tracker.make_fixture(request)
+
+
+@pytest.fixture
+def krb_service_host(request):
+tracker = HostTracker(u'krb-srv-host')
+
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='function')
+def krbalias_service(request, krb_service_host):
+krb_service_host.ensure_exists()
+
+tracker = ServiceTracker(name=u'SRV1', host_fqdn=krb_service_host.name)
+
+return tracker.make_fixture(request)
+
+
+@pytest.fixture
+def ldapservice(request):
+tracker = ServiceTracker(
+name=u'ldap', host_fqdn=api.env.host, options={'has_keytab': True})
+
+