diff --git a/controlpanel/api/models/user.py b/controlpanel/api/models/user.py index 38acfcc4d..c0765f6c6 100644 --- a/controlpanel/api/models/user.py +++ b/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 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 84c3a5857..da4df9631 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 4e498fc23..3c07d7f7c 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"]) == 8 + assert len(response.data["results"]) == 9 def test_detail(client, users): diff --git a/tests/conftest.py b/tests/conftest.py index 384d9fe7b..e5dcea726 100644 --- a/tests/conftest.py +++ b/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"), } 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 8e979d94f..8f223ea1a 100644 --- a/tests/frontend/views/test_index.py +++ b/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@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 704e4effc..ba4527fac 100644 --- a/tests/frontend/views/test_user.py +++ b/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): 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