launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === 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-07 00:19:42 +0000
> @@ -19,6 +19,7 @@
>  valid_name_pattern = re.compile(r"^[a-z0-9][a-z0-9\+\.\-]+$")
>  valid_bug_name_pattern = re.compile(r"^[a-z][a-z0-9\+\.\-]+$")
>  invalid_name_pattern = re.compile(r"^[^a-z0-9]+|[^a-z0-9\\+\\.\\-]+")
> +valid_username_pattern = re.compile(r"(?!^[\d-]+$)^[a-z\d](-?[a-z\d]){2,31}$")

You're going to need an equivalent of sanitize_name too, and to use it in lib/lp/registry/model/person.py:generate_nick.  For the latter, please particularly check that it copes with cases that are now more difficult, such as an email address with a local part that's right up against the length limit and that ends with either "-" or a character that's replaced with "-".

>  
>  
>  def sanitize_name(name):
> @@ -97,3 +145,18 @@
>  
>          raise LaunchpadValidationError(structured(message))
>      return True
> +
> +
> +def username_validator(name):
> +    """Return True if the `Person.name` is valid, or raise a
> +    LaunchpadValidationError.
> +    """
> +    if not valid_username(name):
> +        message = _(dedent("""
> +            Invalid username '${name}'. Usernames must be at least two characters long
> +            and start with a letter or number. All letters must be lower-case.

This doesn't seem quite accurate.  "Usernames must be at least three characters long, and must both start and end with a letter or number", perhaps?

> +            The character <samp>-</samp> is allowed after the first character."""),

Maybe something about the non-consecutive requirement?

> +            mapping={'name': html_escape(name)})
> +
> +        raise LaunchpadValidationError(structured(message))
> +    return True
> 
> === modified file 'lib/lp/bugs/model/bugtracker.py'
> --- lib/lp/bugs/model/bugtracker.py	2019-02-23 08:15:45 +0000
> +++ lib/lp/bugs/model/bugtracker.py	2019-05-07 00:19:42 +0000
> @@ -645,9 +645,18 @@
>          if bugtracker_person is not None:
>              return bugtracker_person.person
>  
> -        # Generate a valid Launchpad name for the Person.
> -        base_canonical_name = (
> -            "%s-%s" % (sanitize_name(display_name.lower()), self.name))
> +        # Generate a valid Launchpad name for the Person extracting an
> +        # username from the given display_name smaller enough to be joined
> +        # with the tracker name and up to 2 digits (99) for disambiguation
> +        # and fit in 32 characters limit and avoid consecutive hyphens.
> +        remaining = 32 - len(self.name) - 2
> +        username = sanitize_name(display_name.lower())[:remaining]
> +        username = username if not username.endswith('-') else username[:-1]
> +
> +        # XXX cprov 2019-05-06: bugtracker names may be longer than 32-chars,
> +        # we need extra name-cropping if this simple strategy gets accepted.

I think we should probably just require bugtracker names only to comply with the older rules instead, at least for now.  We certainly shouldn't limit their length as strictly, IMO.

For internal use such as this, it may be necessary to have a flag when creating a person that allows using the name validator rather than the username validator (or it could maybe just select validation rules based on the person creation rationale, if that turns out to be good enough).

> +
> +        base_canonical_name = "%s-%s" % (username, self.name)
>          canonical_name = base_canonical_name
>  
>          person_set = getUtility(IPersonSet)
> 
> === modified file 'lib/lp/registry/interfaces/person.py'
> --- lib/lp/registry/interfaces/person.py	2018-08-02 16:12:57 +0000
> +++ lib/lp/registry/interfaces/person.py	2019-05-07 00:19:42 +0000
> @@ -662,11 +665,11 @@
>      name = exported(
>          PersonNameField(
>              title=_('Name'), required=True, readonly=False,
> -            constraint=name_validator,
> +            constraint=username_validator,
>              description=_(
>                  "A short unique name, beginning with a lower-case "

s/beginning/beginning and ending/

>                  "letter or number, and containing only letters, "
> -                "numbers, dots, hyphens, or plus signs.")))
> +                "numbers and non-consecutive hyphens.")))
>      display_name = exported(
>          StrippedTextLine(
>              title=_('Display Name'), required=True, readonly=False,
> 
> === modified file 'lib/lp/registry/tests/test_nickname.py'
> --- lib/lp/registry/tests/test_nickname.py	2015-10-26 14:54:43 +0000
> +++ lib/lp/registry/tests/test_nickname.py	2019-05-07 00:19:42 +0000
> @@ -36,14 +36,14 @@
>          # valid nick that doesn't start with symbols.
>          parts = ['---bar', 'foo.bar', 'foo-bar', 'foo+bar']
>          nicks = [generate_nick("%s@xxxxxxxxxxx" % part) for part in parts]
> -        self.assertEqual(['bar', 'foo-bar', 'foo-bar', 'foo+bar'], nicks)
> +        self.assertEqual(['bar', 'foo-bar', 'foo-bar', 'foo-bar'], nicks)
>  
>      def test_enforces_minimum_length(self):
>          # Nicks must be a minimum length. generate_nick enforces this by
>          # adding random suffixes to the required length.
> -        self.assertIs(None, getUtility(IPersonSet).getByName('i'))
> -        nick = generate_nick('i@xxxxxxxxxxx')
> -        self.assertEqual('i-b', nick)
> +        self.assertIs(None, getUtility(IPersonSet).getByName('hi'))
> +        nick = generate_nick('hi@xxxxxxxxxxx')
> +        self.assertEqual('hi-5', nick)

As indicated above, I'd like to see at least a few more tests here for corner cases of the new rules.

>  
>      def test_can_create_noncolliding_nicknames(self):
>          # Given the same email address, generate_nick doesn't recreate the
> 
> === modified file 'lib/lp/translations/stories/standalone/xx-person-activity.txt'
> --- lib/lp/translations/stories/standalone/xx-person-activity.txt	2018-06-02 22:39:54 +0000
> +++ lib/lp/translations/stories/standalone/xx-person-activity.txt	2019-05-07 00:19:42 +0000
> @@ -51,13 +51,18 @@
>  URL-escaped user names
>  ----------------------
>  
> -Since the user's name is included in the URL, and user names can contain
> -some slightly weird characters, it is escaped especially for this usage.
> +Since the user's name is included in the URL, and legacy user names can
> +contain some slightly weird characters, it is escaped especially for this
> +usage.
>  
> -For instance, here's a user called a+b.
> +For instance, here's a legacy user called "a+b".
>  
>      >>> login('carlos@xxxxxxxxxxxxx')
> -    >>> ab = factory.makePerson(name='a+b')
> +    >>> ab = factory.makePerson()
> +    >>> from zope.security.proxy import removeSecurityProxy
> +    >>> naked_person = removeSecurityProxy(ab)
> +    >>> naked_person.name = 'a+b'
> +    >>> naked_person.display_name = 'A+b'

If you follow my suggestion above to have a flag or a creation-rationale-based indication for which validation rules to use when creating a person, then it ought to be possible to refactor this test to avoid having to remove the security proxy.

>      >>> sr_pofile = factory.makePOFile('sr')
>      >>> message = factory.makeCurrentTranslationMessage(
>      ...     pofile=sr_pofile, translator=ab)


-- 
https://code.launchpad.net/~cprov/launchpad/strict-usernames/+merge/366985
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.