Merge lp:~canonical-isd-hackers/canonical-identity-provider/password-db-entry-missing into lp:canonical-identity-provider/release

[Szilveszter Farkas]

[Łukasz Czyżykowski (lukasz-czyzykowski)]

- why in test_authenticate_account_no_password you're getting object again? looks like this was asserted in the beginning of the test

- in test_reset_password_when_account_{active, active_no_password} I would rather see account.status = AccountStatus.ACTIVE; account.save() instead of the assert, as this more in line of what is checked in those tests

- I feel that the logic in views.ui for accessing account password should be moved to account model

[Szilveszter Farkas (phanatic)]

1) I wanted to make sure that the Account object isn't deleted (there's a OneToOne relationship between Account and AccountPassword).

2) That's something not introduced by my changes, but I'm happy to change that.

3) We discussed that on IRC.

[Łukasz Czyżykowski (lukasz-czyzykowski)]

As we discussed on IRC, there's no clean way of fixing that in Django and current db schema.

review: Approve

=== modified file 'identityprovider/auth.py'
--- identityprovider/auth.py	2010-04-21 15:29:24 +0000
+++ identityprovider/auth.py	2010-05-11 12:40:55 +0000
@@ -3,7 +3,8 @@
 
 from datetime import datetime
 
-from identityprovider.models import Account, EmailAddress, Consumer, Token
+from identityprovider.models import (Account, AccountPassword, EmailAddress,
+    Consumer, Token)
 from identityprovider.models.api import APIUser
 from identityprovider.models.const import EmailStatus
 from identityprovider.utils import validate_launchpad_password
@@ -27,7 +28,11 @@
         if not account.is_active:
             return None
 
-        encrypted = account.accountpassword.password
+        try:
+            encrypted = account.accountpassword.password
+        except AccountPassword.DoesNotExist:
+            # password lost somehow, user should reset it
+            return None
         if validate_launchpad_password(password, encrypted):
             account.last_login = datetime.now()
             return account

=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2010-05-04 22:26:22 +0000
+++ identityprovider/models/account.py	2010-05-11 12:40:55 +0000
@@ -155,7 +155,8 @@
 
     @property
     def can_reactivate(self):
-        return (self.status == AccountStatus.DEACTIVATED)
+        return (self.status == AccountStatus.DEACTIVATED
+            or self.status == AccountStatus.NOACCOUNT)
 
     @property
     def can_reset_password(self):

=== modified file 'identityprovider/tests/test_auth.py'
--- identityprovider/tests/test_auth.py	2010-04-21 15:29:24 +0000
+++ identityprovider/tests/test_auth.py	2010-05-11 12:40:55 +0000
@@ -71,6 +71,16 @@
         account.status = _status
         account.save()
 
+    def test_authenticate_account_no_password(self):
+        account = Account.objects.get_by_email('[e-mail]')
+        account.accountpassword.delete()
+        
+        response = self.backend.authenticate('[e-mail]', 'test')
+        
+        self.assertTrue(response is None)
+        account = Account.objects.get_by_email('[e-mail]')
+        self.assertTrue(account is not None)
+
     def test_oauth_authenticate_account_active(self):
         account = Account.objects.get_by_email('[e-mail]')
         consumer, created = Consumer.objects.get_or_create(account=account)

=== modified file 'identityprovider/tests/test_views_ui.py'
--- identityprovider/tests/test_views_ui.py	2010-05-10 16:59:23 +0000
+++ identityprovider/tests/test_views_ui.py	2010-05-11 12:40:55 +0000
@@ -32,6 +32,12 @@
     def setUp(self):
         logging.disable(logging.WARNING)
 
+        # disable csrf
+        self.disable_csrf()
+
+    def tearDown(self):
+        self.reset_csrf()
+
     def reset_csrf(self):
         settings.MIDDLEWARE_CLASSES = self._MIDDLEWARE_CLASSES
 
