Merge lp:~twom/canonical-identity-provider/confirm-password-before-changing into lp:canonical-identity-provider/release

[Tom Wardill]

Add a confirm password box to the edit account details screen.

Adds a confirm password box to the edit account details screen, required for changing any account details (email, display name, password, 2fa options).

As the password boxes are handled by a mixin, allow the mixin to take a parameter, as we don't want a 'forgotten password' reset to require the password that you just forgot.

[Maximiliano Bertacchini (maxiberta)]

LGTM, with a couple of nitpicks. Ran locally and tested:

- Create account - new password and new password confirmation are required as usual.
- Edit account - current password confirmation is required as expected.
- Edit account - POST with no "oldpassword" correctly validated server-side.
- Forgot password - no current password is requested.

Would be great to hear a second opinion, just in case. Thanks!

review: Approve

[Colin Watson (cjwatson)]

If the user has 2FA enabled, is that also checked? I don't see any obvious mention of that in this MP, but I haven't tried running it so I might have missed something.

[Tom Wardill (twom)]

> If the user has 2FA enabled, is that also checked? I don't see any obvious
> mention of that in this MP, but I haven't tried running it so I might have
> missed something.

2FA is not checked before password change. It's something I considered, but on checking around (Google, Github, Dropbox), other sites don't seem to require it for a password change.

I guess you're already at '2FA' (ish) if you're in a position to be able to change someone else's password, you'd need a working session _and_ the current password.

[Daniel Manrique (roadmr)]

Looks OK code wise.

I played with this a bit UI-wise and it seems fine, I don't like that it asks for the password for everything (e.g. I wanted to switch 2FA policy to "always ask" and I was asked for the password anyway) - OTOH it's not a stretch to require that users know their password at all times :) so this is slightly more inconvenient but answers the security concern that prompted these changes.

review: Approve

=== modified file 'src/identityprovider/forms.py'
--- src/identityprovider/forms.py	2018-05-28 19:44:56 +0000
+++ src/identityprovider/forms.py	2018-12-13 16:29:16 +0000
@@ -6,6 +6,7 @@
 
 from django import forms
 from django.conf import settings
+from django.contrib.auth.hashers import check_password
 from django.forms import Form, fields, widgets
 from django.urls import reverse
 from django.utils import translation
@@ -105,9 +106,12 @@
 
     def __init__(self, *args, **kwargs):
         self.account = kwargs.pop('account', None)
+        self.require_confirmation = kwargs.pop('require_confirmation', True)
         super(ResetPasswordFormMixin, self).__init__(*args, **kwargs)
 
         validate_policy = get_password_validator(self.account)
