From a5847226a418ea36bdaef376473340005f8a6003 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Mon, 13 Jan 2025 10:24:19 +0000
Subject: [PATCH] Update create user to expect users from Entra (#1429)

* Update create user to expect users from Entra

To enable CICA users to access the CP, we will
enable a new connection in Auth0 with EntraID.
This updates the code we use to create a user from
the ID token to normalize their name, and store
their email as the justice_email, to avoid them
having to reauth to capture it.

* Use users slug rather than username in helm

Previously the username would be used when installing user helm charts.
This was fine when all users came from github, as their usernames were
guaranteed to be valid with helm. However this is not the case with
usernames from Entra, as they can include invalid characters such as '.'
which results in an error after the user logs in trying to provision
the user. This changes uses the slug, which is valid for helm, however
accessing tools will still not be compatible so tooling will not be
available for these users.

* Improve justice email validation

* Revert "Use users slug rather than username in helm"

This reverts commit 93cb6cdabc70890504042d7eab7060b2ce2b81ff.

We will resolve the issue these changes were meant to fix in a different
way when we fully implement access with EntraID

* Limit access unless user is created via GitHub auth

* Additional test for creating user

* Fix typo

* Fix failing tests
---
 controlpanel/api/models/user.py               | 13 +++++++-
 controlpanel/api/rules.py                     | 24 ++++++++------
 controlpanel/frontend/jinja2/base.html        |  4 +++
 controlpanel/frontend/views/__init__.py       |  4 +++
 controlpanel/oidc.py                          | 31 ++++++++++++++-----
 controlpanel/settings/common.py               |  2 ++
 tests/api/filters/test_app_filter.py          |  4 +--
 tests/api/filters/test_users3bucket_filter.py |  6 ++--
 tests/api/models/test_user.py                 | 21 +++++++++++++
 tests/api/views/test_user.py                  |  2 +-
 tests/conftest.py                             |  1 +
 tests/frontend/views/test_app.py              |  2 +-
 tests/frontend/views/test_app_variables.py    |  2 +-
 tests/frontend/views/test_datasource.py       |  8 +++--
 tests/frontend/views/test_index.py            | 11 +++++++
 tests/frontend/views/test_parameter.py        |  2 +-
 tests/frontend/views/test_secrets.py          |  2 +-
 tests/frontend/views/test_user.py             |  2 +-
 tests/test_oidc.py                            | 29 +++++++++++++++--
 19 files changed, 137 insertions(+), 33 deletions(-)

diff --git a/controlpanel/api/models/user.py b/controlpanel/api/models/user.py
index 39e464292..4405f1a8a 100644
--- a/controlpanel/api/models/user.py
+++ b/controlpanel/api/models/user.py
@@ -126,6 +126,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(
@@ -161,7 +172,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
diff --git a/controlpanel/api/rules.py b/controlpanel/api/rules.py
index 6d692e70d..cd800fe0d 100644
--- a/controlpanel/api/rules.py
+++ b/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,6 +221,7 @@ 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)
@@ -223,8 +229,8 @@ def is_owner(user, obj):
 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)
diff --git a/controlpanel/frontend/jinja2/base.html b/controlpanel/frontend/jinja2/base.html
index 3daa89397..21ab1d9bf 100644
--- a/controlpanel/frontend/jinja2/base.html
+++ b/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",
diff --git a/controlpanel/frontend/views/__init__.py b/controlpanel/frontend/views/__init__.py
index 31a62e43c..d420e73cc 100644
--- a/controlpanel/frontend/views/__init__.py
+++ b/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"))
 
diff --git a/controlpanel/oidc.py b/controlpanel/oidc.py
index 2aa2bc173..beacb683a 100644
--- a/controlpanel/oidc.py
+++ b/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
 
