Rebase: added and improved tests for logout; test for save/logout rac…

…e condition; minor fixes

securedrop/journalist_app/__init__.py

@@ -5,7 +5,7 @@
 from pathlib import Path
 from datetime import datetime
 from flask import (Flask, redirect, url_for, flash, g, request,
-                   render_template, json, abort, session)
+                   render_template, json, abort)
 from flask_assets import Environment
 from flask_babel import gettext
 from flask_wtf.csrf import CSRFProtect, CSRFError
@@ -29,8 +29,7 @@
 from journalist_app import account, admin, api, main, col
 from journalist_app.sessions import Session, session
 from journalist_app.utils import get_source
-from models import InstanceConfig, Journalist
-from werkzeug.exceptions import default_exceptions
+from models import InstanceConfig
 
 # https://www.python.org/dev/peps/pep-0484/#runtime-or-type-checking
 if typing.TYPE_CHECKING:

securedrop/journalist_app/admin.py

@@ -26,8 +26,9 @@
 )
 from flask_babel import gettext
 from journalist_app.decorators import admin_required
+from journalist_app.sessions import logout_user
 from journalist_app.utils import (commit_account_changes, set_diceware_password,
-                                  validate_hotp_secret, logout_user)
+                                  validate_hotp_secret)
 from journalist_app.forms import LogoForm, NewUserForm, SubmissionPreferencesForm, OrgNameForm
 from sdconfig import SDConfig
 from passphrases import PassphraseGenerator
@@ -345,7 +346,7 @@ def delete_user(user_id: int) -> werkzeug.Response:
             abort(403)
         else:
             user.delete()
-            logout_user(user.id)
+            logout_user(user.id, is_current=False)
             db.session.commit()
             flash(gettext("Deleted user '{user}'.").format(user=user.username), "notification")
 
@@ -361,7 +362,7 @@ def new_password(user_id: int) -> werkzeug.Response:
 
         password = request.form.get('password')
         if set_diceware_password(user, password) is not False:
-            logout_user(user.id)
+            logout_user(user.id, is_current=False)
             db.session.commit()
         return redirect(url_for("admin.edit_user", user_id=user_id))
 

securedrop/journalist_app/api.py

@@ -5,7 +5,7 @@
 
 import flask
 import werkzeug
-from flask import abort, Blueprint, jsonify, request, g
+from flask import abort, Blueprint, jsonify, request
 
 from sqlalchemy import Column
 from sqlalchemy.exc import IntegrityError

securedrop/journalist_app/decorators.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 from typing import Any
 
-from flask import redirect, url_for, flash, g, session
+from flask import redirect, url_for, flash, session
 from flask_babel import gettext
 from functools import wraps
 from typing import Any, Callable

securedrop/journalist_app/sessions.py

@@ -23,6 +23,9 @@ def on_update(self: ServerSideSession) -> None:
         if initial and 'uid' in initial:
             self.set_uid(initial['uid'])
             self.set_user()
+        else:
+            self.uid = None
+            self.user = None
         CallbackDict.__init__(self, initial, on_update)
         self.sid = sid
         self.token: str = token
@@ -32,36 +35,39 @@ def on_update(self: ServerSideSession) -> None:
         self.to_regenerate = False
         self.modified = False
 
-
     def get_token(self) -> Optional[str]:
         return self.token
 
     def get_lifetime(self) -> datetime:
         return datetime.now(timezone.utc) + timedelta(seconds=self.lifetime)
 
     def set_user(self) -> None:
-        if hasattr(self, 'uid') and self.uid is not None:
+        if self.uid is not None:
             self.user = Journalist.query.get(self.uid)
+        if self.user is None:
+            # The uid has no match in the database, and this should really not happen
+            self.uid = None
+            self.to_destroy = True
 
     def get_user(self) -> Optional[Journalist]:
-        if hasattr(self, 'user'):
-            return self.user
+        return self.user
 
     def get_uid(self) -> Optional[int]:
-        if hasattr(self, 'uid'):
-            return self.uid
+        return self.uid
 
-    def set_uid(self, uid) -> None:
+    def set_uid(self, uid: int) -> None:
+        self.user = None
         self.uid = uid
 
     def logged_in(self) -> bool:
-        if hasattr(self, 'uid') and self.uid is not None:
+        if self.uid is not None:
             return True
         else:
             return False
 
     def destroy(self) -> None:
         self.uid = None
+        self.user = None
         self.to_destroy = True
 
     def regenerate(self) -> None:
