From fd2dc39a1bbd6e5e28ef456b567d9edef11b67e8 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Tue, 17 Oct 2023 16:19:55 +0100 Subject: [PATCH] Store the user that revokes all access to a bucket --- controlpanel/api/models/apps3bucket.py | 5 +++-- controlpanel/api/models/policys3bucket.py | 3 ++- controlpanel/api/models/users3bucket.py | 5 +++-- controlpanel/api/tasks/handlers/s3.py | 4 +++- tests/api/tasks/test_s3.py | 17 ++++++++++++----- 5 files changed, 23 insertions(+), 11 deletions(-) diff --git a/controlpanel/api/models/apps3bucket.py b/controlpanel/api/models/apps3bucket.py index 9cd5d44d4..33078e797 100644 --- a/controlpanel/api/models/apps3bucket.py +++ b/controlpanel/api/models/apps3bucket.py @@ -43,5 +43,6 @@ def __repr__(self): def grant_bucket_access(self): tasks.S3BucketGrantToApp(self, self.current_user).create_task() - def revoke_bucket_access(self): - tasks.S3BucketRevokeAppAccess(self, self.current_user).create_task() + def revoke_bucket_access(self, revoked_by=None): + revoked_by = revoked_by or None + tasks.S3BucketRevokeAppAccess(self, revoked_by).create_task() diff --git a/controlpanel/api/models/policys3bucket.py b/controlpanel/api/models/policys3bucket.py index 6781f279e..a0c6b29ec 100644 --- a/controlpanel/api/models/policys3bucket.py +++ b/controlpanel/api/models/policys3bucket.py @@ -38,7 +38,8 @@ def grant_bucket_access(self): self.resources, ) - def revoke_bucket_access(self): + def revoke_bucket_access(self, revoked_by=None): + # TODO update to use a Task to revoke access, to match user/app access{{ if self.s3bucket.is_folder: return cluster.RoleGroup(self.policy).revoke_folder_access( root_folder_path=self.s3bucket.name diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py index 7e5cfbec3..2f9d00d4e 100644 --- a/controlpanel/api/models/users3bucket.py +++ b/controlpanel/api/models/users3bucket.py @@ -47,8 +47,9 @@ def __repr__(self): def grant_bucket_access(self): tasks.S3BucketGrantToUser(self, self.current_user).create_task() - def revoke_bucket_access(self): + def revoke_bucket_access(self, revoked_by=None): # TODO when soft delete is added, this should be updated to use the user that # has deleted the parent S3bucket to ensure we store the user that has sent the # task in the case of cascading deletes - tasks.S3BucketRevokeUserAccess(self, self.current_user).create_task() + revoked_by = revoked_by or self.current_user + tasks.S3BucketRevokeUserAccess(self, revoked_by).create_task() diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py index f37e90b19..b748031a1 100644 --- a/controlpanel/api/tasks/handlers/s3.py +++ b/controlpanel/api/tasks/handlers/s3.py @@ -86,9 +86,11 @@ def handle(self, *args, **kwargs): remain in place. In order to keep IAM roles updated, this task collects objects that would have been deleted by a cascade, and revokes access to deleted bucket """ + task_user = User.objects.filter(pk=self.task_user_pk).first() collector = Collector(using="default") collector.collect([self.object]) for model, instance in collector.instances_with_model(): if not issubclass(model, AccessToS3Bucket): continue - instance.revoke_bucket_access() + + instance.revoke_bucket_access(revoked_by=task_user) diff --git a/tests/api/tasks/test_s3.py b/tests/api/tasks/test_s3.py index 88b329dd4..20331f4f6 100644 --- a/tests/api/tasks/test_s3.py +++ b/tests/api/tasks/test_s3.py @@ -121,14 +121,21 @@ def test_revoke_app_access(cluster, complete): @patch("controlpanel.api.models.UserS3Bucket.revoke_bucket_access", new=MagicMock()) @patch("controlpanel.api.models.AppS3Bucket.revoke_bucket_access", new=MagicMock()) @patch("controlpanel.api.models.PolicyS3Bucket.revoke_bucket_access", new=MagicMock()) -def test_revoke_all_access(): +def test_revoke_all_access(users): bucket = mommy.make("api.S3Bucket") user_access = mommy.make("api.UserS3Bucket", s3bucket=bucket) app_access = mommy.make("api.AppS3Bucket", s3bucket=bucket) policy_access = mommy.make("api.PolicyS3Bucket", s3bucket=bucket) + task = mommy.make("api.Task", user_id=users["superuser"].pk) - revoke_all_access_s3bucket(bucket.pk, bucket.created_by) + revoke_all_access_s3bucket(bucket.pk, task.user_id) - user_access.revoke_bucket_access.assert_called_once() - app_access.revoke_bucket_access.assert_called_once() - policy_access.revoke_bucket_access.assert_called_once() + user_access.revoke_bucket_access.assert_called_once_with( + revoked_by=users["superuser"], + ) + app_access.revoke_bucket_access.assert_called_once_with( + revoked_by=users["superuser"], + ) + policy_access.revoke_bucket_access.assert_called_once_with( + revoked_by=users["superuser"], + )