From e964ab3069301ad944bda61cb8a39ad4665d1135 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Tue, 30 Jan 2024 15:40:21 +0000
Subject: [PATCH 1/5] Fix UI bugs to allow users to register apps

- Update rules to allow app admin to create Auth0 clients
- Display the Webapps nav button to all users, so they have access to the register app button
---
 controlpanel/api/rules.py                       | 2 +-
 controlpanel/frontend/jinja2/base.html          | 1 -
 controlpanel/frontend/jinja2/webapp-detail.html | 2 +-
 tests/api/permissions/test_app_permissions.py   | 4 ++--
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/controlpanel/api/rules.py b/controlpanel/api/rules.py
index 939d5f31a..7d19478f5 100644
--- a/controlpanel/api/rules.py
+++ b/controlpanel/api/rules.py
@@ -50,7 +50,7 @@ def is_app_admin(user, obj):
 add_perm("api.list_app", is_authenticated)
 add_perm("api.create_app", is_authenticated)
 add_perm("api.retrieve_app", is_authenticated & is_app_admin)
-add_perm("api.update_app", is_authenticated & is_superuser)
+add_perm("api.update_app", is_authenticated & is_app_admin)
 add_perm("api.destroy_app", is_authenticated & is_superuser)
 add_perm("api.add_app_customer", is_authenticated & is_app_admin)
 add_perm("api.remove_app_customer", is_authenticated & is_app_admin)
diff --git a/controlpanel/frontend/jinja2/base.html b/controlpanel/frontend/jinja2/base.html
index 5f1b0181e..4304d9e67 100644
--- a/controlpanel/frontend/jinja2/base.html
+++ b/controlpanel/frontend/jinja2/base.html
@@ -107,7 +107,6 @@
         "active": page_name == "webapp-datasource-list",
       },
       {
-        "hide": not request.user.userapps.filter(is_admin=True).exists(),
         "text": "Webapps",
         "href": url("list-apps"),
         "active": page_name == "webapps",
diff --git a/controlpanel/frontend/jinja2/webapp-detail.html b/controlpanel/frontend/jinja2/webapp-detail.html
index 99155982d..cb94acff2 100644
--- a/controlpanel/frontend/jinja2/webapp-detail.html
+++ b/controlpanel/frontend/jinja2/webapp-detail.html
@@ -90,7 +90,7 @@ <h2 class="govuk-heading-m">Deployment Pipeline</h2>
 
   {% for env_name, deployment_setting in deployments_settings.items() %}
   <h2 class="govuk-heading-m" >Deployment settings under {{ env_name }}</h2>
-    {% if deployment_setting.get('is_redundant') and request.user.has_perm('api.update_app', app) %}
+    {% if deployment_setting.get('is_redundant') and request.user.has_perm('api.destroy_app', app) %}
       <section class="cpanel-section">
         <p style="color:red">It appears this deployment environment is redundant and can be removed</p>
         <form action="{{ url('remove-app-deployment-env', kwargs={'pk': app.id, 'env_name': env_name}) }}" method="post">
diff --git a/tests/api/permissions/test_app_permissions.py b/tests/api/permissions/test_app_permissions.py
index fc378d9af..b7e53725b 100644
--- a/tests/api/permissions/test_app_permissions.py
+++ b/tests/api/permissions/test_app_permissions.py
@@ -99,13 +99,13 @@ def test_authenticated_user_has_basic_perms(client, users):
         (app_detail, "normal_user", status.HTTP_404_NOT_FOUND),
         (app_delete, "normal_user", status.HTTP_403_FORBIDDEN),
         (app_create, "normal_user", status.HTTP_201_CREATED),
-        (app_update, "normal_user", status.HTTP_403_FORBIDDEN),
+        (app_update, "normal_user", status.HTTP_404_NOT_FOUND),
 
         (app_list, "app_admin", status.HTTP_200_OK),
         (app_detail, "app_admin", status.HTTP_200_OK),
         (app_delete, "app_admin", status.HTTP_403_FORBIDDEN),
         (app_create, "app_admin", status.HTTP_201_CREATED),
-        (app_update, "app_admin", status.HTTP_403_FORBIDDEN),
+        (app_update, "app_admin", status.HTTP_200_OK),
     ],
 )
 @pytest.mark.django_db

