From bb7a1bbac48877dc443620033cd422eb77e425ee Mon Sep 17 00:00:00 2001 From: LTrasca Date: Wed, 27 Nov 2024 12:39:39 +0000 Subject: [PATCH 1/7] added middleware logic --- app/main/middlewares/__init__.py | 0 app/main/middlewares/log_page_view.py | 20 ++++++++++++++++++++ app/main/routes.py | 14 ++++++++++++++ 3 files changed, 34 insertions(+) create mode 100644 app/main/middlewares/__init__.py create mode 100644 app/main/middlewares/log_page_view.py diff --git a/app/main/middlewares/__init__.py b/app/main/middlewares/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/app/main/middlewares/log_page_view.py b/app/main/middlewares/log_page_view.py new file mode 100644 index 00000000..e3a710a2 --- /dev/null +++ b/app/main/middlewares/log_page_view.py @@ -0,0 +1,20 @@ +import json +from functools import wraps + +from flask import current_app, request, session + + +def log_page_view(route_function): + @wraps(route_function) + def wrapper(*args, **kwargs): + user_id = session.get("user_id", "anonymous") + log_data = { + "event": "page_view", + "user_id": user_id, + "route": request.path, + "method": request.method, + } + current_app.audit_logger.info(json.dumps(log_data)) + return route_function(*args, **kwargs) + + return wrapper diff --git a/app/main/routes.py b/app/main/routes.py index b33b33d4..ea8c7879 100644 --- a/app/main/routes.py +++ b/app/main/routes.py @@ -33,6 +33,7 @@ from app.main.flask_config_helpers import ( get_keycloak_instance_from_flask_config, ) +from app.main.middlewares.log_page_view import log_page_view from app.main.util.date_filters_validator import validate_date_filters from app.main.util.filter_sort_builder import ( build_browse_consignment_filters, @@ -76,6 +77,7 @@ def index(): @bp.route("/sign-out", methods=["GET"]) @access_token_sign_in_required +@log_page_view def sign_out(): keycloak_openid = get_keycloak_instance_from_flask_config() keycloak_openid.logout(session["refresh_token"]) @@ -85,6 +87,7 @@ def sign_out(): @bp.route("/sign-in", methods=["GET"]) +@log_page_view def sign_in(): keycloak_openid = get_keycloak_instance_from_flask_config() auth_url = keycloak_openid.auth_url( @@ -96,6 +99,7 @@ def sign_in(): @bp.route("/callback", methods=["GET"]) +@log_page_view def callback(): keycloak_openid = get_keycloak_instance_from_flask_config() code = request.args.get("code") @@ -126,6 +130,7 @@ def accessibility(): @bp.route("/browse", methods=["GET"]) @access_token_sign_in_required +@log_page_view def browse(): form = SearchForm() page = int(request.args.get("page", 1)) @@ -205,6 +210,7 @@ def browse(): @bp.route("/browse/transferring_body/", methods=["GET"]) @access_token_sign_in_required +@log_page_view def browse_transferring_body(_id: uuid.UUID): """ Render the browse transferring body view page. @@ -288,6 +294,7 @@ def browse_transferring_body(_id: uuid.UUID): @bp.route("/browse/series/", methods=["GET"]) @access_token_sign_in_required +@log_page_view def browse_series(_id: uuid.UUID): """ Render the browse series view page. @@ -376,6 +383,7 @@ def browse_series(_id: uuid.UUID): @bp.route("/browse/consignment/", methods=["GET"]) @access_token_sign_in_required +@log_page_view def browse_consignment(_id: uuid.UUID): """ Render the browse consignment view page. @@ -464,6 +472,7 @@ def browse_consignment(_id: uuid.UUID): @bp.route("/search", methods=["GET"]) @access_token_sign_in_required +@log_page_view def search(): form_data = request.form.to_dict() args_data = request.args.to_dict() @@ -495,6 +504,7 @@ def search(): @bp.route("/search_results_summary", methods=["GET"]) @access_token_sign_in_required +@log_page_view def search_results_summary(): ayr_user = AYRUser(session.get("user_groups")) if ayr_user.is_standard_user: @@ -550,6 +560,7 @@ def search_results_summary(): @bp.route("/search/transferring_body/", methods=["GET"]) @access_token_sign_in_required +@log_page_view def search_transferring_body(_id: uuid.UUID): body = db.session.get(Body, _id) validate_body_user_groups_or_404(body.Name) @@ -646,6 +657,7 @@ def search_transferring_body(_id: uuid.UUID): @bp.route("/record/", methods=["GET"]) @access_token_sign_in_required +@log_page_view def record(record_id: uuid.UUID): """ Render the record details page. @@ -705,6 +717,7 @@ def record(record_id: uuid.UUID): @bp.route("/download/") @access_token_sign_in_required +@log_page_view def download_record(record_id: uuid.UUID): file = db.session.get(File, record_id) render = request.args.get("render", False) @@ -798,6 +811,7 @@ def http_exception(error): @bp.route("/record//manifest") @access_token_sign_in_required +@log_page_view def generate_manifest(record_id: uuid.UUID): file = db.session.get(File, record_id) From 59bfc374353a279d16ef0cf86a2324bc86da7e6e Mon Sep 17 00:00:00 2001 From: LTrasca Date: Wed, 27 Nov 2024 14:14:53 +0000 Subject: [PATCH 2/7] added unit tests for middleware --- app/tests/test_middlewares.py | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 app/tests/test_middlewares.py diff --git a/app/tests/test_middlewares.py b/app/tests/test_middlewares.py new file mode 100644 index 00000000..29f98696 --- /dev/null +++ b/app/tests/test_middlewares.py @@ -0,0 +1,36 @@ +from unittest.mock import MagicMock, patch + +import pytest + +from app.main.middlewares.log_page_view import log_page_view + + +@pytest.mark.parametrize( + "user_id, method, expected_user_id", + [ + ("12345", "GET", "12345"), + (None, "POST", "anonymous"), + ], +) +@patch("app.main.middlewares.log_page_view.session") +@patch("app.main.middlewares.log_page_view.current_app") +def test_log_page_view( + mock_current_app, mock_session, app, user_id, method, expected_user_id +): + """ + Test that the log page view middleware calls the current_app.audit_logger with correct values + """ + mock_session.get.return_value = user_id + mock_current_app.audit_logger.info = MagicMock() + view_name = "/log_page_view" + + @app.route(view_name) + @log_page_view + def protected_view(): + return "Foobar" + + with app.test_client() as client: + response = client.get(view_name) + + assert response == "Foobar" + mock_current_app.audit_logger.info.assert_called_once_with({}) From 6b719d982b9ff672517b4e39fd0a47e945c46723 Mon Sep 17 00:00:00 2001 From: LTrasca Date: Wed, 27 Nov 2024 14:46:41 +0000 Subject: [PATCH 3/7] fixed tests --- app/tests/test_middlewares.py | 90 +++++++++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 21 deletions(-) diff --git a/app/tests/test_middlewares.py b/app/tests/test_middlewares.py index 29f98696..5e4ecdde 100644 --- a/app/tests/test_middlewares.py +++ b/app/tests/test_middlewares.py @@ -1,36 +1,84 @@ -from unittest.mock import MagicMock, patch +import json +from unittest.mock import MagicMock import pytest +from flask import current_app +from flask.testing import FlaskClient from app.main.middlewares.log_page_view import log_page_view @pytest.mark.parametrize( - "user_id, method, expected_user_id", + "route_path, method, user_id_in_session, route_function, expected_response, expected_log_data", [ - ("12345", "GET", "12345"), - (None, "POST", "anonymous"), + ( + "/test_route", + "GET", + "test_user", + lambda: "Test Response", + b"Test Response", + { + "event": "page_view", + "user_id": "test_user", + "route": "/test_route", + "method": "GET", + }, + ), + ( + "/anonymous_route", + "GET", + None, + lambda: "Anonymous Response", + b"Anonymous Response", + { + "event": "page_view", + "user_id": "anonymous", + "route": "/anonymous_route", + "method": "GET", + }, + ), + ( + "/post_route", + "POST", + "test_user", + lambda: "Post Response", + b"Post Response", + { + "event": "page_view", + "user_id": "test_user", + "route": "/post_route", + "method": "POST", + }, + ), ], ) -@patch("app.main.middlewares.log_page_view.session") -@patch("app.main.middlewares.log_page_view.current_app") def test_log_page_view( - mock_current_app, mock_session, app, user_id, method, expected_user_id + app, + client: FlaskClient, + route_path, + method, + user_id_in_session, + route_function, + expected_response, + expected_log_data, ): - """ - Test that the log page view middleware calls the current_app.audit_logger with correct values - """ - mock_session.get.return_value = user_id - mock_current_app.audit_logger.info = MagicMock() - view_name = "/log_page_view" - - @app.route(view_name) + mock_logger = MagicMock() + + @app.route(route_path, methods=[method]) @log_page_view - def protected_view(): - return "Foobar" + def dynamic_route(): + return route_function() + + with app.app_context(): + current_app.audit_logger = mock_logger + + if user_id_in_session is not None: + with client.session_transaction() as session: + session["user_id"] = user_id_in_session + + response = client.open(route_path, method=method) - with app.test_client() as client: - response = client.get(view_name) + assert response.status_code == 200 + assert response.data == expected_response - assert response == "Foobar" - mock_current_app.audit_logger.info.assert_called_once_with({}) + mock_logger.info.assert_called_once_with(json.dumps(expected_log_data)) From 336da12349abf414daddadd4dad450c7c8fce6b9 Mon Sep 17 00:00:00 2001 From: LTrasca Date: Fri, 29 Nov 2024 12:30:07 +0000 Subject: [PATCH 4/7] removed if statement from test_log_page_view --- app/tests/test_middlewares.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/app/tests/test_middlewares.py b/app/tests/test_middlewares.py index 5e4ecdde..e6fdc552 100644 --- a/app/tests/test_middlewares.py +++ b/app/tests/test_middlewares.py @@ -9,12 +9,12 @@ @pytest.mark.parametrize( - "route_path, method, user_id_in_session, route_function, expected_response, expected_log_data", + "route_path, method, session_object, route_function, expected_response, expected_log_data", [ ( "/test_route", "GET", - "test_user", + {"user_id": "test_user"}, lambda: "Test Response", b"Test Response", { @@ -27,7 +27,7 @@ ( "/anonymous_route", "GET", - None, + {}, lambda: "Anonymous Response", b"Anonymous Response", { @@ -40,7 +40,7 @@ ( "/post_route", "POST", - "test_user", + {"user_id": "test_user"}, lambda: "Post Response", b"Post Response", { @@ -57,7 +57,7 @@ def test_log_page_view( client: FlaskClient, route_path, method, - user_id_in_session, + session_object, route_function, expected_response, expected_log_data, @@ -72,9 +72,8 @@ def dynamic_route(): with app.app_context(): current_app.audit_logger = mock_logger - if user_id_in_session is not None: - with client.session_transaction() as session: - session["user_id"] = user_id_in_session + with client.session_transaction() as session: + session.update(session_object) response = client.open(route_path, method=method) From 2e079f2c43338be74d3ca18383d189e48f5f3477 Mon Sep 17 00:00:00 2001 From: LTrasca Date: Fri, 29 Nov 2024 14:16:27 +0000 Subject: [PATCH 5/7] fixed unit tests and removed logger mock in favor of caplog --- app/tests/test_download.py | 16 +++++++------- app/tests/test_middlewares.py | 39 +++++++++-------------------------- 2 files changed, 18 insertions(+), 37 deletions(-) diff --git a/app/tests/test_download.py b/app/tests/test_download.py index f37d770a..07d286f9 100644 --- a/app/tests/test_download.py +++ b/app/tests/test_download.py @@ -76,8 +76,8 @@ def test_download_record_standard_user_with_citable_reference_with_file_extensio msg = json.dumps({"user_id": user_id, "file": key}) - assert caplog.records[0].levelname == "INFO" - assert caplog.records[0].message == msg + assert caplog.records[1].levelname == "INFO" + assert caplog.records[1].message == msg @mock_aws def test_download_record_standard_user_with_citable_reference_without_file_extension( @@ -119,8 +119,8 @@ def test_download_record_standard_user_with_citable_reference_without_file_exten msg = json.dumps({"user_id": user_id, "file": key}) - assert caplog.records[0].levelname == "INFO" - assert caplog.records[0].message == msg + assert caplog.records[1].levelname == "INFO" + assert caplog.records[1].message == msg @mock_aws def test_download_record_standard_user_without_citable_reference( @@ -162,8 +162,8 @@ def test_download_record_standard_user_without_citable_reference( msg = json.dumps({"user_id": user_id, "file": key}) - assert caplog.records[0].levelname == "INFO" - assert caplog.records[0].message == msg + assert caplog.records[1].levelname == "INFO" + assert caplog.records[1].message == msg @mock_aws def test_download_record_standard_user_get_file_errors( @@ -226,8 +226,8 @@ def test_download_record_standard_user_read_file_error( msg = "Error reading S3 file content: Read error" assert response.status_code == 500 - assert caplog.records[0].levelname == "ERROR" - assert caplog.records[0].message == msg + assert caplog.records[1].levelname == "ERROR" + assert caplog.records[1].message == msg @mock_aws def test_raises_404_for_standard_user_without_access_to_files_transferring_body( diff --git a/app/tests/test_middlewares.py b/app/tests/test_middlewares.py index e6fdc552..ea1f65f5 100644 --- a/app/tests/test_middlewares.py +++ b/app/tests/test_middlewares.py @@ -1,15 +1,12 @@ -import json -from unittest.mock import MagicMock - import pytest -from flask import current_app from flask.testing import FlaskClient +from moto import mock_aws from app.main.middlewares.log_page_view import log_page_view @pytest.mark.parametrize( - "route_path, method, session_object, route_function, expected_response, expected_log_data", + "route_path, method, session_object, route_function, expected_response, expected_log", [ ( "/test_route", @@ -17,12 +14,7 @@ {"user_id": "test_user"}, lambda: "Test Response", b"Test Response", - { - "event": "page_view", - "user_id": "test_user", - "route": "/test_route", - "method": "GET", - }, + '{"event": "page_view", "user_id": "test_user", "route": "/test_route", "method": "GET"}', ), ( "/anonymous_route", @@ -30,12 +22,7 @@ {}, lambda: "Anonymous Response", b"Anonymous Response", - { - "event": "page_view", - "user_id": "anonymous", - "route": "/anonymous_route", - "method": "GET", - }, + '{"event": "page_view", "user_id": "anonymous", "route": "/anonymous_route", "method": "GET"}', ), ( "/post_route", @@ -43,15 +30,11 @@ {"user_id": "test_user"}, lambda: "Post Response", b"Post Response", - { - "event": "page_view", - "user_id": "test_user", - "route": "/post_route", - "method": "POST", - }, + '{"event": "page_view", "user_id": "test_user", "route": "/post_route", "method": "POST"}', ), ], ) +@mock_aws def test_log_page_view( app, client: FlaskClient, @@ -60,18 +43,15 @@ def test_log_page_view( session_object, route_function, expected_response, - expected_log_data, + expected_log, + caplog, ): - mock_logger = MagicMock() @app.route(route_path, methods=[method]) @log_page_view def dynamic_route(): return route_function() - with app.app_context(): - current_app.audit_logger = mock_logger - with client.session_transaction() as session: session.update(session_object) @@ -80,4 +60,5 @@ def dynamic_route(): assert response.status_code == 200 assert response.data == expected_response - mock_logger.info.assert_called_once_with(json.dumps(expected_log_data)) + assert caplog.records[0].levelname == "INFO" + assert caplog.records[0].message == expected_log From e8242a3b639807133638a376527b47f2de2ad062 Mon Sep 17 00:00:00 2001 From: LTrasca Date: Fri, 29 Nov 2024 14:50:20 +0000 Subject: [PATCH 6/7] removed access log from download route and adjusted unit tests --- app/main/middlewares/log_page_view.py | 2 +- app/main/routes.py | 7 +++--- app/tests/test_download.py | 35 ++------------------------- app/tests/test_middlewares.py | 8 +++--- 4 files changed, 11 insertions(+), 41 deletions(-) diff --git a/app/main/middlewares/log_page_view.py b/app/main/middlewares/log_page_view.py index e3a710a2..91224536 100644 --- a/app/main/middlewares/log_page_view.py +++ b/app/main/middlewares/log_page_view.py @@ -9,7 +9,7 @@ def log_page_view(route_function): def wrapper(*args, **kwargs): user_id = session.get("user_id", "anonymous") log_data = { - "event": "page_view", + "event": "api_request", "user_id": user_id, "route": request.path, "method": request.method, diff --git a/app/main/routes.py b/app/main/routes.py index ea8c7879..016c2deb 100644 --- a/app/main/routes.py +++ b/app/main/routes.py @@ -1,5 +1,4 @@ import io -import json import uuid import boto3 @@ -773,9 +772,9 @@ def download_record(record_id: uuid.UUID): as_attachment=True, download_name=download_filename, ) - current_app.app_logger.info( - json.dumps({"user_id": session["user_id"], "file": key}) - ) + # current_app.app_logger.info( + # json.dumps({"user_id": session["user_id"], "file": key}) + # ) return response diff --git a/app/tests/test_download.py b/app/tests/test_download.py index 07d286f9..4c5f653e 100644 --- a/app/tests/test_download.py +++ b/app/tests/test_download.py @@ -1,4 +1,3 @@ -import json from unittest.mock import MagicMock, patch import boto3 @@ -40,7 +39,7 @@ def test_invalid_id_raises_404(self, client: FlaskClient): @mock_aws def test_download_record_standard_user_with_citable_reference_with_file_extension( - self, app, client, mock_standard_user, caplog + self, app, client, mock_standard_user ): """ Given a File in the database with corresponding file in the s3 bucket @@ -69,19 +68,9 @@ def test_download_record_standard_user_with_citable_reference_with_file_extensio ) assert response.data == b"record" - with client.session_transaction() as session: - user_id = session["user_id"] - - key = file.consignment.ConsignmentReference + "/" + str(file.FileId) - - msg = json.dumps({"user_id": user_id, "file": key}) - - assert caplog.records[1].levelname == "INFO" - assert caplog.records[1].message == msg - @mock_aws def test_download_record_standard_user_with_citable_reference_without_file_extension( - self, app, client, mock_standard_user, caplog + self, app, client, mock_standard_user ): """ Given a File in the database with corresponding file in the s3 bucket @@ -112,16 +101,6 @@ def test_download_record_standard_user_with_citable_reference_without_file_exten ) assert response.data == b"record" - with client.session_transaction() as session: - user_id = session["user_id"] - - key = file.consignment.ConsignmentReference + "/" + str(file.FileId) - - msg = json.dumps({"user_id": user_id, "file": key}) - - assert caplog.records[1].levelname == "INFO" - assert caplog.records[1].message == msg - @mock_aws def test_download_record_standard_user_without_citable_reference( self, app, client, mock_standard_user, caplog @@ -155,16 +134,6 @@ def test_download_record_standard_user_without_citable_reference( ) assert response.data == b"record" - with client.session_transaction() as session: - user_id = session["user_id"] - - key = file.consignment.ConsignmentReference + "/" + str(file.FileId) - - msg = json.dumps({"user_id": user_id, "file": key}) - - assert caplog.records[1].levelname == "INFO" - assert caplog.records[1].message == msg - @mock_aws def test_download_record_standard_user_get_file_errors( self, app, client, mock_standard_user diff --git a/app/tests/test_middlewares.py b/app/tests/test_middlewares.py index ea1f65f5..e7dff311 100644 --- a/app/tests/test_middlewares.py +++ b/app/tests/test_middlewares.py @@ -14,7 +14,7 @@ {"user_id": "test_user"}, lambda: "Test Response", b"Test Response", - '{"event": "page_view", "user_id": "test_user", "route": "/test_route", "method": "GET"}', + '{"event": "api_request", "user_id": "test_user", "route": "/test_route", "method": "GET"}', ), ( "/anonymous_route", @@ -22,7 +22,7 @@ {}, lambda: "Anonymous Response", b"Anonymous Response", - '{"event": "page_view", "user_id": "anonymous", "route": "/anonymous_route", "method": "GET"}', + '{"event": "api_request", "user_id": "anonymous", "route": "/anonymous_route", "method": "GET"}', ), ( "/post_route", @@ -30,7 +30,7 @@ {"user_id": "test_user"}, lambda: "Post Response", b"Post Response", - '{"event": "page_view", "user_id": "test_user", "route": "/post_route", "method": "POST"}', + '{"event": "api_request", "user_id": "test_user", "route": "/post_route", "method": "POST"}', ), ], ) @@ -46,6 +46,8 @@ def test_log_page_view( expected_log, caplog, ): + """Test that the log_page_view middleware generates the correct log when an + endpoint decorated with it is requested""" @app.route(route_path, methods=[method]) @log_page_view From e449f2b82ff15e7b05a5a0853cb8cef5cf89142c Mon Sep 17 00:00:00 2001 From: LTrasca Date: Fri, 29 Nov 2024 14:55:21 +0000 Subject: [PATCH 7/7] removed reference to caplog fixture --- app/tests/test_download.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/tests/test_download.py b/app/tests/test_download.py index 4c5f653e..983a5b21 100644 --- a/app/tests/test_download.py +++ b/app/tests/test_download.py @@ -103,7 +103,7 @@ def test_download_record_standard_user_with_citable_reference_without_file_exten @mock_aws def test_download_record_standard_user_without_citable_reference( - self, app, client, mock_standard_user, caplog + self, app, client, mock_standard_user ): """ Given a File in the database with corresponding file in the s3 bucket