Fix bugs with organization form and slug input (#12306)

Found today that we introduced some bugs on the organization form,
namely we aren't validating the user provided slug, only validating the
user provided name rendered to a slug.

This also ensures we're slugging for DNS safely, as this is just
prepended to project URLs without much validation.

Closes #12305

Author: agjohnson

readthedocs/organizations/forms.py

@@ -60,23 +60,30 @@ def __init__(self, *args, **kwargs):
             )
         super().__init__(*args, **kwargs)
 
-    def clean_name(self):
-        """Raise exception on duplicate organization slug."""
-        name = self.cleaned_data["name"]
+    def clean_slug(self):
+        slug_source = self.cleaned_data["slug"]
 
         # Skip slug validation on already created organizations.
         if self.instance.pk:
-            return name
-
-        potential_slug = slugify(name)
-        if not potential_slug:
-            raise forms.ValidationError(_("Invalid organization name: no slug generated"))
-        if Organization.objects.filter(slug=potential_slug).exists():
+            return slug_source
+
+        slug = slugify(slug_source, dns_safe=True)
+        if not slug:
+            # If the was not empty, but renders down to something empty, the
+            # user gave an invalid slug. However, we can't suggest anything
+            # useful because the slug is empty. This is an edge case for input
+            # like `---`, so the error here doesn't need to be very specific.
+            raise forms.ValidationError(_("Invalid slug, use more valid characters."))
+        elif slug != slug_source:
+            # There is a difference between the slug from the front end code, or
+            # the user is trying to submit the form without our front end code.
             raise forms.ValidationError(
-                _("Organization %(name)s already exists"),
-                params={"name": name},
+                _("Invalid slug, use suggested slug '%(slug)s' instead"),
+                params={"slug": slug},
             )
-        return name
+        if Organization.objects.filter(slug=slug).exists():
+            raise forms.ValidationError(_("Slug is already used by another organization"))
+        return slug
 
 
 class OrganizationSignupFormBase(OrganizationForm):
@@ -106,9 +113,6 @@ class Meta:
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
 
-        # `slug` is not required since its value is auto-generated from `name` if not provided
-        self.fields["slug"].required = False
-
     @staticmethod
     def _create_default_teams(organization):
         organization.teams.create(name="Admins", access=ADMIN_ACCESS)

readthedocs/organizations/tests/test_forms.py

@@ -143,15 +143,27 @@ def test_add_duplicate_invite_by_email(self):
 
 
 class OrganizationSignupTest(OrganizationTestCase):
-    def test_create_organization_with_empy_slug(self):
+    def test_create_organization_with_empty_slug(self):
         data = {
             "name": "往事",
             "email": "[email protected]",
         }
         form = forms.OrganizationSignupForm(data, user=self.user)
         self.assertFalse(form.is_valid())
         self.assertEqual(
-            "Invalid organization name: no slug generated", form.errors["name"][0]
+            "This field is required.", form.errors["slug"][0]
+        )
+
+    def test_create_organization_with_invalid_unicode_slug(self):
+        data = {
+            "name": "往事",
+            "email": "[email protected]",
+            "slug": "-",
+        }
+        form = forms.OrganizationSignupForm(data, user=self.user)
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            "Invalid slug, use more valid characters.", form.errors["slug"][0]
         )
 
     def test_create_organization_with_big_name(self):
@@ -165,17 +177,20 @@ def test_create_organization_with_big_name(self):
 
     def test_create_organization_with_existent_slug(self):
         data = {
-            "name": "mozilla",
+            "name": "Fauxzilla",
             "email": "[email protected]",
+            "slug": "mozilla",
         }
         form = forms.OrganizationSignupForm(data, user=self.user)
         # there is already an organization with the slug ``mozilla`` (lowercase)
         self.assertFalse(form.is_valid())
+        self.assertEqual("Slug is already used by another organization", form.errors["slug"][0])
 
     def test_create_organization_with_nonexistent_slug(self):
         data = {
             "name": "My New Organization",
             "email": "[email protected]",
+            "slug": "my-new-organization",
         }
         form = forms.OrganizationSignupForm(data, user=self.user)
         self.assertTrue(form.is_valid())
@@ -191,3 +206,13 @@ def test_create_organization_with_invalid_slug(self):
         form = forms.OrganizationSignupForm(data, user=self.user)
         self.assertFalse(form.is_valid())
         self.assertIn("consisting of letters, numbers", form.errors["slug"][0])
+
+    def test_create_organization_with_dns_invalid_slug(self):
+        data = {
+            "name": "My Org",
+            "email": "[email protected]",
+            "slug": "-invalid_slug-",
+        }
+        form = forms.OrganizationSignupForm(data, user=self.user)
+        self.assertFalse(form.is_valid())
+        self.assertIn("Invalid slug, use suggested slug 'invalid-slug' instead", form.errors["slug"][0])

readthedocs/organizations/tests/test_views.py

@@ -361,6 +361,7 @@ def test_organization_signup(self):
         data = {
             "name": "Testing Organization",
             "email": "[email protected]",
+            "slug": "testing-organization",
         }
         resp = self.client.post(reverse("organization_create"), data=data)
         self.assertEqual(Organization.objects.count(), 1)