Merge lp:~salgado/launchpad/bug-556594 into lp:launchpad

[Guilherme Salgado]

Fix account-reactivation upon logging in

While fixing bug 556594 I realized we weren't actually reactivating
accounts on login, so I had to fix this first.

As part of this I had to fix the makeAccount() factory method to not
create deactivated accounts with preferred email addresses (as that's
not a valid state for them to be in), and also changed
IAccount.reactivate() to not assert that the password is not empty as
that doesn't matter anymore (the password is no longer used for anything
in LP, but it'll be around until we find the time to get rid of the
Account and AccountPassword tables).

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/database/account.py
  lib/canonical/launchpad/webapp/login.py
  lib/lp/testing/factory.py
  lib/canonical/launchpad/webapp/tests/test_login.py

== Pylint notices ==

lib/canonical/launchpad/database/account.py
    9: [F0401] Unable to import 'zope.component'
    10: [F0401] Unable to import 'zope.security.proxy'
    11: [F0401] Unable to import 'zope.interface'

lib/canonical/launchpad/webapp/login.py
    5: [W0105] String statement has no effect
    21: [F0401] Unable to import 'zope.app.security.interfaces'
    22: [F0401] Unable to import 'zope.component'
    23: [F0401] Unable to import 'zope.event'
    24: [F0401] Unable to import 'zope.interface'
    25: [F0401] Unable to import 'zope.publisher.browser'
    26: [F0401] Unable to import 'zope.publisher.interfaces.http'
    27: [F0401] Unable to import 'zope.security.proxy'
    28: [F0401] Unable to import 'zope.session.interfaces'

lib/lp/testing/factory.py
    33: [F0401] Unable to import 'zope.component'
    34: [F0401] Unable to import 'zope.security.proxy'
    1753: [F0401, LaunchpadObjectFactory.makeRecipe] Unable to import 'bzrlib.plugins.builder.recipe'

lib/canonical/launchpad/webapp/tests/test_login.py
    21: [F0401] Unable to import 'zope.component'
    22: [F0401] Unable to import 'zope.security.management'
    23: [F0401] Unable to import 'zope.security.proxy'
    24: [F0401] Unable to import 'zope.session.interfaces'

[Brad Crittenden (bac)]

Hi Salgado,

This branch looks good. I'd change one thing:
s/'Reactivated by the user herself'/'Reactivated by the user'/

It conveys the full message without getting into the need for a gender-specific pronoun.

review: Approve (code)

[Guilherme Salgado (salgado)]

r10632 fixes the actual bug.

When logging in with an mail address that's associated with a different
OpenID identifier, change the identifier associated with the registered
email address to match the one given by the OP. (Fixes bug 556594)

Part of the changes on this branch have already been reviewed
by Brad, so I'm including the incremental diff here: http://paste.ubuntu.com/410236/

[Brad Crittenden (bac)]

The additional changes look good Salgado. Thanks for the update.

review: Approve (code)

[Francis J. Lacoste (flacoste)]

review: Approve (release-critical)

=== modified file 'lib/canonical/launchpad/database/account.py'
--- lib/canonical/launchpad/database/account.py	2010-02-26 02:33:23 +0000
+++ lib/canonical/launchpad/database/account.py	2010-04-07 12:53:31 +0000
@@ -144,10 +144,6 @@
 
     def reactivate(self, comment, password, preferred_email):
         """See `IAccountSpecialRestricted`."""
-        if password in (None, ''):
-            raise AssertionError(
-                "Account %s cannot be reactivated without a "
-                "password." % self.id)
         self.activate(comment, password, preferred_email)
 
     # The password is actually stored in a separate table for security

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-04-05 17:09:46 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-04-07 12:53:31 +0000
@@ -29,9 +29,11 @@
 
 from z3c.ptcompat import ViewPageTemplateFile
 
+from canonical.cachedproperty import cachedproperty
 from canonical.config import config
 from canonical.launchpad import _
 from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
