From 2fdc4ef6865b6259295c0f6ec6352da3a11c2d89 Mon Sep 17 00:00:00 2001 From: John DeAngelis Date: Wed, 14 Aug 2024 11:25:26 -0400 Subject: [PATCH 1/3] add code to PUT/PATCH /user to prevent user from locking themselves out Signed-off-by: John DeAngelis --- backend/ops_api/ops/services/users.py | 3 +++ backend/ops_api/tests/ops/users/test_users_patch.py | 12 ++++++++++++ backend/ops_api/tests/ops/users/test_users_put.py | 9 +++++++++ 3 files changed, 24 insertions(+) diff --git a/backend/ops_api/ops/services/users.py b/backend/ops_api/ops/services/users.py index 086db49d56..b780677b71 100644 --- a/backend/ops_api/ops/services/users.py +++ b/backend/ops_api/ops/services/users.py @@ -49,6 +49,9 @@ def update_user(session: Session, **kwargs) -> User: raise Forbidden("You do not have permission to update this user.") if data.get("status") in [UserStatus.INACTIVE, UserStatus.LOCKED]: + if user_id == request_user.id: + raise Forbidden("You cannot deactivate yourself.") + user_sessions = get_all_user_sessions(user_id, session) deactivate_all_user_sessions(user_sessions) diff --git a/backend/ops_api/tests/ops/users/test_users_patch.py b/backend/ops_api/tests/ops/users/test_users_patch.py index 1b696450d8..05f886fc4a 100644 --- a/backend/ops_api/tests/ops/users/test_users_patch.py +++ b/backend/ops_api/tests/ops/users/test_users_patch.py @@ -157,3 +157,15 @@ def test_patch_user_changing_status_deactivates_user_session(auth_client, new_us # cleanup loaded_db.delete(user_session) loaded_db.commit() + + +@pytest.mark.usefixtures("app_ctx") +def test_patch_user_cannot_deactivate_yourself(auth_client, new_user, loaded_db, test_admin_user): + response = auth_client.patch( + url_for("api.users-item", id=test_admin_user.id), + json={ + "id": test_admin_user.id, + "status": UserStatus.INACTIVE.name, + }, + ) + assert response.status_code == 403 diff --git a/backend/ops_api/tests/ops/users/test_users_put.py b/backend/ops_api/tests/ops/users/test_users_put.py index 68632021db..28ffef855c 100644 --- a/backend/ops_api/tests/ops/users/test_users_put.py +++ b/backend/ops_api/tests/ops/users/test_users_put.py @@ -222,3 +222,12 @@ def test_put_user_changing_status_deactivates_user_session(auth_client, new_user # cleanup loaded_db.delete(user_session) loaded_db.commit() + + +@pytest.mark.usefixtures("app_ctx") +def test_put_user__cannot_deactivate_yourself(auth_client, new_user, loaded_db, test_admin_user): + response = auth_client.put( + url_for("api.users-item", id=test_admin_user.id), + json={"email": "new_user@example.com", "status": UserStatus.INACTIVE.name}, + ) + assert response.status_code == 403 From b3d2483055328cf8eac06b56f06ecd89f7666cd9 Mon Sep 17 00:00:00 2001 From: John DeAngelis Date: Wed, 14 Aug 2024 13:32:06 -0400 Subject: [PATCH 2/3] return 400 for some /user exceptions rather than 403 which currently logs the user out Signed-off-by: John DeAngelis --- backend/ops_api/ops/services/users.py | 6 +++--- backend/ops_api/tests/ops/users/test_users_patch.py | 6 +++--- backend/ops_api/tests/ops/users/test_users_put.py | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/backend/ops_api/ops/services/users.py b/backend/ops_api/ops/services/users.py index b780677b71..92737540d7 100644 --- a/backend/ops_api/ops/services/users.py +++ b/backend/ops_api/ops/services/users.py @@ -41,16 +41,16 @@ def update_user(session: Session, **kwargs) -> User: raise RuntimeError("No data provided to update user.") if "id" in data and user_id != data.get("id"): - raise Forbidden("User ID does not match ID in data.") + raise RuntimeError("User ID does not match ID in data.") get_user(session, id=user_id) # Ensure user exists if not is_user_admin(request_user): - raise Forbidden("You do not have permission to update this user.") + raise RuntimeError("You do not have permission to update this user.") if data.get("status") in [UserStatus.INACTIVE, UserStatus.LOCKED]: if user_id == request_user.id: - raise Forbidden("You cannot deactivate yourself.") + raise RuntimeError("You cannot deactivate yourself.") user_sessions = get_all_user_sessions(user_id, session) deactivate_all_user_sessions(user_sessions) diff --git a/backend/ops_api/tests/ops/users/test_users_patch.py b/backend/ops_api/tests/ops/users/test_users_patch.py index 05f886fc4a..33f6918364 100644 --- a/backend/ops_api/tests/ops/users/test_users_patch.py +++ b/backend/ops_api/tests/ops/users/test_users_patch.py @@ -63,7 +63,7 @@ def test_patch_user_unauthorized_different_user(client, loaded_db, test_non_admi json={"id": test_user.id, "email": "new_user@example.com", "first_name": "New First Name"}, headers={"Authorization": f"Bearer {str(access_token)}"}, ) - assert response.status_code == 403 + assert response.status_code == 400 @pytest.mark.usefixtures("app_ctx") @@ -121,7 +121,7 @@ def test_patch_user_must_be_user_admin_to_change_status(client, test_user, test_ json={"id": test_non_admin_user.id, "status": UserStatus.ACTIVE.name}, headers={"Authorization": f"Bearer {str(access_token)}"}, ) - assert response.status_code == 403 + assert response.status_code == 400 def test_patch_user_changing_status_deactivates_user_session(auth_client, new_user, loaded_db): @@ -168,4 +168,4 @@ def test_patch_user_cannot_deactivate_yourself(auth_client, new_user, loaded_db, "status": UserStatus.INACTIVE.name, }, ) - assert response.status_code == 403 + assert response.status_code == 400 diff --git a/backend/ops_api/tests/ops/users/test_users_put.py b/backend/ops_api/tests/ops/users/test_users_put.py index 28ffef855c..23e70cb5ef 100644 --- a/backend/ops_api/tests/ops/users/test_users_put.py +++ b/backend/ops_api/tests/ops/users/test_users_put.py @@ -65,7 +65,7 @@ def test_put_user_unauthorized_different_user(client, loaded_db, test_non_admin_ json={"id": test_user.id, "email": "new_user@example.com", "first_name": "New First Name"}, headers={"Authorization": f"Bearer {str(access_token)}"}, ) - assert response.status_code == 403 + assert response.status_code == 400 @pytest.mark.usefixtures("app_ctx") @@ -168,7 +168,7 @@ def test_put_user_wrong_user(auth_client, new_user, loaded_db, test_admin_user): url_for("api.users-item", id=new_user.id), json={"id": 0, "email": "new_user@example.com"}, ) - assert response.status_code == 403 + assert response.status_code == 400 def test_put_user_must_be_user_admin_to_change_status(client, test_user, test_non_admin_user): @@ -181,7 +181,7 @@ def test_put_user_must_be_user_admin_to_change_status(client, test_user, test_no json={"id": test_non_admin_user.id, "status": UserStatus.ACTIVE.name}, headers={"Authorization": f"Bearer {str(access_token)}"}, ) - assert response.status_code == 403 + assert response.status_code == 400 def test_put_user_changing_status_deactivates_user_session(auth_client, new_user, loaded_db): @@ -230,4 +230,4 @@ def test_put_user__cannot_deactivate_yourself(auth_client, new_user, loaded_db, url_for("api.users-item", id=test_admin_user.id), json={"email": "new_user@example.com", "status": UserStatus.INACTIVE.name}, ) - assert response.status_code == 403 + assert response.status_code == 400 From a6dfa27425c98ef7bfd10689a3431225d000a415 Mon Sep 17 00:00:00 2001 From: John DeAngelis Date: Wed, 14 Aug 2024 13:39:25 -0400 Subject: [PATCH 3/3] return 400 for some /user exceptions rather than 403 which currently logs the user out Signed-off-by: John DeAngelis --- backend/ops_api/ops/error_handlers.py | 7 ++++++- backend/ops_api/ops/services/users.py | 10 +++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/backend/ops_api/ops/error_handlers.py b/backend/ops_api/ops/error_handlers.py index e0eeb5153b..cb4cae5674 100644 --- a/backend/ops_api/ops/error_handlers.py +++ b/backend/ops_api/ops/error_handlers.py @@ -1,7 +1,7 @@ from flask_jwt_extended.exceptions import NoAuthorizationError from marshmallow import ValidationError from sqlalchemy.exc import PendingRollbackError -from werkzeug.exceptions import Forbidden, NotFound +from werkzeug.exceptions import BadRequest, Forbidden, NotFound from ops_api.ops.auth.exceptions import AuthenticationError, InvalidUserSessionError, NotActiveUserError from ops_api.ops.utils.response import make_response_with_headers @@ -58,6 +58,11 @@ def handle_exception_forbidden(e): app.logger.exception(e) return make_response_with_headers({}, 403) + @app.errorhandler(BadRequest) + def handle_exception_bad_request(e): + app.logger.exception(e) + return make_response_with_headers({}, 400) + @app.errorhandler(Exception) def handle_exception(e): app.logger.exception(e) diff --git a/backend/ops_api/ops/services/users.py b/backend/ops_api/ops/services/users.py index 92737540d7..a273a2bd69 100644 --- a/backend/ops_api/ops/services/users.py +++ b/backend/ops_api/ops/services/users.py @@ -2,7 +2,7 @@ from sqlalchemy import ColumnElement, select from sqlalchemy.orm import Session -from werkzeug.exceptions import Forbidden, NotFound +from werkzeug.exceptions import BadRequest, Forbidden, NotFound from models import Role, User, UserStatus from ops_api.ops.auth.utils import deactivate_all_user_sessions, get_all_user_sessions @@ -38,19 +38,19 @@ def update_user(session: Session, **kwargs) -> User: data = kwargs.get("data", {}) if not data: - raise RuntimeError("No data provided to update user.") + raise BadRequest("No data provided to update user.") if "id" in data and user_id != data.get("id"): - raise RuntimeError("User ID does not match ID in data.") + raise BadRequest("User ID does not match ID in data.") get_user(session, id=user_id) # Ensure user exists if not is_user_admin(request_user): - raise RuntimeError("You do not have permission to update this user.") + raise BadRequest("You do not have permission to update this user.") if data.get("status") in [UserStatus.INACTIVE, UserStatus.LOCKED]: if user_id == request_user.id: - raise RuntimeError("You cannot deactivate yourself.") + raise BadRequest("You cannot deactivate yourself.") user_sessions = get_all_user_sessions(user_id, session) deactivate_all_user_sessions(user_sessions)