+        if self.require_confirmation:
+            self.fields['oldpassword'] = PasswordField()
         self.fields['password'] = PasswordField(
             placeholder=validate_policy.PASSWORD_POLICY_HELP_TEXT,
             help_text=validate_policy.PASSWORD_POLICY_HELP_TEXT,
@@ -116,6 +120,14 @@
         self.fields['passwordconfirm'] = PasswordField(
             placeholder=_('Retype password'))
 
+    def clean_oldpassword(self):
+        old_password = self.cleaned_data.get('oldpassword')
+        if (not old_password and self.data.get('oldpassword')):
+            raise forms.ValidationError(FIELD_REQUIRED)
+        if not check_password(old_password, self.account.encrypted_password):
+            raise forms.ValidationError(_("Incorrect password"))
+        return old_password
+
     def clean_password(self):
         password = self.cleaned_data.get('password')
         if (not password and self.data.get('passwordconfirm')):
@@ -124,7 +136,7 @@
 
     def clean_passwordconfirm(self):
         password = self.cleaned_data.get('passwordconfirm')
-        if (not password and self.data.get('passwordconfirm')):
+        if (not password and self.data.get('password')):
             raise forms.ValidationError(FIELD_REQUIRED)
         return password
 
@@ -141,7 +153,10 @@
 
 
 class ResetPasswordForm(ResetPasswordFormMixin, forms.Form):
-    pass
+
+    def __init__(self, *args, **kwargs):
+        kwargs['require_confirmation'] = False
+        super(ResetPasswordForm, self).__init__(*args, **kwargs)
 
 
 class NewAccountForm(GenericEmailForm, ResetPasswordForm):
@@ -155,6 +170,7 @@
     def __init__(self, *args, **kwargs):
         self.require_username = kwargs.pop('require_username', False)
         self.require_password = kwargs.pop('require_password', True)
+        kwargs['require_confirmation'] = False
         super(NewAccountForm, self).__init__(*args, **kwargs)
         # validate password against email
         leak_creds_validator = get_password_validator(

=== modified file 'src/identityprovider/tests/sso_server/test_home_page.py'
--- src/identityprovider/tests/sso_server/test_home_page.py	2015-06-29 20:44:44 +0000
+++ src/identityprovider/tests/sso_server/test_home_page.py	2018-12-13 16:29:16 +0000
@@ -58,6 +58,7 @@
         # The user can edit their details from the main account page:
         data = dict(
             displayname="New name",
+            oldpassword=self.default_password,
             password="TestPass23", passwordconfirm="TestPass23",
             preferred_email=self.account.preferredemail.id,
         )

=== modified file 'src/identityprovider/tests/test_forms.py'
--- src/identityprovider/tests/test_forms.py	2018-05-25 21:47:30 +0000
+++ src/identityprovider/tests/test_forms.py	2018-12-13 16:29:16 +0000
@@ -426,7 +426,8 @@
 
 class EditAccountFormTestCase(FormBaseTestCase):
 
-    fields = ['displayname', 'preferred_email', 'password', 'passwordconfirm']
+    fields = ['displayname', 'preferred_email',
+              'oldpassword', 'password', 'passwordconfirm']
 
     def setUp(self):
         super(EditAccountFormTestCase, self).setUp()
@@ -471,6 +472,7 @@
             password='foobarbaz', passwordconfirm='foobarbaz',
             displayname='Foo Bar',
             preferred_email=self.account.preferredemail.id,
+            oldpassword=DEFAULT_USER_PASSWORD,
         )
         form = EditAccountForm(data=data, instance=self.account)
         self.assertEqual(form.is_valid(), True)
@@ -526,7 +528,8 @@
         invalid_email = self.account.unverified_emails()[0]
         form = self.get_form(
             data={'displayname': self.account.displayname,
-                  'preferred_email': invalid_email.id})
+                  'preferred_email': invalid_email.id,
+                  'oldpassword': DEFAULT_USER_PASSWORD})
         self.assert_form_is_valid(form)
         self.assertFalse(form.password_changed)
         form.save()
@@ -570,6 +573,7 @@
         original_value = self.account.accountpassword.password
         form = self.get_form(
             data={'password': 'testingpass', 'passwordconfirm': 'testingpass',
+                  'oldpassword': DEFAULT_USER_PASSWORD,
                   'displayname': self.account.displayname,
                   'preferred_email': preferred_email})
         self.assert_form_is_valid(form)
@@ -578,6 +582,15 @@
         account = Account.objects.get(id=self.account.id)
         self.assertNotEqual(account.accountpassword.password, original_value)
 
+    def test_confirm_change_password(self):
+        preferred_email = self.account.preferredemail.id
+        form = self.get_form(
+            data={'password': 'testingpass', 'passwordconfirm': 'testingpass',
+                  'displayname': self.account.displayname,
+                  'preferred_email': preferred_email})
+        self.assertFalse(form.is_valid())
+        self.assertEqual(1, len(form.errors['oldpassword']))
+
     def test_password_change_resets_last_leak_check(self):
         preferred_email = self.account.preferredemail.id
         last_checked = now()
@@ -587,6 +600,7 @@
                          last_checked)
         form = self.get_form(
             data={'password': 'testingpass', 'passwordconfirm': 'testingpass',
+                  'oldpassword': DEFAULT_USER_PASSWORD,
                   'displayname': self.account.displayname,
                   'preferred_email': preferred_email})
         self.assert_form_is_valid(form)