diff --git a/controlpanel/settings/common.py b/controlpanel/settings/common.py
index 837649ea7..a4677c016 100644
--- a/controlpanel/settings/common.py
+++ b/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,
diff --git a/tests/api/filters/test_app_filter.py b/tests/api/filters/test_app_filter.py
index fb7a3dd17..af4cc7ae9 100644
--- a/tests/api/filters/test_app_filter.py
+++ b/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)
diff --git a/tests/api/filters/test_users3bucket_filter.py b/tests/api/filters/test_users3bucket_filter.py
index efafb2089..96afaa4f6 100644
--- a/tests/api/filters/test_users3bucket_filter.py
+++ b/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")
diff --git a/tests/api/models/test_user.py b/tests/api/models/test_user.py
index 108be4fca..8a3dcc261 100644
--- a/tests/api/models/test_user.py
+++ b/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()
diff --git a/tests/api/views/test_user.py b/tests/api/views/test_user.py
index b02339932..e55eed3df 100644
--- a/tests/api/views/test_user.py
+++ b/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"]) == 5
+    assert len(response.data["results"]) == 6
 
 
 def test_detail(client, users):
diff --git a/tests/conftest.py b/tests/conftest.py
index 613d0ee2f..033f8f938 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -137,6 +137,7 @@ def users(
             is_database_admin=True,
         ),
         "quicksight_user": quicksight_user,
+        "non_tool_user": baker.make("api.User"),
     }
 
 
diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py
index 6ed554c85..371e19b27 100644
--- a/tests/frontend/views/test_app.py
+++ b/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
diff --git a/tests/frontend/views/test_app_variables.py b/tests/frontend/views/test_app_variables.py
index a38b51101..8e3360e33 100644
--- a/tests/frontend/views/test_app_variables.py
+++ b/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
diff --git a/tests/frontend/views/test_datasource.py b/tests/frontend/views/test_datasource.py
index 46c3fa5c8..6022ac498 100644
--- a/tests/frontend/views/test_datasource.py
+++ b/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
diff --git a/tests/frontend/views/test_index.py b/tests/frontend/views/test_index.py
index 848028644..89532feb5 100644
--- a/tests/frontend/views/test_index.py
+++ b/tests/frontend/views/test_index.py
@@ -84,6 +84,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@example.com"
+        user.save()
+        client.force_login(user)
+
+        response = client.get("/")
+
+        assert response.status_code == 302
+        assert response.url == reverse("help")
+
 
 class TestPost:
 
diff --git a/tests/frontend/views/test_parameter.py b/tests/frontend/views/test_parameter.py
index 60b1b40ab..2d3794485 100644
--- a/tests/frontend/views/test_parameter.py
+++ b/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
diff --git a/tests/frontend/views/test_secrets.py b/tests/frontend/views/test_secrets.py
index b7d604b4e..33518d436 100644
--- a/tests/frontend/views/test_secrets.py
+++ b/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
diff --git a/tests/frontend/views/test_user.py b/tests/frontend/views/test_user.py
index 9864e7baf..938eebf63 100644
--- a/tests/frontend/views/test_user.py
+++ b/tests/frontend/views/test_user.py
@@ -112,7 +112,7 @@ def test_permission(client, users, view, user, expected_status):
 @pytest.mark.parametrize(
     "view,user,expected_count",
     [
-        (list, "superuser", 5),
+        (list, "superuser", 6),
     ],
 )
 def test_list(client, users, view, user, expected_count):
diff --git a/tests/test_oidc.py b/tests/test_oidc.py
index bd0275b9b..5508ccd7a 100644
--- a/tests/test_oidc.py
+++ b/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@exmaple.com", "User, Test", "Test User", None),
+        ("email@exmaple.com", "Test User", "Test User", None),
+        ("email@justice.gov.uk", "User, Test", "Test User", "email@justice.gov.uk"),
+        ("email@justice.gov.uk", "Test User", "Test User", "email@justice.gov.uk"),
+    ],
+)
+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