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.
* 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
 Flask-Testing
 mock
 pip-tools

securedrop/requirements/test-requirements.txt

@@ -5,21 +5,22 @@
 #    pip-compile --output-file test-requirements.txt test-requirements.in
 #
 beautifulsoup4==4.5.1
-click==6.6                # via flask, pip-tools
-coverage==4.2             # via pytest-cov
-first==2.0.1              # via pip-tools
+blinker==1.4
+click==6.6
+coverage==4.2
+first==2.0.1
 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
 selenium==2.53.6
-six==1.10.0               # via mock, pip-tools
+six==1.10.0
 Werkzeug==0.11.11         # via flask

securedrop/test.sh

@@ -10,7 +10,8 @@ export PYTHONPATH=./tests
 export SECUREDROP_ENV=test
 
 # -f makes unittest fail fast, so we can use && to avoid burying test failures
-python -m unittest -fv tests.functional.submit_and_retrieve_message && \
-python -m unittest -fv tests.functional.submit_and_retrieve_file && \
-python -m unittest -fv tests.functional.admin_interface
+python -m unittest -fv tests.functional.make_account_changes && \
+	python -m unittest -fv tests.functional.submit_and_retrieve_message && \
+	python -m unittest -fv tests.functional.submit_and_retrieve_file && \
+	python -m unittest -fv tests.functional.admin_interface