Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added journalist password change API #3874

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Added journalist password change API #3874

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a49f0b3
Added journalist password change API
mdrose Oct 13, 2018
5e56470
Requested revisions
mdrose Oct 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion securedrop/journalist_app/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
from journalist_app import utils
from models import (Journalist, Reply, Source, Submission,
LoginThrottledException, InvalidUsernameException,
BadTokenException, WrongPasswordException)
BadTokenException, WrongPasswordException,
InvalidPasswordLength, NonDicewarePassword)
from store import NotEncrypted


Expand Down Expand Up @@ -279,6 +280,26 @@ def get_current_user():
user = get_user_object(request)
return jsonify(user.to_json()), 200

@api.route('/account/new-password', methods=['POST'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently we have an /user endpoint which gets information about the current user, what about making this new endpoint /user/new-passphrase instead of /account/new-password?

another thought: we should also add a link to this in the response from the /user endpoint:

{
  "is_admin": true,
  "last_login": "2018-07-09T20:29:41.696782Z",
  "username": "journalist",
  "uuid": "a2405127-1c9e-4a3a-80ea-95f6a71e5738",
  "new_passphrase_url": "/user/new-passphrase"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@token_required
def new_password():
user = get_user_object(request)
new_password = json.loads(request.data)['new_password']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if new_password is not found in the request body?

(any expectations like this for the request body that might not be valid are good test cases to add)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New commit adds checks for missing request parameters and invalid data types, along with relevant tests.


try:
user.set_password(new_password)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the regular journalist interface for journalist passphrase reset, we have two additional requirements (for defense in depth) which is to provide a valid passphrase and 2FA token before changing the passphrase. additionally, we have the web application generating the diceware passphrases both for sources and journalists (shown only in the session where they are generated/changed). we should do the same here in the API too.

recapping - the logic should be:

  1. client sends request to this endpoint with old_passphrase and two_factor_code in the body
  2. server checks they are valid
  3. server generates a diceware passphrase and provides the user a new_passphrase in the response

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dangerous because if a client doesn't finish the read (broken connection), then the client could be locked out. Changing a passphrase should require multiple steps.

  1. Client requests new passphrase
  2. Server check auth, sends new passphrase, and stores new_passphrase_hash or something in the DB
  3. Client replies with the new passphrase to confirm (separate endpoint)
  4. Server checks the request matches the new hash them moves it over in the db
  5. Server responds that the passphrase has changed

Additionally, errors in the client (failure to display passphrase, crash, etc) could mean that at the network level, everything went just fine, but the user is still locked out.

Because this is happening over Tor, we have to plan for exceptional robustness.

Copy link
Contributor Author

@mdrose mdrose Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redshiftzero:

  1. server generates a diceware passphrase and provides the user a new_passphrase in the response

@heartsucker:

  1. Server check auth, sends new passphrase, and stores new_passphrase_hash or something in the DB

So, currently, the web interface basically offers a new password suggestion, which doesn't get stored in the database. It's still up to the client to send a new password, which then gets validated against criteria (length / words).

except (InvalidPasswordLength, NonDicewarePassword) as e:
return jsonify({'message': str(e)}), 400
except Exception as e:
mdrose marked this conversation as resolved.
Show resolved Hide resolved
return jsonify({'message': 'An error occurred while setting the password. Password unchanged'}), 500

try:
db.session.commit()
except Exception as e:
mdrose marked this conversation as resolved.
Show resolved Hide resolved
return jsonify({'message': 'An error occurred on database commit. Password unchanged.'}), 500
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also suggest that all 500 errors on this endpoint use the same string to not give additional information to an attacker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


return jsonify({'message': 'Password changed successfully'}), 200

def _handle_http_exception(error):
# Workaround for no blueprint-level 404/5 error handlers, see:
# https://github.com/pallets/flask/issues/503#issuecomment-71383286
Expand Down
6 changes: 5 additions & 1 deletion securedrop/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ class NonDicewarePassword(PasswordError):

"""Raised when attempting to validate a password that is not diceware-like
"""
def __str__(self):
return ("Password needs to be a passphrase of at least "
"{} words.").format(Journalist.MIN_PASSWORD_WORDS)


class Journalist(db.Model):
Expand Down Expand Up @@ -361,6 +364,7 @@ def _scrypt_hash(self, password, salt):

MAX_PASSWORD_LEN = 128
MIN_PASSWORD_LEN = 14
MIN_PASSWORD_WORDS = 7

def set_password(self, passphrase):
self.check_password_acceptable(passphrase)
Expand Down Expand Up @@ -398,7 +402,7 @@ def check_password_acceptable(cls, password):
raise InvalidPasswordLength(password)

# Ensure all passwords are "diceware-like"
if len(password.split()) < 7:
if len(password.split()) < cls.MIN_PASSWORD_WORDS:
raise NonDicewarePassword()

def valid_password(self, passphrase):
Expand Down
43 changes: 42 additions & 1 deletion securedrop/tests/test_journalist_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
from itsdangerous import TimedJSONWebSignatureSerializer

from db import db
from models import Journalist, Reply, Source, SourceStar, Submission
from models import (Journalist, Reply, Source, SourceStar, Submission,
NonDicewarePassword, InvalidPasswordLength)

os.environ['SECUREDROP_ENV'] = 'test' # noqa
from utils.api_helper import get_api_headers
Expand Down Expand Up @@ -662,6 +663,46 @@ def test_authorized_user_can_add_reply(journalist_app, journalist_api_token,
assert reply_content == saved_content


def test_new_password_unacceptable_400(journalist_app, journalist_api_token,
test_journo):
original_hash = test_journo['journalist'].passphrase_hash

with journalist_app.test_client() as app:
new_password = 'a' * (Journalist.MIN_PASSWORD_LEN - 1)
data = {'new_password': new_password}
response = app.post(url_for('api.new_password'),
data=json.dumps(data),
headers=get_api_headers(journalist_api_token))

assert response.status_code == 400
assert (response.get_json()['message'] == str(NonDicewarePassword()) or
response.get_json()['message'] == str(InvalidPasswordLength(new_password))
)

with journalist_app.app_context():
user = Journalist.query.get(test_journo['id'])
assert original_hash == user.passphrase_hash


def test_new_password_success_200(journalist_app, journalist_api_token,
test_journo):
original_hash = test_journo['journalist'].passphrase_hash

with journalist_app.test_client() as app:
new_password = 'a ' * max( (Journalist.MIN_PASSWORD_LEN+1) / 2,
Journalist.MIN_PASSWORD_WORDS)

data = {'new_password': new_password}
response = app.post(url_for('api.new_password'),
data=json.dumps(data),
headers=get_api_headers(journalist_api_token))
assert response.status_code == 200

with journalist_app.app_context():
user = Journalist.query.get(test_journo['id'])
assert original_hash != user.passphrase_hash


def test_reply_without_content_400(journalist_app, journalist_api_token,
test_source, test_journo):
with journalist_app.test_client() as app:
Expand Down