From 607697d0f6d1394579d4a7203b79520571351d03 Mon Sep 17 00:00:00 2001 From: Peter C Date: Sat, 20 Apr 2024 22:14:24 -0400 Subject: [PATCH] Fix access request completed notification parameter for ModifyGroupRoles (#42) --- api/operations/modify_role_groups.py | 2 +- api/plugins/conditional_access.py | 2 +- api/plugins/notifications.py | 8 ++++---- tests/test_access_request.py | 21 +++++++++++++++++++++ 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/api/operations/modify_role_groups.py b/api/operations/modify_role_groups.py index b9fb7ff..2bc116c 100644 --- a/api/operations/modify_role_groups.py +++ b/api/operations/modify_role_groups.py @@ -517,7 +517,7 @@ async def _notify_access_request(self, access_request: AccessRequest) -> None: self.notification_hook.access_request_completed( access_request=access_request, - group=access_request.requested_group.name, + group=access_request.requested_group, requester=requester, approvers=approvers, notify_requester=True, diff --git a/api/plugins/conditional_access.py b/api/plugins/conditional_access.py index 965ac7f..e0a28ef 100644 --- a/api/plugins/conditional_access.py +++ b/api/plugins/conditional_access.py @@ -48,7 +48,7 @@ def access_request_created( # Log and do not raise since request failures should not # break the flow. The access request can still be manually # approved or denied - logger.error("Failed to execute request created callback") + logger.exception("Failed to execute request created callback") return [] diff --git a/api/plugins/notifications.py b/api/plugins/notifications.py index c0cb901..7dce76f 100644 --- a/api/plugins/notifications.py +++ b/api/plugins/notifications.py @@ -62,7 +62,7 @@ def access_request_created(access_request: AccessRequest, # Log and do not raise since notification failures should not # break the flow. Users can still manually ping approvers # to process their request from the UI - logger.error("Failed to execute access request created notification callback") + logger.exception("Failed to execute access request created notification callback") @hookimpl(wrapper=True) @@ -77,7 +77,7 @@ def access_request_completed(access_request: AccessRequest, # Log and do not raise since notification failures should not # break the flow. Users can still manually ping approvers # to process their request from the UI - logger.error("Failed to execute access request completed notification callback") + logger.exception("Failed to execute access request completed notification callback") @hookimpl(wrapper=True) @@ -90,7 +90,7 @@ def access_expiring_user(groups: list[OktaGroup], # Log and do not raise since notification failures should not # break the flow. Users can still manually ping approvers # to process their request from the UI - logger.error("Failed to execute access expiring for user notification callback") + logger.exception("Failed to execute access expiring for user notification callback") @hookimpl(wrapper=True) def access_expiring_owner(owner: OktaUser, @@ -104,7 +104,7 @@ def access_expiring_owner(owner: OktaUser, # Log and do not raise since notification failures should not # break the flow. Users can still manually ping approvers # to process their request from the UI - logger.error("Failed to execute access expiring for owner notification callback") + logger.exception("Failed to execute access expiring for owner notification callback") def get_notification_hook() -> pluggy.HookRelay: diff --git a/tests/test_access_request.py b/tests/test_access_request.py index 3f93b9d..03f519b 100644 --- a/tests/test_access_request.py +++ b/tests/test_access_request.py @@ -415,6 +415,10 @@ def test_create_app_access_request_notification( assert access_request is not None assert request_created_notification_spy.call_count == 1 + _, kwargs = request_created_notification_spy.call_args + assert kwargs['access_request'] == access_request + assert kwargs['group'] == app_group + assert kwargs['requester'] == user access_owner = OktaUser.query.filter( OktaUser.email == app.config["CURRENT_OKTA_USER_EMAIL"] @@ -426,6 +430,10 @@ def test_create_app_access_request_notification( assert add_membership_spy.call_count == 1 assert request_completed_notification_spy.call_count == 1 + _, kwargs = request_completed_notification_spy.call_args + assert kwargs['access_request'] == access_request + assert kwargs['group'] == app_group + assert kwargs['requester'] == user access_request.status = AccessRequestStatus.PENDING access_request.resolved_at = None @@ -440,6 +448,10 @@ def test_create_app_access_request_notification( assert add_membership_spy.call_count == 0 assert request_completed_notification_spy.call_count == 1 + _, kwargs = request_completed_notification_spy.call_args + assert kwargs['access_request'] == access_request + assert kwargs['group'] == app_group + assert kwargs['requester'] == user def test_get_all_possible_request_approvers(app: Flask, mocker: MockerFixture, db: SQLAlchemy) -> None: access_admin = OktaUser.query.filter(OktaUser.email == app.config["CURRENT_OKTA_USER_EMAIL"]).first() @@ -528,6 +540,9 @@ def test_resolve_app_access_request_notification( assert access_request is not None assert request_created_notification_spy.call_count == 1 _, kwargs = request_created_notification_spy.call_args + assert kwargs['access_request'] == access_request + assert kwargs['group'] == app_group + assert kwargs['requester'] == user assert len(kwargs['approvers']) == 2 assert app_owner_user1 in kwargs['approvers'] assert app_owner_user2 in kwargs['approvers'] @@ -539,6 +554,9 @@ def test_resolve_app_access_request_notification( assert add_ownership_spy.call_count == 1 assert request_completed_notification_spy.call_count == 1 _, kwargs = request_completed_notification_spy.call_args + assert kwargs['access_request'] == access_request + assert kwargs['group'] == app_group + assert kwargs['requester'] == user assert len(kwargs['approvers']) == 4 assert access_admin in kwargs['approvers'] assert app_owner_user1 in kwargs['approvers'] @@ -560,6 +578,9 @@ def test_resolve_app_access_request_notification( assert add_ownership_spy.call_count == 0 assert request_completed_notification_spy.call_count == 1 _, kwargs = request_completed_notification_spy.call_args + assert kwargs['access_request'] == access_request + assert kwargs['group'] == app_group + assert kwargs['requester'] == user assert len(kwargs['approvers']) == 4 assert access_admin in kwargs['approvers'] assert app_owner_user1 in kwargs['approvers']