On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,

attaching patch with some basic coverage for external trust feature. Bit
more detailed info in commit message.

Since the feature requires me to run commands previously used only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable me to
reuse existing function without copy-pasting them for one variable change.


Lenka




Hi Lenka,

I have a few comments:

1.)

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False):

This modification seems extremely kludgy to me.

I would rather change the function signature to:

-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):

and pass the domain name itself to the function instead of doing magic inside. The same goes with `remove trust_with_ad`. You may want to fix the calls to them in existing code so that the domain is passed instead of the whole trust object.

2.)

+def sync_time_hostname(host, srv_hostname):
+    """
+    Syncs time with the remote server given by its hostname.
+    Please note that this function leaves ntpd stopped.
+    """
+    host.run_command(['systemctl', 'stop', 'ntpd'])
+    host.run_command(['ntpdate', srv_hostname])
+
+

this looks like a duplicate of the function above it, why is it even needed?

3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+    """
+    Test establishing external trust with subdomain
+    """
+
+    @classmethod
+    def install(cls, mh):
+        super(ADTrustBase, cls).install(mh)
+        cls.ad = cls.ad_domains[0].ads[0]
+        cls.install_adtrust()
+        cls.check_sid_generation()
+
+        # Determine whether the subdomain AD is available
+        try:
+            child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+ cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:])
+            cls.ad_subdomain_hostname = child_ad.hostname
+        except LookupError:
+            cls.ad_subdomain = None
+
+        cls.configure_dns_and_time()

Please use proper OOP practices when overriding the behavior of the base test class.

For starters you could rewrite the install method like this:

+    @classmethod
+    def install(cls, mh):
+        # Determine whether the subdomain AD is available
+        try:
+            cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+ cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:])
+            cls.ad_subdomain_hostname = child_ad.hostname
+        except LookupError:
+ raise nose.SkipTest("AFAIK this will skip the whole test class")
+        super(ADTrustBase, cls).install(mh)

with child_ad stored as class attribute, you can override `configure_dns_and_time`:
+    def configure_dns_and_time(cls):
+        tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
# no need to re-implement the function just to get to the child AD DC hostname now
+        tasks.sync_time(cls.master, cls.child_ad.hostname)

You can then use this class as an intermediary in the hierarchy and inherit the other external/non-external trust test classes from it, since most setup method in them are just copy-pasted from this one.

Hi,
thanks for review, fixed patch attached.

Lenka
From ae860f97d701b296f52de1eb0c84dd43d1429f76 Mon Sep 17 00:00:00 2001
From: Lenka Doudova <ldoud...@redhat.com>
Date: Thu, 30 Jun 2016 12:23:02 +0200
Subject: [PATCH] Tests: External trust

Provides basic coverage for external trust feature.
Test cases:
1. verify an external trust with AD subdomain can be established
   - verify only one trustdomain is listed
   - verify subdomain users are resolvable
   - verify trust can be deleted
2. verify non-external trust with AD subdomain cannot be established
3. verify an external trust with AD forest root domain can be established
   - verify that even if AD subdomain is specified, it is not associated with the trust
   - verify trust can be deleted

