Skip to content

Commit

Permalink
Fix bug preventing user/app deletion (#175)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
xoen authored and andyhd committed May 29, 2018
1 parent b618b27 commit 1f15563
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 7 deletions.
21 changes: 20 additions & 1 deletion control_panel_api/aws.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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,
Expand Down
36 changes: 30 additions & 6 deletions control_panel_api/tests/test_aws.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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,
Expand Down

0 comments on commit 1f15563

Please sign in to comment.