Merge "Bypass user and group verification in RemoveRole"

openstackclient/identity/v3/role.py

@@ -64,59 +64,61 @@ def _add_identity_and_resource_options_to_parser(parser):
 
 
 def _process_identity_and_resource_options(parsed_args,
-                                           identity_client_manager):
+                                           identity_client_manager,
+                                           validate_actor_existence=True):
+
+    def _find_user():
+        try:
+            return common.find_user(
+                identity_client_manager,
+                parsed_args.user,
+                parsed_args.user_domain
+            ).id
+        except exceptions.CommandError:
+            if not validate_actor_existence:
+                return parsed_args.user
+            raise
+
+    def _find_group():
+        try:
+            return common.find_group(
+                identity_client_manager,
+                parsed_args.group,
+                parsed_args.group_domain
+            ).id
+        except exceptions.CommandError:
+            if not validate_actor_existence:
+                return parsed_args.group
+            raise
+
     kwargs = {}
     if parsed_args.user and parsed_args.system:
-        kwargs['user'] = common.find_user(
+        kwargs['user'] = _find_user()
-            identity_client_manager,
-            parsed_args.user,
-            parsed_args.user_domain,
-        ).id
         kwargs['system'] = parsed_args.system
     elif parsed_args.user and parsed_args.domain:
-        kwargs['user'] = common.find_user(
+        kwargs['user'] = _find_user()
-            identity_client_manager,
-            parsed_args.user,
-            parsed_args.user_domain,
-        ).id
         kwargs['domain'] = common.find_domain(
             identity_client_manager,
             parsed_args.domain,
         ).id
     elif parsed_args.user and parsed_args.project:
-        kwargs['user'] = common.find_user(
+        kwargs['user'] = _find_user()
-            identity_client_manager,
-            parsed_args.user,
-            parsed_args.user_domain,
-        ).id
         kwargs['project'] = common.find_project(
             identity_client_manager,
             parsed_args.project,
             parsed_args.project_domain,
         ).id
     elif parsed_args.group and parsed_args.system:
-        kwargs['group'] = common.find_group(
+        kwargs['group'] = _find_group()
-            identity_client_manager,
-            parsed_args.group,
-            parsed_args.group_domain,
-        ).id
         kwargs['system'] = parsed_args.system
     elif parsed_args.group and parsed_args.domain:
-        kwargs['group'] = common.find_group(
+        kwargs['group'] = _find_group()
-            identity_client_manager,
-            parsed_args.group,
-            parsed_args.group_domain,
-        ).id
         kwargs['domain'] = common.find_domain(
             identity_client_manager,
             parsed_args.domain,
         ).id
     elif parsed_args.group and parsed_args.project:
-        kwargs['group'] = common.find_group(
+        kwargs['group'] = _find_group()
-            identity_client_manager,
-            parsed_args.group,
-            parsed_args.group_domain,
-        ).id
         kwargs['project'] = common.find_project(
             identity_client_manager,
             parsed_args.project,
@@ -340,7 +342,9 @@ class RemoveRole(command.Command):
         )
 
         kwargs = _process_identity_and_resource_options(
-            parsed_args, self.app.client_manager.identity)
+            parsed_args, self.app.client_manager.identity,
+            validate_actor_existence=False
+        )
         identity_client.roles.revoke(role.id, **kwargs)
 
 

openstackclient/tests/unit/identity/v3/test_role.py

@@ -19,6 +19,7 @@ from unittest import mock
 from osc_lib import exceptions
 from osc_lib import utils
 
+from openstackclient.identity import common
 from openstackclient.identity.v3 import role
 from openstackclient.tests.unit import fakes
 from openstackclient.tests.unit.identity.v3 import fakes as identity_fakes
@@ -846,6 +847,47 @@ class TestRoleRemove(TestRole):
         )
         self.assertIsNone(result)
 