From 8413943e742b871b9061e414a2ea4c883158ad6f Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Tue, 30 Jan 2024 16:49:48 +0000
Subject: [PATCH 2/5] Limit datasources to be linked to an app to users admin
 buckets

Updates the register app form, so that users cannot grant apps access
to datasources they dont have admin access for. Add unit test and
update other tests that require a user object
---
 controlpanel/frontend/forms.py | 17 +++++++-
 tests/frontend/test_forms.py   | 76 ++++++++++++++++++++++++++++------
 2 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/controlpanel/frontend/forms.py b/controlpanel/frontend/forms.py
index 7762f4fd8..e24b4b4a6 100644
--- a/controlpanel/frontend/forms.py
+++ b/controlpanel/frontend/forms.py
@@ -16,7 +16,7 @@
     RepositoryNotFound,
     extract_repo_info_from_url,
 )
-from controlpanel.api.models import App, S3Bucket, Tool, User
+from controlpanel.api.models import App, S3Bucket, Tool, User, UserS3Bucket
 from controlpanel.api.models.access_to_s3bucket import S3BUCKET_PATH_REGEX
 from controlpanel.api.models.iam_managed_policy import POLICY_NAME_REGEX
 from controlpanel.api.models.ip_allowlist import IPAllowlist
@@ -144,11 +144,24 @@ class CreateAppForm(AppAuth0Form):
         required=False,
     )
     existing_datasource_id = DatasourceChoiceField(
-        queryset=S3Bucket.objects.filter(is_data_warehouse=False, is_deleted=False),
+        queryset=S3Bucket.objects.none(),
         empty_label="Select",
         required=False,
     )
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.fields["existing_datasource_id"].queryset = self.get_datasource_queryset()
+
+    def get_datasource_queryset(self):
+        queryset = S3Bucket.objects.filter(is_data_warehouse=False, is_deleted=False)
+        if self.request.user.is_superuser:
+            return queryset
+
+        user_admin_buckets = self.request.user.users3buckets.filter(is_admin=True)
+        queryset = queryset.filter(users3buckets__in=user_admin_buckets)
+        return queryset
+
     def clean(self):
         cleaned_data = super().clean()
         connect_data_source = cleaned_data["connect_bucket"]
diff --git a/tests/frontend/test_forms.py b/tests/frontend/test_forms.py
index 4a10f523d..8473182a8 100644
--- a/tests/frontend/test_forms.py
+++ b/tests/frontend/test_forms.py
@@ -4,6 +4,7 @@
 # Third-party
 import pytest
 from django.core.exceptions import ValidationError
+from django.urls import reverse
 
 # First-party/Local
 from controlpanel.api import aws
@@ -118,7 +119,14 @@ def test_tool_release_form_get_target_users():
         )
 
 
