launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/cleaner-usernames into lp:launchpad.

Commit message:
Preventing users to set 'unclean' usernames.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cprov/launchpad/cleaner-usernames/+merge/367101

SSO already creates PLACEHOLDER accounts (openid->account->person), so new users coming to LP have an username already defined during SSO registration, `generate_nick()` is not used.

By forcing users to obey the new 'clean' rules when modifying their usernames we will prevent new 'unclean' usernames to surface in the ecosystem.

The Person.name schema was not changed, so LP administrators and internal components are still able to bypass the new criteria and set 'unclean' usernames to other accounts, when it is necessary: e.g deactivation and merging

LP teams creation and edits are not affected by new rules. 
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/cleaner-usernames into lp:launchpad.

=== modified file 'lib/lp/app/validators/name.py'
--- lib/lp/app/validators/name.py	2012-11-29 05:52:36 +0000
+++ lib/lp/app/validators/name.py	2019-05-08 12:45:11 +0000
@@ -39,7 +39,7 @@
 def valid_name(name):
     """Return True if the name is valid, otherwise False.
 
-    Lauchpad `name` attributes are designed for use as url components
+    Launchpad `name` attributes are designed for use as url components
     and short unique identifiers to things.
 
     The default name constraints may be too strict for some objects,

=== modified file 'lib/lp/app/validators/tests/test_validators.py'
--- lib/lp/app/validators/tests/test_validators.py	2014-03-03 19:42:30 +0000
+++ lib/lp/app/validators/tests/test_validators.py	2019-05-08 12:45:11 +0000
@@ -27,11 +27,12 @@
     suite.addTest(
         DocTestSuite(validators, optionflags=ELLIPSIS | NORMALIZE_WHITESPACE))
 
-    from lp.app.validators import cve, email, name, url, version
+    from lp.app.validators import cve, email, name, url, username, version
     suite.addTest(suitefor(cve))
     suite.addTest(suitefor(email))
     suite.addTest(suitefor(name))
     suite.addTest(suitefor(url))
+    suite.addTest(suitefor(username))
     suite.addTest(suitefor(version))
 
     return suite

=== added file 'lib/lp/app/validators/username.py'
--- lib/lp/app/validators/username.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/validators/username.py	2019-05-08 12:45:11 +0000
@@ -0,0 +1,97 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Validators for the clean-username (`Person.name`) attribute."""
+
+__metaclass__ = type
+
+import re
+from textwrap import dedent
+
+from lp import _
+from lp.app.validators import LaunchpadValidationError
+from lp.services.webapp.escaping import (
+    html_escape,
+    structured,
+    )
+
+
+username_valid_pattern = re.compile(r"^[a-z0-9](-?[a-z0-9])+$")
+username_blocked_pattern = re.compile(r"^[0-9-]+$")
+username_invalid_pattern = re.compile(r"^[^a-z0-9]+|[^a-z0-9\\-]+")
+
+
+def sanitize_username(username):
+    """Remove from the given username all characters that are not allowed.
+
+    The characters not allowed in Launchpad usernames are described by
+    `username_invalid_pattern`.
+
+    >>> sanitize_username('foo_bar')
+    'foobar'
+    >>> sanitize_username('foo.bar+baz')
+    'foobarbaz'
+    >>> sanitize_username('foo $fd?+.0')
+    'foofd0'
+
+    """
+    return username_invalid_pattern.sub('', username)
+
+
+def valid_username(username):
+    """Return True if the username is valid, otherwise False.
+
+    Launchpad `username` (`Person.name`) attribute is designed to serve as
+    an human-friendly identifier to a identity across multiple services.
+
+    >>> valid_username('hello')
+    True
+    >>> valid_username('helLo')
+    False
+    >>> valid_username('hel|o')
+    False
+    >>> valid_username('hel.o')
+    False
+    >>> valid_username('hel+o')
+    False
+    >>> valid_username('he')
+    False
+    >>> valid_username('hel')
+    True
+    >>> valid_username('h' * 32)
+    True
+    >>> valid_username('h' * 33)
+    False
+    >>> valid_username('-he')
+    False
+    >>> valid_username('he-')
+    False
+
+    """
+    if not username_valid_pattern.match(username):
+        return False
+
+    length = len(username)
+    if length < 3 or length > 32:
+        return False
+
+    if username_blocked_pattern.match(username):
+        return False
+
+    return True
+
+
+def username_validator(username):
+    """Return True if the username is valid, or raise a
+    LaunchpadValidationError.
+    """
+    if not valid_username(username):
+        message = _(dedent("""
+            Invalid username '${username}'. Usernames must be at least three
+            and no longer than 32 characters long. They must start and end with
+            a letter or number, all letters must be lower-case and
+            non-consecutive hyphens are allowed."""),
+        mapping={'username': html_escape(username)})
+        raise LaunchpadValidationError(structured(message))
+
+    return True

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2018-07-16 00:50:41 +0000
+++ lib/lp/registry/browser/person.py	2019-05-08 12:45:11 +0000
@@ -116,7 +116,9 @@
     )
 from lp.app.interfaces.headings import IHeadingBreadcrumb
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.app.validators import LaunchpadValidationError
 from lp.app.validators.email import valid_email