@@ -611,6 +625,7 @@
         data = dict(
             preferred_email=self.account.preferredemail.id,
             displayname='Zaraza New',
+            oldpassword=DEFAULT_USER_PASSWORD,
         )
         form = self.get_form(data=data)
 
@@ -624,7 +639,8 @@
 class EditAccountFormDevicePrefsEnabledTestCase(EditAccountFormTestCase):
 
     fields = ['displayname', 'preferred_email', 'password', 'passwordconfirm',
-              'warn_about_backup_device', 'twofactor_required']
+              'warn_about_backup_device', 'twofactor_required',
+              'oldpassword']
     placeholders = ['displayname', 'password', 'passwordconfirm']
 
     def get_form(self, **kwargs):
@@ -654,6 +670,7 @@
             twofactor_required=new_value,
             preferred_email=self.account.preferredemail.id,
             displayname=self.account.displayname,
+            oldpassword=DEFAULT_USER_PASSWORD,
         )
         form = self.get_form(data=data)
 
@@ -683,6 +700,7 @@
             warn_about_backup_device=new_value,
             preferred_email=self.account.preferredemail.id,
             displayname=self.account.displayname,
+            oldpassword=DEFAULT_USER_PASSWORD,
         )
         form = self.get_form(data=data)
 
@@ -706,6 +724,7 @@
         data = dict(
             preferred_email=self.account.preferredemail.id,
             displayname='Zaraza New',
+            oldpassword=DEFAULT_USER_PASSWORD,
             twofactor_required=False,
         )
         form = self.get_form(data=data)
@@ -739,6 +758,7 @@
         data = {
             'preferred_email': self.account.preferredemail.id,
             'displayname': self.account.displayname,
+            'oldpassword': DEFAULT_USER_PASSWORD,
             'twofactor_required': False,
             'warn_about_backup_device': False,
         }

=== modified file 'src/identityprovider/tests/test_views_server.py'
--- src/identityprovider/tests/test_views_server.py	2018-09-07 14:19:01 +0000
+++ src/identityprovider/tests/test_views_server.py	2018-12-13 16:29:16 +0000
@@ -1352,7 +1352,8 @@
         self.assertTemplateUsed(response, 'account/edit.html')
         self.assert_message_shown(response, messages.ERROR, msg)
         self.assertEqual(response.context['form'].errors,
-                         {'username': ['Field required']})
+                         {'username': [u'Field required'],
+                          'oldpassword': [u'Field required']})
 
     @switches(USERNAME_UI=True)
     def test_nickname_required__no_username__flag_enabled__post_valid(self):
@@ -1362,6 +1363,7 @@
             'displayname': self.account.displayname,
             'preferred_email': self.account.preferredemail.id,
             'username': 'test-valid+username.',
+            'oldpassword': self.login_password,
         }
         response = self.client.post(self.url, data, follow=True)
         # Simulate username set in Launchpad.

=== modified file 'src/webui/templates/widgets/passwords.html'
--- src/webui/templates/widgets/passwords.html	2014-12-10 15:30:54 +0000
+++ src/webui/templates/widgets/passwords.html	2018-12-13 16:29:16 +0000
@@ -1,4 +1,19 @@
 {% load i18n %}
+{% if fields.oldpassword %}
+<p>{% trans "To edit any details on this page, you must confirm your current password." %}</p>
+
+<div class="input-row{% if fields.oldpassword.errors or fields.non_field_errors %} haserrors{% endif %} oldpassword-input">
+    {% if edit_account_labels %}
+        <label for="id_oldpassword">{% trans "Current password" %}:</label>
+    {% endif %}
+
+    {% if fields.oldpassword.errors %}
+        <span class="error">{{ fields.oldpassword.errors|first }}</span>
+    {% endif %}
+
+    {{ fields.oldpassword }}
+</div>
+{% endif %}
 
 <div class="input-row{% if fields.password.errors or fields.non_field_errors %} haserrors{% endif %} password-input"
      id="password-change">

