From 27d198363d7ad862e7fde7c535d7fe34b8cdd500 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Sun, 1 Oct 2017 16:06:21 +0200 Subject: [PATCH 1/5] added session expiration to journalist interface --- securedrop/journalist.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/securedrop/journalist.py b/securedrop/journalist.py index f5d2f71cd7..467adf5d33 100644 --- a/securedrop/journalist.py +++ b/securedrop/journalist.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- -from datetime import datetime +from datetime import datetime, timedelta import functools from flask import (Flask, request, render_template, send_file, redirect, flash, @@ -65,6 +65,13 @@ def get_source(filesystem_id): @app.before_request def setup_g(): """Store commonly used values in Flask's special g object""" + if 'expires' in session and datetime.utcnow() >= session['expires']: + session.clear() + flash(gettext('You have been logged out due to inactivity'), 'error') + + session['expires'] = datetime.utcnow() + \ + timedelta(minutes=getattr(config, 'SESSION_EXPIRATION_MINUTES', 30)) + uid = session.get('uid', None) if uid: g.user = Journalist.query.get(uid) @@ -162,6 +169,7 @@ def login(): @app.route('/logout') def logout(): session.pop('uid', None) + session.pop('expires', None) return redirect(url_for('index')) From 51f300869f6b60b7618db24da5fa11bcf8a82681 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Sun, 1 Oct 2017 16:06:44 +0200 Subject: [PATCH 2/5] added session expiration to source interface --- securedrop/source.py | 1 + securedrop/source_app/__init__.py | 11 +++++++++++ securedrop/tests/test_source.py | 4 ++++ 3 files changed, 16 insertions(+) diff --git a/securedrop/source.py b/securedrop/source.py index 60e4de8153..d05864491f 100644 --- a/securedrop/source.py +++ b/securedrop/source.py @@ -1,4 +1,5 @@ # -*- coding: utf-8 -*- + import config from source_app import create_app diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 5f90b5ce09..623b66942a 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -1,3 +1,4 @@ +from datetime import datetime, timedelta from flask import (Flask, render_template, flash, Markup, request, g, session, url_for, redirect) from flask_babel import gettext @@ -76,6 +77,16 @@ def setup_g(): g.html_lang = i18n.locale_to_rfc_5646(g.locale) g.locales = i18n.get_locale2name() + if 'expires' in session and datetime.utcnow() >= session['expires']: + session.clear() + flash(gettext('You have been logged out due to inactivity'), + 'error') + + session['expires'] = datetime.utcnow() + \ + timedelta(minutes=getattr(config, + 'SESSION_EXPIRATION_MINUTES', + 30)) + # ignore_static here because `crypto_util.hash_codename` is scrypt # (very time consuming), and we don't need to waste time running if # we're just serving a static resource that won't need to access diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index c8e5ec0320..ed4d3ea52b 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -166,7 +166,11 @@ def test_login_and_logout(self): self.assertEqual(resp.status_code, 200) self.assertTrue(session['logged_in']) resp = c.get('/logout', follow_redirects=True) + + # sessions always have 'expires', so pop it for the next check + session.pop('expires', None) self.assertTrue(not session) + self.assertIn('Thank you for exiting your session!', resp.data) def test_user_must_log_in_for_protected_views(self): From 47cb1df4841ffea94276e3a16f6b7712386eb651 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Sun, 1 Oct 2017 16:48:14 +0200 Subject: [PATCH 3/5] refactor tests to work around issue #1444 --- securedrop/tests/test_source.py | 251 ++++++++++++++-------------- securedrop/tests/utils/db_helper.py | 12 +- 2 files changed, 133 insertions(+), 130 deletions(-) diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index ed4d3ea52b..53e2f619c5 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -14,6 +14,8 @@ import version import utils import json +import config +from utils.db_helper import new_codename overly_long_codename = 'a' * (Source.MAX_CODENAME_LEN + 1) @@ -76,15 +78,16 @@ def test_generate_has_login_link(self): self.assertEqual(already_have_codename_link['href'], '/login') def test_generate_already_logged_in(self): - self._new_codename() - # Make sure it redirects to /lookup when logged in - resp = self.client.get('/generate') - self.assertEqual(resp.status_code, 302) - # Make sure it flashes the message on the lookup page - resp = self.client.get('/generate', follow_redirects=True) - # Should redirect to /lookup - self.assertEqual(resp.status_code, 200) - self.assertIn("because you are already logged in.", resp.data) + with self.client as client: + new_codename(client, session) + # Make sure it redirects to /lookup when logged in + resp = client.get('/generate') + self.assertEqual(resp.status_code, 302) + # Make sure it flashes the message on the lookup page + resp = client.get('/generate', follow_redirects=True) + # Should redirect to /lookup + self.assertEqual(resp.status_code, 200) + self.assertIn("because you are already logged in.", resp.data) def test_create_new_source(self): with self.client as c: @@ -124,33 +127,31 @@ def test_create_duplicate_codename(self, logger): self.assertIn("Attempt to create a source with duplicate codename", logger.call_args[0][0]) - def _new_codename(self): - return utils.db_helper.new_codename(self.client, session) - def test_lookup(self): """Test various elements on the /lookup page.""" - codename = self._new_codename() - resp = self.client.post('login', data=dict(codename=codename), - follow_redirects=True) - # redirects to /lookup - self.assertIn("public key", resp.data) - # download the public key - resp = self.client.get('journalist-key') - self.assertIn("BEGIN PGP PUBLIC KEY BLOCK", resp.data) + with self.client as client: + codename = new_codename(client, session) + resp = client.post('login', data=dict(codename=codename), + follow_redirects=True) + # redirects to /lookup + self.assertIn("public key", resp.data) + # download the public key + resp = client.get('journalist-key') + self.assertIn("BEGIN PGP PUBLIC KEY BLOCK", resp.data) def test_login_and_logout(self): resp = self.client.get('/login') self.assertEqual(resp.status_code, 200) self.assertIn("Enter Codename", resp.data) - codename = self._new_codename() - with self.client as c: - resp = c.post('/login', data=dict(codename=codename), - follow_redirects=True) + with self.client as client: + codename = new_codename(client, session) + resp = client.post('/login', data=dict(codename=codename), + follow_redirects=True) self.assertEqual(resp.status_code, 200) self.assertIn("Submit Materials", resp.data) self.assertTrue(session['logged_in']) - resp = c.get('/logout', follow_redirects=True) + resp = client.get('/logout', follow_redirects=True) with self.client as c: resp = c.post('/login', data=dict(codename='invalid'), @@ -182,23 +183,24 @@ def test_user_must_log_in_for_protected_views(self): def test_login_with_whitespace(self): """ Test that codenames with leading or trailing whitespace still work""" - def login_test(codename): - resp = self.client.get('/login') - self.assertEqual(resp.status_code, 200) - self.assertIn("Enter Codename", resp.data) - with self.client as c: - resp = c.post('/login', data=dict(codename=codename), - follow_redirects=True) + with self.client as client: + def login_test(codename): + resp = client.get('/login') + self.assertEqual(resp.status_code, 200) + self.assertIn("Enter Codename", resp.data) + + resp = client.post('/login', data=dict(codename=codename), + follow_redirects=True) self.assertEqual(resp.status_code, 200) self.assertIn("Submit Materials", resp.data) self.assertTrue(session['logged_in']) - resp = c.get('/logout', follow_redirects=True) + resp = client.get('/logout', follow_redirects=True) - codename = self._new_codename() - login_test(codename + ' ') - login_test(' ' + codename + ' ') - login_test(' ' + codename) + codename = new_codename(client, session) + login_test(codename + ' ') + login_test(' ' + codename + ' ') + login_test(' ' + codename) def _dummy_submission(self): """ @@ -217,31 +219,34 @@ def test_initial_submission_notification(self): first submission is always greeted with a notification reminding sources to check back later for replies. """ - self._new_codename() - resp = self._dummy_submission() - self.assertEqual(resp.status_code, 200) - self.assertIn( - "Thank you for sending this information to us.", - resp.data) + with self.client as client: + new_codename(client, session) + resp = self._dummy_submission() + self.assertEqual(resp.status_code, 200) + self.assertIn( + "Thank you for sending this information to us.", + resp.data) def test_submit_message(self): - self._new_codename() - self._dummy_submission() - resp = self.client.post('/submit', data=dict( - msg="This is a test.", - fh=(StringIO(''), ''), - ), follow_redirects=True) - self.assertEqual(resp.status_code, 200) - self.assertIn("Thanks! We received your message", resp.data) + with self.client as client: + new_codename(client, session) + self._dummy_submission() + resp = client.post('/submit', data=dict( + msg="This is a test.", + fh=(StringIO(''), ''), + ), follow_redirects=True) + self.assertEqual(resp.status_code, 200) + self.assertIn("Thanks! We received your message", resp.data) def test_submit_empty_message(self): - self._new_codename() - resp = self.client.post('/submit', data=dict( - msg="", - fh=(StringIO(''), ''), - ), follow_redirects=True) - self.assertIn("You must enter a message or choose a file to submit.", - resp.data) + with self.client as client: + new_codename(client, session) + resp = self.client.post('/submit', data=dict( + msg="", + fh=(StringIO(''), ''), + ), follow_redirects=True) + self.assertIn("You must enter a message or choose a file to submit.", + resp.data) def test_submit_big_message(self): ''' @@ -249,35 +254,38 @@ def test_submit_big_message(self): just residing in memory. Make sure the different return type of SecureTemporaryFile is handled as well as BytesIO. ''' - self._new_codename() - self._dummy_submission() - resp = self.client.post('/submit', data=dict( - msg="AA" * (1024 * 512), - fh=(StringIO(''), ''), - ), follow_redirects=True) - self.assertEqual(resp.status_code, 200) - self.assertIn("Thanks! We received your message", resp.data) + with self.client as client: + new_codename(client, session) + self._dummy_submission() + resp = client.post('/submit', data=dict( + msg="AA" * (1024 * 512), + fh=(StringIO(''), ''), + ), follow_redirects=True) + self.assertEqual(resp.status_code, 200) + self.assertIn("Thanks! We received your message", resp.data) def test_submit_file(self): - self._new_codename() - self._dummy_submission() - resp = self.client.post('/submit', data=dict( - msg="", - fh=(StringIO('This is a test'), 'test.txt'), - ), follow_redirects=True) - self.assertEqual(resp.status_code, 200) - self.assertIn('Thanks! We received your document', resp.data) + with self.client as client: + new_codename(client, session) + self._dummy_submission() + resp = client.post('/submit', data=dict( + msg="", + fh=(StringIO('This is a test'), 'test.txt'), + ), follow_redirects=True) + self.assertEqual(resp.status_code, 200) + self.assertIn('Thanks! We received your document', resp.data) def test_submit_both(self): - self._new_codename() - self._dummy_submission() - resp = self.client.post('/submit', data=dict( - msg="This is a test", - fh=(StringIO('This is a test'), 'test.txt'), - ), follow_redirects=True) - self.assertEqual(resp.status_code, 200) - self.assertIn("Thanks! We received your message and document", - resp.data) + with self.client as client: + new_codename(client, session) + self._dummy_submission() + resp = client.post('/submit', data=dict( + msg="This is a test", + fh=(StringIO('This is a test'), 'test.txt'), + ), follow_redirects=True) + self.assertEqual(resp.status_code, 200) + self.assertIn("Thanks! We received your message and document", + resp.data) def test_delete_all_successfully_deletes_replies(self): journalist, _ = utils.db_helper.init_journalist() @@ -313,14 +321,15 @@ def test_submit_sanitizes_filename(self, gzipfile): insecure_filename = '../../bin/gpg' sanitized_filename = 'bin_gpg' - self._new_codename() - self.client.post('/submit', data=dict( - msg="", - fh=(StringIO('This is a test'), insecure_filename), - ), follow_redirects=True) - gzipfile.assert_called_with(filename=sanitized_filename, - mode=ANY, - fileobj=ANY) + with self.client as client: + new_codename(client, session) + client.post('/submit', data=dict( + msg="", + fh=(StringIO('This is a test'), insecure_filename), + ), follow_redirects=True) + gzipfile.assert_called_with(filename=sanitized_filename, + mode=ANY, + fileobj=ANY) def test_tor2web_warning_headers(self): resp = self.client.get('/', headers=[('X-tor2web', 'encrypted')]) @@ -372,19 +381,20 @@ def test_failed_normalize_timestamps_logs_warning(self, call, logger): still occur, but a warning should be logged (this will trigger an OSSEC alert).""" - self._new_codename() - self._dummy_submission() - resp = self.client.post('/submit', data=dict( - msg="This is a test.", - fh=(StringIO(''), ''), - ), follow_redirects=True) - self.assertEqual(resp.status_code, 200) - self.assertIn("Thanks! We received your message", resp.data) + with self.client as client: + new_codename(client, session) + self._dummy_submission() + resp = client.post('/submit', data=dict( + msg="This is a test.", + fh=(StringIO(''), ''), + ), follow_redirects=True) + self.assertEqual(resp.status_code, 200) + self.assertIn("Thanks! We received your message", resp.data) - logger.assert_called_once_with( - "Couldn't normalize submission " - "timestamps (touch exited with 1)" - ) + logger.assert_called_once_with( + "Couldn't normalize submission " + "timestamps (touch exited with 1)" + ) @patch('source.app.logger.error') def test_source_is_deleted_while_logged_in(self, logger): @@ -392,27 +402,24 @@ def test_source_is_deleted_while_logged_in(self, logger): a NoResultFound will occur. The source should be redirected to the index when this happens, and a warning logged.""" - codename = self._new_codename() - resp = self.client.post('login', data=dict(codename=codename), - follow_redirects=True) + with self.client as client: + codename = new_codename(client, session) + resp = client.post('login', data=dict(codename=codename), + follow_redirects=True) - # Now the journalist deletes the source - filesystem_id = crypto_util.hash_codename(codename) - crypto_util.delete_reply_keypair(filesystem_id) - source = Source.query.filter_by(filesystem_id=filesystem_id).one() - db_session.delete(source) - db_session.commit() + # Now the journalist deletes the source + filesystem_id = crypto_util.hash_codename(codename) + crypto_util.delete_reply_keypair(filesystem_id) + source = Source.query.filter_by(filesystem_id=filesystem_id).one() + db_session.delete(source) + db_session.commit() - # Source attempts to continue to navigate - resp = self.client.post('/lookup', follow_redirects=True) - self.assertEqual(resp.status_code, 200) - self.assertIn('Submit documents for the first time', resp.data) - self.assertNotIn('logged_in', session.keys()) - self.assertNotIn('codename', session.keys()) - - logger.assert_called_once_with( - "Found no Sources when one was expected: " - "No row was found for one()") + # Source attempts to continue to navigate + resp = client.post('/lookup', follow_redirects=True) + self.assertEqual(resp.status_code, 200) + self.assertIn('Submit documents for the first time', resp.data) + self.assertNotIn('logged_in', session.keys()) + self.assertNotIn('codename', session.keys()) def test_login_with_invalid_codename(self): """Logging in with a codename with invalid characters should return diff --git a/securedrop/tests/utils/db_helper.py b/securedrop/tests/utils/db_helper.py index f085fa5f5f..3d35bbff13 100644 --- a/securedrop/tests/utils/db_helper.py +++ b/securedrop/tests/utils/db_helper.py @@ -144,15 +144,11 @@ def submit(source, num_submissions): return submissions -# NOTE: this method is potentially dangerous to rely on for now due -# to the fact flask_testing.TestCase only uses on request context -# per method (see -# https://github.com/freedomofpress/securedrop/issues/1444). def new_codename(client, session): """Helper function to go through the "generate codename" flow. """ - with client as c: - c.get('/generate') - codename = session['codename'] - c.post('/create') + client.get('/generate') + print(session) + codename = session['codename'] + client.post('/create') return codename From d3fbac0814969a89f7368c94e698b25e2729cf8d Mon Sep 17 00:00:00 2001 From: heartsucker Date: Thu, 5 Oct 2017 10:19:39 +0200 Subject: [PATCH 4/5] added session expiration tests --- securedrop/tests/test_journalist.py | 38 ++++++++++++++++++- securedrop/tests/test_source.py | 58 ++++++++++++++++++++++++----- securedrop/tests/utils/db_helper.py | 4 +- 3 files changed, 88 insertions(+), 12 deletions(-) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index d6f6e40383..6b277960ab 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -5,7 +5,7 @@ import unittest import zipfile -from flask import url_for, escape +from flask import url_for, escape, session from flask_testing import TestCase from mock import patch, ANY, MagicMock from sqlalchemy.orm.exc import StaleDataError @@ -965,6 +965,42 @@ def test_add_star_redirects_to_index(self): filesystem_id=source.filesystem_id)) self.assertRedirects(resp, url_for('index')) + def test_journalist_session_expiration(self): + try: + old_expiration = config.SESSION_EXPIRATION_MINUTES + has_session_expiration = True + except AttributeError: + has_session_expiration = False + + try: + with self.client as client: + # do a real login to get a real session + # (none of the mocking `g` hacks) + resp = self.client.post(url_for('login'), + data=dict(username=self.user.username, + password=VALID_PASSWORD, + token='mocked')) + assert resp.status_code == 200 + + # set the expiration to ensure we trigger an expiration + config.SESSION_EXPIRATION_MINUTES = -1 + + resp = client.get(url_for('edit_account'), + follow_redirects=True) + + # check that the session was cleared (apart from 'expires' + # which is always present and 'csrf_token' which leaks no info) + session.pop('expires', None) + session.pop('csrf_token', None) + assert not session, session + assert ('You have been logged out due to inactivity' in + resp.data.decode('utf-8')) + finally: + if has_session_expiration: + config.SESSION_EXPIRATION_MINUTES = old_expiration + else: + del config.SESSION_EXPIRATION_MINUTES + class TestJournalistAppTwo(unittest.TestCase): diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 53e2f619c5..d50aa85145 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -202,13 +202,13 @@ def login_test(codename): login_test(' ' + codename + ' ') login_test(' ' + codename) - def _dummy_submission(self): + def _dummy_submission(self, client): """ Helper to make a submission (content unimportant), mostly useful in testing notification behavior for a source's first vs. their subsequent submissions """ - return self.client.post('/submit', data=dict( + return client.post('/submit', data=dict( msg="Pay no attention to the man behind the curtain.", fh=(StringIO(''), ''), ), follow_redirects=True) @@ -221,7 +221,7 @@ def test_initial_submission_notification(self): """ with self.client as client: new_codename(client, session) - resp = self._dummy_submission() + resp = self._dummy_submission(client) self.assertEqual(resp.status_code, 200) self.assertIn( "Thank you for sending this information to us.", @@ -230,7 +230,7 @@ def test_initial_submission_notification(self): def test_submit_message(self): with self.client as client: new_codename(client, session) - self._dummy_submission() + self._dummy_submission(client) resp = client.post('/submit', data=dict( msg="This is a test.", fh=(StringIO(''), ''), @@ -241,11 +241,12 @@ def test_submit_message(self): def test_submit_empty_message(self): with self.client as client: new_codename(client, session) - resp = self.client.post('/submit', data=dict( + resp = client.post('/submit', data=dict( msg="", fh=(StringIO(''), ''), ), follow_redirects=True) - self.assertIn("You must enter a message or choose a file to submit.", + self.assertIn("You must enter a message or choose a file to " + "submit.", resp.data) def test_submit_big_message(self): @@ -256,7 +257,7 @@ def test_submit_big_message(self): ''' with self.client as client: new_codename(client, session) - self._dummy_submission() + self._dummy_submission(client) resp = client.post('/submit', data=dict( msg="AA" * (1024 * 512), fh=(StringIO(''), ''), @@ -267,7 +268,7 @@ def test_submit_big_message(self): def test_submit_file(self): with self.client as client: new_codename(client, session) - self._dummy_submission() + self._dummy_submission(client) resp = client.post('/submit', data=dict( msg="", fh=(StringIO('This is a test'), 'test.txt'), @@ -278,7 +279,7 @@ def test_submit_file(self): def test_submit_both(self): with self.client as client: new_codename(client, session) - self._dummy_submission() + self._dummy_submission(client) resp = client.post('/submit', data=dict( msg="This is a test", fh=(StringIO('This is a test'), 'test.txt'), @@ -383,7 +384,7 @@ def test_failed_normalize_timestamps_logs_warning(self, call, logger): with self.client as client: new_codename(client, session) - self._dummy_submission() + self._dummy_submission(client) resp = client.post('/submit', data=dict( msg="This is a test.", fh=(StringIO(''), ''), @@ -421,6 +422,10 @@ def test_source_is_deleted_while_logged_in(self, logger): self.assertNotIn('logged_in', session.keys()) self.assertNotIn('codename', session.keys()) + logger.assert_called_once_with( + "Found no Sources when one was expected: " + "No row was found for one()") + def test_login_with_invalid_codename(self): """Logging in with a codename with invalid characters should return an informative message to the user.""" @@ -432,3 +437,36 @@ def test_login_with_invalid_codename(self): follow_redirects=True) self.assertEqual(resp.status_code, 200) self.assertIn("Invalid input.", resp.data) + + def _test_source_session_expiration(self): + try: + old_expiration = config.SESSION_EXPIRATION_MINUTES + has_session_expiration = True + except AttributeError: + has_session_expiration = False + + try: + with self.client as client: + codename = new_codename(client, session) + + # set the expiration to ensure we trigger an expiration + config.SESSION_EXPIRATION_MINUTES = -1 + + resp = client.post('/login', + data=dict(codename=codename), + follow_redirects=True) + assert resp.status_code == 200 + resp = client.get('/lookup', follow_redirects=True) + + # check that the session was cleared (apart from 'expires' + # which is always present and 'csrf_token' which leaks no info) + session.pop('expires', None) + session.pop('csrf_token', None) + assert not session, session + assert ('You have been logged out due to inactivity' in + resp.data.decode('utf-8')) + finally: + if has_session_expiration: + config.SESSION_EXPIRATION_MINUTES = old_expiration + else: + del config.SESSION_EXPIRATION_MINUTES diff --git a/securedrop/tests/utils/db_helper.py b/securedrop/tests/utils/db_helper.py index 3d35bbff13..bcd9e15e41 100644 --- a/securedrop/tests/utils/db_helper.py +++ b/securedrop/tests/utils/db_helper.py @@ -147,8 +147,10 @@ def submit(source, num_submissions): def new_codename(client, session): """Helper function to go through the "generate codename" flow. """ + # clear the session because our tests have implicit reliance on each other + session.clear() + client.get('/generate') - print(session) codename = session['codename'] client.post('/create') return codename From 5090c3d38b0c7decfcf48cc675c024d6fead2c91 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Thu, 5 Oct 2017 10:41:16 +0200 Subject: [PATCH 5/5] added SESSION_EXPIRATION_MINUTES to example config --- securedrop/config.py.example | 3 +++ 1 file changed, 3 insertions(+) diff --git a/securedrop/config.py.example b/securedrop/config.py.example index 9eb9dab691..5d37be1155 100644 --- a/securedrop/config.py.example +++ b/securedrop/config.py.example @@ -89,3 +89,6 @@ DATABASE_FILE = os.path.join(SECUREDROP_DATA_ROOT, 'db.sqlite') #SUPPORTED_LOCALES = ['en_US', 'fr_FR', 'nb_NO'] # Which of the available locales should be displayed by default ? DEFAULT_LOCALE = 'en_US' + +# How long a session is valid before it expires and logs a user out +SESSION_EXPIRATION_MINUTES = 30