Hi Martin,

Thanks for the review! The new version is attached

On 03/24/2016 06:08 PM, Martin Babinsky wrote:
> On 03/21/2016 01:51 PM, Oleg Fayans wrote:
>>
>>
>>
> Hi Oleg,
> 
> I have a few comments:
> 
> 1.)
> please make the commit message more clear, briefly describe what kind of
> test cases were added to the suite and maybe add a link to the test plan.

Done

> 
> 2.)
> I see negative test scenarios for attempting to issue
> 'ipa-csreplica-manage connect' and 'disconnect' under domain level 1.
> However, for full coverage there should be also a negative test case for
> 'ipa-csreplica-manage del' which should also issue error in domain level
> 1, see
> https://git.fedorahosted.org/cgit/freeipa.git/commit/install/tools/ipa-csreplica-manage?h=ipa-4-3&id=6119dbb9a915283434f718b38a70017e3ad00840
> 
> 
> Could you please add this case to the patch and also to the Test plan so
> that we have full coverage of this?

Done

> 
> 3.)
> test_one_command_installation exploded during client enrollment part on
> "Joining realm failed: incorrect password". This is probably caused by
> missing '-P', 'admin' option here:
> """
> +        self.replicas[0].run_command(['ipa-replica-install', '-p',
> +                                     self.master.config.admin_password,
> +                                     '-n', self.master.domain.name,
> +                                     '-r', self.master.domain.realm])
> +
> """

Fixed. Turned out, it's enough to just provide '-w'

> 
> 4.)
> I am not very happy about the organization of
> 'TestUnprivilegedUserPermissions' class.
> 
> For starters, I would add this whole block:
> """
> +        password = self.master.config.dirman_password
> +        new_password = '$ome0therPaaS'
> +        replica = self.replicas[0]
> +        adduser_stdin_text = "%s\n%s\n" %
> (self.master.config.admin_password,
> + self.master.config.admin_password)
> +        user_kinit_stdin_text = "%s\n%s\n%s\n" % (password, new_password,
> +                                                  new_password)
> +        tasks.kinit_admin(self.master)
> +        self.master.run_command(['ipa', 'user-add', 'testuser',
> '--password',
> +                                 '--first', 'John', '--last', 'Donn'],
> +                                stdin_text=adduser_stdin_text)
> +        # Now we need to change the password for the user
> +        self.master.run_command(['kinit', 'testuser'],
> +                                stdin_text=user_kinit_stdin_text)
> +        # And again kinit admin
> +        tasks.kinit_admin(self.master)
> """
> 
> into 'install()' method, since it indeed sets-up the test harness. You
> can add the user name and password to class members so that you can then
> use them from the test cases. Which brings me to the second point: I
> know that the test plan mentions this as a single test case, but I would
> like this:
> 
> """
> +        result1 = replica.run_command(['ipa-client-install', '-p',
> 'testuser',
> +                                       '-w', new_password,
> +                                       '--domain', replica.domain.name,
> +                                       '--realm', replica.domain.realm,
> '-U'],
> +                                      raiseonerr=False)
> +        assert_error(result1, "No permission to join this host", 1)
> +        tasks.install_client(self.master, replica)
> +        result2 = replica.run_command(['ipa-replica-install', '-P',
> 'testuser',
> +                                       '-p', new_password,
> +                                       '-n', self.master.domain.name,
> +                                       '-r', self.master.domain.realm],
> +                                      raiseonerr=False)
> +        assert_error(result2,
> +                     "Insufficient privileges to promote the server", 1)
> +        self.master.run_command(['ipa', 'group-add-member', 'admins',
> +                                 '--users=testuser'])
> +
> +        replica.run_command(['ipa-replica-install', '-P', 'testuser',
> +                             '-p', new_password,
> +                             '-n', self.master.domain.name,
> +                             '-r', self.master.domain.realm])
> """
> 
> to be split into three separate test methods for the sake of clarity, e.g.:
> "test_client_enrollment_by_unprivileged_user"
> "test_replica_install_by_unprovileged_user"
> "test_replica_install_after_adding_to_admin_group"

I like that! Implemented.

