From 4e6de07ebb4d040ff93eef91a2e1f0ffcf6796d7 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Fri, 13 Oct 2023 14:43:42 +0100 Subject: [PATCH] Add task to revoke all access to a S3bucket when it is soft deleted --- controlpanel/api/models/s3bucket.py | 3 +++ controlpanel/api/tasks/handlers/__init__.py | 2 ++ controlpanel/api/tasks/handlers/s3.py | 21 +++++++++++++++++- controlpanel/api/tasks/s3bucket.py | 13 +++++++++++ .../frontend/jinja2/datasource-detail.html | 18 ++++++++++----- tests/api/models/test_s3bucket.py | 7 +++++- tests/api/tasks/test_s3.py | 22 ++++++++++++++++++- tests/frontend/views/test_datasource.py | 14 ++++++++++-- 8 files changed, 89 insertions(+), 11 deletions(-) diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py index 8d72ed467..9d508b905 100644 --- a/controlpanel/api/models/s3bucket.py +++ b/controlpanel/api/models/s3bucket.py @@ -15,6 +15,7 @@ from controlpanel.api import cluster, tasks, validators from controlpanel.api.models.apps3bucket import AppS3Bucket from controlpanel.api.models.users3bucket import UserS3Bucket +from controlpanel.api.tasks.s3bucket import S3BucketRevokeAllAccess def s3bucket_console_url(name): @@ -180,3 +181,5 @@ def soft_delete(self, deleted_by: User): # TODO update to handle deleting folders if not self.is_folder: self.cluster.mark_for_archival() + + S3BucketRevokeAllAccess(self, self.deleted_by).create_task() diff --git a/controlpanel/api/tasks/handlers/__init__.py b/controlpanel/api/tasks/handlers/__init__.py index 454fd9094..f51323c79 100644 --- a/controlpanel/api/tasks/handlers/__init__.py +++ b/controlpanel/api/tasks/handlers/__init__.py @@ -5,6 +5,7 @@ CreateS3Bucket, GrantAppS3BucketAccess, GrantUserS3BucketAccess, + S3BucketRevokeAllAccess, S3BucketRevokeAppAccess, S3BucketRevokeUserAccess, ) @@ -16,3 +17,4 @@ create_app_auth_settings = celery_app.register_task(CreateAppAuthSettings()) revoke_user_s3bucket_access = celery_app.register_task(S3BucketRevokeUserAccess()) revoke_app_s3bucket_access = celery_app.register_task(S3BucketRevokeAppAccess()) +revoke_all_access_s3bucket = celery_app.register_task(S3BucketRevokeAllAccess()) diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py index fd8fe922e..f37e90b19 100644 --- a/controlpanel/api/tasks/handlers/s3.py +++ b/controlpanel/api/tasks/handlers/s3.py @@ -1,9 +1,10 @@ # Third-party -from celery import Task as CeleryTask +from django.db.models.deletion import Collector # First-party/Local from controlpanel.api import cluster from controlpanel.api.models import App, AppS3Bucket, S3Bucket, User, UserS3Bucket +from controlpanel.api.models.access_to_s3bucket import AccessToS3Bucket from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler, BaseTaskHandler @@ -73,3 +74,21 @@ def handle(self, bucket_arn, app_pk): self.complete() cluster.App(app).revoke_bucket_access(bucket_arn) self.complete() + + +class S3BucketRevokeAllAccess(BaseModelTaskHandler): + model = S3Bucket + name = "s3bucket_revoke_all_access" + + def handle(self, *args, **kwargs): + """ + When an S3Bucket is soft-deleted, the related objects that handle access will + 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 + """ + 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() diff --git a/controlpanel/api/tasks/s3bucket.py b/controlpanel/api/tasks/s3bucket.py index c990ca7a1..4e3e8944d 100644 --- a/controlpanel/api/tasks/s3bucket.py +++ b/controlpanel/api/tasks/s3bucket.py @@ -25,6 +25,19 @@ def _get_args_list(self): ] +class S3BucketRevokeAllAccess(TaskBase): + ENTITY_CLASS = "S3Bucket" + QUEUE_NAME = settings.S3_QUEUE_NAME + + @property + def task_name(self): + return "s3bucket_revoke_all_access" + + @property + def task_description(self): + return "Revokes all access to an S3 bucket" + + class S3AccessMixin: ACTION = None ROLE = None diff --git a/controlpanel/frontend/jinja2/datasource-detail.html b/controlpanel/frontend/jinja2/datasource-detail.html index d7ac893f2..24c34a158 100644 --- a/controlpanel/frontend/jinja2/datasource-detail.html +++ b/controlpanel/frontend/jinja2/datasource-detail.html @@ -21,9 +21,15 @@

{{ page_title }}

{% if bucket.is_deleted %} -

- This bucket was deleted by {{ user_name(bucket.deleted_by) }} on {{ bucket.deleted_at.strftime("%Y/%m/%d %H:%M:%S") }}. -

