Skip to content

Commit

Permalink
Limit access unless user is created via GitHub auth
Browse files Browse the repository at this point in the history
  • Loading branch information
michaeljcollinsuk committed Jan 10, 2025
1 parent 0ff5da6 commit ecfdf23
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 10 deletions.
13 changes: 12 additions & 1 deletion controlpanel/api/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,17 @@ def show_webapp_data_link(self):

return self.userapps.filter(is_admin=True).exists()

@property
def is_github_user(self):
"""
Determin 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(
Expand Down Expand Up @@ -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
Expand Down
24 changes: 15 additions & 9 deletions controlpanel/api/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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",
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions controlpanel/frontend/jinja2/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
4 changes: 4 additions & 0 deletions controlpanel/frontend/views/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"))

Expand Down
21 changes: 21 additions & 0 deletions tests/api/models/test_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
1 change: 1 addition & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ def users(
is_database_admin=True,
),
"quicksight_user": quicksight_user,
"non_tool_user": baker.make("api.User"),
}


Expand Down
11 changes: 11 additions & 0 deletions tests/frontend/views/test_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down

0 comments on commit ecfdf23

Please sign in to comment.