Update grant/revoke database access (#1325)

* Update grant/revoke database access

Check if the database/table is a resource link before updating access.
This means that we always grant/revoke against the source, not the
resource link.

* Update database url path converter to use str

* Catch exceptions when granting/revoking and remove commented out code

* Add mixin to retreive table details

* Use reverse in views

* Refactor mixin to get table access

* Remove unused settings

* Refactor get_table_data to simplify

* Update glue table test fixture

* Safer lookup of target table region

From testing it seems that the region is not always included, for
example in the case where the resource link and shared table/db are in
the same region.

controlpanel/frontend/jinja2/table-detail.html

@@ -16,12 +16,12 @@ <h1 class="govuk-heading-xl">{{ tablename }}</h1>
     </thead>
     <tbody class="govuk-table__body">
       <tr class="govuk-table__row">
-        <td class="govuk-table__cell">{{ "Yes" if table.IsRegisteredWithLakeFormation else "No" }}</td>
+        <td class="govuk-table__cell">{{ "Yes" if table.is_registered_with_lake_formation else "No" }}</td>
       </tr>
     </tbody>
   </table>
 
-  {% if table.IsRegisteredWithLakeFormation %}
+  {% if table.is_registered_with_lake_formation %}
     <h2 class="govuk-heading-l">User Access</h1>
 
     {% if permissions %}
@@ -39,7 +39,7 @@ <h2 class="govuk-heading-l">User Access</h1>
         <tr class="govuk-table__row">
           <td class="govuk-table__cell">{{ permission.user }}</td>
           <td class="govuk-table__cell align-right no-wrap">
-            <form action="{{ url("revoke-table-permissions", kwargs={ "dbname": dbname, "tablename": table.Name, "user": permission.user }) }}" method="post">
+            <form action="{{ url("revoke-table-permissions", kwargs={ "dbname": dbname, "tablename": tablename, "user": permission.user }) }}" method="post">
               {{ csrf_input }}
               <button class="govuk-button cpanel-button--destructive js-confirm"
                       data-confirm-message="Are you sure you want to revoke access for user?">
@@ -55,7 +55,7 @@ <h2 class="govuk-heading-l">User Access</h1>
     {% endif %}
 
 
-    <a class="govuk-button" href="{{ url('grant-table-permissions', kwargs={ "dbname": dbname, "tablename": table.Name}) }}">
+    <a class="govuk-button" href="{{ url('grant-table-permissions', kwargs={ "dbname": dbname, "tablename": tablename}) }}">
       Grant User Permissions
     </a>
   {% endif %}

controlpanel/frontend/urls.py

@@ -221,19 +221,19 @@
     path("accessibility/", views.Accessibility.as_view(), name="accessibility"),
     path("tasks/", views.TaskList.as_view(), name="list-tasks"),
     path("databases/", views.DatabasesListView.as_view(), name="list-databases"),
-    path("databases/<slug:dbname>/", views.TablesListView.as_view(), name="list-tables"),
+    path("databases/<str:dbname>/", views.TablesListView.as_view(), name="list-tables"),
     path(
-        "databases/<slug:dbname>/<slug:tablename>/grant/",
+        "databases/<str:dbname>/<slug:tablename>/grant/",
         views.TableGrantView.as_view(),
         name="grant-table-permissions",
     ),
     path(
-        "databases/<slug:dbname>/<slug:tablename>/<str:user>/revoke/",
+        "databases/<str:dbname>/<str:tablename>/<str:user>/revoke/",
         views.RevokeTableAccessView.as_view(),
         name="revoke-table-permissions",
     ),
     path(
-        "databases/<slug:dbname>/<slug:tablename>/",
+        "databases/<str:dbname>/<str:tablename>/",
         views.ManageTable.as_view(),
         name="manage-table",
     ),

controlpanel/frontend/views/databases.py

@@ -1,10 +1,13 @@
 # Third-party
+import botocore
+import sentry_sdk
 import structlog
 from django.conf import settings
 from django.contrib import messages
 from django.http.response import HttpResponseRedirect
-from django.urls import reverse, reverse_lazy
+from django.urls import reverse
 from django.views.generic import TemplateView, View
+from django.views.generic.base import ContextMixin
 from django.views.generic.edit import FormView
 from rules.contrib.views import PermissionRequiredMixin
 
@@ -27,9 +30,7 @@ class DatabasesListView(
 
     def get_context_data(self, **kwargs):
         context_data = super().get_context_data(**kwargs)
-
         context_data["databases"] = self._get_database_list(settings.DPR_DATABASE_NAME)
-
         return context_data
 
     def _get_database_list(self, database_name=None):
@@ -65,36 +66,84 @@ def get_context_data(self, **kwargs):
         return context_data
 
 
+class GetTableDataMixin(ContextMixin):
+
+    @property
+    def base_table_data(self):
+        return {
+            "database_name": self.kwargs["dbname"],
+            "table_name": self.kwargs["tablename"],
+            "region": settings.AWS_DEFAULT_REGION,
+            "catalog_id": settings.AWS_DATA_ACCOUNT_ID,
+        }
+
+    def get_table_data(self):
+        table_data = self.base_table_data.copy()
+        # check if the db is a resource link to ensurewe use the correct details when getting table
+        glue = AWSGlue()
+        database = glue.get_database(database_name=self.kwargs["dbname"])["Database"]
+        if "TargetDatabase" in database:
+            table_data.update(
+                {
+                    "database_name": database["TargetDatabase"]["DatabaseName"],
+                    "region": database["TargetDatabase"].get("Region"),
+                    "catalog_id": database["TargetDatabase"]["CatalogId"],
+                }
+            )
+            glue = AWSGlue(region_name=database["TargetDatabase"].get("Region"))
+
+        table = glue.get_table(
+            database_name=table_data["database_name"],
+            table_name=self.kwargs["tablename"],
+            catalog_id=table_data["catalog_id"],
+        )["Table"]
+        table_data["is_registered_with_lake_formation"] = table.get("IsRegisteredWithLakeFormation")
+        # check if the table a resource link and update table data with the correct details
+        if "TargetTable" in table:
+            table_data.update(
+                {
+                    "database_name": table["TargetTable"]["DatabaseName"],
+                    "table_name": table["TargetTable"]["Name"],
+                    "region": table["TargetTable"].get("Region"),
+                    "catalog_id": table["TargetTable"]["CatalogId"],
+                }
+            )
+        return table_data
+
+
 class ManageTable(
     OIDCLoginRequiredMixin,
     PermissionRequiredMixin,
+    GetTableDataMixin,
     TemplateView,
 ):
     template_name = "table-detail.html"
     permission_required = "api.is_database_admin"
 
     def get_context_data(self, **kwargs):
         context_data = super().get_context_data(**kwargs)
-        database_name = kwargs["dbname"]
-        table_name = kwargs["tablename"]
-
-        glue = AWSGlue()
-
-        tables_data = glue.get_table(database_name=database_name, table_name=table_name)
-        context_data["table"] = tables_data["Table"]
+        table_data = self.get_table_data()
+        context_data["table"] = table_data
 
-        if context_data["table"]["IsRegisteredWithLakeFormation"]:
-            permissions = self._get_permissions(database_name=database_name, table_name=table_name)
-            context_data["permissions"] = permissions
+        if not table_data["is_registered_with_lake_formation"]:
+            return context_data
 
+        context_data["permissions"] = self._get_permissions(
+            database_name=table_data["database_name"],
+            table_name=table_data["table_name"],
+            region_name=table_data["region"],
+            catalog_id=table_data["catalog_id"],
+        )
         return context_data
 
-    def _get_permissions(self, database_name, table_name):
-        lake_formation = AWSLakeFormation()
+    def _get_permissions(self, database_name, table_name, region_name=None, catalog_id=None):
+
+        region_name = region_name
+        lake_formation = AWSLakeFormation(region_name=region_name)
         result = []
 
         permissions = lake_formation.list_permissions(
-            database_name=database_name, table_name=table_name
+            database_name=database_name, table_name=table_name, catalog_id=catalog_id
         )
 
         for permission in permissions["PrincipalResourcePermissions"]:
@@ -117,6 +166,7 @@ def get_user_from_arn(self, arn, separator):
 class TableGrantView(
     OIDCLoginRequiredMixin,
     PermissionRequiredMixin,
+    GetTableDataMixin,
     FormView,
 ):
     template_name = "table-access-grant.html"
@@ -146,104 +196,61 @@ def form_valid(self, form):
         user = form.cleaned_data["user"]
         permissions = form.cleaned_data["access_level"]
         user_arn = iam_arn(f"role/{user.iam_role_name}")
-        database_name = self.kwargs["dbname"]
-        resource_link_name = self.kwargs["tablename"]
 
-        try:
-            lake_formation = AWSLakeFormation()
-            glue = AWSGlue()
-            table_data = glue.get_table(database_name=database_name, table_name=resource_link_name)
-            remote_catalog_id = table_data["Table"]["TargetTable"]["CatalogId"]
-            remote_database_name = table_data["Table"]["TargetTable"]["DatabaseName"]
-            remote_table_name = table_data["Table"]["TargetTable"]["Name"]
-
-            lake_formation.grant_table_permission(
-                database_name=database_name,
-                table_name=resource_link_name,
-                principal_arn=user_arn,
-                permissions=permissions["resource_link"],
-            )
+        table_data = self.get_table_data()
 
+        # only grants access on the shared table, not the resource link
+        try:
+            lake_formation = AWSLakeFormation(region_name=table_data["region"])
             lake_formation.grant_table_permission(
-                database_name=remote_database_name,
-                table_name=remote_table_name,
-                principal_arn=user_arn,
-                catalog_id=remote_catalog_id,
+                database_name=table_data["database_name"],
+                table_name=table_data["table_name"],
+                catalog_id=table_data["catalog_id"],
                 permissions=permissions["table"],
+                principal_arn=user_arn,
             )
-
-            messages.success(self.request, f"Successfully granted access for user {user.username}")
-        except Exception as e:
-            log.error(f"Could not grant access for user {user.username}", error=str(e))
+        except botocore.exceptions.ClientError as e:
             messages.error(self.request, f"Could not grant access for user {user.username}")
+            sentry_sdk.capture_exception(e)
+            return super().form_invalid(form)
 
+        messages.success(self.request, f"Successfully granted access for user {user.username}")
         return super().form_valid(form)
 
 
 class RevokeTableAccessView(
     OIDCLoginRequiredMixin,
     PermissionRequiredMixin,
+    GetTableDataMixin,
     View,
 ):
     permission_required = "api.is_database_admin"
 
     def post(self, request, *args, **kwargs):
+        table_data = self.get_table_data()
+        lake_formation = AWSLakeFormation(region_name=table_data["region"])
+        principal_arn = iam_arn(f'role/{settings.ENV}_user_{kwargs["user"]}')
 
         try:
-            lake_formation = AWSLakeFormation()
-            glue = AWSGlue()
-            database_name = kwargs["dbname"]
-            table_name = kwargs["tablename"]
-            principal_arn = iam_arn(f'role/{settings.ENV}_user_{kwargs["user"]}')
-
-            table_data = glue.get_table(database_name=database_name, table_name=table_name)
-            remote_catalog_id = table_data["Table"]["TargetTable"]["CatalogId"]
-            remote_database_name = table_data["Table"]["TargetTable"]["DatabaseName"]
-            remote_table_name = table_data["Table"]["TargetTable"]["Name"]
-
-            resource_link_permissions = self._get_resource_permissions(
-                lake_formation, database_name, table_name, principal_arn
-            )
-
-            table_permissions = self._get_resource_permissions(
-                lake_formation,
-                remote_database_name,
-                remote_table_name,
-                principal_arn,
-                remote_catalog_id,
+            # only revokes access on the shared table, not the resource link
+            lake_formation.revoke_table_permission(
+                database_name=table_data["database_name"],
+                table_name=table_data["table_name"],
+                principal_arn=principal_arn,
+                catalog_id=table_data["catalog_id"],
+                permissions=["SELECT"],
             )
-
-            # Revoke access to resource link and linked table here
-            if resource_link_permissions:
-                lake_formation.revoke_table_permission(
-                    database_name=database_name,
-                    table_name=table_name,
-                    principal_arn=principal_arn,
-                    permissions=resource_link_permissions,
-                )
-            if table_permissions:
-                lake_formation.revoke_table_permission(
-                    database_name=remote_database_name,
-                    table_name=remote_table_name,
-                    principal_arn=principal_arn,
-                    catalog_id=remote_catalog_id,
-                    permissions=table_permissions,
-                )
-
             messages.success(self.request, f"Successfully revoked access for user {kwargs['user']}")
-            return HttpResponseRedirect(
-                reverse_lazy(
-                    "manage-table", kwargs={"dbname": database_name, "tablename": table_name}
-                )
-            )
-        except Exception as e:
-            log.error(f"Could not revoke access for user {kwargs['user']}", error=str(e))
+        except botocore.exceptions.ClientError as e:
+            sentry_sdk.capture_exception(e)
             messages.error(self.request, f"Could not revoke access for user {kwargs['user']}")
-            return HttpResponseRedirect(
-                reverse_lazy(
-                    "manage-table", kwargs={"dbname": database_name, "tablename": table_name}
-                )
+
+        return HttpResponseRedirect(
+            reverse(
+                "manage-table",
+                kwargs={"dbname": self.kwargs["dbname"], "tablename": self.kwargs["tablename"]},
             )
+        )
 
     def _get_resource_permissions(
         self, lake_formation, database_name, table_name, principal_arn, catalog_id=None

controlpanel/settings/common.py

@@ -586,6 +586,6 @@
     }
 
 CELERY_IMPORTS = ["controlpanel.api.tasks.handlers"]
-DPR_DATABASE_NAME = os.environ.get("DPR_DATABASE_NAME")
+DPR_DATABASE_NAME = os.environ.get("DPR_DATABASE_NAME", None)
 
 S3_ARCHIVE_BUCKET_NAME = "dev-archive-folder"

requirements.txt

@@ -23,7 +23,7 @@ Jinja2==3.1.4
 kubernetes==25.3.0
 MarkupSafe==2.1.5
 model-bakery==1.17.0
-moto[all]==5.0.9
+moto[all]==5.0.11
 mozilla-django-oidc==4.0.1
 psycopg2-binary==2.9.9
 PyNaCl==1.5.0

tests/api/fixtures/aws.py

@@ -54,6 +54,7 @@ def glue(aws_creds):
                     "CatalogId": "123",
                     "DatabaseName": "external_db",
                     "Name": "external_test_table",
+                    "Region": "eu-west-2",
                 },
             },
         )

tests/frontend/views/test_databases.py

@@ -1,7 +1,3 @@
-# Standard library
-import json
-from unittest.mock import patch
-
 # Third-party
 import pytest
 from django.conf import settings