move approval logic to ModifyRoleGroups

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

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,
+        )

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(

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