launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/dont-expose-domains into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #228355 in Launchpad itself: "Unique name reveals hidden email address"
  https://bugs.launchpad.net/launchpad/+bug/228355

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/dont-expose-domains/+merge/62023

Summary
=======
Even though users can set lp to not show their email address, if their id has been automatically generated the email can still essentially be divulged b/c it's used to create the idea. (e.g. bob@xxxxxxxxx can become bob-gmail if bob is already taken or bob-gmail-com).

This code removes the domain bit from the nick generation, as there is already a random suffix generation piece as a final fall back. If bob is already taken as a username, than the next bob@somedomain becomes bob-c or similar.

Preimplementation
=================
Spoke with Curtis Hovey

Implementation
==============
lib/lp/registry/model/person.py
-------------------------------
Removed the portion of generate_nick that used the email domain to create nicks.

lib/lp/registry/tests/test_nickname.py
lib/lp/registry/doc/nickname.txt
--------------------------------
Removed the doc tests and replaced it with unittests, which are more appropriate.

Tests
=====
bin/test -t test_nickname

QA
==
Create a user on qastaging with a an email that could collide with an existing user (e.g. jcsackett@xxxxxxxxxxx). The domain should not be part of the generated nick.

Lint
====

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_nickname.py

-- 
https://code.launchpad.net/~jcsackett/launchpad/dont-expose-domains/+merge/62023
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/dont-expose-domains into lp:launchpad.

=== removed file 'lib/lp/registry/doc/nickname.txt'
--- lib/lp/registry/doc/nickname.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/registry/doc/nickname.txt	1970-01-01 00:00:00 +0000
@@ -1,96 +0,0 @@
-= Nickname =
-
-For each Person we create a unique nickname that must be generated using
-generate_nick.
-
-A valid nick can contain lower case letters, dashes, and numbers,
-must start with a letter or a number, and must be a minimum of
-four characters.
-
-    >>> from lp.registry.model.person import generate_nick
-    >>> generate_nick("foop@example@com")
-    Traceback (most recent call last):
-        ...
-    NicknameGenerationError: foop@example@com is not a valid email address
-    >>> generate_nick("foop@xxxxxxxxxxx")
-    'foop'
-    >>> generate_nick("bar@xxxxxxxxxxx")
-    'bar'
-    >>> generate_nick("spam@xxxxxxxxxxx")
-    'spam'
-    >>> generate_nick("foo.bar@xxxxxxxxxxxxx")
-    'foo-bar'
-    >>> generate_nick("foo+bar@xxxxxxxxxxx")
-    'foo+bar'
-    >>> generate_nick("--foo@xxxxxxxxxxx")
-    'foo'
-
-We already have a salgado, so generate_nick needs to try a little harder.
-
-    >>> generate_nick("salgado@xxxxxxxxxxx")
-    'salgado-async'
-
-And it needs to maintain a minimum length
-
-    >>> generate_nick("i@tv")
-    'i-tv'
-
-    >>> _counter = 0
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> def new_person(email):
-    ...     from lp.registry.interfaces.person import IPersonSet
-    ...     from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
-    ...     from lp.registry.interfaces.person import (
-    ...         PersonCreationRationale)
-    ...     UNKNOWN = PersonCreationRationale.UNKNOWN
-    ...     person_set = getUtility(IPersonSet)
-    ...     person, ignored = person_set.createPersonAndEmail(
-    ...         email, UNKNOWN)
-    ...     # Switch the email to avoid conflicts when generating clashing names
-    ...     email_address_set = getUtility(IEmailAddressSet)
-    ...     global _counter
-    ...     for email_address in email_address_set.getByPerson(person):
-    ...         removeSecurityProxy(email_address).email = (
-    ...             'nodupe%d@xxxxxxxxxxx' % _counter)
-    ...         _counter += 1
-    ...     flush_database_updates()
-    ...     assert email_address_set.getByEmail(email) is None, 'Oops'
-    ...     return person
-
-The email address is used to generate nicknames. If domain is used to
-attempt to avoid clashes.
-
-    >>> new_person('bongo@xxxxxxxxxxx').name
-    u'bongo'
-    >>> new_person('bongo@xxxxxxxxxxx').name
-    u'bongo-example'
-    >>> new_person('bongo@xxxxxxxxxxx').name
-    u'bongo-example-com'
-
-It should be nearly impossible to make the method fail even in the
-extreme cases where using all components of the domain name fails.
-If nicknames still clash, random suffixes are tried:
-
-    >>> new_person('bongo@xxxxxxxxxxx').name
-    u'bongo-example-com-c'
-
-Random prefixes are tried:
-
-    >>> new_person('bongo@xxxxxxxxxxx').name
-    u'q-bongo-example-com'
-
-And random mutations are tried:
-
-    >>> new_person('bongo@xxxxxxxxxxx').name
-    u'bongo-examale-com'
-
-The only way it can fail is if some idiot admin goes and registers a
-'match-everything' pattern in the blacklist:
-
-    >>> from canonical.database.sqlbase import cursor
-    >>> cur = cursor()
-    >>> cur.execute("INSERT INTO NameBlacklist (regexp) VALUES ('.')")
-    >>> generate_nick("foo@xxxxxxxxxxx")
-    Traceback (most recent call last):
-    ...
-    NicknameGenerationError: No nickname could be generated...

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-05-18 03:36:29 +0000
+++ lib/lp/registry/model/person.py	2011-05-23 19:19:28 +0000
@@ -4541,9 +4541,8 @@
         raise NicknameGenerationError("%s is not a valid email address"
                                       % email_addr)
 
