Refactor account changes logic

* If a user tries to change a password for an account and enters the existing
  password, the operation is now idempotent. Similarly, if an admin "changes"
  a user's username to itself, this operation is also idempotent. The new
  `commit_account_changes` function only writes to the database if the `db.User`
  object of the user's account being changed has been locally modified, which it
  determines via the `Session.is_modified(db.User)` method.
* The `/admin/edit/<int:user_id>` and `/account` routes now use the same
  template. Since the former provides a superset of the functionality of the
  latter, and there is no security risk in doing so, DRYing out the templates
  here made sense. While this fixes #1597, combining the templates also has
  potential to expose elements meant for admins to users in some future
  regression. To defend against this, I introduced a new functional test module,
  `tests.functional.make_account_changes`, which provides comprehensive checks
  to this regard. I also did some DRYing of these route functions since they
  both change passwords, commit account changes to the database, and flash
  messages regarding the success or failure of these operations.
* I also made some improvements to the relevant flashed messages for account
  changes, making progress towards #1476, #1601, #1602. It seemed like
  "success" and "failure" were better categorical indicators than
  "notification" and "failure," so I went with that and used the green
  checkmark PNG we have already employed in the source interface. I changed all
  the flashed icons here to PNGs while I was at it to make a small step towards
  making the JI security-slider-friendly, and explicitly specified their sizes
  to make sure they rendered correctly.
* Changing some of the flashed messages in the process of my refactor, I had to
  change the corresponding unit tests as well. To test both the content and
  category of the flashed message, I decided to take advantage of Flask-Testing's
  `assertMessageFlashed` method. This required installing an additional testing
  dependency, `blinker`, which I did without changing any other package versions.

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)

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/<int:user_id>', 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/<int:user_id>', 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')
 
 

securedrop/journalist_templates/edit_account.html

@@ -1,31 +1,66 @@
 {% extends "base.html" %}
 {% block body %}
+{% if user %}
+<h1>Edit user "{{ user.username }}"</h1>
+<p><a href="/admin">&laquo; Back to admin interface</a></p>
+{% else %}
 <h1>Edit your account</h1>
+{% endif %}
 
 <form method="post">
   <input name="csrf_token" type="hidden" value="{{ csrf_token() }}"/>
+  {% if user %}
+  <p>
+    <label for="username">Change username</label>
+    <input name="username" id="username" type="text" placeholder="{{ user.username }}" />
+  </p>
+  {% endif %}
   <p>
     <label for="password">Change password</label>
     <input name="password" id="password" type="password" />
+    {% if user %}
+    <em class="light">Leave blank for no change</em>
+    {% endif %}
   </p>
   <p>
     <label for="password_again">Change password (confirm)</label>
     <input name="password_again" id="password_again" type="password" />
   </p>
+  {% if user %}
+  <p>
+    <input name="is_admin" id="is_admin" type="checkbox" {% if user.is_admin %}checked{% endif %} />
+    <label for="is_admin">Is Administrator</label>
+  </p>
+  {% endif %}
+  {% if user %}
+  <button class="sd-button" type="submit" id="update-user">UPDATE USER</button>
+  {% else %}
   <button class="sd-button" type="submit" id="update">UPDATE</button>
+  {% endif %}
 </form>
 
 <hr class="no-line">
 
 <h2>Reset Two Factor Authentication</h2>
+{% if user %}
+<p>If a user's two factor authentication credentials have been lost or compromised, you can reset them here. <em>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.</em></p>
+<form method="post" action="{{ url_for('admin_reset_two_factor_totp') }}" id="reset-two-factor-totp">
+  <input name="uid" type="hidden" value="{{ user.id }}"/>
+{% else %}
 <p>If your two factor authentication credentials have been lost or compromised, or you got a new device, you can reset your credentials here. <em>If you do this, make sure you are ready to set up your new device, otherwise you will be locked out of your account.</em></p>
 <form method="post" action="{{ url_for('account_reset_two_factor_totp') }}" id="reset-two-factor-totp">
+{% endif %}
   <input name="csrf_token" type="hidden" value="{{ csrf_token() }}"/>
-  <button class="sd-button" type="submit" class="pull-right">RESET TWO FACTOR AUTHENTICATION (GOOGLE AUTHENTICATOR)</button>
+  <button class="sd-button" type="submit" class="pull-right">RESET TWO FACTOR AUTHENTICATION (APP)</button>
 </form>
 <br />
+{% if user %}
+<form method="post" action="{{ url_for('admin_reset_two_factor_hotp') }}" id="reset-two-factor-hotp">
+  <input name="uid" type="hidden" value="{{ user.id }}"/>
+{% else %}
 <form method="post" action="{{ url_for('account_reset_two_factor_hotp') }}" id="reset-two-factor-hotp">
+{% endif %}
   <input name="csrf_token" type="hidden" value="{{ csrf_token() }}"/>
-  <button class="sd-button" type="submit" class="pull-right">RESET TWO FACTOR AUTHENTICATION (HOTP YUBIKEY)</button>
+  <button class="sd-button" type="submit" class="pull-right">RESET TWO FACTOR AUTHENTICATION (HARDWARE TOKEN)</button>
 </form>
 {% endblock %}

securedrop/journalist_templates/flashed.html

@@ -4,9 +4,16 @@
     {% if category != 'banner-warning' %}
       <div class="flash {{ category }}">
         {% if category == 'notification' %}
-        <i class="fa fa-info-circle pull-left"></i>
+        <img src="{{ url_for('static',
+        filename='i/font-awesome/info-circle-black.png') }}" height=18
+        width=24>
+        {% elif category == 'success' %}
+        <img src="{{ url_for('static', filename='i/success_checkmark.png') }}"
+        height=20.4 width=27.2>
         {% elif category == 'error' %}
-        <i class="fa fa-exclamation-triangle pull-left"></i>
+        <img src="{{ url_for('static',
+        filename='i/font-awesome/exclamation-triangle-black.png') }}" height=18
+        width=24>
         {% endif %}
         {{ message }}
       </div>

securedrop/requirements/test-requirements.in

@@ -1,4 +1,5 @@
 beautifulsoup4
+blinker
 coveralls
 Flask-Testing
 mock

securedrop/requirements/test-requirements.txt

@@ -5,24 +5,25 @@
 #    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
 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

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