https://fedorahosted.org/freeipa/ticket/5743
---
 ipatests/test_integration/tasks.py      |  11 +-
 ipatests/test_integration/test_trust.py | 218 ++++++++++++++++++++++++++++----
 2 files changed, 197 insertions(+), 32 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 5be7cdae3ac777bbf0fc52e6c511969e9fabcd72..48ef6f1963e2405d7db9a18799e9796b7f10bfe5 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -443,7 +443,7 @@ def configure_dns_for_trust(master, ad):
     pass
 
 
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
     """
     Establishes trust with Active Directory. Trust type is detected depending
     on the presence of SfU (Services for Unix) support on the AD.
@@ -461,9 +461,10 @@ def establish_trust_with_ad(master, ad, extra_args=()):
     kinit_admin(master)
     master.run_command(['klist'])
     master.run_command(['smbcontrol', 'all', 'debug', '100'])
+
     util.run_repeatedly(master,
                         ['ipa', 'trust-add',
-                         '--type', 'ad', ad.domain.name,
+                         '--type', 'ad', ad_domain,
                          '--admin', 'Administrator',
                          '--password'] + list(extra_args),
                         stdin_text=master.config.ad_admin_password)
@@ -471,7 +472,7 @@ def establish_trust_with_ad(master, ad, extra_args=()):
     clear_sssd_cache(master)
 
 
-def remove_trust_with_ad(master, ad):
+def remove_trust_with_ad(master, ad_domain):
     """
     Removes trust with Active Directory. Also removes the associated ID range.
     """
@@ -479,10 +480,10 @@ def remove_trust_with_ad(master, ad):
     kinit_admin(master)
 
     # Remove the trust
-    master.run_command(['ipa', 'trust-del', ad.domain.name])
+    master.run_command(['ipa', 'trust-del', ad_domain])
 
     # Remove the range
-    range_name = ad.domain.name.upper() + '_id_range'
+    range_name = ad_domain.upper() + '_id_range'
     master.run_command(['ipa', 'idrange-del', range_name])
 
 
diff --git a/ipatests/test_integration/test_trust.py b/ipatests/test_integration/test_trust.py
index 772a50842f7c251b78d68fd4bd3c91668f580d50..d662e80727b6eab3df93166d35ddbaea6a0f6f7a 100644
--- a/ipatests/test_integration/test_trust.py
+++ b/ipatests/test_integration/test_trust.py
@@ -23,6 +23,7 @@ import re
 from ipatests.test_integration.base import IntegrationTest
 from ipatests.test_integration import tasks
 from ipatests.test_integration import util
+from ipaplatform.paths import paths
 
 
 class ADTrustBase(IntegrationTest):
@@ -36,18 +37,20 @@ class ADTrustBase(IntegrationTest):
     def install(cls, mh):
         super(ADTrustBase, cls).install(mh)
         cls.ad = cls.ad_domains[0].ads[0]
+        cls.ad_domain = cls.ad.domain.name
         cls.install_adtrust()
         cls.check_sid_generation()
-        cls.configure_dns_and_time()
 
         # Determine whether the subdomain AD is available
         try:
-            child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+            cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
             cls.ad_subdomain = '.'.join(
-                                   child_ad.hostname.split('.')[1:])
+                                   cls.child_ad.hostname.split('.')[1:])
         except LookupError:
             cls.ad_subdomain = None
 
+        cls.configure_dns_and_time()
+
     @classmethod
     def install_adtrust(cls):
         """Test adtrust support installation"""
@@ -71,13 +74,13 @@ class ADTrustBase(IntegrationTest):
 
     @classmethod
     def configure_dns_and_time(cls):
-        tasks.configure_dns_for_trust(cls.master, cls.ad)
+        tasks.configure_dns_for_trust(cls.master, cls.ad_domain)
         tasks.sync_time(cls.master, cls.ad)
 
     def test_establish_trust(self):
         """Tests establishing trust with Active Directory"""
 
-        tasks.establish_trust_with_ad(self.master, self.ad,
+        tasks.establish_trust_with_ad(self.master, self.ad_domain,
             extra_args=['--range-type', 'ipa-ad-trust'])
 
     def test_all_trustdomains_found(self):
@@ -90,10 +93,10 @@ class ADTrustBase(IntegrationTest):
 
         result = self.master.run_command(['ipa',
                                           'trustdomain-find',
-                                           self.ad.domain.name])
+                                          self.ad_domain])
 
         # Check that both trustdomains appear in the result
-        assert self.ad.domain.name in result.stdout_text
+        assert self.ad_domain in result.stdout_text
         assert self.ad_subdomain in result.stdout_text
 
 
@@ -103,7 +106,7 @@ class TestBasicADTrust(ADTrustBase):
     def test_range_properties_in_nonposix_trust(self):
         """Check the properties of the created range"""
 
-        range_name = self.ad.domain.name.upper() + '_id_range'
+        range_name = self.ad_domain.upper() + '_id_range'
         result = self.master.run_command(['ipa', 'idrange-show', range_name,
                                           '--all', '--raw'])
 
@@ -117,20 +120,20 @@ class TestBasicADTrust(ADTrustBase):
         """Check that user has SID-generated UID"""
 
         # Using domain name since it is lowercased realm name for AD domains
-        testuser = 'testuser@%s' % self.ad.domain.name
+        testuser = 'testuser@%s' % self.ad_domain
         result = self.master.run_command(['getent', 'passwd', testuser])
 
         # This regex checks that Test User does not have UID 10042 nor belongs
         # to the group with GID 10047
         testuser_regex = "^testuser@%s:\*:(?!10042)(\d+):(?!10047)(\d+):"\
                          "Test User:/home/%s/testuser:/bin/sh$"\
-                         % (re.escape(self.ad.domain.name),
-                            re.escape(self.ad.domain.name))
+                         % (re.escape(self.ad_domain),
+                            re.escape(self.ad_domain))
 
         assert re.search(testuser_regex, result.stdout_text)
 
     def test_remove_nonposix_trust(self):
-        tasks.remove_trust_with_ad(self.master, self.ad)
+        tasks.remove_trust_with_ad(self.master, self.ad_domain)
         tasks.clear_sssd_cache(self.master)
 
 
@@ -139,12 +142,12 @@ class TestPosixADTrust(ADTrustBase):
 
     def test_establish_trust(self):
         # Not specifying the --range-type directly, it should be detected
-        tasks.establish_trust_with_ad(self.master, self.ad)
+        tasks.establish_trust_with_ad(self.master, self.ad_domain)
 
     def test_range_properties_in_posix_trust(self):
         # Check the properties of the created range
 
-        range_name = self.ad.domain.name.upper() + '_id_range'
+        range_name = self.ad_domain.upper() + '_id_range'
 
         result = self.master.run_command(['ipa', 'idrange-show', range_name,
                                           '--all', '--raw'])
@@ -160,13 +163,12 @@ class TestPosixADTrust(ADTrustBase):
         # Check that user has AD-defined UID
 
         # Using domain name since it is lowercased realm name for AD domains
-        testuser = 'testuser@%s' % self.ad.domain.name
+        testuser = 'testuser@%s' % self.ad_domain
         result = self.master.run_command(['getent', 'passwd', testuser])
 
         testuser_stdout = "testuser@%s:*:10042:10047:"\
                           "Test User:/home/%s/testuser:/bin/sh"\
-                          % (self.ad.domain.name,
-                             self.ad.domain.name)
+                          % (self.ad_domain, self.ad_domain)
 
         assert testuser_stdout in result.stdout_text
 
@@ -174,7 +176,7 @@ class TestPosixADTrust(ADTrustBase):
         # Check that user has AD-defined UID
 
         # Using domain name since it is lowercased realm name for AD domains
-        nonposixuser = 'nonposixuser@%s' % self.ad.domain.name
+        nonposixuser = 'nonposixuser@%s' % self.ad_domain
         result = self.master.run_command(['getent', 'passwd', nonposixuser],
                                          raiseonerr=False)
 
@@ -182,7 +184,7 @@ class TestPosixADTrust(ADTrustBase):
         assert result.returncode == 2
 
     def test_remove_trust_with_posix_attributes(self):
-        tasks.remove_trust_with_ad(self.master, self.ad)
+        tasks.remove_trust_with_ad(self.master, self.ad_domain)
         tasks.clear_sssd_cache(self.master)
 
 
@@ -193,7 +195,7 @@ class TestEnforcedPosixADTrust(TestPosixADTrust):
     """
 
     def test_establish_trust_with_posix_attributes(self):
