Merge lp:~cprov/canonical-identity-provider/cleaner-usernames into lp:canonical-identity-provider/release

[Celso Providelo]

Stop accepting usernames with DOTs and PLUSes.

See the entire context in https://trello.com/c/xPMusmTk/234-bug-1790874-cant-verify-accounts-with-a-dot-in-the-store-username

[Daniel Manrique (roadmr)]

LGTM - my only concern is - this will force users with non-compliant usernames to update them if they want to edit their account details, correct? since VALID_NAME_RE is used in EditAccountForm's clean_username method. Do we know about how many users would be in this situation? and - since this username is tied to Launchpad's username, could this result in a situation where a user is completely blocked from performing account changes? example: someone with a non-compliant name and a published PPA will be unable to change their username due to Launchpad policy which could prevent them from e.g. changing their password.

That's about it, the code looks reasonable vs. the intent described in the Trello card.

review: Approve

[Celso Providelo (cprov)]

As discussed on IRC, users with now bad-usernames and PPAs will be locked in the SSO account edit form until we come up with a solution the resembles "usernames aliases" in LP.

[Celso Providelo (cprov)]

Oops, the regexp is missing some requirements (>=3 chars & refuse digit-only terms)

[Daniel Manrique (roadmr)]

+1 with two nitpicks and two possible test string suggestions.

review: Approve

[Daniel Manrique (roadmr)]

LGTM thanks!

review: Approve

[Igor Komlew (glower)]

With this regexp you can have more separators after each other, like: x----x----x

[Daniel Manrique (roadmr)]

The description is ambiguous as to whether multiple consecutive separators are allowed, so a decision has to be made. It was:

roadmr niemeyer: is 'x---x---x' a valid username? that's all we need to know
niemeyer roadmr: No

=== modified file 'src/identityprovider/forms.py'
--- src/identityprovider/forms.py	2018-12-13 14:47:48 +0000
+++ src/identityprovider/forms.py	2019-04-03 17:04:50 +0000
@@ -54,12 +54,13 @@
     ),
 }
 
-VALID_NAME_RE = re.compile(r"^[a-z0-9][a-z0-9\+\.\-]+$")
+# Should match SCA ``short_namespace_re` expression.
+VALID_NAME_RE = re.compile(r"(?!^[\d-]+$)^[a-z\d](-?[a-z\d]){2,31}$")
 
 INVALID_NAME = _(
-    'Usernames must be at least two characters long and start with a letter '
-    'or number. All letters must be lower-case. The characters +, - and . '
-    'are also allowed after the first character.')
+    'Usernames must be 3 to 32 characters long, using lower-case letters or '
+    'digits and the character - can be used as a separator. Numbers-only '
+    'terms and consecutive separators are not allowed.')
 
 
 class DeleteAccountForm(forms.Form):
@@ -336,6 +337,11 @@
         username = self.cleaned_data['username'].strip()
         if not username:
             return username
+        # Previously set usernames can skip verification. That allows
+        # users to edit their account details even if their usernames
+        # are now considered "distasteful".
+        if username == self.instance.person_name:
+            return username
         if not VALID_NAME_RE.match(username):
             raise forms.ValidationError(INVALID_NAME)
         if username and username != self.instance.person_name:

=== modified file 'src/identityprovider/tests/test_forms.py'
--- src/identityprovider/tests/test_forms.py	2018-12-12 17:26:19 +0000
+++ src/identityprovider/tests/test_forms.py	2019-04-03 17:04:50 +0000
@@ -7,6 +7,7 @@
 from django import forms
 from django.conf import settings
 from django.http import HttpRequest
+from django.test import TestCase
 from django.test.utils import override_settings
 from django.utils.timezone import now
 from django_openid_auth.teams import TeamsRequest