-def test_create_app_form_clean_new_datasource():
+@pytest.fixture
+def create_app_request_superuser(rf, users):
+    request = rf.post(reverse("create-app"))
+    request.user = users["superuser"]
+    return request
+
+
+def test_create_app_form_clean_new_datasource(create_app_request_superuser):
     """
     The CreateAppForm class has a bespoke "clean" method. We should ensure it
     checks the expected things in the correct way.
@@ -128,7 +136,8 @@ def test_create_app_form_clean_new_datasource():
             "repo_url": "https://github.com/ministryofjustice/my_repo",
             "connect_bucket": "new",
             "new_datasource_name": "test-bucketname",
-        }
+        },
+        request=create_app_request_superuser,
     )
     f.clean_repo_url = mock.MagicMock()
     mock_s3 = mock.MagicMock()
@@ -142,7 +151,8 @@ def test_create_app_form_clean_new_datasource():
             "deployment_envs": ["test"],
             "repo_url": "https://github.com/ministryofjustice/my_repo",
             "connect_bucket": "new",
-        }
+        },
+        request=create_app_request_superuser,
     )
     f.clean_repo_url = mock.MagicMock()
     assert f.is_valid() is False
@@ -155,7 +165,8 @@ def test_create_app_form_clean_new_datasource():
             "repo_url": "https://github.com/ministryofjustice/my_repo",
             "connect_bucket": "new",
             "new_datasource_name": "test-bucketname",
-        }
+        },
+        request=create_app_request_superuser,
     )
     f.clean_repo_url = mock.MagicMock()
     mock_s3 = mock.MagicMock()
@@ -164,7 +175,7 @@ def test_create_app_form_clean_new_datasource():
         assert "Datasource named test-bucketname already exists" in f.errors["new_datasource_name"][0]
 
 
-def test_create_app_form_clean_existing_datasource():
+def test_create_app_form_clean_existing_datasource(create_app_request_superuser):
     """
     An existing datasource name is required if the datasource is marked as
     already existing.
@@ -174,7 +185,8 @@ def test_create_app_form_clean_existing_datasource():
             "repo_url": "https://github.com/moj-analytical-services/my_repo",
             "connect_bucket": "existing",
             "connections": ["email"],
-        }
+        },
+        request=create_app_request_superuser,
     )
     f.clean_repo_url = mock.MagicMock()
     # A valid form returns True.
@@ -182,7 +194,7 @@ def test_create_app_form_clean_existing_datasource():
     assert "existing_datasource_id" in f.errors
 
 
-def test_create_app_form_new_datasource_but_bucket_existed():
+def test_create_app_form_new_datasource_but_bucket_existed(create_app_request_superuser):
     bucket_name = "test-bucketname"
     aws.AWSBucket().create(bucket_name, is_data_warehouse=True)
 
@@ -192,7 +204,8 @@ def test_create_app_form_new_datasource_but_bucket_existed():
             "connect_bucket": "new",
             "new_datasource_name": bucket_name,
             "connections": ["email"],
-        }
+        },
+        request=create_app_request_superuser,
     )
     f.clean_repo_url = mock.MagicMock()
     assert f.is_valid() is False
@@ -244,7 +257,7 @@ def test_grant_access_form_clean_paths(path, expected_error):
         assert form.clean_paths() == [path]
 
 