-    user, domain = re.match("^(\S+)@(\S+)$", email_addr).groups()
+    user = re.match("^(\S+)@(\S+)$", email_addr).groups()[0]
     user = user.replace(".", "-").replace("_", "-")
-    domain_parts = domain.split(".")
 
     person_set = PersonSet()
 
@@ -4561,11 +4560,6 @@
     if _valid_nick(generated_nick):
         return generated_nick
 
-    for domain_part in domain_parts:
-        generated_nick = sanitize_name(generated_nick + "-" + domain_part)
-        if _valid_nick(generated_nick):
-            return generated_nick
-
     # We seed the random number generator so we get consistent results,
     # making the algorithm repeatable and thus testable.
     random_state = random.getstate()

=== added file 'lib/lp/registry/tests/test_nickname.py'
--- lib/lp/registry/tests/test_nickname.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_nickname.py	2011-05-23 19:19:28 +0000
@@ -0,0 +1,59 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for nickname generation"""
+
+__metaclass__ = type
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.model.person import (
+    generate_nick,
+    NicknameGenerationError,
+    )
+from lp.testing import TestCaseWithFactory
+
+
+class TestNicknameGeneration(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_rejects_invalid_emails(self):
+        # generate_nick rejects invalid email addresses
+        self.assertRaises(
+            NicknameGenerationError,
+            generate_nick,
+            'foo@example@com')
+
+    def test_uses_email_address(self):
+        # generate_nick uses the first part of the email address to create
+        # the nick.
+        nick = generate_nick('bar@xxxxxxxxxxx')
+        self.assertEqual('bar', nick)
+
+    def test_does_not_start_nick_with_symbols(self):
+        # If an email starts with symbols, generate_nick still creates a
+        # valid nick that doesn't start with symbols.
+        nick = generate_nick('---bar@xxxxxxxxxxx')
+        self.assertEqual('bar', nick)
+
+    def test_enforces_minimum_length(self):
+        # Nicks must be a minimum of four characters. generate_nick creates
+        # nicks over a that length.
+        nick = generate_nick('i@xxxxxxxxxxx')
+        self.assertEqual('i-5', nick)
+
+    def test_can_create_noncolliding_nicknames(self):
+        # Given the same email address, generate_nick doesn't recreate the
+        # same nick once that nick is used.
+        self._useNicknames(['bar'])
+        nick = generate_nick('bar@xxxxxxxxxxx')
+        self.assertEqual('bar-c', nick)
+
+        self._useNicknames(['bar-c'])
+        self.assertEqual('bar-c', nick)
+
+    def _useNicknames(self, nicknames):
+        # Helper method to consume a nickname
+        for nick in nicknames:
+            self.factory.makePerson(name=nick)
+        #flush_database_updates()