Add task to revoke all access to a S3bucket when it is soft deleted

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()

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())

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()

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

controlpanel/frontend/jinja2/datasource-detail.html

@@ -21,9 +21,15 @@ <h1 class="govuk-heading-xl">{{ page_title }}</h1>
 
 <p class="govuk-body">
   {% if bucket.is_deleted %}
-    <p>
-      This bucket was deleted by <a href="{{ url('manage-user', kwargs={ "pk": bucket.deleted_by.auth0_id }) }}">{{ user_name(bucket.deleted_by) }}</a> on {{ bucket.deleted_at.strftime("%Y/%m/%d %H:%M:%S") }}.
-    </p>
+    <div class="govuk-warning-text">
+      <span class="govuk-warning-text__icon" aria-hidden="true">!</span>
+      <strong class="govuk-warning-text__text">
+          <span class="govuk-warning-text__assistive">Warning</span>
+          This bucket was deleted by <a href="{{ url('manage-user', kwargs={ "pk": bucket.deleted_by.auth0_id }) }}">
+          {{ user_name(bucket.deleted_by) }}</a> on {{ bucket.deleted_at.strftime("%Y/%m/%d %H:%M:%S") }}.<br>
+          All access listed below has been revoked in IAM.
+      </strong>
+    </div>
   {% else %}
     <a href="{{ bucket.aws_url }}" class="govuk-button govuk-button--secondary" target="_blank" rel="noopener">
       Open on AWS
@@ -69,7 +75,7 @@ <h2 class="govuk-heading-m">Users and groups with access</h2>
         {%- endif %}
         </td>
         <td class="govuk-table__cell">
-          {% if request.user.has_perm('api.update_users3bucket', member) %}
+          {% if request.user.has_perm('api.update_users3bucket', member) and not bucket.is_deleted %}
           <a href="{% if member.user -%}{{ url("update-access-level", kwargs={"pk": member.id}) }}{%- else -%}{{ url("update-policy-access-level", kwargs={"pk": member.id}) }}{%- endif %}"
              class="govuk-button govuk-button--secondary right">
             Edit access level
@@ -89,15 +95,15 @@ <h2 class="govuk-heading-m">Users and groups with access</h2>
     </tfoot>
   </table>
 
-  {% 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 %}
     <a href="{{ url('grant-datasource-access', kwargs={'pk': bucket.pk}) }}"
        class="govuk-button govuk-button--secondary">
       Grant user access
     </a>
   {% 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 %}
       <a href="{{ url('grant-datasource-policy-access', kwargs={'pk': bucket.pk}) }}"
          class="govuk-button govuk-button--secondary">
         Grant group access

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,
+    )

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()

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)