diff options
| -rw-r--r-- | lower-constraints.txt | 2 | ||||
| -rw-r--r-- | openstackclient/identity/common.py | 2 | ||||
| -rw-r--r-- | openstackclient/identity/v3/role.py | 68 | ||||
| -rw-r--r-- | openstackclient/tests/unit/identity/v3/test_role.py | 379 | ||||
| -rw-r--r-- | releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml | 7 | ||||
| -rw-r--r-- | test-requirements.txt | 3 | ||||
| -rw-r--r-- | tox.ini | 8 |
7 files changed, 430 insertions, 39 deletions
diff --git a/lower-constraints.txt b/lower-constraints.txt index 1fa30674..927faaea 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -113,7 +113,7 @@ python-watcherclient==2.5.0 python-zaqarclient==1.0.0 python-zunclient==3.6.0 pytz==2013.6 -PyYAML==3.12 +PyYAML==3.13 repoze.lru==0.7 requests-mock==1.2.0 requests==2.14.2 diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py index 7be2a17b..a5acfed9 100644 --- a/openstackclient/identity/common.py +++ b/openstackclient/identity/common.py @@ -207,7 +207,7 @@ def _find_identity_resource(identity_client_manager, name_or_id, name_or_id, **kwargs) if identity_resource is not None: return identity_resource - except exceptions.Forbidden: + except (exceptions.Forbidden, identity_exc.Forbidden): pass return resource_type(None, {'id': name_or_id, 'name': name_or_id}) diff --git a/openstackclient/identity/v3/role.py b/openstackclient/identity/v3/role.py index 36f3f938..682d77d6 100644 --- a/openstackclient/identity/v3/role.py +++ b/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( - identity_client_manager, - parsed_args.user, - parsed_args.user_domain, - ).id + kwargs['user'] = _find_user() kwargs['system'] = parsed_args.system elif parsed_args.user and parsed_args.domain: - kwargs['user'] = common.find_user( - identity_client_manager, - parsed_args.user, - parsed_args.user_domain, - ).id + kwargs['user'] = _find_user() 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( - identity_client_manager, - parsed_args.user, - parsed_args.user_domain, - ).id + kwargs['user'] = _find_user() 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( - identity_client_manager, - parsed_args.group, - parsed_args.group_domain, - ).id + kwargs['group'] = _find_group() kwargs['system'] = parsed_args.system elif parsed_args.group and parsed_args.domain: - kwargs['group'] = common.find_group( - identity_client_manager, - parsed_args.group, - parsed_args.group_domain, - ).id + kwargs['group'] = _find_group() 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( - identity_client_manager, - parsed_args.group, - parsed_args.group_domain, - ).id + kwargs['group'] = _find_group() kwargs['project'] = common.find_project( identity_client_manager, parsed_args.project, @@ -337,7 +339,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) diff --git a/openstackclient/tests/unit/identity/v3/test_role.py b/openstackclient/tests/unit/identity/v3/test_role.py index 4278ab1c..557bc7e7 100644 --- a/openstackclient/tests/unit/identity/v3/test_role.py +++ b/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 @@ -102,6 +103,40 @@ class TestRoleAdd(TestRole): # Get the command object to test self.cmd = role.AddRole(self.app, None) + def test_role_add_user_system(self): + arglist = [ + '--user', identity_fakes.user_name, + '--system', 'all', + identity_fakes.role_name, + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', identity_fakes.user_name), + ('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.grant(role, user=, group=, domain=, project=) + self.roles_mock.grant.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_add_user_domain(self): arglist = [ '--user', identity_fakes.user_name, @@ -168,6 +203,40 @@ class TestRoleAdd(TestRole): ) self.assertIsNone(result) + def test_role_add_group_system(self): + arglist = [ + '--group', identity_fakes.group_name, + '--system', 'all', + identity_fakes.role_name, + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', None), + ('group', identity_fakes.group_name), + ('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.grant(role, user=, group=, domain=, project=) + self.roles_mock.grant.assert_called_with( + identity_fakes.role_id, + **kwargs + ) + self.assertIsNone(result) + def test_role_add_group_domain(self): arglist = [ '--group', identity_fakes.group_name, @@ -651,6 +720,81 @@ class TestRoleRemove(TestRole): # Get the command object to test self.cmd = role.RemoveRole(self.app, None) + def test_role_remove_user_system(self): + arglist = [ + '--user', identity_fakes.user_name, + '--system', 'all', + identity_fakes.role_name + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', identity_fakes.user_name), + ('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) + + @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, @@ -684,6 +828,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, @@ -717,6 +901,121 @@ 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, + '--system', 'all', + identity_fakes.role_name, + ] + if self._is_inheritance_testcase(): + arglist.append('--inherited') + verifylist = [ + ('user', None), + ('group', identity_fakes.group_name), + ('system', 'all'), + ('domain', None), + ('project', None), + ('role', identity_fakes.role_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, + '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) + + @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, @@ -751,6 +1050,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, @@ -784,6 +1123,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, diff --git a/releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml b/releasenotes/notes/bug-2006635-3110f7a87a186e62.yaml new file mode 100644 index 00000000..ecc58c8a --- /dev/null +++ b/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>`_] diff --git a/test-requirements.txt b/test-requirements.txt index 55ae1ea4..2e56f4be 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,10 +1,8 @@ # The order of packages is significant, because pip processes them in the order # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. -hacking>=2.0.0 # Apache-2.0 coverage!=4.4,>=4.0 # Apache-2.0 fixtures>=3.0.0 # Apache-2.0/BSD -flake8-import-order>=0.13 # LGPLv3 mock>=2.0.0 # BSD oslotest>=3.2.0 # Apache-2.0 requests>=2.14.2 # Apache-2.0 @@ -15,5 +13,4 @@ stestr>=1.0.0 # Apache-2.0 testtools>=2.2.0 # MIT tempest>=17.1.0 # Apache-2.0 osprofiler>=1.4.0 # Apache-2.0 -bandit!=1.6.0,>=1.1.0 # Apache-2.0 wrapt>=1.7.0 # BSD License @@ -28,9 +28,13 @@ commands = {toxinidir}/tools/fast8.sh [testenv:pep8] +deps = + hacking>=2.0.0,<4.0.0 + bandit!=1.6.0,>=1.1.0 + flake8-import-order>=0.13 # LGPLv3 commands = - flake8 - bandit -r openstackclient -x tests -s B105,B106,B107,B401,B404,B603,B606,B607,B110,B605,B101 + flake8 + bandit -r openstackclient -x tests -s B105,B106,B107,B401,B404,B603,B606,B607,B110,B605,B101 [testenv:bandit] # This command runs the bandit security linter against the openstackclient |