diff --git a/securedrop/db.py b/securedrop/db.py index 721b741d4a..46c2c2fca5 100644 --- a/securedrop/db.py +++ b/securedrop/db.py @@ -270,6 +270,9 @@ def _scrypt_hash(self, password, salt, params=None): MAX_PASSWORD_LEN = 128 def set_password(self, password): + # Don't do anything if user's password hasn't changed. + if self.pw_hash and self.valid_password(password): + return # Enforce a reasonable maximum length for passwords to avoid DoS if len(password) > self.MAX_PASSWORD_LEN: raise InvalidPasswordLength(password) diff --git a/securedrop/journalist.py b/securedrop/journalist.py index 896083b17a..bbb28fc0cf 100644 --- a/securedrop/journalist.py +++ b/securedrop/journalist.py @@ -247,40 +247,67 @@ def admin_reset_two_factor_hotp(): return render_template('admin_edit_hotp_secret.html', uid=uid) +class PasswordMismatchError(Exception): + pass + + +def edit_account_password(user, password, password_again): + if password: + if password != password_again: + flash("Passwords didn't match!", "error") + raise PasswordMismatchError + try: + user.set_password(password) + except InvalidPasswordLength: + flash("Password must be less than {} characters!".format( + Journalist.MAX_PASSWORD_LEN), 'error') + raise + + +def commit_account_changes(user): + if db_session.is_modified(user): + try: + db_session.add(user) + db_session.commit() + except Exception as e: + flash("An unexpected error occurred! Please check the application " + "logs or inform your adminstrator.", "error") + app.logger.error("Account changes for '{}' failed: {}".format(user, + e)) + db_session.rollback() + else: + flash("Account successfully updated!", "success") + + @app.route('/admin/edit/', methods=('GET', 'POST')) @admin_required def admin_edit_user(user_id): user = Journalist.query.get(user_id) if request.method == 'POST': - if request.form['username'] != "": + if request.form['username']: new_username = request.form['username'] - if Journalist.query.filter_by(username=new_username).one_or_none(): - flash("Username {} is already taken".format(new_username), + if new_username == user.username: + pass + elif Journalist.query.filter_by( + username=new_username).one_or_none(): + flash('Username "{}" is already taken!'.format(new_username), "error") + return redirect(url_for("admin_edit_user", user_id=user_id)) else: user.username = new_username - if request.form['password'] != "": - if request.form['password'] != request.form['password_again']: - flash("Passwords didn't match", "error") - return redirect(url_for("admin_edit_user", user_id=user_id)) - try: - user.set_password(request.form['password']) - flash("Password successfully changed for user {} ".format( - user.username), "notification") - except InvalidPasswordLength: - flash("Your password is too long " - "(maximum length {} characters)".format( - Journalist.MAX_PASSWORD_LEN), "error") - return redirect(url_for("admin_edit_user", user_id=user_id)) + try: + edit_account_password(user, request.form['password'], + request.form['password_again']) + except (PasswordMismatchError, InvalidPasswordLength): + return redirect(url_for("admin_edit_user", user_id=user_id)) user.is_admin = bool(request.form.get('is_admin')) - db_session.add(user) - db_session.commit() + commit_account_changes(user) - return render_template("admin_edit_user.html", user=user) + return render_template("edit_account.html", user=user) @app.route('/admin/delete/', methods=('POST',)) @@ -306,31 +333,14 @@ def edit_account(): user = g.user if request.method == 'POST': - if request.form['password'] != "": - if request.form['password'] != request.form['password_again']: - flash("Passwords didn't match", "error") - return redirect(url_for("edit_account")) - try: - user.set_password(request.form['password']) - except InvalidPasswordLength: - flash("Your password is too long " - "(maximum length {} characters)".format( - Journalist.MAX_PASSWORD_LEN), "error") - return redirect(url_for("edit_account")) - try: - db_session.add(user) - db_session.commit() - flash( - "Password successfully changed!", - "notification") - except Exception as e: - flash( - "An unknown error occurred, please inform your administrator", - "error") - app.logger.error("Password change for '{}' failed: {}".format( - user, e)) - db_session.rollback() + edit_account_password(user, request.form['password'], + request.form['password_again']) + except (PasswordMismatchError, InvalidPasswordLength): + return redirect(url_for('edit_account')) + + commit_account_changes(user) + return render_template('edit_account.html') diff --git a/securedrop/journalist_templates/admin_edit_user.html b/securedrop/journalist_templates/admin_edit_user.html deleted file mode 100644 index 8155971fb6..0000000000 --- a/securedrop/journalist_templates/admin_edit_user.html +++ /dev/null @@ -1,43 +0,0 @@ -{% extends "base.html" %} -{% block body %} -

