Merge pull request #1337 from dod-ccpo/apply-session-limit

Forcibly destroy existing session on logout.

.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

deploy/azure/atst-envvars-configmap.yml

@@ -15,6 +15,7 @@ data:
   CSP: azure
   DEBUG: "0"
   FLASK_ENV: master
+  LIMIT_CONCURRENT_SESSIONS: "true"
   LOG_JSON: "true"
   MAIL_PORT: "587"
   MAIL_SENDER: postmaster@atat.code.mil

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]