Merge lp:~jcsackett/launchpad/dont-expose-domains into lp:launchpad

[j.c.sackett]

[r=benji][bug=228355] Removes the use of email domain information in the creation of nicks in generate_nick

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. <email address hidden> 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. <email address hidden>). 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

[Benji York (benji)]

The conversion from doctest to unittest looks like it could use some
more fleshing out.

Two of the scenarios from the doctest that look important aren't
represented in the unittests:

    >>> generate_nick("<email address hidden>")
    'foo-bar'
    >>> generate_nick("<email address hidden>")
    'foo+bar'

The second half of the doctest (starting with "It should be nearly
impossible to..." and going through the testing of
NicknameGenerationError) looks important too and those tests aren't
represented in the unittests.

Here are some smaller things that occurred to me during the review:

The regex in lib/lp/registry/model/person.py doesn't need the second set
of capturing parens now that we're not using the domain name.

These two lines from the end of test_can_create_noncolliding_nicknames
appear to be copypasta (and therefore can be removed):

    self._useNicknames(['bar-c'])
    self.assertEqual('bar-c', nick)

It would be nice if test_enforces_minimum_length asserts that "i" isn't
registered at test start to ensure that we're really seeing
nick-too-short behavior and not nick-already-taken behavior.

[j.c.sackett (jcsackett)]

> The conversion from doctest to unittest looks like it could use some
> more fleshing out.
>
> Two of the scenarios from the doctest that look important aren't
> represented in the unittests:
>
> >>> generate_nick("<email address hidden>")
> 'foo-bar'
> >>> generate_nick("<email address hidden>")
> 'foo+bar'

Fair, I've updated the symbol handling test to show several versions.

> The second half of the doctest (starting with "It should be nearly
> impossible to..." and going through the testing of
> NicknameGenerationError) looks important too and those tests aren't
> represented in the unittests.

They're not anymore; that part tests the use of email domains as part of what's being used for nick generation to the point where random prefixes and suffixes start being used. Now we skip straight to the randomized stuff, and that's tested in the non-colliding part.

> Here are some smaller things that occurred to me during the review:
>
> The regex in lib/lp/registry/model/person.py doesn't need the second set
> of capturing parens now that we're not using the domain name.

Correct, I've updated the second group to noncapturing.

> These two lines from the end of test_can_create_noncolliding_nicknames
> appear to be copypasta (and therefore can be removed):
>
> self._useNicknames(['bar-c'])
> self.assertEqual('bar-c', nick)

Actually, that was supposed to be a test to show what happened if it got another bar@ email address after one collision. It's been updated.

> It would be nice if test_enforces_minimum_length asserts that "i" isn't
> registered at test start to ensure that we're really seeing
> nick-too-short behavior and not nick-already-taken behavior.

Done.

[Benji York (benji)]

Thanks for the fixes and for fixing my misconceptions. Some parting
thoughts:

The bit verifying that "i" name hasn't been used could use a comment as
to what's going on. Similarly for the second half of
test_can_create_noncolliding_nicknames where you're verifying that
prefixes are added.

Also, I think the last line of _useNicknames is commented out code that
you meant to remove.

review: Approve (code)

=== 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("[e-mail]")
-    'foop'
-    >>> generate_nick("[e-mail]")
-    'bar'
-    >>> generate_nick("[e-mail]")
-    'spam'
-    >>> generate_nick("[e-mail]")
-    'foo-bar'
-    >>> generate_nick("[e-mail]")
-    'foo+bar'
-    >>> generate_nick("[e-mail]")
-    'foo'
-
-We already have a salgado, so generate_nick needs to try a little harder.
-
-    >>> generate_nick("[e-mail]")
-    '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 = (
-    ...             '[e-mail]' % _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('[e-mail]').name
-    u'bongo'
-    >>> new_person('[e-mail]').name
-    u'bongo-example'
-    >>> new_person('[e-mail]').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('[e-mail]').name
-    u'bongo-example-com-c'
-
-Random prefixes are tried:
-
-    >>> new_person('[e-mail]').name
-    u'q-bongo-example-com'
-
-And random mutations are tried:
-
-    >>> new_person('[e-mail]').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("[e-mail]")
-    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-24 13:43:18 +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-24 13:43:18 +0000
@@ -0,0 +1,74 @@
+# 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 zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.interfaces.person import IPersonSet
+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('[e-mail]')
+        self.assertEqual('bar', nick)
+
+    def test_handles_symbols(self):
+        # If an email starts with symbols, generate_nick still creates a
+        # valid nick that doesn't start with symbols.
+        nicks = [generate_nick(email) for email in [
+                                            '[e-mail]',
+                                            '[e-mail]',
+                                            '[e-mail]',
+                                            '[e-mail]',
+                                            ]]
+        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.
+        # The nick 'i' isn't used, so we know any additional prefi
+        person = getUtility(IPersonSet).getByName('i')
+        self.assertIs(None, person)
+        nick = generate_nick('[e-mail]')
+        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('[e-mail]')
+        self.assertEqual('bar-c', nick)
+
+        # If we used the previously created nick and get another bar@ email
+        # address, another new nick is generated.
+        self._useNicknames(['bar-c'])
+        nick = generate_nick('[e-mail]')
+        self.assertEqual('a-bar', nick)
+
+    def _useNicknames(self, nicknames):
+        # Helper method to consume a nickname
+        for nick in nicknames:
+            self.factory.makePerson(name=nick)