Edit user "{{ user.username }}"

-

« Back to admin interface

- -
- -

- - -

-

- - - Leave blank for no change -

-

- - -

-

- - -

- -
- -
- -

Reset Two Factor Authentication

-

If a user's two factor authentication credentials have been lost or compromised, you can reset them here. If you do this, make sure the user is present and ready to set up their device with the new two factor credentials. Otherwise, they will be locked out of their account.

-
- - - -
-
-
- - - -
-{% endblock %} diff --git a/securedrop/journalist_templates/edit_account.html b/securedrop/journalist_templates/edit_account.html index 79f2157f7f..ec598751f6 100644 --- a/securedrop/journalist_templates/edit_account.html +++ b/securedrop/journalist_templates/edit_account.html @@ -1,31 +1,66 @@ {% extends "base.html" %} {% block body %} +{% if user %} +

Edit user "{{ user.username }}"

+

« Back to admin interface

+{% else %}

Edit your account

+{% endif %}
+ {% if user %} +

+ + +

+ {% endif %}

+ {% if user %} + Leave blank for no change + {% endif %}

+ {% if user %} +

+ + +

+ {% endif %} + {% if user %} + + {% else %} + {% endif %}

Reset Two Factor Authentication

+{% if user %} +

If a user's two factor authentication credentials have been lost or compromised, you can reset them here. If you do this, make sure the user is present and ready to set up their device with the new two factor credentials. Otherwise, they will be locked out of their account.

+
+ +{% else %}

If your two factor authentication credentials have been lost or compromised, or you got a new device, you can reset your credentials here. If you do this, make sure you are ready to set up your new device, otherwise you will be locked out of your account.

+{% endif %} - +

