From 7cf565ed7e6e65a0b6ecd5ad37fbbdbb792a55f9 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Wed, 14 Oct 2020 21:03:54 -0400 Subject: [PATCH 1/8] Add check for login state --- securedrop/source_app/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 94bed90ef4..202e7db863 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -121,7 +121,7 @@ def load_instance_config() -> None: def setup_g() -> Optional[werkzeug.Response]: """Store commonly used values in Flask's special g object""" - if 'expires' in session and datetime.utcnow() >= session['expires']: + if 'expires' in session and datetime.utcnow() >= session['expires'] and logged_in(): msg = render_template('session_timeout.html') # clear the session after we render the message so it's localized From 78654c0403bbbb5ff1f98ccef9909e6d1652c174 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Wed, 14 Oct 2020 21:04:04 -0400 Subject: [PATCH 2/8] Add test to check no message is displayed --- securedrop/tests/test_source.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 090924775d..595a2fec83 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -758,6 +758,24 @@ def test_source_session_expiration_create(config, source_app): assert 'You were logged out due to inactivity' in text +def test_source_no_session_expiration_message_when_not_logged_in(config, source_app): + """If sources never logged in, no message should be displayed + after SESSION_EXPIRATION_MINUTES.""" + + with source_app.test_client() as app: + seconds_session_expire = 1 + config.SESSION_EXPIRATION_MINUTES = seconds_session_expire / 60. + + resp = app.get(url_for('main.index')) + assert resp.status_code == 200 + + time.sleep(seconds_session_expire + 1) + + refreshed_resp = app.get(url_for('main.index'), follow_redirects=True) + text = refreshed_resp.data.decode('utf-8') + assert 'You were logged out due to inactivity' not in text + + def test_csrf_error_page(config, source_app): source_app.config['WTF_CSRF_ENABLED'] = True with source_app.test_client() as app: From b955a30ee28756f7737fdf550bde029252fdd424 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Fri, 16 Oct 2020 21:08:24 -0400 Subject: [PATCH 3/8] Remove new keys when checking when empty session --- securedrop/tests/test_source.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 595a2fec83..13e65db305 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -727,6 +727,8 @@ def test_source_session_expiration(config, source_app): # which is always present and 'csrf_token' which leaks no info) session.pop('expires', None) session.pop('csrf_token', None) + session.pop('generate_flow_record', None) + session.pop('login_record', None) assert not session text = resp.data.decode('utf-8') @@ -752,6 +754,8 @@ def test_source_session_expiration_create(config, source_app): # which is always present and 'csrf_token' which leaks no info) session.pop('expires', None) session.pop('csrf_token', None) + session.pop('generate_flow_record', None) + session.pop('login_record', None) assert not session text = resp.data.decode('utf-8') From a7c4c708628d3c3ace5114b69890e10649016e10 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Fri, 16 Oct 2020 21:09:41 -0400 Subject: [PATCH 4/8] Use new properties to distinguish users --- securedrop/source_app/__init__.py | 22 +++++++++++++++++----- securedrop/source_app/utils.py | 2 ++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 202e7db863..5a57d28914 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -23,7 +23,7 @@ from sdconfig import SDConfig from source_app import main, info, api from source_app.decorators import ignore_static -from source_app.utils import logged_in +from source_app.utils import logged_in, was_in_generate_flow from store import Storage @@ -109,7 +109,7 @@ def check_tor2web() -> None: 'provide anonymity. ' 'Why is this dangerous?') .format(url=url_for('info.tor2web_warning'))), - "banner-warning") + "banner-warning") @app.before_request @ignore_static @@ -121,14 +121,26 @@ def load_instance_config() -> None: def setup_g() -> Optional[werkzeug.Response]: """Store commonly used values in Flask's special g object""" - if 'expires' in session and datetime.utcnow() >= session['expires'] and logged_in(): + if 'expires' in session and datetime.utcnow() >= session['expires']: msg = render_template('session_timeout.html') + # Before the session is cleared, record the fact that + # the user was in the codename generation flow or logged in + generate_flow_record = session.get('generate_flow_record', was_in_generate_flow()) + login_record = session.get('login_record', logged_in()) + # clear the session after we render the message so it's localized session.clear() - # Redirect to index with flashed message - flash(Markup(msg), "important") + # Persist these records across sessions to distinguish users whose sessions expired + # from users who never logged in or generated a codename + session['generate_flow_record'] = generate_flow_record + session['login_record'] = login_record + + # Redirect to index with flashed message only if + # the user has ever logged in OR has ever gone through the codename generation flow + if session['generate_flow_record'] or session['login_record']: + flash(Markup(msg), "important") return redirect(url_for('main.index')) session['expires'] = datetime.utcnow() + \ diff --git a/securedrop/source_app/utils.py b/securedrop/source_app/utils.py index 6c72a6de9a..a79c1b0de7 100644 --- a/securedrop/source_app/utils.py +++ b/securedrop/source_app/utils.py @@ -20,6 +20,8 @@ if typing.TYPE_CHECKING: from typing import Optional # noqa: F401 +def was_in_generate_flow() -> bool: + return 'codenames' in session def logged_in() -> bool: return 'logged_in' in session From 02bb9d6d05877a63e66941e9fd44c665a0175537 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Fri, 16 Oct 2020 21:09:51 -0400 Subject: [PATCH 5/8] Change deprecated method --- securedrop/source_app/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index b7092e99ed..d86d4940e8 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -257,7 +257,7 @@ def submit() -> werkzeug.Response: current_app.logger.info("generating key, entropy: {}".format( entropy_avail)) else: - current_app.logger.warn( + current_app.logger.warning( "skipping key generation. entropy: {}".format( entropy_avail)) From 957998d6460a67fcf4dac671b50378046ea8cc77 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Fri, 16 Oct 2020 21:37:25 -0400 Subject: [PATCH 6/8] Make linter happy --- securedrop/source_app/utils.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/securedrop/source_app/utils.py b/securedrop/source_app/utils.py index a79c1b0de7..b2fa4c1405 100644 --- a/securedrop/source_app/utils.py +++ b/securedrop/source_app/utils.py @@ -20,9 +20,11 @@ if typing.TYPE_CHECKING: from typing import Optional # noqa: F401 + def was_in_generate_flow() -> bool: return 'codenames' in session + def logged_in() -> bool: return 'logged_in' in session From b579ced3ca021a13abf8e34c92f91686aa0533a6 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Mon, 26 Oct 2020 21:43:02 -0400 Subject: [PATCH 7/8] Use a single session property show_expiration_message --- securedrop/source_app/__init__.py | 20 ++++++++++---------- securedrop/tests/test_source.py | 6 ++---- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 5a57d28914..4a2bea453e 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -124,22 +124,22 @@ def setup_g() -> Optional[werkzeug.Response]: if 'expires' in session and datetime.utcnow() >= session['expires']: msg = render_template('session_timeout.html') - # Before the session is cleared, record the fact that - # the user was in the codename generation flow or logged in - generate_flow_record = session.get('generate_flow_record', was_in_generate_flow()) - login_record = session.get('login_record', logged_in()) + # Show expiration message only if the user was in the codename generation flow or was logged in + show_expiration_message = any([ + session.get('show_expiration_message'), + logged_in(), + was_in_generate_flow(), + ]) # clear the session after we render the message so it's localized session.clear() - # Persist these records across sessions to distinguish users whose sessions expired + # Persist this properety across sessions to distinguish users whose sessions actually expired # from users who never logged in or generated a codename - session['generate_flow_record'] = generate_flow_record - session['login_record'] = login_record + session['show_expiration_message'] = show_expiration_message - # Redirect to index with flashed message only if - # the user has ever logged in OR has ever gone through the codename generation flow - if session['generate_flow_record'] or session['login_record']: + # Redirect to index with flashed message + if session['show_expiration_message']: flash(Markup(msg), "important") return redirect(url_for('main.index')) diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 13e65db305..a434ecc7a0 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -727,8 +727,7 @@ def test_source_session_expiration(config, source_app): # which is always present and 'csrf_token' which leaks no info) session.pop('expires', None) session.pop('csrf_token', None) - session.pop('generate_flow_record', None) - session.pop('login_record', None) + session.pop('show_expiration_message', None) assert not session text = resp.data.decode('utf-8') @@ -754,8 +753,7 @@ def test_source_session_expiration_create(config, source_app): # which is always present and 'csrf_token' which leaks no info) session.pop('expires', None) session.pop('csrf_token', None) - session.pop('generate_flow_record', None) - session.pop('login_record', None) + session.pop('show_expiration_message', None) assert not session text = resp.data.decode('utf-8') From cff3019b76cd05c72922d9f60d8fb6210ef244e9 Mon Sep 17 00:00:00 2001 From: Sheon Han Date: Mon, 26 Oct 2020 21:54:05 -0400 Subject: [PATCH 8/8] Fix linting errors --- securedrop/source_app/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 4a2bea453e..d5ea2333f1 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -124,7 +124,8 @@ def setup_g() -> Optional[werkzeug.Response]: if 'expires' in session and datetime.utcnow() >= session['expires']: msg = render_template('session_timeout.html') - # Show expiration message only if the user was in the codename generation flow or was logged in + # Show expiration message only if the user was + # either in the codename generation flow or logged in show_expiration_message = any([ session.get('show_expiration_message'), logged_in(), @@ -134,7 +135,7 @@ def setup_g() -> Optional[werkzeug.Response]: # clear the session after we render the message so it's localized session.clear() - # Persist this properety across sessions to distinguish users whose sessions actually expired + # Persist this properety across sessions to distinguish users whose sessions expired # from users who never logged in or generated a codename session['show_expiration_message'] = show_expiration_message