@@ -136,9 +142,6 @@
             email_obj.status = EmailStatus.NEW
             email_obj.save()
 
-        # disable csrf
-        self.disable_csrf()
-
         r = self.client.post('/+forgot_password',
                              {'email': '[e-mail]'})
 
@@ -154,7 +157,6 @@
             email_obj.status = EmailStatus.VALIDATED
             email_obj.save()
         account.preferredemail = _preferred_email
-        self.reset_csrf()
 
     def test_claim_token_for_validate_email(self):
         token = self.create_token(at.LoginTokenType.VALIDATEEMAIL)
@@ -169,7 +171,6 @@
 
     def test_config_email(self):
         self.authenticate()
-        self.disable_csrf()
 
         token = self.create_token(at.LoginTokenType.VALIDATEEMAIL,
                                   email="[e-mail]",
@@ -178,8 +179,6 @@
             {'post': 'yes'})
         self.assertRedirects(r, token.redirection_url)
 
-        self.reset_csrf()
-
     def test_token_from_email_when_it_is_turned_on(self):
         r = self.get_debug_token_response(True)
         self.assertTemplateUsed(r, 'registration/token.html')
@@ -215,9 +214,6 @@
         self.assertTemplateUsed(r, 'registration/forgot_password.html')
 
     def test_forgot_password_email_when_account_active(self):