> 
> 5.)
> """
> +        result = self.replicas[0].run_command(['ipa-server-install',
> +                                               '--uninstall', '-U'],
> +                                              raiseonerr=False)
> +        assert("Uninstallation leads to disconnected topology"
> +               in result.stderr_text)
> +        self.replicas[0].run_command(['ipa-server-install', '--uninstall',
> +                                      '-U',
> '--ignore-topology-disconnect'])
> """
> here you should assert against command stdout, since the error message
> is emitted only by plain print(). Yes it is weird but that's the way it
> is. It will probably be changed when I implement
> https://fedorahosted.org/freeipa/ticket/5588 so we can fix it when the
> ticket is finished.

Yeah, I've noticed that already. Fixed

> 

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 07d98081324eeae72c1c62fa5113d26101da0d76 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Thu, 31 Mar 2016 17:16:31 +0200
Subject: [PATCH] Added 5 more tests to Replica Promotion testsuite

The following testcases were automated:
1. Test one command replica installation
2. Test csreplica-manage-(del, connect, disconnect) are disabled in domain
level 1
3. Client enrollment and replica promotion by an unprivileged user are
prohibited
4. Replica uninstallation is prohibited if it disconnects a part of existing
topology (is possible only with --ignore-topology-disconnect option)
https://fedorahosted.org/freeipa/ticket/5723
---
 .../test_integration/test_replica_promotion.py     | 132 ++++++++++++++++++++-
 1 file changed, 131 insertions(+), 1 deletion(-)

diff --git a/ipatests/test_integration/test_replica_promotion.py b/ipatests/test_integration/test_replica_promotion.py
index c60b0964b4ba77ef6162920145d532fd2ef83bd2..85beaec4530848b6005712a1ae1dead5f5d69cd4 100644
--- a/ipatests/test_integration/test_replica_promotion.py
+++ b/ipatests/test_integration/test_replica_promotion.py
@@ -174,6 +174,18 @@ class TestReplicaPromotionLevel1(ReplicaPromotionBase):
                              " to generate replica file\n"
                              "is supported only in 0-level IPA domain", 1)
 
+    @replicas_cleanup
+    def test_one_command_installation(self):
+        """
+        TestCase:
+        http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+        #Test_case:_Replica_can_be_installed_using_one_command
+        """
+        self.replicas[0].run_command(['ipa-replica-install', '-w',
+                                     self.master.config.admin_password,
+                                     '-n', self.master.domain.name,
+                                     '-r', self.master.domain.realm])
+
 
 class TestReplicaManageCommands(IntegrationTest):
     topology = "star"
@@ -211,7 +223,7 @@ class TestReplicaManageCommands(IntegrationTest):
                               ' deprecated with managed IPA replication'
                               ' topology. Please use `ipa topologysegment-*`'
                               ' commands to manage the topology', 1)
