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

@@ -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")

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,

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,

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": "",
             },
             {

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

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)

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),
+    ]

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

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:

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"
+                )

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")},
         ]