-        tasks.establish_trust_with_ad(self.master, self.ad,
+        tasks.establish_trust_with_ad(self.master, self.ad_domain,
             extra_args=['--range-type', 'ipa-ad-trust-posix'])
 
 
@@ -214,13 +216,175 @@ class TestInvalidRangeTypes(ADTrustBase):
             tasks.kinit_admin(self.master)
 
             result = self.master.run_command(
-                               ['ipa', 'trust-add',
-                               '--type', 'ad', self.ad.domain.name,
-                               '--admin', 'Administrator',
-                               '--range-type', range_type,
-                               '--password'],
-                               raiseonerr=False,
-                               stdin_text=self.master.config.ad_admin_password)
+                ['ipa', 'trust-add', '--type', 'ad', self.ad_domain, '--admin',
+                 'Administrator', '--range-type', range_type, '--password'],
+                raiseonerr=False,
+                stdin_text=self.master.config.ad_admin_password)
 
             # The trust-add command is supposed to fail
             assert result.returncode == 1
+
+
+class TestExternalTrustWithSubdomain(ADTrustBase):
+    """
+    Test establishing external trust with subdomain
+    """
+
+    @classmethod
+    def configure_dns_and_time(cls):
+        tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
+        tasks.sync_time_hostname(cls.master, cls.child_ad)
+
+    def test_establish_trust(self):
+        """ Tests establishing external trust with Active Directory """
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        tasks.establish_trust_with_ad(
+            self.master, self.ad_subdomain,
+            extra_args=['--range-type', 'ipa-ad-trust', '--external'],
+            subdomain=True)
+
+    def test_all_trustdomains_found(self):
+        """ Test that only one trustdomain is found """
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        result = self.master.run_command(['ipa', 'trustdomain-find',
+                                          self.ad_subdomain])
+
+        assert self.ad_subdomain in result.stdout_text
+        assert "Number of entries returned 1" in result.stdout_text
+
+    def test_user_gid_uid_resolution_in_nonposix_trust(self):
+        """ Check that user has SID-generated UID """
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        testuser = 'subdomaintestuser@{0}'.format(self.ad_subdomain)
+        result = self.master.run_command(['getent', 'passwd', testuser])
+
+        testuser_regex = "^subdomaintestuser@{0}:\*:(?!10042)(\d+):"\
+                         "(?!)10047(\d+):Subdomain TestUser:"\
+                         "/home/{1}/subdomaintestuser:/bin/sh$".format(
+                             re.escape(self.ad_subdomain),
+                             re.escape(self.ad_subdomain))
+
+        assert re.search(testuser_regex, result.stdout_text)
+
+    def test_remove_nonposix_trust(self):
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        tasks.remove_trust_with_ad(self.master, self.ad_subdomain,
+                                   subdomain=True)
+        tasks.clear_sssd_cache(self.master)
+
+
+class TestNonexternalTrustWithSubdomain(IntegrationTest):
+    """
+    Tests that a non-external trust to a subdomain cannot be established
+    """
+    topology = 'line'
+    num_ad_domains = 1
+    optional_extra_roles = ['ad_subdomain']
+
+    @classmethod
+    def install(cls, mh):
+        super(TestNonexternalTrustWithSubdomain, cls).install(mh)
+        cls.ad = cls.ad_domains[0].ads[0]
+        cls.ad_domain = cls.ad.domain.name
+        cls.install_adtrust()
+        cls.check_sid_generation()
+
+        # Determine whether the subdomain AD is available
+        try:
+            cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+            cls.ad_subdomain = '.'.join(cls.child_ad.hostname.split('.')[1:])
+        except LookupError:
+            cls.ad_subdomain = None
+
+        cls.configure_dns_and_time()
+
+    @classmethod
+    def install_adtrust(cls):
+        tasks.install_adtrust(cls.master)
+
+    @classmethod
+    def check_sid_generation(cls):
+        """Test SID generation"""
+
+        command = ['ipa', 'user-show', 'admin', '--all', '--raw']
+
+        _sid_identifier_authority = '(0x[0-9a-f]{1,12}|[0-9]{1,10})'
+        sid_regex = 'S-1-5-21-%(idauth)s-%(idauth)s-%(idauth)s'\
+                    % dict(idauth=_sid_identifier_authority)
+        stdout_re = re.escape('  ipaNTSecurityIdentifier: ') + sid_regex
+
+        util.run_repeatedly(cls.master, command,
+                            test=lambda x: re.search(stdout_re, x))
+
+    @classmethod
+    def configure_dns_and_time(cls):
+        tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
+        tasks.sync_time_hostname(cls.master, cls.child_ad)
+
+    def test_establish_trust(self):
+        """ Tests establishing non-external trust with Active Directory """
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        self.master.run_command(['kinit', '-kt', paths.IPA_KEYTAB,
+                                 'HTTP/%s' % self.master.hostname])
+        self.master.run_command(['systemctl', 'restart', 'krb5kdc.service'])
+        self.master.run_command(['kdestroy', '-A'])
+
+        tasks.kinit_admin(self.master)
+        self.master.run_command(['klist'])
+        self.master.run_command(['smbcontrol', 'all', 'debug', '100'])
+
+        result = self.master.run_command([
+            'ipa', 'trust-add', '--type', 'ad', self.ad_subdomain, '--admin',
+            'Administrator', '--password', '--range-type', 'ipa-ad-trust'
+            ], stdin_text=self.master.config.ad_admin_password,
+            raiseonerr=False)
+
+        assert result != 0
+        assert "Domain '{0}' is not a root domain".format(self.ad_subdomain) \
+            in result.stderr_text
+
+
+class TestExternalTrustWithRootDomain(ADTrustBase):
+    """
+    Test establishing external trust with root domain
+    Main purpose of this test is to verify that subdomains are not
+    associated with the external trust, hence all tests are skipped
+    if no subdomain is specified.
+    """
+
+    def test_establish_trust(self):
+        """ Tests establishing external trust with Active Directory """
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        tasks.establish_trust_with_ad(
+            self.master, self.ad_domain,
+            extra_args=['--range-type', 'ipa-ad-trust', '--external'])
+
+    def test_all_trustdomains_found(self):
+        """ Test that only one trustdomain is found """
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        result = self.master.run_command(['ipa', 'trustdomain-find',
+                                          self.ad_domain])
+
+        assert self.ad_domain in result.stdout_text
+        assert "Number of entries returned 1" in result.stdout_text
+
+    def test_remove_nonposix_trust(self):
+        if self.ad_subdomain is None:
+            raise nose.SkipTest('AD subdomain is not available.')
+
+        tasks.remove_trust_with_ad(self.master, self.ad_domain)
+        tasks.clear_sssd_cache(self.master)
-- 
2.7.4

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