From 58a88d9d4c2dac9d8f18fff195bc4b621952931e Mon Sep 17 00:00:00 2001 From: Dmitriy Uvarenkov Date: Sat, 1 Oct 2016 12:30:30 +0300 Subject: [PATCH] Correct deleting of role assignments If role assignment is already removed by any means we do not want to send request to delete it in Heat. Depends-On: I1fbb1449723843d88501b8f11913e9908fe4cd00 Closes-Bug: #1615947 Change-Id: I15f542c0d5756356a5d5ac14a35704c07903e24b --- .../openstack/keystone/role_assignments.py | 72 +++++++++++-------- .../keystone/test_role_assignments.py | 45 ++++++++++++ 2 files changed, 87 insertions(+), 30 deletions(-) diff --git a/heat/engine/resources/openstack/keystone/role_assignments.py b/heat/engine/resources/openstack/keystone/role_assignments.py index 3e12e9c41d..6bb402ad4d 100644 --- a/heat/engine/resources/openstack/keystone/role_assignments.py +++ b/heat/engine/resources/openstack/keystone/role_assignments.py @@ -123,35 +123,39 @@ class KeystoneRoleAssignmentMixin(object): user=user_id ) - def _remove_role_assignments_from_group(self, group_id, role_assignments): + def _remove_role_assignments_from_group(self, group_id, role_assignments, + current_assignments): for role_assignment in self._normalize_to_id(role_assignments): - if role_assignment.get(self.PROJECT) is not None: - self.client().roles.revoke( - role=role_assignment.get(self.ROLE), - project=role_assignment.get(self.PROJECT), - group=group_id - ) - elif role_assignment.get(self.DOMAIN) is not None: - self.client().roles.revoke( - role=role_assignment.get(self.ROLE), - domain=role_assignment.get(self.DOMAIN), - group=group_id - ) + if role_assignment in current_assignments: + if role_assignment.get(self.PROJECT) is not None: + self.client().roles.revoke( + role=role_assignment.get(self.ROLE), + project=role_assignment.get(self.PROJECT), + group=group_id + ) + elif role_assignment.get(self.DOMAIN) is not None: + self.client().roles.revoke( + role=role_assignment.get(self.ROLE), + domain=role_assignment.get(self.DOMAIN), + group=group_id + ) - def _remove_role_assignments_from_user(self, user_id, role_assignments): + def _remove_role_assignments_from_user(self, user_id, role_assignments, + current_assignments): for role_assignment in self._normalize_to_id(role_assignments): - if role_assignment.get(self.PROJECT) is not None: - self.client().roles.revoke( - role=role_assignment.get(self.ROLE), - project=role_assignment.get(self.PROJECT), - user=user_id - ) - elif role_assignment.get(self.DOMAIN) is not None: - self.client().roles.revoke( - role=role_assignment.get(self.ROLE), - domain=role_assignment.get(self.DOMAIN), - user=user_id - ) + if role_assignment in current_assignments: + if role_assignment.get(self.PROJECT) is not None: + self.client().roles.revoke( + role=role_assignment.get(self.ROLE), + project=role_assignment.get(self.PROJECT), + user=user_id + ) + elif role_assignment.get(self.DOMAIN) is not None: + self.client().roles.revoke( + role=role_assignment.get(self.ROLE), + domain=role_assignment.get(self.DOMAIN), + user=user_id + ) def _normalize_to_id(self, role_assignment_prps): role_assignments = [] @@ -272,26 +276,34 @@ class KeystoneRoleAssignmentMixin(object): new_role_assignments) if len(removed_role_assignments) > 0: + current_assignments = self.parse_list_assignments( + user_id=user_id, group_id=group_id) if user_id is not None: self._remove_role_assignments_from_user( user_id, - removed_role_assignments) + removed_role_assignments, + current_assignments) elif group_id is not None: self._remove_role_assignments_from_group( group_id, - removed_role_assignments) + removed_role_assignments, + current_assignments) def delete_assignment(self, user_id=None, group_id=None): self._stored_properties_data if self.properties[self.ROLES] is not None: + current_assignments = self.parse_list_assignments( + user_id=user_id, group_id=group_id) if user_id is not None: self._remove_role_assignments_from_user( user_id, - (self.properties[self.ROLES])) + (self.properties[self.ROLES]), + current_assignments) elif group_id is not None: self._remove_role_assignments_from_group( group_id, - (self.properties[self.ROLES])) + (self.properties[self.ROLES]), + current_assignments) def validate_assignment_properties(self): if self.properties.get(self.ROLES) is not None: diff --git a/heat/tests/openstack/keystone/test_role_assignments.py b/heat/tests/openstack/keystone/test_role_assignments.py index 3527bc7117..dae1820e1e 100644 --- a/heat/tests/openstack/keystone/test_role_assignments.py +++ b/heat/tests/openstack/keystone/test_role_assignments.py @@ -96,6 +96,16 @@ class KeystoneRoleAssignmentMixinTest(common.HeatTestCase): (self.test_role_assignment.client_plugin. return_value) = self.keystone_client_plugin + self.parse_assgmnts = self.test_role_assignment.parse_list_assignments + self.test_role_assignment.parse_list_assignments = mock.MagicMock() + self.test_role_assignment.parse_list_assignments.return_value = [ + {'role': 'role_1', + 'domain': 'domain_1', + 'project': None}, + {'role': 'role_1', + 'project': 'project_1', + 'domain': None}] + def test_properties_title(self): property_title_map = {MixinClass.ROLES: 'roles'} @@ -325,6 +335,21 @@ class KeystoneRoleAssignmentMixinTest(common.HeatTestCase): group='group_1', project='project_1') + def test_role_assignment_delete_removed(self): + self.test_role_assignment.parse_list_assignments.return_value = [ + {'role': 'role_1', + 'domain': 'domain_1', + 'project': None}] + + self.assertIsNone(self.test_role_assignment.delete_assignment( + user_id='user_1')) + + expected = [ + ({'role': 'role_1', 'user': 'user_1', 'domain': 'domain_1'},) + ] + + self.assertItemsEqual(expected, self.roles.revoke.call_args_list) + def test_validate_1(self): self.test_role_assignment.properties = mock.MagicMock() @@ -347,6 +372,7 @@ class KeystoneRoleAssignmentMixinTest(common.HeatTestCase): self.test_role_assignment.validate) def test_empty_parse_list_assignments(self): + self.test_role_assignment.parse_list_assignments = self.parse_assgmnts self.assertEqual([], self.test_role_assignment.parse_list_assignments()) @@ -357,6 +383,7 @@ class KeystoneRoleAssignmentMixinTest(common.HeatTestCase): self._test_parse_list_assignments('group') def _test_parse_list_assignments(self, entity=None): + self.test_role_assignment.parse_list_assignments = self.parse_assgmnts dict_obj = mock.MagicMock() dict_obj.to_dict.side_effect = [{'scope': { 'project': {'id': 'fc0fe982401643368ff2eb11d9ca70f1'}}, @@ -419,6 +446,15 @@ class KeystoneUserRoleAssignmentTest(common.HeatTestCase): (self.test_role_assignment.client_plugin. return_value) = self.keystone_client_plugin + self.test_role_assignment.parse_list_assignments = mock.MagicMock() + self.test_role_assignment.parse_list_assignments.return_value = [ + {'role': 'role_1', + 'domain': 'domain_1', + 'project': None}, + {'role': 'role_1', + 'project': 'project_1', + 'domain': None}] + def test_user_role_assignment_handle_create(self): self.test_role_assignment.handle_create() @@ -539,6 +575,15 @@ class KeystoneGroupRoleAssignmentTest(common.HeatTestCase): (self.test_role_assignment.client_plugin. return_value) = self.keystone_client_plugin + self.test_role_assignment.parse_list_assignments = mock.MagicMock() + self.test_role_assignment.parse_list_assignments.return_value = [ + {'role': 'role_1', + 'domain': 'domain_1', + 'project': None}, + {'role': 'role_1', + 'project': 'project_1', + 'domain': None}] + def test_group_role_assignment_handle_create(self): self.test_role_assignment.handle_create()