From df9c143fe2922800934749043addfa532defc509 Mon Sep 17 00:00:00 2001 From: Dharshana Vanderbona Date: Wed, 12 Jun 2024 10:56:44 +0100 Subject: [PATCH 1/2] Remove S3 urls from application UI This is done by doing the following. - When uploading files, always upload to a folder called temporary_files. - Update the save method of the application, so that we copy any files still pointing to the temporary_files folder and pint to the new location (applications/) for each file field. Also, the old file is deleted from the temporary location. - Implement new view method to retrieve the file from S3 and serve instead of exposing the S3 urls. The application pages as well as the admin pages have been updated with specific view methods to download temporary as well as application related files. - Increase the width of the file field columns to 255 so we can store the file paths --- .../request/admin/model_admins.py | 78 +++++++++++++++---- request_a_govuk_domain/request/db.py | 10 ++- ...n_ministerial_request_evidence_and_more.py | 46 +++++++++++ .../request/models/application.py | 54 +++++++++++-- .../request/models/storage_util.py | 8 ++ .../templates/admin/clearable_file_input.html | 14 ++-- .../request/templates/confirm.html | 6 +- .../templates/exemption_upload_confirm.html | 2 +- .../templates/minister_upload_confirm.html | 2 +- .../written_permission_upload_confirm.html | 2 +- request_a_govuk_domain/request/views.py | 26 ++++++- request_a_govuk_domain/urls.py | 2 + 12 files changed, 213 insertions(+), 37 deletions(-) create mode 100644 request_a_govuk_domain/request/migrations/0009_alter_application_ministerial_request_evidence_and_more.py diff --git a/request_a_govuk_domain/request/admin/model_admins.py b/request_a_govuk_domain/request/admin/model_admins.py index e559fb6f..61d3f30b 100644 --- a/request_a_govuk_domain/request/admin/model_admins.py +++ b/request_a_govuk_domain/request/admin/model_admins.py @@ -22,6 +22,50 @@ Registrar, ) from .forms import ReviewForm +from ..models.storage_util import s3_root_storage + + +class FileDownloadMixin: + """ + Provide file download support for the given admin class. + """ + + def download_file_view(self, request, object_id, field_name): + """ + Implement this method to provide the specific retrieval of the object based on the given id + :param request: Current request object + :param object_id: id of the object containing the filed + :param field_name: which file field to download from the object + :return: + """ + pass + + @property + def uid(self): + """ + Unique id for the current subclass + :return: + """ + pass + + def get_urls(self): + """ + Generate the custom url for the download_file_view. + :return: + """ + urls = super().get_urls() + custom_urls = [ + path( + f"download_{self.uid}///", + self.admin_site.admin_view(self.download_file_view), + name=f"{self.uid}_download_file", + ), + ] + return custom_urls + urls + + def generate_download_link(self, obj, field_name, link_text): + link = reverse(f"admin:{self.uid}_download_file", args=[obj.pk, field_name]) + return format_html(f'{link_text}') class CustomAdminFileWidget(AdminFileWidget): @@ -59,7 +103,7 @@ def convert_to_local_time(obj): ) -class ReviewAdmin(admin.ModelAdmin): +class ReviewAdmin(FileDownloadMixin, admin.ModelAdmin): model = Review form = ReviewForm change_form_template = "admin/review_change_form.html" @@ -81,26 +125,17 @@ class ReviewAdmin(admin.ModelAdmin): "application__registrant_org", ) - def generate_download_link(self, obj, field_name, link_text): - link = reverse("admin:review_download_file", args=[obj.pk, field_name]) - return format_html(f'{link_text}') - def download_file_view(self, request, object_id, field_name): review = self.model.objects.get(id=object_id) application = review.application file = getattr(application, field_name) - return FileResponse(file.open("rb")) + # We need to use root_storage as the default storage always + # refer to the temp_files as the parent. + return FileResponse(s3_root_storage().open(file.name, "rb")) - def get_urls(self): - urls = super().get_urls() - custom_urls = [ - path( - "download///", - self.admin_site.admin_view(self.download_file_view), - name="review_download_file", - ), - ] - return custom_urls + urls + @property + def uid(self): + return "review" def _get_formatted_display_fields(self, display_fields: dict) -> str: return render_to_string( @@ -372,7 +407,7 @@ def has_add_permission(self, request): return False -class ApplicationAdmin(admin.ModelAdmin): +class ApplicationAdmin(FileDownloadMixin, admin.ModelAdmin): model = Application list_display = [ "reference", @@ -394,6 +429,15 @@ class ApplicationAdmin(admin.ModelAdmin): django.db.models.fields.files.FileField: {"widget": CustomAdminFileWidget}, } + def download_file_view(self, request, object_id, field_name): + application = self.model.objects.get(id=object_id) + file = getattr(application, field_name) + return FileResponse(s3_root_storage().open(file.name, "rb")) + + @property + def uid(self): + return "application" + @admin.display(description="Time Submitted (UK time)") def time_submitted_local_time(self, obj): return convert_to_local_time(obj.time_submitted) diff --git a/request_a_govuk_domain/request/db.py b/request_a_govuk_domain/request/db.py index 390277cd..4c342cd9 100644 --- a/request_a_govuk_domain/request/db.py +++ b/request_a_govuk_domain/request/db.py @@ -16,10 +16,10 @@ RegistryPublishedPerson, Review, ) +from .models.storage_util import select_storage from .utils import route_number, is_valid_session_data - logger = logging.getLogger(__name__) @@ -46,7 +46,13 @@ def sanitised_registration_data(rd: dict, session_id: str) -> dict: def clear_upload(name: str) -> None: rd.pop(name, None) - rd.pop(f"{name}_file_uploaded_filename", None) + uploaded_file_name = rd.pop(f"{name}_file_uploaded_filename", None) + # Delete any temporary files that are no longer needed by the application + if uploaded_file_name: + storage = select_storage() + if storage.exists(uploaded_file_name): + storage.delete(uploaded_file_name) + rd.pop(f"{name}_file_original_filename", None) rd.pop(f"{name}_file_uploaded_url", None) diff --git a/request_a_govuk_domain/request/migrations/0009_alter_application_ministerial_request_evidence_and_more.py b/request_a_govuk_domain/request/migrations/0009_alter_application_ministerial_request_evidence_and_more.py new file mode 100644 index 00000000..a33c100c --- /dev/null +++ b/request_a_govuk_domain/request/migrations/0009_alter_application_ministerial_request_evidence_and_more.py @@ -0,0 +1,46 @@ +# Generated by Django 4.2.13 on 2024-06-12 11:49 + +from django.db import migrations, models +import request_a_govuk_domain.request.models.storage_util + + +class Migration(migrations.Migration): + dependencies = [ + ("request", "0008_alter_registrant_type"), + ] + + operations = [ + migrations.AlterField( + model_name="application", + name="ministerial_request_evidence", + field=models.FileField( + blank=True, + max_length=255, + null=True, + storage=request_a_govuk_domain.request.models.storage_util.select_storage, + upload_to="", + ), + ), + migrations.AlterField( + model_name="application", + name="policy_exemption_evidence", + field=models.FileField( + blank=True, + max_length=255, + null=True, + storage=request_a_govuk_domain.request.models.storage_util.select_storage, + upload_to="", + ), + ), + migrations.AlterField( + model_name="application", + name="written_permission_evidence", + field=models.FileField( + blank=True, + max_length=255, + null=True, + storage=request_a_govuk_domain.request.models.storage_util.select_storage, + upload_to="", + ), + ), + ] diff --git a/request_a_govuk_domain/request/models/application.py b/request_a_govuk_domain/request/models/application.py index 87e87cd2..02c84b9e 100644 --- a/request_a_govuk_domain/request/models/application.py +++ b/request_a_govuk_domain/request/models/application.py @@ -1,9 +1,11 @@ -from django.utils.translation import gettext_lazy as _ -from django.db import models from django.contrib.auth.models import User -from .person import RegistryPublishedPerson, RegistrarPerson, RegistrantPerson +from django.db import models +from django.utils.translation import gettext_lazy as _ + from .organisation import Registrant, Registrar +from .person import RegistryPublishedPerson, RegistrarPerson, RegistrantPerson from .storage_util import select_storage +from ...settings import IS_AWS REF_NUM_LENGTH = 17 @@ -62,14 +64,54 @@ class Application(models.Model): registrant_org = models.ForeignKey(Registrant, on_delete=models.CASCADE) registrar_org = models.ForeignKey(Registrar, on_delete=models.CASCADE) written_permission_evidence = models.FileField( - null=True, blank=True, storage=select_storage + null=True, + blank=True, + storage=select_storage, + max_length=255, ) ministerial_request_evidence = models.FileField( - null=True, blank=True, storage=select_storage + null=True, + blank=True, + storage=select_storage, + max_length=255, ) policy_exemption_evidence = models.FileField( - null=True, blank=True, storage=select_storage + null=True, + blank=True, + storage=select_storage, + max_length=255, ) def __str__(self): return f"{self.reference} - {self.domain_name}" + + def save( + self, force_insert=False, force_update=False, using=None, update_fields=None + ): + """ + When the application is saved, move the temporary files to the applications directory + :param force_insert: + :param force_update: + :param using: + :param update_fields: + :return: + """ + if IS_AWS: + storage = select_storage() + # Move any temporary files in to the application specific folder + for file_field in [ + self.policy_exemption_evidence, + self.ministerial_request_evidence, + self.written_permission_evidence, + ]: + if file_field and not file_field.name.startswith("applications"): + from_path = "temp_files/" + file_field.name + to_path = f"applications/{self.reference}/" + file_field.name + storage.connection.meta.client.copy_object( + Bucket=storage.bucket_name, + CopySource=storage.bucket_name + "/" + from_path, + Key=to_path, + ) + storage.delete(from_path) + file_field.name = to_path + super().save(force_insert, force_update, using, update_fields) diff --git a/request_a_govuk_domain/request/models/storage_util.py b/request_a_govuk_domain/request/models/storage_util.py index afed3532..b76dacc7 100644 --- a/request_a_govuk_domain/request/models/storage_util.py +++ b/request_a_govuk_domain/request/models/storage_util.py @@ -11,3 +11,11 @@ def select_storage(): :return: """ return S3Boto3Storage() if settings.S3_STORAGE_ENABLED else FileSystemStorage() + + +def s3_root_storage(): + """ + Storage instance that allows access from the root level on S3 + :return: + """ + return S3Boto3Storage() diff --git a/request_a_govuk_domain/request/templates/admin/clearable_file_input.html b/request_a_govuk_domain/request/templates/admin/clearable_file_input.html index 72128684..f513cb7d 100644 --- a/request_a_govuk_domain/request/templates/admin/clearable_file_input.html +++ b/request_a_govuk_domain/request/templates/admin/clearable_file_input.html @@ -1,6 +1,10 @@ -{% if widget.is_initial %}

