From 1f155635b436f0428248515dcdd60c01c8c55bb5 Mon Sep 17 00:00:00 2001 From: Aldo Giambelluca Date: Tue, 29 May 2018 14:58:16 +0100 Subject: [PATCH] Fix bug preventing user/app deletion (#175) * Improved test for aws.detach_policy_from_role() By checking the arguments as well and improved the test data * Fix bug preventing user/app deletion. When a user/app is deleted we also delete their IAM role. In order to delete an IAM role all its attached managed policies and inline policies need to be deleted first or the operation will fail. We already detached all the managed policies attached but we didn't delete the role inline policies. As (most) or the roles now have an `s3-access` inline policy the operation was failing raising a `DeleteConflict` error. === Other details considerations === The user/app DB record is deleted before we delete its IAM role. When a user/app is deleted the associated records (`users3buckets`/`apps3buckets`) are deleted as well. When these are deleted the corresponsing user/app IAM role's `s3-access` inline policy is updated to revoke access to that S3 bucket. This means that even if an error occurred the user/app will not have any access to S3 buckets. As the `perform_destroy` method is wrapped in a DB transaction, therefore when this error occurs the transaction is rolled back and these records are restored. This is confusing but as mentioned above the user/app will not have access to any S3 bucket. === Ticket === https://trello.com/c/04FsXha3/984-cant-delete-users --- control_panel_api/aws.py | 21 ++++++++++++++++- control_panel_api/tests/test_aws.py | 36 ++++++++++++++++++++++++----- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/control_panel_api/aws.py b/control_panel_api/aws.py index b4ac0bc51..1f0e007c5 100644 --- a/control_panel_api/aws.py +++ b/control_panel_api/aws.py @@ -94,6 +94,7 @@ def delete_role(self, role_name): """Delete the given IAM role.""" self._detach_role_policies(role_name) + self._delete_role_inline_policies(role_name) self._do('iam', 'delete_role', RoleName=role_name) def _detach_role_policies(self, role_name): @@ -106,9 +107,27 @@ def _detach_role_policies(self, role_name): for policy in policies["AttachedPolicies"]: self.detach_policy_from_role( role_name=role_name, - policy_arn=policy["PolicyArn"] + policy_arn=policy["PolicyArn"], ) + def _delete_role_inline_policies(self, role_name): + """Deletes all inline policies in the given role""" + + policies = self._do( + 'iam', 'list_role_policies', RoleName=role_name) + + if policies: + for policy_name in policies["PolicyNames"]: + self.delete_role_inline_policy( + role_name=role_name, + policy_name=policy_name, + ) + + def delete_role_inline_policy(self, policy_name, role_name): + self._do('iam', 'delete_role_policy', + RoleName=role_name, + PolicyName=policy_name) + def attach_policy_to_role(self, policy_arn, role_name): self._do('iam', 'attach_role_policy', RoleName=role_name, diff --git a/control_panel_api/tests/test_aws.py b/control_panel_api/tests/test_aws.py index 5a25b294f..abba8deb5 100644 --- a/control_panel_api/tests/test_aws.py +++ b/control_panel_api/tests/test_aws.py @@ -65,8 +65,18 @@ def test_attach_policy_to_role(self): ) def test_detach_policy_from_role(self): - aws.detach_policy_from_role('policyarn', 'foo') - aws.client.return_value.detach_role_policy.assert_called() + aws.detach_policy_from_role('policy_arn', 'role_name') + aws.client.return_value.detach_role_policy.assert_called_with( + RoleName='role_name', + PolicyArn='policy_arn', + ) + + def test_delete_role_inline_policy(self): + aws.delete_role_inline_policy('policy_name', 'role_name') + aws.client.return_value.delete_role_policy.assert_called_with( + RoleName='role_name', + PolicyName='policy_name', + ) def test_create_role(self): role_name = "a_role" @@ -80,25 +90,39 @@ def test_create_role(self): ) def test_delete_role(self): - aws.client.return_value.list_attached_role_policies.return_value = { + aws_client = aws.client.return_value + aws_client.list_attached_role_policies.return_value = { "AttachedPolicies": [ {"PolicyArn": "arn_1"}, {"PolicyArn": "arn_2"}, ], } + aws_client.list_role_policies.return_value = { + "PolicyNames": [ + 's3-access', + 'other-inline-policy', + ], + } role_name = "a_role" aws.delete_role(role_name) + # Check managed policies are detached from role expected_detach_calls = [ call(RoleName=role_name, PolicyArn='arn_1'), call(RoleName=role_name, PolicyArn='arn_2'), ] - - # Check policies are detached from role - aws.client.return_value.detach_role_policy.assert_has_calls( + aws_client.detach_role_policy.assert_has_calls( expected_detach_calls) + # Check inline policies are deleted + expected_delete_policy_calls = [ + call(RoleName=role_name, PolicyName='s3-access'), + call(RoleName=role_name, PolicyName='other-inline-policy'), + ] + aws_client.delete_role_policy.assert_has_calls( + expected_delete_policy_calls) + # Check role is deleted aws.client.return_value.delete_role.assert_called_with( RoleName=role_name,