From 5e81ed5b7b3234fdd2ce6218f7d25c5af77151ac Mon Sep 17 00:00:00 2001 From: MCatherine Date: Tue, 23 Jul 2024 15:06:10 -0700 Subject: [PATCH] fix: #1367 Fix and simplify audit log (#1498) --- .../router_access_control_privilege.py | 35 +++++----- .../app/routers/router_application_admin.py | 64 ++++++++++--------- .../routers/router_user_role_assignment.py | 60 ++++++++--------- server/backend/api/app/utils/audit_util.py | 5 +- 4 files changed, 81 insertions(+), 83 deletions(-) diff --git a/server/admin_management/api/app/routers/router_access_control_privilege.py b/server/admin_management/api/app/routers/router_access_control_privilege.py index 7b8b92208..9a610c838 100644 --- a/server/admin_management/api/app/routers/router_access_control_privilege.py +++ b/server/admin_management/api/app/routers/router_access_control_privilege.py @@ -2,7 +2,7 @@ from typing import List from api.app import jwt_validation, schemas -from api.app.models import model as models +from api.app.models.model import FamUser from api.app.routers.router_guards import ( authorize_by_app_id, authorize_by_application_role, @@ -76,17 +76,22 @@ def create_access_control_privilege_many( ) try: - audit_event_log.requesting_user = user_service.get_user_by_cognito_user_id( - requester.cognito_user_id - ) + audit_event_log.requesting_user = requester audit_event_log.role = role_service.get_role_by_id( access_control_privilege_request.role_id ) audit_event_log.application = audit_event_log.role.application - return access_control_privilege_service.create_access_control_privilege_many( + response = access_control_privilege_service.create_access_control_privilege_many( access_control_privilege_request, requester.cognito_user_id, target_user ) + # get target user from database, so for existing user, we can get the cognito user id + audit_event_log.target_user = user_service.get_user_by_domain_and_guid( + access_control_privilege_request.user_type_code, + access_control_privilege_request.user_guid, + ) + + return response except Exception as e: audit_event_log.event_outcome = AuditEventOutcome.FAIL @@ -94,16 +99,13 @@ def create_access_control_privilege_many( raise e finally: - audit_event_log.target_user = user_service.get_user_by_domain_and_guid( - access_control_privilege_request.user_type_code, - access_control_privilege_request.user_guid, - ) + # if failed to get target user from database, use the information from request if audit_event_log.target_user is None: - audit_event_log.target_user = models.FamUser( - user_type_code=access_control_privilege_request.user_type_code, - user_name=access_control_privilege_request.user_name, - user_guid="unknown", - cognito_user_id="unknown", + audit_event_log.target_user = FamUser( + user_type_code=target_user.user_type_code, + user_name=target_user.user_name, + user_guid=target_user.user_guid, + cognito_user_id=target_user.cognito_user_id, ) audit_event_log.log_event() @@ -141,7 +143,6 @@ def get_access_control_privileges_by_application_id( def delete_access_control_privilege( access_control_privilege_id: int, request: Request, - user_service: UserService = Depends(user_service_instance), access_control_privilege_service: AccessControlPrivilegeService = Depends( access_control_privilege_service_instance ), @@ -158,9 +159,7 @@ def delete_access_control_privilege( ) try: - audit_event_log.requesting_user = user_service.get_user_by_cognito_user_id( - requester.cognito_user_id - ) + audit_event_log.requesting_user = requester access_control_privilege = access_control_privilege_service.get_acp_by_id( access_control_privilege_id ) diff --git a/server/admin_management/api/app/routers/router_application_admin.py b/server/admin_management/api/app/routers/router_application_admin.py index 21e3cb996..e4022fdb6 100644 --- a/server/admin_management/api/app/routers/router_application_admin.py +++ b/server/admin_management/api/app/routers/router_application_admin.py @@ -2,23 +2,26 @@ from typing import List from api.app import database, jwt_validation, schemas -from api.app.models import model as models -from api.app.routers.router_guards import (authorize_by_fam_admin, - enforce_self_grant_guard, - get_current_requester, - get_verified_target_user, - validate_param_application_admin_id, - validate_param_application_id, - validate_param_user_type) -from api.app.routers.router_utils import (application_admin_service_instance, - application_service_instance, - user_service_instance) +from api.app.models.model import FamUser +from api.app.routers.router_guards import ( + authorize_by_fam_admin, + enforce_self_grant_guard, + get_current_requester, + get_verified_target_user, + validate_param_application_admin_id, + validate_param_application_id, + validate_param_user_type, +) +from api.app.routers.router_utils import ( + application_admin_service_instance, + application_service_instance, + user_service_instance, +) from api.app.schemas import Requester, TargetUser from api.app.services.application_admin_service import ApplicationAdminService from api.app.services.application_service import ApplicationService from api.app.services.user_service import UserService -from api.app.utils.audit_util import (AuditEventLog, AuditEventOutcome, - AuditEventType) +from api.app.utils.audit_util import AuditEventLog, AuditEventOutcome, AuditEventType from fastapi import APIRouter, Depends, Request, Response from sqlalchemy.orm import Session @@ -75,33 +78,36 @@ def create_application_admin( ) try: - audit_event_log.requesting_user = user_service.get_user_by_cognito_user_id( - requester.cognito_user_id - ) + audit_event_log.requesting_user = requester audit_event_log.application = application_service.get_application( application_admin_request.application_id ) - return application_admin_service.create_application_admin( + response = application_admin_service.create_application_admin( application_admin_request, target_user, requester.cognito_user_id ) + # get target user from database, so for existing user, we can get the cognito user id + audit_event_log.target_user = user_service.get_user_by_domain_and_guid( + application_admin_request.user_type_code, + application_admin_request.user_guid, + ) + + return response + except Exception as e: audit_event_log.event_outcome = AuditEventOutcome.FAIL audit_event_log.exception = e raise e finally: - audit_event_log.target_user = user_service.get_user_by_domain_and_guid( - application_admin_request.user_type_code, - application_admin_request.user_guid, - ) + # if failed to get target user from database, use the information from request if audit_event_log.target_user is None: - audit_event_log.target_user = models.FamUser( - user_type_code=application_admin_request.user_type_code, - user_name=application_admin_request.user_name, - user_guid="unknown", - cognito_user_id="unknown", + audit_event_log.target_user = FamUser( + user_type_code=target_user.user_type_code, + user_name=target_user.user_name, + user_guid=target_user.user_guid, + cognito_user_id=target_user.cognito_user_id, ) audit_event_log.log_event() @@ -134,14 +140,10 @@ def delete_application_admin( try: application_admin_service = ApplicationAdminService(db) - user_service = UserService(db) - application_admin = application_admin_service.get_application_admin_by_id( application_admin_id ) - audit_event_log.requesting_user = user_service.get_user_by_cognito_user_id( - requester.cognito_user_id - ) + audit_event_log.requesting_user = requester audit_event_log.application = application_admin.application audit_event_log.target_user = application_admin.user diff --git a/server/backend/api/app/routers/router_user_role_assignment.py b/server/backend/api/app/routers/router_user_role_assignment.py index bf8f29f4f..ae434d27c 100644 --- a/server/backend/api/app/routers/router_user_role_assignment.py +++ b/server/backend/api/app/routers/router_user_role_assignment.py @@ -2,15 +2,19 @@ from http import HTTPStatus from api.app.crud import crud_role, crud_user, crud_user_role -from api.app.models import model as models from api.app.routers.router_guards import ( - authorize_by_application_role, authorize_by_privilege, - authorize_by_user_type, enforce_bceid_by_same_org_guard, - enforce_bceid_terms_conditions_guard, enforce_self_grant_guard, - get_current_requester, get_verified_target_user) + authorize_by_application_role, + authorize_by_privilege, + authorize_by_user_type, + enforce_bceid_by_same_org_guard, + enforce_bceid_terms_conditions_guard, + enforce_self_grant_guard, + get_current_requester, + get_verified_target_user, +) from api.app.schemas import Requester, TargetUser -from api.app.utils.audit_util import (AuditEventLog, AuditEventOutcome, - AuditEventType) +from api.app.models.model import FamUser +from api.app.utils.audit_util import AuditEventLog, AuditEventOutcome, AuditEventType from fastapi import APIRouter, Depends, Request, Response from sqlalchemy.orm import Session @@ -67,21 +71,24 @@ def create_user_role_assignment( ) try: - - requesting_user = get_requesting_user(db, requester.cognito_user_id) role = crud_role.get_role(db, role_assignment_request.role_id) audit_event_log.role = role audit_event_log.application = role.application - audit_event_log.requesting_user = requesting_user + audit_event_log.requesting_user = requester - return crud_user_role.create_user_role( + response = crud_user_role.create_user_role( + db, role_assignment_request, target_user, requester.cognito_user_id + ) + # get target user from database, so for existing user, we can get the cognito user id + audit_event_log.target_user = crud_user.get_user_by_domain_and_guid( db, - role_assignment_request, - target_user, - requesting_user.cognito_user_id + role_assignment_request.user_type_code, + role_assignment_request.user_guid, ) + return response + except Exception as e: audit_event_log.event_outcome = AuditEventOutcome.FAIL audit_event_log.exception = e @@ -89,19 +96,14 @@ def create_user_role_assignment( raise e finally: - audit_event_log.target_user = crud_user.get_user_by_domain_and_name( - db, - role_assignment_request.user_type_code, - role_assignment_request.user_name, - ) + # if failed to get target user from database, use the information from request if audit_event_log.target_user is None: - audit_event_log.target_user = models.FamUser( - user_type_code=role_assignment_request.user_type_code, - user_name=role_assignment_request.user_name, - user_guid="unknown", - cognito_user_id="unknown", + audit_event_log.target_user = FamUser( + user_type_code=target_user.user_type_code, + user_name=target_user.user_name, + user_guid=target_user.user_guid, + cognito_user_id=target_user.cognito_user_id, ) - audit_event_log.log_event() @@ -144,13 +146,12 @@ def delete_user_role_assignment( ) try: - requesting_user = get_requesting_user(db, requester.cognito_user_id) user_role = crud_user_role.find_by_id(db, user_role_xref_id) audit_event_log.role = user_role.role audit_event_log.target_user = user_role.user audit_event_log.application = user_role.role.application - audit_event_log.requesting_user = requesting_user + audit_event_log.requesting_user = requester crud_user_role.delete_fam_user_role_assignment(db, user_role_xref_id) @@ -165,8 +166,3 @@ def delete_user_role_assignment( user_role.role.client_number.forest_client_number ) audit_event_log.log_event() - - -def get_requesting_user(db: Session, cognito_user_id: str) -> models.FamUser: - requester = crud_user.get_user_by_cognito_user_id(db, cognito_user_id) - return requester diff --git a/server/backend/api/app/utils/audit_util.py b/server/backend/api/app/utils/audit_util.py index ceeb82990..b9f909d3d 100644 --- a/server/backend/api/app/utils/audit_util.py +++ b/server/backend/api/app/utils/audit_util.py @@ -2,6 +2,7 @@ from enum import Enum import json from api.app.models import model as models +from api.app.schemas import Requester from fastapi import Request, HTTPException @@ -25,7 +26,7 @@ class AuditEventLog: application: models.FamApplication role: models.FamRole forest_client_number: str - requesting_user: models.FamUser + requesting_user: Requester target_user: models.FamUser exception: Exception @@ -37,7 +38,7 @@ def __init__( application: models.FamApplication = None, role: models.FamRole = None, forest_client_number: str = None, - requesting_user: models.FamUser = None, + requesting_user: Requester = None, target_user: models.FamUser = None, exception: Exception = None, ):