+
+ + + Warning + This bucket was deleted by + {{ user_name(bucket.deleted_by) }} on {{ bucket.deleted_at.strftime("%Y/%m/%d %H:%M:%S") }}.
+ All access listed below has been revoked in IAM. + +
{% else %} Open on AWS @@ -69,7 +75,7 @@

Users and groups with access

{%- endif %} - {% if request.user.has_perm('api.update_users3bucket', member) %} + {% if request.user.has_perm('api.update_users3bucket', member) and not bucket.is_deleted %} Edit access level @@ -89,7 +95,7 @@

Users and groups with access

- {% if request.user.has_perm('api.create_users3bucket', bucket) and users_options|length %} + {% if request.user.has_perm('api.create_users3bucket', bucket) and users_options|length and not bucket.is_deleted %} Grant user access @@ -97,7 +103,7 @@

Users and groups with access

{% endif %} {% if request.user.has_perm('api.manage_groups') %} - {% if request.user.has_perm('api.create_policys3bucket', bucket) and policies_options|length %} + {% if request.user.has_perm('api.create_policys3bucket', bucket) and policies_options|length and not bucket.is_deleted %} Grant group access diff --git a/tests/api/models/test_s3bucket.py b/tests/api/models/test_s3bucket.py index deb17e0aa..67dcd6db3 100644 --- a/tests/api/models/test_s3bucket.py +++ b/tests/api/models/test_s3bucket.py @@ -98,7 +98,7 @@ def test_cluster(name, expected): assert isinstance(S3Bucket(name=name).cluster, expected) -def test_soft_delete(bucket, users): +def test_soft_delete(bucket, users, sqs, helpers): user = users["superuser"] assert bucket.is_deleted is False @@ -109,3 +109,8 @@ def test_soft_delete(bucket, users): assert bucket.deleted_by == user assert bucket.deleted_at is not None archive.assert_called_once() + + messages = helpers.retrieve_messages(sqs, queue_name=settings.S3_QUEUE_NAME) + helpers.validate_task_with_sqs_messages( + messages, S3Bucket.__name__, bucket.id, queue_name=settings.S3_QUEUE_NAME, + ) diff --git a/tests/api/tasks/test_s3.py b/tests/api/tasks/test_s3.py index dc275accf..88b329dd4 100644 --- a/tests/api/tasks/test_s3.py +++ b/tests/api/tasks/test_s3.py @@ -1,6 +1,8 @@ +# Standard library +from unittest.mock import MagicMock, patch + # Third-party import pytest -from mock import patch from model_mommy import mommy # First-party/Local @@ -9,6 +11,7 @@ create_s3bucket, grant_app_s3bucket_access, grant_user_s3bucket_access, + revoke_all_access_s3bucket, revoke_app_s3bucket_access, revoke_user_s3bucket_access, ) @@ -112,3 +115,20 @@ def test_revoke_app_access(cluster, complete): app_bucket_access.s3bucket.arn ) complete.assert_called_once() + + +@pytest.mark.django_db +@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(): + 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) + + revoke_all_access_s3bucket(bucket.pk, bucket.created_by) + + user_access.revoke_bucket_access.assert_called_once() + app_access.revoke_bucket_access.assert_called_once() + policy_access.revoke_bucket_access.assert_called_once() diff --git a/tests/frontend/views/test_datasource.py b/tests/frontend/views/test_datasource.py index 464f3ece9..2f4a892ac 100644 --- a/tests/frontend/views/test_datasource.py +++ b/tests/frontend/views/test_datasource.py @@ -3,6 +3,7 @@ # Third-party import pytest +from django.conf import settings from django.urls import reverse, reverse_lazy from model_mommy import mommy from rest_framework import status @@ -404,7 +405,9 @@ def test_grant_access_invalid_form(client, users3buckets, users, kwargs): ("app_data1", reverse_lazy("list-webapp-datasources")), ] ) -def test_delete_calls_soft_delete(client, buckets, users, bucket, success_url): +def test_delete_calls_soft_delete( + client, buckets, users, bucket, success_url, sqs, helpers, +): admin = users["bucket_admin"] bucket = buckets[bucket] @@ -418,6 +421,11 @@ def test_delete_calls_soft_delete(client, buckets, users, bucket, success_url): assert bucket.deleted_at is not None assert response.url == success_url + messages = helpers.retrieve_messages(sqs, queue_name=settings.S3_QUEUE_NAME) + helpers.validate_task_with_sqs_messages( + messages, S3Bucket.__name__, bucket.id, queue_name=settings.S3_QUEUE_NAME, + ) + @pytest.mark.parametrize( "user, bucket, expected_status", @@ -440,7 +448,9 @@ def test_delete_calls_soft_delete(client, buckets, users, bucket, success_url): ] ) -def test_detail_for_deleted_datasouce(client, buckets, users, user, bucket, expected_status): +def test_detail_for_deleted_datasource( + client, buckets, users, user, bucket, expected_status +): user = users[user] bucket = buckets[bucket] bucket.soft_delete(deleted_by=user)