+{% if user %} +
+ +{% else %} +{% endif %} - +
{% endblock %} diff --git a/securedrop/journalist_templates/flashed.html b/securedrop/journalist_templates/flashed.html index 507d63633a..600c0e2a12 100644 --- a/securedrop/journalist_templates/flashed.html +++ b/securedrop/journalist_templates/flashed.html @@ -4,9 +4,16 @@ {% if category != 'banner-warning' %}
{% if category == 'notification' %} - + + {% elif category == 'success' %} + {% elif category == 'error' %} - + {% endif %} {{ message }}
diff --git a/securedrop/requirements/test-requirements.in b/securedrop/requirements/test-requirements.in index 7a0c5d4653..c893461741 100644 --- a/securedrop/requirements/test-requirements.in +++ b/securedrop/requirements/test-requirements.in @@ -1,4 +1,5 @@ beautifulsoup4 +blinker coveralls Flask-Testing mock diff --git a/securedrop/requirements/test-requirements.txt b/securedrop/requirements/test-requirements.txt index fed1395954..032e889d89 100644 --- a/securedrop/requirements/test-requirements.txt +++ b/securedrop/requirements/test-requirements.txt @@ -5,6 +5,7 @@ # pip-compile --output-file test-requirements.txt test-requirements.in # beautifulsoup4==4.5.1 +blinker==1.4 click==6.6 # via flask, pip-tools coverage==4.2 # via coveralls, pytest-cov coveralls==1.1 @@ -12,17 +13,17 @@ docopt==0.6.2 # via coveralls first==2.0.1 # via pip-tools Flask-Testing==0.6.1 Flask==0.11.1 # via flask-testing -funcsigs==1.0.2 # via mock -itsdangerous==0.24 # via flask +funcsigs==1.0.2 +itsdangerous==0.24 Jinja2==2.8 # via flask MarkupSafe==0.23 # via jinja2 mock==2.0.0 -pbr==1.10.0 # via mock +pbr==1.10.0 pip-tools==1.7.0 py==1.4.31 pytest-cov==2.4.0 pytest==3.0.3 requests==2.13.0 # via coveralls selenium==2.53.6 -six==1.10.0 # via mock, pip-tools +six==1.10.0 Werkzeug==0.11.11 # via flask diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index f1f7a38e54..2659528395 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -75,10 +75,6 @@ def _admin_visits_admin_interface(self): h1s = self.driver.find_elements_by_tag_name('h1') self.assertIn("Admin Interface", [el.text for el in h1s]) - users_table_rows = self.driver.find_elements_by_css_selector( - 'table#users tr.user') - self.assertEquals(len(users_table_rows), 1) - def _add_user(self, username, password, is_admin=False): username_field = self.driver.find_element_by_css_selector( 'input[name="username"]') @@ -176,12 +172,76 @@ def _new_user_can_log_in(self): self.driver.find_element_by_link_text, 'Admin') + def _edit_account(self): + edit_account_link = self.driver.find_element_by_link_text( + 'Edit Account') + edit_account_link.click() + + # The header says "Edit your account" + h1s = self.driver.find_elements_by_tag_name('h1')[0] + self.assertEqual('Edit your account', h1s.text) + # There's no link back to the admin interface. + with self.assertRaises(NoSuchElementException): + self.driver.find_element_by_partial_link_text('Back to admin interface') + # There's no field to change your username. + with self.assertRaises(NoSuchElementException): + self.driver.find_element_by_css_selector('#username') + # There's no checkbox to change the administrator status of your + # account. + with self.assertRaises(NoSuchElementException): + username_field = self.driver.find_element_by_css_selector('#is_admin') + # 2FA reset buttons at the bottom point to the user URLs for reset. + totp_reset_button = self.driver.find_elements_by_css_selector( + '#reset-two-factor-totp')[0] + self.assertRegexpMatches(totp_reset_button.get_attribute('action'), + '/account/reset-2fa-totp') + hotp_reset_button = self.driver.find_elements_by_css_selector( + '#reset-two-factor-hotp')[0] + self.assertRegexpMatches(hotp_reset_button.get_attribute('action'), + '/account/reset-2fa-hotp') + def _edit_user(self, username): + user = Journalist.query.filter_by(username=username).one() + new_user_edit_links = filter( lambda el: el.get_attribute('data-username') == username, self.driver.find_elements_by_tag_name('a')) self.assertEquals(len(new_user_edit_links), 1) new_user_edit_links[0].click() + # The header says "Edit user "username"". + h1s = self.driver.find_elements_by_tag_name('h1')[0] + self.assertEqual('Edit user "{}"'.format(username), h1s.text) + # There's a convenient link back to the admin interface. + admin_interface_link = self.driver.find_element_by_partial_link_text( + 'Back to admin interface') + self.assertRegexpMatches(admin_interface_link.get_attribute('href'), + '/admin$') + # There's a field to change the user's username and it's already filled + # out with the user's username. + username_field = self.driver.find_element_by_css_selector('#username') + self.assertEqual(username_field.get_attribute('placeholder'), username) + # There's a checkbox to change the administrator status of the user and + # it's already checked appropriately to reflect the current status of + # our user. + username_field = self.driver.find_element_by_css_selector('#is_admin') + self.assertEqual(bool(username_field.get_attribute('checked')), + user.is_admin) + # 2FA reset buttons at the bottom point to the admin URLs for + # resettting 2FA and include the correct user id in the hidden uid. + totp_reset_button = self.driver.find_elements_by_css_selector( + '#reset-two-factor-totp')[0] + self.assertRegexpMatches(totp_reset_button.get_attribute('action'), + '/admin/reset-2fa-totp') + totp_reset_uid = totp_reset_button.find_element_by_name('uid') + self.assertEqual(int(totp_reset_uid.get_attribute('value')), user.id) + self.assertFalse(totp_reset_uid.is_displayed()) + hotp_reset_button = self.driver.find_elements_by_css_selector( + '#reset-two-factor-hotp')[0] + self.assertRegexpMatches(hotp_reset_button.get_attribute('action'), + '/admin/reset-2fa-hotp') + hotp_reset_uid = hotp_reset_button.find_element_by_name('uid') + self.assertEqual(int(hotp_reset_uid.get_attribute('value')), user.id) + self.assertFalse(hotp_reset_uid.is_displayed()) def _admin_can_edit_new_user(self): # Log the new user out diff --git a/securedrop/tests/functional/make_account_changes.py b/securedrop/tests/functional/make_account_changes.py new file mode 100644 index 0000000000..6dc00ae1f3 --- /dev/null +++ b/securedrop/tests/functional/make_account_changes.py @@ -0,0 +1,24 @@ +# -*- coding: utf-8 -*- +from unittest import TestCase + +from functional_test import FunctionalTest +from journalist_navigation_steps import JournalistNavigationSteps + +class MakeAccountChanges(FunctionalTest, JournalistNavigationSteps, TestCase): + def test_admin_edit_account_html_template_rendering(self): + """The edit_account.html template is used both when an admin is editing + a user's account, and when a user is editing their own account. While + there is no security risk in doing so, we do want to ensure the UX is + as expected: that only the elements that belong in a particular view + are exposed there.""" + self._admin_logs_in() + self._admin_visits_admin_interface() + # Admin view of admin user + self._edit_user('admin') + self._admin_visits_admin_interface() + self._admin_adds_a_user() + # Admin view of non-admin user + self._edit_user('dellsberg') + # User view of self + self._edit_account() + self._logout() diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 6f19c702ed..a85843e0c8 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -181,14 +181,14 @@ def test_admin_edits_user_password_success_response(self): data=dict(username=self.user.username, is_admin=False, password='valid', password_again='valid')) - self.assertIn('Password successfully changed', resp.data) + self.assertMessageFlashed("Account successfully updated!", 'success') def test_user_edits_password_success_reponse(self): self._login_user() resp = self.client.post(url_for('edit_account'), data=dict(password='valid', password_again='valid')) - self.assertIn("Password successfully changed", resp.data) + self.assertMessageFlashed("Account successfully updated!", 'success') def test_admin_edits_user_password_mismatch_warning(self): self._login_admin() @@ -199,7 +199,7 @@ def test_admin_edits_user_password_mismatch_warning(self): password='not', password_again='thesame'), follow_redirects=True) - self.assertIn(escape("Passwords didn't match"), resp.data) + self.assertMessageFlashed("Passwords didn't match!", "error") def test_user_edits_password_mismatch_redirect(self): self._login_user() @@ -237,7 +237,9 @@ def test_admin_edits_user_password_too_long_warning(self): password_again=overly_long_password), follow_redirects=True) - self.assertIn('Your password is too long', resp.data) + self.assertMessageFlashed('Password must be less than {} ' + 'characters!'.format( + Journalist.MAX_PASSWORD_LEN), 'error') def test_user_edits_password_too_long_warning(self): self._login_user() @@ -248,7 +250,9 @@ def test_user_edits_password_too_long_warning(self): password_again=overly_long_password), follow_redirects=True) - self.assertIn('Your password is too long', resp.data) + self.assertMessageFlashed('Password must be less than {} ' + 'characters!'.format( + Journalist.MAX_PASSWORD_LEN), 'error') def test_admin_add_user_password_too_long_warning(self): self._login_admin() @@ -272,8 +276,8 @@ def test_admin_edits_user_invalid_username(self): data=dict(username=new_username, is_admin=False, password='', password_again='')) - self.assertIn('Username {} is already taken'.format(new_username), - resp.data) + self.assertMessageFlashed('Username "{}" is already taken!'.format( + new_username), 'error') def test_admin_resets_user_hotp(self): self._login_admin() @@ -436,14 +440,16 @@ def test_too_long_user_password_change(self): password_again=overly_long_password), follow_redirects=True) - self.assertIn('Your password is too long', res.data) + self.assertMessageFlashed('Password must be less than {} ' + 'characters!'.format( + Journalist.MAX_PASSWORD_LEN), 'error') def test_valid_user_password_change(self): self._login_user() res = self.client.post(url_for('edit_account'), data=dict( password='valid', password_again='valid')) - self.assertIn("Password successfully changed", res.data) + self.assertMessageFlashed("Account successfully updated!", 'success') def test_regenerate_totp(self): self._login_user()