From 6928d9fc99a6871f62ba52b511989bb0b512ddf7 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Mon, 15 Apr 2019 17:59:25 +0200 Subject: [PATCH 1/8] added token revocation --- securedrop/models.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/securedrop/models.py b/securedrop/models.py index ac180a5c87..7b5c5509fa 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -574,6 +574,11 @@ def validate_api_token_and_get_user(token): data = s.loads(token) except BadData: return None + + revoked_token = RevokedToken.query.filter_by(token=token).one_or_none() + if revoked_token is not None: + return None + return Journalist.query.get(data['id']) def to_json(self): @@ -598,3 +603,16 @@ class JournalistLoginAttempt(db.Model): def __init__(self, journalist): self.journalist_id = journalist.id + + +class RevokedToken(db.Model): + + """ + API tokens that have been revoked either through a logout or other revocation mechanism. + """ + + __tablename__ = 'revoked_tokens' + + id = Column(Integer, primary_key=True) + journalist_id = Column(Integer, ForeignKey('journalists.id')) + token = db.Column(db.Text, nullable=False, unique=True) From 707d061d0f6f8947360dabf214a0265b860115f6 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Mon, 15 Apr 2019 19:29:41 +0200 Subject: [PATCH 2/8] added token revoke endpoint --- securedrop/journalist_app/api.py | 26 +++++++++++++++++++++---- securedrop/tests/test_journalist_api.py | 19 +++++++++++++++++- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/securedrop/journalist_app/api.py b/securedrop/journalist_app/api.py index 009c79e9c7..aa776d5202 100644 --- a/securedrop/journalist_app/api.py +++ b/securedrop/journalist_app/api.py @@ -10,7 +10,7 @@ from db import db from journalist_app import utils -from models import (Journalist, Reply, Source, Submission, +from models import (Journalist, Reply, Source, Submission, RevokedToken, LoginThrottledException, InvalidUsernameException, BadTokenException, WrongPasswordException) from store import NotEncrypted @@ -75,10 +75,18 @@ def get_endpoints(): @api.before_request def validate_data(): if request.method == 'POST': - # flag and star can have empty payloads + # flag, star, and logout can have empty payloads if not request.data: - if ('flag' not in request.path and 'star' not in request.path): - return abort(400, 'malformed request') + dataless_endpoints = [ + 'add_star', + 'remove_star', + 'flag', + 'logout', + ] + for endpoint in dataless_endpoints: + if request.endpoint == 'api.' + endpoint: + return + return abort(400, 'malformed request') # other requests must have valid JSON payload else: try: @@ -309,6 +317,16 @@ def get_current_user(): user = get_user_object(request) return jsonify(user.to_json()), 200 + @api.route('/logout', methods=['POST']) + @token_required + def logout(): + user = get_user_object(request) + auth_token = request.headers.get('Authorization').split(" ")[1] + revoked_token = RevokedToken(token=auth_token, journalist_id=user.id) + db.session.add(revoked_token) + db.session.commit() + return jsonify({'message': 'Your token has been revoked.'}), 200 + def _handle_api_http_exception(error): # Workaround for no blueprint-level 404/5 error handlers, see: # https://github.com/pallets/flask/issues/503#issuecomment-71383286 diff --git a/securedrop/tests/test_journalist_api.py b/securedrop/tests/test_journalist_api.py index 1a470fdd90..d5075e392e 100644 --- a/securedrop/tests/test_journalist_api.py +++ b/securedrop/tests/test_journalist_api.py @@ -10,7 +10,7 @@ from itsdangerous import TimedJSONWebSignatureSerializer from db import db -from models import Journalist, Reply, Source, SourceStar, Submission +from models import Journalist, Reply, Source, SourceStar, Submission, RevokedToken os.environ['SECUREDROP_ENV'] = 'test' # noqa from .utils.api_helper import get_api_headers @@ -933,3 +933,20 @@ def test_reply_download_generates_checksum(journalist_app, assert fetched_reply.checksum # we don't want to recalculat this value assert not mock_add_checksum.called + + +def test_revoke_token(journalist_app, test_journo, journalist_api_token): + with journalist_app.test_client() as app: + # without token 403's + resp = app.post(url_for('api.logout')) + assert resp.status_code == 403 + + resp = app.post(url_for('api.logout'), headers=get_api_headers(journalist_api_token)) + assert resp.status_code == 200 + + revoked_token = RevokedToken.query.filter_by(token=journalist_api_token).one() + assert revoked_token.journalist_id == test_journo['id'] + + resp = app.get(url_for('api.get_all_sources'), + headers=get_api_headers(journalist_api_token)) + assert resp.status_code == 403 From bd78d7732c62e62b6659f31ee9c2a5a7f18f5f2f Mon Sep 17 00:00:00 2001 From: heartsucker Date: Tue, 16 Apr 2019 14:08:19 +0200 Subject: [PATCH 3/8] updated migration to include revoked_tokens table --- ...139cfdc8c_add_checksum_columns_revoke_table.py} | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) rename securedrop/alembic/versions/{b58139cfdc8c_add_checksum_columns.py => b58139cfdc8c_add_checksum_columns_revoke_table.py} (86%) diff --git a/securedrop/alembic/versions/b58139cfdc8c_add_checksum_columns.py b/securedrop/alembic/versions/b58139cfdc8c_add_checksum_columns_revoke_table.py similarity index 86% rename from securedrop/alembic/versions/b58139cfdc8c_add_checksum_columns.py rename to securedrop/alembic/versions/b58139cfdc8c_add_checksum_columns_revoke_table.py index ba2789f0f4..fefcb66317 100644 --- a/securedrop/alembic/versions/b58139cfdc8c_add_checksum_columns.py +++ b/securedrop/alembic/versions/b58139cfdc8c_add_checksum_columns_revoke_table.py @@ -1,4 +1,4 @@ -"""add checksum columns +"""add checksum columns and revoke token table Revision ID: b58139cfdc8c Revises: f2833ac34bb6 @@ -36,6 +36,16 @@ def upgrade(): with op.batch_alter_table('submissions', schema=None) as batch_op: batch_op.add_column(sa.Column('checksum', sa.String(length=255), nullable=True)) + op.create_table( + 'revoked_tokens', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('journalist_id', sa.Integer(), nullable=True), + sa.Column('token', sa.Text(), nullable=False), + sa.ForeignKeyConstraint(['journalist_id'], ['journalists.id'], ), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('token') + ) + try: app = create_app(config) @@ -77,6 +87,8 @@ def upgrade(): def downgrade(): + op.drop_table('revoked_tokens') + with op.batch_alter_table('submissions', schema=None) as batch_op: batch_op.drop_column('checksum') From b2d91ced191f60ab5538e27d9e992b25b20f61bf Mon Sep 17 00:00:00 2001 From: heartsucker Date: Tue, 16 Apr 2019 14:29:21 +0200 Subject: [PATCH 4/8] updated migration tests to include revoked_token table --- .../tests/migrations/migration_b58139cfdc8c.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/securedrop/tests/migrations/migration_b58139cfdc8c.py b/securedrop/tests/migrations/migration_b58139cfdc8c.py index 73871ab9b6..c0909752a4 100644 --- a/securedrop/tests/migrations/migration_b58139cfdc8c.py +++ b/securedrop/tests/migrations/migration_b58139cfdc8c.py @@ -151,6 +151,8 @@ def check_upgrade(self): and without being able to inject this config, the checksum function won't succeed. The above `load_data` function provides data that can be manually verified by checking the `rqworker` log file in `/tmp/`. + + The other part of the migration, creating a table, cannot be tested regardless. ''' pass @@ -175,9 +177,15 @@ def load_data(self): self.create_submission(checksum=True) self.create_reply(checksum=True) + # add a revoked token for enable a foreign key connection + self.add_revoked_token() + def check_downgrade(self): ''' Verify that the checksum column is now gone. + + The dropping of the revoked_tokens table cannot be checked. If the migration completes, + then it wokred correctly. ''' with self.app.app_context(): sql = "SELECT * FROM submissions" @@ -197,3 +205,13 @@ def check_downgrade(self): submission['checksum'] except NoSuchColumnError: pass + + def add_revoked_token(self): + params = { + 'journalist_id': self.journalist_id, + 'token': 'abc123', + } + sql = '''INSERT INTO revoked_tokens (journalist_id, token) + VALUES (:journalist_id, :token) + ''' + db.engine.execute(text(sql), **params) From 7518e077dd699bf546fcd26c85194caf4a647729 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Tue, 30 Apr 2019 16:11:37 -0700 Subject: [PATCH 5/8] app: clean up no longer needed revoked tokens from the database --- securedrop/journalist_app/__init__.py | 7 ++++++- securedrop/journalist_app/utils.py | 17 ++++++++++++++++- securedrop/models.py | 10 ++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index 561160e79f..fd8cfbe4e3 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -18,7 +18,8 @@ from db import db from journalist_app import account, admin, api, main, col from journalist_app.utils import (get_source, logged_in, - JournalistInterfaceSessionInterface) + JournalistInterfaceSessionInterface, + cleanup_expired_revoked_tokens) from models import Journalist from store import Storage from worker import rq_worker_queue @@ -124,6 +125,10 @@ def _handle_http_exception(error): template_filters.rel_datetime_format app.jinja_env.filters['filesizeformat'] = template_filters.filesizeformat + @app.before_first_request + def expire_blacklisted_tokens(): + return cleanup_expired_revoked_tokens() + @app.before_request def setup_g(): """Store commonly used values in Flask's special g object""" diff --git a/securedrop/journalist_app/utils.py b/securedrop/journalist_app/utils.py index 91b7f77f84..b8b6a78deb 100644 --- a/securedrop/journalist_app/utils.py +++ b/securedrop/journalist_app/utils.py @@ -13,7 +13,7 @@ from models import (get_one_or_else, Source, Journalist, InvalidUsernameException, WrongPasswordException, LoginThrottledException, BadTokenException, SourceStar, - PasswordError, Submission) + PasswordError, Submission, RevokedToken) from rm import srm from store import add_checksum_for_file from worker import rq_worker_queue @@ -353,3 +353,18 @@ def save_session(self, app, session, response): else: super(JournalistInterfaceSessionInterface, self).save_session( app, session, response) + + +def cleanup_expired_revoked_tokens(): + """Remove tokens that have now expired from the revoked token table.""" + + revoked_tokens = db.session.query(RevokedToken).all() + + for revoked_token in revoked_tokens: + if Journalist.validate_token_is_not_expired_or_invalid(revoked_token.token): + pass # The token has not expired, we must keep in the revoked token table. + else: + # The token is no longer valid, remove from the revoked token table. + db.session.delete(revoked_token) + + db.session.commit() diff --git a/securedrop/models.py b/securedrop/models.py index 7b5c5509fa..f98fe77892 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -567,6 +567,16 @@ def generate_api_token(self, expiration): current_app.config['SECRET_KEY'], expires_in=expiration) return s.dumps({'id': self.id}).decode('ascii') + @staticmethod + def validate_token_is_not_expired_or_invalid(token): + s = TimedJSONWebSignatureSerializer(current_app.config['SECRET_KEY']) + try: + s.loads(token) + except BadData: + return None + + return True + @staticmethod def validate_api_token_and_get_user(token): s = TimedJSONWebSignatureSerializer(current_app.config['SECRET_KEY']) From 0d7e702a5f04f1e9e544b4e99eb57f6a9ddeabbe Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Tue, 30 Apr 2019 16:50:00 -0700 Subject: [PATCH 6/8] test: cover new utility function for revoked token cleanup --- securedrop/tests/test_journalist_utils.py | 42 +++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 securedrop/tests/test_journalist_utils.py diff --git a/securedrop/tests/test_journalist_utils.py b/securedrop/tests/test_journalist_utils.py new file mode 100644 index 0000000000..441cd083dd --- /dev/null +++ b/securedrop/tests/test_journalist_utils.py @@ -0,0 +1,42 @@ +# -*- coding: utf-8 -*- +from flask import url_for +import os +import pytest +import random + +from models import RevokedToken +from sqlalchemy.orm.exc import NoResultFound + +from journalist_app.utils import cleanup_expired_revoked_tokens + +os.environ['SECUREDROP_ENV'] = 'test' # noqa +from .utils.api_helper import get_api_headers + +random.seed('◔ ⌣ ◔') + + +def test_revoke_token_cleanup_does_not_delete_tokens_if_not_expired(journalist_app, test_journo, + journalist_api_token): + with journalist_app.test_client() as app: + resp = app.post(url_for('api.logout'), headers=get_api_headers(journalist_api_token)) + assert resp.status_code == 200 + + cleanup_expired_revoked_tokens() + + revoked_token = RevokedToken.query.filter_by(token=journalist_api_token).one() + assert revoked_token.journalist_id == test_journo['id'] + + +def test_revoke_token_cleanup_does_deletes_tokens_that_are_expired(journalist_app, test_journo, + journalist_api_token, mocker): + with journalist_app.test_client() as app: + resp = app.post(url_for('api.logout'), headers=get_api_headers(journalist_api_token)) + assert resp.status_code == 200 + + # Mock response from expired token method when token is expired + mocker.patch('journalist_app.admin.Journalist.validate_token_is_not_expired_or_invalid', + return_value=None) + cleanup_expired_revoked_tokens() + + with pytest.raises(NoResultFound): + RevokedToken.query.filter_by(token=journalist_api_token).one() From a1ca5ebec6c5782c3fe85b58c729ac39613f36ba Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Tue, 30 Apr 2019 21:07:28 -0700 Subject: [PATCH 7/8] test: tests that don't use the app fixtures need to create a db --- securedrop/tests/test_i18n.py | 3 +++ securedrop/tests/test_template_filters.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/securedrop/tests/test_i18n.py b/securedrop/tests/test_i18n.py index 1e6dbd9f71..4e57103078 100644 --- a/securedrop/tests/test_i18n.py +++ b/securedrop/tests/test_i18n.py @@ -25,6 +25,7 @@ os.environ['SECUREDROP_ENV'] = 'test' # noqa from sdconfig import SDConfig +from db import db import i18n import i18n_tool import journalist_app as journalist_app_module @@ -217,6 +218,8 @@ def test_i18n(journalist_app, config): # grabs values at init time and we can't inject them later. for app in (journalist_app_module.create_app(fake_config), source_app.create_app(fake_config)): + with app.app_context(): + db.create_all() assert i18n.LOCALES == fake_config.SUPPORTED_LOCALES verify_i18n(app) diff --git a/securedrop/tests/test_template_filters.py b/securedrop/tests/test_template_filters.py index 69e7d07ba9..53c6a0b70f 100644 --- a/securedrop/tests/test_template_filters.py +++ b/securedrop/tests/test_template_filters.py @@ -5,6 +5,7 @@ from flask import session os.environ['SECUREDROP_ENV'] = 'test' # noqa +from db import db import i18n import i18n_tool import journalist_app @@ -110,6 +111,8 @@ def do_test(config, create_app): pybabel('init', '-i', pot, '-d', config.TEMP_DIR, '-l', l) app = create_app(config) + with app.app_context(): + db.create_all() assert i18n.LOCALES == config.SUPPORTED_LOCALES verify_filesizeformat(app) From 8b1e3e51ae50478d7b2e078b2702a8e15404fec7 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Tue, 30 Apr 2019 21:14:52 -0700 Subject: [PATCH 8/8] docs: update journalist API documentation for logout endpoint --- docs/development/journalist_api.rst | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/docs/development/journalist_api.rst b/docs/development/journalist_api.rst index 2f5c500cec..d72e62cf9c 100644 --- a/docs/development/journalist_api.rst +++ b/docs/development/journalist_api.rst @@ -56,11 +56,22 @@ HTTP Authorization header: Authorization: Token eyJhbGciOiJIUzI1NiIsImV4cCI6MTUzMDU4NjU4MiwifWF0IjoxNTMwNTc5MzgyfQ.eyJpZCI6MX0.P_PfcLMk1Dq5VCIANo-lJbu0ZyCL2VcT8qf9fIZsTCM This header will be checked with each API request to see if it is valid and -not yet expired. Tokens currently expire after 8 hours, but note that clients -should use the expiration time provided in the response to determine when -the token will expire. After the token expires point, users must -login again. Clients implementing logout functionality should delete tokens -locally upon logout. +not yet expired. Tokens currently expire after 8 hours. + +Logout +------ + +Clients should use the logout endpoint to invalidate their token: + +``POST /api/v1/logout`` with the token in the HTTP Authorization header +and you will get the following response upon successful invalidation of the +API token: + +.. code:: sh + + { + "message": "Your token has been revoked." + } Errors ~~~~~~