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
4 changes: 2 additions & 2 deletions controlpanel/api/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",)


Expand Down
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),
),
]
1 change: 1 addition & 0 deletions controlpanel/api/models/policys3bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
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())
24 changes: 23 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,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
michaeljcollinsuk marked this conversation as resolved.
Show resolved Hide resolved

instance.current_user = task_user
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
32 changes: 23 additions & 9 deletions controlpanel/frontend/jinja2/datasource-detail.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,25 @@
<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">
<h2 class="govuk-heading-m">Users and groups with access</h2>
<h2 class="govuk-heading-m">Users and groups with{% if bucket.is_deleted %} revoked{% endif %} access</h2>
<table class="govuk-table">
<thead class="govuk-table__head">
<tr class="govuk-table__row">
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 @@ -73,25 +85,27 @@ <h2 class="govuk-heading-m">Users and groups with access</h2>
</tr>
{% endfor %}
</tbody>

{% set plural = access_list|length > 1 %}
<tfoot class="govuk-table__foot">
<tr class="govuk-table__row">
<td class="govuk-table__cell" colspan="3">
{{ 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
</tr>
</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,7 +226,7 @@ <h2 class="govuk-heading-m">Data access log</h2>
</section>
{% endif %}

{% if request.user.has_perm('api.destroy_s3bucket', bucket) %}
{% if not bucket.is_deleted and request.user.has_perm('api.destroy_s3bucket', bucket) %}
<section class="cpanel-section">
<form action="{{ url('delete-datasource', kwargs={ "pk": bucket.id }) }}" method="post">
{{ csrf_input }}
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 %}
Loading
Loading