Forcibly destroy existing session on logout.

To comply with security guidelines, we need to destroy the session when
a user logs out. This means that the session's key in the Redis cache
needs to be deleted. Flask expects to _always_ have a session object. If
the current session object does not exist in the Redis cache, Flask will
reserialize and store it at the end of the request. In order for
session deletion to work, we need to delete the key for the existing
session and then replace the session object with a new, empty one.

This also updates the SessionLimiter class so that the session prefix is
configurable.

.secrets.baseline

@@ -3,7 +3,7 @@
     "files": "^.secrets.baseline$|^.*pgsslrootcert.yml$",
     "lines": null
   },
-  "generated_at": "2020-01-16T16:10:27Z",
+  "generated_at": "2020-01-19T20:21:20Z",
   "plugins_used": [
     {
       "base64_limit": 4.5,
@@ -32,13 +32,6 @@
         "is_verified": false,
         "line_number": 156,
         "type": "Secret Keyword"
-      },
-      {
-        "hashed_secret": "81b127e2222d9bfc4609053faec85300f7525463",
-        "is_secret": false,
-        "is_verified": false,
-        "line_number": 290,
-        "type": "Secret Keyword"
       }
     ],
     "alembic.ini": [
@@ -89,7 +82,7 @@
         "hashed_secret": "afc848c316af1a89d49826c5ae9d00ed769415f3",
         "is_secret": false,
         "is_verified": false,
-        "line_number": 30,
+        "line_number": 31,
         "type": "Secret Keyword"
       }
     ],

README.md

@@ -255,6 +255,7 @@ To generate coverage reports for the Javascript tests:
 - `SERVER_NAME`: Hostname for ATAT. Only needs to be specified in contexts where the hostname cannot be inferred from the request, such as Celery workers. https://flask.palletsprojects.com/en/1.1.x/config/#SERVER_NAME
 - `SESSION_COOKIE_NAME`: String value specifying the name to use for the session cookie. https://flask.palletsprojects.com/en/1.1.x/config/#SESSION_COOKIE_NAME
 - `SESSION_COOKIE_DOMAIN`: String value specifying the name to use for the session cookie. This should be set to the root domain so that it is valid for both the main site and the authentication subdomain. https://flask.palletsprojects.com/en/1.1.x/config/#SESSION_COOKIE_DOMAIN
+- `SESSION_KEY_PREFIX`: A prefix that is added before all session keys: https://pythonhosted.org/Flask-Session/#configuration
 - `SESSION_TYPE`: String value specifying the cookie storage backend. https://pythonhosted.org/Flask-Session/
 - `SESSION_USE_SIGNER`: Boolean value specifying if the cookie sid should be signed.
 - `SQLALCHEMY_ECHO`: Boolean value specifying if SQLAlchemy should log queries to stdout.

atst/domain/auth.py

@@ -1,4 +1,13 @@
-from flask import g, redirect, url_for, session, request, current_app as app
+from flask import (
+    g,
+    redirect,
+    url_for,
+    session,
+    request,
+    current_app as app,
+    _request_ctx_stack as request_ctx_stack,
+)
+from werkzeug.datastructures import ImmutableTypeConversionDict
 
 from atst.domain.users import Users
 
@@ -56,12 +65,26 @@ def get_last_login():
     return session.get("user_id") and session.get("last_login")
 
 
+def _nullify_session(session):
+    session_key = f"{app.config.get('SESSION_KEY_PREFIX')}{session.sid}"
+    app.redis.delete(session_key)
+    request.cookies = ImmutableTypeConversionDict()
+    request_ctx_stack.top.session = app.session_interface.open_session(app, request)
+
+
+def _current_dod_id():
+    return g.current_user.dod_id if session.get("user_id") else None
+
+
 def logout():
-    if session.get("user_id"):  # pragma: no branch
-        dod_id = g.current_user.dod_id
-        del session["user_id"]
-        del session["last_login"]
+    dod_id = _current_dod_id()
+
+    _nullify_session(session)
+
+    if dod_id:
         app.logger.info(f"user with EDIPI {dod_id} has logged out")
+    else:
+        app.logger.info("unauthenticated user has logged out")
 
 
 def _unprotected_route(request):

atst/utils/session_limiter.py

@@ -4,6 +4,7 @@ from atst.domain.users import Users
 class SessionLimiter(object):
     def __init__(self, config, session, redis):
         self.limit_logins = config["LIMIT_CONCURRENT_SESSIONS"]
+        self.session_prefix = config.get("SESSION_KEY_PREFIX", "session:")
         self.session = session
         self.redis = redis
 
@@ -16,4 +17,4 @@ class SessionLimiter(object):
         Users.update_last_session_id(user, session_id)
 
     def _delete_session(self, session_id):
-        self.redis.delete("session:{}".format(session_id))
+        self.redis.delete(f"{self.session_prefix}{session_id}")

config/base.ini

@@ -42,6 +42,7 @@ SECRET_KEY = change_me_into_something_secret
 SERVER_NAME
 SESSION_COOKIE_NAME=atat
 SESSION_COOKIE_DOMAIN
+SESSION_KEY_PREFIX=session:
 SESSION_TYPE = redis
 SESSION_USE_SIGNER = True
 SQLALCHEMY_ECHO = False

tests/domain/test_auth.py

@@ -0,0 +1,29 @@
+from flask import make_response, session
+
+from atst.domain.auth import logout
+
+
+def _write_session(app):
+    response = make_response("")
+    app.session_interface.save_session(app, session, response)
+    return session
+
+
+def test_logout_destroys_session(app):
+    session = _write_session(app)
+    key = app.config.get("SESSION_KEY_PREFIX") + session.sid
+    assert app.redis.get(key)
+    logout()
+    assert app.redis.get(key) is None
+
+
+def test_logout_logs_dod_id_for_current_user(monkeypatch, mock_logger):
+    dod_id = "3434343434"
+    monkeypatch.setattr("atst.domain.auth._current_dod_id", lambda: dod_id)
+    logout()
+    assert dod_id in mock_logger.messages[-1]
+
+
+def test_logout_logs_message_for_unathenticated_user(mock_logger):
+    logout()
+    assert "unauthenticated" in mock_logger.messages[-1]