-        tasks.create_segment(master, replica1, replica2)
+        segment = tasks.create_segment(master, replica1, replica2)
         result4 = master.run_command(["ipa-replica-manage",
                                       "disconnect",
                                       replica1.hostname,
@@ -221,3 +233,121 @@ class TestReplicaManageCommands(IntegrationTest):
                               ' deprecated with managed IPA replication'
                               ' topology. Please use `ipa topologysegment-*`'
                               ' commands to manage the topology', 1)
+
+        # http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+        #Test_case:_ipa-csreplica-manage_connect_is_deprecated
+        #_in_domain_level_1
+
+        result5 = master.run_command(['ipa-csreplica-manage', 'del',
+                                      replica1.hostname,
+                                      '-p', master.config.dirman_password],
+                                     raiseonerr=False)
+        assert_error(result5, "Removal of IPA CS replication agreement"
+                              " and replication data is deprecated with"
+                              " managed IPA replication topology", 1)
+
+        tasks.destroy_segment(master, segment[0]['name'])
+        result6 = master.run_command(["ipa-csreplica-manage",
+                                      "connect",
+                                      replica1.hostname,
+                                      replica2.hostname,
+                                      '-p', master.config.dirman_password],
+                                     raiseonerr=False)
+        assert_error(result6, "Creation of IPA CS replication agreement is"
+                              " deprecated with managed IPA replication"
+                              " topology", 1)
+        tasks.create_segment(master, replica1, replica2)
+        result7 = master.run_command(["ipa-csreplica-manage",
+                                      "disconnect",
+                                      replica1.hostname,
+                                      replica2.hostname,
+                                      '-p', master.config.dirman_password],
+                                     raiseonerr=False)
+        assert_error(result7, "Removal of IPA CS replication agreement is"
+                              " deprecated with managed IPA"
+                              " replication topology", 1)
+
+
+class TestUnprivilegedUserPermissions(IntegrationTest):
+    """
+    TestCase:
+    http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+    #Test_case:_Unprivileged_users_are_not_allowed_to_enroll
+    _and_promote_clients
+    """
+    num_replicas = 1
+    domain_level = DOMAIN_LEVEL_1
+
+    @classmethod
+    def install(cls, mh):
+        cls.username = 'testuser'
+        tasks.install_master(cls.master, domain_level=cls.domain_level)
+        password = cls.master.config.dirman_password
+        cls.new_password = '$ome0therPaaS'
+        adduser_stdin_text = "%s\n%s\n" % (cls.master.config.admin_password,
+                                           cls.master.config.admin_password)
+        user_kinit_stdin_text = "%s\n%s\n%s\n" % (password, cls.new_password,
+                                                  cls.new_password)
+        tasks.kinit_admin(cls.master)
+        cls.master.run_command(['ipa', 'user-add', cls.username, '--password',
+                                '--first', 'John', '--last', 'Donn'],
+                               stdin_text=adduser_stdin_text)
+        # Now we need to change the password for the user
+        cls.master.run_command(['kinit', cls.username],
+                               stdin_text=user_kinit_stdin_text)
+        # And again kinit admin
+        tasks.kinit_admin(cls.master)
+
+    def test_client_enrollment_by_unprivileged_user(self):
+        replica = self.replicas[0]
+        result1 = replica.run_command(['ipa-client-install',
+                                       '-p', self.username,
+                                       '-w', self.new_password,
+                                       '--domain', replica.domain.name,
+                                       '--realm', replica.domain.realm, '-U'],
+                                      raiseonerr=False)
+        assert_error(result1, "No permission to join this host", 1)
+
+    def test_replica_promotion_by_unprivileged_user(self):
+        replica = self.replicas[0]
+        tasks.install_client(self.master, replica)
+        result2 = replica.run_command(['ipa-replica-install',
+                                       '-P', self.username,
+                                       '-p', self.new_password,
+                                       '-n', self.master.domain.name,
+                                       '-r', self.master.domain.realm],
+                                      raiseonerr=False)
+        assert_error(result2,
+                     "Insufficient privileges to promote the server", 1)
+
+    def test_replica_promotion_after_adding_to_admin_group(self):
+        self.master.run_command(['ipa', 'group-add-member', 'admins',
+                                 '--users=%s' % self.username])
+
+        self.replicas[0].run_command(['ipa-replica-install',
+                                      '-P', self.username,
+                                      '-p', self.new_password,
+                                      '-n', self.master.domain.name,
+                                      '-r', self.master.domain.realm])
+
+
+class TestProhibitReplicaUninstallation(IntegrationTest):
+    topology = 'line'
+    num_replicas = 2
+    domain_level = DOMAIN_LEVEL_1
+
+    def test_replica_uninstallation_prohibited(self):
+        """
+        http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+        #Test_case:_Prohibit_ipa_server_uninstallation_from_disconnecting
+        _topology_segment
+        """
+        result = self.replicas[0].run_command(['ipa-server-install',
+                                               '--uninstall', '-U'],
+                                              raiseonerr=False)
+        assert(result.returncode == 0), ("The replica was removed without "
+                                         "'--ignore-topology-disconnect' option")
+        assert("Uninstallation leads to disconnected topology"
+               in result.stdout_text), ("Expected error message was not found")
+        self.replicas[0].run_command(['ipa-server-install', '--uninstall',
+                                      '-U', '--ignore-topology-disconnect'])
-- 
1.8.3.1

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