diff --git a/ap/auth/oidc.py b/ap/auth/oidc.py index e8249c26..2cd46ab0 100644 --- a/ap/auth/oidc.py +++ b/ap/auth/oidc.py @@ -31,7 +31,7 @@ def filter_users_by_claims(self): return User.objects.filter(pk=user_id).first() def _get_username(self, user_info): - return user_info.get("username") or User.construct_username(user_info.get("name")) + return user_info.get("upn") or user_info.get("preferred_username") or user_info.get("email") def _create_user(self): user_info = self.token.get("userinfo") diff --git a/ap/aws/lakeformation.py b/ap/aws/lakeformation.py index 57282025..7e6de2de 100644 --- a/ap/aws/lakeformation.py +++ b/ap/aws/lakeformation.py @@ -1,3 +1,4 @@ +import botocore import sentry_sdk import structlog @@ -100,7 +101,14 @@ def revoke_table_permissions( Permissions=permissions or [], PermissionsWithGrantOption=grantable_permissions or [], ) - except client.exceptions.InvalidInputException as error: + except botocore.exceptions.ClientError as error: + msg = error.response["Error"]["Message"] + code = error.response["Error"]["Code"] + if ( + code == "InvalidInputException" + and "Grantee has no permissions and no grantable permissions on resource" in msg + ): + return logger.info(f"Nothing to revoke, continuing. Original exception: '{msg}'") sentry_sdk.capture_exception(error) logger.info(f"Error revoking permissions for {principal}", error=error) raise error @@ -118,17 +126,28 @@ def revoke_database_permissions( Grant the principal permissions to the database. """ client = self.get_client(region_name) - return client.revoke_permissions( - Principal={"DataLakePrincipalIdentifier": principal}, - Resource={ - "Database": { - "Name": database, - "CatalogId": resource_catalog_id or self.catalog_id, + try: + client.revoke_permissions( + Principal={"DataLakePrincipalIdentifier": principal}, + Resource={ + "Database": { + "Name": database, + "CatalogId": resource_catalog_id or self.catalog_id, + }, }, - }, - Permissions=permissions or ["DESCRIBE"], - CatalogId=catalog_id or self.catalog_id, - ) + Permissions=permissions or ["DESCRIBE"], + CatalogId=catalog_id or self.catalog_id, + ) + except botocore.exceptions.ClientError as error: + msg = error.response["Error"]["Message"] + code = error.response["Error"]["Code"] + if ( + code == "InvalidInputException" + and "Grantee has no permissions and no grantable permissions on resource" in msg + ): + return logger.info(f"Nothing to revoke, continuing. Original exception: '{msg}'") + sentry_sdk.capture_exception(error) + raise error def create_lake_formation_opt_in( self, diff --git a/ap/aws/quicksight.py b/ap/aws/quicksight.py index e87f6987..9beadda1 100644 --- a/ap/aws/quicksight.py +++ b/ap/aws/quicksight.py @@ -7,7 +7,7 @@ class QuicksightService(base.AWSService): aws_service_name = "quicksight" def get_embed_url(self, user): - user_arn = self.arn(resource=f"user/default/{user.email}") + user_arn = self.arn(resource=f"user/default/{user.username}") response = self._request( "generate_embed_url_for_registered_user", AwsAccountId=self.account_id, diff --git a/ap/core/context_processors.py b/ap/core/context_processors.py index fb06d73f..a64f1cc2 100644 --- a/ap/core/context_processors.py +++ b/ap/core/context_processors.py @@ -32,7 +32,7 @@ def header_context(request): return { "header_nav_items": [ { - "name": request.user.email if is_logged_in else "", + "name": request.user.username if is_logged_in else "", "url": "", }, { diff --git a/ap/database_access/models/access.py b/ap/database_access/models/access.py index db8fdb4b..f4cd3216 100644 --- a/ap/database_access/models/access.py +++ b/ap/database_access/models/access.py @@ -1,9 +1,10 @@ from functools import cached_property from django.conf import settings -from django.db import models +from django.db import models, transaction from django.urls import reverse +import botocore from django_extensions.db.models import TimeStampedModel from ap import aws @@ -69,7 +70,7 @@ def get_absolute_url(self): def grant_lakeformation_permissions(self, create_hybrid_opt_in=False): lake_formation = aws.LakeFormationService() quicksight_user = lake_formation.arn( - resource=f"user/default/{self.user.email}", + resource=f"user/default/{self.user.username}", service="quicksight", ) lake_formation.grant_database_permissions( @@ -92,7 +93,7 @@ def delete(self, *args, **kwargs): # revoke access lake_formation = aws.LakeFormationService() quicksight_user = lake_formation.arn( - resource=f"user/default/{self.user.email}", + resource=f"user/default/{self.user.username}", service="quicksight", region_name=settings.AWS_DEFAULT_REGION, ) @@ -167,7 +168,7 @@ def get_absolute_table_detail_url(self): def grant_lakeformation_permissions(self, create_hybrid_opt_in=False): lake_formation = aws.LakeFormationService() quicksight_user = lake_formation.arn( - resource=f"user/default/{self.database_access.user.email}", + resource=f"user/default/{self.database_access.user.username}", service="quicksight", region_name=settings.AWS_DEFAULT_REGION, ) @@ -200,7 +201,7 @@ def grant_lakeformation_permissions(self, create_hybrid_opt_in=False): def revoke_lakeformation_permissions(self, revoke_hybrid_opt_in=False): lake_formation = aws.LakeFormationService() quicksight_user = lake_formation.arn( - resource=f"user/default/{self.database_access.user.email}", + resource=f"user/default/{self.database_access.user.username}", service="quicksight", ) @@ -228,8 +229,13 @@ def revoke_lakeformation_permissions(self, revoke_hybrid_opt_in=False): ) def delete(self, **kwargs): - self.revoke_lakeformation_permissions(revoke_hybrid_opt_in=True) - super().delete(**kwargs) - if not self.database_access.table_access.exists(): - # if this was the last table access for the database, revoke database access - self.database_access.delete() + try: + with transaction.atomic(): + self.revoke_lakeformation_permissions(revoke_hybrid_opt_in=True) + if not self.database_access.table_access.exclude(pk=self.pk).exists(): + # if this is the last table access for the database, revoke database access + self.database_access.delete() + super().delete(**kwargs) + except botocore.exceptions.ClientError as error: + self.grant_lakeformation_permissions(create_hybrid_opt_in=True) + raise error diff --git a/ap/database_access/views.py b/ap/database_access/views.py index 91846317..d6cecdab 100644 --- a/ap/database_access/views.py +++ b/ap/database_access/views.py @@ -42,7 +42,6 @@ def get_context_data(self, **kwargs: Any) -> dict[str, Any]: class BreadcrumbsMixin(ContextMixin): - def get_breadcrumbs(self): raise NotImplementedError() @@ -162,23 +161,13 @@ def get_grantable_access(self): return table_access.grantable_permissions.all() - def get_success_url(self) -> str: - return reverse( - "database_access:table_detail", - kwargs={ - "database_name": self.kwargs["database_name"], - "table_name": self.kwargs["table_name"], - }, - ) - def form_valid(self, form: BaseModelForm) -> HttpResponse: try: return super().form_valid(form) except botocore.exceptions.ClientError as error: - if error.response["Error"]["Code"] == "InvalidInputException": - sentry_sdk.capture_exception(error) - form.add_error(None, "An error occured granting permissions") - return self.form_invalid(form) + sentry_sdk.capture_exception(error) + form.add_error(None, "An error occured granting permissions") + return self.form_invalid(form) class GrantTableAccessView(OIDCLoginRequiredMixin, TableAccessMixin, BreadcrumbsMixin, CreateView): @@ -281,7 +270,7 @@ def get_form_kwargs(self): return form_kwargs -class RevokeTableAccessView(OIDCLoginRequiredMixin, TableAccessMixin, BreadcrumbsMixin, DeleteView): +class RevokeTableAccessView(OIDCLoginRequiredMixin, BreadcrumbsMixin, DeleteView): template_name = "database_access/database/revoke_access.html" model = models.TableAccess context_object_name = "access" @@ -309,3 +298,23 @@ def get_breadcrumbs(self): def get_queryset(self) -> QuerySet[Any]: return super().get_queryset().select_related("database_access__user") + + def get_success_url(self) -> str: + return reverse( + "database_access:table_detail", + kwargs={ + "database_name": self.kwargs["database_name"], + "table_name": self.kwargs["table_name"], + }, + ) + + def form_valid(self, form: BaseModelForm) -> HttpResponse: + try: + return super().form_valid(form) + except botocore.exceptions.ClientError as error: + sentry_sdk.capture_exception(error) + form.add_error( + None, + "An error occured revoking permissions. Please try again. If the issue persists, contact the AP support.", # noqa + ) + return self.form_invalid(form) diff --git a/ap/users/migrations/0002_update_usernames.py b/ap/users/migrations/0002_update_usernames.py new file mode 100644 index 00000000..8c4bca99 --- /dev/null +++ b/ap/users/migrations/0002_update_usernames.py @@ -0,0 +1,22 @@ +# Generated by Django 5.1 on 2024-09-20 16:23 + +from django.db import migrations + + +def update_usernames(apps, schema_editor): + User = apps.get_model("users", "User") + for user in User.objects.all(): + email, domain = user.email.split("@") + domain = domain.lower() + user.username = f"{email}@{domain}" + user.save() + + +class Migration(migrations.Migration): + dependencies = [ + ("users", "0001_initial"), + ] + + operations = [ + migrations.RunPython(code=update_usernames, reverse_code=migrations.RunPython.noop), + ] diff --git a/chart/Chart.yaml b/chart/Chart.yaml index fae299d6..259baced 100644 --- a/chart/Chart.yaml +++ b/chart/Chart.yaml @@ -3,8 +3,8 @@ apiVersion: v2 name: analytical-platform-ui description: Analytical Platform UI type: application -version: 0.2.4 -appVersion: 0.2.4 +version: 0.2.5 +appVersion: 0.2.5 icon: https://upload.wikimedia.org/wikipedia/en/thumb/4/4a/Ministry_of_Justice_logo_%28United_Kingdom%29.svg/611px-Ministry_of_Justice_logo_%28United_Kingdom%29.svg.png maintainers: - name: moj-data-platform-robot diff --git a/manage.py b/manage.py index 856ba06c..5d6a46fa 100755 --- a/manage.py +++ b/manage.py @@ -10,7 +10,7 @@ def main(): """Run administrative tasks.""" dotenv.load_dotenv() - os.environ.setdefault("DJANGO_SETTINGS_MODULE", "ap.settings") + os.environ.setdefault("DJANGO_SETTINGS_MODULE", "ap.settings.local") try: from django.core.management import execute_from_command_line except ImportError as exc: diff --git a/tests/aws/test_lakeformation.py b/tests/aws/test_lakeformation.py new file mode 100644 index 00000000..6d9c17ab --- /dev/null +++ b/tests/aws/test_lakeformation.py @@ -0,0 +1,93 @@ +from unittest.mock import MagicMock, patch + +import botocore +import botocore.exceptions +import pytest + +from ap.aws.lakeformation import LakeFormationService + + +class TestRevoke: + def test_revoke_database_no_error(self): + """ + Test that if the user does not have access to revoke, no error is raised + """ + lf = LakeFormationService() + with patch.object(lf, "get_client") as mock_get_client: + mock_client = MagicMock() + mock_client.revoke_permissions.side_effect = botocore.exceptions.ClientError( + { + "Error": { + "Code": "InvalidInputException", + "Message": "Grantee has no permissions and no grantable permissions on resource", # noqa + } + }, + "revoke_permissions", + ) + mock_get_client.return_value = mock_client + assert ( + lf.revoke_database_permissions(database="db_without_access", principal="user") + is None + ) + + def test_revoke_database_raises_error(self): + lf = LakeFormationService() + with patch.object(lf, "get_client") as mock_get_client: + mock_client = MagicMock() + mock_client.revoke_permissions.side_effect = botocore.exceptions.ClientError( + { + "Error": { + "Code": "SomeOtherError", + "Message": "Some other error message", + } + }, + "revoke_permissions", + ) + mock_get_client.return_value = mock_client + # revoking should raises exception + with pytest.raises(botocore.exceptions.ClientError): + lf.revoke_database_permissions(database="db_without_access", principal="user") + + def test_revoke_table_no_error(self): + """ + Test that if the user does not have access to revoke, no error is raised + """ + lf = LakeFormationService() + with patch.object(lf, "get_client") as mock_get_client: + mock_client = MagicMock() + mock_client.revoke_permissions.side_effect = botocore.exceptions.ClientError( + { + "Error": { + "Code": "InvalidInputException", + "Message": "Grantee has no permissions and no grantable permissions on resource", # noqa + } + }, + "revoke_permissions", + ) + mock_get_client.return_value = mock_client + assert ( + lf.revoke_table_permissions( + database="db_without_access", table="table_without_access", principal="user" + ) + is None + ) + + def test_revoke_table_raises_error(self): + lf = LakeFormationService() + with patch.object(lf, "get_client") as mock_get_client: + mock_client = MagicMock() + mock_client.revoke_permissions.side_effect = botocore.exceptions.ClientError( + { + "Error": { + "Code": "SomeOtherError", + "Message": "Some other error message", + } + }, + "revoke_permissions", + ) + mock_get_client.return_value = mock_client + # revoking should raises exception + with pytest.raises(botocore.exceptions.ClientError): + lf.revoke_table_permissions( + database="db_without_access", table="table_without_access", principal="user" + ) diff --git a/tests/core/test_context_processors.py b/tests/core/test_context_processors.py index 17e29b3f..1985017b 100644 --- a/tests/core/test_context_processors.py +++ b/tests/core/test_context_processors.py @@ -23,7 +23,7 @@ def test_nav_items(self, request_obj): def test_header_context(self, request_obj): expected_nav_items = [ - {"name": request_obj.user.email, "url": ""}, + {"name": request_obj.user.username, "url": ""}, {"name": "Sign out", "url": reverse("logout")}, ]