@@ -150,6 +156,8 @@ def open_session(self, app: Flask, request: Request) -> Optional[ServerSideSessi
                                           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)
 
     def save_session(  # type: ignore[override] # noqa
@@ -188,9 +196,14 @@ def save_session(  # type: ignore[override] # noqa
             self.redis.delete(self.key_prefix + session.sid)
             session.sid = self._generate_sid()
             session.token = self._get_signer(app).dumps(session.sid)  # type: ignore
-        if session.modified or session.new:
+        if session.new or session.to_regenerate:
             self.redis.setex(name=self.key_prefix + session.sid, value=val,
                              time=expires)
+        elif session.modified:
+            # To prevent race conditions where session is delete by an admin in the middle of a req
+            # accept to save the session object if and only if alrady exists using the xx flag
+            self.redis.set(name=self.key_prefix + session.sid, value=val,
+                           ex=expires, xx=True)
         if not session.is_api and (session.new or session.to_regenerate):
             response.headers.add('Vary', 'Cookie')
             response.set_cookie(app.session_cookie_name, session.token,
@@ -228,6 +241,25 @@ def _get_interface(self, app: Flask) -> SessionInterface:
         return session_interface
 
 
+def logout_user(uid: int, is_current: bool = True) -> None:
+    redis = Redis()
+    for key in (redis.keys(app.config['SESSION_KEY_PREFIX'] + "*") +
+                redis.keys("api_" + app.config['SESSION_KEY_PREFIX'] + "*")):
+        found = redis.get(key)
+        if found:
+            sess = session_json_serializer.loads(found.decode())
+            if 'uid' in sess and sess['uid'] == uid:
+                redis.delete(key)
+    if is_current:
+        session.pop('uid')
+
+
+def logout_all() -> None:
+    redis = Redis()
+    for key in (redis.keys(app.config['SESSION_KEY_PREFIX'] + "*") +
+                redis.keys("api_" + app.config['SESSION_KEY_PREFIX'] + "*")):
+        redis.delete(key)
+
 # Re-export flask.session, but with the correct type information for mypy.
 from flask import session  # noqa
 session = typing.cast(ServerSideSession, session)

securedrop/journalist_app/utils.py

@@ -5,12 +5,10 @@
 
 import flask
 import werkzeug
-from flask import (g, flash, current_app, abort, send_file, redirect, url_for,
+from flask import (flash, current_app, abort, send_file, redirect, url_for,
                    Markup, escape, session)
-from flask.sessions import session_json_serializer
 from flask_babel import gettext, ngettext
 from sqlalchemy.exc import IntegrityError
-from redis import Redis
 
 from db import db
 from encryption import EncryptionManager
@@ -529,22 +527,3 @@ def serve_file_with_etag(db_obj: Union[Reply, Submission]) -> flask.Response:
     response.direct_passthrough = False
     response.headers["Etag"] = db_obj.checksum
     return response
-
-
-def logout_user(uid: int) -> None:
-    redis = Redis()
-    for key in (redis.keys(current_app.config['SESSION_KEY_PREFIX'] + "*") +
-                redis.keys("api_" + current_app.config['SESSION_KEY_PREFIX'] + "*")):
-        found = redis.get(key)
-        if found:
-            sess = session_json_serializer.loads(found.decode())
-            if 'uid' in sess and sess['uid'] == uid:
-                redis.delete(key)
-    session.pop('uid')
-
-
-def logout_all() -> None:
-    redis = Redis()
-    for key in (redis.keys(current_app.config['SESSION_KEY_PREFIX'] + "*") +
-                redis.keys("api_" + current_app.config['SESSION_KEY_PREFIX'] + "*")):
-        redis.delete(key)

securedrop/tests/test_integration.py

@@ -8,6 +8,11 @@
 from base64 import b32encode
 from binascii import unhexlify
 from io import BytesIO
+import mock
+
+from bs4 import BeautifulSoup
+from flask import escape, session
+from pyotp import HOTP, TOTP
 
 import journalist_app as journalist_app_module
 import mock

securedrop/tests/test_journalist.py

@@ -68,7 +68,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')
+    assert ((username in resp.data.decode('utf-8')) == success)
 
 
 @pytest.mark.parametrize("otp_secret", ["", "GA", "GARBAGE", "JHCOGO7VCER3EJ4"])
@@ -705,7 +705,6 @@ def test_user_edits_password_success_response(config, journalist_app, test_journ
                 follow_redirects=True,
             )
 
-            assert page_language(resp.data) == language_tag(locale)
             msgids = [
                 "Password updated. Don't forget to save it in your KeePassX database. New password:"
             ]
@@ -744,7 +743,7 @@ def test_user_edits_password_expires_session(journalist_app, test_journo):
                 ins.assert_redirects(resp, url_for("main.login"))
 
             # verify the session was expired after the password was changed
-            assert "uid" not in session
+            assert (session.uid is None and session.user is None)
     finally:
         models.LOGIN_HARDENING = original_hardening
 

securedrop/tests/test_journalist_session.py

@@ -4,7 +4,7 @@
 from datetime import datetime, timedelta, timezone
 import re
 
-from flask import url_for
+from flask import url_for, Response
 from flask.sessions import session_json_serializer
 from redis import Redis
 from pyotp import TOTP
@@ -282,3 +282,24 @@ def test_session_bad_signature(journalist_app, test_journo):
                        headers=get_api_headers(token))
         assert resp.status_code == 200
         assert resp.json['uuid'] == test_journo['uuid']
+
+
+def test_session_race_condition(mocker, journalist_app, test_journo):
+    with journalist_app.test_request_context() as app:
+        session = journalist_app.session_interface.open_session(journalist_app, app.request)
+        assert session.sid is not None
+        session['uid'] = test_journo['id']
+        app.response = Response()
+        journalist_app.session_interface.save_session(journalist_app, session, app.response)
+        assert redis.get(journalist_app.config['SESSION_KEY_PREFIX'] + session.sid) is not None
+        app.request.cookies = {journalist_app.config['SESSION_COOKIE_NAME']: session.token}
+        session2 = journalist_app.session_interface.open_session(journalist_app, app.request)
+        assert session2.sid == session.sid
+        assert session2['uid'] == test_journo['id']
+        # Force entering the redis set xx case
+        session.modified = True
+        session.new = False
+        session.to_regenerate = False
+        redis.delete(journalist_app.config['SESSION_KEY_PREFIX'] + session.sid)
+        journalist_app.session_interface.save_session(journalist_app, session, app.response)
+        assert redis.get(journalist_app.config['SESSION_KEY_PREFIX'] + session.sid) is None

securedrop/tests/utils/__init__.py

@@ -1,6 +1,5 @@
 # -*- coding: utf-8 -*-
 
-from flask import g
 from pyotp import TOTP
 
 from . import asynchronous  # noqa