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

Conversation

mdrose
Copy link
Contributor

@mdrose mdrose commented Oct 14, 2018

Status

Ready for review / Work in progress

This is only the API call for password changes. I haven't done 2FA yet.

Unit tests are included.

Description of Changes

Changes are in response to #3869

Testing

API endpoint is /api/v1/user/new-passphrase

The new password is specified in the "new_passphrase" attribute in the JSON data.

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

this is looking super good so far @mdrose! dropped some notes inline

@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.

@@ -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

new_password = json.loads(request.data)['new_password']

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

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).

securedrop/journalist_app/api.py Outdated Show resolved Hide resolved
securedrop/journalist_app/api.py Outdated Show resolved Hide resolved
new_password = json.loads(request.data)['new_password']

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.

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.

try:
db.session.commit()
except Exception as e:
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

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.

@eloquence
Copy link
Member

Hi @mdrose, sorry for the long silence on this PR. Just a note that we're still interested in landing this change -- the main reason we've not looked at it is simply that the SecureDrop Client, which is the main consumer of this API, currently does not expose any account management features, and it's not been a very high priority for beta users to add such features, because they are so rarely used.

That said, we will begin working on performance improvements and new features for the API later this year (#5104). Ideally, the SecureDrop Client should enable a journalist user to do everything they can do through the web UI, so this is still a candidate for the next round of API improvements. We'll revisit the work you've done here in that context.

Thanks again for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants