From e037c81338ccf80c55310e188e22012731c81918 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Wed, 15 May 2019 16:57:09 -0400 Subject: [PATCH 01/11] Only allow one session --- .../ab1167fc8260_add_user_last_session_id.py | 28 +++++++++++++++++++ atst/domain/users.py | 7 +++++ atst/models/user.py | 1 + atst/routes/__init__.py | 5 +++- 4 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/ab1167fc8260_add_user_last_session_id.py diff --git a/alembic/versions/ab1167fc8260_add_user_last_session_id.py b/alembic/versions/ab1167fc8260_add_user_last_session_id.py new file mode 100644 index 00000000..7f2d1e69 --- /dev/null +++ b/alembic/versions/ab1167fc8260_add_user_last_session_id.py @@ -0,0 +1,28 @@ +"""add_user_last_session_id + +Revision ID: ab1167fc8260 +Revises: 432c5287256d +Create Date: 2019-05-15 16:25:48.766451 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = 'ab1167fc8260' +down_revision = '432c5287256d' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('users', sa.Column('last_session_id', postgresql.UUID(as_uuid=True), server_default=sa.text('uuid_generate_v4()'), nullable=True)) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('users', 'last_session_id') + # ### end Alembic commands ### diff --git a/atst/domain/users.py b/atst/domain/users.py index baf5acb0..43dbdb86 100644 --- a/atst/domain/users.py +++ b/atst/domain/users.py @@ -89,6 +89,13 @@ class Users(object): db.session.add(user) db.session.commit() + + @classmethod + def update_last_session_id(cls, user, session_id): + user.last_session_id = session_id + db.session.add(user) + db.session.commit() + @classmethod def finalize(cls, user): user.provisional = False diff --git a/atst/models/user.py b/atst/models/user.py index c1693a37..0f093a94 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -58,6 +58,7 @@ class User( designation = Column(String) date_latest_training = Column(Date) last_login = Column(TIMESTAMP(timezone=True), nullable=True) + last_session_id = types.Id() provisional = Column(Boolean) diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index f923517f..7dbecddd 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -8,9 +8,9 @@ from flask import ( url_for, request, make_response, + current_app as app ) -from flask import current_app as app from jinja2.exceptions import TemplateNotFound import pendulum import os @@ -123,8 +123,11 @@ def redirect_after_login_url(): def current_user_setup(user): + session_id = session.sid + app.redis.delete("session:{}".format(user.last_session_id)) session["user_id"] = user.id session["last_login"] = user.last_login + Users.update_last_session_id(user, session_id) Users.update_last_login(user) From 5dcbee333a6c65fe3b31ab2619062f7b76c9d220 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 20 May 2019 09:58:39 -0400 Subject: [PATCH 02/11] Make session limiter configurable --- atst/app.py | 11 ++++++++++- atst/domain/users.py | 1 - atst/routes/__init__.py | 6 ++---- atst/utils/session_limiter.py | 19 +++++++++++++++++++ config/base.ini | 1 + config/prod.ini | 1 + 6 files changed, 33 insertions(+), 6 deletions(-) create mode 100644 atst/utils/session_limiter.py diff --git a/atst/app.py b/atst/app.py index c729c65a..ccecd534 100644 --- a/atst/app.py +++ b/atst/app.py @@ -2,7 +2,7 @@ import os import re import pathlib from configparser import ConfigParser -from flask import Flask, request, g +from flask import Flask, request, g, session from flask_session import Session import redis from unipath import Path @@ -30,6 +30,7 @@ from atst.utils.form_cache import FormCache from atst.utils.json import CustomJSONEncoder from atst.queue import queue from atst.utils.notification_sender import NotificationSender +from atst.utils.session_limiter import SessionLimiter from logging.config import dictConfig from atst.utils.logging import JsonFormatter, RequestContextFilter @@ -70,6 +71,7 @@ def make_app(config): db.init_app(app) csrf.init_app(app) Session(app) + make_session_limiter(app, session, config) assets_environment.init_app(app) make_error_pages(app) @@ -162,6 +164,9 @@ def map_config(config): "DISABLE_CRL_CHECK": config.getboolean("default", "DISABLE_CRL_CHECK"), "CRL_FAIL_OPEN": config.getboolean("default", "CRL_FAIL_OPEN"), "LOG_JSON": config.getboolean("default", "LOG_JSON"), + "LIMIT_CONCURRENT_SESSIONS": config.getboolean( + "default", "LIMIT_CONCURRENT_SESSIONS" + ), } @@ -253,6 +258,10 @@ def make_notification_sender(app): app.notification_sender = NotificationSender(queue) +def make_session_limiter(app, session, config): + app.session_limiter = SessionLimiter(config, session, app.redis) + + def apply_json_logger(): dictConfig( { diff --git a/atst/domain/users.py b/atst/domain/users.py index 43dbdb86..daf69c68 100644 --- a/atst/domain/users.py +++ b/atst/domain/users.py @@ -89,7 +89,6 @@ class Users(object): db.session.add(user) db.session.commit() - @classmethod def update_last_session_id(cls, user, session_id): user.last_session_id = session_id diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index 7dbecddd..94acb897 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -8,7 +8,7 @@ from flask import ( url_for, request, make_response, - current_app as app + current_app as app, ) from jinja2.exceptions import TemplateNotFound @@ -123,11 +123,9 @@ def redirect_after_login_url(): def current_user_setup(user): - session_id = session.sid - app.redis.delete("session:{}".format(user.last_session_id)) session["user_id"] = user.id session["last_login"] = user.last_login - Users.update_last_session_id(user, session_id) + app.session_limiter.on_login(user) Users.update_last_login(user) diff --git a/atst/utils/session_limiter.py b/atst/utils/session_limiter.py new file mode 100644 index 00000000..cae43c5d --- /dev/null +++ b/atst/utils/session_limiter.py @@ -0,0 +1,19 @@ +from atst.domain.users import Users + + +class SessionLimiter(object): + def __init__(self, config, session, redis): + self.limit_logins = config["LIMIT_CONCURRENT_SESSIONS"] + self.session = session + self.redis = redis + + def on_login(self, user): + if not self.limit_logins: + return + + session_id = self.session.sid + self._delete_session(user.last_session_id) + Users.update_last_session_id(user, session_id) + + def _delete_session(self, session_id): + self.redis.delete("session:{}".format(session_id)) diff --git a/config/base.ini b/config/base.ini index 36f1e0a7..ce98fbf2 100644 --- a/config/base.ini +++ b/config/base.ini @@ -34,3 +34,4 @@ STORAGE_SECRET='' STORAGE_PROVIDER=LOCAL STORAGE_CRL_ARCHIVE_NAME = dod_crls.tar.bz WTF_CSRF_ENABLED = true +LIMIT_CONCURRENT_SESSIONS = false diff --git a/config/prod.ini b/config/prod.ini index bbbf8f8b..ebc1dc40 100644 --- a/config/prod.ini +++ b/config/prod.ini @@ -1,3 +1,4 @@ [default] SESSION_COOKIE_SECURE=True SESSION_COOKIE_DOMAIN=atat.codes +LIMIT_CONCURRENT_SESSIONS=True From 9291ea497cc4e61f5641ca6d248e42b7bc347da3 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 20 May 2019 10:39:38 -0400 Subject: [PATCH 03/11] Fix migration path --- alembic/versions/ab1167fc8260_add_user_last_session_id.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alembic/versions/ab1167fc8260_add_user_last_session_id.py b/alembic/versions/ab1167fc8260_add_user_last_session_id.py index 7f2d1e69..2bbcac8b 100644 --- a/alembic/versions/ab1167fc8260_add_user_last_session_id.py +++ b/alembic/versions/ab1167fc8260_add_user_last_session_id.py @@ -1,7 +1,7 @@ """add_user_last_session_id Revision ID: ab1167fc8260 -Revises: 432c5287256d +Revises: 404bb5bb3a0e Create Date: 2019-05-15 16:25:48.766451 """ @@ -11,7 +11,7 @@ from sqlalchemy.dialects import postgresql # revision identifiers, used by Alembic. revision = 'ab1167fc8260' -down_revision = '432c5287256d' +down_revision = '404bb5bb3a0e' branch_labels = None depends_on = None From 888d41f484cd8fe7ece2470397f350e7a19da295 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 20 May 2019 10:46:35 -0400 Subject: [PATCH 04/11] Formatting --- atst/app.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/atst/app.py b/atst/app.py index ccecd534..351051b1 100644 --- a/atst/app.py +++ b/atst/app.py @@ -254,6 +254,10 @@ def make_mailer(app): app.mailer = mailer.Mailer(mailer_connection, sender) +def make_session_limiter(app, session, config): + app.session_limiter = SessionLimiter(config, session, app.redis) + + def make_notification_sender(app): app.notification_sender = NotificationSender(queue) From 7aa238c59f6a3a2543327a7e0bcb33f6900871e7 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 20 May 2019 15:47:46 -0400 Subject: [PATCH 05/11] Remove last_session_id server_default --- alembic/versions/ab1167fc8260_add_user_last_session_id.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/alembic/versions/ab1167fc8260_add_user_last_session_id.py b/alembic/versions/ab1167fc8260_add_user_last_session_id.py index 2bbcac8b..99ad8b4a 100644 --- a/alembic/versions/ab1167fc8260_add_user_last_session_id.py +++ b/alembic/versions/ab1167fc8260_add_user_last_session_id.py @@ -18,7 +18,7 @@ depends_on = None def upgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.add_column('users', sa.Column('last_session_id', postgresql.UUID(as_uuid=True), server_default=sa.text('uuid_generate_v4()'), nullable=True)) + op.add_column('users', sa.Column('last_session_id', postgresql.UUID(as_uuid=True), nullable=True)) # ### end Alembic commands ### From 5fe895e109522314a9998ddce22da35a81c8e9f7 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Wed, 22 May 2019 15:43:56 -0400 Subject: [PATCH 06/11] Id() column type is for primary keys --- atst/models/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atst/models/user.py b/atst/models/user.py index 0f093a94..61ba0c79 100644 --- a/atst/models/user.py +++ b/atst/models/user.py @@ -58,7 +58,7 @@ class User( designation = Column(String) date_latest_training = Column(Date) last_login = Column(TIMESTAMP(timezone=True), nullable=True) - last_session_id = types.Id() + last_session_id = Column(UUID(as_uuid=True), nullable=True) provisional = Column(Boolean) From 8fae9d0956b73a88484760205cb68f3fc67a4bb8 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 23 May 2019 16:26:37 -0400 Subject: [PATCH 07/11] Tests for session_limiter --- tests/utils/test_session_limiter.py | 51 +++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 tests/utils/test_session_limiter.py diff --git a/tests/utils/test_session_limiter.py b/tests/utils/test_session_limiter.py new file mode 100644 index 00000000..50d0ba54 --- /dev/null +++ b/tests/utils/test_session_limiter.py @@ -0,0 +1,51 @@ +import pytest +from redis import Redis +from unittest.mock import Mock +from uuid import uuid4 + +from atst.utils.session_limiter import SessionLimiter +from tests.factories import UserFactory +from atst.models.user import User + + +@pytest.fixture +def mock_redis(): + return Mock(spec=Redis) + + +@pytest.fixture +def mock_session(): + mock = Mock(spec=["sid"]) + mock.sid = uuid4() + return mock + + +def test_session_limiter_deletes_users_old_session(mock_redis, mock_session): + last_session_id = uuid4() + current_session_id = uuid4() + + mock_session.sid = current_session_id + + session_limiter = SessionLimiter( + {"LIMIT_CONCURRENT_SESSIONS": True}, mock_session, mock_redis + ) + user = UserFactory.create(last_session_id=last_session_id) + session_limiter.on_login(user) + + mock_redis.delete.assert_called_with("session:{}".format(last_session_id)) + + +def test_session_limiter_updates_users_last_sesion_id(mock_redis, mock_session, db): + last_session_id = uuid4() + current_session_id = uuid4() + + mock_session.sid = current_session_id + + session_limiter = SessionLimiter( + {"LIMIT_CONCURRENT_SESSIONS": True}, mock_session, mock_redis + ) + user = UserFactory.create(last_session_id=last_session_id) + session_limiter.on_login(user) + + user = db.query(User).get(user.id) + assert user.last_session_id == last_session_id From d2392cb3d0e6f11d36c4a45fd6deccb6d225c933 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 28 May 2019 10:29:02 -0400 Subject: [PATCH 08/11] Fix test --- tests/utils/test_session_limiter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/utils/test_session_limiter.py b/tests/utils/test_session_limiter.py index 50d0ba54..4595d4e3 100644 --- a/tests/utils/test_session_limiter.py +++ b/tests/utils/test_session_limiter.py @@ -47,5 +47,5 @@ def test_session_limiter_updates_users_last_sesion_id(mock_redis, mock_session, user = UserFactory.create(last_session_id=last_session_id) session_limiter.on_login(user) - user = db.query(User).get(user.id) - assert user.last_session_id == last_session_id + user = db.session.query(User).get(user.id) + assert user.last_session_id == current_session_id From a56d9c9ca8e230d43d6b819b520baf4e1f5a2a79 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Tue, 28 May 2019 10:50:38 -0400 Subject: [PATCH 09/11] Fix migration path --- alembic/versions/ab1167fc8260_add_user_last_session_id.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alembic/versions/ab1167fc8260_add_user_last_session_id.py b/alembic/versions/ab1167fc8260_add_user_last_session_id.py index 99ad8b4a..b1039571 100644 --- a/alembic/versions/ab1167fc8260_add_user_last_session_id.py +++ b/alembic/versions/ab1167fc8260_add_user_last_session_id.py @@ -1,7 +1,7 @@ """add_user_last_session_id Revision ID: ab1167fc8260 -Revises: 404bb5bb3a0e +Revises: c5deba1826be Create Date: 2019-05-15 16:25:48.766451 """ @@ -11,7 +11,7 @@ from sqlalchemy.dialects import postgresql # revision identifiers, used by Alembic. revision = 'ab1167fc8260' -down_revision = '404bb5bb3a0e' +down_revision = 'c5deba1826be' branch_labels = None depends_on = None From d82fd46a3c0d4ff154080a25d416f53de960ca06 Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 28 May 2019 13:38:26 -0400 Subject: [PATCH 10/11] Mock out `g` in logging utils test. This gives us better test isolation. Previously, we were manually setting `g.current_user` with a factory instance and not cleaning it up properly, which could break later tests. --- tests/utils/test_logging.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/utils/test_logging.py b/tests/utils/test_logging.py index cb8198d3..fd934d07 100644 --- a/tests/utils/test_logging.py +++ b/tests/utils/test_logging.py @@ -2,6 +2,7 @@ from io import StringIO import json import logging from uuid import uuid4 +from unittest.mock import Mock import pytest @@ -62,13 +63,16 @@ def test_json_formatter_for_exceptions(logger, log_stream_content): assert log.get("details") -def test_request_context_filter(logger, log_stream_content, request_ctx): - user = UserFactory.create() - uuid = str(uuid4()) +def test_request_context_filter(logger, log_stream_content, request_ctx, monkeypatch): + request_uuid = str(uuid4()) + user_uuid = str(uuid4()) - request_ctx.g.current_user = user - request_ctx.request.environ["HTTP_X_REQUEST_ID"] = uuid + user = Mock(spec=["id"]) + user.id = user_uuid + + monkeypatch.setattr("atst.utils.logging.g", Mock(current_user=user)) + request_ctx.request.environ["HTTP_X_REQUEST_ID"] = request_uuid logger.info("this user is doing something") log = json.loads(log_stream_content()) - assert log["user_id"] == str(user.id) - assert log["request_id"] == uuid + assert log["user_id"] == str(user_uuid) + assert log["request_id"] == request_uuid From 9b3775291e9f404bc6d1d13ceb61b2e5fce16f10 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Wed, 29 May 2019 16:05:52 -0400 Subject: [PATCH 11/11] Remove redundant make_session_limiter --- atst/app.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/atst/app.py b/atst/app.py index 351051b1..ccecd534 100644 --- a/atst/app.py +++ b/atst/app.py @@ -254,10 +254,6 @@ def make_mailer(app): app.mailer = mailer.Mailer(mailer_connection, sender) -def make_session_limiter(app, session, config): - app.session_limiter = SessionLimiter(config, session, app.redis) - - def make_notification_sender(app): app.notification_sender = NotificationSender(queue)