{{ widget.initial_text }}: {{ widget.value }} (opens in new tab){% if not widget.required %} - - +{% if widget.is_initial %}

{{ widget.initial_text }}: + {{ widget.name }} + (opens in new tab){% if not widget.required %} + + {% endif %}
-{{ widget.input_text }}:{% endif %} -{% if widget.is_initial %}

{% endif %} + {{ widget.input_text }}:{% endif %} + +{% if widget.is_initial %}

{% endif %} diff --git a/request_a_govuk_domain/request/templates/confirm.html b/request_a_govuk_domain/request/templates/confirm.html index 54fdb1cf..36827bdc 100644 --- a/request_a_govuk_domain/request/templates/confirm.html +++ b/request_a_govuk_domain/request/templates/confirm.html @@ -100,7 +100,7 @@

Domain request details

{% gds_summary_list_row_value %} {% if registration_data.written_permission == "yes" %} Yes, evidence provided:
- {{ registration_data.exemption_file_original_filename }} + {{ registration_data.exemption_file_original_filename }} {% endif %} {% endgds_summary_list_row_value %} {% gds_summary_list_row_actions %} @@ -118,7 +118,7 @@

Domain request details

{% gds_summary_list_row_value %} {% if registration_data.written_permission == "yes" %} Yes, evidence provided:
- {{ registration_data.written_permission_file_original_filename }} + {{ registration_data.written_permission_file_original_filename }} {% else %} No {# This shouldn't normally happen #} {% endif %} @@ -152,7 +152,7 @@

Domain request details

No evidence provided {% else %} Yes, evidence provided:
- {{ registration_data.minister_file_original_filename }} + {{ registration_data.minister_file_original_filename }} {% endif %} {% endgds_summary_list_row_value %} {% gds_summary_list_row_actions %} diff --git a/request_a_govuk_domain/request/templates/exemption_upload_confirm.html b/request_a_govuk_domain/request/templates/exemption_upload_confirm.html index c2893179..a281f3e9 100644 --- a/request_a_govuk_domain/request/templates/exemption_upload_confirm.html +++ b/request_a_govuk_domain/request/templates/exemption_upload_confirm.html @@ -22,7 +22,7 @@

- {{ registration_data.exemption_file_original_filename }} + {{ registration_data.exemption_file_original_filename }}
diff --git a/request_a_govuk_domain/request/templates/minister_upload_confirm.html b/request_a_govuk_domain/request/templates/minister_upload_confirm.html index 415a7123..7f9338c6 100644 --- a/request_a_govuk_domain/request/templates/minister_upload_confirm.html +++ b/request_a_govuk_domain/request/templates/minister_upload_confirm.html @@ -22,7 +22,7 @@

- {{ registration_data.minister_file_original_filename }} + {{ registration_data.minister_file_original_filename }}
diff --git a/request_a_govuk_domain/request/templates/written_permission_upload_confirm.html b/request_a_govuk_domain/request/templates/written_permission_upload_confirm.html index 855e307d..bb10065b 100644 --- a/request_a_govuk_domain/request/templates/written_permission_upload_confirm.html +++ b/request_a_govuk_domain/request/templates/written_permission_upload_confirm.html @@ -22,7 +22,7 @@

- {{ registration_data.written_permission_file_original_filename }} + {{ registration_data.written_permission_file_original_filename }}
diff --git a/request_a_govuk_domain/request/views.py b/request_a_govuk_domain/request/views.py index d5cbcfb8..bc86a0be 100644 --- a/request_a_govuk_domain/request/views.py +++ b/request_a_govuk_domain/request/views.py @@ -4,9 +4,9 @@ from datetime import datetime from django.db import transaction +from django.http import HttpResponse, HttpRequest, FileResponse, HttpResponseNotFound from django.shortcuts import render, redirect from django.urls import reverse_lazy -from django.http import HttpResponse, HttpRequest from django.views import View from django.views.generic import TemplateView, RedirectView from django.views.generic.edit import FormView @@ -320,6 +320,14 @@ class UploadRemoveView(RedirectView): pattern_name = "" # to be subclassed def get_redirect_url(self, *args, **kwargs): + registration_data = self.request.session.get("registration_data", {}) + if path_to_delete := registration_data.get( + self.page_type + "_file_uploaded_filename" + ): + storage = select_storage() + if storage.exists(path_to_delete): + storage.delete(path_to_delete) + # delete the session data remove_from_session( self.request.session, @@ -329,6 +337,7 @@ def get_redirect_url(self, *args, **kwargs): self.page_type + "_file_uploaded_url", ], ) + return super().get_redirect_url(*args, **kwargs) @@ -500,6 +509,21 @@ def form_valid(self, form): return super().form_valid(form) +def download_file(request, file_type): + """ + Utility method to download a file which was temporarily uploaded during the application process. + :param request: Current request object + :param file_type: Type of file to be downloaded + :return: + """ + registration_data = request.session["registration_data"] + storage = select_storage() + file_name = registration_data[f"{file_type}_file_uploaded_filename"] + if storage.exists(file_name): + return FileResponse(storage.open(file_name, "rb")) + return HttpResponseNotFound("Not Found") + + class UploadView(FormView): page_type = "" template_name = "" diff --git a/request_a_govuk_domain/urls.py b/request_a_govuk_domain/urls.py index d8e0aef2..535c388f 100644 --- a/request_a_govuk_domain/urls.py +++ b/request_a_govuk_domain/urls.py @@ -54,6 +54,7 @@ security_txt_view, robots_txt_view, forbidden_view, + download_file, AccessibilityView, TermsAndConditionsView, ) @@ -178,6 +179,7 @@ ), path(".well-known/security.txt", security_txt_view), path("robots.txt", robots_txt_view), + path("download_file/", download_file, name="download_file"), ] + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT) From b8e1e9c81c09fd84b8594d8c6e87f547e2f7d742 Mon Sep 17 00:00:00 2001 From: Dharshana Vanderbona Date: Tue, 18 Jun 2024 10:40:06 +0100 Subject: [PATCH 2/2] Changes to get the sample data population working. Sample data population had to be modified in order to simulate the availability of files in the /temp_files folder on AWS. Also the init task had to be in the same network as S3 to access the mock bucket. --- docker-compose.yml | 1 + .../request/admin/model_admins.py | 4 ++-- .../management/commands/create_sample_data.py | 24 ++++++++++++++++++- .../request/models/application.py | 14 +++++++---- .../request/models/storage_util.py | 10 ++++++-- 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index f9ac5d70..d674b5f6 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -100,6 +100,7 @@ services: condition: service_healthy networks: - clam-net + - s3-net volumes: diff --git a/request_a_govuk_domain/request/admin/model_admins.py b/request_a_govuk_domain/request/admin/model_admins.py index 61d3f30b..dd333c24 100644 --- a/request_a_govuk_domain/request/admin/model_admins.py +++ b/request_a_govuk_domain/request/admin/model_admins.py @@ -34,7 +34,7 @@ def download_file_view(self, request, object_id, field_name): """ Implement this method to provide the specific retrieval of the object based on the given id :param request: Current request object - :param object_id: id of the object containing the filed + :param object_id: id of the object containing the field :param field_name: which file field to download from the object :return: """ @@ -130,7 +130,7 @@ def download_file_view(self, request, object_id, field_name): application = review.application file = getattr(application, field_name) # We need to use root_storage as the default storage always - # refer to the temp_files as the parent. + # refer to the TEMP_STORAGE_ROOT as the parent. return FileResponse(s3_root_storage().open(file.name, "rb")) @property diff --git a/request_a_govuk_domain/request/management/commands/create_sample_data.py b/request_a_govuk_domain/request/management/commands/create_sample_data.py index 29371c38..5263ff0e 100644 --- a/request_a_govuk_domain/request/management/commands/create_sample_data.py +++ b/request_a_govuk_domain/request/management/commands/create_sample_data.py @@ -1,11 +1,15 @@ +import logging import os import shutil from datetime import datetime +from pathlib import Path from django.core.management.base import BaseCommand from django.db import IntegrityError -from request_a_govuk_domain.request import models +from request_a_govuk_domain.request import models +from request_a_govuk_domain.request.models.storage_util import select_storage +from request_a_govuk_domain.settings import S3_STORAGE_ENABLED SCRIPT_PATH = os.path.dirname(os.path.abspath(__file__)) SEED_DOCS_PATH = os.path.join(SCRIPT_PATH, "..", "..", "..", "..", "seed", "documents") @@ -38,6 +42,7 @@ POLICY_TEAM_EXEMPTION_FN = "policy_team_exception.png" DUMMY_REGISTRARS = ["WeRegister", "WeAlsoRegister", "WeLikeToRegister"] +logger = logging.getLogger(__name__) def create_sample_application( @@ -50,6 +55,23 @@ def create_sample_application( ministerial_request_file: str | None = None, policy_exemption_file: str | None = None, ): + # Copy the sample data to the temporary storage so the system will assume it is comping from the temporary + # location. This is needed as we have overridden the save method of the application to fetch the data + # from the TEMP_STORAGE_ROOT root location if we are using S3 + if S3_STORAGE_ENABLED: + for f in [ + written_permission_file, + ministerial_request_file, + policy_exemption_file, + ]: + logger.info("Copying seed file %s", f) + if f: + with open( + Path(__file__).parent.joinpath(f"../../../media/{f}").resolve(), + "rb", + ) as f_content: + select_storage().save(f, f_content) + registrant = models.Registrant.objects.create(name=registrant_name) registrant_person = models.RegistrantPerson.objects.create(name=person_names[0]) diff --git a/request_a_govuk_domain/request/models/application.py b/request_a_govuk_domain/request/models/application.py index 02c84b9e..b70c200a 100644 --- a/request_a_govuk_domain/request/models/application.py +++ b/request_a_govuk_domain/request/models/application.py @@ -1,13 +1,16 @@ +import logging + from django.contrib.auth.models import User from django.db import models from django.utils.translation import gettext_lazy as _ from .organisation import Registrant, Registrar from .person import RegistryPublishedPerson, RegistrarPerson, RegistrantPerson -from .storage_util import select_storage -from ...settings import IS_AWS +from .storage_util import select_storage, TEMP_STORAGE_ROOT +from ...settings import S3_STORAGE_ENABLED REF_NUM_LENGTH = 17 +logger = logging.getLogger(__name__) class ApplicationStatus(models.TextChoices): @@ -96,7 +99,7 @@ def save( :param update_fields: :return: """ - if IS_AWS: + if S3_STORAGE_ENABLED: storage = select_storage() # Move any temporary files in to the application specific folder for file_field in [ @@ -105,7 +108,10 @@ def save( self.written_permission_evidence, ]: if file_field and not file_field.name.startswith("applications"): - from_path = "temp_files/" + file_field.name + from_path = TEMP_STORAGE_ROOT + file_field.name + logger.info( + "Copying temporary file to application folder %s", from_path + ) to_path = f"applications/{self.reference}/" + file_field.name storage.connection.meta.client.copy_object( Bucket=storage.bucket_name, diff --git a/request_a_govuk_domain/request/models/storage_util.py b/request_a_govuk_domain/request/models/storage_util.py index b76dacc7..14263f4a 100644 --- a/request_a_govuk_domain/request/models/storage_util.py +++ b/request_a_govuk_domain/request/models/storage_util.py @@ -2,15 +2,21 @@ from django.core.files.storage import FileSystemStorage from storages.backends.s3boto3 import S3Boto3Storage +TEMP_STORAGE_ROOT = "temp_files/" + def select_storage(): """ Utility method to select the storage type based on where the application is run. - When deploying to AWS, We need to set the IS_AWS to true by setting the environment variable + When deploying to AWS, We need to set the S3_STORAGE_ENABLED to true by setting the environment variable with the same name. :return: """ - return S3Boto3Storage() if settings.S3_STORAGE_ENABLED else FileSystemStorage() + return ( + S3Boto3Storage(location=TEMP_STORAGE_ROOT) + if settings.S3_STORAGE_ENABLED + else FileSystemStorage() + ) def s3_root_storage():