Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dpat 1749 #1212

Merged
merged 10 commits into from
Oct 19, 2023
31 changes: 31 additions & 0 deletions controlpanel/api/migrations/0031_add_soft_delete_fields.py
Original file line number Diff line number Diff line change
@@ -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),
),
]
25 changes: 25 additions & 0 deletions controlpanel/api/models/s3bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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()
6 changes: 6 additions & 0 deletions controlpanel/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)


Expand Down
2 changes: 2 additions & 0 deletions controlpanel/api/tasks/handlers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
CreateS3Bucket,
GrantAppS3BucketAccess,
GrantUserS3BucketAccess,
S3BucketRevokeAllAccess,
S3BucketRevokeAppAccess,
S3BucketRevokeUserAccess,
)
Expand All @@ -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())
21 changes: 20 additions & 1 deletion controlpanel/api/tasks/handlers/s3.py
Original file line number Diff line number Diff line change
@@ -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


Expand Down Expand Up @@ -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
michaeljcollinsuk marked this conversation as resolved.
Show resolved Hide resolved
instance.revoke_bucket_access()
13 changes: 13 additions & 0 deletions controlpanel/api/tasks/s3bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions controlpanel/frontend/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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):
Expand Down
25 changes: 19 additions & 6 deletions controlpanel/frontend/jinja2/datasource-detail.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,21 @@
<h1 class="govuk-heading-xl">{{ page_title }}</h1>

<p class="govuk-body">
<a href="{{ bucket.aws_url }}" class="govuk-button govuk-button--secondary" target="_blank" rel="noopener">
Open on AWS
</a>
{% if bucket.is_deleted %}
<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>
michaeljcollinsuk marked this conversation as resolved.
Show resolved Hide resolved
{% else %}
<a href="{{ bucket.aws_url }}" class="govuk-button govuk-button--secondary" target="_blank" rel="noopener">
Open on AWS
</a>
{% endif %}
</p>

<section class="cpanel-section track_task">
Expand Down Expand Up @@ -63,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
Expand All @@ -83,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
Expand Down Expand Up @@ -212,6 +224,7 @@ <h2 class="govuk-heading-m">Data access log</h2>
</section>
{% endif %}

<!-- TODO replace with a restore button when bucket has been soft-deleted -->
{% if request.user.has_perm('api.destroy_s3bucket', bucket) %}
<section class="cpanel-section">
<form action="{{ url('delete-datasource', kwargs={ "pk": bucket.id }) }}" method="post">
Expand Down
13 changes: 9 additions & 4 deletions controlpanel/frontend/jinja2/datasource-list.html
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
Expand Down Expand Up @@ -71,4 +71,9 @@ <h3 class="govuk-heading-m">Other {{ datasource_type }} data sources</h3>
</table>
{% endif %}

{% if request.user.is_superuser and deleted_datasources %}
<h3 class="govuk-heading-m">Deleted data sources</h3>
{{ datasource_list(deleted_datasources, datasource_type|default(""), request.user) }}
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea to show this on admin page on a separated group


{% endblock %}
36 changes: 26 additions & 10 deletions controlpanel/frontend/views/datasource.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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):
Expand All @@ -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 = {}
Expand All @@ -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):
Expand All @@ -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"]
Expand Down Expand Up @@ -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:
Expand Down
18 changes: 18 additions & 0 deletions tests/api/models/test_s3bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Loading
Loading