From a956062558421e74ccbcd688061ebdfc37ae0a86 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Thu, 15 Feb 2024 15:47:08 +0000 Subject: [PATCH] Bugfix/app registration (#1250) * Update auth settings, and add regression test formating the github key name * Refactor format_github_key_name to simplify logic * Regression test for get_github_key_display_name * Simplify logic for get_github_key_display_name * Remove DATA_ACCOUNT_ID from created github secrets This secret is no longer needed when registering an app, as the base images are no longer stored in ECR. Instead they are made public in Docker Hub * Auto-prefix new app datasource names with the env * Hide manage customers button until auth0 client exists * Redirect to manage app page after registering an app --- controlpanel/api/cluster.py | 29 ++++++++++--------- controlpanel/api/models/app.py | 6 ++++ .../frontend/jinja2/includes/app-list.html | 10 ++++--- .../frontend/jinja2/webapp-create.html | 7 ++++- controlpanel/frontend/views/app.py | 2 +- controlpanel/frontend/views/app_variables.py | 2 +- controlpanel/frontend/views/secrets.py | 2 +- settings.yaml | 2 ++ tests/api/cluster/test_app.py | 26 ++++++++++++++++- tests/api/models/test_app.py | 14 +++++++++ tests/frontend/views/test_app.py | 6 +++- 11 files changed, 82 insertions(+), 24 deletions(-) diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py index bcc068801..eb66629df 100644 --- a/controlpanel/api/cluster.py +++ b/controlpanel/api/cluster.py @@ -389,7 +389,6 @@ class App(EntityResource): AUTHENTICATION_REQUIRED = "AUTHENTICATION_REQUIRED" AUTH0_PASSWORDLESS = "AUTH0_PASSWORDLESS" APP_ROLE_ARN = "APP_ROLE_ARN" - DATA_ACCOUNT_ID = 'DATA_ACCOUNT_ID' def __init__(self, app, github_api_token=None, auth0_instance=None): super(App, self).__init__() @@ -415,7 +414,6 @@ def _create_secrets(self, env_name, client=None): secret_data: dict = { App.IP_RANGES: self.app.env_allowed_ip_ranges(env_name=env_name), App.APP_ROLE_ARN: self.app.iam_role_arn, - App.DATA_ACCOUNT_ID: settings.AWS_DATA_ACCOUNT_ID } if client: secret_data[App.AUTH0_CLIENT_ID] = client["client_id"] @@ -534,24 +532,27 @@ def format_github_key_name(key_name): Format the self-defined secret/variable by adding prefix if create/update value back to github and there is no prefix in the name """ - if key_name not in settings.AUTH_SETTINGS_ENVS \ - and key_name not in settings.AUTH_SETTINGS_SECRETS: - if not key_name.startswith(settings.APP_SELF_DEFINE_SETTING_PREFIX): - return f"{settings.APP_SELF_DEFINE_SETTING_PREFIX}{key_name}" - return key_name + if key_name in settings.AUTH_SETTINGS_ENVS: + return key_name + + if key_name in settings.AUTH_SETTINGS_SECRETS: + return key_name + + if key_name.startswith(settings.APP_SELF_DEFINE_SETTING_PREFIX): + return key_name + + return f"{settings.APP_SELF_DEFINE_SETTING_PREFIX}{key_name}" @staticmethod - def get_github_key_display_name(key_name): + def get_github_key_display_name(key_name: str) -> str: """ Format the self-defined secret/variable by removing the prefix if reading it from github and there is prefix in the name """ - if key_name and key_name not in settings.AUTH_SETTINGS_ENVS \ - and key_name not in settings.AUTH_SETTINGS_SECRETS: - if settings.APP_SELF_DEFINE_SETTING_PREFIX in key_name: - return key_name.replace( - settings.APP_SELF_DEFINE_SETTING_PREFIX, "") - return key_name + if not key_name.startswith(settings.APP_SELF_DEFINE_SETTING_PREFIX): + return key_name + + return key_name.replace(settings.APP_SELF_DEFINE_SETTING_PREFIX, "", 1) def create_or_update_secret(self, env_name, secret_key, secret_value): org_name, repo_name = extract_repo_info_from_url(self.app.repo_url) diff --git a/controlpanel/api/models/app.py b/controlpanel/api/models/app.py index 288d53811..07533261d 100644 --- a/controlpanel/api/models/app.py +++ b/controlpanel/api/models/app.py @@ -83,6 +83,12 @@ def release_name(self): def iam_role_arn(self): return cluster.iam_arn(f"role/{self.iam_role_name}") + @property + def can_manage_customers(self): + if not self.app_conf: + return False + return bool(self.app_conf.get(self.KEY_WORD_FOR_AUTH_SETTINGS)) + def get_group_id(self, env_name): return self.get_auth_client(env_name).get("group_id") diff --git a/controlpanel/frontend/jinja2/includes/app-list.html b/controlpanel/frontend/jinja2/includes/app-list.html index f5338ec20..64d6d45d3 100644 --- a/controlpanel/frontend/jinja2/includes/app-list.html +++ b/controlpanel/frontend/jinja2/includes/app-list.html @@ -36,10 +36,12 @@ {{ yes_no(user.is_app_admin(app.id)) }} - - Manage customers - + {% if app.can_manage_customers %} + + Manage customers + + {% endif %} diff --git a/controlpanel/frontend/jinja2/webapp-create.html b/controlpanel/frontend/jinja2/webapp-create.html index a066eda94..30b62d58b 100644 --- a/controlpanel/frontend/jinja2/webapp-create.html +++ b/controlpanel/frontend/jinja2/webapp-create.html @@ -23,7 +23,12 @@ "text": '60 chars max, only lowercase letters, numbers, periods and hyphens, auto-prefixed with "' + env + '"' }, "errorMessage": {"text": form.new_datasource_name.errors|join(". ")} if form.new_datasource_name.errors else {}, - "value": form.new_datasource_name.value() + "value": form.new_datasource_name.value(), + "attributes": { + "data-bucket-prefix": env + "-", + "pattern": "[a-z0-9.-]{1,60}", + "maxlength": "60", + } }) }} {% endset %} diff --git a/controlpanel/frontend/views/app.py b/controlpanel/frontend/views/app.py index 34324bbcc..7d252f0f7 100644 --- a/controlpanel/frontend/views/app.py +++ b/controlpanel/frontend/views/app.py @@ -151,7 +151,7 @@ def get_success_url(self): self.request, f"Successfully registered {self.object.name} webapp", ) - return reverse_lazy("list-apps") + return reverse_lazy("manage-app", kwargs={"pk": self.object.pk}) def form_valid(self, form): try: diff --git a/controlpanel/frontend/views/app_variables.py b/controlpanel/frontend/views/app_variables.py index bc373fda9..b946c9ae6 100644 --- a/controlpanel/frontend/views/app_variables.py +++ b/controlpanel/frontend/views/app_variables.py @@ -37,7 +37,7 @@ def get_form_kwargs(self): kwargs["initial"]["env_name"] = data.get("env_name") kwargs["initial"]["key"] = self.kwargs.get("var_name") kwargs["initial"]["display_key"] = cluster.App.get_github_key_display_name( - self.kwargs.get("var_name")) + self.kwargs.get("var_name", "")) if kwargs["initial"]["key"]: try: var_info = cluster.App( diff --git a/controlpanel/frontend/views/secrets.py b/controlpanel/frontend/views/secrets.py index b2967e6e7..b9c01df68 100644 --- a/controlpanel/frontend/views/secrets.py +++ b/controlpanel/frontend/views/secrets.py @@ -40,7 +40,7 @@ def get_form_kwargs(self): kwargs["initial"]["env_name"] = data.get("env_name") kwargs["initial"]["key"] = self.kwargs.get("secret_name") kwargs["initial"]["display_key"] = cluster.App.get_github_key_display_name( - self.kwargs.get("secret_name")) + self.kwargs.get("secret_name", "")) return kwargs def get_success_url(self, app_id): diff --git a/settings.yaml b/settings.yaml index aaa9713fa..e72b5f680 100644 --- a/settings.yaml +++ b/settings.yaml @@ -95,12 +95,14 @@ AUTH_SETTINGS_SECRETS: - AUTH0_CLIENT_ID - AUTH0_CLIENT_SECRET - IP_RANGES + - APP_ROLE_ARN AUTH_SETTINGS_NO_EDIT: - AUTH0_CLIENT_ID - AUTH0_CLIENT_SECRET - AUTH0_DOMAIN - AUTH0_PASSWORDLESS + - APP_ROLE_ARN AUTH_SETTINGS_ENVS: - AUTH0_DOMAIN diff --git a/tests/api/cluster/test_app.py b/tests/api/cluster/test_app.py index 7530b755d..4b082bc24 100644 --- a/tests/api/cluster/test_app.py +++ b/tests/api/cluster/test_app.py @@ -163,7 +163,6 @@ def test_create_secrets(app): secrets = { app_cluster.IP_RANGES: "1.2.3", app_cluster.APP_ROLE_ARN: app.iam_role_arn, - app_cluster.DATA_ACCOUNT_ID: settings.AWS_DATA_ACCOUNT_ID } with patch.object(app_cluster, "create_or_update_secrets"): app_cluster._create_secrets(env_name="dev", client=None) @@ -173,6 +172,31 @@ def test_create_secrets(app): ) +@pytest.mark.parametrize("key, expected", [ + ("AUTH0_CLIENT_ID", "AUTH0_CLIENT_ID"), + ("AUTH0_CLIENT_SECRET", "AUTH0_CLIENT_SECRET"), + ("AUTH0_DOMAIN", "AUTH0_DOMAIN"), + ("AUTH0_PASSWORDLESS", "AUTH0_PASSWORDLESS"), + ("APP_ROLE_ARN", "APP_ROLE_ARN"), + ("CUSTOM_SETTING", "XXX_CUSTOM_SETTING"), +]) +def test_format_github_key_name(key, expected): + assert cluster.App(None).format_github_key_name(key_name=key) == expected + + +@pytest.mark.parametrize("key, expected", [ + ("AUTH0_CLIENT_ID", "AUTH0_CLIENT_ID"), + ("AUTH0_CLIENT_SECRET", "AUTH0_CLIENT_SECRET"), + ("AUTH0_DOMAIN", "AUTH0_DOMAIN"), + ("AUTH0_PASSWORDLESS", "AUTH0_PASSWORDLESS"), + ("APP_ROLE_ARN", "APP_ROLE_ARN"), + ("XXX_CUSTOM_SETTING", "CUSTOM_SETTING"), + ("XXX_XXX_CUSTOM_SETTING", "XXX_CUSTOM_SETTING"), +]) +def test_get_github_key_display_name(key, expected): + assert cluster.App(None).get_github_key_display_name(key) == expected + + # TODO can this be removed? mock_ingress = MagicMock(name="Ingress") mock_ingress.spec.rules = [MagicMock(name="Rule", host="test-app.example.com")] diff --git a/tests/api/models/test_app.py b/tests/api/models/test_app.py index c9132c5b0..453f06322 100644 --- a/tests/api/models/test_app.py +++ b/tests/api/models/test_app.py @@ -209,3 +209,17 @@ def test_app_allowed_ip_ranges(): def test_iam_role_arn(): app = App(slug="example-app") assert app.iam_role_arn == f"arn:aws:iam::{settings.AWS_DATA_ACCOUNT_ID}:role/test_app_example-app" + + +@pytest.mark.parametrize("conf, expected", [ + (None, False), + ({}, False), + ({"some_setting_key": []}, False), + ({App.KEY_WORD_FOR_AUTH_SETTINGS: None}, False), + ({App.KEY_WORD_FOR_AUTH_SETTINGS: {}}, False), + ({App.KEY_WORD_FOR_AUTH_SETTINGS: {"dev": {}}}, True), +]) +def test_can_manage_customers(conf, expected): + app = App() + app.app_conf = conf + assert app.can_manage_customers is expected diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py index d3697c02d..041a7f2fb 100644 --- a/tests/frontend/views/test_app.py +++ b/tests/frontend/views/test_app.py @@ -721,6 +721,9 @@ def test_register_app_with_creating_datasource(client, users): related_bucket_ids = [a.s3bucket_id for a in created_app.apps3buckets.all()] assert len(related_bucket_ids) == 1 assert bucket.id in related_bucket_ids + assert response.url == reverse( + "manage-app", kwargs={"pk": created_app.pk} + ) def test_register_app_invalid_organisation(client, users): @@ -731,7 +734,8 @@ def test_register_app_invalid_organisation(client, users): connect_bucket="later", ) - response = client.post(reverse("create-app"), data) + url = reverse("create-app") + response = client.post(url, data) # 200 due to errors assert response.status_code == 200