From 54371e954df3b1ea48ab605a52c17bfcb0899026 Mon Sep 17 00:00:00 2001 From: Andy Driver Date: Tue, 22 Oct 2019 14:52:00 +0100 Subject: [PATCH] Error handling (#766) * Explicitly catch and gracefully handle errors * Add logging and error handling for Github repo listing * Prevent creating App with already registered Github repo * Improve create app form validation and errors * Validate app customer email addresses * Handle Auth0 errors adding and removing customers * Set default choice on create app form * Code review amends --- controlpanel/api/cluster.py | 9 +- controlpanel/api/models/app.py | 35 ++++- controlpanel/api/validators.py | 30 +++- controlpanel/frontend/forms.py | 146 +++++++++++++----- .../frontend/jinja2/webapp-detail.html | 4 +- controlpanel/frontend/views/app.py | 49 ++++-- controlpanel/settings/common.py | 22 +-- requirements.lock | 4 +- requirements.txt | 2 +- tests/frontend/views/test_app.py | 78 +++++++++- 10 files changed, 292 insertions(+), 87 deletions(-) diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py index d081a2f42..907ed4aa1 100644 --- a/controlpanel/api/cluster.py +++ b/controlpanel/api/cluster.py @@ -184,8 +184,12 @@ def get_repositories(user): repos = [] github = Github(user.github_api_token) for name in settings.GITHUB_ORGS: - org = github.get_organization(name) - repos.extend(org.get_repos()) + try: + org = github.get_organization(name) + repos.extend(org.get_repos()) + except GithubException as err: + log.warning(f'Failed getting {name} Github org repos for {user}: {err}') + raise err return repos @@ -194,6 +198,7 @@ def get_repository(user, repo_name): try: return github.get_repo(repo_name) except GithubException.UnknownObjectException: + log.warning(f'Failed getting {repo_name} Github repo for {user}: {err}') return None diff --git a/controlpanel/api/models/app.py b/controlpanel/api/models/app.py index a0c6cbbd8..e4d07fc49 100644 --- a/controlpanel/api/models/app.py +++ b/controlpanel/api/models/app.py @@ -51,16 +51,23 @@ def customers(self): def add_customers(self, emails): emails = list(filter(None, emails)) if emails: - auth0.AuthorizationAPI().add_group_members( - group_name=self.slug, - emails=emails, - user_options={"connection": "email"}, - ) + try: + auth0.AuthorizationAPI().add_group_members( + group_name=self.slug, + emails=emails, + user_options={"connection": "email"}, + ) + except auth0.Auth0Error as e: + raise AddCustomerError from e def delete_customers(self, user_ids): - auth0.AuthorizationAPI().delete_group_members( - group_name=self.slug, user_ids=user_ids - ) + try: + auth0.AuthorizationAPI().delete_group_members( + group_name=self.slug, + user_ids=user_ids, + ) + except auth0.Auth0Error as e: + raise DeleteCustomerError from e @property def status(self): @@ -79,3 +86,15 @@ def save(self, *args, **kwargs): def delete(self, *args, **kwargs): cluster.App(self).delete_iam_role() super().delete(*args, **kwargs) + + +class AddCustomerError(Exception): + pass + + +class DeleteCustomerError(Exception): + pass + + +App.AddCustomerError = AddCustomerError +App.DeleteCustomerError = DeleteCustomerError diff --git a/controlpanel/api/validators.py b/controlpanel/api/validators.py index 201a243ac..5fa8235e7 100644 --- a/controlpanel/api/validators.py +++ b/controlpanel/api/validators.py @@ -5,12 +5,15 @@ def validate_env_prefix(value): """Validate the name has env-prefix, check current set ENV value""" - if not value.startswith("{}-".format(settings.ENV)): + if not value.startswith(f"{settings.ENV}-"): raise ValidationError("Name must have correct env prefix e.g. {}-bucketname".format(settings.ENV)) # An S3 bucket name needs to be min 3 chars, max 63 chars long. -validate_s3_bucket_length = RegexValidator(regex='^.{3,63}$', message="must be between 3 and 63 characters") +validate_s3_bucket_length = RegexValidator( + regex='^.{3,63}$', + message="must be between 3 and 63 characters", +) # See: AWS' Bucket Restrictions and Limitations # http://docs.aws.amazon.com/en_gb/AmazonS3/latest/dev/BucketRestrictions.html @@ -21,6 +24,23 @@ def validate_env_prefix(value): # # An S3 bucket name starts with a label, it can have more than one label # separated by a dot. -validate_s3_bucket_labels = RegexValidator(regex='^([a-z][a-z0-9-]*[a-z0-9])(.[a-z][a-z0-9-]*[a-z0-9])*$', - message="is invalid, check AWS S3 bucket names restrictions (for example, " - "can only contains letters, digits, dots and hyphens)") +validate_s3_bucket_labels = RegexValidator( + regex='^([a-z][a-z0-9-]*[a-z0-9])(.[a-z][a-z0-9-]*[a-z0-9])*$', + message="is invalid, check AWS S3 bucket names restrictions (for example, ""can only contains letters, digits, dots and hyphens)", +) + + +def validate_github_repository_url(value): + github_base_url = "https://github.com/" + + if not value.startswith(github_base_url): + raise ValidationError("Must be a Github hosted repository") + + repo_name = value[len(github_base_url):] + org, _ = repo_name.split("/", 1) + + if org not in settings.GITHUB_ORGS: + orgs = ", ".join(settings.GITHUB_ORGS) + raise ValidationError( + f"Unknown Github organization, must be one of {orgs}", + ) diff --git a/controlpanel/frontend/forms.py b/controlpanel/frontend/forms.py index 0b034f537..bc8213e3a 100644 --- a/controlpanel/frontend/forms.py +++ b/controlpanel/frontend/forms.py @@ -1,17 +1,22 @@ +import re + 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 +from django.core.validators import RegexValidator, validate_email from controlpanel.api import validators from controlpanel.api.cluster import get_repository -from controlpanel.api.models import S3Bucket +from controlpanel.api.models import App, S3Bucket 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.parameter import APP_TYPE_CHOICES +APP_CUSTOMERS_DELIMITERS = re.compile(r'[,; ]+') + + class DatasourceChoiceField(forms.ModelChoiceField): def __init__(self, *args, **kwargs): @@ -23,13 +28,23 @@ def label_from_instance(self, instance): class CreateAppForm(forms.Form): - repo_url = forms.CharField(max_length=512) - connect_bucket = forms.ChoiceField(choices=[ - ("new", "new"), - ("existing", "existing"), - ("later", "later"), - ]) + repo_url = forms.CharField( + max_length=512, + validators=[ + validators.validate_github_repository_url, + ], + ) + connect_bucket = forms.ChoiceField( + required=True, + initial="new", + choices=[ + ("new", "new"), + ("existing", "existing"), + ("later", "later"), + ], + ) new_datasource_name = forms.CharField( + max_length=63, validators=[ validators.validate_env_prefix, validators.validate_s3_bucket_labels, @@ -49,53 +64,50 @@ def __init__(self, *args, **kwargs): def clean(self): cleaned_data = super().clean() - - if cleaned_data['connect_bucket'] == "new" and not cleaned_data.get('new_datasource_name'): - self.add_error('new_datasource_name', "This field is required.") - - if cleaned_data['connect_bucket'] == "existing" and not cleaned_data.get('existing_datasource_id'): + connect = cleaned_data['connect_bucket'] + new_datasource = cleaned_data.get('new_datasource_name') + existing_datasource = cleaned_data.get('existing_datasource_id') + + if connect == "new": + if new_datasource: + try: + S3Bucket.objects.get(name=new_datasource) + self.add_error( + f"Datasource named {new_datasource} already exists" + ) + except S3Bucket.DoesNotExist: + pass + + else: + self.add_error('new_datasource_name', "This field is required.") + + if connect == "existing" and not existing_datasource: self.add_error('existing_datasource_id', "This field is required.") return cleaned_data def clean_repo_url(self): - github_base_url = "https://github.com/" value = self.cleaned_data['repo_url'] - - if not value.startswith(github_base_url): - raise ValidationError("Invalid Github repository URL") - - repo_name = value[len(github_base_url):] - org, _ = repo_name.split("/", 1) - - if org not in settings.GITHUB_ORGS: - orgs = ", ".join(settings.GITHUB_ORGS) - raise ValidationError( - f"Unknown Github organization, must be one of {orgs}", - ) - + repo_name = value.replace("https://github.com/", "", 1) repo = get_repository(self.request.user, repo_name) if repo is None: raise ValidationError( f"Github repository not found - it may be private", ) - return value + if App.objects.filter(repo_url=value).exists(): + raise ValidationError(f"App already exists for this repository URL") - -def has_env_prefix(value): - if not value.startswith(f'{settings.ENV}-'): - raise ValidationError( - f"Bucket name must be prefixed with {settings.ENV}-" - ) + return value class CreateDatasourceForm(forms.Form): name = forms.CharField( - max_length=60, + max_length=63, validators=[ - has_env_prefix, - RegexValidator(r'[a-z0-9.-]{1,60}'), + validators.validate_env_prefix, + validators.validate_s3_bucket_labels, + validators.validate_s3_bucket_length, ], ) @@ -117,8 +129,10 @@ class GrantAccessForm(forms.Form): required=True, ), label="Paths (optional)", - help_text="Add specific paths for this user or group to access or leave blank " - "for whole bucket access", + help_text=( + "Add specific paths for this user or group to access or leave blank " + "for whole bucket access" + ), required=False, delimiter="\n", ) @@ -176,10 +190,28 @@ def __init__(self, *args, **kwargs): class CreateParameterForm(forms.Form): key = forms.CharField( - validators=[RegexValidator(r'[a-zA-Z0-9_]{1,50}')] + max_length=50, + validators=[ + RegexValidator( + r'[a-zA-Z0-9_]{1,50}', + message=( + "Must be 50 characters or fewer and contain only alphanumeric " + "characters and underscores" + ), + ), + ], ) role_name = forms.CharField( - validators=[RegexValidator(r'[a-zA-Z0-9_-]{1,60}')] + max_length=60, + validators=[ + RegexValidator( + r'[a-zA-Z0-9_-]{1,60}', + message=( + "Must be 60 characters or fewer and contain only alphanumeric " + "characters, underscores and hyphens" + ), + ), + ], ) value = forms.CharField(widget=forms.PasswordInput) app_type = forms.ChoiceField(choices=APP_TYPE_CHOICES) @@ -187,9 +219,41 @@ class CreateParameterForm(forms.Form): class CreateIAMManagedPolicyForm(forms.Form): name = forms.CharField( + # TODO restrict allowed characters in group policy name validators=[RegexValidator(POLICY_NAME_REGEX)] ) class AddUserToIAMManagedPolicyForm(forms.Form): user_id = forms.CharField(max_length=128) + + +class AppCustomersField(forms.Field): + + def __init__(self, *, delimiters=APP_CUSTOMERS_DELIMITERS, strip=True, **kwargs): + self.delimiters = delimiters + self.strip = strip + super().__init__(**kwargs) + + def to_python(self, value): + emails = self.delimiters.split(value) + if self.strip: + emails = [email.strip() for email in emails] + return emails + + def clean(self, value): + value = self.to_python(value) + for email in value: + try: + validate_email(email) + except ValidationError: + raise ValidationError( + '"%(value)s" is not a valid email address', + params={'value': email}, + ) + return value + + +class AddAppCustomersForm(forms.Form): + customer_email = AppCustomersField() + diff --git a/controlpanel/frontend/jinja2/webapp-detail.html b/controlpanel/frontend/jinja2/webapp-detail.html index 5e321ae0f..bfc41100a 100644 --- a/controlpanel/frontend/jinja2/webapp-detail.html +++ b/controlpanel/frontend/jinja2/webapp-detail.html @@ -121,9 +121,11 @@

Add app customers by entering their email addresses (separated by spaces) {% if errors and errors.customer_email %} + {% for error in errors.customer_email %} - Error: {{ errors.customer_email }} + Error: {{ error }} + {% endfor %} {% endif %} diff --git a/controlpanel/frontend/views/app.py b/controlpanel/frontend/views/app.py index d12890a58..4e4ffdcf4 100644 --- a/controlpanel/frontend/views/app.py +++ b/controlpanel/frontend/views/app.py @@ -3,6 +3,7 @@ from django.conf import settings from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib import messages +from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404 from django.urls import reverse_lazy from django.template.defaultfilters import pluralize @@ -16,6 +17,7 @@ ) from django.views.generic.list import ListView from rules.contrib.views import PermissionRequiredMixin +import sentry_sdk from controlpanel.api.cluster import get_repositories from controlpanel.api import cluster @@ -28,6 +30,7 @@ UserS3Bucket, ) from controlpanel.frontend.forms import ( + AddAppCustomersForm, CreateAppForm, GrantAppAccessForm, ) @@ -80,6 +83,11 @@ def get_context_data(self, **kwargs): exclude_connected=True, ) + add_customer_form_errors = self.request.session.pop('add_customer_form_errors', None) + if add_customer_form_errors: + errors = context.setdefault('errors', {}) + errors['customer_email'] = add_customer_form_errors['customer_email'] + context['kibana_base_url'] = settings.KIBANA_BASE_URL return context @@ -164,6 +172,7 @@ def get_success_url(self): return reverse_lazy("manage-app", kwargs={"pk": self.object.app.id}) def form_valid(self, form): + # TODO this can be replaced with AppS3Bucket.objects.get_or_create() try: self.object = AppS3Bucket.objects.get( s3bucket=form.cleaned_data['datasource'], @@ -180,6 +189,11 @@ def form_valid(self, form): return FormMixin.form_valid(self, form) def form_invalid(self, form): + # It should be impossible to get here. The form consists of + # ChoiceFields, so the only way an invalid input can be submitted is by + # constructing the request manually - in which (suspicious) case we should + # return as little information as possible + log.warning('Received suspicious invalid grant app access request') raise Exception(form.errors) @@ -232,16 +246,28 @@ def post(self, request, *args, **kwargs): return super().post(request, *args, **kwargs) -class AddCustomers(UpdateApp): +class AddCustomers(LoginRequiredMixin, PermissionRequiredMixin, UpdateView): + form_class = AddAppCustomersForm + model = App permission_required = 'api.add_app_customer' - def perform_update(self, **kwargs): - app = self.get_object() - emails = self.request.POST.get('customer_email') - emails = re.split(r'[,; ]+', emails) - emails = [email.strip() for email in emails] - app.add_customers(emails) + def form_invalid(self, form): + self.request.session['add_customer_form_errors'] = form.errors + return HttpResponseRedirect( + reverse_lazy("manage-app", kwargs={"pk": self.kwargs['pk']}), + ) + + def form_valid(self, form): + self.get_object().add_customers(form.cleaned_data['customer_email']) + return HttpResponseRedirect(self.get_success_url()) + + def get_form_kwargs(self): + kwargs = FormMixin.get_form_kwargs(self) + return kwargs + + def get_success_url(self, *args, **kwargs): messages.success(self.request, f"Successfully added customers") + return reverse_lazy("manage-app", kwargs={"pk": self.kwargs["pk"]}) class RemoveCustomer(UpdateApp): @@ -250,8 +276,13 @@ class RemoveCustomer(UpdateApp): def perform_update(self, **kwargs): app = self.get_object() user_ids = self.request.POST.getlist('customer') - app.delete_customers(user_ids) - messages.success(self.request, f"Successfully removed customer{pluralize(user_ids)}") + try: + app.delete_customers(user_ids) + except App.DeleteCustomerError as e: + sentry_sdk.capture_exception(e) + messages.error(self.request, f"Failed removing customer{pluralize(user_ids)}") + else: + messages.success(self.request, f"Successfully removed customer{pluralize(user_ids)}") class AddAdmin(UpdateApp): diff --git a/controlpanel/settings/common.py b/controlpanel/settings/common.py index 06f477d93..3be6c0017 100644 --- a/controlpanel/settings/common.py +++ b/controlpanel/settings/common.py @@ -70,8 +70,6 @@ "django_redis", # Django REST Framework "rest_framework", - # Sentry error tracking - "raven.contrib.django.raven_compat", # Django Rules object permissions "rules", # Analytics Platform Control Panel API @@ -379,16 +377,18 @@ # -- Sentry error tracking if os.environ.get("SENTRY_DSN"): - RAVEN_CONFIG = { - "dsn": os.environ.get("SENTRY_DSN", ""), - "environment": os.environ.get("ENV", "dev"), - "ignore_exceptions": [], - } + import sentry_sdk + from sentry_sdk.integrations.django import DjangoIntegration + sentry_sdk.init( + dsn=os.environ["SENTRY_DSN"], + environment=os.environ.get("ENV", "dev"), + integrations=[DjangoIntegration()], + ) if "shell" in sys.argv or "shell_plus" in sys.argv: - RAVEN_CONFIG["ignore_exceptions"] = ["*"] - -else: - INSTALLED_APPS.remove("raven.contrib.django.raven_compat") + sentry_sdk.init( + # discard all events + before_send=lambda event, hint: None, + ) # -- Static files diff --git a/requirements.lock b/requirements.lock index 208af9bdd..602b57418 100644 --- a/requirements.lock +++ b/requirements.lock @@ -1,6 +1,6 @@ aioredis==1.3.0 asgiref==3.2.1 -asn1crypto==1.1.0 +asn1crypto==1.2.0 async-timeout==3.0.1 atomicwrites==1.3.0 attrs==19.3.0 @@ -86,7 +86,6 @@ python-dateutil==2.8.0 python-jose==3.0.1 pytz==2019.3 PyYAML==5.1.2 -raven==6.10.0 redis==3.3.11 requests==2.22.0 requests-oauthlib==1.2.0 @@ -94,6 +93,7 @@ responses==0.10.6 rsa==4.0 rules==2.1 s3transfer==0.2.1 +sentry-sdk==0.12.3 service-identity==18.1.0 six==1.12.0 sqlparse==0.3.0 diff --git a/requirements.txt b/requirements.txt index 3b0342059..bd6cdd20e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,8 +26,8 @@ PyGithub==1.43.8 pytest==5.1.1 pytest-django==3.5.1 python-jose[cryptography]==3.0.1 -raven==6.10.0 rules==2.1 +sentry-sdk==0.12.3 service_identity==18.1.0 urllib3==1.25.3 uvicorn==0.8.6 diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py index 38ce88401..3b02a68da 100644 --- a/tests/frontend/views/test_app.py +++ b/tests/frontend/views/test_app.py @@ -1,5 +1,6 @@ from unittest.mock import patch +from django.contrib.messages import get_messages from django.template.response import TemplateResponse from django.views.generic.base import TemplateResponseMixin from django.urls import reverse @@ -7,6 +8,8 @@ import pytest from rest_framework import status +from controlpanel.api.auth0 import Auth0Error + NUM_APPS = 3 @@ -78,7 +81,7 @@ def apps3bucket(app, s3buckets): return mommy.make('api.AppS3Bucket', app=app, s3bucket=s3buckets['connected']) -def list(client, *args): +def list_apps(client, *args): return client.get(reverse('list-apps')) @@ -142,9 +145,9 @@ def connect_bucket(client, app, _, s3buckets, *args): @pytest.mark.parametrize( 'view,user,expected_status', [ - (list, 'superuser', status.HTTP_200_OK), - (list, 'app_admin', status.HTTP_200_OK), - (list, 'normal_user', status.HTTP_200_OK), + (list_apps, 'superuser', status.HTTP_200_OK), + (list_apps, 'app_admin', status.HTTP_200_OK), + (list_apps, 'normal_user', status.HTTP_200_OK), (list_all, 'superuser', status.HTTP_200_OK), (list_all, 'app_admin', status.HTTP_403_FORBIDDEN), @@ -210,9 +213,9 @@ def test_bucket_permissions(client, apps3bucket, users, view, user, expected_sta @pytest.mark.parametrize( 'view,user,expected_count', [ - (list, 'superuser', 0), - (list, 'normal_user', 0), - (list, 'app_admin', 1), + (list_apps, 'superuser', 0), + (list_apps, 'normal_user', 0), + (list_apps, 'app_admin', 1), (list_all, 'superuser', NUM_APPS), ], @@ -222,3 +225,64 @@ def test_list(client, app, users, view, user, expected_count): response = view(client, app, users) assert len(response.context_data['object_list']) == expected_count + +def add_customer_success(client, response): + return 'add_customer_form_errors' not in client.session + + +def add_customer_form_error(client, response): + return 'add_customer_form_errors' in client.session + + +@pytest.mark.parametrize( + 'emails, expected_response', + [ + ('foo@example.com', add_customer_success), + ('foo@example.com, bar@example.com', add_customer_success), + ('foobar', add_customer_form_error), + ('foo@example.com, foobar', add_customer_form_error), + ('', add_customer_form_error), + ], + ids=[ + 'single-valid-email', + 'multiple-delimited-emails', + 'invalid-email', + 'mixed-valid-invalid-emails', + 'no-emails', + ], +) +def test_add_customers(client, app, users, emails, expected_response): + client.force_login(users['superuser']) + data = {'customer_email': emails} + response = client.post(reverse('add-app-customers', kwargs={'pk': app.id}), data) + assert expected_response(client, response) + + +def remove_customer_success(client, response): + messages = [str(m) for m in get_messages(response.wsgi_request)] + return f'Successfully removed customer' in messages + + +def remove_customer_failure(client, response): + messages = [str(m) for m in get_messages(response.wsgi_request)] + return f'Failed removing customer' in messages + + +@pytest.mark.parametrize( + 'side_effect, expected_response', + [ + (None, remove_customer_success), + (Auth0Error, remove_customer_failure), + ], + ids=[ + 'success', + 'failure', + ], +) +def test_delete_customers(client, app, customers, users, side_effect, expected_response): + customers.delete_group_members.side_effect = side_effect + client.force_login(users['superuser']) + data = {'customer': ['email|1234']} + response = client.post(reverse('remove-app-customer', kwargs={'pk': app.id}), data) + assert expected_response(client, response) +