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 93cb6cd.

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

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

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"]) == 5
+    assert len(response.data["results"]) == 6
 
 
 def test_detail(client, users):

tests/conftest.py

@@ -137,6 +137,7 @@ def users(
             is_database_admin=True,
         ),
         "quicksight_user": quicksight_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

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

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

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