Merged main. Fixed more tests

controlpanel/api/models/user.py

@@ -130,6 +130,17 @@ def show_webapp_data_link(self):
 
         return self.userapps.filter(is_admin=True).exists()
 
+    @property
+    def is_github_user(self):
+        """
+        Determine if the user was created via the github connection. We can use this to limit what
+        actions non-github users do within the AP.
+        This has been added specifically to allow CICA users to authenticate with EntraID in order
+        to access QuickSight in the AP. This is a temporary change, that should be removed once auth
+        with Entra is fully implemented.
+        """
+        return self.auth0_id.startswith("github|")
+
     def is_app_admin(self, app_id):
         return (
             self.userapps.filter(
@@ -165,7 +176,7 @@ def set_quicksight_access(self, enable):
 
     def save(self, *args, **kwargs):
         existing = User.objects.filter(pk=self.pk).first()
-        if not existing:
+        if not existing and self.is_github_user:
             cluster.User(self).create()
 
         already_superuser = existing and existing.is_superuser

controlpanel/api/rules.py

@@ -47,8 +47,13 @@ def is_app_admin(user, obj):
     return False
 
 
-add_perm("api.list_app", is_authenticated)
-add_perm("api.create_app", is_authenticated)
+@predicate
+def is_github_user(user):
+    return user.is_github_user
+
+
+add_perm("api.list_app", is_authenticated & is_github_user)
+add_perm("api.create_app", is_authenticated & is_github_user)
 add_perm("api.retrieve_app", is_authenticated & is_app_admin)
 add_perm("api.update_app", is_authenticated & is_app_admin)
 add_perm("api.destroy_app", is_authenticated & is_superuser)
@@ -126,8 +131,8 @@ def is_database_admin(user):
     return user.is_database_admin
 
 
-add_perm("api.list_s3bucket", is_authenticated)
-add_perm("api.create_s3bucket", is_authenticated)
+add_perm("api.list_s3bucket", is_authenticated & is_github_user)
+add_perm("api.create_s3bucket", is_authenticated & is_github_user)
 add_perm("api.retrieve_s3bucket", is_authenticated & has_bucket_access)
 add_perm("api.update_s3bucket", is_authenticated & has_bucket_write_access)
 add_perm("api.destroy_s3bucket", is_authenticated & is_bucket_admin)
@@ -167,7 +172,7 @@ def is_self(user, other):
 add_perm("api.update_tool_release", is_authenticated & is_superuser)
 
 
-add_perm("api.list_apps3bucket", is_authenticated)
+add_perm("api.list_apps3bucket", is_authenticated & is_github_user)
 add_perm("api.create_apps3bucket", is_authenticated & is_superuser)
 add_perm(
     "api.retrieve_apps3bucket",
@@ -180,14 +185,14 @@ def is_self(user, other):
 add_perm("api.destroy_apps3bucket", is_authenticated & is_superuser)
 
 
-add_perm("api.list_users3bucket", is_authenticated)
+add_perm("api.list_users3bucket", is_authenticated & is_github_user)
 add_perm("api.create_users3bucket", is_authenticated & is_bucket_admin)
 add_perm("api.retrieve_users3bucket", is_authenticated & is_bucket_admin)
 add_perm("api.update_users3bucket", is_authenticated & is_bucket_admin)
 add_perm("api.destroy_users3bucket", is_authenticated & is_bucket_admin)
 
 
-add_perm("api.list_tool", is_authenticated)
+add_perm("api.list_tool", is_authenticated & is_github_user)
 add_perm("api.create_tool", is_authenticated & is_superuser)
 add_perm("api.retrieve_tool", is_authenticated & is_superuser)
 add_perm("api.update_tool", is_authenticated & is_superuser)
@@ -216,15 +221,16 @@ def is_owner(user, obj):
     return False
 
 
+# TODO these are not used and should be removed
 add_perm("api.list_deployment", is_authenticated)
 add_perm("api.create_deployment", is_authenticated)
 add_perm("api.retrieve_deployment", is_authenticated & is_owner)
 add_perm("api.update_deployment", is_authenticated & is_owner)
 add_perm("api.destroy_deployment", is_authenticated & is_owner)
 
 
-add_perm("api.list_parameter", is_authenticated)
-add_perm("api.create_parameter", is_authenticated)
+add_perm("api.list_parameter", is_authenticated & is_github_user)
+add_perm("api.create_parameter", is_authenticated & is_github_user)
 add_perm("api.retrieve_parameter", is_authenticated & is_owner)
 add_perm("api.update_parameter", is_authenticated & is_owner)
 add_perm("api.destroy_parameter", is_authenticated & is_owner)

controlpanel/frontend/jinja2/base.html

@@ -99,11 +99,13 @@
         "href": url("admin:index"),
       },
       {
+        "hide": not request.user.is_github_user,
         "text": "Analytical tools",
         "href": url("list-tools"),
         "active": page_name == "tools",
       },
       {
+        "hide": not request.user.is_github_user,
         "text": "Warehouse data",
         "href": url("list-warehouse-datasources"),
         "active": page_name == "warehouse-datasource-list",
@@ -115,6 +117,7 @@
         "active": page_name == "webapp-datasource-list",
       },
       {
+        "hide": not request.user.is_github_user,
         "text": "Webapps",
         "href": url("list-apps"),
         "active": page_name == "webapps",
@@ -126,6 +129,7 @@
         "active": page_name == "databases",
       },
       {
+        "hide": not request.user.is_github_user,
         "text": "Parameters",
         "href": url("list-parameters"),
         "active": page_name == "parameters",

controlpanel/frontend/views/__init__.py

@@ -139,6 +139,10 @@ def get(self, request, *args, **kwargs):
         if settings.features.justice_auth.enabled and not request.user.justice_email:
             return super().get(request, *args, **kwargs)
 
+        # this is temporary change for users authorising via Entra, specifically CICA users
+        if not request.user.is_github_user:
+            return HttpResponseRedirect(reverse("help"))
+
         # Redirect to the tools page.
         return HttpResponseRedirect(reverse("list-tools"))
 

controlpanel/oidc.py

@@ -28,12 +28,26 @@ class OIDCSubAuthenticationBackend(OIDCAuthenticationBackend):
     """
 
     def create_user(self, claims):
-        return User.objects.create(
-            pk=claims.get("sub"),
-            username=claims.get(settings.OIDC_FIELD_USERNAME),
-            email=claims.get(settings.OIDC_FIELD_EMAIL),
-            name=claims.get(settings.OIDC_FIELD_NAME),
-        )
+        user_details = {
+            "pk": claims.get("sub"),
+            "username": claims.get(settings.OIDC_FIELD_USERNAME),
+            "email": claims.get(settings.OIDC_FIELD_EMAIL),
+            "name": self.normalise_name(claims.get(settings.OIDC_FIELD_NAME)),
+        }
+        email_domain = user_details["email"].split("@")[-1]
+        if email_domain in settings.JUSTICE_EMAIL_DOMAINS:
+            user_details["justice_email"] = user_details["email"]
+
+        return User.objects.create(**user_details)
+
+    def normalise_name(self, name):
+        """
+        Normalise name to be in the format "Firstname Lastname"
+        """
+        if "," in name:
+            parts = [part.strip() for part in name.split(",")]
+            name = " ".join(reversed(parts))
+        return name
 
     def update_user(self, user, claims):
         # Update the non-key information to sync the user's info
@@ -44,8 +58,9 @@ def update_user(self, user, claims):
         if user.email != claims.get(settings.OIDC_FIELD_EMAIL):
             user.email = claims.get(settings.OIDC_FIELD_EMAIL)
             user.save()
-        if user.name != claims.get(settings.OIDC_FIELD_NAME):
-            user.name = claims.get(settings.OIDC_FIELD_NAME)
+        normalised_name = self.normalise_name(claims.get(settings.OIDC_FIELD_NAME))
+        if user.name != normalised_name:
+            user.name = normalised_name
             user.save()
         return user
 

controlpanel/settings/common.py

@@ -198,6 +198,8 @@
 OIDC_FIELD_USERNAME = "nickname"
 OIDC_STORE_ID_TOKEN = True
 
+JUSTICE_EMAIL_DOMAINS = ["justice.gov.uk", "cica.justice.gov.uk"]
+
 # Auth0
 AUTH0 = {
     "client_id": OIDC_RP_CLIENT_ID,

tests/api/filters/test_app_filter.py

@@ -6,8 +6,8 @@
 
 class AppFilterTest(APITestCase):
     def setUp(self):
-        self.superuser = baker.make("api.User", is_superuser=True)
-        self.app_admin = baker.make("api.User", is_superuser=False)
+        self.superuser = baker.make("api.User", auth0_id="github|superuser", is_superuser=True)
+        self.app_admin = baker.make("api.User", auth0_id="github|user", is_superuser=False)
         self.app_1 = baker.make("api.App", name="App 1")
         self.app_2 = baker.make("api.App", name="App 2")
         baker.make("api.UserApp", user=self.app_admin, app=self.app_1, is_admin=True)

tests/api/filters/test_users3bucket_filter.py

@@ -10,9 +10,9 @@
 
 class UserS3BucketFilterTest(APITestCase):
     def setUp(self):
-        self.superuser = baker.make("api.User", is_superuser=True)
-        self.normal_user = baker.make("api.User", is_superuser=False)
-        self.other_user = baker.make("api.User", is_superuser=False)
+        self.superuser = baker.make("api.User", auth0_id="github|superuser", is_superuser=True)
+        self.normal_user = baker.make("api.User", auth0_id="github|user", is_superuser=False)
+        self.other_user = baker.make("api.User", auth0_id="github|other_user", is_superuser=False)
 
         self.s3bucket_1 = baker.make("api.S3Bucket", name="test-bucket-1")
         self.s3bucket_2 = baker.make("api.S3Bucket", name="test-bucket-2")

tests/api/models/test_user.py

@@ -118,3 +118,24 @@ def test_set_quicksight_access(users, enable):
             policy=cluster.User.QUICKSIGHT_POLICY_NAME,
             attach=enable,
         )
+
+
+@pytest.mark.parametrize(
+    "auth0_id, expected",
+    [
+        ("github|user_1", True),
+        ("github|user_2", True),
+        ("entra|user_1", False),
+        ("other|user_2", False),
+    ],
+)
+def test_user_is_github_user(auth0_id, expected):
+    user = User(auth0_id=auth0_id)
+    assert user.is_github_user == expected
+
+
+def test_non_github_user_not_provisioned():
+    user = User.objects.create(auth0_id="not_github|user_1")
+    with patch.object(cluster.User, "create") as mock_create:
+        user.save()
+        mock_create.assert_not_called()

tests/api/views/test_user.py

@@ -30,7 +30,7 @@ def ExtendedAuth0():
 def test_list(client, users):
     response = client.get(reverse("user-list"))
     assert response.status_code == status.HTTP_200_OK
-    assert len(response.data["results"]) == 8
+    assert len(response.data["results"]) == 9
 
 
 def test_detail(client, users):

tests/conftest.py

@@ -161,13 +161,13 @@ def users(
             "api.User",
             auth0_id="github|user_2",
             username="bob",
-            justice_email="Bob.Carolgees@justice.gov.uk",
+            justice_email="Bob.Mort@justice.gov.uk",
             is_superuser=False,
         ),
         "other_user": baker.make(
             "api.User",
             username="carol",
-            justice_email="Carol.Vorderman@justice.gov.uk",
+            justice_email="Carol.Vor@justice.gov.uk",
             auth0_id="github|user_3",
         ),
         "database_user": baker.make(
@@ -185,6 +185,7 @@ def users(
         ),
         "quicksight_compute_author": quicksight_author_user,
         "quicksight_compute_reader": quicksight_reader_user,
+        "non_tool_user": baker.make("api.User"),
     }
 
 

tests/frontend/views/test_app.py

@@ -46,7 +46,7 @@ def github_api_token():
 def users(users):
     users.update(
         {
-            "app_admin": baker.make("api.User", username="app_admin"),
+            "app_admin": baker.make("api.User", auth0_id="github|user", username="app_admin"),
         }
     )
     return users

tests/frontend/views/test_app_variables.py

@@ -14,7 +14,7 @@
 def users(users):
     users.update(
         {
-            "owner": baker.make("api.User", username="owner"),
+            "owner": baker.make("api.User", auth0_id="github|user", username="owner"),
         }
     )
     return users

tests/frontend/views/test_datasource.py

@@ -23,8 +23,12 @@ def enable_db_for_all_tests(db):
 def users(users):
     users.update(
         {
-            "bucket_viewer": baker.make("api.User", username="bucket_viewer"),
-            "bucket_admin": baker.make("api.User", username="bucket_admin"),
+            "bucket_viewer": baker.make(
+                "api.User", auth0_id="github|bucket_viewer", username="bucket_viewer"
+            ),
+            "bucket_admin": baker.make(
+                "api.User", auth0_id="github|bucket_admin", username="bucket_admin"
+            ),
         }
     )
     return users

tests/frontend/views/test_index.py

@@ -82,6 +82,17 @@ def test_with_justice_email(self, client, users):
         assert response.status_code == 302
         assert response.url == reverse("list-tools")
 
+    def test_not_tool_user(self, client, users):
+        user = users["non_tool_user"]
+        user.justice_email = "[email protected]"
+        user.save()
+        client.force_login(user)
+
+        response = client.get("/")
+
+        assert response.status_code == 302
+        assert response.url == reverse("help")
+
 
 class TestPost:
 

tests/frontend/views/test_parameter.py

@@ -12,7 +12,7 @@
 def users(users):
     users.update(
         {
-            "owner": baker.make("api.User", username="owner"),
+            "owner": baker.make("api.User", auth0_id="github|user", username="owner"),
         }
     )
     return users

tests/frontend/views/test_secrets.py

@@ -36,7 +36,7 @@ def github_api_token():
 def users(users):
     users.update(
         {
-            "app_admin": baker.make("api.User", username="app_admin"),
+            "app_admin": baker.make("api.User", auth0_id="github|user", username="app_admin"),
         }
     )
     return users

tests/frontend/views/test_user.py

@@ -111,7 +111,7 @@ def test_permission(client, users, view, user, expected_status):
 @pytest.mark.parametrize(
     "view,user,expected_count",
     [
-        (list, "superuser", 8),
+        (list, "superuser", 9),
     ],
 )
 def test_list(client, users, view, user, expected_count):

tests/test_oidc.py

@@ -1,11 +1,12 @@
 # Standard library
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
 
 # Third-party
 import pytest
+from django.conf import settings
 
 # First-party/Local
-from controlpanel.oidc import StateMismatchHandler
+from controlpanel.oidc import OIDCSubAuthenticationBackend, StateMismatchHandler
 
 
 @pytest.mark.parametrize(
@@ -24,3 +25,27 @@ def test_success_url(users, email, success_url):
     view.request = request
     view.user = user
     assert view.success_url == success_url
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "email, name, expected_name, expected_justice_email",
+    [
+        ("[email protected]", "User, Test", "Test User", None),
+        ("[email protected]", "Test User", "Test User", None),
+        ("[email protected]", "User, Test", "Test User", "[email protected]"),
+        ("[email protected]", "Test User", "Test User", "[email protected]"),
+    ],
+)
+def test_create_user(email, name, expected_name, expected_justice_email):
+    with patch("controlpanel.api.cluster.User.create"):
+        user = OIDCSubAuthenticationBackend().create_user(
+            {
+                "sub": "123",
+                settings.OIDC_FIELD_USERNAME: "testuser",
+                settings.OIDC_FIELD_EMAIL: email,
+                settings.OIDC_FIELD_NAME: name,
+            }
+        )
+        assert user.name == expected_name
+        assert user.justice_email == expected_justice_email