From 1ed8667e969727122e33c4f103e9a6f1c163c5a8 Mon Sep 17 00:00:00 2001 From: eguerrant Date: Tue, 5 Nov 2024 16:42:19 -0800 Subject: [PATCH] move approval logic to ModifyRoleGroups --- api/operations/approve_role_request.py | 24 +---- api/operations/modify_role_groups.py | 63 +++++++++++++ api/views/resources/role_request.py | 2 +- tests/test_role_request.py | 118 +++++++++++++++++++++++-- 4 files changed, 178 insertions(+), 29 deletions(-) diff --git a/api/operations/approve_role_request.py b/api/operations/approve_role_request.py index 505142a..8966fb3 100644 --- a/api/operations/approve_role_request.py +++ b/api/operations/approve_role_request.py @@ -6,7 +6,6 @@ from api.extensions import db from api.models import AccessRequestStatus, AppGroup, OktaGroup, OktaUser, RoleGroup, RoleRequest -from api.models.access_request import get_all_possible_request_approvers from api.operations.constraints import CheckForReason from api.operations.modify_role_groups import ModifyRoleGroups from api.plugins import get_notification_hook @@ -80,12 +79,6 @@ def execute(self) -> RoleRequest: if not self.role_request.active_requested_group.is_managed: return self.role_request - self.role_request.status = AccessRequestStatus.APPROVED - self.role_request.resolved_at = db.func.now() - self.role_request.resolver_user_id = self.approver_id - self.role_request.resolution_reason = self.approval_reason - self.role_request.approval_ending_at = self.ending_at - db.session.commit() # Audit logging @@ -123,6 +116,7 @@ def execute(self) -> RoleRequest: owner_groups_to_add=[self.role_request.requested_group_id], current_user_id=self.approver_id, created_reason=self.approval_reason, + notify=self.notify, ).execute() else: ModifyRoleGroups( @@ -131,21 +125,7 @@ def execute(self) -> RoleRequest: groups_to_add=[self.role_request.requested_group_id], current_user_id=self.approver_id, created_reason=self.approval_reason, + notify=self.notify, ).execute() - if self.notify: - requester = db.session.get(OktaUser, self.role_request.requester_user_id) - requester_role = db.session.get(OktaGroup, self.role_request.requester_role_id) - - approvers = get_all_possible_request_approvers(self.role_request) - - self.notification_hook.access_role_request_completed( - role_request=self.role_request, - role=requester_role, - group=group, - requester=requester, - approvers=approvers, - notify_requester=True, - ) - return self.role_request diff --git a/api/operations/modify_role_groups.py b/api/operations/modify_role_groups.py index 0f8583e..fa8261a 100644 --- a/api/operations/modify_role_groups.py +++ b/api/operations/modify_role_groups.py @@ -15,6 +15,7 @@ OktaUserGroupMember, RoleGroup, RoleGroupMap, + RoleRequest, Tag, ) from api.models.access_request import get_all_possible_request_approvers @@ -37,6 +38,7 @@ def __init__( sync_to_okta: bool = True, current_user_id: Optional[str] = None, created_reason: str = "", + notify: bool = True, ): if isinstance(role_group, str): self.role = ( @@ -98,6 +100,8 @@ def __init__( self.created_reason = created_reason + self.notify = notify + self.notification_hook = get_notification_hook() def execute(self) -> RoleGroup: @@ -417,6 +421,36 @@ async def _execute(self) -> RoleGroup: ) ) + # Approve any pending role requests for memberships granted by this operation + pending_role_requests_query = ( + RoleRequest.query.filter(RoleRequest.status == AccessRequestStatus.PENDING) + .filter(RoleRequest.resolved_at.is_(None)) + .filter(RoleRequest.requester_role == self.role) + ) + + added_group_ids = [group.id for group in self.groups_to_add] + pending_role_memberships = ( + pending_role_requests_query.filter(RoleRequest.request_ownership.is_(False)) + .filter(RoleRequest.requested_group_id.in_(added_group_ids)) + .all() + ) + for role_request in pending_role_memberships: + async_tasks.append( + self._approve_role_request(role_request, role_memberships_added[role_request.requested_group_id]) + ) + + # Approve any pending role requests for ownerships granted by this operation + added_owner_group_ids = [group.id for group in self.owner_groups_to_add] + pending_role_ownerships = ( + pending_role_requests_query.filter(RoleRequest.request_ownership.is_(True)) + .filter(RoleRequest.requested_group_id.in_(added_owner_group_ids)) + .all() + ) + for role_request in pending_role_ownerships: + async_tasks.append( + self._approve_role_request(role_request, role_ownerships_added[role_request.requested_group_id]) + ) + db.session.commit() # Commit all changes @@ -490,3 +524,32 @@ async def _notify_access_request(self, access_request: AccessRequest) -> None: approvers=approvers, notify_requester=True, ) + + def _approve_role_request( + self, role_request: RoleRequest, added_role_group_map: RoleGroupMap + ) -> asyncio.Task[None]: + role_request.status = AccessRequestStatus.APPROVED + role_request.resolved_at = db.func.now() + role_request.resolver_user_id = self.current_user_id + role_request.resolution_reason = self.created_reason + role_request.approval_ending_at = added_role_group_map.ended_at + role_request.approved_membership_id = added_role_group_map.id + + return asyncio.create_task(self._notify_role_request(role_request)) + + async def _notify_role_request(self, role_request: RoleRequest) -> None: + if not self.notify: + return + + requester = db.session.get(OktaUser, role_request.requester_user_id) + + approvers = get_all_possible_request_approvers(role_request) + + self.notification_hook.access_role_request_completed( + role_request=role_request, + role=role_request.requester_role, + group=role_request.requested_group, + requester=requester, + approvers=approvers, + notify_requester=True, + ) diff --git a/api/views/resources/role_request.py b/api/views/resources/role_request.py index 16c1296..a82c6eb 100644 --- a/api/views/resources/role_request.py +++ b/api/views/resources/role_request.py @@ -296,7 +296,7 @@ def get(self) -> ResponseReturnValue: resolver_alias = aliased(OktaUser) query = ( query.join(RoleRequest.requester.of_type(requester_alias)) - .join(RoleRequest.requester_role) + # .join(RoleRequest.requester_role) .join(RoleRequest.requested_group) .outerjoin(RoleRequest.resolver.of_type(resolver_alias)) .filter( diff --git a/tests/test_role_request.py b/tests/test_role_request.py index deca0c9..53d1387 100644 --- a/tests/test_role_request.py +++ b/tests/test_role_request.py @@ -24,11 +24,12 @@ ApproveRoleRequest, CreateRoleRequest, ModifyGroupUsers, + ModifyRoleGroups, RejectRoleRequest, ) from api.plugins import ConditionalAccessResponse, get_conditional_access_hook, get_notification_hook from api.services import okta -from tests.factories import AppGroupFactory, OktaUserFactory, RoleGroupFactory, RoleRequestFactory +from tests.factories import AppGroupFactory, OktaGroupFactory, OktaUserFactory, RoleGroupFactory, RoleRequestFactory SEVEN_DAYS_IN_SECONDS = 7 * 24 * 60 * 60 THREE_DAYS_IN_SECONDS = 3 * 24 * 60 * 60 @@ -422,7 +423,11 @@ def test_create_role_request_not_role_owner( def test_get_all_role_request( - client: FlaskClient, db: SQLAlchemy, role_group: RoleGroup, okta_group: OktaGroup, user: OktaUser + client: FlaskClient, + db: SQLAlchemy, + role_group: RoleGroup, + okta_group: OktaGroup, + user: OktaUser, ) -> None: role_requests_url = url_for("api-role-requests.role_requests") db.session.add(user) @@ -440,15 +445,21 @@ def test_get_all_role_request( assert rep.status_code == 200 results = rep.get_json() - for role_request in role_requests: - assert any(u["id"] == role_request.id for u in results["results"]) + for request in role_requests: + assert any(u["id"] == request.id for u in results["results"]) rep = client.get(role_requests_url, query_string={"q": "pend"}) assert rep.status_code == 200 results = rep.get_json() - for role_request in role_requests: - assert any(u["id"] == role_request.id for u in results["results"]) + for request in role_requests: + assert any(u["id"] == request.id for u in results["results"]) + + rep = client.get(role_requests_url, query_string={"q": role_requests[0].requester_role.name}) + assert rep.status_code == 200 + + results = rep.get_json() + assert any(u["id"] == role_requests[0].id for u in results["results"]) def test_create_role_request_notification( @@ -932,3 +943,98 @@ def test_auto_resolve_create_role_request_with_time_limit_constraint_tag( assert user == kwargs["requester"] assert len(kwargs["group_tags"]) == 1 assert tag in kwargs["group_tags"] + + +def test_role_request_approval_via_direct_add( + client: FlaskClient, + app: Flask, + db: SQLAlchemy, + role_group: RoleGroup, + okta_group: OktaGroup, + user: OktaUser, + mocker: MockerFixture, +) -> None: + okta_group2 = OktaGroupFactory.create() + + db.session.add(user) + db.session.add(role_group) + db.session.add(okta_group) + db.session.add(okta_group2) + db.session.commit() + + ModifyGroupUsers(group=role_group, members_to_add=[user.id], owners_to_add=[user.id], sync_to_okta=False).execute() + + hook = get_notification_hook() + request_created_notification_spy = mocker.patch.object(hook, "access_role_request_created") + request_completed_notification_spy = mocker.patch.object(hook, "access_role_request_completed") + + role_request = CreateRoleRequest( + requester_user=user, + requester_role=role_group, + requested_group=okta_group, + request_ownership=False, + request_reason="test reason", + ).execute() + + assert role_request is not None + assert request_created_notification_spy.call_count == 1 + + access_owner = OktaUser.query.filter(OktaUser.email == app.config["CURRENT_OKTA_USER_EMAIL"]).first() + + ModifyRoleGroups( + role_group=role_group, groups_to_add=[okta_group.id], sync_to_okta=False, created_reason="test" + ).execute() + + assert request_completed_notification_spy.call_count == 1 + _, kwargs = request_completed_notification_spy.call_args + assert kwargs["role_request"] == role_request + assert kwargs["role"] == role_group + assert kwargs["group"] == okta_group + assert kwargs["requester"] == user + assert len(kwargs["approvers"]) == 1 + assert access_owner in kwargs["approvers"] + + group_url = url_for("api-groups.group_members_by_id", group_id=okta_group.id) + rep = client.get(group_url) + assert rep.status_code == 200 + data = rep.get_json() + assert len(data["members"]) == 1 + assert len(data["owners"]) == 0 + assert data["members"][0] == user.id + + request_created_notification_spy.reset_mock() + request_completed_notification_spy.reset_mock() + + role_request = CreateRoleRequest( + requester_user=user, + requester_role=role_group, + requested_group=okta_group2, + request_ownership=True, + request_reason="test reason", + ).execute() + + assert role_request is not None + assert request_created_notification_spy.call_count == 1 + + access_owner = OktaUser.query.filter(OktaUser.email == app.config["CURRENT_OKTA_USER_EMAIL"]).first() + + ModifyRoleGroups( + role_group=role_group, owner_groups_to_add=[okta_group2.id], sync_to_okta=False, created_reason="test" + ).execute() + + assert request_completed_notification_spy.call_count == 1 + _, kwargs = request_completed_notification_spy.call_args + assert kwargs["role_request"] == role_request + assert kwargs["role"] == role_group + assert kwargs["group"] == okta_group2 + assert kwargs["requester"] == user + assert len(kwargs["approvers"]) == 2 + assert access_owner in kwargs["approvers"] + + group_url = url_for("api-groups.group_members_by_id", group_id=okta_group2.id) + rep = client.get(group_url) + assert rep.status_code == 200 + data = rep.get_json() + assert len(data["owners"]) == 1 + assert len(data["members"]) == 0 + assert data["owners"][0] == user.id