From 83d620aed2e4a96c0ebb83480062fe3904c5b395 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Thu, 7 Dec 2023 16:29:30 +0000 Subject: [PATCH 01/12] Simplify the register app form Only ask for a repo URL and datasource when registering an app. This is because the form will now be publically accessible, with a new flow for creating auth0 clients. Also removes JS to lookup repositories, as this is no longer needed. --- controlpanel/frontend/forms.py | 23 --- .../frontend/jinja2/webapp-create.html | 136 ------------------ .../modules/register-app-from-repo.js | 85 ----------- controlpanel/frontend/views/apps_mng.py | 22 --- controlpanel/settings/common.py | 1 - 5 files changed, 267 deletions(-) delete mode 100644 controlpanel/frontend/static/javascripts/modules/register-app-from-repo.js diff --git a/controlpanel/frontend/forms.py b/controlpanel/frontend/forms.py index 4fe857c1c..a3873cfc4 100644 --- a/controlpanel/frontend/forms.py +++ b/controlpanel/frontend/forms.py @@ -115,11 +115,6 @@ def _check_inputs_for_custom_connection(self, cleaned_data): class CreateAppForm(AppAuth0Form): - org_names = forms.ChoiceField( - required=True, - choices=list(zip(settings.GITHUB_ORGS, settings.GITHUB_ORGS)), - ) - repo_url = forms.CharField( max_length=512, validators=[ @@ -151,20 +146,6 @@ class CreateAppForm(AppAuth0Form): required=False, ) - disable_authentication = forms.BooleanField(required=False) - - app_ip_allowlists = forms.ModelMultipleChoiceField( - required=False, queryset=IPAllowlist.objects.filter(deleted=False) - ) - - deployment_envs = DynamicMultiChoiceField(required=False) - - def __init__(self, *args, **kwargs): - super(CreateAppForm, self).__init__(*args, **kwargs) - self.fields["app_ip_allowlists"].initial = IPAllowlist.objects.filter( - is_recommended=True - ) - def clean(self): cleaned_data = super().clean() connect_data_source = cleaned_data["connect_bucket"] @@ -188,10 +169,6 @@ def clean(self): if connect_data_source == "existing" and not existing_datasource: self.add_error("existing_datasource_id", "This field is required.") - cleaned_data["auth0_connections"] = self._check_inputs_for_custom_connection( - cleaned_data - ) - return cleaned_data def clean_repo_url(self): diff --git a/controlpanel/frontend/jinja2/webapp-create.html b/controlpanel/frontend/jinja2/webapp-create.html index 9f6c84fff..a066eda94 100644 --- a/controlpanel/frontend/jinja2/webapp-create.html +++ b/controlpanel/frontend/jinja2/webapp-create.html @@ -56,54 +56,7 @@

{{ page_title }}

- - -
{% set error_repo_msg = form.errors.get("repo_url") %} @@ -119,30 +72,6 @@

{{ page_title }}



-
@@ -176,71 +105,6 @@

{{ page_title }}

] }) }} - {% set permissions_link %} -
For more information on permissions, please have a look at this document.
- {% endset %} - - {{ - govukCheckboxes({ - "fieldset": { - "legend": { - "text": "Disable Authentication", - "classes": "govuk-fieldset__legend--m", - }, - }, - "classes": "govuk-!-width-two-thirds", - "hint": { - "text": 'A flag to indicate if the app disables authentication. ' + permissions_link + " The auth0 won't be created if the option is ticked" - }, - "name": "disable_authentication", - "items": [ - { - "value": "True", - "text": "Disable app authentication", - "checked": form.disable_authentication.value(), - }, - ], - "errorMessage": { "html": form.disable_authentication.errors|join(". ") } if form.disable_authentication.errors else {} - }) - }} - -
- - - Customers will only be able to access this app from the IP networks in the allowlists selected below. - If no allowlists are selected, the app will be accessible from any IP address. - - -
- {% for ip_allowlist in form.fields.app_ip_allowlists.choices %} - {% set id = "app-ip-allowlists-" + loop.index|string %} - {% set name = "app_ip_allowlists" %} -
- - {{ govukLabel({ - "text": ip_allowlist[1], - "classes": 'govuk-checkboxes__label' + (' ' + (ip_allowlist.label|default({})).classes|default("")), - "attributes": (ip_allowlist.label|default({})).attributes|default(""), - "for": id - }) }} -
- {% endfor %} -
-
- - {{ auth0_connections_form({ - "fieldset": { - "legend": { - "text": "Oauth0 client - connections", - "classes": "govuk-fieldset__legend--m", - }, - }, - "errors": form.errors, - "field": form.fields['connections'], - "selected_values": form.connections.value() - } ) - }} -
diff --git a/controlpanel/frontend/static/javascripts/modules/register-app-from-repo.js b/controlpanel/frontend/static/javascripts/modules/register-app-from-repo.js deleted file mode 100644 index 500f80f59..000000000 --- a/controlpanel/frontend/static/javascripts/modules/register-app-from-repo.js +++ /dev/null @@ -1,85 +0,0 @@ -var repos = []; - -moj.Modules.registerAppFromRepo = { - $repoListSelector: $("#display_result_repo"), - formId: "register_app", - $orgSelectName: $("input[type=radio][name=org_names]"), - $loadingGIF: $("#loading_gif"), - $loadingText: $("#loading_text"), - $currentPageIndex: $('#current_index'), - $reposLoaded: $('#repos_loaded'), - $nextRepoPageBtn: $('#add_more'), - $envMultiSelector: $("#deployment_envs_list"), - - init() { - if ($(`form#${this.formId}`).length) { - this.$form = $(`form#${this.formId}`); - this.$formSubmit = this.$form.find('input[type="submit"]'); - - this.$repoListSelector.autocomplete({source: repos}); - $('#ui-id-1').css({'padding-inline-start': '0px'}) - - this.bindEvents(); - this.addToRepos(1); - } - }, - - addToRepos(index) { - let currentOrg = $("input[name='org_names']:checked").val(); - this.$loadingGIF.show(); - - fetch('/api/cpanel/v1/repos/' + currentOrg + '/?' + new URLSearchParams({page: index})) - .then(response => response.json()) - .then(data => { - repos = repos.concat(data.map(item => ({label: item.full_name, value: item.html_url}))); - this.$reposLoaded.text(repos.length) - this.$repoListSelector.autocomplete('option', 'source', repos); - - this.$loadingGIF.hide(); - this.$loadingText.text("added: "); - - this.$currentPageIndex.val(index +1); - }) - .catch(err => console.log('err', err)); - }, - - readRepoEnvs() { - let currentOrg = this.$orgSelectName.val(); - let currentRepo = this.$repoListSelector.val(); - let repoURLs = currentRepo.split("/") - fetch('/api/cpanel/v1/repos/' + repoURLs[3] + '/' + repoURLs[4] + '/environments') - .then(response => response.json()) - .then(data => { - this.$envMultiSelector.empty(); - for (let i = 0; i < data.length; ++i) { - var item_str = '
'; - item_str += ''; - item_str += ''; - item_str += '
'; - this.$envMultiSelector.append(item_str); - } - }) - .catch(err => console.log('err', err)); - }, - - bindEvents() { - - this.$nextRepoPageBtn.on('click', () => { - let index = parseInt($('#current_index').val()); - this.addToRepos(index); - }); - - this.$repoListSelector.on('change', () => { - this.readRepoEnvs(); - }); - - this.$orgSelectName.on('change', () => { - repos = []; - this.$currentPageIndex.val(1); - let index = parseInt(this.$currentPageIndex.val()); - this.addToRepos(index) - }); - - }, - -} diff --git a/controlpanel/frontend/views/apps_mng.py b/controlpanel/frontend/views/apps_mng.py index 7a54d7b30..24b7b9b19 100644 --- a/controlpanel/frontend/views/apps_mng.py +++ b/controlpanel/frontend/views/apps_mng.py @@ -23,12 +23,6 @@ class AppManager: """ def register_app(self, user, app_data): - # prepare the data - github_api_token = user.github_api_token - envs = app_data.get("deployment_envs") - disable_authentication = app_data.get("disable_authentication") or False - connections = app_data.get("auth0_connections") - ip_allowlists = app_data.get("app_ip_allowlists") repo_url = app_data["repo_url"] _, name = repo_url.rsplit("/", 1) @@ -36,26 +30,10 @@ def register_app(self, user, app_data): new_app = self._create_app( name=name, repo_url=repo_url, - disable_authentication=disable_authentication, - connections=connections, current_user=user, - deployment_envs=envs, - has_ip_ranges=True if ip_allowlists else False ) - self._add_ip_allowlists(new_app, envs, ip_allowlists) self._add_app_to_users(new_app, user) - # self._create_app_role(new_app) self._create_or_link_datasource(new_app, user, app_data) - # with transaction.atomic(): - # self._add_ip_allowlists(new_app, envs, ip_allowlists) - # self._add_app_to_users(new_app, user) - # # self._create_app_role(new_app) - # self._create_or_link_datasource(new_app, user, app_data) - - # self._create_auth_settigs( - # new_app, envs, github_api_token, disable_authentication, connections - # ) - return new_app def trigger_tasks_for_ip_range_removal(self, user, deleted_object): diff --git a/controlpanel/settings/common.py b/controlpanel/settings/common.py index d12a7c955..bc57be88f 100644 --- a/controlpanel/settings/common.py +++ b/controlpanel/settings/common.py @@ -427,7 +427,6 @@ os.environ.get("GITHUB_ORGS", "").split(",") + [ 'ministryofjustice', - "moj-analytical-services", ] ), ) From dab5e45fa985c808a15a24f2953840f143d53975 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Mon, 8 Jan 2024 12:17:11 +0000 Subject: [PATCH 02/12] Update app registration test following changes --- tests/frontend/views/test_app.py | 35 +++++++++++++++++++------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py index 6c34ac3e9..4e22a028c 100644 --- a/tests/frontend/views/test_app.py +++ b/tests/frontend/views/test_app.py @@ -3,20 +3,19 @@ from unittest.mock import patch # Third-party -from bs4 import BeautifulSoup import pytest import requests +from bs4 import BeautifulSoup from django.contrib.messages import get_messages from django.urls import reverse from model_mommy import mommy from rest_framework import status # First-party/Local -from controlpanel.api import cluster -from controlpanel.api import auth0 -from controlpanel.api.models.app import DeleteCustomerError -from tests.api.fixtures.aws import * +from controlpanel.api import auth0, cluster from controlpanel.api.models import App, AppIPAllowList, S3Bucket +from controlpanel.api.models.app import DeleteCustomerError +from tests.api.fixtures.aws import * # noqa NUM_APPS = 3 @@ -210,12 +209,7 @@ def update_auth0_connections(client, app, *args): def create(client, *args): - data = { - "repo_url": "https://github.com/moj-analytical-services/test_app", - "connect_bucket": "later", - "disable_authentication": False, - } - return client.post(reverse("create-app"), data) + return client.get(reverse("create-app")) def delete(client, app, *args): @@ -713,11 +707,9 @@ def test_register_app_with_creating_datasource(client, users): assert App.objects.filter(name=test_app_name).count() == 0 client.force_login(users["superuser"]) data = dict( - org_names="moj-analytical-services", - repo_url=f"https://github.com/moj-analytical-services/{test_app_name}", + repo_url=f"https://github.com/ministryofjustice/{test_app_name}", connect_bucket="new", new_datasource_name=test_bucket_name, - connections=[] ) response = client.post(reverse("create-app"), data) @@ -730,3 +722,18 @@ def test_register_app_with_creating_datasource(client, users): assert len(related_bucket_ids) == 1 assert bucket.id in related_bucket_ids + +def test_register_app_invalid_organisation(client, users): + client.force_login(users["superuser"]) + app_name = "example-app-old-org" + data = dict( + repo_url=f"https://github.com/moj-analytical-services/{app_name}", + connect_bucket="later", + ) + + response = client.post(reverse("create-app"), data) + + # 200 due to errors + assert response.status_code == 200 + assert "repo_url" in response.context_data["form"].errors + assert App.objects.filter(name=app_name).count() == 0 From 0160cc3e0cb5a1835779bd79c7c9e99939c7ab0b Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Mon, 8 Jan 2024 17:14:42 +0000 Subject: [PATCH 03/12] Fix create app form tests --- tests/frontend/test_forms.py | 98 +++----------------------------- tests/frontend/views/test_app.py | 2 +- 2 files changed, 10 insertions(+), 90 deletions(-) diff --git a/tests/frontend/test_forms.py b/tests/frontend/test_forms.py index db5510598..6a95abb88 100644 --- a/tests/frontend/test_forms.py +++ b/tests/frontend/test_forms.py @@ -124,13 +124,9 @@ def test_create_app_form_clean_new_datasource(): """ f = forms.CreateAppForm( data={ - "org_names": "moj-analytical-services", - "deployment_envs": ["test"], - "repo_url": "https://github.com/moj-analytical-services/my_repo", + "repo_url": "https://github.com/ministryofjustice/my_repo", "connect_bucket": "new", "new_datasource_name": "test-bucketname", - "connections": ["email"], - "disable_authentication": True, } ) f.clean_repo_url = mock.MagicMock() @@ -142,33 +138,29 @@ def test_create_app_form_clean_new_datasource(): # A new datasource name is required if the connection is new. f = forms.CreateAppForm( data={ - "org_names": "moj-analytical-services", "deployment_envs": ["test"], - "repo_url": "https://github.com/moj-analytical-services/my_repo", + "repo_url": "https://github.com/ministryofjustice/my_repo", "connect_bucket": "new", - "disable_authentication": True, } ) f.clean_repo_url = mock.MagicMock() assert f.is_valid() is False assert "new_datasource_name" in f.errors + assert "This field is required" in f.errors["new_datasource_name"][0] # If a datasource already exists, report the duplication. f = forms.CreateAppForm( data={ - "org_names": "moj-analytical-services", "deployment_envs": ["test"], - "repo_url": "https://github.com/moj-analytical-services/my_repo", + "repo_url": "https://github.com/ministryofjustice/my_repo", "connect_bucket": "new", "new_datasource_name": "test-bucketname", - "connections": ["email"], - "disable_authentication": True, } ) f.clean_repo_url = mock.MagicMock() mock_s3 = mock.MagicMock() with mock.patch("controlpanel.frontend.forms.S3Bucket.objects", mock_s3): assert f.is_valid() is False - assert "new_datasource_name" in f.errors + assert "Datasource named test-bucketname already exists" in f.errors["new_datasource_name"][0] def test_create_app_form_clean_existing_datasource(): @@ -259,13 +251,9 @@ def test_create_app_form_clean_repo_url(): # The good case. f = forms.CreateAppForm( data={ - "org_names": "moj-analytical-services", - "deployment_envs": ["test"], - "repo_url": "https://github.com/moj-analytical-services/my_repo", + "repo_url": "https://github.com/ministryofjustice/my_repo", "connect_bucket": "new", "new_datasource_name": "test-bucketname", - "connections": ["email"], - "disable_authentication": True, } ) f.request = mock.MagicMock() @@ -284,12 +272,9 @@ def test_create_app_form_clean_repo_url(): # Repo not found. f = forms.CreateAppForm( data={ - "org_names": "moj-analytical-services", - "deployment_envs": ["test"], "repo_url": "https://github.com/moj-analytical-services/my_repo", "connect_bucket": "new", "new_datasource_name": "test-bucketname", - "disable_authentication": True, } ) f.request = mock.MagicMock() @@ -304,17 +289,14 @@ def test_create_app_form_clean_repo_url(): "controlpanel.frontend.forms.S3Bucket.objects", mock_s3 ): assert f.is_valid() is False - assert "repo_url" in f.errors + assert "Unknown Github organization" in f.errors["repo_url"][0] # App already exists. f = forms.CreateAppForm( data={ - "org_names": "moj-analytical-services", - "deployment_envs": ["test"], - "repo_url": "https://github.com/moj-analytical-services/my_repo", + "repo_url": "https://github.com/ministryofjustice/my_repo", "connect_bucket": "new", "new_datasource_name": "test-bucketname", - "disable_authentication": True, } ) f.request = mock.MagicMock() @@ -329,69 +311,7 @@ def test_create_app_form_clean_repo_url(): "controlpanel.frontend.forms.S3Bucket.objects", mock_s3 ): assert f.is_valid() is False - assert "repo_url" in f.errors - - -def test_create_app_with_custom_connection(): - # Good case. - f = forms.CreateAppForm( - data={ - "org_names": "moj-analytical-services", - "deployment_envs": ["test"], - "repo_url": "https://github.com/moj-analytical-services/my_repo", - "connect_bucket": "new", - "new_datasource_name": "test-bucketname", - "connections": ["email", "auth0_nomis"], - "disable_authentication": True, - "auth0_nomis_auth0_client_id": "nomis-client-id", - "auth0_nomis_auth0_client_secret": "nomis-client-secret", - "auth0_nomis_auth0_conn_name": "nomis-conn-name", - }, - all_connections_names=["github", "email", "auth0_nomis"], - custom_connections=["auth0_nomis"], - ) - f.request = mock.MagicMock() - mock_get_repo = mock.MagicMock(return_value=True) - mock_app = mock.MagicMock() - mock_app.objects.filter().exists.return_value = False - mock_s3 = mock.MagicMock() - mock_s3.get.side_effect = S3Bucket.DoesNotExist("Boom") - with mock.patch( - "controlpanel.frontend.forms.GithubAPI.get_repository", mock_get_repo - ), mock.patch("controlpanel.frontend.forms.App", mock_app), mock.patch( - "controlpanel.frontend.forms.S3Bucket.objects", mock_s3 - ): - assert f.is_valid() is True - - # Bad case: missing client credential for nomis login + not valid connection name - f = forms.CreateAppForm( - data={ - "repo_url": "https://github.com/moj-analytical-services/my_repo", - "connect_bucket": "new", - "new_datasource_name": "test-bucketname", - "connections": ["email", "auth0_nomis"], - "disable_authentication": True, - "auth0_nomis_auth0_client_id": "nomis-client-id", - "auth0_nomis_auth0_client_secret": "", - "auth0_nomis_auth0_conn_name": "nomis_conn_name", - }, - all_connections_names=["github", "email", "auth0_nomis"], - custom_connections=["auth0_nomis"], - ) - f.request = mock.MagicMock() - mock_get_repo = mock.MagicMock(return_value=True) - mock_app = mock.MagicMock() - mock_app.objects.filter().exists.return_value = False - mock_s3 = mock.MagicMock() - mock_s3.get.side_effect = S3Bucket.DoesNotExist("Boom") - with mock.patch( - "controlpanel.frontend.forms.GithubAPI.get_repository", mock_get_repo - ), mock.patch("controlpanel.frontend.forms.App", mock_app), mock.patch( - "controlpanel.frontend.forms.S3Bucket.objects", mock_s3 - ): - assert f.is_valid() is False - assert "auth0_nomis_auth0_client_secret" in f.errors - assert "auth0_nomis_auth0_conn_name" in f.errors + assert f.errors["repo_url"][0] == "App already exists for this repository URL" def test_update_app_with_custom_connection(): diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py index 4e22a028c..f1ccc6dc3 100644 --- a/tests/frontend/views/test_app.py +++ b/tests/frontend/views/test_app.py @@ -15,7 +15,7 @@ from controlpanel.api import auth0, cluster from controlpanel.api.models import App, AppIPAllowList, S3Bucket from controlpanel.api.models.app import DeleteCustomerError -from tests.api.fixtures.aws import * # noqa +from tests.api.fixtures.aws import * NUM_APPS = 3 From 37481515b7fe61c53797520feb8bf7793887f7c0 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Wed, 10 Jan 2024 16:03:59 +0000 Subject: [PATCH 04/12] Update validation on repostiory URL --- controlpanel/api/github.py | 14 ++++++++++++-- controlpanel/api/validators.py | 11 ++++++++++- controlpanel/frontend/forms.py | 16 ++++++++++------ tests/api/test_validators.py | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 9 deletions(-) diff --git a/controlpanel/api/github.py b/controlpanel/api/github.py index 93c60d518..455c04e49 100644 --- a/controlpanel/api/github.py +++ b/controlpanel/api/github.py @@ -11,7 +11,6 @@ # First-party/Local from controlpanel.utils import encrypt_data_by_using_public_key - log = structlog.getLogger(__name__) @@ -19,6 +18,10 @@ class GithubAPIException(Exception): pass +class RepositoryNotFound(GithubAPIException): + status_code = 404 + + def extract_repo_info_from_url(repo_url): url_parts = repo_url.split("/") if len(url_parts) < 4: @@ -62,6 +65,11 @@ def get_repository(self, repo_name: str): self._get_repo_api_url(repo_name=repo_name, api_call=None), headers=self.headers, ) + if response.status_code == 404: + raise RepositoryNotFound( + f"Repository '{repo_name}' not found, it may be private" + ) + return self._process_response(response) def read_app_deploy_info(self, repo_name: str, deploy_file="deploy.json"): @@ -156,7 +164,9 @@ def create_or_update_repo_env_secret( if not public_key: public_key = self.get_repo_env_public_key(repo_name, env_name) secret_data = { - "encrypted_value": encrypt_data_by_using_public_key(public_key["key"], secret_value), + "encrypted_value": encrypt_data_by_using_public_key( + public_key["key"], secret_value + ), "key_id": public_key["key_id"], } repo_secret_url = self._get_repo_env_api_url( diff --git a/controlpanel/api/validators.py b/controlpanel/api/validators.py index f434238a4..fd5ef6d08 100644 --- a/controlpanel/api/validators.py +++ b/controlpanel/api/validators.py @@ -74,9 +74,18 @@ def validate_github_repository_url(value): if not value.startswith(github_base_url): raise ValidationError("Must be a Github hosted repository") + if value[-1] == "/": + raise ValidationError("Repository URL should not include a trailing slash") + repo_name = value[len(github_base_url) :] # noqa: E203 - org, _ = repo_name.split("/", 1) + repo_parts = list(filter(None, repo_name.split("/"))) + if len(repo_parts) > 2: + raise ValidationError("Repository URL should not include subdirectories") + + if len(repo_parts) < 2: + raise ValidationError("Repository URL is missing the repository name") + org = repo_parts[0] if org not in settings.GITHUB_ORGS: orgs = ", ".join(settings.GITHUB_ORGS) raise ValidationError( diff --git a/controlpanel/frontend/forms.py b/controlpanel/frontend/forms.py index a3873cfc4..7762f4fd8 100644 --- a/controlpanel/frontend/forms.py +++ b/controlpanel/frontend/forms.py @@ -3,7 +3,6 @@ # Third-party from django import forms -from django.conf import settings from django.contrib.postgres.forms import SimpleArrayField from django.core.exceptions import ValidationError from django.core.validators import RegexValidator, validate_email @@ -12,7 +11,11 @@ from controlpanel.api import validators from controlpanel.api.cluster import AWSRoleCategory from controlpanel.api.cluster import S3Folder as ClusterS3Folder -from controlpanel.api.github import GithubAPI, extract_repo_info_from_url +from controlpanel.api.github import ( + GithubAPI, + RepositoryNotFound, + extract_repo_info_from_url, +) from controlpanel.api.models import App, S3Bucket, Tool, User from controlpanel.api.models.access_to_s3bucket import S3BUCKET_PATH_REGEX from controlpanel.api.models.iam_managed_policy import POLICY_NAME_REGEX @@ -174,10 +177,11 @@ def clean(self): def clean_repo_url(self): repo_url = self.cleaned_data["repo_url"] org_name, repo_name = extract_repo_info_from_url(repo_url) - repo = GithubAPI( - self.request.user.github_api_token, github_org=org_name - ).get_repository(repo_name) - if repo is None: + try: + GithubAPI( + self.request.user.github_api_token, github_org=org_name + ).get_repository(repo_name) + except RepositoryNotFound: raise ValidationError( "Github repository not found - it may be private", ) diff --git a/tests/api/test_validators.py b/tests/api/test_validators.py index d93f92a88..fec17e4f3 100644 --- a/tests/api/test_validators.py +++ b/tests/api/test_validators.py @@ -74,3 +74,35 @@ def test_validate_auth0_conn_name_fail2(auth0_client_id): ) def test_validate_auth0_conn_name_pass2(auth0_client_id): validators.validate_auth0_client_id(auth0_client_id) + + +@pytest.mark.parametrize( + "url, error", + [ + ("https://gitlab.com/org/repo", "Must be a Github hosted repository"), + ( + "https://github.com/moj-analytical-services/repo", + "Unknown Github organization", + ), + ( + "https://github.com/ministryofjustice/repo/", + "Repository URL should not include a trailing slash", + ), + ( + "https://github.com/ministryofjustice/repo/subdir", + "Repository URL should not include subdirectories", + ), + ( + "https://github.com/ministryofjustice/", + "Repository URL is missing the repository name", + ), + ("https://github.com/ministryofjustice/repo", None), + ], +) +def test_validate_github_repository_url(url, error): + if not error: + assert validators.validate_github_repository_url(url) is None + else: + with pytest.raises(ValidationError) as exc: + validators.validate_github_repository_url(url) + assert exc.value.args[0] == error From c4fe48dd77a7fe13bb721b4cd6149b5fa3b523a3 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Wed, 10 Jan 2024 16:53:01 +0000 Subject: [PATCH 05/12] Add additional test case for checking repo when creating app --- tests/frontend/test_forms.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/tests/frontend/test_forms.py b/tests/frontend/test_forms.py index 6a95abb88..4a10f523d 100644 --- a/tests/frontend/test_forms.py +++ b/tests/frontend/test_forms.py @@ -7,6 +7,7 @@ # First-party/Local from controlpanel.api import aws +from controlpanel.api.github import RepositoryNotFound from controlpanel.api.models import S3Bucket from controlpanel.frontend import forms @@ -269,18 +270,17 @@ def test_create_app_form_clean_repo_url(): ): assert f.is_valid() is True - # Repo not found. + # App already exists. f = forms.CreateAppForm( data={ - "repo_url": "https://github.com/moj-analytical-services/my_repo", + "repo_url": "https://github.com/ministryofjustice/my_repo", "connect_bucket": "new", "new_datasource_name": "test-bucketname", } ) f.request = mock.MagicMock() - mock_get_repo = mock.MagicMock(return_value=None) mock_app = mock.MagicMock() - mock_app.objects.filter().exists.return_value = False + mock_app.objects.filter().exists.return_value = True mock_s3 = mock.MagicMock() mock_s3.get.side_effect = S3Bucket.DoesNotExist("Boom") with mock.patch( @@ -289,29 +289,28 @@ def test_create_app_form_clean_repo_url(): "controlpanel.frontend.forms.S3Bucket.objects", mock_s3 ): assert f.is_valid() is False - assert "Unknown Github organization" in f.errors["repo_url"][0] + assert f.errors["repo_url"][0] == "App already exists for this repository URL" - # App already exists. + # Repo in correct org but not found f = forms.CreateAppForm( data={ - "repo_url": "https://github.com/ministryofjustice/my_repo", + "repo_url": "https://github.com/ministryofjustice/doesnt-exist", "connect_bucket": "new", "new_datasource_name": "test-bucketname", } ) f.request = mock.MagicMock() - mock_get_repo = mock.MagicMock(return_value=True) mock_app = mock.MagicMock() - mock_app.objects.filter().exists.return_value = True - mock_s3 = mock.MagicMock() - mock_s3.get.side_effect = S3Bucket.DoesNotExist("Boom") + mock_app.objects.filter().exists.return_value = False with mock.patch( - "controlpanel.frontend.forms.GithubAPI.get_repository", mock_get_repo + "controlpanel.frontend.forms.GithubAPI.get_repository", + side_effect=RepositoryNotFound ), mock.patch("controlpanel.frontend.forms.App", mock_app), mock.patch( "controlpanel.frontend.forms.S3Bucket.objects", mock_s3 ): assert f.is_valid() is False - assert f.errors["repo_url"][0] == "App already exists for this repository URL" + error = f.errors["repo_url"][0] + assert "Github repository not found - it may be private" in error def test_update_app_with_custom_connection(): From ea40b1437ec07e1e75697803e49cbb520f52a7d5 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Fri, 12 Jan 2024 16:28:29 +0000 Subject: [PATCH 06/12] Update app assume role policy Adds statement to allow cloud platform to assume the app IAM role --- controlpanel/api/cluster.py | 23 +++++++++++++- .../assume_roles/cloud_platform_oidc.json | 17 ++++++++++ controlpanel/settings/common.py | 3 ++ controlpanel/settings/test.py | 2 ++ tests/api/cluster/test_app.py | 31 +++++++++++++++++-- 5 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 controlpanel/api/templates/assume_roles/cloud_platform_oidc.json diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py index e13ba3c88..5d5c92ce1 100644 --- a/controlpanel/api/cluster.py +++ b/controlpanel/api/cluster.py @@ -1,4 +1,5 @@ # Standard library +import json import os import secrets from copy import deepcopy @@ -9,6 +10,7 @@ import structlog from django.conf import settings from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist +from django.template.loader import render_to_string # First-party/Local from controlpanel.api import auth0, helm @@ -483,8 +485,27 @@ def _add_missing_mandatory_vars(self, env_name, app_env_vars, created_var_names) def iam_role_name(self): return f"{settings.ENV}_app_{self.app.slug}" + @property + def oidc_provider_statement(self): + """ + Builds the assume role statement for the OIDC provider, currently Cloud Platform + """ + statement = render_to_string( + template_name="assume_roles/cloud_platform_oidc.json", + context={ + "identity_provider": settings.OIDC_APP_EKS_PROVIDER, + "identity_provider_arn": iam_arn( + f"oidc-provider/{settings.OIDC_APP_EKS_PROVIDER}" + ), + "app_name": self.app.slug, + } + ) + return json.loads(statement) + def create_iam_role(self): - self.aws_role_service.create_role(self.iam_role_name, BASE_ASSUME_ROLE_POLICY) + assume_role_policy = BASE_ASSUME_ROLE_POLICY.copy() + assume_role_policy.update(self.oidc_provider_statement) + self.aws_role_service.create_role(self.iam_role_name, assume_role_policy) def grant_bucket_access(self, bucket_arn, access_level, path_arns): self.aws_role_service.grant_bucket_access( diff --git a/controlpanel/api/templates/assume_roles/cloud_platform_oidc.json b/controlpanel/api/templates/assume_roles/cloud_platform_oidc.json new file mode 100644 index 000000000..9c13e16ba --- /dev/null +++ b/controlpanel/api/templates/assume_roles/cloud_platform_oidc.json @@ -0,0 +1,17 @@ +{ + "Sid": "AllowCloudPlatformOIDCProvider", + "Effect": "Allow", + "Principal": { + "Federated": "{{ identity_provider_arn }}" + }, + "Action": "sts:AssumeRoleWithWebIdentity", + "Condition": { + "StringEquals": { + "{{ identity_provider }}:aud": "sts.amazonaws.com", + "{{ identity_provider }}:sub": [ + "system:serviceaccount:data-platform-app-{{ app_name }}-dev:data-platform-app-{{ app_name }}-dev-sa", + "system:serviceaccount:data-platform-app-{{ app_name }}-prod:data-platform-app-{{ app_name }}-prod-sa" + ] + } + } +} diff --git a/controlpanel/settings/common.py b/controlpanel/settings/common.py index bc57be88f..0a65b31c0 100644 --- a/controlpanel/settings/common.py +++ b/controlpanel/settings/common.py @@ -166,6 +166,9 @@ # Hostname of the OIDC provider OIDC_DOMAIN = os.environ.get("OIDC_DOMAIN") +# Provider for cloud platform +OIDC_APP_EKS_PROVIDER = os.environ.get("OIDC_APP_EKS_PROVIDER") + # OIDC endpoints OIDC_OP_AUTHORIZATION_ENDPOINT = os.environ.get("OIDC_OP_AUTHORIZATION_ENDPOINT") OIDC_OP_JWKS_ENDPOINT = os.environ.get("OIDC_OP_JWKS_ENDPOINT") diff --git a/controlpanel/settings/test.py b/controlpanel/settings/test.py index 4f260e330..1b2c18210 100644 --- a/controlpanel/settings/test.py +++ b/controlpanel/settings/test.py @@ -20,6 +20,8 @@ OIDC_ALLOW_UNSECURED_JWT = True OIDC_DOMAIN = "oidc.idp.example.com" +OIDC_APP_EKS_PROVIDER = "oidc-app-example" + TOOLS_DOMAIN = "example.com" CSRF_COOKIE_SECURE = False diff --git a/tests/api/cluster/test_app.py b/tests/api/cluster/test_app.py index 6be859f60..e8bc68903 100644 --- a/tests/api/cluster/test_app.py +++ b/tests/api/cluster/test_app.py @@ -50,9 +50,36 @@ def repos(githubapi): yield githubapi -def test_app_create_iam_role(aws_create_role, app): +@pytest.fixture +def oidc_provider_statement(app, settings): + statement = dict() + statement["Sid"] = "AllowCloudPlatformOIDCProvider" + statement["Effect"] = "Allow" + statement["Action"] = "sts:AssumeRoleWithWebIdentity" + statement["Principal"] = { + "Federated": f"arn:aws:iam::{settings.AWS_DATA_ACCOUNT_ID}:oidc-provider/{settings.OIDC_APP_EKS_PROVIDER}" # noqa + } + statement["Condition"] = { + "StringEquals": { + f"{settings.OIDC_APP_EKS_PROVIDER}:aud": "sts.amazonaws.com", + f"{settings.OIDC_APP_EKS_PROVIDER}:sub": [ + f"system:serviceaccount:data-platform-app-{app.slug}-dev:data-platform-app-{app.slug}-dev-sa", # noqa + f"system:serviceaccount:data-platform-app-{app.slug}-prod:data-platform-app-{app.slug}-prod-sa" # noqa + ] + } + } + return statement + + +def test_oidc_provider_statement(app, oidc_provider_statement): + assert cluster.App(app).oidc_provider_statement == oidc_provider_statement + + +def test_app_create_iam_role(aws_create_role, app, oidc_provider_statement): cluster.App(app).create_iam_role() - aws_create_role.assert_called_with(app.iam_role_name, BASE_ASSUME_ROLE_POLICY) + statement = BASE_ASSUME_ROLE_POLICY.copy() + statement.update(oidc_provider_statement) + aws_create_role.assert_called_with(app.iam_role_name, statement) @pytest.fixture # noqa: F405 From d88b1ab9f89a4df827a4e9ca241f5b06735049da Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Fri, 12 Jan 2024 16:34:44 +0000 Subject: [PATCH 07/12] Update test fixture --- tests/api/cluster/test_app.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/api/cluster/test_app.py b/tests/api/cluster/test_app.py index e8bc68903..af7f3f5af 100644 --- a/tests/api/cluster/test_app.py +++ b/tests/api/cluster/test_app.py @@ -11,7 +11,7 @@ @pytest.fixture def app(): - return models.App(slug="slug", repo_url="https://gitpub.example.com/test-repo") + return models.App(slug="test-app", repo_url="https://gitpub.example.com/test-repo") @pytest.fixture @@ -128,7 +128,7 @@ def test_update_auth_connections(app, ExtendedAuth0): key_name='AUTH0_PASSWORDLESS', key_value=False) update_conns.assert_called_with( - app_name='data-platform-app-slug-testing_env', + app_name=f'data-platform-app-{app.slug}-testing_env', client_id='testing_client_id', new_conns=new_conns, existing_conns='email') @@ -147,7 +147,7 @@ def test_update_auth_connections(app, ExtendedAuth0): key_name='AUTH0_PASSWORDLESS', key_value=True) update_conns.assert_called_with( - app_name='data-platform-app-slug-testing_env', + app_name=f'data-platform-app-{app.slug}-testing_env', client_id='testing_client_id', new_conns=new_conns, existing_conns='github') From 121baf41991358bd73e1ce065719310df3889d2a Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Fri, 12 Jan 2024 16:47:15 +0000 Subject: [PATCH 08/12] Fix building of the app assume role policy --- controlpanel/api/cluster.py | 2 +- tests/api/cluster/test_app.py | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py index 5d5c92ce1..2295167c5 100644 --- a/controlpanel/api/cluster.py +++ b/controlpanel/api/cluster.py @@ -504,7 +504,7 @@ def oidc_provider_statement(self): def create_iam_role(self): assume_role_policy = BASE_ASSUME_ROLE_POLICY.copy() - assume_role_policy.update(self.oidc_provider_statement) + assume_role_policy["Statement"].append(self.oidc_provider_statement) self.aws_role_service.create_role(self.iam_role_name, assume_role_policy) def grant_bucket_access(self, bucket_arn, access_level, path_arns): diff --git a/tests/api/cluster/test_app.py b/tests/api/cluster/test_app.py index af7f3f5af..6547661ce 100644 --- a/tests/api/cluster/test_app.py +++ b/tests/api/cluster/test_app.py @@ -76,10 +76,12 @@ def test_oidc_provider_statement(app, oidc_provider_statement): def test_app_create_iam_role(aws_create_role, app, oidc_provider_statement): + expected_assume_role = BASE_ASSUME_ROLE_POLICY.copy() + expected_assume_role["Statement"].append(oidc_provider_statement) + cluster.App(app).create_iam_role() - statement = BASE_ASSUME_ROLE_POLICY.copy() - statement.update(oidc_provider_statement) - aws_create_role.assert_called_with(app.iam_role_name, statement) + + aws_create_role.assert_called_with(app.iam_role_name, expected_assume_role) @pytest.fixture # noqa: F405 From 6524d7242bed11f6822556f4b3a2e8c3885106ec Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Fri, 12 Jan 2024 16:57:39 +0000 Subject: [PATCH 09/12] Update to use deepcopy --- controlpanel/api/cluster.py | 2 +- tests/api/cluster/test_app.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py index 2295167c5..23e5b5dda 100644 --- a/controlpanel/api/cluster.py +++ b/controlpanel/api/cluster.py @@ -503,7 +503,7 @@ def oidc_provider_statement(self): return json.loads(statement) def create_iam_role(self): - assume_role_policy = BASE_ASSUME_ROLE_POLICY.copy() + assume_role_policy = deepcopy(BASE_ASSUME_ROLE_POLICY) assume_role_policy["Statement"].append(self.oidc_provider_statement) self.aws_role_service.create_role(self.iam_role_name, assume_role_policy) diff --git a/tests/api/cluster/test_app.py b/tests/api/cluster/test_app.py index 6547661ce..bafa396f1 100644 --- a/tests/api/cluster/test_app.py +++ b/tests/api/cluster/test_app.py @@ -1,4 +1,5 @@ # Standard library +from copy import deepcopy from unittest.mock import MagicMock, patch # Third-party @@ -76,7 +77,7 @@ def test_oidc_provider_statement(app, oidc_provider_statement): def test_app_create_iam_role(aws_create_role, app, oidc_provider_statement): - expected_assume_role = BASE_ASSUME_ROLE_POLICY.copy() + expected_assume_role = deepcopy(BASE_ASSUME_ROLE_POLICY) expected_assume_role["Statement"].append(oidc_provider_statement) cluster.App(app).create_iam_role() From 69570b2e5c2846fae4a708c28457d272f5f484da Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Mon, 15 Jan 2024 11:12:35 +0000 Subject: [PATCH 10/12] Tidy up deployment envs --- controlpanel/api/models/app.py | 11 ++++++----- controlpanel/api/tasks/app.py | 1 - tests/api/models/test_app.py | 4 ---- tests/api/views/test_app.py | 4 ++-- 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/controlpanel/api/models/app.py b/controlpanel/api/models/app.py index 099860d0a..88359206f 100644 --- a/controlpanel/api/models/app.py +++ b/controlpanel/api/models/app.py @@ -232,9 +232,6 @@ def delete_customer_by_email(self, email, group_id): def status(self): return "Deployed" - def deployment_envs(self, github_token): - return cluster.App(self, github_token).get_deployment_envs() - def delete(self, *args, **kwargs): github_api_token = None if "github_api_token" in kwargs: @@ -319,8 +316,12 @@ class DeleteCustomerError(Exception): @receiver(post_save, sender=App) def trigger_app_create_related_messages(sender, instance, created, **kwargs): - if created: - tasks.AppCreateRole(instance, instance.current_user).create_task() + if not created: + return + tasks.AppCreateRole(instance, instance.current_user).create_task() + + # TODO this could be removed as part of a review of task queue usage + if instance.deployment_envs: tasks.AppCreateAuth(instance, instance.current_user, extra_data=dict( deployment_envs=instance.deployment_envs, disable_authentication=instance.disable_authentication, diff --git a/controlpanel/api/tasks/app.py b/controlpanel/api/tasks/app.py index ba399419d..edf91985f 100644 --- a/controlpanel/api/tasks/app.py +++ b/controlpanel/api/tasks/app.py @@ -33,7 +33,6 @@ def _get_args_list(self): self.extra_data.get('deployment_envs'), self.extra_data.get('disable_authentication'), self.extra_data.get('connections'), - # self.extra_data.get('has_ip_ranges') # NOT USED, REMOVE IT? ] @property diff --git a/tests/api/models/test_app.py b/tests/api/models/test_app.py index f479a1f56..3d81825ff 100644 --- a/tests/api/models/test_app.py +++ b/tests/api/models/test_app.py @@ -50,10 +50,6 @@ def test_create(sqs, helpers): helpers.validate_task_with_sqs_messages( iam_messages, App.__name__, app.id, queue_name=settings.IAM_QUEUE_NAME ) - auth_messages = helpers.retrieve_messages(sqs, queue_name=settings.AUTH_QUEUE_NAME) - helpers.validate_task_with_sqs_messages( - auth_messages, App.__name__, app.id, queue_name=settings.AUTH_QUEUE_NAME - ) @pytest.mark.django_db diff --git a/tests/api/views/test_app.py b/tests/api/views/test_app.py index bb6a50432..aaacdbf94 100644 --- a/tests/api/views/test_app.py +++ b/tests/api/views/test_app.py @@ -173,8 +173,8 @@ def test_create(client, users, sqs, helpers): assert response.data["repo_url"] == "https://example.com/bar" app = App.objects.get(repo_url="https://example.com/bar") - messages = helpers.retrieve_messages(sqs) - helpers.validate_task_with_sqs_messages(messages, App.__name__, app.id) + messages = helpers.retrieve_messages(sqs, queue_name=settings.IAM_QUEUE_NAME) + helpers.validate_task_with_sqs_messages(messages, App.__name__, app.id, queue_name=settings.IAM_QUEUE_NAME) def test_update(client, app): From a7766e622298ab0343356187a7726cf6af56634b Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Mon, 15 Jan 2024 12:51:27 +0000 Subject: [PATCH 11/12] Make the app registration form accessible App registration form should be open to logged in users, to help enable self service of app registration. Updating connections remains superuser only. --- controlpanel/api/rules.py | 4 +++- controlpanel/api/serializers.py | 2 +- controlpanel/frontend/views/app.py | 2 +- tests/api/permissions/test_app_permissions.py | 4 ++-- tests/frontend/views/test_app.py | 4 ++-- 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/controlpanel/api/rules.py b/controlpanel/api/rules.py index 058a3f7bb..939d5f31a 100644 --- a/controlpanel/api/rules.py +++ b/controlpanel/api/rules.py @@ -48,7 +48,7 @@ def is_app_admin(user, obj): add_perm("api.list_app", is_authenticated) -add_perm("api.create_app", is_authenticated & is_superuser) +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.destroy_app", is_authenticated & is_superuser) @@ -187,6 +187,8 @@ def is_self(user, other): add_perm("api.update_ip_allowlists", is_authenticated & is_superuser) add_perm("api.destroy_ip_allowlists", is_authenticated & is_superuser) +add_perm("api.create_connections", is_authenticated & is_superuser) + @predicate def is_owner(user, obj): diff --git a/controlpanel/api/serializers.py b/controlpanel/api/serializers.py index 7761379c4..e0b70c56c 100644 --- a/controlpanel/api/serializers.py +++ b/controlpanel/api/serializers.py @@ -362,7 +362,7 @@ class AppAuthSettingsSerializer(serializers.BaseSerializer): "edit_link": "update-app-ip-allowlists" }, cluster.App.AUTH0_CONNECTIONS: { - "permission_flag": "api.create_app", + "permission_flag": "api.create_connections", "edit_link": "update-auth0-connections" } } diff --git a/controlpanel/frontend/views/app.py b/controlpanel/frontend/views/app.py index 821a0138d..02750ec3d 100644 --- a/controlpanel/frontend/views/app.py +++ b/controlpanel/frontend/views/app.py @@ -170,7 +170,7 @@ class UpdateAppAuth0Connections( form_class = UpdateAppAuth0ConnectionsForm model = App - permission_required = "api.create_app" + permission_required = "api.create_connections" template_name = "webapp-auth0-connections-update.html" success_url = "manage-app" diff --git a/tests/api/permissions/test_app_permissions.py b/tests/api/permissions/test_app_permissions.py index 0c14960d5..047faefed 100644 --- a/tests/api/permissions/test_app_permissions.py +++ b/tests/api/permissions/test_app_permissions.py @@ -98,13 +98,13 @@ def test_authenticated_user_has_basic_perms(client, users): (app_detail, "app_user", status.HTTP_403_FORBIDDEN), (app_detail, "normal_user", status.HTTP_404_NOT_FOUND), (app_delete, "normal_user", status.HTTP_403_FORBIDDEN), - (app_create, "normal_user", status.HTTP_403_FORBIDDEN), + (app_create, "normal_user", status.HTTP_201_CREATED), (app_update, "normal_user", status.HTTP_403_FORBIDDEN), (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_403_FORBIDDEN), + (app_create, "app_admin", status.HTTP_201_CREATED), (app_update, "app_admin", status.HTTP_403_FORBIDDEN), ], ) diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py index f1ccc6dc3..9033f96d8 100644 --- a/tests/frontend/views/test_app.py +++ b/tests/frontend/views/test_app.py @@ -282,8 +282,8 @@ def update_ip_allowlists(client, app, *args): (update_auth0_connections, "app_admin", status.HTTP_403_FORBIDDEN), (update_auth0_connections, "normal_user", status.HTTP_403_FORBIDDEN), (create, "superuser", status.HTTP_200_OK), - (create, "app_admin", status.HTTP_403_FORBIDDEN), - (create, "normal_user", status.HTTP_403_FORBIDDEN), + (create, "app_admin", status.HTTP_200_OK), + (create, "normal_user", status.HTTP_200_OK), (delete, "superuser", status.HTTP_302_FOUND), (delete, "app_admin", status.HTTP_403_FORBIDDEN), (delete, "normal_user", status.HTTP_403_FORBIDDEN), From 91411e3d961938fbf37a3a3979dc1c0e25a2b728 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Mon, 15 Jan 2024 15:15:57 +0000 Subject: [PATCH 12/12] Check repo url on app registration API endpoint --- controlpanel/api/serializers.py | 6 ++++ tests/api/permissions/test_app_permissions.py | 4 +-- tests/api/views/test_app.py | 30 +++++++++++++++---- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/controlpanel/api/serializers.py b/controlpanel/api/serializers.py index e0b70c56c..624f5f2e2 100644 --- a/controlpanel/api/serializers.py +++ b/controlpanel/api/serializers.py @@ -5,6 +5,7 @@ # Third-party from django.conf import settings +from django.core.exceptions import ValidationError from rest_framework import serializers # First-party/Local @@ -18,6 +19,7 @@ UserApp, UserS3Bucket, ) +from controlpanel.api import validators class AppS3BucketSerializer(serializers.ModelSerializer): @@ -177,6 +179,10 @@ class Meta: def validate_repo_url(self, value): """Normalise repo URLs by removing trailing .git""" + try: + validators.validate_github_repository_url(value) + except ValidationError as e: + raise serializers.ValidationError(e.message) return value.rsplit(".git", 1)[0] diff --git a/tests/api/permissions/test_app_permissions.py b/tests/api/permissions/test_app_permissions.py index 047faefed..fc378d9af 100644 --- a/tests/api/permissions/test_app_permissions.py +++ b/tests/api/permissions/test_app_permissions.py @@ -61,12 +61,12 @@ def app_delete(client, app, *args): def app_create(client, *args): - data = {"name": "test-app", "repo_url": "https://example.com"} + data = {"name": "test-app", "repo_url": "https://github.com/ministryofjustice/example"} return client.post(reverse("app-list"), data) def app_update(client, app, *args): - data = {"name": "test-app", "repo_url": "https://example.com"} + data = {"name": "test-app", "repo_url": "https://github.com/ministryofjustice/example"} return client.put( reverse("app-detail", (app.res_id,)), json.dumps(data), diff --git a/tests/api/views/test_app.py b/tests/api/views/test_app.py index aaacdbf94..7aaa6da7b 100644 --- a/tests/api/views/test_app.py +++ b/tests/api/views/test_app.py @@ -5,10 +5,12 @@ from botocore.exceptions import ClientError from model_mommy import mommy from rest_framework import status +from rest_framework.exceptions import ValidationError from rest_framework.reverse import reverse # First-party/Local from controlpanel.api.models import App +from controlpanel.api.serializers import AppSerializer from tests.api.fixtures.aws import * @@ -16,7 +18,7 @@ def app(): return mommy.make( "api.App", - repo_url="https://example.com/foo.git", + repo_url="https://github.com/ministryofjustice/example.git", ) @@ -165,20 +167,20 @@ def test_delete(client, app, authz): def test_create(client, users, sqs, helpers): - data = {"name": "bar", "repo_url": "https://example.com/bar.git"} + data = {"name": "bar", "repo_url": "https://github.com/ministryofjustice/new-example.git"} response = client.post(reverse("app-list"), data) assert response.status_code == status.HTTP_201_CREATED assert response.data["created_by"] == users["superuser"].auth0_id - assert response.data["repo_url"] == "https://example.com/bar" + assert response.data["repo_url"] == "https://github.com/ministryofjustice/new-example" - app = App.objects.get(repo_url="https://example.com/bar") + app = App.objects.get(repo_url="https://github.com/ministryofjustice/new-example") messages = helpers.retrieve_messages(sqs, queue_name=settings.IAM_QUEUE_NAME) helpers.validate_task_with_sqs_messages(messages, App.__name__, app.id, queue_name=settings.IAM_QUEUE_NAME) def test_update(client, app): - data = {"name": "foo", "repo_url": "http://foo.com.git"} + data = {"name": "foo", "repo_url": "https://github.com/ministryofjustice/new.git"} response = client.put( reverse("app-detail", (app.res_id,)), data, @@ -186,7 +188,7 @@ def test_update(client, app): ) assert response.status_code == status.HTTP_200_OK assert response.data["name"] == data["name"] - assert response.data["repo_url"] == "http://foo.com" + assert response.data["repo_url"] == "https://github.com/ministryofjustice/new" @pytest.mark.skip( @@ -202,3 +204,19 @@ def test_aws_error_and_transaction(client): with pytest.raises(App.DoesNotExist): # noqa: F405 App.objects.get(name=data["name"]) + + +@pytest.mark.parametrize("url, valid", [ + ("https://example.com/repo", False), + ("https://github.com/someorg/repo", False), + ("https://github.com/ministryofjustice", False), + ("http://github.com/ministryofjustice/nothttps", False), + ("https://github.com/ministryofjustice/success", True), +]) +def test_validate_repo_url(url, valid): + serializer = AppSerializer() + if valid: + assert serializer.validate_repo_url(url) == url + else: + with pytest.raises(ValidationError): + serializer.validate_repo_url(url)