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.

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

Added journalist password change API #3874

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 38 additions & 1 deletion securedrop/journalist_app/api.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from datetime import datetime, timedelta
from functools import wraps
from six import string_types
import json
from werkzeug.exceptions import default_exceptions # type: ignore

Expand All @@ -9,7 +10,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 +281,41 @@ def get_current_user():
user = get_user_object(request)
return jsonify(user.to_json()), 200

@api.route('/user/new-passphrase', methods=['POST'])
@token_required
def set_passphrase():
REQUIRED_ATTRIBUTES = ['old_passphrase', 'two_factor_code', 'new_passphrase']

user = get_user_object(request)
data = json.loads(request.data)

# Validate request
for attribute in REQUIRED_ATTRIBUTES:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so this isn't a complaint about anything here being wrong, but I wrote a kinda-equivalent to WTForms for JSON just so that this could be extracted away from the endpoints (moved into classes) and still provide clear, sensible error messages.

https://github.com/heartsucker/python-json-serde

I know we don't want to add a lot of dependencies here, but this (or a cleaned up, slightly better version) might add to less hand written code in the app and make everything a little cleaner.

Copy link
Contributor Author

@mdrose mdrose Oct 17, 2018

Choose a reason for hiding this comment

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

Yeah, obviously this should probably be refactored in some form for use in other endpoints. As you said, there's certainly libraries that do this sort of stuff, if we're ok with another dependency.

There's many out there. I know there's WTForms style implementations out there, like yours. There's also JSON Schema implementations, like this: https://github.com/Julian/jsonschema etc.

Regardless, the API endpoint will still have to define the data it expects, so, I think it also depends on how complicated the API schema gets and how complicated it is to validate. Right now it's pretty simple, but, obviously, that can change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I used to work at a company that tried to define all its messages with JSONSchema, and it was horrible because of the complexity. I would really rather not use it because things that can be expressed in a couple of lines of code / sentences became super unwieldy in JSONSchema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really attached to any of these and there's just so many (Marshmallow and Schematic also come to mind).

There are advantages to serialized schemas (like JSON Schema), though. You can keep the server and client in sync better, with fewer modifications / duplication client side, since the client can just react to the server's schema.

Anyway, the current messages are pretty simple and I don't know if we need the complexity / external dependency of this kind of stuff right now, but I don't know the roadmap for this project in terms of future functionality.

Regardless, just let me know how you guys would like me to proceed and I'll adjust this PR accordingly.

if attribute not in data:
return jsonify({'message':
'Invalid request: {} unspecified'.format(attribute)}), 400
elif not isinstance(data[attribute], string_types):
return jsonify({'message':
'Invalid request: {} must be a string'}.format(attribute)), 400

try:
Journalist.login(user.username, data['old_passphrase'], data['two_factor_code'])
except (BadTokenException, WrongPasswordException):
return jsonify({'message':
'Invalid credentials. Passphrase change denied'}), 403

# Set password
try:
user.set_password(data['new_passphrase'])
db.session.commit()
except (InvalidPasswordLength, NonDicewarePassword) as e:
return jsonify({'message': str(e)}), 400
except Exception:
return jsonify({'message':
'An error occurred while setting the passphrase. Passphrase unchanged'}), 500

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
9 changes: 7 additions & 2 deletions securedrop/models.py
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 Expand Up @@ -574,7 +578,8 @@ def to_json(self):
'username': self.username,
'last_login': self.last_access.isoformat() + 'Z',
'is_admin': self.is_admin,
'uuid': self.uuid
'uuid': self.uuid,
'new_passphrase_url': url_for('api.set_passphrase')
}
return json_user

Expand Down
96 changes: 95 additions & 1 deletion securedrop/tests/test_journalist_api.py
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,99 @@ def test_authorized_user_can_add_reply(journalist_app, journalist_api_token,
assert reply_content == saved_content


def test_set_passphrase_blank_request_400(journalist_app, journalist_api_token):
with journalist_app.test_client() as app:
response = app.post(url_for('api.set_passphrase'),
data='{}',
headers=get_api_headers(journalist_api_token))
assert response.status_code == 400


def test_set_passphrase_nonstring_request_400(journalist_app, journalist_api_token):
with journalist_app.test_client() as app:
data = {'new_passphrase': {}}
response = app.post(url_for('api.set_passphrase'),
data=json.dumps(data),
headers=get_api_headers(journalist_api_token))
assert response.status_code == 400


def test_set_passphrase_wrong_pass_403(journalist_app, journalist_api_token,
test_journo):
topt = test_journo['journalist'].totp
with journalist_app.test_client() as app:
data = {'old_passphrase': test_journo['password'] + 'a',
'two_factor_code': topt.now(),
'new_passphrase': 'aaaa'}
response = app.post(url_for('api.set_passphrase'),
data=json.dumps(data),
headers=get_api_headers(journalist_api_token))

assert response.status_code == 403


def test_set_passphrase_wrong_otp_403(journalist_app, journalist_api_token,
test_journo):
topt = test_journo['journalist'].totp
with journalist_app.test_client() as app:
data = {'old_passphrase': test_journo['password'],
'two_factor_code': topt.now() + '1',
'new_passphrase': 'aaaa'}
response = app.post(url_for('api.set_passphrase'),
data=json.dumps(data),
headers=get_api_headers(journalist_api_token))

assert response.status_code == 403


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

with journalist_app.test_client() as app:
new_passphrase = 'a' * (Journalist.MIN_PASSWORD_LEN - 1)
data = {'old_passphrase': test_journo['password'],
'two_factor_code': topt.now(),
'new_passphrase': new_passphrase}
response = app.post(url_for('api.set_passphrase'),
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_passphrase))
)

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


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

with journalist_app.test_client() as app:
new_passphrase = ('a ' * Journalist.MIN_PASSWORD_WORDS)[:-1]
pass_len_short = Journalist.MIN_PASSWORD_LEN - len(new_passphrase)
if pass_len_short > 0:
new_passphrase += 'a' * pass_len_short

data = {'old_passphrase': test_journo['password'],
'two_factor_code': topt.now(),
'new_passphrase': new_passphrase}
response = app.post(url_for('api.set_passphrase'),
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