Bugfix/allow app registration

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(

controlpanel/api/rules.py

@@ -50,14 +50,14 @@ 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)
 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)

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"]
@@ -327,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(

controlpanel/frontend/jinja2/base.html

@@ -101,18 +101,16 @@
         "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",
       },
       {
-        "hide": not request.user.userapps.filter(is_admin=True).exists(),
         "text": "Webapps",
         "href": url("list-apps"),
         "active": page_name == "webapps",
       },
-
       {
         "hide": not request.user.is_superuser,
         "text": "Groups",

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

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

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

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

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,15 +185,16 @@ 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.
     assert f.is_valid() is False
     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(