From 20d0b4c883b9556e10bbf7d0a1c9c2a59fa87b3c Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Mon, 23 Sep 2024 15:28:12 +0100 Subject: [PATCH] Bugfix/revoke permissions (#289) * Wrap tableaccess delete in a transaction Changes are to ensure that if deleting the table access fails, the users original permissions are restored, and the database object remains. Otherwise the state of the DB and LF could get out of sync. * Default to local settings when using manage.py * Allow revoking to silently fail if the permission does not exist This change allows the revoke_permission method to silently fail if the permission does not exist. This is useful when revoking permissions that may have already been revoked, and avoids getting in a state where the user cannot be granted permissions again. * Update user usernames When creating users, use the UPN, preferred username or email address. This will only occur on initial login. For existing users, a data migration has been addeded to update the usernames to their email address, with the domain lowercase. This resolves a bug where permissions were not granted to the correct quicksight user ARN, as it was using the email value and the users quicksight username used the User Principal Name (UPN). * Bump chart version --- ap/auth/oidc.py | 2 +- ap/aws/lakeformation.py | 41 ++++++--- ap/aws/quicksight.py | 2 +- ap/core/context_processors.py | 2 +- ap/database_access/models/access.py | 26 +++--- ap/database_access/views.py | 39 ++++---- ap/users/migrations/0002_update_usernames.py | 22 +++++ chart/Chart.yaml | 4 +- manage.py | 2 +- tests/aws/test_lakeformation.py | 93 ++++++++++++++++++++ tests/core/test_context_processors.py | 2 +- 11 files changed, 192 insertions(+), 43 deletions(-) create mode 100644 ap/users/migrations/0002_update_usernames.py create mode 100644 tests/aws/test_lakeformation.py 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")}, ]