+from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
 from canonical.launchpad.interfaces.openidconsumer import IOpenIDConsumerStore
 from lp.registry.interfaces.person import IPerson, PersonCreationRationale
 from canonical.launchpad.readonly import is_read_only
@@ -256,6 +258,10 @@
         logInPrincipal(
             self.request, loginsource.getPrincipalByLogin(email), email)
 
+    @cachedproperty
+    def sreg_response(self):
+        return sreg.SRegResponse.fromSuccessResponse(self.openid_response)
+
     def _createAccount(self, openid_identifier):
         # Here we assume the OP sent us the user's email address and
         # full name in the response. Note we can only do that because
@@ -264,21 +270,31 @@
         # asked to.  Once we start using other OPs we won't be able to
         # make this assumption here as they might not include what we
         # want in the response.
-        sreg_response = sreg.SRegResponse.fromSuccessResponse(
-            self.openid_response)
-        assert sreg_response is not None, (
+        assert self.sreg_response is not None, (
             "OP didn't include an sreg extension in the response.")
-        email_address = sreg_response.get('email')
-        full_name = sreg_response.get('fullname')
+        email_address = self.sreg_response.get('email')
+        full_name = self.sreg_response.get('fullname')
         assert email_address is not None and full_name is not None, (
             "No email address or full name found in sreg response; "
             "can't create a new account for this identity URL.")
-        account, email = getUtility(IAccountSet).createAccountAndEmail(
-            email_address,
-            PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
-            full_name,
-            password=None,
-            openid_identifier=openid_identifier)
+        email = getUtility(IEmailAddressSet).getByEmail(email_address)
+        if email is not None:
+            account = email.account
+            assert account is not None, (
+                "This email address should have an associated account.")
+            removeSecurityProxy(account).openid_identifier = openid_identifier
+            if account.status == AccountStatus.DEACTIVATED:
+                comment = 'Reactivated by the user'
+                password = None # Needed just to please reactivate() below.
+                removeSecurityProxy(account).reactivate(
+                    comment, password, removeSecurityProxy(email))
+        else:
+            account, email = getUtility(IAccountSet).createAccountAndEmail(
+                email_address,
+                PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
+                full_name,
+                password=None,
+                openid_identifier=openid_identifier)
         return account
 
     def render(self):
@@ -298,6 +314,20 @@
 
                 if account.status == AccountStatus.SUSPENDED:
                     return self.suspended_account_template()
+                elif account.status == AccountStatus.DEACTIVATED:
+                    comment = 'Reactivated by the user'
+                    password = '' # Needed just to please reactivate() below.
+                    sreg_email = self.sreg_response.get('email')
+                    email_address = getUtility(IEmailAddressSet).getByEmail(
+                        sreg_email)
+                    if email_address is None:
+                        email_address = getUtility(IEmailAddressSet).new(
+                            sreg_email, account=account)
+                    removeSecurityProxy(account).reactivate(
+                        comment, password, removeSecurityProxy(email_address))
+                else:
+                    # Account is active, so nothing to do.
+                    pass
                 if IPerson(account, None) is None:
                     removeSecurityProxy(account).createPerson(
                         PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-03-30 14:57:02 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-04-07 12:53:31 +0000
@@ -199,19 +199,61 @@
         # stuff. 
         self.assertLastWriteIsSet(view.request)
 
+    def test_unseen_identity_with_registered_email(self):
+        # When we get a positive assertion about an identity URL we've never
+        # seen but whose email address is already registered, we just change
+        # the identity URL that's associated with the existing email address.
+        identifier = '4w7kmzU'
+        email = '[e-mail]'
+        account = self.factory.makeAccount(
+            'Test account', email=email, status=AccountStatus.DEACTIVATED)
+        account_set = getUtility(IAccountSet)
+        self.assertRaises(
+            LookupError, account_set.getByOpenIDIdentifier, identifier)
+        openid_response = FakeOpenIDResponse(
+            'http://testopenid.dev/+id/%s' % identifier, status=SUCCESS,
+            email=email, full_name='Foo User')
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view, html = self._createAndRenderView(openid_response)
+        self.assertTrue(view.login_called)
+
+        # The existing account's openid_identifier was updated, the account
+        # was reactivated and its preferred email was set, but its display
+        # name was not changed.
+        self.assertEquals(identifier, account.openid_identifier)
+        self.assertEquals(AccountStatus.ACTIVE, account.status)
+        self.assertEquals(
+            email, removeSecurityProxy(account.preferredemail).email)
+        person = IPerson(account, None)
+        self.assertIsNot(None, person)
+        self.assertEquals('Test account', person.displayname)
+
+        # We also update the last_write flag in the session, to make sure
+        # further requests use the master DB and thus see the newly created
+        # stuff. 
+        self.assertLastWriteIsSet(view.request)
+
     def test_deactivated_account(self):
         # The user has the account's password and is trying to login, so we'll
         # just re-activate their account.
+        email = '[e-mail]'
         account = self.factory.makeAccount(
-            'Test account', status=AccountStatus.DEACTIVATED)
+            'Test account', email=email, status=AccountStatus.DEACTIVATED)
         self.assertIs(None, IPerson(account, None))
-        view, html = self._createViewWithResponse(account)
+        openid_identifier = removeSecurityProxy(account).openid_identifier
+        openid_response = FakeOpenIDResponse(
+            'http://testopenid.dev/+id/%s' % openid_identifier,
+            status=SUCCESS, email=email, full_name=account.displayname)
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view, html = self._createAndRenderView(openid_response)
         self.assertIsNot(None, IPerson(account, None))
         self.assertTrue(view.login_called)
         response = view.request.response
         self.assertEquals(httplib.TEMPORARY_REDIRECT, response.getStatus())
         self.assertEquals(view.request.form['starting_url'],
                           response.getHeader('Location'))
+        self.assertEquals(AccountStatus.ACTIVE, account.status)
+        self.assertEquals(email, account.preferredemail.email)
         # We also update the last_write flag in the session, to make sure
         # further requests use the master DB and thus see the newly created
         # stuff. 

=== modified file 'lib/lp/registry/doc/person-account.txt'
--- lib/lp/registry/doc/person-account.txt	2010-01-19 14:43:13 +0000
+++ lib/lp/registry/doc/person-account.txt	2010-04-07 12:53:31 +0000
@@ -208,27 +208,16 @@
 
 == Reactivating user accounts ==
 
-Accounts can be reactivated. A comment is required to log why this was
-done, and a password is required too. There are two preconditions before
-this method can be called on an Account: The password and the preferred
-email address cannot not be None.
-
-    >>> foobar.account.reactivate(
-    ...     'This will raise an error.', password=None, preferred_email=None)
-    Traceback (most recent call last):
-     ...
-    AssertionError: Account ... cannot be reactivated without a password.
-
-    >>> foobar.account.reactivate(
-    ...     'This will raise an error.', password='ok', preferred_email=None)
+Accounts can be reactivated. A comment and a non-None preferred email address
+are required to reactivate() an account, though.
+
+    >>> foobar.account.reactivate(
+    ...     'This will raise an error.', password='', preferred_email=None)
     Traceback (most recent call last):
      ...
     AssertionError: Account ... cannot be activated without a preferred
                     email address.
 
-The account can be reactivated after the password and preferred email
-address are set.
-
     >>> foobar.reactivate(
     ...     'User reactivated the account using reset password.',
     ...     password="ok",

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-04-05 17:40:35 +0000
+++ lib/lp/testing/factory.py	2010-04-07 12:53:31 +0000
@@ -340,9 +340,11 @@
         removeSecurityProxy(account).status = status
         if email is None:
             email = self.getUniqueEmailAddress()
+        email_status = EmailAddressStatus.PREFERRED
+        if status != AccountStatus.ACTIVE:
+            email_status = EmailAddressStatus.NEW
         email = self.makeEmail(
-            email, person=None, account=account,
-            email_status=EmailAddressStatus.PREFERRED)
+            email, person=None, account=account, email_status=email_status)
         if commit:
             transaction.commit()
         return account