@@ -35,6 +36,7 @@
 )
 from identityprovider.forms import (
     INVALID_NAME,
+    VALID_NAME_RE,
     AccountStatusForm,
     DeviceRenameForm,
     EditAccountForm,
@@ -58,6 +60,59 @@
 from identityprovider.validators import BasicValidator
 
 
+class ValidNameTestCase(TestCase):
+
+    def test_valid_names(self):
+        names = [
+            # 3 or more lowercase chars.
+            'foo',
+            # Or chars and digits
+            'f00',
+            # One or more dashes followed by at least one chars or digits.
+            'fo-o'
+            'f-oo',
+            'foo-1',
+            '1-foo',
+            'f-o-o',
+            # Up to 32 chars long
+            'a' * 32,
+        ]
+        for n in names:
+            self.assertTrue(
+                VALID_NAME_RE.match(n), '%r is unexpectedly invalid.' % n)
+
+    def test_invalid_names(self):
+        names = [
+            # 3 to 32 chars long.
+            'fu',
+            'a' * 33,
+            # No uppercase-chars
+            'Foo',
+            # No digits-only,
+            '111',
+            # No dashes-only.
+            '---',
+            # No single-char separator
+            'a-a',
+            # No consecutive dashes.
+            'a--a',
+            # No digit+hyphen-only.
+            '1-23',
+            # No DOTs.
+            'foo.bar',
+            # No PLUSes.
+            'foo+bar',
+            # No starting or ending with DASHes.
+            '-foo',
+            'foo-',
+            # No other special chars.
+            'foo%bar',
+        ]
+        for n in names:
+            self.assertFalse(
+                VALID_NAME_RE.match(n), '%r is unexpectedly valid.' % n)
+
+
 class FormBaseTestCase(SSOBaseTestCase):
 
     fields = []

=== modified file 'src/identityprovider/tests/test_views_server.py'
--- src/identityprovider/tests/test_views_server.py	2018-12-19 21:57:07 +0000
+++ src/identityprovider/tests/test_views_server.py	2019-04-03 17:04:50 +0000
@@ -1362,7 +1362,7 @@
         data = {
             'displayname': self.account.displayname,
             'preferred_email': self.account.preferredemail.id,
-            'username': 'test-valid+username.',
+            'username': 'test-valid-username',
             'oldpassword': self.login_password,
         }
         response = self.client.post(self.url, data, follow=True)

=== modified file 'src/webui/tests/test_views_account.py'
--- src/webui/tests/test_views_account.py	2018-12-19 21:57:07 +0000
+++ src/webui/tests/test_views_account.py	2019-04-03 17:04:50 +0000
@@ -20,6 +20,7 @@
 from pyquery import PyQuery
 
 from identityprovider.const import SESSION_TOKEN_KEY, SESSION_TOKEN_NAME
+from identityprovider.forms import INVALID_NAME
 from identityprovider.models import (
     Account,
     AuthToken,
@@ -402,12 +403,27 @@
         r = self.post_username_change('BAD')
         self.assertEqual(r.status_code, 200)
         self.assertIsNone(self.account.person)
-        self.assertContains(
-            r,
-            'Usernames must be at least two characters long and start with a '
-            'letter or number. All letters must be lower-case. The '
-            'characters +, - and . are also allowed after the first '
-            'character.')
+        self.assertContains(r, 'Usernames must be')
+        self.assertContains(r,  INVALID_NAME)
+
+    @switches(USERNAME_UI=True)
+    def test_edit_username_preserves_bad_usernames(self):
+        # Users with existing 'bad' usernames are not be forced to change them
+        # just because we changed the criteria.
+        bad_username = 'Bad+Taste.-'
+        self.lp.set_lp_username(
+            self.account.openid_identifier, bad_username)
+        self.lp.flush_to_db()
+        # 'bad' username is displayed as the default field value.
+        r = self.client.get(self.url)
+        self.assertContains(r, 'Username')
+        self.assertContains(r, 'value="%s"' % bad_username)
+        # The form can be submitted as is and will succeed.
+        r = self.post_username_change(bad_username)
+        self.assertEqual(r.status_code, 302)
+        self.assertEqual(bad_username, self.account.person.name)
+        r = self.client.get(self.url)
+        self.assertContains(r, 'value="%s"' % bad_username)
 
     def test_index_edit_password(self):
         oauth_tokens = self.account.token_set.all()