-def test_create_app_form_clean_repo_url():
+def test_create_app_form_clean_repo_url(create_app_request_superuser):
     """
     Ensure the various states of a GitHub repository result in a valid form or
     errors.
@@ -255,7 +268,8 @@ def test_create_app_form_clean_repo_url():
             "repo_url": "https://github.com/ministryofjustice/my_repo",
             "connect_bucket": "new",
             "new_datasource_name": "test-bucketname",
-        }
+        },
+        request=create_app_request_superuser,
     )
     f.request = mock.MagicMock()
     mock_get_repo = mock.MagicMock(return_value=True)
@@ -276,7 +290,8 @@ def test_create_app_form_clean_repo_url():
             "repo_url": "https://github.com/ministryofjustice/my_repo",
             "connect_bucket": "new",
             "new_datasource_name": "test-bucketname",
-        }
+        },
+        request=create_app_request_superuser
     )
     f.request = mock.MagicMock()
     mock_app = mock.MagicMock()
@@ -297,7 +312,8 @@ def test_create_app_form_clean_repo_url():
             "repo_url": "https://github.com/ministryofjustice/doesnt-exist",
             "connect_bucket": "new",
             "new_datasource_name": "test-bucketname",
-        }
+        },
+        request=create_app_request_superuser,
     )
     f.request = mock.MagicMock()
     mock_app = mock.MagicMock()
@@ -313,6 +329,42 @@ def test_create_app_form_clean_repo_url():
         assert "Github repository not found - it may be private" in error
 
 
+@pytest.mark.parametrize("user", ["superuser", "normal_user", "other_user"])
+def test_create_app_form_get_datasource_queryset(users, rf, user):
+    """
+    Assert that for each user,
+    """
+    superuser_bucket = S3Bucket.objects.create(
+        name="superuser_bucket",
+        created_by=users["superuser"],
+        is_data_warehouse=False,
+    )
+    user_bucket = S3Bucket.objects.create(
+        name="user_bucket",
+        created_by=users["normal_user"],
+        is_data_warehouse=False,
+    )
+    warehouse_bucket = S3Bucket.objects.create(
+        name="warehouse_bucket",
+        created_by=users["superuser"],
+        is_data_warehouse=True,
+    )
+    expected_buckets = {
+        "superuser": [superuser_bucket, user_bucket],
+        "normal_user": [user_bucket],
+        "other_user": []
+    }
+
+    request = rf.post(reverse("create-app"))
+    request.user = users[user]
+    form = forms.CreateAppForm(request=request)
+
+    queryset = form.get_datasource_queryset()
+
+    assert list(queryset) == expected_buckets[user]
+    assert warehouse_bucket not in expected_buckets[user]
+
+
 def test_update_app_with_custom_connection():
     # Good case.
     f = forms.UpdateAppAuth0ConnectionsForm(

From 4a05247f5119baf044a8747e911299c101490f6a Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 31 Jan 2024 11:38:25 +0000
Subject: [PATCH 3/5] Limit datasource options to link an app to

On manage app page, only display datasources that the app admin
already has access to. Also limit only to non-data warehouses.
Superusers can see all buckets.
---
 controlpanel/api/rules.py                     |  4 +--
 controlpanel/frontend/forms.py                | 29 ++++++++++++++-----
 .../frontend/jinja2/webapp-detail.html        |  2 +-
 controlpanel/frontend/views/app.py            |  7 +++--
 tests/frontend/views/test_app.py              |  8 ++---
 5 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/controlpanel/api/rules.py b/controlpanel/api/rules.py
index 7d19478f5..8ac819383 100644
--- a/controlpanel/api/rules.py
+++ b/controlpanel/api/rules.py
@@ -56,8 +56,8 @@ def is_app_admin(user, obj):
 add_perm("api.remove_app_customer", is_authenticated & is_app_admin)
 add_perm("api.add_app_admin", is_authenticated & is_app_admin)
 add_perm("api.revoke_app_admin", is_authenticated & is_app_admin)
-add_perm("api.add_app_bucket", is_authenticated & is_superuser)
-add_perm("api.remove_app_bucket", is_authenticated & is_superuser)
+add_perm("api.add_app_bucket", is_authenticated & is_app_admin)
+add_perm("api.remove_app_bucket", is_authenticated & is_superuser)  # TODO change to is_app_admin
 add_perm("api.view_app_logs", is_authenticated & is_app_admin)
 add_perm("api.manage_groups", is_authenticated & is_superuser)
 add_perm("api.create_policys3bucket", is_authenticated & is_superuser)
diff --git a/controlpanel/frontend/forms.py b/controlpanel/frontend/forms.py
index e24b4b4a6..982c37d87 100644
--- a/controlpanel/frontend/forms.py
+++ b/controlpanel/frontend/forms.py
@@ -340,19 +340,34 @@ class GrantAppAccessForm(forms.Form):
 
     def __init__(self, *args, **kwargs):
         self.app = kwargs.pop("app")
-        self.exclude_connected = kwargs.pop("exclude_connected", False)
+        self.user = kwargs.pop("user")
+        self.exclude_connected = kwargs.pop("exclude_connected", True)
 
         super().__init__(*args, **kwargs)
 
+        self.fields["datasource"].queryset = self.get_datasource_queryset()
+
+    def get_datasource_queryset(self):
+        """
+        For all users excludes deleted buckets
+        Optionally excludes connected buckets
+        If the user is a superuser, returns all remaining buckets
+        Otherwise, returns only non-data warehouse buckets that the user has access to
+        """
+        queryset = S3Bucket.objects.filter(is_deleted=False)
         if self.exclude_connected:
-            self.fields["datasource"].queryset = S3Bucket.objects.exclude(
-                id__in=[a.s3bucket_id for a in self.app.apps3buckets.all()],
-            ).filter(is_deleted=False)
-        else:
-            self.fields["datasource"].queryset = S3Bucket.objects.filter(
-                is_deleted=False,
+            queryset = queryset.exclude(
+                pk__in=self.app.apps3buckets.values_list("s3bucket__pk", flat=True)
             )
 
+        if self.user.is_superuser:
+            return queryset
+
+        return queryset.filter(
+            users3buckets__in=self.user.users3buckets.filter(is_admin=True),
+            is_data_warehouse=False,
+        )
+
 
 class CreateIAMManagedPolicyForm(forms.Form):
     name = forms.CharField(
diff --git a/controlpanel/frontend/jinja2/webapp-detail.html b/controlpanel/frontend/jinja2/webapp-detail.html
index cb94acff2..d3a501d22 100644
--- a/controlpanel/frontend/jinja2/webapp-detail.html
+++ b/controlpanel/frontend/jinja2/webapp-detail.html
@@ -204,7 +204,7 @@ <h2 class="govuk-heading-m">App data sources</h2>
       </tr>
     </thead>
     <tbody class="govuk-table__body">
-      {% set buckets = app.apps3buckets.all() %}
+      {% set buckets = app.apps3buckets.filter(s3bucket__is_deleted=False) %}
       {% set num_buckets = buckets|length %}
       {% for bucket in buckets %}
       <tr class="govuk-table__row">
diff --git a/controlpanel/frontend/views/app.py b/controlpanel/frontend/views/app.py
index 02750ec3d..34324bbcc 100644
--- a/controlpanel/frontend/views/app.py
+++ b/controlpanel/frontend/views/app.py
@@ -113,7 +113,7 @@ def get_context_data(self, **kwargs):
 
         context["grant_access_form"] = GrantAppAccessForm(
             app=app,
-            exclude_connected=True,
+            user=self.request.user,
         )
 
         # If auth settings not returned, all envs marked redundant in the serializer.
@@ -258,16 +258,17 @@ def get_success_url(self, app):
 class GrantAppAccess(
     OIDCLoginRequiredMixin,
     PermissionRequiredMixin,
-    CreateView,
+    UpdateView,
 ):
     form_class = GrantAppAccessForm
-    model = AppS3Bucket
+    model = App
     permission_required = "api.add_app_bucket"
 
     def get_form_kwargs(self):
         kwargs = FormMixin.get_form_kwargs(self)
         if "app" not in kwargs:
             kwargs["app"] = App.objects.get(pk=self.kwargs["pk"])
+        kwargs["user"] = self.request.user
         return kwargs
 
     def get_success_url(self):
diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py
index 9033f96d8..d3697c02d 100644
--- a/tests/frontend/views/test_app.py
+++ b/tests/frontend/views/test_app.py
@@ -177,11 +177,11 @@ def repos_for_redundant_auth(githubapi):
 
 
 @pytest.fixture(autouse=True)
-def s3buckets(app):
+def s3buckets(app, users):
     with patch("controlpanel.api.aws.AWSBucket.create") as _:
         buckets = {
-            "not_connected": mommy.make("api.S3Bucket"),
-            "connected": mommy.make("api.S3Bucket"),
+            "not_connected": mommy.make("api.S3Bucket", created_by=users["app_admin"]),
+            "connected": mommy.make("api.S3Bucket", created_by=users["app_admin"]),
         }
         return buckets
 
@@ -303,7 +303,7 @@ def update_ip_allowlists(client, app, *args):
         (remove_customer_by_email, "app_admin", status.HTTP_302_FOUND),
         (remove_customer_by_email, "normal_user", status.HTTP_403_FORBIDDEN),
         (connect_bucket, "superuser", status.HTTP_302_FOUND),
-        (connect_bucket, "app_admin", status.HTTP_403_FORBIDDEN),
+        (connect_bucket, "app_admin", status.HTTP_302_FOUND),
         (connect_bucket, "normal_user", status.HTTP_403_FORBIDDEN),
         (update_ip_allowlists, "superuser", status.HTTP_302_FOUND),
         (update_ip_allowlists, "app_admin", status.HTTP_302_FOUND),

From b15cbc8ae55b8e203c7ddd08387d914302f95ccd Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 31 Jan 2024 11:54:39 +0000
Subject: [PATCH 4/5] Update logic to display webapp data link

Add method to user to check if they are an app admin, and if so
display the webapp data button
---
 controlpanel/api/models/user.py        | 19 +++++++++++++++++++
 controlpanel/frontend/jinja2/base.html |  3 +--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/controlpanel/api/models/user.py b/controlpanel/api/models/user.py
index dbca72d27..96bfef0d6 100644
--- a/controlpanel/api/models/user.py
+++ b/controlpanel/api/models/user.py
@@ -3,6 +3,7 @@
 from django.contrib.auth.models import AbstractUser
 from django.contrib.auth.signals import user_logged_in
 from django.db import models
+from django.utils.functional import cached_property
 
 # First-party/Local
 from controlpanel.api import auth0, cluster, slack
@@ -94,6 +95,24 @@ def github_api_token(self, value):
     def slug(self):
         return sanitize_dns_label(self.username)
 
+    @cached_property
+    def show_webapp_data_link(self):
+        """
+        Check if the user already has an app bucket, or if the user is an admin of an
+        app
+        """
+        if self.is_superuser:
+            return True
+
+        if self.users3buckets.filter(
+                s3bucket__is_deleted=False
+        ).exclude(
+            s3bucket__is_data_warehouse=True
+        ).exists():
+            return True
+
+        return self.userapps.filter(is_admin=True).exists()
+
     def is_app_admin(self, app_id):
         return (
             self.userapps.filter(
diff --git a/controlpanel/frontend/jinja2/base.html b/controlpanel/frontend/jinja2/base.html
index 4304d9e67..6367bc148 100644
--- a/controlpanel/frontend/jinja2/base.html
+++ b/controlpanel/frontend/jinja2/base.html
@@ -101,7 +101,7 @@
         "active": page_name == "warehouse-datasource-list",
       },
       {
-        "hide": not request.user.users3buckets.filter(s3bucket__is_data_warehouse=False).exists(),
+        "hide": not request.user.show_webapp_data_link,
         "text": "Webapp data",
         "href": url("list-webapp-datasources"),
         "active": page_name == "webapp-datasource-list",
@@ -111,7 +111,6 @@
         "href": url("list-apps"),
         "active": page_name == "webapps",
       },
-
       {
         "hide": not request.user.is_superuser,
         "text": "Groups",

From cdb415d619eb48987dcff6cebad7c70140687434 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 1 Feb 2024 17:30:46 +0000
Subject: [PATCH 5/5] Hide register app button for non-superusers

---
 controlpanel/frontend/jinja2/webapp-list.html | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/controlpanel/frontend/jinja2/webapp-list.html b/controlpanel/frontend/jinja2/webapp-list.html
index 5b5844cda..1c7f886d7 100644
--- a/controlpanel/frontend/jinja2/webapp-list.html
+++ b/controlpanel/frontend/jinja2/webapp-list.html
@@ -12,7 +12,7 @@ <h1 class="govuk-heading-xl">{{ page_title }}</h1>
   {% if request.user.has_perm('api.list_app') %}
   {{ app_list(apps, request.user) }}
 
-    {% if request.user.has_perm('api.create_app') %}
+    {% if request.user.is_superuser and request.user.has_perm('api.create_app') %}
     <p class="govuk-body">
       <a class="govuk-button" href="{{ url('create-app') }}">
         Register an app