-        # disable CSRF
-        self.disable_csrf()
-
         account = Account.objects.get_by_email('[e-mail]')
 
         r = self.client.post('/+forgot_password',
@@ -226,14 +222,9 @@
         self.assertEqual(len(mail.outbox), 1)
         mail.outbox = []
 
-        self.reset_csrf()
-
     def test_forgot_password_email_when_account_deactivated(self):
         """ Deactivated accounts can be reactivated using the reset password
         functionality. Bug #556878 """
-        # disable CSRF
-        self.disable_csrf()
-
         account = Account.objects.get_by_email('[e-mail]')
         # make sure the account is deactivated
         _status = account.status
@@ -247,61 +238,69 @@
         mail.outbox = []
         self.assertEqual(account.preferredemail.email, '[e-mail]')
 
-        account.status = _status
-        account.save()
-
-        self.reset_csrf()
-
-    def test_forgot_password_email_when_account_inactive(self):
-        # disable CSRF
-        self.disable_csrf()
-
-        account = Account.objects.get_by_email('[e-mail]')
-
-        for status, _ in AccountStatus._get_choices():
-            if status in (AccountStatus.ACTIVE, AccountStatus.DEACTIVATED):
-                # skip as this is already tested in
-                # test_forgot_password_email_when_account_active and
-                # test_forgot_password_email_when_account_deactivated
-                continue
-
-            account.status = status
-            account.save()
-
-            r = self.client.post('/+forgot_password',
-                                 {'email': '[e-mail]'})
-
-            self.assertEqual(len(mail.outbox), 0)
-            mail.outbox = []
-
-        self.reset_csrf()
+    def test_forgot_password_email_when_account_noaccount(self):
+        account = Account.objects.get_by_email('[e-mail]')
+        # make sure the account is deactivated
+        account.status = AccountStatus.NOACCOUNT
+        account.save()
+
+        r = self.client.post('/+forgot_password',
+                             {'email': '[e-mail]'})
+
+        self.assertEqual(len(mail.outbox), 1)
+        mail.outbox = []
+        self.assertEqual(account.preferredemail.email, '[e-mail]')
+
+    def test_forgot_password_email_when_account_suspended(self):
+        account = Account.objects.get_by_email('[e-mail]')
+
+        # make sure the account is suspended
+        _status = account.status
+        account.status = AccountStatus.SUSPENDED
+        account.save()
+
+        r = self.client.post('/+forgot_password',
+                             {'email': '[e-mail]'})
+
+        self.assertEqual(len(mail.outbox), 0)
+        mail.outbox = []
 
     def test_reset_password_when_account_active(self):
-        # disable CSRF
-        self.disable_csrf()
-
-        r = self.client.post('/+forgot_password',
-                             {'email': '[e-mail]'})
-        # claim token
-        token = self.client.session['token_forgotpassword']
-
-        account = Account.objects.get_by_email('[e-mail]')
-        # make sure the account is active
-        self.assertEqual(account.status, AccountStatus.ACTIVE)
-
-        # confirm account
-        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
-        r = self.client.post('/token/%s/+resetpassword' % token, data)
-        self.assertRedirects(r, '/')
-
-        self.reset_csrf()
+        r = self.client.post('/+forgot_password',
+                             {'email': '[e-mail]'})
+        # claim token
+        token = self.client.session['token_forgotpassword']
+
+        account = Account.objects.get_by_email('[e-mail]')
+        # make sure the account is active
+        account.status = AccountStatus.ACTIVE
+        account.save()
+
+        # confirm account
+        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
+        r = self.client.post('/token/%s/+resetpassword' % token, data)
+        self.assertRedirects(r, '/')
+
+    def test_reset_password_when_account_active_no_password(self):
+        r = self.client.post('/+forgot_password',
+                             {'email': '[e-mail]'})
+        # claim token
+        token = self.client.session['token_forgotpassword']
+
+        account = Account.objects.get_by_email('[e-mail]')
+        account.accountpassword.delete()
+        # make sure the account is active
+        account.status = AccountStatus.ACTIVE
+        account.save()
+
+        # confirm account
+        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
+        r = self.client.post('/token/%s/+resetpassword' % token, data)
+        self.assertRedirects(r, '/')
 
     def test_reset_password_when_account_deactivated(self):
         """ Deactivated accounts can be reactivated using the reset password
         functionality. Bug #556878 """
-        # disable CSRF
-        self.disable_csrf()
-
         r = self.client.post('/+forgot_password',
                              {'email': '[e-mail]'})
         # claim token
@@ -309,7 +308,6 @@
 
         account = Account.objects.get_by_email('[e-mail]')
         # make sure the account is deactivated
-        _status = account.status
         account.status = AccountStatus.DEACTIVATED
         account.save()
 
@@ -318,18 +316,9 @@
         r = self.client.post('/token/%s/+resetpassword' % token, data)
         self.assertRedirects(r, '/')
 
-        account.status = _status
-        account.save()
-
-        self.reset_csrf()
-
     def test_reset_password_when_account_deactivated_no_preferred_email(self):
-        # disable CSRF
-        self.disable_csrf()
-
         account = Account.objects.get_by_email('[e-mail]')
         # make sure the account is deactivated and preferred email is removed
-        _status = account.status
         _preferred_email = account.preferredemail
         account.status = AccountStatus.DEACTIVATED
         account.save()
@@ -358,40 +347,38 @@
             email_obj.status = EmailStatus.VALIDATED
             email_obj.save()
         account.preferredemail = _preferred_email
-        account.status = _status
-        account.save()
-
-        self.reset_csrf()
-
-    def test_reset_password_when_account_inactive(self):
-        # disable CSRF
-        self.disable_csrf()
-
-        r = self.client.post('/+forgot_password',
-                             {'email': '[e-mail]'})
-        # claim token
-        token = self.client.session['token_forgotpassword']
-
-        account = Account.objects.get_by_email('[e-mail]')
-        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
-
-        # test by account status
-        for status, _ in AccountStatus._get_choices():
-            if status in (AccountStatus.ACTIVE, AccountStatus.DEACTIVATED):
-                # skip as this is tested in
-                # test_reset_password_when_account_active and
-                # test_reset_password_when_account_deactivateds
-                continue
-
-            # change account status
-            account.status = status
-            account.save()
-
-            # confirm account
-            r = self.client.post('/token/%s/+resetpassword' % token, data)
-            self.assertRedirects(r, '/+bad-token')
-
-        self.reset_csrf()
+
+    def test_reset_password_when_account_noaccount(self):
+        r = self.client.post('/+forgot_password',
+                             {'email': '[e-mail]'})
+        # claim token
+        token = self.client.session['token_forgotpassword']
+
+        account = Account.objects.get_by_email('[e-mail]')
+        # make sure the account is deactivated
+        account.status = AccountStatus.NOACCOUNT
+        account.save()
+
+        # confirm account
+        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
+        r = self.client.post('/token/%s/+resetpassword' % token, data)
+        self.assertRedirects(r, '/')
+
+    def test_reset_password_when_account_suspended(self):
+        r = self.client.post('/+forgot_password',
+                             {'email': '[e-mail]'})
+        # claim token
+        token = self.client.session['token_forgotpassword']
+
+        account = Account.objects.get_by_email('[e-mail]')
+        account.status = AccountStatus.SUSPENDED
+        account.save()
+
+        data = {'password': 'Password1', 'passwordconfirm': 'Password1'}
+
+        # confirm account
+        r = self.client.post('/token/%s/+resetpassword' % token, data)
+        self.assertRedirects(r, '/+bad-token')
 
     def request_when_captcha_fails(self, url, data):
         class MockCaptcha(object):
@@ -424,9 +411,6 @@
         self.assertTemplateUsed(r, 'registration/new_account.html')
 
     def test_new_account_when_inactive(self):
-        # disable CSRF
-        self.disable_csrf()
-
         account = Account.objects.get_by_email('[e-mail]')
 
         for status, _ in AccountStatus._get_choices():
@@ -445,8 +429,6 @@
             self.assertEqual(len(mail.outbox), mails_sent)
             mail.outbox = []
 
-        self.reset_csrf()
-
     def test_edit_account_template(self):
         r = self.client.post('/+login', {'email': '[e-mail]',
                                          'password': 'test',
@@ -511,7 +493,6 @@
         self.assertEqual(email.status, EmailStatus.NEW)
 
     def test_confirm_email_post(self):
-        self.disable_csrf()
         self.authenticate()
 
         account, email, token = self._setup_account_with_new_email()
@@ -526,8 +507,6 @@
         email = EmailAddress.objects.get(email=email.email)
         self.assertEqual(email.status, EmailStatus.VALIDATED)
 
-        self.reset_csrf()
-
     def test_confirm_email_as_another_user_fails(self):
         self.authenticate()
         account = Account.objects.get_by_email('[e-mail]')

=== modified file 'identityprovider/views/ui.py'
--- identityprovider/views/ui.py	2010-05-05 13:11:46 +0000
+++ identityprovider/views/ui.py	2010-05-11 12:40:55 +0000
@@ -22,8 +22,8 @@
 from identityprovider.decorators import guest_required, dont_cache, limitlogin
 from identityprovider.forms import (LoginForm, ForgotPasswordForm,
     ResetPasswordForm, NewAccountForm, ConfirmNewAccountForm)
-from identityprovider.models import (Account, AuthToken, AuthTokenFactory,
-    EmailAddress)
+from identityprovider.models import (Account, AccountPassword, AuthToken,
+    AuthTokenFactory, EmailAddress)
 from identityprovider.models.const import (AccountStatus, EmailStatus,
     LoginTokenType)
 import identityprovider.signed as signed
@@ -423,7 +423,13 @@
                 if not created:
                     account.preferredemail = email_obj
             email = account.preferredemail.email
-            password_obj = account.accountpassword
+            try:
+                password_obj = account.accountpassword
+            except AccountPassword.DoesNotExist:
+                password_obj = AccountPassword.objects.create(
+                    account=account,
+                    password='invalid',
+                )
             password_obj.password = encrypt_launchpad_password(password)
             password_obj.save()
             user = auth.authenticate(username=email, password=password)