From a47b9bd73b8c1aacd030c7ce6ab1217ddebf509a Mon Sep 17 00:00:00 2001 From: Giulio Date: Wed, 25 May 2022 10:51:58 +0000 Subject: [PATCH] Rebase: add flash messaging for expiration, logout, etc. --- securedrop/i18n.py | 2 +- securedrop/journalist_app/__init__.py | 5 ++- securedrop/journalist_app/account.py | 10 +++--- securedrop/journalist_app/admin.py | 4 +-- securedrop/journalist_app/col.py | 22 ++++-------- securedrop/journalist_app/decorators.py | 4 +-- securedrop/journalist_app/sessions.py | 45 ++++++++++++++++--------- securedrop/journalist_app/utils.py | 28 ++++++++------- securedrop/tests/test_integration.py | 8 ++--- securedrop/tests/test_journalist.py | 14 ++++++-- 10 files changed, 79 insertions(+), 63 deletions(-) diff --git a/securedrop/i18n.py b/securedrop/i18n.py index aef855504aa..d5895a2daf8 100644 --- a/securedrop/i18n.py +++ b/securedrop/i18n.py @@ -214,7 +214,7 @@ def get_locale(config: SDConfig) -> str: - config.FALLBACK_LOCALE """ preferences = [] - if session.get("locale"): + if session and session.get("locale"): preferences.append(session.get("locale")) if request.args.get("l"): preferences.insert(0, request.args.get("l")) diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index a2fa3478b3d..fba53d8e7ef 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -4,7 +4,7 @@ from datetime import datetime, timedelta, timezone from pathlib import Path from datetime import datetime -from flask import (Flask, redirect, url_for, flash, g, request, +from flask import (Flask, redirect, url_for, g, request, render_template, json, abort) from flask_assets import Environment from flask_babel import gettext @@ -96,9 +96,8 @@ def default(self, obj: "Any") -> "Any": @app.errorhandler(CSRFError) # type: ignore def handle_csrf_error(e: CSRFError) -> "Response": app.logger.error("The CSRF token is invalid.") - session.destroy() msg = gettext('You have been logged out due to inactivity.') - flash(msg, 'error') + session.destroy(('error', msg), session.get('locale')) return redirect(url_for('main.login')) def _handle_http_exception( diff --git a/securedrop/journalist_app/account.py b/securedrop/journalist_app/account.py index 0afc7d6b9de..9a3328c52f2 100644 --- a/securedrop/journalist_app/account.py +++ b/securedrop/journalist_app/account.py @@ -38,11 +38,11 @@ def new_password() -> werkzeug.Response: token = request.form.get('token') error_message = gettext('Incorrect password or two-factor code.') # If the user is validated, change their password - if validate_user(user.username, current_password, token, error_message): - password = request.form.get("password") - set_diceware_password(user, password) - session.destroy() - return redirect(url_for('main.login')) + if validate_user(user.username, current_password, token, + error_message): + password = request.form.get('password') + if set_diceware_password(user, password): + return redirect(url_for('main.login')) return redirect(url_for('account.edit')) @view.route("/2fa", methods=("GET", "POST")) diff --git a/securedrop/journalist_app/admin.py b/securedrop/journalist_app/admin.py index 5db35ad4fe0..cb5d4b46fa1 100644 --- a/securedrop/journalist_app/admin.py +++ b/securedrop/journalist_app/admin.py @@ -7,7 +7,7 @@ import werkzeug from flask import (Blueprint, render_template, request, url_for, redirect, g, - current_app, flash, abort, session) + current_app, flash, abort) from flask_babel import gettext from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound @@ -26,7 +26,7 @@ ) from flask_babel import gettext from journalist_app.decorators import admin_required -from journalist_app.sessions import logout_user +from journalist_app.sessions import logout_user, session from journalist_app.utils import (commit_account_changes, set_diceware_password, validate_hotp_secret) from journalist_app.forms import LogoForm, NewUserForm, SubmissionPreferencesForm, OrgNameForm diff --git a/securedrop/journalist_app/col.py b/securedrop/journalist_app/col.py index 5bee657dd82..31d064a6e11 100644 --- a/securedrop/journalist_app/col.py +++ b/securedrop/journalist_app/col.py @@ -12,7 +12,6 @@ current_app, escape, flash, - g, redirect, render_template, request, @@ -21,20 +20,11 @@ ) from flask_babel import gettext from journalist_app.forms import ReplyForm -from journalist_app.utils import ( - col_delete, - col_delete_data, - col_download_all, - col_download_unread, - col_star, - col_un_star, - delete_collection, - get_source, - make_star_false, - make_star_true, - mark_seen, -) -from models import Reply, Submission +from journalist_app.sessions import session +from journalist_app.utils import (make_star_true, make_star_false, get_source, + delete_collection, col_download_unread, + col_download_all, col_star, col_un_star, + col_delete, col_delete_data, mark_seen) from sdconfig import SDConfig from sqlalchemy.orm.exc import NoResultFound from store import Storage @@ -151,7 +141,7 @@ def download_single_file(filesystem_id: str, fn: str) -> werkzeug.Response: # mark as seen by the current user try: - journalist = g.get("user") + journalist = session.get_user() if fn.endswith("reply.gpg"): reply = Reply.query.filter(Reply.filename == fn).one() mark_seen([reply], journalist) diff --git a/securedrop/journalist_app/decorators.py b/securedrop/journalist_app/decorators.py index 6b9da2c58f7..a3ae61cc197 100644 --- a/securedrop/journalist_app/decorators.py +++ b/securedrop/journalist_app/decorators.py @@ -1,10 +1,10 @@ # -*- coding: utf-8 -*- from typing import Any -from flask import redirect, url_for, flash, session +from flask import redirect, url_for, flash from flask_babel import gettext from functools import wraps -from typing import Any, Callable +from journalist_app.sessions import session from typing import Callable diff --git a/securedrop/journalist_app/sessions.py b/securedrop/journalist_app/sessions.py index f26eb826f63..c08b915b65b 100644 --- a/securedrop/journalist_app/sessions.py +++ b/securedrop/journalist_app/sessions.py @@ -1,9 +1,10 @@ import typing from datetime import datetime, timedelta, timezone from secrets import token_urlsafe +from flask_babel import gettext from flask import Flask, Request, Response from flask import current_app as app -from typing import Optional, Any +from typing import Optional, Any, Dict, Tuple from flask.sessions import SessionInterface as FlaskSessionInterface from flask.sessions import SessionMixin, session_json_serializer from json.decoder import JSONDecodeError @@ -24,7 +25,7 @@ def on_update(self: ServerSideSession) -> None: self.set_uid(initial['uid']) self.set_user() else: - self.uid = None + self.uid: Optional[int] = None self.user = None CallbackDict.__init__(self, initial, on_update) self.sid = sid @@ -34,6 +35,8 @@ def on_update(self: ServerSideSession) -> None: self.to_destroy = False self.to_regenerate = False self.modified = False + self.flash: Optional[Tuple[str, str]] = None + self.locale: Optional[str] = None def get_token(self) -> Optional[str]: return self.token @@ -65,7 +68,14 @@ def logged_in(self) -> bool: else: return False - def destroy(self) -> None: + def destroy( + self, + flash: Optional[Tuple[str, str]] = None, + locale: Optional[str] = None + ) -> None: + # The parameters are needed to pass the information to the new session + self.locale = locale + self.flash = flash self.uid = None self.user = None self.to_destroy = True @@ -93,8 +103,6 @@ def _get_signer(self, app: Flask) -> URLSafeTimedSerializer: :param header_name: if use_header, set the header name to parse """ - session_class = ServerSideSession - def __init__(self, lifetime: int, renew_count: int, redis: Optional[Redis], key_prefix: str, salt: str, header_name: str) -> None: self.serializer = session_json_serializer @@ -111,10 +119,13 @@ def __init__(self, lifetime: int, renew_count: int, redis: Optional[Redis], self.new = False self.has_same_site_capability = hasattr(self, "get_cookie_samesite") - def _new_session(self, is_api: bool = False) -> ServerSideSession: + def _new_session(self, is_api: bool = False, initial: Any = None) -> ServerSideSession: sid = self._generate_sid() - token = self._get_signer(app).dumps(sid) - session = self.session_class(sid=sid, token=token, lifetime=self.lifetime) # type: ignore + token: str = self._get_signer(app).dumps(sid) # type: ignore + session = ServerSideSession(sid=sid, + token=token, + lifetime=self.lifetime, + initial=initial) session.new = True session.is_api = is_api return session @@ -150,15 +161,14 @@ def open_session(self, app: Flask, request: Request) -> Optional[ServerSideSessi if val is not None: try: data = self.serializer.loads(val.decode()) - - return self.session_class(sid=sid, - token=self._get_signer(app).dumps(sid), # type: ignore - initial=data) + token: str = self._get_signer(app).dumps(sid) # type: ignore + return ServerSideSession(sid=sid, token=token, initial=data) except (JSONDecodeError, NotImplementedError): return self._new_session(is_api) # signed session_id provided in cookie is valid, but the session is not on the server # anymore so maybe here is the code path for a meaningful error message - return self._new_session(is_api) + msg = gettext("You have been logged out due to inactivity.") + return self._new_session(is_api, initial={"_flashes": [("error", msg)]}) def save_session( # type: ignore[override] # noqa self, @@ -172,11 +182,14 @@ def save_session( # type: ignore[override] # noqa domain = self.get_cookie_domain(app) path = self.get_cookie_path(app) if session.to_destroy: + initial: Dict[str, Any] = {"locale": session.locale} + if session.flash: + initial["_flashes"] = [session.flash] self.redis.delete(self.key_prefix + session.sid) if not session.is_api: - response.delete_cookie(app.session_cookie_name, - domain=domain, path=path) - return + # Instead of deleting the cookie and send a new sid with the next request + # create the new session already, so we can pass along messages and locale + session = self._new_session(False, initial=initial) expires = self.redis.ttl(name=self.key_prefix + session.sid) if session.new: session['renew_count'] = self.renew_count diff --git a/securedrop/journalist_app/utils.py b/securedrop/journalist_app/utils.py index 850f17b327f..2a36ce7a475 100644 --- a/securedrop/journalist_app/utils.py +++ b/securedrop/journalist_app/utils.py @@ -6,8 +6,9 @@ import flask import werkzeug from flask import (flash, current_app, abort, send_file, redirect, url_for, - Markup, escape, session) + Markup, escape) from flask_babel import gettext, ngettext +from journalist_app.sessions import session from sqlalchemy.exc import IntegrityError from db import db @@ -466,19 +467,22 @@ def set_diceware_password(user: Journalist, password: Optional[str]) -> bool: return False # using Markup so the HTML isn't escaped - flash( - Markup( - "

{message} {password}

".format( - message=Markup.escape( - gettext( - "Password updated. Don't forget to save it in your KeePassX database. " - "New password:" - ) - ), - password=Markup.escape("" if password is None else password), + session.destroy( + ( + 'success', + Markup( + "

{message} {password}

".format( + message=Markup.escape( + gettext( + "Password updated. Don't forget to save it in your KeePassX database. " + "New password:" + ) + ), + password=Markup.escape("" if password is None else password) + ) ) ), - "success", + session.get('locale') ) return True diff --git a/securedrop/tests/test_integration.py b/securedrop/tests/test_integration.py index 93ee2df9ab5..06cbc1b8327 100644 --- a/securedrop/tests/test_integration.py +++ b/securedrop/tests/test_integration.py @@ -11,7 +11,7 @@ import mock from bs4 import BeautifulSoup -from flask import escape, session +from flask import escape from pyotp import HOTP, TOTP import journalist_app as journalist_app_module @@ -19,8 +19,8 @@ from bs4 import BeautifulSoup from db import db from encryption import EncryptionManager -from flask import escape, g, session -from pyotp import HOTP, TOTP +from store import Storage +from journalist_app.sessions import session from source_app.session_manager import SessionManager from store import Storage @@ -44,7 +44,7 @@ def _login_user(app, user_dict): follow_redirects=True, ) assert resp.status_code == 200 - assert user_dict['username'] in resp.data.decode('utf-8') + assert session.get_user() is not None def test_submit_message(journalist_app, source_app, test_journo, app_storage): diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 6e11bed6bce..e7528261d75 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -20,8 +20,18 @@ from db import db from encryption import EncryptionManager, GpgKeyNotFoundError from flaky import flaky -from flask import current_app, escape, g, session, url_for +from flask import current_app, escape, g, url_for from flask_babel import gettext, ngettext +from mock import patch +from pyotp import TOTP +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm.exc import StaleDataError +from sqlalchemy.sql.expression import func +from html import escape as htmlescape + +import journalist_app as journalist_app_module +from encryption import EncryptionManager, GpgKeyNotFoundError +from journalist_app.sessions import session from journalist_app.utils import mark_seen from mock import call, patch from models import ( @@ -68,7 +78,7 @@ def _login_user(app, username, password, otp_secret, success=True): follow_redirects=True, ) assert resp.status_code == 200 - assert ((username in resp.data.decode('utf-8')) == success) + assert ((session.get_user() is not None) == success) @pytest.mark.parametrize("otp_secret", ["", "GA", "GARBAGE", "JHCOGO7VCER3EJ4"])