+    @mock.patch.object(common, 'find_user')
+    def test_role_remove_non_existent_user_system(self, find_mock):
+        # Simulate the user not being in keystone, the client should gracefully
+        # handle this exception and send the request to remove the role since
+        # keystone supports removing role assignments with non-existent actors
+        # (e.g., users or groups).
+        find_mock.side_effect = exceptions.CommandError
+
+        arglist = [
+            '--user', identity_fakes.user_id,
+            '--system', 'all',
+            identity_fakes.role_name
+        ]
+        if self._is_inheritance_testcase():
+            arglist.append('--inherited')
+        verifylist = [
+            ('user', identity_fakes.user_id),
+            ('group', None),
+            ('system', 'all'),
+            ('domain', None),
+            ('project', None),
+            ('role', identity_fakes.role_name),
+            ('inherited', self._is_inheritance_testcase()),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'user': identity_fakes.user_id,
+            'system': 'all',
+            'os_inherit_extension_inherited': self._is_inheritance_testcase(),
+        }
+        # RoleManager.revoke(role, user=, group=, domain=, project=)
+        self.roles_mock.revoke.assert_called_with(
+            identity_fakes.role_id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
     def test_role_remove_user_domain(self):
         arglist = [
             '--user', identity_fakes.user_name,
@@ -879,6 +921,46 @@ class TestRoleRemove(TestRole):
         )
         self.assertIsNone(result)
 
+    @mock.patch.object(common, 'find_user')
+    def test_role_remove_non_existent_user_domain(self, find_mock):
+        # Simulate the user not being in keystone, the client the gracefully
+        # handle this exception and send the request to remove the role since
+        # keystone will validate.
+        find_mock.side_effect = exceptions.CommandError
+
+        arglist = [
+            '--user', identity_fakes.user_id,
+            '--domain', identity_fakes.domain_name,
+            identity_fakes.role_name
+        ]
+        if self._is_inheritance_testcase():
+            arglist.append('--inherited')
+        verifylist = [
+            ('user', identity_fakes.user_id),
+            ('group', None),
+            ('system', None),
+            ('domain', identity_fakes.domain_name),
+            ('project', None),
+            ('role', identity_fakes.role_name),
+            ('inherited', self._is_inheritance_testcase()),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'user': identity_fakes.user_id,
+            'domain': identity_fakes.domain_id,
+            'os_inherit_extension_inherited': self._is_inheritance_testcase(),
+        }
+        # RoleManager.revoke(role, user=, group=, domain=, project=)
+        self.roles_mock.revoke.assert_called_with(
+            identity_fakes.role_id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
     def test_role_remove_user_project(self):
         arglist = [
             '--user', identity_fakes.user_name,
@@ -912,6 +994,46 @@ class TestRoleRemove(TestRole):
         )
         self.assertIsNone(result)
 
+    @mock.patch.object(common, 'find_user')
+    def test_role_remove_non_existent_user_project(self, find_mock):
+        # Simulate the user not being in keystone, the client the gracefully
+        # handle this exception and send the request to remove the role since
+        # keystone will validate.
+        find_mock.side_effect = exceptions.CommandError
+
+        arglist = [
+            '--user', identity_fakes.user_id,
+            '--project', identity_fakes.project_name,
+            identity_fakes.role_name
+        ]
+        if self._is_inheritance_testcase():
+            arglist.append('--inherited')
+        verifylist = [
+            ('user', identity_fakes.user_id),
+            ('group', None),
+            ('system', None),
+            ('domain', None),
+            ('project', identity_fakes.project_name),
+            ('role', identity_fakes.role_name),
+            ('inherited', self._is_inheritance_testcase()),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'user': identity_fakes.user_id,
+            'project': identity_fakes.project_id,
+            'os_inherit_extension_inherited': self._is_inheritance_testcase(),
+        }
+        # RoleManager.revoke(role, user=, group=, domain=, project=)
+        self.roles_mock.revoke.assert_called_with(
+            identity_fakes.role_id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
     def test_role_remove_group_system(self):
         arglist = [
             '--group', identity_fakes.group_name,
@@ -947,6 +1069,46 @@ class TestRoleRemove(TestRole):
         )
         self.assertIsNone(result)
 
+    @mock.patch.object(common, 'find_group')
+    def test_role_remove_non_existent_group_system(self, find_mock):
+        # Simulate the user not being in keystone, the client the gracefully
+        # handle this exception and send the request to remove the role since
+        # keystone will validate.
+        find_mock.side_effect = exceptions.CommandError
+
+        arglist = [
+            '--group', identity_fakes.group_id,
+            '--system', 'all',
+            identity_fakes.role_name
+        ]
+        if self._is_inheritance_testcase():
+            arglist.append('--inherited')
+        verifylist = [
+            ('user', None),
+            ('group', identity_fakes.group_id),
+            ('system', 'all'),
+            ('domain', None),
+            ('project', None),
+            ('role', identity_fakes.role_name),
+            ('inherited', self._is_inheritance_testcase()),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'group': identity_fakes.group_id,
+            'system': 'all',
+            'os_inherit_extension_inherited': self._is_inheritance_testcase(),
+        }
+        # RoleManager.revoke(role, user=, group=, domain=, project=)
+        self.roles_mock.revoke.assert_called_with(
+            identity_fakes.role_id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
     def test_role_remove_group_domain(self):
         arglist = [
             '--group', identity_fakes.group_name,
@@ -981,6 +1143,46 @@ class TestRoleRemove(TestRole):
         )
         self.assertIsNone(result)
 
+    @mock.patch.object(common, 'find_group')
+    def test_role_remove_non_existent_group_domain(self, find_mock):
+        # Simulate the user not being in keystone, the client the gracefully
+        # handle this exception and send the request to remove the role since
+        # keystone will validate.
+        find_mock.side_effect = exceptions.CommandError
+
+        arglist = [
+            '--group', identity_fakes.group_id,
+            '--domain', identity_fakes.domain_name,
+            identity_fakes.role_name
+        ]
+        if self._is_inheritance_testcase():
+            arglist.append('--inherited')
+        verifylist = [
+            ('user', None),
+            ('group', identity_fakes.group_id),
+            ('system', None),
+            ('domain', identity_fakes.domain_name),
+            ('project', None),
+            ('role', identity_fakes.role_name),
+            ('inherited', self._is_inheritance_testcase()),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'group': identity_fakes.group_id,
+            'domain': identity_fakes.domain_id,
+            'os_inherit_extension_inherited': self._is_inheritance_testcase(),
+        }
+        # RoleManager.revoke(role, user=, group=, domain=, project=)
+        self.roles_mock.revoke.assert_called_with(
+            identity_fakes.role_id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
     def test_role_remove_group_project(self):
         arglist = [
             '--group', identity_fakes.group_name,
@@ -1014,6 +1216,46 @@ class TestRoleRemove(TestRole):
         )
         self.assertIsNone(result)
 
+    @mock.patch.object(common, 'find_group')
+    def test_role_remove_non_existent_group_project(self, find_mock):
+        # Simulate the user not being in keystone, the client the gracefully
+        # handle this exception and send the request to remove the role since
+        # keystone will validate.
+        find_mock.side_effect = exceptions.CommandError
+
+        arglist = [
+            '--group', identity_fakes.group_id,
+            '--project', identity_fakes.project_name,
+            identity_fakes.role_name
+        ]
+        if self._is_inheritance_testcase():
+            arglist.append('--inherited')
+        verifylist = [
+            ('user', None),
+            ('group', identity_fakes.group_id),
+            ('system', None),
+            ('domain', None),
+            ('project', identity_fakes.project_name),
+            ('role', identity_fakes.role_name),
+            ('inherited', self._is_inheritance_testcase()),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'group': identity_fakes.group_id,
+            'project': identity_fakes.project_id,
+            'os_inherit_extension_inherited': self._is_inheritance_testcase(),
+        }
+        # RoleManager.revoke(role, user=, group=, domain=, project=)
+        self.roles_mock.revoke.assert_called_with(
+            identity_fakes.role_id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
     def test_role_remove_domain_role_on_group_domain(self):
         self.roles_mock.get.return_value = fakes.FakeResource(
             None,

releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    You can now remove role assignments from keystone that reference non-existent
+    users or groups.
+
+    [Bug `2006635 <https://storyboard.openstack.org/#!/story/2006635>`_]