+from lp.app.validators.username import username_validator
 from lp.app.widgets.image import ImageChangeWidget
 from lp.app.widgets.itemswidgets import (
     LaunchpadDropdownWidget,
@@ -2720,9 +2722,22 @@
     def validate(self, data):
         """If the name changed, warn the user about the implications."""
         new_name = data.get('name')
+
+        # Name was not changed, carry on ...
+        if not new_name or new_name == self.context.name:
+            return
+
+        # Ensure the new (user) name is valid.
+        try:
+            username_validator(new_name)
+        except LaunchpadValidationError as err:
+            self.setFieldError('name', str(err))
+            return
+
+        # Ensure the user is aware of the implications of changing username.
         bypass_check = self.request.form_ng.getOne(
             'i_know_this_is_an_openid_security_issue', 0)
-        if (new_name and new_name != self.context.name and not bypass_check):
+        if not bypass_check:
             # Warn the user that they might shoot themselves in the foot.
             self.setFieldError('name', structured(dedent('''
             <div class="inline-warning">

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2018-01-02 16:10:26 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2019-05-08 12:45:11 +0000
@@ -567,6 +567,42 @@
         self.ppa = self.factory.makeArchive(owner=self.person)
         self.view = create_initialized_view(self.person, '+edit')
 
+    def test_unclean_usernames_cannot_be_set(self):
+        # Users cannot set unclean usernames
+        form = {
+            'field.name': 'unclean.name',
+            'field.actions.save': 'Save Changes',
+            }
+        view = create_initialized_view(self.person, '+edit', form=form)
+
+        expected_msg = html_escape(dedent("""
+            Invalid username 'unclean.name'. Usernames must be at least three
+            and no longer than 32 characters long. They must start and end with
+            a letter or number, all letters must be lower-case and
+            non-consecutive hyphens are allowed."""))
+        self.assertEqual(expected_msg, view.errors[0])
+
+    def test_unclean_usernames_do_not_block_edit(self):
+        # Users with unclean usernames (less restrictive) are not forced
+        # to update it along with other details of their account.
+        dirty_person = self.factory.makePerson(name='unclean.name')
+        login_person(dirty_person)
+
+        form = {
+            'field.display_name': 'Nice Displayname',
+            'field.name': dirty_person.name,
+            'field.actions.save': 'Save Changes',
+            }
+        view = create_initialized_view(dirty_person, '+edit', form=form)
+
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            'The changes to your personal details have been saved.',
+            notifications[0].message)
+        self.assertEqual('Nice Displayname', dirty_person.displayname)
+        self.assertEqual('unclean.name', dirty_person.name)
+
     def createAddEmailView(self, email_address):
         """Test helper to create +editemails view."""
         form = {