=== modified file 'src/webui/tests/test_views_account.py'
--- src/webui/tests/test_views_account.py	2018-05-28 20:15:33 +0000
+++ src/webui/tests/test_views_account.py	2018-12-13 16:29:16 +0000
@@ -261,13 +261,15 @@
 
     def test_index_edit_displayname(self):
         data = {'displayname': "New Display Name",
-                'preferred_email': self.account.preferredemail.id}
+                'preferred_email': self.account.preferredemail.id,
+                'oldpassword': DEFAULT_USER_PASSWORD}
         r = self.client.post(self.url, data)
         self.assertEqual(r.status_code, 302)
 
     def test_index_edit_displayname_no_email(self):
         data = {'displayname': "New Display Name",
-                'preferred_email': self.account.preferredemail.id}
+                'preferred_email': self.account.preferredemail.id,
+                'oldpassword': DEFAULT_USER_PASSWORD}
         r = self.client.post(self.url, data)
         self.assertEqual(r.status_code, 302)
         # no mail when editing this attribute
@@ -287,6 +289,7 @@
     def post_username_change(self, username, flush=True):
         data = {'displayname': self.account.displayname,
                 'preferred_email': self.account.preferredemail.id,
+                'oldpassword': DEFAULT_USER_PASSWORD,
                 'username': username}
         r = self.client.post(self.url, data)
         if flush:
@@ -415,6 +418,7 @@
         data = {
             'displayname': "New Display Name",
             'preferred_email': self.account.preferredemail.id,
+            'oldpassword': DEFAULT_USER_PASSWORD,
             'password': 'new-Password',
             'passwordconfirm': 'new-Password',
             'accept_tos': True
@@ -651,6 +655,35 @@
             username=self.login_email, password=DEFAULT_USER_PASSWORD)
         self.assertFalse(success)
 
+    def test_confirm_password_before_changing(self):
+        oauth_tokens = self.account.token_set.all()
+        # web login token should be there
+        assert oauth_tokens
+        orig_token = oauth_tokens[0]
+
+        data = {
+            'displayname': "New Display Name",
+            'preferred_email': self.account.preferredemail.id,
+            'password': 'new-Password',
+            'passwordconfirm': 'new-Password',
+            'accept_tos': True
+        }
+        response = self.client.post(self.url, data)
+        self.assertEqual(
+            response.context['form'].errors,
+            {'oldpassword': [u'Field required']})
+
+        data['oldpassword'] = DEFAULT_USER_PASSWORD
+
+        response = self.client.post(self.url, data)
+        self.assertEqual(response.status_code, 302)
+
+        account = Account.objects.get(displayname='New Display Name')
+        oauth_tokens = account.token_set.all()
+        self.assertEqual(oauth_tokens.count(), 1)
+        # previous token was invalidated
+        self.assertNotIn(orig_token, oauth_tokens)
+
 
 class AccountEditTestCase(AuthenticatedTestCase):
 
@@ -730,6 +763,7 @@
         data = dict(
             displayname=self.account.displayname,
             preferred_email=self.account.preferredemail.id,
+            oldpassword=DEFAULT_USER_PASSWORD,
         )
         if self.twofactor_enabled in (True, None):
             data['twofactor_required'] = self.user_wants_twofactor
@@ -794,6 +828,7 @@
         data = dict(
             displayname=self.account.displayname,
             preferred_email=self.account.preferredemail.id,
+            oldpassword=DEFAULT_USER_PASSWORD,
             twofactor_required=True,
         )
 
@@ -820,6 +855,7 @@
         data = dict(
             displayname=self.account.displayname,
             preferred_email=self.account.preferredemail.id,
+            oldpassword=DEFAULT_USER_PASSWORD,
             twofactor_required=False,
         )
         response = self.client.post(self.url, data=data)