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

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

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

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}",
+        )

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,20 +190,70 @@ 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)
 
 
 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()
+

controlpanel/frontend/jinja2/webapp-detail.html

@@ -121,9 +121,11 @@ <h2 class="govuk-heading-m">
         Add app customers by entering their email addresses (separated by spaces)
       </label>
       {% if errors and errors.customer_email %}
+        {% for error in errors.customer_email %}
       <span id="customer_email-error" class="govuk-error-message">
-        <span class="govuk-visually-hidden">Error:</span> {{ errors.customer_email }}
+        <span class="govuk-visually-hidden">Error:</span> {{ error }}
       </span>
+        {% endfor %}
       {% endif %}
       <input id="customer_email" class="govuk-input cpanel-input--1-3" name="customer_email" autocomplete="off">
     </div>