diff --git a/controlpanel/api/admin.py b/controlpanel/api/admin.py index c64022b71..22d6bcf05 100644 --- a/controlpanel/api/admin.py +++ b/controlpanel/api/admin.py @@ -24,8 +24,8 @@ class AppAdmin(admin.ModelAdmin): class S3Admin(admin.ModelAdmin): - list_display = ("name", "created_by", "created", "is_data_warehouse") - list_filter = ("created_by", "is_data_warehouse") + list_display = ("name", "created_by", "created", "is_data_warehouse", "is_deleted") + list_filter = ("created_by", "is_data_warehouse", "is_deleted") search_fields = ("name",) diff --git a/controlpanel/api/migrations/0031_add_soft_delete_fields.py b/controlpanel/api/migrations/0031_add_soft_delete_fields.py new file mode 100644 index 000000000..c66261360 --- /dev/null +++ b/controlpanel/api/migrations/0031_add_soft_delete_fields.py @@ -0,0 +1,31 @@ +# Generated by Django 4.2.1 on 2023-10-09 15:33 + +# Third-party +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0030_task'), + ] + + operations = [ + migrations.AddField( + model_name='s3bucket', + name='deleted_at', + field=models.DateTimeField(null=True), + ), + migrations.AddField( + model_name='s3bucket', + name='deleted_by', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='deleted_s3buckets', to=settings.AUTH_USER_MODEL), + ), + migrations.AddField( + model_name='s3bucket', + name='is_deleted', + field=models.BooleanField(default=False), + ), + ] diff --git a/controlpanel/api/models/policys3bucket.py b/controlpanel/api/models/policys3bucket.py index 6781f279e..feb7e4299 100644 --- a/controlpanel/api/models/policys3bucket.py +++ b/controlpanel/api/models/policys3bucket.py @@ -39,6 +39,7 @@ def grant_bucket_access(self): ) def revoke_bucket_access(self): + # 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/s3bucket.py b/controlpanel/api/models/s3bucket.py index 90de336f3..9d508b905 100644 --- a/controlpanel/api/models/s3bucket.py +++ b/controlpanel/api/models/s3bucket.py @@ -3,16 +3,19 @@ # Third-party from django.conf import settings +from django.contrib.auth.models import User from django.core.validators import MinLengthValidator from django.db import models from django.db.models import Q from django.db.transaction import atomic +from django.utils import timezone from django_extensions.db.models import TimeStampedModel # First-party/Local 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): @@ -56,6 +59,14 @@ class S3Bucket(TimeStampedModel): is_data_warehouse = models.BooleanField(default=False) # TODO remove this field - it's unused location_url = models.CharField(max_length=128, null=True) + is_deleted = models.BooleanField(default=False) + deleted_by = models.ForeignKey( + "User", + on_delete=models.SET_NULL, + null=True, + related_name="deleted_s3buckets" + ) + deleted_at = models.DateTimeField(null=True) objects = S3BucketQuerySet.as_manager() @@ -158,3 +169,17 @@ def delete(self, *args, **kwargs): if not self.is_folder: self.cluster.mark_for_archival() super().delete(*args, **kwargs) + + def soft_delete(self, deleted_by: User): + """ + Mark the object as deleted, but do not remove it from the database + """ + self.is_deleted = True + self.deleted_by = deleted_by + self.deleted_at = timezone.now() + self.save() + # 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/serializers.py b/controlpanel/api/serializers.py index ac17f4114..7761379c4 100644 --- a/controlpanel/api/serializers.py +++ b/controlpanel/api/serializers.py @@ -209,12 +209,18 @@ class Meta: "created_by", "is_data_warehouse", "location_url", + "is_deleted", + "deleted_by", + "deleted_at", ) read_only_fields = ( "apps3buckets", "users3buckets", "created_by", "url", + "is_deleted", + "deleted_by", + "deleted_at", ) 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..69b7ab0d7 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,24 @@ 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 + """ + 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.current_user = task_user + 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/forms.py b/controlpanel/frontend/forms.py index 9341dd333..4fe857c1c 100644 --- a/controlpanel/frontend/forms.py +++ b/controlpanel/frontend/forms.py @@ -146,7 +146,7 @@ class CreateAppForm(AppAuth0Form): required=False, ) existing_datasource_id = DatasourceChoiceField( - queryset=S3Bucket.objects.filter(is_data_warehouse=False), + queryset=S3Bucket.objects.filter(is_data_warehouse=False, is_deleted=False), empty_label="Select", required=False, ) @@ -353,9 +353,11 @@ def __init__(self, *args, **kwargs): if self.exclude_connected: self.fields["datasource"].queryset = S3Bucket.objects.exclude( id__in=[a.s3bucket_id for a in self.app.apps3buckets.all()], - ) + ).filter(is_deleted=False) else: - self.fields["datasource"].queryset = S3Bucket.objects.all() + self.fields["datasource"].queryset = S3Bucket.objects.filter( + is_deleted=False, + ) class CreateIAMManagedPolicyForm(forms.Form): diff --git a/controlpanel/frontend/jinja2/datasource-detail.html b/controlpanel/frontend/jinja2/datasource-detail.html index 0c133010a..82106174a 100644 --- a/controlpanel/frontend/jinja2/datasource-detail.html +++ b/controlpanel/frontend/jinja2/datasource-detail.html @@ -20,13 +20,25 @@

{{ page_title }}

- - Open on AWS - + {% if bucket.is_deleted %} +

+ + + 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 + + {% endif %}

-

Users and groups with access

+

Users and groups with{% if bucket.is_deleted %} revoked{% endif %} access

@@ -63,7 +75,7 @@

Users and groups with access

{%- endif %} {% endfor %} + + {% set plural = access_list|length > 1 %}
- {% 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 @@ -73,17 +85,19 @@

Users and groups with access
{{ access_list|length }} - user{%- if access_list|length != 1 -%}s{% endif %} or group{%- if access_list|length != 1 -%}s have{% else %} has{% endif %} + user{%- if plural -%}s{% endif %} or group{%- if plural -%}s{% endif %}{% if bucket.is_deleted %} had{% elif plural %} have{% else %} has{% endif %} access to this {{ datasource_type }} data source
- {% 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 @@ -91,7 +105,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 @@ -212,7 +226,7 @@

Data access log

{% endif %} -{% if request.user.has_perm('api.destroy_s3bucket', bucket) %} +{% if not bucket.is_deleted and request.user.has_perm('api.destroy_s3bucket', bucket) %}
{{ csrf_input }} diff --git a/controlpanel/frontend/jinja2/datasource-list.html b/controlpanel/frontend/jinja2/datasource-list.html index 8e2027d88..7c575bc6a 100644 --- a/controlpanel/frontend/jinja2/datasource-list.html +++ b/controlpanel/frontend/jinja2/datasource-list.html @@ -4,12 +4,12 @@ {% extends "base.html" %} -{% if datasource_type %} - {% set page_name = datasource_type + "-datasource-list" %} - {% set page_title = "Your " + datasource_type + " data sources" %} -{% else %} +{% if all_datasources %} {% set page_name = "all-datasources" %} {% set page_title = "All data sources" %} +{% else %} + {% set page_name = datasource_type + "-datasource-list" %} + {% set page_title = "Your " + datasource_type + " data sources" %} {% endif %} {% set access_levels_html %} @@ -71,4 +71,9 @@

Other {{ datasource_type }} data sources

{% endif %} +{% if request.user.is_superuser and deleted_datasources %} +

Deleted data sources

+ {{ datasource_list(deleted_datasources, datasource_type|default(""), request.user) }} +{% endif %} + {% endblock %} diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py index 5099089ea..94c79b12a 100644 --- a/controlpanel/frontend/views/datasource.py +++ b/controlpanel/frontend/views/datasource.py @@ -6,6 +6,7 @@ from django.contrib import messages from django.core.exceptions import PermissionDenied from django.db import transaction +from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404 from django.urls import reverse_lazy from django.views.generic.base import ContextMixin @@ -44,7 +45,7 @@ class DatasourceMixin(ContextMixin): def get_context_data(self, *args, **kwargs): context = super().get_context_data(*args, **kwargs) - context["all-datasources"] = self.all_datasources + context["all_datasources"] = self.all_datasources context["datasource_type"] = self.get_datasource_type() return context @@ -75,6 +76,7 @@ def get_queryset(self): return S3Bucket.objects.prefetch_related("users3buckets").filter( is_data_warehouse=self.datasource_type == "warehouse", users3buckets__user=self.request.user, + is_deleted=False, ) def get_context_data(self, *args, **kwargs): @@ -84,6 +86,7 @@ def get_context_data(self, *args, **kwargs): "users3buckets__user" ).filter( is_data_warehouse=self.datasource_type == "warehouse", + is_deleted=False, ) other_datasources = all_datasources.exclude(id__in=self.get_queryset()) other_datasources_admins = {} @@ -103,7 +106,14 @@ class AdminBucketList(BucketList): permission_required = "api.is_superuser" def get_queryset(self): - return S3Bucket.objects.prefetch_related("users3buckets").all() + return S3Bucket.objects.prefetch_related("users3buckets").filter( + is_deleted=False + ) + + def get_context_data(self, *args, **kwargs): + context = super().get_context_data(*args, **kwargs) + context["deleted_datasources"] = S3Bucket.objects.filter(is_deleted=True) + return context class WebappBucketList(BucketList): @@ -121,6 +131,12 @@ class BucketDetail( permission_required = "api.retrieve_s3bucket" template_name = "datasource-detail.html" + def get_queryset(self): + queryset = super().get_queryset() + if not self.request.user.is_superuser: + queryset = queryset.filter(is_deleted=False) + return queryset + def get_context_data(self, *args, **kwargs): context = super().get_context_data(*args, **kwargs) bucket = kwargs["object"] @@ -198,16 +214,16 @@ class DeleteDatasource( permission_required = "api.destroy_s3bucket" success_url = reverse_lazy("list-warehouse-datasources") - def delete(self, *args, **kwargs): - bucket = self.get_object() - if not bucket.is_data_warehouse: - self.success_url = reverse_lazy("list-webapp-datasources") - - response = super().delete(*args, **kwargs) + def get_success_url(self): + if not self.object.is_data_warehouse: + return reverse_lazy("list-webapp-datasources") + return self.success_url + def form_valid(self, *args, **kwargs): + self.object = self.get_object() + self.object.soft_delete(deleted_by=self.request.user) messages.success(self.request, "Successfully deleted data source") - - return response + return HttpResponseRedirect(self.get_success_url()) class UpdateAccessLevelMixin: diff --git a/tests/api/models/test_s3bucket.py b/tests/api/models/test_s3bucket.py index 9d9d89f95..67dcd6db3 100644 --- a/tests/api/models/test_s3bucket.py +++ b/tests/api/models/test_s3bucket.py @@ -96,3 +96,21 @@ def test_is_folder(name, expected): ) def test_cluster(name, expected): assert isinstance(S3Bucket(name=name).cluster, expected) + + +def test_soft_delete(bucket, users, sqs, helpers): + user = users["superuser"] + + assert bucket.is_deleted is False + with patch("controlpanel.api.cluster.S3Bucket.mark_for_archival") as archive: + bucket.soft_delete(deleted_by=user) + + assert bucket.is_deleted is True + 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..368283731 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,21 @@ 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(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, 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() diff --git a/tests/api/views/test_s3bucket.py b/tests/api/views/test_s3bucket.py index 29da0ca30..00e3baa85 100644 --- a/tests/api/views/test_s3bucket.py +++ b/tests/api/views/test_s3bucket.py @@ -54,6 +54,9 @@ def test_detail(client, bucket): "created_by", "is_data_warehouse", "location_url", + "is_deleted", + "deleted_by", + "deleted_at", } assert set(response.data) == expected_s3bucket_fields diff --git a/tests/frontend/views/test_datasource.py b/tests/frontend/views/test_datasource.py index 1f43f02a0..2f4a892ac 100644 --- a/tests/frontend/views/test_datasource.py +++ b/tests/frontend/views/test_datasource.py @@ -3,7 +3,8 @@ # Third-party import pytest -from django.urls import reverse +from django.conf import settings +from django.urls import reverse, reverse_lazy from model_mommy import mommy from rest_framework import status @@ -101,9 +102,10 @@ def list_all(client, *args): return client.get(reverse("list-all-datasources")) -def detail(client, buckets, *args): +def detail(client, buckets, *args, bucket=None): + bucket = bucket or buckets["warehouse1"] return client.get( - reverse("manage-datasource", kwargs={"pk": buckets["warehouse1"].id}) + reverse("manage-datasource", kwargs={"pk": bucket.id}) ) @@ -115,9 +117,10 @@ def create(client, *args, **kwargs): return client.post(reverse("create-datasource") + "?type=warehouse", data) -def delete(client, buckets, *args): - return client.delete( - reverse("delete-datasource", kwargs={"pk": buckets["warehouse1"].id}) +def delete(client, buckets, *args, bucket=None): + bucket = bucket or buckets["warehouse1"] + return client.post( + reverse("delete-datasource", kwargs={"pk": bucket.id}) ) @@ -213,23 +216,24 @@ def test_access_permissions(client, users3buckets, users, view, user, expected_s @pytest.mark.parametrize( - "view,user,expected_count", + "view,user,expected_count,show_deleted", [ - (list_warehouse, "superuser", 0), - (list_warehouse, "normal_user", 0), - (list_warehouse, "bucket_viewer", 1), - (list_warehouse, "bucket_admin", 2), - (list_app_data, "superuser", 0), - (list_app_data, "normal_user", 0), - (list_app_data, "bucket_viewer", 1), - (list_app_data, "bucket_admin", 2), - (list_all, "superuser", 5), + (list_warehouse, "superuser", 0, False), + (list_warehouse, "normal_user", 0, False), + (list_warehouse, "bucket_viewer", 1, False), + (list_warehouse, "bucket_admin", 2, False), + (list_app_data, "superuser", 0, False), + (list_app_data, "normal_user", 0, False), + (list_app_data, "bucket_viewer", 1, False), + (list_app_data, "bucket_admin", 2, False), + (list_all, "superuser", 5, True), ], ) -def test_list(client, buckets, users, view, user, expected_count): +def test_list(client, buckets, users, view, user, expected_count, show_deleted): client.force_login(users[user]) response = view(client, buckets, users) assert len(response.context_data["object_list"]) == expected_count + assert ("deleted_datasources" in response.context_data) is show_deleted @pytest.mark.parametrize( @@ -392,3 +396,66 @@ def test_grant_access_invalid_form(client, users3buckets, users, kwargs): assert response.status_code == 200 assert response.context_data["form"].is_valid() is False + + +@pytest.mark.parametrize( + "bucket, success_url", + [ + ("warehouse1", reverse_lazy("list-warehouse-datasources")), + ("app_data1", reverse_lazy("list-webapp-datasources")), + ] +) +def test_delete_calls_soft_delete( + client, buckets, users, bucket, success_url, sqs, helpers, +): + admin = users["bucket_admin"] + bucket = buckets[bucket] + + client.force_login(admin) + response = delete(client, buckets, bucket=bucket) + bucket.refresh_from_db() + + assert bucket.pk is not None + assert bucket.is_deleted is True + assert bucket.deleted_by == admin + 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", + [ + ("superuser", "app_data1", status.HTTP_200_OK), + ("superuser", "app_data2", status.HTTP_200_OK), + ("superuser", "warehouse1", status.HTTP_200_OK), + ("superuser", "warehouse2", status.HTTP_200_OK), + ("superuser", "other", status.HTTP_200_OK), + ("bucket_viewer", "app_data1", status.HTTP_404_NOT_FOUND), + ("bucket_viewer", "app_data2", status.HTTP_404_NOT_FOUND), + ("bucket_viewer", "warehouse1", status.HTTP_404_NOT_FOUND), + ("bucket_viewer", "warehouse2", status.HTTP_404_NOT_FOUND), + ("bucket_viewer", "other", status.HTTP_404_NOT_FOUND), + ("bucket_admin", "app_data1", status.HTTP_404_NOT_FOUND), + ("bucket_admin", "app_data2", status.HTTP_404_NOT_FOUND), + ("bucket_admin", "warehouse1", status.HTTP_404_NOT_FOUND), + ("bucket_admin", "warehouse2", status.HTTP_404_NOT_FOUND), + ("bucket_admin", "other", status.HTTP_404_NOT_FOUND), + + ] +) +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) + + client.force_login(user) + response = detail(client, user, bucket=bucket) + + assert response.status_code == expected_status