diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 6b39cd83..bc78267f 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -498,15 +498,34 @@ role membership and the direct grant are not collapsed into one record. ## Approver routing -For all request types, approval routing follows this chain — this affects notification plugin -code and any logic that determines who can approve: - -1. **Group/role owner** — designated owner(s) of the target group or role; groups may be unowned -2. **App owner** — if the target is an `AppGroup` with no group owners, falls back to app owner; apps may also be unowned -3. **Access admin** — final fallback if no owners exist at any prior tier - -Implemented in `api/models/okta_group.py` (`get_group_managers`) and -`api/models/app_group.py` (`get_app_managers`, `get_access_owners`). +Who reviews a request is defined once, in `api/models/request_reviewers.py`. Notifications +(open and close), the "Assigned to Me" views, and the request detail pages all read it; never +rebuild the chain elsewhere. + +Each request has owner levels, nearest first: + +| Request | Owner levels | +|---|---| +| Access request, individual or role-based, for group G | owners of G → owners of G's app (app groups only) → Access admins | +| Group request to create G | owners of the requested app (app groups only) → Access admins | + +An **eligible reviewer** owns at a level and is not the requester, not deleted, and (unless an +Access admin) not blocked by a `disallow_self_add_*` tag: approving a role-based request would +add a reviewer who is a member of the requesting role to G. This mirrors +`CheckForSelfAdd.execute_for_role`. A request's **assigned reviewers** are the eligible reviewers +at the nearest level that has any. The detail pages list the **eligible reviewers by level** +(`get_eligible_reviewers_by_level`): nearest first, each user once at their nearest level, levels +with no eligible reviewer left out. The assigned reviewers are therefore listed first, and the +rest show a requester who to contact to escalate. + +Close notifications go to the reviewers assigned while the request was open, so an operation that +changes ownership or deletes a group takes that snapshot (`snapshot_assigned_reviewers`, or +`snapshot_obsolete_request_reviewers` for a group's pending requests) before it makes the change. + +Assignment is not authorization. Who may approve or reject is checked by the resolve routes +(`can_manage_group`, the self-approval guard, `CheckForSelfAdd`, and the group-request +admin/app-owner checks); an Access admin can resolve any request. Every assigned reviewer can +approve; `tests/test_request_reviewers.py` checks that. ## Authorization @@ -586,8 +605,7 @@ owner, app owner (if app group), or Access admin. **`GroupRequest`** — a user requests creation of a new group, app group, or role. A new `App` cannot be requested via this flow. On approval, the requester is added as an owner. App group -requests require selecting a parent app and route to the app's owners if they exist, otherwise -to Access admins; vanilla group and role requests route to Access admins. +requests require selecting a parent app. For who reviews it, see Approver routing. ## Plugin system @@ -700,3 +718,6 @@ infrastructure details live there too. and role adds too - **Querying tags without filtering for enabled** — disabled tags still exist in the DB - **Putting operator-specific logic in the Access repo** — it belongs in an operator's own plugin +- **Rebuilding the reviewer chain** — use `api/models/request_reviewers.py` + (`get_assigned_reviewers`, `is_assigned_reviewer`) instead of chaining + `get_group_managers` / `get_app_managers` / `get_access_owners` diff --git a/README.md b/README.md index 8d93ab23..ef54ba61 100644 --- a/README.md +++ b/README.md @@ -309,11 +309,10 @@ A value of `0` or less is rejected at startup: it would close every pending request on the next sync, and is almost always a mistaken attempt to disable expiration. Use `never` for that. -Closing a request notifies its requester and every approver eligible to review -it; for a role request that includes Access admins. A sync run that closes many -requests at once sends a correspondingly large number of notifications, so on a -busy instance it is worth knowing how many requests are pending past a cutoff or -past their requested end date. +Closing a request notifies its requester and its assigned reviewers. A sync run +that closes many requests at once sends a correspondingly large number of +notifications, so on a busy instance it is worth knowing how many requests are +pending past a cutoff or past their requested end date. ##### Requiring request reasons @@ -619,7 +618,7 @@ As of Access 2.0 the plugin hooks are **async** — Access is an async applicati - **Delivery is not guaranteed, and `sync_group` is only as much of a safety net as you make it.** A worker killed between the response and the drain drops the fire, as does a request that fails after queuing one. Access does not police what `sync_group` does, so a plugin that implements it as a full idempotent reconciliation converges afterwards and one that does not, does not. One gap it cannot close either way: it sees only current membership, so a delta-based revoke cannot recover a lost `group_members_removed`. If your plugin records events rather than converging state — an audit or notification plugin — expect gaps. - **`group_deleted` is the exception and runs inline, on the request.** The sweep skips soft-deleted groups, so nothing would revisit a delete that went missing, and the external group would outlive the Access one. Your `group_deleted` therefore blocks the API response — keep it as quick as the external system allows — and the only way to lose it is to raise. - **Whatever a hook writes is absent from the create/update response the user just got.** Declare `pending_values` on the status property that carries your reconcile state (e.g. `pending_values=("pending",)` on a `sync_status`) and the UI keeps refreshing the group until it moves off them. If you declare them, write a status on *every* reconcile outcome, including the ones where you decide there is nothing to do — an empty status reads as "the hook has not run yet" and the UI will poll for it. -- **Request-path notification hooks receive read-only, detached ORM snapshots.** Every notification raised from an HTTP request — the `*_created` and `*_completed` hooks for access, role, and group requests — runs in a post-response background task, after the request's DB session has been torn down, so Access detaches (`expunge`s) the objects it passes. Read only the attributes Access already loaded — the request, its group/role, a group request's `requested_app`, the requester, and the approvers, and their columns — and don't trigger a lazy load / re-query (there's no live session behind them) or expect mutations to persist. If your notifier needs a wider object graph than what's passed, open an issue so the operation can eager-load it. (The `access_expiring_*` hooks are the exception — they run inline in the syncer CronJob.) +- **Request-path notification hooks receive read-only, detached ORM snapshots.** Every notification raised from an HTTP request — the `*_created` and `*_completed` hooks for access, role, and group requests — runs in a post-response background task, after the request's DB session has been torn down, so Access detaches (`expunge`s) the objects it passes. Read only the attributes Access already loaded — the request, its group/role, a group request's `requested_app`, the requester, and the approvers, and their columns — and don't trigger a lazy load / re-query (there's no live session behind them) or expect mutations to persist. `approvers` is the request's assigned reviewers in every request hook; see `NotificationPluginSpec`. If your notifier needs a wider object graph than what's passed, open an issue so the operation can eager-load it. (The `access_expiring_*` hooks are the exception — they run inline in the syncer CronJob.) - **Log through `logging.getLogger(__name__)`.** Access attributes a log line to your plugin by matching the logger name against your plugin's top-level package, so lines from the root logger (`logging.warning(...)`) or a logger named outside your package show up as `lib:` instead of `plugin:`. See `LOG_FORMAT` above. - **Metrics `batch_metrics` is an async context manager** — use it as `async with metrics.batch_metrics(): await metrics.record_counter(...)`. - **These hooks stay synchronous:** the app-group-lifecycle metadata/config-schema/status-schema/validation hooks (`get_plugin_metadata`, `get_plugin_*_config_properties`, `validate_plugin_*_config`, `get_plugin_*_status_properties`). They are pure schema accessors with no I/O. diff --git a/api/models/access_request.py b/api/models/access_request.py deleted file mode 100644 index 8f65bcd4..00000000 --- a/api/models/access_request.py +++ /dev/null @@ -1,35 +0,0 @@ -from typing import List, Set - -from api.models.app_group import get_access_owners, get_app_managers -from api.models.core_models import AccessRequest, AppGroup, GroupRequest, OktaUser, RoleRequest -from api.models.okta_group import get_group_managers - - -async def get_all_possible_request_approvers( - access_request: AccessRequest | RoleRequest | GroupRequest, -) -> Set[OktaUser]: - # This will return the entire set of possible access request approvers - # to ensure that even if the resolved set of approvers changes - # we still are able to mark the request as resolved for any users - # that were notified of the request. - group_owners: List[OktaUser] = [] - app_managers: List[OktaUser] = [] - - # AccessRequest / RoleRequest have `requested_group_id` + `requested_group`; - # GroupRequest doesn't (it tracks a *requested* group as separate name/type - # fields, since the group hasn't been created yet). Branch on the type so - # mypy narrows the union accurately. - if isinstance(access_request, (AccessRequest, RoleRequest)): - group_owners = await get_group_managers(access_request.requested_group_id) - if isinstance(access_request.requested_group, AppGroup): - app_managers = await get_app_managers(access_request.requested_group.app_id) - elif ( - isinstance(access_request, GroupRequest) - and access_request.requested_group_type == "app_group" - and access_request.requested_app_id is not None - ): - app_managers = await get_app_managers(access_request.requested_app_id) - - access_app_owners = await get_access_owners() - - return set(group_owners + access_app_owners + app_managers) diff --git a/api/models/core_models.py b/api/models/core_models.py index e088b626..431992ea 100644 --- a/api/models/core_models.py +++ b/api/models/core_models.py @@ -657,7 +657,9 @@ class AppGroup(OktaGroup): __tablename__ = "app_group" id: Mapped[str] = mapped_column(Unicode(50), ForeignKey("okta_group.id"), primary_key=True) - app_id: Mapped[str] = mapped_column(Unicode(50), ForeignKey("app.id"), nullable=False) + # Indexed for the lookups of an app's groups and its owners group, which + # Postgres would otherwise seq-scan (it does not index FK columns). + app_id: Mapped[str] = mapped_column(Unicode(50), ForeignKey("app.id"), nullable=False, index=True) # Is this group the app owner group and can administer the app and other app groups? # Membership to an app onwer group implicitly grants group owner permissions on the # group to administer and manage membership of the app owner group diff --git a/api/models/request_reviewers.py b/api/models/request_reviewers.py new file mode 100644 index 00000000..e5fad8f7 --- /dev/null +++ b/api/models/request_reviewers.py @@ -0,0 +1,331 @@ +"""Who reviews a request. + +This module is the one definition of request reviewers. Notifications, the +"Assigned to Me" views, and the request detail pages all read it. + +Each request has a list of owner levels, nearest first: + +- An access request (`AccessRequest`) or role request (`RoleRequest`) for + group G: the owners of G; then the owners of G's app, if G is an app + group; then Access admins. +- A group request (`GroupRequest`) to create G: the owners of the requested + app, if G is an app group; then Access admins. + +A user who owns at a level is an eligible reviewer, with three exceptions. +The requester is not eligible. A deleted user is not eligible. And a user +who is not an Access admin is not eligible when approving would add them to +G against an enabled `disallow_self_add_*` tag on G. Only a role request can +do that, when the reviewer is a member of the requesting role. This matches +`CheckForSelfAdd.execute_for_role`, which blocks that approval. + +A request's assigned reviewers are the eligible reviewers at the nearest +level that has any. Each level is a SQL condition on the request row. So one +definition answers both "who reviews this request" +(`get_eligible_reviewers_by_level`, `get_assigned_reviewers`) and "which +requests does this user review" (`is_assigned_reviewer`). + +Being assigned does not decide who may resolve a request. The approve and +reject routes check that on their own. +""" + +from dataclasses import dataclass +from typing import Any, Callable, Iterable, Literal, cast + +from sqlalchemy import ColumnElement, ColumnExpressionArgument, and_, exists, func, not_, or_, select +from sqlalchemy.orm import aliased + +from api.extensions import db +from api.models.core_models import ( + AccessRequest, + App, + AppGroup, + GroupRequest, + OktaGroup, + OktaGroupTagMap, + OktaUser, + OktaUserGroupMember, + RoleRequest, + Tag, +) + +OwnerLevel = Literal["group_owners", "app_owners", "access_admins"] +AnyRequest = AccessRequest | RoleRequest | GroupRequest +RequestModel = type[AccessRequest] | type[RoleRequest] | type[GroupRequest] +ReviewerId = ColumnExpressionArgument[str] | str +_OwnershipCondition = Callable[[RequestModel, ReviewerId], ColumnElement[bool]] + + +@dataclass(frozen=True) +class OwnerLevelReviewers: + """A request's eligible reviewers at one owner level. + + Attributes: + owner_level: The level. + reviewers: The users eligible at this level and at no nearer one. + """ + + owner_level: OwnerLevel + reviewers: list[OktaUser] + + +def _is_active(row: type[OktaUserGroupMember] | type[OktaGroupTagMap]) -> ColumnElement[bool]: + """The row has not ended.""" + return or_(row.ended_at.is_(None), row.ended_at > func.now()) + + +def _exists(froms: tuple[Any, ...], *conditions: ColumnElement[bool]) -> ColumnElement[bool]: + """EXISTS over `froms`, correlated to every other table its conditions name. + + SQLAlchemy only correlates a subquery to the query directly around it. A + nested EXISTS that names the request row or the reviewer column from + further out would get its own unrelated copy of that table. Listing + `froms` also renders a flat-aliased `AppGroup` as `okta_group JOIN + app_group`, not as two unjoined tables. + """ + return exists().select_from(*froms).where(*conditions).correlate_except(*froms) + + +def _owns_group(request_cls: RequestModel, reviewer_id: ReviewerId) -> ColumnElement[bool]: + """The reviewer owns the requested group of an access or role request.""" + assert request_cls is not GroupRequest + cls = cast(type[AccessRequest] | type[RoleRequest], request_cls) + group = aliased(OktaGroup) + ownership = aliased(OktaUserGroupMember) + return _exists( + (group, ownership), + group.id == cls.requested_group_id, + group.deleted_at.is_(None), + ownership.group_id == group.id, + ownership.user_id == reviewer_id, + ownership.is_owner.is_(True), + _is_active(ownership), + ) + + +def _owns_app(request_cls: RequestModel, reviewer_id: ReviewerId) -> ColumnElement[bool]: + """The reviewer owns the owners group of the request's app. + + For a group request, the app is the requested app, and only when the + request is for an app group. For an access or role request, the app is + the one the requested app group belongs to. + + Each branch is one flat EXISTS that joins the requested group (if any), + the owners group, and the ownership row. Postgres can run a flat EXISTS + as a semi-join. An EXISTS nested inside it would make Postgres rescan + users for every request, and an admin's "Assigned to Me" would take tens + of seconds. + + The app's `deleted_at` is not checked, because `DeleteApp` relies on it. + `DeleteApp` marks the app deleted first, then deletes its groups with the + owners group last. Until then, requests for its other groups still go to + the app owners. + """ + owners_group = aliased(AppGroup, flat=True) + ownership = aliased(OktaUserGroupMember) + owns_owners_group = [ + owners_group.is_owner.is_(True), + owners_group.deleted_at.is_(None), + ownership.group_id == owners_group.id, + ownership.user_id == reviewer_id, + ownership.is_owner.is_(True), + _is_active(ownership), + ] + if request_cls is GroupRequest: + return and_( + GroupRequest.requested_group_type == "app_group", + _exists( + (owners_group, ownership), + owners_group.app_id == GroupRequest.requested_app_id, + *owns_owners_group, + ), + ) + cls = cast(type[AccessRequest] | type[RoleRequest], request_cls) + group = aliased(AppGroup, flat=True) + return _exists( + (group, owners_group, ownership), + group.id == cls.requested_group_id, + group.deleted_at.is_(None), + owners_group.app_id == group.app_id, + *owns_owners_group, + ) + + +def _is_access_admin(_request_cls: RequestModel, reviewer_id: ReviewerId) -> ColumnElement[bool]: + """The reviewer is a member of the Access app's owners group, as `is_access_admin` checks.""" + access_app = aliased(App) + owners_group = aliased(AppGroup, flat=True) + membership = aliased(OktaUserGroupMember) + return _exists( + (access_app, owners_group, membership), + access_app.name == App.ACCESS_APP_RESERVED_NAME, + access_app.deleted_at.is_(None), + owners_group.app_id == access_app.id, + owners_group.is_owner.is_(True), + owners_group.deleted_at.is_(None), + membership.group_id == owners_group.id, + membership.user_id == reviewer_id, + membership.is_owner.is_(False), + _is_active(membership), + ) + + +def _owner_levels(request_cls: RequestModel) -> list[tuple[OwnerLevel, _OwnershipCondition]]: + """The owner levels for a kind of request, nearest first.""" + if request_cls is GroupRequest: + return [("app_owners", _owns_app), ("access_admins", _is_access_admin)] + return [ + ("group_owners", _owns_group), + ("app_owners", _owns_app), + ("access_admins", _is_access_admin), + ] + + +def _is_eligible( + request_cls: RequestModel, ownership_condition: _OwnershipCondition, reviewer_id: ReviewerId +) -> ColumnElement[bool]: + """The reviewer owns at this level and is eligible to review the request.""" + reviewer = aliased(OktaUser) + conditions = [ + ownership_condition(request_cls, reviewer_id), + request_cls.requester_user_id != reviewer_id, + _exists((reviewer,), reviewer.id == reviewer_id, reviewer.deleted_at.is_(None)), + ] + if request_cls is RoleRequest: + # Not blocked by a `disallow_self_add_*` tag: approving would add a + # non-admin reviewer who is a member of the requesting role to the group. + role_membership = aliased(OktaUserGroupMember) + group = aliased(OktaGroup) + tag_map = aliased(OktaGroupTagMap) + tag = aliased(Tag) + blocked_by_tag = and_( + not_(_is_access_admin(request_cls, reviewer_id)), + _exists( + (role_membership,), + role_membership.group_id == RoleRequest.requester_role_id, + role_membership.user_id == reviewer_id, + role_membership.is_owner.is_(False), + _is_active(role_membership), + ), + _exists( + (group, tag_map, tag), + group.id == RoleRequest.requested_group_id, + group.is_managed.is_(True), + tag_map.group_id == group.id, + _is_active(tag_map), + tag.id == tag_map.tag_id, + tag.deleted_at.is_(None), + tag.enabled.is_(True), + or_( + and_( + RoleRequest.request_ownership.is_(True), + tag.constraints[Tag.DISALLOW_SELF_ADD_OWNERSHIP_CONSTRAINT_KEY].as_boolean().is_(True), + ), + and_( + RoleRequest.request_ownership.is_(False), + tag.constraints[Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY].as_boolean().is_(True), + ), + ), + ), + ) + conditions.append(not_(blocked_by_tag)) + return and_(*conditions) + + +def is_assigned_reviewer(request_cls: RequestModel, reviewer_id: ReviewerId) -> ColumnElement[bool]: + """SQL condition: the reviewer is an assigned reviewer of the request row. + + The condition refers to `request_cls`, so use it in a query that selects + from that model. For example: + `select(AccessRequest).where(is_assigned_reviewer(AccessRequest, user_id))`. + + Args: + request_cls: `AccessRequest`, `RoleRequest`, or `GroupRequest`. + reviewer_id: A user id, or a user-id column for a correlated query. + + Returns: + True for rows where the reviewer is eligible at some owner level and + no nearer level has an eligible reviewer. + """ + clauses: list[ColumnElement[bool]] = [] + no_one_nearer: list[ColumnElement[bool]] = [] + for _level, ownership_condition in _owner_levels(request_cls): + clauses.append(and_(_is_eligible(request_cls, ownership_condition, reviewer_id), *no_one_nearer)) + candidate = aliased(OktaUser) + no_one_nearer.append(not_(_exists((candidate,), _is_eligible(request_cls, ownership_condition, candidate.id)))) + return or_(*clauses) + + +async def get_eligible_reviewers_by_level(request: AnyRequest) -> list[OwnerLevelReviewers]: + """Return the request's eligible reviewers at each owner level, nearest first. + + Each user is listed once, at the nearest level where they are eligible. + Levels with no eligible reviewer are left out. So the first entry, if + any, holds the assigned reviewers. The rest show a requester who to + contact to escalate. + + Args: + request: A persisted access, role, or group request. + + Returns: + One entry per level that has an eligible reviewer, nearest first. + Empty when no one is eligible. + """ + request_cls = type(request) + levels: list[OwnerLevelReviewers] = [] + listed: set[str] = set() + for level, ownership_condition in _owner_levels(request_cls): + stmt = ( + select(OktaUser) + .where(OktaUser.deleted_at.is_(None)) + .where( + _exists( + (request_cls,), + request_cls.id == request.id, + _is_eligible(request_cls, ownership_condition, OktaUser.id), + ) + ) + .order_by(OktaUser.email) + ) + reviewers = [u for u in await db.session.scalars(stmt) if u.id not in listed] + if reviewers: + listed.update(u.id for u in reviewers) + levels.append(OwnerLevelReviewers(level, reviewers)) + return levels + + +async def get_assigned_reviewers(request: AnyRequest) -> list[OktaUser]: + """Return the request's assigned reviewers. + + These are the eligible reviewers at the nearest owner level that has + any. A user is in the result exactly when `is_assigned_reviewer` holds + for them and this request. + + Args: + request: A persisted access, role, or group request. + + Returns: + The assigned reviewers, ordered by email. Empty when no one is eligible. + """ + levels = await get_eligible_reviewers_by_level(request) + return levels[0].reviewers if levels else [] + + +async def snapshot_assigned_reviewers(requests: Iterable[AnyRequest]) -> dict[str, list[OktaUser]]: + """Return each request's assigned reviewers, keyed by request id. + + A request's close notification goes to the reviewers assigned while it + was open. Some operations close requests as a side effect of changing + ownership: adding users or roles to a group, or deleting or unmanaging a + group. The change can move who is assigned. A role granted ownership of + a group makes its members owners of that group, and deleting a group + ends its ownerships. So these operations call this before the change. A + request the snapshot misses, such as one opened after it, has its + reviewers computed when it closes. + + Args: + requests: Persisted access, role, or group requests. + + Returns: + Request id to that request's assigned reviewers. + """ + return {request.id: await get_assigned_reviewers(request) for request in requests} diff --git a/api/operations/_obsolete_requests.py b/api/operations/_obsolete_requests.py new file mode 100644 index 00000000..22c947f2 --- /dev/null +++ b/api/operations/_obsolete_requests.py @@ -0,0 +1,55 @@ +"""The pending requests a group's deletion or unmanaging closes. + +`DeleteGroup` and `UnmanageGroup` select these twice: once before they change +the group's ownership, for `snapshot_assigned_reviewers`, and again when +rejecting. Both selections go through here so the snapshot covers exactly the +requests that are later rejected. +""" + +from typing import Sequence + +from sqlalchemy import or_, select + +from api.extensions import db +from api.models import AccessRequest, AccessRequestStatus, OktaUser, RoleRequest +from api.models.request_reviewers import snapshot_assigned_reviewers + + +async def pending_access_requests_for_group(group_id: str) -> Sequence[AccessRequest]: + """Return the pending access requests for the group `group_id`.""" + return ( + await db.session.scalars( + select(AccessRequest) + .where(AccessRequest.requested_group_id == group_id) + .where(AccessRequest.status == AccessRequestStatus.PENDING) + .where(AccessRequest.resolved_at.is_(None)) + ) + ).all() + + +async def pending_role_requests_for_group(group_id: str) -> Sequence[RoleRequest]: + """Return the pending role requests naming `group_id` as the requested group or the requesting role.""" + return ( + await db.session.scalars( + select(RoleRequest) + .where(or_(RoleRequest.requested_group_id == group_id, RoleRequest.requester_role_id == group_id)) + .where(RoleRequest.status == AccessRequestStatus.PENDING) + .where(RoleRequest.resolved_at.is_(None)) + ) + ).all() + + +async def snapshot_obsolete_request_reviewers( + group_id: str, +) -> tuple[dict[str, list[OktaUser]], dict[str, list[OktaUser]]]: + """Return the assigned reviewers of the group's pending access and role requests, keyed by request id. + + Args: + group_id: The group about to be deleted or unmanaged. + + Returns: + A pair of request-id-to-reviewers maps: access requests, then role requests. + """ + access_reviewers = await snapshot_assigned_reviewers(await pending_access_requests_for_group(group_id)) + role_reviewers = await snapshot_assigned_reviewers(await pending_role_requests_for_group(group_id)) + return access_reviewers, role_reviewers diff --git a/api/operations/approve_group_request.py b/api/operations/approve_group_request.py index 04257592..28133f26 100644 --- a/api/operations/approve_group_request.py +++ b/api/operations/approve_group_request.py @@ -21,7 +21,7 @@ RoleGroup, Tag, ) -from api.models.access_request import get_all_possible_request_approvers +from api.models.request_reviewers import get_assigned_reviewers from api.models.app_group import get_access_owners from api.models.okta_group import is_group_name_taken from api.models.tag import effective_ended_at @@ -360,7 +360,9 @@ async def execute(self) -> Optional[GroupRequest]: if self.notify: requester = await db.session.get(OktaUser, group_request.requester_user_id) - approvers = await get_all_possible_request_approvers(group_request) + # Same as before the commit: a group request's levels are the requested + # app's owners and Access admins, which creating the group leaves unchanged. + approvers = await get_assigned_reviewers(group_request) await defer_notification( db.session, NotificationHook.ACCESS_GROUP_REQUEST_COMPLETED, diff --git a/api/operations/create_access_request.py b/api/operations/create_access_request.py index 7ff2a454..4cdd7c00 100644 --- a/api/operations/create_access_request.py +++ b/api/operations/create_access_request.py @@ -19,8 +19,7 @@ OktaUser, RoleGroup, ) -from api.models.app_group import get_access_owners, get_app_managers -from api.models.okta_group import get_group_managers +from api.models.request_reviewers import get_assigned_reviewers from api.operations.approve_access_request import ApproveAccessRequest from api.operations.reject_access_request import RejectAccessRequest from api.operations._fan_out import defer_notification @@ -80,21 +79,7 @@ async def execute(self) -> Optional[AccessRequest]: db.session.add(access_request) await db.session.commit() - # Fetch the users to notify - approvers = await get_group_managers(requested_group.id) - - # If there are no approvers, try to get the app managers - # or if the only approver is the requester, try to get the app managers - if ( - (len(approvers) == 0 and type(requested_group) is AppGroup) - or (len(approvers) == 1 and approvers[0].id == requester.id) - and type(requested_group) is AppGroup - ): - approvers = await get_app_managers(requested_group.app_id) - - # If there are still no approvers, try to get the access owners - if len(approvers) == 0 or (len(approvers) == 1 and approvers[0].id == requester.id): - approvers = await get_access_owners() + approvers = await get_assigned_reviewers(access_request) group = ( await db.session.scalars( diff --git a/api/operations/create_group_request.py b/api/operations/create_group_request.py index 8d8a4c69..ccb07920 100644 --- a/api/operations/create_group_request.py +++ b/api/operations/create_group_request.py @@ -19,8 +19,9 @@ RoleGroup, Tag, ) -from api.models.app_group import get_access_owners, get_app_managers +from api.models.app_group import get_app_managers from api.models.okta_group import is_group_name_taken +from api.models.request_reviewers import get_assigned_reviewers from api.models.tag import coalesce_ended_at from api.operations.approve_group_request import ApproveGroupRequest from api.operations.reject_group_request import RejectGroupRequest @@ -166,12 +167,7 @@ async def execute(self) -> Optional[GroupRequest]: # already saved, so route it to reviewers like any other. logger.warning("App owner auto-approval of group request %s failed: %s", group_request.id, e) - # Fetch the users to notify - # If app group, notify app managers; otherwise notify access owners - if self.requested_app_id is not None: - approvers = await get_app_managers(self.requested_app_id) - else: - approvers = await get_access_owners() + approvers = await get_assigned_reviewers(group_request) # Audit logging _ctx = get_request_context() diff --git a/api/operations/create_role_request.py b/api/operations/create_role_request.py index 5a021f2b..60c1f550 100644 --- a/api/operations/create_role_request.py +++ b/api/operations/create_role_request.py @@ -5,7 +5,7 @@ import logging -from sqlalchemy import func, or_, select +from sqlalchemy import select from api.context import get_request_context from sqlalchemy.orm import joinedload, selectin_polymorphic, selectinload @@ -16,14 +16,10 @@ OktaGroup, OktaGroupTagMap, OktaUser, - OktaUserGroupMember, RoleGroup, RoleRequest, - Tag, ) -from api.models.app_group import get_access_owners, get_app_managers -from api.models.okta_group import get_group_managers -from api.models.tag import effective_constraint +from api.models.request_reviewers import get_assigned_reviewers from api.operations.approve_role_request import ApproveRoleRequest from api.operations.reject_role_request import RejectRoleRequest from api.operations._fan_out import defer_notification @@ -99,52 +95,7 @@ async def execute(self) -> Optional[RoleRequest]: db.session.add(role_request) await db.session.commit() - # Fetch the users to notify - approvers = await get_group_managers(requested_group.id) - - role_memberships = [ - u.user_id - for u in ( - await db.session.scalars( - select(OktaUserGroupMember) - .where(OktaUserGroupMember.group_id == requester_role.id) - .where(OktaUserGroupMember.is_owner.is_(False)) - .where( - or_( - OktaUserGroupMember.ended_at.is_(None), - OktaUserGroupMember.ended_at > func.now(), - ) - ) - ) - ).all() - ] - - # If group tagged with disallow self add constraint, filter out approvers who are also members of the role - if self.request_ownership: - disallow_self_add_owner = effective_constraint( - Tag.DISALLOW_SELF_ADD_OWNERSHIP_CONSTRAINT_KEY, requested_group - ) - if disallow_self_add_owner: - approvers = [a for a in approvers if a.id not in role_memberships] - else: - disallow_self_add_member = effective_constraint( - Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY, requested_group - ) - if disallow_self_add_member: - approvers = [a for a in approvers if a.id not in role_memberships] - - # If there are no approvers, try to get the app managers - # or if the only approver is the requester, try to get the app managers - if ( - (len(approvers) == 0 and type(requested_group) is AppGroup) - or (len(approvers) == 1 and approvers[0].id == requester.id) - and type(requested_group) is AppGroup - ): - approvers = await get_app_managers(requested_group.app_id) - - # If there are still no approvers, try to get the access owners - if len(approvers) == 0 or (len(approvers) == 1 and approvers[0].id == requester.id): - approvers = await get_access_owners() + approvers = await get_assigned_reviewers(role_request) group = ( await db.session.scalars( diff --git a/api/operations/delete_app.py b/api/operations/delete_app.py index 7ef94ee9..c1838a35 100644 --- a/api/operations/delete_app.py +++ b/api/operations/delete_app.py @@ -59,10 +59,16 @@ async def execute(self) -> None: app.deleted_at = func.now() await db.session.commit() - # Delete all associated Okta App Groups and end their membership + # Delete all associated Okta App Groups and end their membership. The + # owners group goes last: its owners are the app owners, whom requests + # on the other groups are assigned to, and each DeleteGroup captures + # its requests' assigned reviewers before deleting. app_groups = ( await db.session.scalars( - select(AppGroup).where(AppGroup.deleted_at.is_(None)).where(AppGroup.app_id == app.id) + select(AppGroup) + .where(AppGroup.deleted_at.is_(None)) + .where(AppGroup.app_id == app.id) + .order_by(AppGroup.is_owner) ) ).all() app_group_ids = [ag.id for ag in app_groups] diff --git a/api/operations/delete_group.py b/api/operations/delete_group.py index beff9cbd..b91ba97a 100644 --- a/api/operations/delete_group.py +++ b/api/operations/delete_group.py @@ -10,8 +10,6 @@ from api.extensions import db from api.operations._fan_out import defer_or_drain_fan_out from api.models import ( - AccessRequest, - AccessRequestStatus, App, AppGroup, OktaGroup, @@ -20,7 +18,11 @@ OktaUserGroupMember, RoleGroup, RoleGroupMap, - RoleRequest, +) +from api.operations._obsolete_requests import ( + pending_access_requests_for_group, + pending_role_requests_for_group, + snapshot_obsolete_request_reviewers, ) from api.operations.reject_access_request import RejectAccessRequest from api.operations.reject_role_request import RejectRoleRequest @@ -92,6 +94,9 @@ async def execute(self) -> None: ) ) + # Capture assigned reviewers before deleting changes ownership; see `snapshot_assigned_reviewers`. + access_reviewers, role_reviewers = await snapshot_obsolete_request_reviewers(group.id) + if self.sync_to_okta: okta_tasks.append(asyncio.create_task(okta.delete_group(group.id))) @@ -270,41 +275,22 @@ async def execute(self) -> None: await db.session.commit() # Reject all pending access requests for this group - obsolete_access_requests = ( - await db.session.scalars( - select(AccessRequest) - .where(AccessRequest.requested_group_id == group.id) - .where(AccessRequest.status == AccessRequestStatus.PENDING) - .where(AccessRequest.resolved_at.is_(None)) - ) - ).all() - for obsolete_access_request in obsolete_access_requests: + for obsolete_access_request in await pending_access_requests_for_group(group.id): await RejectAccessRequest( access_request=obsolete_access_request, rejection_reason="Closed because the requested group was deleted", current_user_id=current_user_id, + assigned_reviewers=access_reviewers.get(obsolete_access_request.id), ).execute() # Reject all pending role requests touching this group, either as the # requested target or as the requester role. - obsolete_role_requests = ( - await db.session.scalars( - select(RoleRequest) - .where( - or_( - RoleRequest.requested_group_id == group.id, - RoleRequest.requester_role_id == group.id, - ) - ) - .where(RoleRequest.status == AccessRequestStatus.PENDING) - .where(RoleRequest.resolved_at.is_(None)) - ) - ).all() - for obsolete_role_request in obsolete_role_requests: + for obsolete_role_request in await pending_role_requests_for_group(group.id): await RejectRoleRequest( role_request=obsolete_role_request, rejection_reason="Closed because a group in this role request was deleted", current_user_id=current_user_id, + assigned_reviewers=role_reviewers.get(obsolete_role_request.id), ).execute() # End all tag mappings for this group diff --git a/api/operations/modify_group_users.py b/api/operations/modify_group_users.py index 6ab66ed9..f8e53076 100644 --- a/api/operations/modify_group_users.py +++ b/api/operations/modify_group_users.py @@ -22,7 +22,7 @@ RoleGroupMap, Tag, ) -from api.models.access_request import get_all_possible_request_approvers +from api.models.request_reviewers import get_assigned_reviewers, snapshot_assigned_reviewers from api.models.tag import effective_ended_at from api.routers._eager import effective_constraint_options from api.operations.constraints import CheckForReason, CheckForSelfAdd @@ -248,6 +248,31 @@ async def execute(self) -> OktaGroup: # Access requests approved by this operation; their notifications are # prepared after the final commit and dispatched alongside async_tasks. approved_access_requests: list[AccessRequest] = [] + # Capture assigned reviewers before ownership changes; see `snapshot_assigned_reviewers`. + # The requests it can fulfil come from a user it adds and target this + # group or, for a role, a group the role is associated with. + assigned_before_change: dict[str, list[OktaUser]] = {} + if self.notify: + added_user_ids = {user.id for user in members_to_add} | {user.id for user in owners_to_add} + fulfillable_group_ids = {group.id} + if type(group) is RoleGroup: + fulfillable_group_ids |= { + mapping.group_id + for mapping in ( + *group.active_role_associated_group_member_mappings, + *group.active_role_associated_group_owner_mappings, + ) + } + candidates = ( + await db.session.scalars( + select(AccessRequest) + .where(AccessRequest.status == AccessRequestStatus.PENDING) + .where(AccessRequest.resolved_at.is_(None)) + .where(AccessRequest.requested_group_id.in_(fulfillable_group_ids)) + .where(AccessRequest.requester_user_id.in_(added_user_ids)) + ) + ).all() + assigned_before_change = await snapshot_assigned_reviewers(candidates) # First remove all users from the group including those that we wish to add. # That way we can easily extend time-bounded group memberships and audit when @@ -779,7 +804,9 @@ async def execute(self) -> OktaGroup: # reload it explicitly so the hook sees a concrete value. await db.session.refresh(access_request, attribute_names=["resolved_at"]) requester = await db.session.get(OktaUser, access_request.requester_user_id) - approvers = await get_all_possible_request_approvers(access_request) + approvers = assigned_before_change.get(access_request.id) + if approvers is None: + approvers = await get_assigned_reviewers(access_request) # Spawn the notify task and drain it in the same batch as the Okta # tasks below, so on the inline path the notifications run # concurrently (not one-awaited-at-a-time). prepare_notification_task diff --git a/api/operations/modify_role_groups.py b/api/operations/modify_role_groups.py index 160ce879..a2fed446 100644 --- a/api/operations/modify_role_groups.py +++ b/api/operations/modify_role_groups.py @@ -24,7 +24,7 @@ RoleRequest, Tag, ) -from api.models.access_request import get_all_possible_request_approvers +from api.models.request_reviewers import get_assigned_reviewers, snapshot_assigned_reviewers from api.models.tag import effective_ended_at from api.operations.constraints import CheckForReason, CheckForSelfAdd from api.operations._time_limits import limit_access_conferred_by_roles, propagating_seconds_limit @@ -246,6 +246,41 @@ async def execute(self) -> RoleGroup: # prepared after the final commit and dispatched alongside async_tasks. approved_access_requests: list[AccessRequest] = [] approved_role_requests: list[RoleRequest] = [] + # Capture assigned reviewers before ownership changes; see `snapshot_assigned_reviewers`. + # The requests it can fulfil target a group it adds the role to: a role + # request from this role, or an access request from a role member. + assigned_before_change: dict[str, list[OktaUser]] = {} + if self.notify: + added_group_ids = {g.id for g in groups_to_add} | {g.id for g in owner_groups_to_add} + role_member_ids = ( + await db.session.scalars( + select(OktaUserGroupMember.user_id) + .where(OktaUserGroupMember.group_id == self.role.id) + .where(OktaUserGroupMember.is_owner.is_(False)) + .where(or_(OktaUserGroupMember.ended_at.is_(None), OktaUserGroupMember.ended_at > func.now())) + ) + ).all() + candidate_access_requests = ( + await db.session.scalars( + select(AccessRequest) + .where(AccessRequest.status == AccessRequestStatus.PENDING) + .where(AccessRequest.resolved_at.is_(None)) + .where(AccessRequest.requested_group_id.in_(added_group_ids)) + .where(AccessRequest.requester_user_id.in_(role_member_ids)) + ) + ).all() + candidate_role_requests = ( + await db.session.scalars( + select(RoleRequest) + .where(RoleRequest.status == AccessRequestStatus.PENDING) + .where(RoleRequest.resolved_at.is_(None)) + .where(RoleRequest.requester_role_id == self.role.id) + .where(RoleRequest.requested_group_id.in_(added_group_ids)) + ) + ).all() + assigned_before_change = await snapshot_assigned_reviewers( + [*candidate_access_requests, *candidate_role_requests] + ) # App groups whose membership changed, and who gained or lost it. Accumulated during the # loops below and handed to the lifecycle hooks after the commit that makes each change # durable -- see the comments at the collection sites. @@ -691,7 +726,9 @@ async def execute(self) -> RoleGroup: # reload it explicitly so the hook sees a concrete value. await db.session.refresh(access_request, attribute_names=["resolved_at"]) requester = await db.session.get(OktaUser, access_request.requester_user_id) - approvers = await get_all_possible_request_approvers(access_request) + approvers = assigned_before_change.get(access_request.id) + if approvers is None: + approvers = await get_assigned_reviewers(access_request) # Spawn + drain in one batch with the Okta tasks below, so on the # inline path notifications run concurrently. prepare_notification_task # expunges the payload so the async hook can read it after the @@ -714,7 +751,9 @@ async def execute(self) -> RoleGroup: group = role_request.requested_group await db.session.refresh(role_request, attribute_names=["resolved_at"]) requester = await db.session.get(OktaUser, role_request.requester_user_id) - approvers = await get_all_possible_request_approvers(role_request) + approvers = assigned_before_change.get(role_request.id) + if approvers is None: + approvers = await get_assigned_reviewers(role_request) async_tasks.append( prepare_notification_task( db.session, diff --git a/api/operations/reject_access_request.py b/api/operations/reject_access_request.py index 0522b3d7..149f3e65 100644 --- a/api/operations/reject_access_request.py +++ b/api/operations/reject_access_request.py @@ -9,7 +9,7 @@ from api.exceptions import ConflictError from api.extensions import db from api.models import AccessRequest, AccessRequestStatus, AppGroup, OktaGroup, OktaUser, RoleGroup -from api.models.access_request import get_all_possible_request_approvers +from api.models.request_reviewers import get_assigned_reviewers from api.operations._fan_out import defer_notification from api.plugins import NotificationHook from api.schemas import AuditLogSchema, EventType @@ -24,7 +24,24 @@ def __init__( notify: bool = True, notify_requester: bool = True, current_user_id: Optional[str | OktaUser] = None, + assigned_reviewers: Optional[list[OktaUser]] = None, ): + """Reject a pending access request. + + Args: + access_request: The request, or its id. + rejection_reason: Recorded as the request's resolution reason. + notify: Send the close notification. + notify_requester: Include the requester in the close notification. + current_user_id: The rejecting user, or their id; None for a system rejection. + assigned_reviewers: The request's assigned reviewers from + `snapshot_assigned_reviewers`, for a caller that changes + ownership before rejecting. When None, they are computed at + rejection time. + + Raises: + ConflictError: From `execute`, when the request is no longer pending. + """ self.access_request_id = access_request if isinstance(access_request, str) else access_request.id self.current_user_id = ( current_user_id.id @@ -35,6 +52,7 @@ def __init__( self.rejection_reason = rejection_reason self.notify = notify self.notify_requester = notify_requester + self.assigned_reviewers = assigned_reviewers async def execute(self) -> AccessRequest: # Lock the request row so a reject can't race a concurrent approve/ @@ -107,7 +125,11 @@ async def execute(self) -> AccessRequest: if self.notify: requester = await db.session.get(OktaUser, access_request.requester_user_id) - approvers = await get_all_possible_request_approvers(access_request) + approvers = ( + self.assigned_reviewers + if self.assigned_reviewers is not None + else await get_assigned_reviewers(access_request) + ) await defer_notification( db.session, diff --git a/api/operations/reject_group_request.py b/api/operations/reject_group_request.py index 45833469..f0dde7ff 100644 --- a/api/operations/reject_group_request.py +++ b/api/operations/reject_group_request.py @@ -8,7 +8,7 @@ from api.exceptions import ConflictError from api.extensions import db from api.models import AccessRequestStatus, AppGroup, GroupRequest, OktaUser, OktaUserGroupMember -from api.models.access_request import get_all_possible_request_approvers +from api.models.request_reviewers import get_assigned_reviewers from api.models.app_group import get_access_owners from api.operations._fan_out import defer_notification from api.plugins import NotificationHook @@ -127,7 +127,7 @@ async def execute(self) -> GroupRequest: if self.notify: requester = await db.session.get(OktaUser, group_request.requester_user_id) - approvers = await get_all_possible_request_approvers(group_request) + approvers = await get_assigned_reviewers(group_request) await defer_notification( db.session, diff --git a/api/operations/reject_role_request.py b/api/operations/reject_role_request.py index a325207b..c53aa8a0 100644 --- a/api/operations/reject_role_request.py +++ b/api/operations/reject_role_request.py @@ -9,7 +9,7 @@ from api.exceptions import ConflictError from api.extensions import db from api.models import AccessRequestStatus, AppGroup, OktaGroup, OktaUser, RoleRequest -from api.models.access_request import get_all_possible_request_approvers +from api.models.request_reviewers import get_assigned_reviewers from api.operations._fan_out import defer_notification from api.plugins import NotificationHook from api.schemas import AuditLogSchema, EventType @@ -24,7 +24,24 @@ def __init__( notify: bool = True, notify_requester: bool = True, current_user_id: Optional[str | OktaUser] = None, + assigned_reviewers: Optional[list[OktaUser]] = None, ): + """Reject a pending role request. + + Args: + role_request: The request, or its id. + rejection_reason: Recorded as the request's resolution reason. + notify: Send the close notification. + notify_requester: Include the requester in the close notification. + current_user_id: The rejecting user, or their id; None for a system rejection. + assigned_reviewers: The request's assigned reviewers from + `snapshot_assigned_reviewers`, for a caller that changes + ownership before rejecting. When None, they are computed at + rejection time. + + Raises: + ConflictError: From `execute`, when the request is no longer pending. + """ self.role_request_id = role_request if isinstance(role_request, str) else role_request.id self.current_user_id = ( current_user_id.id @@ -35,6 +52,7 @@ def __init__( self.rejection_reason = rejection_reason self.notify = notify self.notify_requester = notify_requester + self.assigned_reviewers = assigned_reviewers async def execute(self) -> RoleRequest: # Lock the request row so a reject can't race a concurrent approve/ @@ -117,7 +135,11 @@ async def execute(self) -> RoleRequest: requester = await db.session.get(OktaUser, role_request.requester_user_id) requester_role = await db.session.get(OktaGroup, role_request.requester_role_id) - approvers = await get_all_possible_request_approvers(role_request) + approvers = ( + self.assigned_reviewers + if self.assigned_reviewers is not None + else await get_assigned_reviewers(role_request) + ) await defer_notification( db.session, diff --git a/api/operations/unmanage_group.py b/api/operations/unmanage_group.py index 0d63bafa..1fea063a 100644 --- a/api/operations/unmanage_group.py +++ b/api/operations/unmanage_group.py @@ -6,15 +6,17 @@ from sqlalchemy import func, or_, select, update from api.extensions import db from api.models import ( - AccessRequest, - AccessRequestStatus, AppGroup, OktaGroup, OktaUser, OktaUserGroupMember, RoleGroup, RoleGroupMap, - RoleRequest, +) +from api.operations._obsolete_requests import ( + pending_access_requests_for_group, + pending_role_requests_for_group, + snapshot_obsolete_request_reviewers, ) from api.operations.reject_access_request import RejectAccessRequest from api.operations.reject_role_request import RejectRoleRequest @@ -51,6 +53,12 @@ async def execute(self, dry_run: bool = False) -> None: if group.is_managed: return + # Capture assigned reviewers before unmanaging changes ownership; see `snapshot_assigned_reviewers`. + access_reviewers: dict[str, list[OktaUser]] = {} + role_reviewers: dict[str, list[OktaUser]] = {} + if not dry_run: + access_reviewers, role_reviewers = await snapshot_obsolete_request_reviewers(group.id) + # TODO: End all direct ownerships of the group? # End all group memberships and ownerships via a role (not direct memberships or ownerships) @@ -117,15 +125,7 @@ async def execute(self, dry_run: bool = False) -> None: # Unmanaged roles can still be associated with groups # Reject all pending access requests for this group - obsolete_access_requests = ( - await db.session.scalars( - select(AccessRequest) - .where(AccessRequest.requested_group_id == group.id) - .where(AccessRequest.status == AccessRequestStatus.PENDING) - .where(AccessRequest.resolved_at.is_(None)) - ) - ).all() - for obsolete_access_request in obsolete_access_requests: + for obsolete_access_request in await pending_access_requests_for_group(group.id): logger.info( f"Rejecting obsolete access request {obsolete_access_request.id} for unmanaged group {group.id}" ) @@ -134,28 +134,17 @@ async def execute(self, dry_run: bool = False) -> None: access_request=obsolete_access_request, rejection_reason="Closed because the requested group is no longer managed by Access", current_user_id=current_user_id, + assigned_reviewers=access_reviewers.get(obsolete_access_request.id), ).execute() # Reject all pending role requests touching this group, either as the # requested target or as the requester role. - obsolete_role_requests = ( - await db.session.scalars( - select(RoleRequest) - .where( - or_( - RoleRequest.requested_group_id == group.id, - RoleRequest.requester_role_id == group.id, - ) - ) - .where(RoleRequest.status == AccessRequestStatus.PENDING) - .where(RoleRequest.resolved_at.is_(None)) - ) - ).all() - for obsolete_role_request in obsolete_role_requests: + for obsolete_role_request in await pending_role_requests_for_group(group.id): logger.info(f"Rejecting obsolete role request {obsolete_role_request.id} for unmanaged group {group.id}") if not dry_run: await RejectRoleRequest( role_request=obsolete_role_request, rejection_reason="Closed because a group in this role request is no longer managed by Access", current_user_id=current_user_id, + assigned_reviewers=role_reviewers.get(obsolete_role_request.id), ).execute() diff --git a/api/plugins/notifications.py b/api/plugins/notifications.py index d4d6024c..d2cb4430 100644 --- a/api/plugins/notifications.py +++ b/api/plugins/notifications.py @@ -86,6 +86,17 @@ class NotificationPluginSpec: object graph, that graph must be eager-loaded by the operation before dispatch — open an issue. + `approvers` is the request's assigned reviewers in both the `*_created` and + the `*_completed` hooks: the eligible reviewers at the nearest owner level + that has any. It never includes the requester. A `*_completed` hook + receives the reviewers assigned just before whatever closed the request, + with two exceptions: a request opened while an ownership change was under + way gets the reviewers assigned after it, and a role request closed by + unmanaging its group can include owners a `disallow_self_add_*` tag would + exclude, because the group is no longer managed by then. Either way they + may differ from who was notified at creation, so a plugin that updates + messages it sent should track those messages by request id. + (The `access_expiring_*` hooks are the exception: they fire from the syncer CronJob, not an HTTP request, and run inline on its session.) """ diff --git a/api/routers/_reviewers.py b/api/routers/_reviewers.py new file mode 100644 index 00000000..1f592f50 --- /dev/null +++ b/api/routers/_reviewers.py @@ -0,0 +1,18 @@ +"""Builds the `/reviewers` response shared by the three request routers.""" + +from api.models.request_reviewers import AnyRequest, get_eligible_reviewers_by_level +from api.schemas import OktaUserSummary, RequestReviewerLevel, RequestReviewers + + +async def request_reviewers_response(request: AnyRequest) -> RequestReviewers: + """The request's eligible reviewers by level, nearest first; see `RequestReviewers`.""" + levels = await get_eligible_reviewers_by_level(request) + return RequestReviewers( + reviewers_by_level=[ + RequestReviewerLevel( + owner_level=level.owner_level, + reviewers=[OktaUserSummary.model_validate(u, from_attributes=True) for u in level.reviewers], + ) + for level in levels + ], + ) diff --git a/api/routers/access_requests.py b/api/routers/access_requests.py index 35dbc597..d7e8a417 100644 --- a/api/routers/access_requests.py +++ b/api/routers/access_requests.py @@ -15,23 +15,24 @@ from api.models import ( AccessRequest, AccessRequestStatus, - App, AppGroup, OktaGroup, OktaUser, - OktaUserGroupMember, RoleGroup, ) +from api.models.request_reviewers import is_assigned_reviewer from api.operations import ApproveAccessRequest, CreateAccessRequest, RejectAccessRequest from fastapi_pagination.ext.sqlalchemy import apaginate from api.pagination import Page, validated from api.routers._eager import group_tag_map_options, role_group_map_options from api.routers._fan_out import defer_fan_out +from api.routers._reviewers import request_reviewers_response from api.schemas import ( AccessRequestDetail, AccessRequestSummary, CreateAccessRequestBody, + RequestReviewers, ResolveAccessRequestBody, SearchAccessRequestQuery, ) @@ -129,6 +130,7 @@ async def list_access_requests( ) if q_args.assignee_user_id: + # Requests whose assigned reviewers include the assignee. assignee_user_id = current_user_id if q_args.assignee_user_id == "@me" else q_args.assignee_user_id assignee_user = ( await db.scalars( @@ -136,33 +138,7 @@ async def list_access_requests( ) ).first() if assignee_user is not None: - groups_owned_subquery = ( - select(OktaGroup.id) - .options(selectinload(OktaGroup.active_user_ownerships)) - .join(OktaGroup.active_user_ownerships) - .where(OktaGroup.deleted_at.is_(None)) - .where(OktaUserGroupMember.user_id == assignee_user.id) - ) - owner_app_group_alias = aliased(AppGroup) - app_groups_owned_subquery = ( - select(AppGroup.id) - .options( - joinedload(AppGroup.app) - .joinedload(App.active_owner_app_groups.of_type(owner_app_group_alias)) - .selectinload(owner_app_group_alias.active_user_ownerships) - ) - .join(AppGroup.app) - .join(App.active_owner_app_groups.of_type(owner_app_group_alias)) - .join(owner_app_group_alias.active_user_ownerships) - .where(AppGroup.deleted_at.is_(None)) - .where(OktaUserGroupMember.user_id == assignee_user.id) - ) - stmt = stmt.join(AccessRequest.requested_group).where( - or_( - OktaGroup.id.in_(groups_owned_subquery), - OktaGroup.id.in_(app_groups_owned_subquery), - ) - ) + stmt = stmt.where(is_assigned_reviewer(AccessRequest, assignee_user.id)) else: stmt = stmt.where(false()) @@ -224,6 +200,17 @@ async def get_access_request( return AccessRequestDetail.model_validate(ar, from_attributes=True) +@router.get("/{access_request_id}/reviewers", name="access_request_reviewers") +async def get_access_request_reviewers( + access_request_id: str, db: DbSession, current_user_id: CurrentUserId +) -> RequestReviewers: + """Eligible reviewers of an access request by owner level, nearest first; the first level is assigned.""" + request = await db.get(AccessRequest, access_request_id) + if request is None: + raise HTTPException(404, "Not Found") + return await request_reviewers_response(request) + + @router.post("", name="access_requests_create", status_code=201) async def post_access_request( body: CreateAccessRequestBody, diff --git a/api/routers/group_requests.py b/api/routers/group_requests.py index 2a2c252b..a015f167 100644 --- a/api/routers/group_requests.py +++ b/api/routers/group_requests.py @@ -6,13 +6,14 @@ from fastapi import APIRouter, Depends, HTTPException, Query from fastapi_pagination.ext.sqlalchemy import apaginate -from sqlalchemy import String, and_, cast, false, or_, select +from sqlalchemy import String, cast, false, or_, select from sqlalchemy.orm import aliased, joinedload from starlette.requests import Request from api.auth.dependencies import CurrentUserId from api.database import DbSession from api.models import AccessRequestStatus, App, GroupRequest, OktaUser, Tag +from api.models.request_reviewers import is_assigned_reviewer from api.operations import ApproveGroupRequest, CreateGroupRequest, RejectGroupRequest from api.pagination import Page, validated from api.plugins.app_group_lifecycle import ( @@ -21,9 +22,11 @@ validate_group_plugin_config_or_raise, ) from api.routers._fan_out import defer_fan_out +from api.routers._reviewers import request_reviewers_response from api.schemas import ( CreateGroupRequestBody, GroupRequestDetail, + RequestReviewers, ResolveGroupRequestBody, SearchGroupRequestQuery, ) @@ -52,9 +55,6 @@ async def list_group_requests( current_user_id: CurrentUserId, q_args: Annotated[SearchGroupRequestQuery, Query()], ) -> Page[GroupRequestDetail]: - from api.auth.permissions import is_access_admin - from api.models.app_group import get_app_managers - stmt = select(GroupRequest).options(*_load_options()).order_by(GroupRequest.created_at.desc()) if q_args.status: @@ -79,9 +79,7 @@ async def list_group_requests( stmt = stmt.where(GroupRequest.requested_app_id == q_args.requested_app_id) if q_args.assignee_user_id: - # "Requests I can resolve". Admins see every pending request; app - # owners see app-group requests for apps they own. In both cases the - # assignee's own requests are stripped out. + # Requests whose assigned reviewers include the assignee. assignee_user_id = current_user_id if q_args.assignee_user_id == "@me" else q_args.assignee_user_id assignee_user = ( await db.scalars( @@ -89,22 +87,7 @@ async def list_group_requests( ) ).first() if assignee_user is not None: - if not await is_access_admin(db, assignee_user.id): - owned_app_ids: list[str] = [] - for app in (await db.scalars(select(App).where(App.deleted_at.is_(None)))).all(): - manager_ids = [m.id for m in await get_app_managers(app.id)] - if assignee_user.id in manager_ids: - owned_app_ids.append(app.id) - if owned_app_ids: - stmt = stmt.where( - and_( - GroupRequest.requested_app_id.in_(owned_app_ids), - GroupRequest.requested_group_type == "app_group", - ) - ) - else: - stmt = stmt.where(false()) - stmt = stmt.where(GroupRequest.requester_user_id != assignee_user.id) + stmt = stmt.where(is_assigned_reviewer(GroupRequest, assignee_user.id)) else: stmt = stmt.where(false()) @@ -166,6 +149,17 @@ async def get_group_request(group_request_id: str, db: DbSession, current_user_i return GroupRequestDetail.model_validate(gr, from_attributes=True) +@router.get("/{group_request_id}/reviewers", name="group_request_reviewers") +async def get_group_request_reviewers( + group_request_id: str, db: DbSession, current_user_id: CurrentUserId +) -> RequestReviewers: + """Eligible reviewers of a group request by owner level, nearest first; the first level is assigned.""" + request = await db.get(GroupRequest, group_request_id) + if request is None: + raise HTTPException(404, "Not Found") + return await request_reviewers_response(request) + + @router.post("", name="group_requests_create", status_code=201) async def post_group_request( body: CreateGroupRequestBody, diff --git a/api/routers/role_requests.py b/api/routers/role_requests.py index 3630adb2..8242e69a 100644 --- a/api/routers/role_requests.py +++ b/api/routers/role_requests.py @@ -6,37 +6,34 @@ from fastapi import APIRouter, Depends, HTTPException, Query -from sqlalchemy import String, and_, cast, false, not_, or_, select -from sqlalchemy.orm import aliased, joinedload, selectin_polymorphic, selectinload +from sqlalchemy import String, cast, false, or_, select +from sqlalchemy.orm import aliased, joinedload, selectinload from starlette.requests import Request from api.auth.dependencies import CurrentUserId from api.database import DbSession from api.models import ( AccessRequestStatus, - App, - AppGroup, OktaGroup, OktaUser, - OktaUserGroupMember, RoleGroup, RoleRequest, - Tag, ) -from api.models.tag import effective_constraint +from api.models.request_reviewers import is_assigned_reviewer from api.operations import ApproveRoleRequest, CreateRoleRequest, RejectRoleRequest from fastapi_pagination.ext.sqlalchemy import apaginate from api.pagination import Page, validated from api.routers._eager import ( - effective_constraint_options, group_tag_map_options, polymorphic_group_options, user_group_member_options, ) from api.routers._fan_out import defer_fan_out +from api.routers._reviewers import request_reviewers_response from api.schemas import ( CreateRoleRequestBody, + RequestReviewers, ResolveRoleRequestBody, RoleRequestDetail, RoleRequestSummary, @@ -92,8 +89,6 @@ async def list_role_requests( current_user_id: CurrentUserId, q_args: Annotated[SearchRoleRequestQuery, Query()], ) -> Page[RoleRequestSummary]: - from api.auth.permissions import is_access_admin - stmt = select(RoleRequest).options(*_summary_load_options()).order_by(RoleRequest.created_at.desc()) if q_args.status: @@ -112,8 +107,7 @@ async def list_role_requests( ) # The role and the requested group both live in `okta_group`, so each - # filter joins its own alias; the `assignee_user_id` branch below joins - # the unaliased `OktaGroup` for the requested group. + # filter joins its own alias. if q_args.requester_role_id: role_alias = aliased(OktaGroup) stmt = stmt.join(RoleRequest.requester_role.of_type(role_alias)).where( @@ -133,12 +127,7 @@ async def list_role_requests( ) if q_args.assignee_user_id: - # "Requests I can resolve" — same admin/non-admin branching as Flask. - # Admins see every PENDING request whose target group has the - # `disallow_self_add_*` tag and where every existing owner is also a - # member of the requester role (otherwise approval is impossible). - # Non-admins are restricted to groups they own, and pre-filtered to - # exclude requests they can't resolve due to those same tags. + # Requests whose assigned reviewers include the assignee. assignee_user_id = current_user_id if q_args.assignee_user_id == "@me" else q_args.assignee_user_id assignee_user = ( await db.scalars( @@ -146,152 +135,7 @@ async def list_role_requests( ) ).first() if assignee_user is not None: - groups_owned_subquery = ( - select(OktaGroup.id) - .options(selectinload(OktaGroup.active_user_ownerships)) - .join(OktaGroup.active_user_ownerships) - .where(OktaGroup.deleted_at.is_(None)) - .where(OktaUserGroupMember.user_id == assignee_user.id) - ) - owner_app_group_alias = aliased(AppGroup) - app_groups_owned_subquery = ( - select(AppGroup.id) - .options( - joinedload(AppGroup.app) - .joinedload(App.active_owner_app_groups.of_type(owner_app_group_alias)) - .selectinload(owner_app_group_alias.active_user_ownerships) - ) - .join(AppGroup.app) - .join(App.active_owner_app_groups.of_type(owner_app_group_alias)) - .join(owner_app_group_alias.active_user_ownerships) - .where(AppGroup.deleted_at.is_(None)) - .where(OktaUserGroupMember.user_id == assignee_user.id) - ) - - if await is_access_admin(db, assignee_user.id): - # Pending role requests for ownership / membership where the - # target group is tagged `disallow_self_add_*` and every - # existing owner is in the requester role's membership. - tagged_owner_requests = [ - rr - for rr in ( - await db.scalars( - select(RoleRequest) - .options( - joinedload(RoleRequest.requester_role).options( - selectinload(OktaGroup.active_user_memberships) - ), - joinedload(RoleRequest.requested_group).options( - selectinload(OktaGroup.active_user_ownerships), - selectin_polymorphic(OktaGroup, [AppGroup, RoleGroup]), - *effective_constraint_options(), - ), - ) - .where(RoleRequest.status == AccessRequestStatus.PENDING) - .where(RoleRequest.request_ownership.is_(True)) - ) - ).all() - if effective_constraint(Tag.DISALLOW_SELF_ADD_OWNERSHIP_CONSTRAINT_KEY, rr.requested_group) - ] - tagged_member_requests = [ - rr - for rr in ( - await db.scalars( - select(RoleRequest) - .options( - joinedload(RoleRequest.requester_role).options( - selectinload(OktaGroup.active_user_memberships) - ), - joinedload(RoleRequest.requested_group).options( - selectinload(OktaGroup.active_user_ownerships), - selectin_polymorphic(OktaGroup, [AppGroup, RoleGroup]), - *effective_constraint_options(), - ), - ) - .where(RoleRequest.status == AccessRequestStatus.PENDING) - .where(RoleRequest.request_ownership.is_(False)) - ) - ).all() - if effective_constraint(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY, rr.requested_group) - ] - - blocked_request_ids: list[str] = [] - for req in tagged_owner_requests + tagged_member_requests: - role_member_ids = [m.user_id for m in req.requester_role.active_user_memberships] - if all(o.user_id in role_member_ids for o in req.requested_group.active_user_ownerships): - blocked_request_ids.append(req.id) - - stmt = stmt.join(RoleRequest.requested_group).where( - or_( - OktaGroup.id.in_(groups_owned_subquery), - OktaGroup.id.in_(app_groups_owned_subquery), - RoleRequest.id.in_(blocked_request_ids), - ) - ) - else: - stmt = stmt.join(RoleRequest.requested_group).where( - or_( - OktaGroup.id.in_(groups_owned_subquery), - OktaGroup.id.in_(app_groups_owned_subquery), - ) - ) - - owned_groups = ( - ( - await db.scalars( - select(OktaGroup) - .options( - selectin_polymorphic(OktaGroup, [AppGroup, RoleGroup]), - *effective_constraint_options(), - ) - .where( - or_( - OktaGroup.id.in_(groups_owned_subquery), - OktaGroup.id.in_(app_groups_owned_subquery), - ) - ) - ) - ) - .unique() - .all() - ) - owned_groups_no_self_owner = [ - g.id - for g in owned_groups - if effective_constraint(Tag.DISALLOW_SELF_ADD_OWNERSHIP_CONSTRAINT_KEY, g) - ] - owned_groups_no_self_member = [ - g.id - for g in owned_groups - if effective_constraint(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY, g) - ] - role_membership_ids = [ - rg.id - for rg in ( - await db.scalars(select(RoleGroup).options(joinedload(RoleGroup.active_user_memberships))) - ) - .unique() - .all() - if assignee_user.id in [m.user_id for m in rg.active_user_memberships] - ] - stmt = stmt.where( - not_( - or_( - and_( - RoleRequest.requested_group_id.in_(owned_groups_no_self_owner), - RoleRequest.requester_role_id.in_(role_membership_ids), - RoleRequest.request_ownership.is_(True), - ), - and_( - RoleRequest.requested_group_id.in_(owned_groups_no_self_member), - RoleRequest.requester_role_id.in_(role_membership_ids), - RoleRequest.request_ownership.is_(False), - ), - ) - ) - ) - # Whether admin or not, never include the assignee's own requests. - stmt = stmt.where(RoleRequest.requester_user_id != assignee_user.id) + stmt = stmt.where(is_assigned_reviewer(RoleRequest, assignee_user.id)) else: stmt = stmt.where(false()) @@ -355,6 +199,17 @@ async def get_role_request(role_request_id: str, db: DbSession, current_user_id: return RoleRequestDetail.model_validate(rr, from_attributes=True) +@router.get("/{role_request_id}/reviewers", name="role_request_reviewers") +async def get_role_request_reviewers( + role_request_id: str, db: DbSession, current_user_id: CurrentUserId +) -> RequestReviewers: + """Eligible reviewers of a role request by owner level, nearest first; the first level is assigned.""" + request = await db.get(RoleRequest, role_request_id) + if request is None: + raise HTTPException(404, "Not Found") + return await request_reviewers_response(request) + + @router.post("", name="role_requests_create", status_code=201) async def post_role_request( body: CreateRoleRequestBody, diff --git a/api/schemas/__init__.py b/api/schemas/__init__.py index bf502442..f8e7642a 100644 --- a/api/schemas/__init__.py +++ b/api/schemas/__init__.py @@ -84,6 +84,8 @@ EffectiveConstraintsQuery, GroupMember, GroupRequestDetail, + RequestReviewerLevel, + RequestReviewers, ResolveAccessRequestBody, ResolveGroupRequestBody, ResolveRoleRequestBody, diff --git a/api/schemas/requests_schemas.py b/api/schemas/requests_schemas.py index 00344886..c239165d 100644 --- a/api/schemas/requests_schemas.py +++ b/api/schemas/requests_schemas.py @@ -29,6 +29,7 @@ from api.access_config import get_access_config from api.config import settings +from api.models.request_reviewers import OwnerLevel from api.schemas.core_schemas import ( AppIdRef, GroupRef, @@ -224,6 +225,29 @@ class ResolveAccessRequestBody(BaseModel): ending_at: Optional[FlexibleDatetime] = None +# --- Request reviewers ------------------------------------------------------ + + +class RequestReviewerLevel(BaseModel): + """A request's eligible reviewers at one owner level.""" + + owner_level: OwnerLevel + reviewers: list[OktaUserSummary] + + +class RequestReviewers(BaseModel): + """A request's eligible reviewers by owner level. + + `reviewers_by_level` lists the levels that have an eligible reviewer, + nearest first, with each user at their nearest level. The first level is + the assigned one: its reviewers have the request in "Assigned to Me" and + were notified. The rest show a requester who to contact if they need to + escalate. An empty list means no one is eligible to review the request. + """ + + reviewers_by_level: list[RequestReviewerLevel] + + # --- Role requests ---------------------------------------------------------- diff --git a/migrations/versions/4e1c7b9a2d3f_index_app_group_app_id.py b/migrations/versions/4e1c7b9a2d3f_index_app_group_app_id.py new file mode 100644 index 00000000..8932f018 --- /dev/null +++ b/migrations/versions/4e1c7b9a2d3f_index_app_group_app_id.py @@ -0,0 +1,27 @@ +"""index app_group app_id + +Revision ID: 4e1c7b9a2d3f +Revises: 09099786c745 +Create Date: 2026-10-01 12:00:00.000000 + +""" + +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "4e1c7b9a2d3f" +down_revision = "09099786c745" +branch_labels = None +depends_on = None + + +def upgrade(): + # Plain (non-CONCURRENT) CREATE INDEX, like the okta_user_group_member + # indexes: transactional, at the cost of briefly blocking writes to this + # small table while it builds. + op.create_index("ix_app_group_app_id", "app_group", ["app_id"]) + + +def downgrade(): + op.drop_index("ix_app_group_app_id", table_name="app_group") diff --git a/src/api/apiComponents.ts b/src/api/apiComponents.ts index b0d66395..6ac4b6a1 100644 --- a/src/api/apiComponents.ts +++ b/src/api/apiComponents.ts @@ -337,6 +337,95 @@ export const useAccessRequestByIdPut = ( ); }; +export type AccessRequestReviewersPathParams = { + accessRequestId: string; +}; + +export type AccessRequestReviewersError = Fetcher.ErrorWrapper<{ + status: Exclude; + payload: Schemas.ProblemDetail; +}>; + +export type AccessRequestReviewersVariables = { + pathParams: AccessRequestReviewersPathParams; +} & ApiContext['fetcherOptions']; + +/** + * Eligible reviewers of an access request by owner level, nearest first; the first level is assigned. + */ +export const fetchAccessRequestReviewers = (variables: AccessRequestReviewersVariables, signal?: AbortSignal) => + apiFetch({ + url: '/api/requests/{accessRequestId}/reviewers', + method: 'get', + ...variables, + signal, + }); + +/** + * Eligible reviewers of an access request by owner level, nearest first; the first level is assigned. + */ +export function accessRequestReviewersQuery(variables: AccessRequestReviewersVariables): { + queryKey: reactQuery.QueryKey; + queryFn: (options: QueryFnOptions) => Promise; +}; + +export function accessRequestReviewersQuery(variables: AccessRequestReviewersVariables | reactQuery.SkipToken): { + queryKey: reactQuery.QueryKey; + queryFn: ((options: QueryFnOptions) => Promise) | reactQuery.SkipToken; +}; + +export function accessRequestReviewersQuery(variables: AccessRequestReviewersVariables | reactQuery.SkipToken) { + return { + queryKey: queryKeyFn({ + path: '/api/requests/{accessRequestId}/reviewers', + operationId: 'accessRequestReviewers', + variables, + }), + queryFn: + variables === reactQuery.skipToken + ? reactQuery.skipToken + : ({signal}: QueryFnOptions) => fetchAccessRequestReviewers(variables, signal), + }; +} + +/** + * Eligible reviewers of an access request by owner level, nearest first; the first level is assigned. + */ +export const useSuspenseAccessRequestReviewers = ( + variables: AccessRequestReviewersVariables, + options?: Omit< + reactQuery.UseQueryOptions, + 'queryKey' | 'queryFn' | 'initialData' + >, +) => { + const {queryOptions, fetcherOptions} = useApiContext(options); + return reactQuery.useSuspenseQuery({ + ...accessRequestReviewersQuery(deepMerge(fetcherOptions, variables)), + ...options, + ...queryOptions, + }); +}; + +/** + * Eligible reviewers of an access request by owner level, nearest first; the first level is assigned. + */ +export const useAccessRequestReviewers = ( + variables: AccessRequestReviewersVariables | reactQuery.SkipToken, + options?: Omit< + reactQuery.UseQueryOptions, + 'queryKey' | 'queryFn' | 'initialData' + >, +) => { + const {queryOptions, fetcherOptions} = useApiContext(options); + return reactQuery.useQuery({ + ...accessRequestReviewersQuery( + variables === reactQuery.skipToken ? variables : deepMerge(fetcherOptions, variables), + ), + ...options, + ...queryOptions, + }); +}; + export type AppsQueryParams = { q?: string | null; /** @@ -1391,6 +1480,95 @@ export const useGroupRequestByIdPut = ( }); }; +export type GroupRequestReviewersPathParams = { + groupRequestId: string; +}; + +export type GroupRequestReviewersError = Fetcher.ErrorWrapper<{ + status: Exclude; + payload: Schemas.ProblemDetail; +}>; + +export type GroupRequestReviewersVariables = { + pathParams: GroupRequestReviewersPathParams; +} & ApiContext['fetcherOptions']; + +/** + * Eligible reviewers of a group request by owner level, nearest first; the first level is assigned. + */ +export const fetchGroupRequestReviewers = (variables: GroupRequestReviewersVariables, signal?: AbortSignal) => + apiFetch({ + url: '/api/group-requests/{groupRequestId}/reviewers', + method: 'get', + ...variables, + signal, + }); + +/** + * Eligible reviewers of a group request by owner level, nearest first; the first level is assigned. + */ +export function groupRequestReviewersQuery(variables: GroupRequestReviewersVariables): { + queryKey: reactQuery.QueryKey; + queryFn: (options: QueryFnOptions) => Promise; +}; + +export function groupRequestReviewersQuery(variables: GroupRequestReviewersVariables | reactQuery.SkipToken): { + queryKey: reactQuery.QueryKey; + queryFn: ((options: QueryFnOptions) => Promise) | reactQuery.SkipToken; +}; + +export function groupRequestReviewersQuery(variables: GroupRequestReviewersVariables | reactQuery.SkipToken) { + return { + queryKey: queryKeyFn({ + path: '/api/group-requests/{groupRequestId}/reviewers', + operationId: 'groupRequestReviewers', + variables, + }), + queryFn: + variables === reactQuery.skipToken + ? reactQuery.skipToken + : ({signal}: QueryFnOptions) => fetchGroupRequestReviewers(variables, signal), + }; +} + +/** + * Eligible reviewers of a group request by owner level, nearest first; the first level is assigned. + */ +export const useSuspenseGroupRequestReviewers = ( + variables: GroupRequestReviewersVariables, + options?: Omit< + reactQuery.UseQueryOptions, + 'queryKey' | 'queryFn' | 'initialData' + >, +) => { + const {queryOptions, fetcherOptions} = useApiContext(options); + return reactQuery.useSuspenseQuery({ + ...groupRequestReviewersQuery(deepMerge(fetcherOptions, variables)), + ...options, + ...queryOptions, + }); +}; + +/** + * Eligible reviewers of a group request by owner level, nearest first; the first level is assigned. + */ +export const useGroupRequestReviewers = ( + variables: GroupRequestReviewersVariables | reactQuery.SkipToken, + options?: Omit< + reactQuery.UseQueryOptions, + 'queryKey' | 'queryFn' | 'initialData' + >, +) => { + const {queryOptions, fetcherOptions} = useApiContext(options); + return reactQuery.useQuery({ + ...groupRequestReviewersQuery( + variables === reactQuery.skipToken ? variables : deepMerge(fetcherOptions, variables), + ), + ...options, + ...queryOptions, + }); +}; + export type GroupsQueryParams = { q?: string | null; managed?: boolean | null; @@ -2824,6 +3002,93 @@ export const useRoleRequestByIdPut = ( }); }; +export type RoleRequestReviewersPathParams = { + roleRequestId: string; +}; + +export type RoleRequestReviewersError = Fetcher.ErrorWrapper<{ + status: Exclude; + payload: Schemas.ProblemDetail; +}>; + +export type RoleRequestReviewersVariables = { + pathParams: RoleRequestReviewersPathParams; +} & ApiContext['fetcherOptions']; + +/** + * Eligible reviewers of a role request by owner level, nearest first; the first level is assigned. + */ +export const fetchRoleRequestReviewers = (variables: RoleRequestReviewersVariables, signal?: AbortSignal) => + apiFetch({ + url: '/api/role-requests/{roleRequestId}/reviewers', + method: 'get', + ...variables, + signal, + }); + +/** + * Eligible reviewers of a role request by owner level, nearest first; the first level is assigned. + */ +export function roleRequestReviewersQuery(variables: RoleRequestReviewersVariables): { + queryKey: reactQuery.QueryKey; + queryFn: (options: QueryFnOptions) => Promise; +}; + +export function roleRequestReviewersQuery(variables: RoleRequestReviewersVariables | reactQuery.SkipToken): { + queryKey: reactQuery.QueryKey; + queryFn: ((options: QueryFnOptions) => Promise) | reactQuery.SkipToken; +}; + +export function roleRequestReviewersQuery(variables: RoleRequestReviewersVariables | reactQuery.SkipToken) { + return { + queryKey: queryKeyFn({ + path: '/api/role-requests/{roleRequestId}/reviewers', + operationId: 'roleRequestReviewers', + variables, + }), + queryFn: + variables === reactQuery.skipToken + ? reactQuery.skipToken + : ({signal}: QueryFnOptions) => fetchRoleRequestReviewers(variables, signal), + }; +} + +/** + * Eligible reviewers of a role request by owner level, nearest first; the first level is assigned. + */ +export const useSuspenseRoleRequestReviewers = ( + variables: RoleRequestReviewersVariables, + options?: Omit< + reactQuery.UseQueryOptions, + 'queryKey' | 'queryFn' | 'initialData' + >, +) => { + const {queryOptions, fetcherOptions} = useApiContext(options); + return reactQuery.useSuspenseQuery({ + ...roleRequestReviewersQuery(deepMerge(fetcherOptions, variables)), + ...options, + ...queryOptions, + }); +}; + +/** + * Eligible reviewers of a role request by owner level, nearest first; the first level is assigned. + */ +export const useRoleRequestReviewers = ( + variables: RoleRequestReviewersVariables | reactQuery.SkipToken, + options?: Omit< + reactQuery.UseQueryOptions, + 'queryKey' | 'queryFn' | 'initialData' + >, +) => { + const {queryOptions, fetcherOptions} = useApiContext(options); + return reactQuery.useQuery({ + ...roleRequestReviewersQuery(variables === reactQuery.skipToken ? variables : deepMerge(fetcherOptions, variables)), + ...options, + ...queryOptions, + }); +}; + export type RolesQueryParams = { q?: string | null; owner_id?: string | null; @@ -3688,6 +3953,11 @@ export type QueryOperation = operationId: 'accessRequestById'; variables: AccessRequestByIdVariables | reactQuery.SkipToken; } + | { + path: '/api/requests/{accessRequestId}/reviewers'; + operationId: 'accessRequestReviewers'; + variables: AccessRequestReviewersVariables | reactQuery.SkipToken; + } | { path: '/api/apps'; operationId: 'apps'; @@ -3728,6 +3998,11 @@ export type QueryOperation = operationId: 'groupRequestById'; variables: GroupRequestByIdVariables | reactQuery.SkipToken; } + | { + path: '/api/group-requests/{groupRequestId}/reviewers'; + operationId: 'groupRequestReviewers'; + variables: GroupRequestReviewersVariables | reactQuery.SkipToken; + } | { path: '/api/groups'; operationId: 'groups'; @@ -3788,6 +4063,11 @@ export type QueryOperation = operationId: 'roleRequestById'; variables: RoleRequestByIdVariables | reactQuery.SkipToken; } + | { + path: '/api/role-requests/{roleRequestId}/reviewers'; + operationId: 'roleRequestReviewers'; + variables: RoleRequestReviewersVariables | reactQuery.SkipToken; + } | { path: '/api/roles'; operationId: 'roles'; diff --git a/src/api/apiSchemas.ts b/src/api/apiSchemas.ts index dbd752f2..ef4bbe21 100644 --- a/src/api/apiSchemas.ts +++ b/src/api/apiSchemas.ts @@ -1047,6 +1047,27 @@ export type ProblemDetailError = { } | null; }; +/** + * A request's eligible reviewers at one owner level. + */ +export type RequestReviewerLevel = { + owner_level: 'group_owners' | 'app_owners' | 'access_admins'; + reviewers: OktaUserSummary[]; +}; + +/** + * A request's eligible reviewers by owner level. + * + * `reviewers_by_level` lists the levels that have an eligible reviewer, + * nearest first, with each user at their nearest level. The first level is + * the assigned one: its reviewers have the request in "Assigned to Me" and + * were notified. The rest show a requester who to contact if they need to + * escalate. An empty list means no one is eligible to review the request. + */ +export type RequestReviewers = { + reviewers_by_level: RequestReviewerLevel[]; +}; + export type ResolveAccessRequestBody = { approved: boolean; /** diff --git a/src/components/NavItems.tsx b/src/components/NavItems.tsx index e85cfb7c..08aeb862 100644 --- a/src/components/NavItems.tsx +++ b/src/components/NavItems.tsx @@ -137,7 +137,7 @@ export default function NavItems(props: NavItemsProps) { displayText="Assigned to Me" displayIcon={} sx={{pl: 4}} - tooltip="Individual access requests awaiting your approval" + tooltip="Individual access requests assigned to you for review" /> } sx={{pl: 4}} - tooltip="Role-based access requests awaiting your approval" + tooltip="Role-based access requests assigned to you for review" /> } sx={{pl: 4}} - tooltip="Group creation requests awaiting your approval" + tooltip="Group creation requests assigned to you for review" /> ({ + id, + email: `${first.toLowerCase()}@example.com`, + first_name: first, + last_name: 'Tester', + display_name: null, +}); + +const LEVELS: RequestReviewersData['reviewers_by_level'] = [ + {owner_level: 'group_owners', reviewers: [user('u2', 'Zed'), user('u1', 'Amy')]}, + {owner_level: 'app_owners', reviewers: [user('u4', 'Bea')]}, + {owner_level: 'access_admins', reviewers: [user('u3', 'Root')]}, +]; + +const renderPanel = (reviewers: RequestReviewersData | undefined) => + render( + + + , + ); + +describe('RequestReviewers', () => { + it('renders nothing while the reviewers are loading', () => { + const {container} = renderPanel(undefined); + expect(container).toBeEmptyDOMElement(); + }); + + it('renders one titled table per owner level, in order', () => { + renderPanel({reviewers_by_level: LEVELS}); + const tables = screen.getAllByRole('table'); + expect(tables.map((t) => t.getAttribute('aria-label'))).toEqual(['group_owners', 'app_owners', 'access_admins']); + expect(within(tables[0]).getByText('App-Foo-Admin Owners')).toBeTruthy(); + expect(within(tables[1]).getByText('Foo App Owners')).toBeTruthy(); + expect(within(tables[2]).getByText(`${ACCESS_APP_RESERVED_NAME} Admins`)).toBeTruthy(); + }); + + it('marks only the first level as assigned and sorts reviewers by name', () => { + renderPanel({reviewers_by_level: LEVELS}); + const tables = screen.getAllByRole('table'); + expect(within(tables[0]).getByText('Assigned')).toBeTruthy(); + expect(within(tables[1]).queryByText('Assigned')).toBeNull(); + expect(within(tables[2]).queryByText('Assigned')).toBeNull(); + const names = within(tables[0]) + .getAllByRole('link') + .filter((link) => link.textContent?.includes('Tester')) + .map((link) => link.textContent); + expect(names).toEqual(['Amy Tester', 'Zed Tester']); + }); + + it('warns when no one is eligible', () => { + renderPanel({reviewers_by_level: []}); + expect(screen.queryAllByRole('table')).toHaveLength(0); + expect(screen.getByText('No one is currently eligible to review this request.')).toBeTruthy(); + expect(screen.queryByText('Assigned')).toBeNull(); + expect(screen.queryByText(/assigned to/)).toBeNull(); + }); + + it('does not warn when someone is eligible', () => { + renderPanel({reviewers_by_level: LEVELS}); + expect(screen.queryByText('No one is currently eligible to review this request.')).toBeNull(); + }); + + it('says the request is assigned to the reviewers below when one level has reviewers', () => { + renderPanel({reviewers_by_level: [LEVELS[2]]}); + expect(screen.getByText(/assigned to the reviewers below/)).toBeTruthy(); + expect(screen.queryByText(/escalate/)).toBeNull(); + }); + + it('lists the other reviewers for escalation when more than one level has reviewers', () => { + renderPanel({reviewers_by_level: LEVELS}); + expect(screen.getByText(/assigned to the first group of reviewers below/)).toBeTruthy(); + expect(screen.getByText(/listed in order if you need to escalate/)).toBeTruthy(); + }); +}); diff --git a/src/components/RequestReviewers.tsx b/src/components/RequestReviewers.tsx new file mode 100644 index 00000000..b751da10 --- /dev/null +++ b/src/components/RequestReviewers.tsx @@ -0,0 +1,128 @@ +import React from 'react'; +import {Link as RouterLink} from 'react-router-dom'; + +import Alert from '@mui/material/Alert'; +import Box from '@mui/material/Box'; +import Chip from '@mui/material/Chip'; +import Divider from '@mui/material/Divider'; +import Link from '@mui/material/Link'; +import Paper from '@mui/material/Paper'; +import Table from '@mui/material/Table'; +import TableBody from '@mui/material/TableBody'; +import TableCell from '@mui/material/TableCell'; +import TableHead from '@mui/material/TableHead'; +import TableRow from '@mui/material/TableRow'; +import Typography from '@mui/material/Typography'; + +import {displayUserName} from '../helpers'; +import {ACCESS_APP_RESERVED_NAME} from '../authorization'; +import {OktaUserSummary, RequestReviewers as RequestReviewersData} from '../api/apiSchemas'; + +type OwnerLevel = RequestReviewersData['reviewers_by_level'][number]['owner_level']; + +interface RequestReviewersProps { + reviewers: RequestReviewersData | undefined; + // Names the group_owners level. + groupName?: string | null; + // Names the app_owners level. + appName?: string | null; +} + +function levelTitle(level: OwnerLevel, groupName?: string | null, appName?: string | null): string { + switch (level) { + case 'group_owners': + return `${groupName ?? 'Group'} Owners`; + case 'app_owners': + return `${appName ?? 'App'} App Owners`; + case 'access_admins': + return `${ACCESS_APP_RESERVED_NAME} Admins`; + } +} + +function sortByName(a: OktaUserSummary, b: OktaUserSummary): number { + return displayUserName(a).localeCompare(displayUserName(b)); +} + +// What the intro says about assignment, given how many levels have an eligible reviewer. +function assignmentNote(levelCount: number): string | null { + if (levelCount == 0) { + return null; + } + if (levelCount == 1) { + return "It's assigned to the reviewers below."; + } + return "It's assigned to the first group of reviewers below; the others who can review it are listed in order if you need to escalate."; +} + +// A pending request's eligible reviewers, one table per owner level nearest first. The first +// level is the one the request is assigned to; see the backend `RequestReviewers` schema. +export default function RequestReviewers({reviewers, groupName, appName}: RequestReviewersProps) { + if (reviewers == undefined) { + return null; + } + + return ( + + + + Request is pending. {assignmentNote(reviewers.reviewers_by_level.length)} + + + {reviewers.reviewers_by_level.length == 0 ? ( + + No one is currently eligible to review this request. + + ) : null} + {reviewers.reviewers_by_level.map(({owner_level, reviewers: users}, index) => ( + + + + + + + + {levelTitle(owner_level, groupName, appName)} + + {index == 0 ? : null} + + + + + Name + Email + + + + Total Reviewers: {users.length} + + + + + + {[...users].sort(sortByName).map((user) => ( + + + + {displayUserName(user)} + + + + + {user.email.toLowerCase()} + + + + ))} + +
+ + ))} + + ); +} diff --git a/src/pages/Home.tsx b/src/pages/Home.tsx index ff155cf4..6c8b98ea 100644 --- a/src/pages/Home.tsx +++ b/src/pages/Home.tsx @@ -181,9 +181,9 @@ const guide: Record> = { general: { [`What is ${appName}?`]: `${appName} is a tool for managing who in your organization has access to different resources. It expands upon the feature set provided by Okta and was created to be transparent and discoverable and to enable employees to view and manage their own access.`, 'Users, Groups, Roles, Apps': `Users are people in your organization. This may include, but is not limited to, employees and contractors. Users may be a member or owner of a group. \n\nThere are three types of groups in ${appName} with different features, namely 'vanilla' standalone groups, app groups, and roles, all of which map to Okta groups. \n\nRoles may be added as members or owners of app groups and standalone groups. When this happens, all members of the role are added as a member or owner of the group. \n\nApps are resources, such as third-party SaaS applications (eg. GitHub), or first-party services, like an internal administrator dashboard. Any app that is compatible with Okta can be an app within ${appName}. App groups can be tied to specific permissions associated with an app.`, - 'Individual Access Requests': `Users can create access requests to join a group. Access requests must be approved by a group owner, an app owner (if it's an app group), or ${appName} administrator. The 'Access Requests' nav section (under 'Individual') displays all individual access requests in your organization as well as your own requests and requests assigned to you. Access requests may also be created from that page with the Create Request button and dialog.`, - 'Role-Based Access Requests': `Role owners can create role-based access requests to ask that their roles are added to a group. Access requests must be approved by a group owner, an app owner (if it's an app group), or ${appName} administrator. The 'Access Requests' nav section (under 'Role-Based') displays all role-based access requests in your organization as well as your own requests and requests assigned to you. Role-based access requests may also be created from that page with the Create Request button and dialog.`, - 'Group Requests': `All users can create group requests to ask that a group is created. Group requests must be approved by an app owner (if it's an app group) or ${appName} administrator. The 'Group Requests' tab displays all group requests in your organization as well as your own requests and requests assigned to you. Group requests may also be created from that page with the Create Request button and dialog.`, + 'Individual Access Requests': `Users can create access requests to join a group. Access requests must be approved by a group owner, an app owner (if it's an app group), or ${appName} administrator (see [[users--who-reviews-a-request|who reviews a request]]). The 'Access Requests' nav section (under 'Individual') displays all individual access requests in your organization as well as your own requests and requests assigned to you. Access requests may also be created from that page with the Create Request button and dialog.`, + 'Role-Based Access Requests': `Role owners can create role-based access requests to ask that their roles are added to a group. Access requests must be approved by a group owner, an app owner (if it's an app group), or ${appName} administrator (see [[users--who-reviews-a-request|who reviews a request]]). The 'Access Requests' nav section (under 'Role-Based') displays all role-based access requests in your organization as well as your own requests and requests assigned to you. Role-based access requests may also be created from that page with the Create Request button and dialog.`, + 'Group Requests': `All users can create group requests to ask that a group is created. Group requests must be approved by an app owner (if it's an app group) or ${appName} administrator (see [[users--who-reviews-a-request|who reviews a request]]). The 'Group Requests' tab displays all group requests in your organization as well as your own requests and requests assigned to you. Group requests may also be created from that page with the Create Request button and dialog.`, 'Audit Pages': "Every user, group, and role has a corresponding audit page, which shows the access history for the entity. It can be viewed by clicking the clock-arrow icon next to the user/role/group name on the entity's page. Roles additionally have a 'Role audit' page that can be viewed by clicking the icon below the clock-arrow icon on a role page (it's similar to a Celtic knot). It displays the role's membership and ownership history.", 'Tags and Constraints': @@ -193,7 +193,7 @@ const guide: Record> = { }, users: { 'Creating an access request': - "You can create access requests for membership or ownership by clicking 'Request Membership' or 'Request Ownership' directly from the page for the group or role you need access to. Otherwise, navigate to the 'Access Requests' tab then click the 'Create Request' button at the top of the page. From there, you can select the group, duration, and provide a reason for the request. After submitting, the group owner(s) will be notified to approve or deny the request.", + "You can create access requests for membership or ownership by clicking 'Request Membership' or 'Request Ownership' directly from the page for the group or role you need access to. Otherwise, navigate to the 'Access Requests' tab then click the 'Create Request' button at the top of the page. From there, you can select the group, duration, and provide a reason for the request. After submitting, the request's assigned reviewers will be notified to approve or deny the request (see [[users--who-reviews-a-request|who reviews a request]]).", 'View your access': 'On the top right of any page, you can click the person icon to navigate to your user page. From there, you can see all of the groups you are an owner or member of.', "View someone else's access": `To view someone else's access, navigate to the 'Users' tab. From there, you can scroll through all of the people in your organization or search for a specific user's page. Alternatively, you can click the user's name almost anywhere it appears in ${appName} (on a group page, in an access request, etc.).`, @@ -207,7 +207,8 @@ const guide: Record> = { 'Viewing tags': "A list of existing tags can be viewed by clicking the 'Tags' button at the top of the 'Groups' and 'Apps' pages. To view the details of a tag, click on the row you are interested in to see which groups the tag is applied to and any constraints it enforces.\n\nYou can also navigate to a tag's page by clicking on the tag on the page for an app or group that has the tag applied.", 'Creating a group creation request': - "Users can request that groups are created. To do so, navigate to the 'Group Requests' tab, then click the 'Create Request' button at the top of the page. From there, you can select the group type and associated app (if applicable), and fill out group details like name, description, and tags. After submitting the request, an admin or app owner (if the request is for an app group) will need to approve or reject the request.\n\nIf the request is approved, you (the requester) will be added as the owner of the new group.\n\nIf you are an app owner and request to create an app group for your app, it will be automatically approved.", + "Users can request that groups are created. To do so, navigate to the 'Group Requests' tab, then click the 'Create Request' button at the top of the page. From there, you can select the group type and associated app (if applicable), and fill out group details like name, description, and tags. After submitting the request, an admin or app owner (if the request is for an app group) will need to approve or reject the request.\n\nIf the request is approved, you (the requester) will be added as the owner of the new group.\n\nIf you own the app's owners group (App--Owners) and request to create an app group for that app, it will be automatically approved.", + 'Who reviews a request?': `Each request is assigned to the nearest owners who can review it. Notifications and the 'Assigned to Me' pages follow the same rule.\n\nAn individual or role-based access request goes to the owners of the group. If none of them can review it, it goes to the owners of the group's app (for an app group), and then to ${appName} admins.\n\nA request to create an app group goes to the owners of its app, and then to ${appName} admins. Requests to create a role or any other group go to ${appName} admins.\n\nNo one reviews their own request. If a group has a tag that stops owners adding themselves, an owner who is a member of the requesting role can't review a role-based request for it, so the request goes to the next owners. ${appName} admins are exempt from this tag rule.\n\nEach request's page lists everyone who can review it, in order, starting with the reviewers it is assigned to, so you know who to contact if you need to escalate.`, }, 'people-lead': { 'Group owners vs. role owners': @@ -215,9 +216,9 @@ const guide: Record> = { 'What can group owners do?': 'Group owners are responsible for managing groups. They are able to change the group name and description, add additional owners, and manage group memberships. They can also delete groups.', 'Responding to an access request': - "If your organization has the notification system set up, you will receive a notification when someone requests access to a group you own for themselves or on behalf of a role they own. Otherwise, you can see individual access requests for groups you own under 'Access Requests' > 'Individual' > 'Assigned to Me' and role-based access requests under 'Access Requests' > 'Role-Based' > 'Assigned to Me'. \n\nFrom there, you can click 'View' to see the details of the access request, historical access, set an amount of time for access, provide a reason for approving or denying the request, and then either approve or reject the request.", + "If your organization has the notification system set up, you will receive a notification when an access request is assigned to you (see [[users--who-reviews-a-request|who reviews a request]]). Otherwise, 'Assigned to Me' lists the requests assigned to you: individual access requests under 'Access Requests' > 'Individual' > 'Assigned to Me' and role-based access requests under 'Access Requests' > 'Role-Based' > 'Assigned to Me'. \n\nFrom there, you can click 'View' to see the details of the access request, historical access, set an amount of time for access, provide a reason for approving or denying the request, and then either approve or reject the request.", 'Creating a role-based request': - "You can create role-based access requests for membership or ownership of groups from the 'Access Requests' > 'Role-Based' page. Click the 'Create Request' button at the top of the page to open the request creating dialog. From there, you can select the role for which you'd like to make the request (only roles you own will be shown), the group, the duration, and whether you'd like to request membership or ownership, and provide a reason for the request. After submitting, the group owner(s) will be notified to approve or deny the request.", + "You can create role-based access requests for membership or ownership of groups from the 'Access Requests' > 'Role-Based' page. Click the 'Create Request' button at the top of the page to open the request creating dialog. From there, you can select the role for which you'd like to make the request (only roles you own will be shown), the group, the duration, and whether you'd like to request membership or ownership, and provide a reason for the request. After submitting, the request's assigned reviewers will be notified to approve or deny the request (see [[users--who-reviews-a-request|who reviews a request]]).", 'Managing a group': "Group owners are able to edit the group's name and description by clicking the pencil icon to the right of the group's name on its group page. Owners may also delete the group by selecting the garbage can icon to the right of the group name.\n\nGroup owners are also responsible for managing access to the group. This can be accomplished by approving access requests made for the group and by adding users or roles as owners and members from the group's page. With all groups, it is possible to grant temporary access, which will automatically expire after the set time.", 'Managing expiring access for groups you own': @@ -236,7 +237,7 @@ const guide: Record> = { 'Managing tags for your app': "As an app owner, you can manage the set of tags applied to your app. These tags and their associated constraints are inherited by all the groups associated with your app. To modify your app's tags, navigate to the app's page and click the edit button (pencil icon).", 'Responding to group creation requests': - "If your organization has the notification system set up, you will receive a notification when someone requests to create an app group associated with an app that you own. Otherwise, you can see group requests for apps you own under ‘Group Requests' > 'Assigned to Me'.\n\nFrom there, you can click 'View' to see the details of the group request, modify the group details associated with the request, provide a reason for your decision, and then either approve or reject the request.", + "If your organization has the notification system set up, you will receive a notification when a request to create an app group for an app that you own is assigned to you. Otherwise, requests assigned to you appear under 'Group Requests' > 'Assigned to Me' (see [[users--who-reviews-a-request|who reviews a request]]).\n\nFrom there, you can click 'View' to see the details of the group request, modify the group details associated with the request, provide a reason for your decision, and then either approve or reject the request.", }, admin: { [`About ${appName} administrators`]: `${appName} administrators are the users who are members of the App-${appName} -Owners group. Administrators are able create or edit all groups, roles, and apps and manage the access for any group in the system. Administrators can also create tags, set tag constraints, apply tags to groups or apps, and remove tags.`, @@ -247,11 +248,11 @@ const guide: Record> = { 'Creating tags, managing tag constraints, and removing tags': `To create a tag, navigate to the 'Tags' page by clicking the button labeled 'Tags' from the 'Groups' or 'Apps' pages. Then, click the 'Create Tag' button at the top of the page. This opens the tag creation dialog, where you set the tag's name and description, whether it is enabled, and, optionally, its constraints. A disabled tag enforces none of its constraints.\n\nThe dialog asks where the tag's constraints apply before it asks what they are, because the answer changes what each one does. A tag that reaches roles holds membership of the roles accessing a tagged group to the same rules; a tag that applies to tagged groups only governs a role's access to the group but not who is in the role. For what each constraint does under either choice, see [[admin--what-each-tag-constraint-does|What each tag constraint does]].\n\nAfter creating a tag, you can apply it to apps and groups from the tag's page. If you add the tag to an app, all of the app's app groups (including app groups created in the future) will inherit the tag and any constraints associated with it.\n\nEditing a tag's time limit applies to access that already exists: any grant longer than the new limit is shortened when you save. Nothing is ever extended.\n\nFrom each tag's page, you can additionally remove the tag from apps and groups by clicking the X on that row and edit the tag's details by clicking the pencil icon to the right of the tag's name.\n\nYou can also add or remove tags from each app or group's individual pages from the dialog opened by clicking the pencil icon to the right of the app or group's name.`, 'What each tag constraint does': CONSTRAINT_GUIDE, 'Responding to group creation requests': - "If your organization has the notification system set up, you will receive a notification when someone requests to create a role, group, or app group for an unowned app. Otherwise, you can see group requests for that you may respond to under ‘Group Requests' > 'Assigned to Me'.\n\nFrom there, you can click 'View' to see the details of the group request, modify the group details, provide a reason for your decision, and then either approve or reject the request.", + "If your organization has the notification system set up, you will receive a notification when a group request is assigned to you: requests to create roles and plain groups, and app groups whose app has no owners who can review them. Otherwise, requests assigned to you appear under 'Group Requests' > 'Assigned to Me' (see [[users--who-reviews-a-request|who reviews a request]]).\n\nFrom there, you can click 'View' to see the details of the group request, modify the group details, provide a reason for your decision, and then either approve or reject the request.", }, faq: { 'I need to add a role as a member or owner of a group I do not own': - "In the menu bar, click 'Access Requests' and navigate to the 'Role-Based' section. From there, if you own at least one role, you can click the 'Create Request' button to create a request for your role to be added to a group. If your organization has notifications enabled, a notification will be sent to the group owner(s) about the access request. If you do not own any roles, you will not be able to create a role-based access request and you will need to reach out to the role owner.", + "In the menu bar, click 'Access Requests' and navigate to the 'Role-Based' section. From there, if you own at least one role, you can click the 'Create Request' button to create a request for your role to be added to a group. If your organization has notifications enabled, the request's assigned reviewers will be notified (see [[users--who-reviews-a-request|who reviews a request]]). If you do not own any roles, you will not be able to create a role-based access request and you will need to reach out to the role owner.", 'I need to create a new group/role': `If you are an ${appName} admin, please click [[admin--creating-a-group|here]] to see the 'Admins' user guide for step-by-step instructions.\n\nIf you are not an ${appName} admin, please see the instructions [[users--creating-a-group-creation-request|here]].`, 'I need to create a new app': `If you are an ${appName} admin, please click [[admin--creating-an-app|here]] to see step-by-step instructions.\n\nIf you are not an ${appName} admin, please reach out to one for app creation. At this moment, there is not a way to request that an app is created through ${appName}.`, 'I want to set up my group to enforce a constraint (like a maximum membership duration)': `Tags can be used to enforce a variety of constraints, including enforcing a maximum membership duration. If a tag exists that has the constraint you are looking for enabled, you can apply it to any group you own from the dialog that is opened by clicking the pencil icon next to the group's name. If a tag does not exist, reach out to an ${appName} admin to create the tag for you.`, diff --git a/src/pages/group_requests/Read.test.tsx b/src/pages/group_requests/Read.test.tsx index 347d703b..8152f15b 100644 --- a/src/pages/group_requests/Read.test.tsx +++ b/src/pages/group_requests/Read.test.tsx @@ -7,6 +7,7 @@ import {AppDetail, GroupRequestDetail, OktaUserDetail} from '../../api/apiSchema import {ACCESS_APP_RESERVED_NAME} from '../../authorization'; const resolveMutate = vi.fn(); +const reviewersQuery = vi.fn((_variables: unknown, _options: {enabled: boolean}) => ({data: undefined})); const APP_NAME = 'HammerAndChiselZendeskSandbox'; const APP_TAG = {id: 'sox-tag-00000000000', name: 'SOX'}; @@ -108,6 +109,7 @@ vi.mock('../../api/apiComponents', () => ({ }), useGroupRequestById: () => ({data: PENDING_APP_GROUP_REQUEST, isError: false, isLoading: false}), useGroupRequestByIdPut: () => ({mutate: resolveMutate}), + useGroupRequestReviewers: (variables: unknown, options: {enabled: boolean}) => reviewersQuery(variables, options), useAppById: () => ({data: APP, isLoading: false}), useApps: () => ({data: {items: appSearchResults}}), useTags: () => ({data: {items: []}}), @@ -127,6 +129,7 @@ const renderPage = () => beforeEach(() => { resolveMutate.mockClear(); constraintsAskedFor.mockClear(); + reviewersQuery.mockClear(); currentUser = APP_OWNER; ownerTimeLimit = null; appSearchResults = [APP]; @@ -167,6 +170,17 @@ describe('an app owner approving an app group request', () => { }); }); +describe('the reviewers panel for an app owner who can approve the request', () => { + // Approvers get the resolve form instead; the panel is for viewers who cannot resolve it. + it('does not fetch the reviewers', async () => { + renderPage(); + + await screen.findByRole('button', {name: /Approve/}); + expect(reviewersQuery).toHaveBeenCalled(); + expect(reviewersQuery.mock.calls.every(([, options]) => options.enabled === false)).toBe(true); + }); +}); + // A request's tags are resolved by id, not by searching the tag list. The list // is paginated and ordered by name, so a request naming a tag that no single // page contains would otherwise seed a partial selection -- and approval diff --git a/src/pages/group_requests/Read.tsx b/src/pages/group_requests/Read.tsx index 7f01f739..627106fc 100644 --- a/src/pages/group_requests/Read.tsx +++ b/src/pages/group_requests/Read.tsx @@ -45,6 +45,7 @@ import { tagByIdQuery, useApps, useGroupRequestById, + useGroupRequestReviewers, useTags, useGroupRequestByIdPut, } from '../../api/apiComponents'; @@ -65,6 +66,7 @@ import ConstraintsUnavailableAlert from '../../components/ConstraintsUnavailable import AppGroupLifecyclePluginConfigurationForm from '../../components/AppGroupLifecyclePluginConfigurationForm'; import Loading from '../../components/Loading'; +import RequestReviewers from '../../components/RequestReviewers'; import accessConfig from '../../config/accessConfig'; import {pluginIdForApp, extractRequestedPluginData} from './pluginConfig'; import PluginConfigDisplay from './PluginConfigDisplay'; @@ -331,6 +333,11 @@ export default function ReadGroupRequest() { const canApprove = (admin || isAppOwner) && !ownRequest; const canResolve = canApprove || ownRequest; + const {data: reviewers} = useGroupRequestReviewers( + {pathParams: {groupRequestId: groupRequest.id}}, + {enabled: groupRequest.status == 'PENDING' && !canApprove}, + ); + const {data: appSearchData} = useApps({ queryParams: {page: 1, size: 10, q: appSearchInput}, }); @@ -1064,13 +1071,20 @@ export default function ReadGroupRequest() { + {ownRequest ? ( + + ) : null} ) : ( - - - Request is pending and can be reviewed by app owners or Access admins. - - + ) ) : ( diff --git a/src/pages/requests/Read.tsx b/src/pages/requests/Read.tsx index 9f3a2687..6e66986c 100644 --- a/src/pages/requests/Read.tsx +++ b/src/pages/requests/Read.tsx @@ -14,11 +14,6 @@ import AccessRequestIcon from '../../components/icons/MoreTime'; import PendingIcon from '@mui/icons-material/HelpOutline'; import ApprovedIcon from '@mui/icons-material/CheckCircleOutline'; import RejectedIcon from '@mui/icons-material/HighlightOff'; -import Table from '@mui/material/Table'; -import TableBody from '@mui/material/TableBody'; -import TableCell from '@mui/material/TableCell'; -import TableHead from '@mui/material/TableHead'; -import TableRow from '@mui/material/TableRow'; import Alert from '@mui/material/Alert'; import FormControl from '@mui/material/FormControl'; import Timeline from '@mui/lab/Timeline'; @@ -38,17 +33,16 @@ import dayjs, {Dayjs} from 'dayjs'; import RelativeTime from 'dayjs/plugin/relativeTime'; import IsSameOrBefore from 'dayjs/plugin/isSameOrBefore'; -import {groupBy, displayUserName} from '../../helpers'; +import {displayUserName} from '../../helpers'; import {approvalUntilDefault, timeLimitLabel, useConstraintsForGroups} from '../../constraints'; import ConstraintsUnavailableAlert from '../../components/ConstraintsUnavailableAlert'; import {useCurrentUser} from '../../authentication'; -import {canManageGroup, ACCESS_APP_RESERVED_NAME} from '../../authorization'; +import {canManageGroup} from '../../authorization'; import { useAccessRequestById, + useAccessRequestReviewers, useGroupById, useAppById, - useAppGroupsById, - useGroupMemberDetailsById, useAccessRequestByIdPut, useUsersAndGroups, useGroupsAndRoles, @@ -61,7 +55,6 @@ import { ResolveAccessRequestBody, GroupDetail, GroupRefForMembership, - OktaUserGroupMemberDetail, OktaGroupTagMapDetail, AppGroupDetail, AppGroupForAppDetail, @@ -74,21 +67,12 @@ import NotFound from '../NotFound'; import ChangeTitle from '../../tab-title'; import Loading from '../../components/Loading'; import accessConfig from '../../config/accessConfig'; -import {EmptyListEntry} from '../../components/EmptyListEntry'; import AccessHistory from '../../components/AccessHistory'; +import RequestReviewers from '../../components/RequestReviewers'; dayjs.extend(RelativeTime); dayjs.extend(IsSameOrBefore); -function sortGroupMembers( - [aUserId, aUsers]: [string, Array], - [bUserId, bUsers]: [string, Array], -): number { - let aEmail = aUsers[0].active_user?.email ?? ''; - let bEmail = bUsers[0].active_user?.email ?? ''; - return aEmail.localeCompare(bEmail); -} - interface ResolveRequestForm { until?: string; customUntil?: string; @@ -239,62 +223,9 @@ export default function ReadRequest() { // eslint-disable-next-line react-hooks/exhaustive-deps }, [timeLimit, autofill_until, requestedUntil, requestedUntilAdjusted]); - // Owner/approver lists are no longer inlined on the group/app payloads; they - // come from the bounded owner-filtered endpoints instead. - const {data: groupOwnerData} = useGroupMemberDetailsById( - { - pathParams: {groupId: accessRequest.requested_group?.id ?? ''}, - queryParams: {owner: true, size: 100}, - }, - { - enabled: accessRequest.requested_group != null && (!requestedGroupManager || ownRequest), - }, - ); - const ownerships = groupBy(groupOwnerData?.items ?? [], (m: OktaUserGroupMemberDetail) => m.active_user?.id); - - const {data: appOwnerGroupsData} = useAppGroupsById( - { - pathParams: {appId: ((accessRequest.requested_group ?? {}) as AppGroupDetail).app?.id ?? ''}, - queryParams: {owner: true}, - }, - { - enabled: accessRequest.requested_group?.type == 'app_group' && (!requestedGroupManager || ownRequest), - }, - ); - - // App owners = owners of the app's owner group; the app-groups payload only - // carries counts, so fetch that group's owners explicitly. - const appOwnerGroupId = appOwnerGroupsData?.items?.[0]?.id ?? ''; - const {data: appOwnerMemberData} = useGroupMemberDetailsById( - {pathParams: {groupId: appOwnerGroupId}, queryParams: {owner: true, size: 100}}, - {enabled: appOwnerGroupId !== ''}, - ); - const appOwnershipsArray = appOwnerMemberData?.items ?? []; - const appOwnerships = groupBy(appOwnershipsArray, (m: OktaUserGroupMemberDetail) => m.active_user?.id); - - const {data: accessAppOwnerGroupsData} = useAppGroupsById( - { - pathParams: {appId: ACCESS_APP_RESERVED_NAME}, - queryParams: {owner: true}, - }, - { - enabled: - accessRequest.requested_group != null && - (!requestedGroupManager || ownRequest) && - (groupOwnerData?.items?.length ?? 0) == 0 && - (accessRequest.requested_group?.type != 'app_group' || appOwnershipsArray.length == 0), - }, - ); - - // Final fallback approvers = members of the Access app's owner group. - const accessAppOwnerGroupId = accessAppOwnerGroupsData?.items?.[0]?.id ?? ''; - const {data: accessAppOwnerMemberData} = useGroupMemberDetailsById( - {pathParams: {groupId: accessAppOwnerGroupId}, queryParams: {owner: false, size: 100}}, - {enabled: accessAppOwnerGroupId !== ''}, - ); - const accessAppOwnerships = groupBy( - accessAppOwnerMemberData?.items ?? [], - (m: OktaUserGroupMemberDetail) => m.active_user?.id, + const {data: reviewers} = useAccessRequestReviewers( + {pathParams: {accessRequestId: accessRequest.id}}, + {enabled: accessRequest.status == 'PENDING' && (!requestedGroupManager || ownRequest)}, ); const {data: userGroupAudits} = useUsersAndGroups({ @@ -746,212 +677,11 @@ export default function ReadRequest() { ) : null}{' '} {!requestedGroupManager || ownRequest ? ( - - - - Request is pending and can be reviewed by the following owners - - - {accessRequest.requested_group?.type != 'app_group' || - !(accessRequest.requested_group as AppGroupDetail).is_owner ? ( - - - - - - - {accessRequest.requested_group?.name}{' '} - {accessRequest.requested_group?.type == 'role_group' - ? 'Owners' - : 'Group Owners'} - - - - - Name - Email - - - - Total Owners: {Object.keys(ownerships).length} - - - - - - {Object.keys(ownerships).length > 0 ? ( - Object.entries(ownerships) - .sort(sortGroupMembers) - .map(([userId, users]: [string, Array]) => ( - - - - {displayUserName(users[0].active_user)} - - - - - {users[0].active_user?.email.toLowerCase()} - - - - )) - ) : ( - - )} - -
- - ) : null} - {accessRequest.requested_group?.type == 'app_group' ? ( - - - - - - - {((accessRequest.requested_group ?? {}) as AppGroupDetail).app?.name} - {' App Owners'} - - - - - Name - Email - - - - Total Owners: {Object.keys(appOwnerships).length} - - - - - - {Object.keys(appOwnerships).length > 0 ? ( - Object.entries(appOwnerships) - .sort(sortGroupMembers) - .map(([userId, users]: [string, Array]) => ( - - - - {displayUserName(users[0].active_user)} - - - - - {users[0].active_user?.email.toLowerCase()} - - - - )) - ) : ( - - )} - -
- - ) : null} - {Object.keys(ownerships).length == 0 && - (accessRequest.requested_group?.type != 'app_group' || appOwnershipsArray.length == 0) ? ( - - - - - - - {ACCESS_APP_RESERVED_NAME} - {' Admins'} - - - - - Name - Email - - - - Total Owners: {Object.keys(accessAppOwnerships).length} - - - - - - {Object.keys(accessAppOwnerships).length > 0 ? ( - Object.entries(accessAppOwnerships) - .sort(sortGroupMembers) - .map(([userId, users]: [string, Array]) => ( - - - - {displayUserName(users[0].active_user)} - - - - - {users[0].active_user?.email.toLowerCase()} - - - - )) - ) : ( - - )} - -
- - ) : null} - + ) : null} ) : ( diff --git a/src/pages/role_requests/Read.tsx b/src/pages/role_requests/Read.tsx index e10b9306..5af1d84d 100644 --- a/src/pages/role_requests/Read.tsx +++ b/src/pages/role_requests/Read.tsx @@ -16,11 +16,6 @@ import AlertIcon from '@mui/icons-material/Campaign'; import PendingIcon from '@mui/icons-material/HelpOutline'; import ApprovedIcon from '@mui/icons-material/CheckCircleOutline'; import RejectedIcon from '@mui/icons-material/HighlightOff'; -import Table from '@mui/material/Table'; -import TableBody from '@mui/material/TableBody'; -import TableCell from '@mui/material/TableCell'; -import TableHead from '@mui/material/TableHead'; -import TableRow from '@mui/material/TableRow'; import Alert from '@mui/material/Alert'; import FormControl from '@mui/material/FormControl'; import Timeline from '@mui/lab/Timeline'; @@ -41,17 +36,16 @@ import RelativeTime from 'dayjs/plugin/relativeTime'; import IsSameOrBefore from 'dayjs/plugin/isSameOrBefore'; import RoleMembers from './RoleMembers'; -import {groupBy, displayUserName, ownerCantAddSelf} from '../../helpers'; +import {ownerCantAddSelf} from '../../helpers'; import {approvalUntilDefault, timeLimitLabel, useConstraintsForGroups} from '../../constraints'; import ConstraintsUnavailableAlert from '../../components/ConstraintsUnavailableAlert'; import {useCurrentUser} from '../../authentication'; -import {canManageGroup, isAccessAdmin, ACCESS_APP_RESERVED_NAME} from '../../authorization'; +import {canManageGroup} from '../../authorization'; import { useRoleRequestById, + useRoleRequestReviewers, useGroupById, useAppById, - useAppGroupsById, - useGroupMemberDetailsById, useRoleRequestByIdPut, RoleRequestByIdPutError, RoleRequestByIdPutVariables, @@ -63,7 +57,6 @@ import { AppGroupForAppDetail, GroupRefForMembership, OktaUserGroupMemberDetail, - OktaUserSummary, GroupDetail, RoleGroupDetail, RoleRequestDetail, @@ -77,19 +70,11 @@ import NotFound from '../NotFound'; import Loading from '../../components/Loading'; import ChangeTitle from '../../tab-title'; import AccessHistory from '../../components/AccessHistory'; +import RequestReviewers from '../../components/RequestReviewers'; dayjs.extend(RelativeTime); dayjs.extend(IsSameOrBefore); -function sortGroupMembers( - [aUserId, aUsers]: [string, Array], - [bUserId, bUsers]: [string, Array], -): number { - let aEmail = aUsers[0].active_user?.email ?? ''; - let bEmail = bUsers[0].active_user?.email ?? ''; - return aEmail.localeCompare(bEmail); -} - interface ResolveRequestForm { until?: string; customUntil?: string; @@ -135,7 +120,6 @@ export default function ReadRoleRequest() { const {id} = useParams(); const currentUser = useCurrentUser(); - const admin = isAccessAdmin(currentUser); const [until, setUntil] = React.useState(null); const [requestError, setRequestError] = React.useState(''); @@ -285,66 +269,9 @@ export default function ReadRoleRequest() { // eslint-disable-next-line react-hooks/exhaustive-deps }, [timeLimit, autofill_until, requestedUntil, requestedUntilAdjusted]); - // Owner/approver lists are no longer inlined on the group/app payloads; they - // come from the bounded owner-filtered endpoints instead. - const {data: groupOwnerData} = useGroupMemberDetailsById( - { - pathParams: {groupId: roleRequest.requested_group?.id ?? ''}, - queryParams: {owner: true, size: 100}, - }, - { - enabled: roleRequest.requested_group != null && (!requestedGroupManager || ownRequest), - }, - ); - let ownerships = groupBy(groupOwnerData?.items ?? [], (m: OktaUserGroupMemberDetail) => m.active_user?.id); - const ownerIds = Object.keys(ownerships); - - // If Access admin and all group owners are blocked, add a note - const adminNoteForBlocked = tagged && admin && ownerIds.every((v) => roleMembers.includes(v)) && ownerIds.length > 0; - - const {data: appOwnerGroupsData} = useAppGroupsById( - { - pathParams: {appId: ((roleRequest.requested_group ?? {}) as AppGroupDetail).app?.id ?? ''}, - queryParams: {owner: true}, - }, - { - enabled: roleRequest.requested_group?.type == 'app_group' && (!requestedGroupManager || ownRequest), - }, - ); - - // App owners = owners of the app's owner group; the app-groups payload only - // carries counts, so fetch that group's owners explicitly. - const appOwnerGroupId = appOwnerGroupsData?.items?.[0]?.id ?? ''; - const {data: appOwnerMemberData} = useGroupMemberDetailsById( - {pathParams: {groupId: appOwnerGroupId}, queryParams: {owner: true, size: 100}}, - {enabled: appOwnerGroupId !== ''}, - ); - const appOwnershipsArray = appOwnerMemberData?.items ?? []; - const appOwnerships = groupBy(appOwnershipsArray, (m: OktaUserGroupMemberDetail) => m.active_user?.id); - - const {data: accessAppOwnerGroupsData} = useAppGroupsById( - { - pathParams: {appId: ACCESS_APP_RESERVED_NAME}, - queryParams: {owner: true}, - }, - { - enabled: - roleRequest.requested_group != null && - (!requestedGroupManager || ownRequest) && - (groupOwnerData?.items?.length ?? 0) == 0 && - (roleRequest.requested_group?.type != 'app_group' || appOwnershipsArray.length == 0), - }, - ); - - // Final fallback approvers = members of the Access app's owner group. - const accessAppOwnerGroupId = accessAppOwnerGroupsData?.items?.[0]?.id ?? ''; - const {data: accessAppOwnerMemberData} = useGroupMemberDetailsById( - {pathParams: {groupId: accessAppOwnerGroupId}, queryParams: {owner: false, size: 100}}, - {enabled: accessAppOwnerGroupId !== ''}, - ); - const accessAppOwnerships = groupBy( - accessAppOwnerMemberData?.items ?? [], - (m: OktaUserGroupMemberDetail) => m.active_user?.id, + const {data: reviewers} = useRoleRequestReviewers( + {pathParams: {roleRequestId: roleRequest.id}}, + {enabled: roleRequest.status == 'PENDING' && (!requestedGroupManager || ownRequest)}, ); // Fetch role/group audit data for the requester role and requested group @@ -654,28 +581,13 @@ export default function ReadRoleRequest() { While you own group {group.name}, you are blocked from responding to this request by group - tags. The request has been forwarded to other group owners or Access admins. + tags. The request has been forwarded to other group owners, app owners, or Access admins. ) : ( <> - {adminNoteForBlocked ? ( - - - - - - All owners of group {group.name} are blocked from approving this request due to group - tags. As an Access admin, you may respond to it on their behalf. - - - - - ) : ( - <> - )} ) : null}{' '} {!requestedGroupManager || ownRequest ? ( - - - - Request is pending and can be reviewed by the following owners or by Access admins - - - {roleRequest.requested_group?.type != 'app_group' || - !(roleRequest.requested_group as AppGroupDetail).is_owner ? ( - - - - - - - {roleRequest.requested_group?.name} Group Owners - - - - - Name - Email - - - - Total Owners: {ownerIds.length} - - - - - - {ownerIds.length > 0 ? ( - Object.entries(ownerships) - .sort(sortGroupMembers) - .map(([userId, users]: [string, Array]) => ( - - - - {displayUserName(users[0].active_user)} - - - - - {users[0].active_user?.email.toLowerCase()} - - - - )) - ) : ( - - - - None - - - - )} - -
- - ) : null} - {roleRequest.requested_group?.type == 'app_group' ? ( - - - - - - - {((roleRequest.requested_group ?? {}) as AppGroupDetail).app?.name} - {' App Owners'} - - - - - Name - Email - - - - Total Owners: {Object.keys(appOwnerships).length} - - - - - - {Object.keys(appOwnerships).length > 0 ? ( - Object.entries(appOwnerships) - .sort(sortGroupMembers) - .map(([userId, users]: [string, Array]) => ( - - - - {displayUserName(users[0].active_user)} - - - - - {users[0].active_user?.email.toLowerCase()} - - - - )) - ) : ( - - - - None - - - - )} - -
- - ) : null} - {ownerIds.length == 0 && - (roleRequest.requested_group?.type != 'app_group' || appOwnershipsArray.length == 0) ? ( - - - - - - - {ACCESS_APP_RESERVED_NAME} - {' Admins'} - - - - - Name - Email - - - - Total Owners: {Object.keys(accessAppOwnerships).length} - - - - - - {Object.keys(accessAppOwnerships).length > 0 ? ( - Object.entries(accessAppOwnerships) - .sort(sortGroupMembers) - .map(([userId, users]: [string, Array]) => ( - - - - {displayUserName(users[0].active_user)} - - - - - {users[0].active_user?.email.toLowerCase()} - - - - )) - ) : ( - - - - None - - - - )} - -
- - ) : null} - + ) : null} ) : ( diff --git a/tests/test_access_request.py b/tests/test_access_request.py index 3adb78c5..03fdb4de 100644 --- a/tests/test_access_request.py +++ b/tests/test_access_request.py @@ -19,7 +19,6 @@ RoleGroup, Tag, ) -from api.models.access_request import get_all_possible_request_approvers from api.operations import ( ApproveAccessRequest, CreateAccessRequest, @@ -660,45 +659,9 @@ async def test_create_app_access_request_notification( assert kwargs["requester"] == user -async def test_get_all_possible_request_approvers(app: FastAPI, mocker: MockerFixture, db: Db) -> None: - access_admin = ( - await db.session.scalars(select(OktaUser).where(OktaUser.email == settings.CURRENT_OKTA_USER_EMAIL)) - ).first() - - users = OktaUserFactory.batch(3) - db.session.add_all(users) - await db.session.commit() - - mocker.patch( - "api.models.access_request.get_group_managers", - return_value=[users[0], users[1]], - ) - - mocker.patch( - "api.models.access_request.get_app_managers", - return_value=[users[0], users[2]], - ) - - req = AccessRequest() - req.requested_group = AppGroupFactory.build() - - approvers = await get_all_possible_request_approvers(req) - - # Assert that the access admin and 3 users are returned with no duplicates - assert len(approvers) == 4 - assert access_admin in approvers - assert users[0] in approvers - assert users[1] in approvers - assert users[2] in approvers - - async def test_resolve_app_access_request_notification( app: FastAPI, db: Db, access_app: App, app_group: AppGroup, user: OktaUser, mocker: MockerFixture ) -> None: - access_admin = ( - await db.session.scalars(select(OktaUser).where(OktaUser.email == settings.CURRENT_OKTA_USER_EMAIL)) - ).first() - app_owner_user1 = OktaUserFactory.build() app_owner_user2 = OktaUserFactory.build() app_owner_group = AppGroupFactory.build() @@ -763,11 +726,7 @@ async def test_resolve_app_access_request_notification( 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"] - assert app_owner_user2 in kwargs["approvers"] - assert user in kwargs["approvers"] + assert set(kwargs["approvers"]) == {app_owner_user1, app_owner_user2} # Reset the access request so we can test the reject path access_request.status = AccessRequestStatus.PENDING @@ -785,11 +744,7 @@ async def test_resolve_app_access_request_notification( 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"] - assert app_owner_user2 in kwargs["approvers"] - assert user in kwargs["approvers"] + assert set(kwargs["approvers"]) == {app_owner_user1, app_owner_user2} async def test_auto_resolve_create_access_request( diff --git a/tests/test_group_request.py b/tests/test_group_request.py index b4de8515..9059c662 100644 --- a/tests/test_group_request.py +++ b/tests/test_group_request.py @@ -98,6 +98,32 @@ async def test_create_app_group_request( assert group_request.requested_group_type == "app_group" +async def test_create_app_group_request_for_unowned_app_notifies_access_admins(db: Db, mocker: MockerFixture) -> None: + """An app group request for an app whose owners group has no owners goes to the Access admins.""" + requester = await OktaUserFactory.create_async() + app_obj = await AppFactory.create_async() + await AppGroupFactory.create_async( + name=f"{AppGroup.APP_GROUP_NAME_PREFIX}{app_obj.name}{AppGroup.APP_NAME_GROUP_NAME_SEPARATOR}{AppGroup.APP_OWNERS_GROUP_NAME_SUFFIX}", + app_id=app_obj.id, + is_owner=True, + ) + admin = (await db.session.scalars(select(OktaUser).where(OktaUser.email == settings.CURRENT_OKTA_USER_EMAIL))).one() + created_spy = mocker.patch.object(get_notification_hook(), "access_group_request_created") + + group_request = await CreateGroupRequest( + requester_user=requester, + requested_group_name=f"App-{app_obj.name}-NewGroup", + requested_group_description="New app group", + requested_group_type="app_group", + requested_app_id=app_obj.id, + request_reason="Need app group", + ).execute() + + assert group_request is not None + assert created_spy.call_count == 1 + assert [approver.id for approver in created_spy.call_args.kwargs["approvers"]] == [admin.id] + + async def test_create_role_group_request( app: FastAPI, client: AsyncClient, diff --git a/tests/test_request_reviewers.py b/tests/test_request_reviewers.py new file mode 100644 index 00000000..5fef7c4a --- /dev/null +++ b/tests/test_request_reviewers.py @@ -0,0 +1,924 @@ +"""Who reviews each kind of request.""" + +from dataclasses import dataclass, field +from datetime import datetime, timedelta, timezone +from typing import Awaitable, Callable, Optional + +import pytest +from fastapi import FastAPI, HTTPException +from httpx import AsyncClient +from pytest_mock import MockerFixture +from sqlalchemy import select + +from api.auth.dependencies import get_current_user_id +from api.config import settings +from api.extensions import Db +from api.models import ( + AccessRequest, + App, + AppGroup, + GroupRequest, + OktaGroup, + OktaUser, + RoleRequest, + Tag, +) +from api.models.request_reviewers import ( + OwnerLevel, + get_assigned_reviewers, + get_eligible_reviewers_by_level, +) +from api.operations.constraints import CheckForSelfAdd +from tests.factories import ( + AccessRequestFactory, + AppFactory, + AppGroupFactory, + AppTagMapFactory, + GroupRequestFactory, + OktaGroupFactory, + OktaGroupTagMapFactory, + OktaUserFactory, + OktaUserGroupMemberFactory, + RoleGroupFactory, + RoleGroupMapFactory, + RoleRequestFactory, + TagFactory, +) +from tests.request_factories import ( + ResolveAccessRequestBodyFactory, + ResolveGroupRequestBodyFactory, + ResolveRoleRequestBodyFactory, +) + + +@dataclass +class Scenario: + """A persisted request plus the names of the users it involves.""" + + request: AccessRequest | RoleRequest | GroupRequest + users: dict[str, OktaUser] + expected_level: Optional[OwnerLevel] + expected_reviewers: set[str] + expected_by_level: list[tuple[OwnerLevel, set[str]]] = field(default_factory=list) + + +async def _admin() -> OktaUser: + """The Access admin seeded by the `db` fixture.""" + from api.extensions import db + + admin = (await db.session.scalars(select(OktaUser).where(OktaUser.email == settings.CURRENT_OKTA_USER_EMAIL))).one() + return admin + + +async def _make_admin(user: OktaUser) -> None: + """Make `user` an Access admin (a member of the Access app's owners group).""" + from api.extensions import db + + owners_group = ( + await db.session.scalars( + select(AppGroup) + .join(App, App.id == AppGroup.app_id) + .where(App.name == App.ACCESS_APP_RESERVED_NAME, AppGroup.is_owner.is_(True)) + ) + ).one() + await OktaUserGroupMemberFactory.create_async(user_id=user.id, group_id=owners_group.id, is_owner=False) + + +async def _own(user: OktaUser, group: OktaGroup, **kwargs: object) -> None: + """Make `user` an owner of `group`.""" + await OktaUserGroupMemberFactory.create_async(user_id=user.id, group_id=group.id, is_owner=True, **kwargs) + + +async def _join(user: OktaUser, group: OktaGroup) -> None: + """Make `user` a member of `group`.""" + await OktaUserGroupMemberFactory.create_async(user_id=user.id, group_id=group.id, is_owner=False) + + +async def _app(*owners: OktaUser) -> tuple[App, AppGroup]: + """An app, its owners group, and `owners` as owners of that group.""" + app = await AppFactory.create_async() + owners_group = await AppGroupFactory.create_async(app_id=app.id, is_owner=True) + for owner in owners: + await _own(owner, owners_group) + return app, owners_group + + +async def _users(*names: str) -> dict[str, OktaUser]: + """A fresh user per name, plus the seeded Access admin as "admin".""" + users = {name: await OktaUserFactory.create_async() for name in names} + users["admin"] = await _admin() + return users + + +async def _tag(key: str, *, enabled: bool = True) -> Tag: + """A tag that turns on the boolean constraint `key`.""" + return await TagFactory.create_async(constraints={key: True}, enabled=enabled) + + +# --- Access requests (individual) --------------------------------------------------------- + + +async def access_group_owners() -> Scenario: + u = await _users("requester", "owner1", "owner2") + group = await OktaGroupFactory.create_async() + await _own(u["owner1"], group) + await _own(u["owner2"], group) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "group_owners", + {"owner1", "owner2"}, + [("group_owners", {"owner1", "owner2"}), ("access_admins", {"admin"})], + ) + + +async def access_sole_owner_is_requester_plain_group() -> Scenario: + u = await _users("requester") + group = await OktaGroupFactory.create_async() + await _own(u["requester"], group) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def access_app_group_without_group_owners() -> Scenario: + u = await _users("requester", "app_owner") + app, _ = await _app(u["app_owner"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "app_owners", + {"app_owner"}, + [("app_owners", {"app_owner"}), ("access_admins", {"admin"})], + ) + + +async def access_requester_is_sole_group_owner_and_an_app_owner() -> Scenario: + u = await _users("requester", "app_owner") + app, _ = await _app(u["app_owner"], u["requester"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + await _own(u["requester"], group) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "app_owners", + {"app_owner"}, + [("app_owners", {"app_owner"}), ("access_admins", {"admin"})], + ) + + +async def access_unowned_app() -> Scenario: + u = await _users("requester") + app, _ = await _app() + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def access_app_owners_group_skips_duplicate_level() -> Scenario: + """A request for an app's owners group lists its owners once, as group owners.""" + u = await _users("requester", "app_owner") + _, owners_group = await _app(u["app_owner"]) + req = await AccessRequestFactory.create_async( + requester_user_id=u["requester"].id, requested_group_id=owners_group.id + ) + return Scenario( + req, + u, + "group_owners", + {"app_owner"}, + [("group_owners", {"app_owner"}), ("access_admins", {"admin"})], + ) + + +async def access_admin_requester_with_another_admin() -> Scenario: + u = await _users("admin2") + await _make_admin(u["admin2"]) + group = await OktaGroupFactory.create_async() + req = await AccessRequestFactory.create_async(requester_user_id=u["admin"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "access_admins", + {"admin2"}, + [("access_admins", {"admin2"})], + ) + + +async def access_nobody_eligible() -> Scenario: + u = await _users() + group = await OktaGroupFactory.create_async() + req = await AccessRequestFactory.create_async(requester_user_id=u["admin"].id, requested_group_id=group.id) + return Scenario(req, u, None, set(), []) + + +async def access_ended_ownership_ignored() -> Scenario: + u = await _users("requester", "former_owner") + group = await OktaGroupFactory.create_async() + await _own(u["former_owner"], group, ended_at=datetime.now(timezone.utc) - timedelta(days=1)) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def access_deleted_owner_ignored() -> Scenario: + from api.extensions import db + + u = await _users("requester", "deleted_owner") + group = await OktaGroupFactory.create_async() + await _own(u["deleted_owner"], group) + u["deleted_owner"].deleted_at = datetime.now(timezone.utc) + await db.session.commit() + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +# --- Role requests -------------------------------------------------------------------------- + + +async def _role_request( + u: dict[str, OktaUser], group: OktaGroup, *, role_members: tuple[str, ...], ownership: bool = False +) -> RoleRequest: + """A role request from a new role that `requester` owns and `role_members` belong to.""" + role = await RoleGroupFactory.create_async() + await _own(u["requester"], role) + for name in role_members: + await _join(u[name], role) + return await RoleRequestFactory.create_async( + requester_user_id=u["requester"].id, + requester_role_id=role.id, + requested_group_id=group.id, + request_ownership=ownership, + ) + + +async def role_tag_blocks_one_group_owner() -> Scenario: + u = await _users("requester", "blocked_owner", "owner") + group = await OktaGroupFactory.create_async() + await _own(u["blocked_owner"], group) + await _own(u["owner"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("blocked_owner",)) + return Scenario( + req, + u, + "group_owners", + {"owner"}, + [("group_owners", {"owner"}), ("access_admins", {"admin"})], + ) + + +async def role_tag_blocks_every_group_owner() -> Scenario: + u = await _users("requester", "blocked_owner") + group = await OktaGroupFactory.create_async() + await _own(u["blocked_owner"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("blocked_owner",)) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def role_ownership_tag_blocks_ownership_request() -> Scenario: + u = await _users("requester", "blocked_owner") + group = await OktaGroupFactory.create_async() + await _own(u["blocked_owner"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_OWNERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("blocked_owner",), ownership=True) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def role_membership_tag_does_not_block_ownership_request() -> Scenario: + u = await _users("requester", "owner") + group = await OktaGroupFactory.create_async() + await _own(u["owner"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("owner",), ownership=True) + return Scenario( + req, + u, + "group_owners", + {"owner"}, + [("group_owners", {"owner"}), ("access_admins", {"admin"})], + ) + + +async def role_disabled_tag_does_not_block() -> Scenario: + u = await _users("requester", "owner") + group = await OktaGroupFactory.create_async() + await _own(u["owner"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY, enabled=False) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("owner",)) + return Scenario( + req, + u, + "group_owners", + {"owner"}, + [("group_owners", {"owner"}), ("access_admins", {"admin"})], + ) + + +async def role_app_inherited_tag_blocks() -> Scenario: + u = await _users("requester", "blocked_owner") + app, _ = await _app() + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + await _own(u["blocked_owner"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY) + app_tag_map = await AppTagMapFactory.create_async(app_id=app.id, tag_id=tag.id) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id, app_tag_map_id=app_tag_map.id) + req = await _role_request(u, group, role_members=("blocked_owner",)) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def role_tag_blocks_app_owner() -> Scenario: + u = await _users("requester", "blocked_app_owner") + app, _ = await _app(u["blocked_app_owner"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("blocked_app_owner",)) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def role_ownership_tag_blocks_app_owner() -> Scenario: + u = await _users("requester", "blocked_app_owner") + app, _ = await _app(u["blocked_app_owner"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + tag = await _tag(Tag.DISALLOW_SELF_ADD_OWNERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("blocked_app_owner",), ownership=True) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def role_admin_exempt_from_tag() -> Scenario: + u = await _users("requester") + group = await OktaGroupFactory.create_async() + await _own(u["admin"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("admin",)) + return Scenario( + req, + u, + "group_owners", + {"admin"}, + [("group_owners", {"admin"})], + ) + + +# --- Group requests ------------------------------------------------------------------------- + + +async def group_app_group_with_app_owners() -> Scenario: + u = await _users("requester", "app_owner") + app, _ = await _app(u["app_owner"]) + req = await GroupRequestFactory.create_async( + requester_user_id=u["requester"].id, requested_group_type="app_group", requested_app_id=app.id + ) + return Scenario( + req, + u, + "app_owners", + {"app_owner"}, + [("app_owners", {"app_owner"}), ("access_admins", {"admin"})], + ) + + +async def group_app_group_unowned_app() -> Scenario: + u = await _users("requester") + app, _ = await _app() + req = await GroupRequestFactory.create_async( + requester_user_id=u["requester"].id, requested_group_type="app_group", requested_app_id=app.id + ) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def group_requester_is_sole_app_owner() -> Scenario: + u = await _users("requester") + app, _ = await _app(u["requester"]) + req = await GroupRequestFactory.create_async( + requester_user_id=u["requester"].id, requested_group_type="app_group", requested_app_id=app.id + ) + return Scenario( + req, + u, + "access_admins", + {"admin"}, + [("access_admins", {"admin"})], + ) + + +async def group_plain_group() -> Scenario: + u = await _users("requester") + req = await GroupRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_type="okta_group") + return Scenario(req, u, "access_admins", {"admin"}, [("access_admins", {"admin"})]) + + +async def group_role_group() -> Scenario: + u = await _users("requester") + req = await GroupRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_type="role_group") + return Scenario(req, u, "access_admins", {"admin"}, [("access_admins", {"admin"})]) + + +SCENARIOS: list[Callable[[], Awaitable[Scenario]]] = [ + access_group_owners, + access_sole_owner_is_requester_plain_group, + access_app_group_without_group_owners, + access_requester_is_sole_group_owner_and_an_app_owner, + access_unowned_app, + access_app_owners_group_skips_duplicate_level, + access_admin_requester_with_another_admin, + access_nobody_eligible, + access_ended_ownership_ignored, + access_deleted_owner_ignored, + role_tag_blocks_one_group_owner, + role_tag_blocks_every_group_owner, + role_ownership_tag_blocks_ownership_request, + role_membership_tag_does_not_block_ownership_request, + role_disabled_tag_does_not_block, + role_app_inherited_tag_blocks, + role_tag_blocks_app_owner, + role_ownership_tag_blocks_app_owner, + role_admin_exempt_from_tag, + group_app_group_with_app_owners, + group_app_group_unowned_app, + group_requester_is_sole_app_owner, + group_plain_group, + group_role_group, +] + + +def _names(scenario: Scenario, users: list[OktaUser]) -> set[str]: + """The scenario names of `users`; raises KeyError for anyone outside the scenario.""" + by_id = {user.id: name for name, user in scenario.users.items()} + return {by_id[user.id] for user in users} + + +@pytest.mark.parametrize("build", SCENARIOS, ids=lambda b: b.__name__) +async def test_assigned_reviewers(db: Db, build: Callable[[], Awaitable[Scenario]]) -> None: + scenario = await build() + levels = await get_eligible_reviewers_by_level(scenario.request) + assert (levels[0].owner_level if levels else None) == scenario.expected_level + assert _names(scenario, await get_assigned_reviewers(scenario.request)) == scenario.expected_reviewers + + +@pytest.mark.parametrize("build", SCENARIOS, ids=lambda b: b.__name__) +async def test_eligible_reviewers_by_level(db: Db, build: Callable[[], Awaitable[Scenario]]) -> None: + scenario = await build() + levels = await get_eligible_reviewers_by_level(scenario.request) + assert [(level.owner_level, _names(scenario, level.reviewers)) for level in levels] == scenario.expected_by_level + + +async def test_close_notification_uses_reviewers_assigned_before_approval(db: Db, mocker: MockerFixture) -> None: + """Approving a role request for ownership makes the role's members owners of + the group; the close notification still goes to the reviewers assigned + while the request was open.""" + from api.operations import ApproveRoleRequest + from api.plugins import get_notification_hook + from api.services import okta + + u = await _users("requester", "app_owner", "role_member") + app, _ = await _app(u["app_owner"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + req = await _role_request(u, group, role_members=("role_member",), ownership=True) + + mocker.patch.object(okta, "add_owner_to_group") + mocker.patch.object(okta, "add_user_to_group") + hook = get_notification_hook() + completed = mocker.patch.object(hook, "access_role_request_completed") + + await ApproveRoleRequest(role_request=req, approver_user=u["app_owner"]).execute() + + assert completed.call_count == 1 + _, kwargs = completed.call_args + assert {user.id for user in kwargs["approvers"]} == {u["app_owner"].id} + + +async def test_modify_group_users_batch_close_notifications_use_reviewers_assigned_before_the_batch( + db: Db, mocker: MockerFixture +) -> None: + """Adding two requesters as owners in one call makes each an owner before + either notification goes out; each still goes to the app owners, who were + assigned while the requests were open, not to the other new owner.""" + from api.operations import ModifyGroupUsers + from api.plugins import get_notification_hook + from api.services import okta + + u = await _users("app_owner", "requester1", "requester2") + app, _ = await _app(u["app_owner"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + requests = [ + await AccessRequestFactory.create_async( + requester_user_id=u[name].id, requested_group_id=group.id, request_ownership=True + ) + for name in ("requester1", "requester2") + ] + + mocker.patch.object(okta, "add_owner_to_group") + completed = mocker.patch.object(get_notification_hook(), "access_request_completed") + + await ModifyGroupUsers( + group=group.id, + owners_to_add=[u["requester1"].id, u["requester2"].id], + current_user_id=u["app_owner"].id, + ).execute() + + assert completed.call_count == 2 + notified = {call.kwargs["access_request"].id: call.kwargs["approvers"] for call in completed.call_args_list} + assert set(notified) == {request.id for request in requests} + for approvers in notified.values(): + assert {user.id for user in approvers} == {u["app_owner"].id} + + +async def test_modify_group_users_snapshots_only_requests_it_can_fulfil(db: Db, mocker: MockerFixture) -> None: + """Adding a member to a role snapshots the member's requests for the role and + the groups the role is associated with, not their other pending requests.""" + import api.operations.modify_group_users as modify_group_users + from api.operations import ModifyGroupUsers + from api.services import okta + + u = await _users("requester", "group_owner") + role = await RoleGroupFactory.create_async() + associated = await OktaGroupFactory.create_async() + unrelated = await OktaGroupFactory.create_async() + await _own(u["group_owner"], associated) + await RoleGroupMapFactory.create_async(role_group_id=role.id, group_id=associated.id, is_owner=False) + fulfillable = await AccessRequestFactory.create_async( + requester_user_id=u["requester"].id, requested_group_id=associated.id + ) + await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=unrelated.id) + + mocker.patch.object(okta, "add_user_to_group") + snapshot = mocker.spy(modify_group_users, "snapshot_assigned_reviewers") + + await ModifyGroupUsers(group=role.id, members_to_add=[u["requester"].id], current_user_id=u["admin"].id).execute() + + assert [request.id for request in snapshot.call_args.args[0]] == [fulfillable.id] + + +@pytest.mark.parametrize("operation", ["ModifyGroupUsers", "ModifyRoleGroups"]) +async def test_close_notification_computes_reviewers_for_a_request_missing_from_the_snapshot( + db: Db, mocker: MockerFixture, operation: str +) -> None: + """A request the snapshot misses, such as one opened after it was taken, has + its reviewers computed when the operation closes it.""" + import api.operations.modify_group_users as modify_group_users + import api.operations.modify_role_groups as modify_role_groups + from api.operations import ModifyGroupUsers, ModifyRoleGroups + from api.plugins import get_notification_hook + from api.services import okta + + u = await _users("requester", "group_owner", "role_member") + group = await OktaGroupFactory.create_async() + await _own(u["group_owner"], group) + mocker.patch.object(okta, "add_user_to_group") + mocker.patch.object(okta, "add_owner_to_group") + hook = get_notification_hook() + + if operation == "ModifyGroupUsers": + mocker.patch.object(modify_group_users, "snapshot_assigned_reviewers", return_value={}) + completed = mocker.patch.object(hook, "access_request_completed") + await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + await ModifyGroupUsers( + group=group.id, members_to_add=[u["requester"].id], current_user_id=u["group_owner"].id + ).execute() + else: + mocker.patch.object(modify_role_groups, "snapshot_assigned_reviewers", return_value={}) + completed = mocker.patch.object(hook, "access_role_request_completed") + role_request = await _role_request(u, group, role_members=("role_member",)) + await ModifyRoleGroups( + role_group=role_request.requester_role_id, + groups_to_add=[group.id], + current_user_id=u["group_owner"].id, + ).execute() + + assert completed.call_count == 1 + assert {user.id for user in completed.call_args.kwargs["approvers"]} == {u["group_owner"].id} + + +async def test_delete_group_close_notification_uses_reviewers_assigned_before_deletion( + db: Db, mocker: MockerFixture +) -> None: + """Deleting an app group closes its pending access requests; the close + notification goes to the app owners assigned while the request was open, + though the deleted group no longer routes to them.""" + from api.operations import DeleteGroup + from api.plugins import get_notification_hook + from api.services import okta + + u = await _users("requester", "app_owner") + app, _ = await _app(u["app_owner"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + + mocker.patch.object(okta, "delete_group") + completed = mocker.patch.object(get_notification_hook(), "access_request_completed") + + await DeleteGroup(group=group.id).execute() + + assert completed.call_count == 1 + assert completed.call_args.kwargs["access_request"].id == req.id + assert {user.id for user in completed.call_args.kwargs["approvers"]} == {u["app_owner"].id} + + +async def test_delete_role_close_notification_uses_reviewers_assigned_before_deletion( + db: Db, mocker: MockerFixture +) -> None: + """Deleting the requesting role ends its memberships, which would lift the + tag block on a group owner who is a role member; the close notification + still goes to the Access admins, who were assigned while it was open.""" + from api.operations import DeleteGroup + from api.plugins import get_notification_hook + + u = await _users("requester", "blocked_owner") + group = await OktaGroupFactory.create_async() + await _own(u["blocked_owner"], group) + tag = await _tag(Tag.DISALLOW_SELF_ADD_MEMBERSHIP_CONSTRAINT_KEY) + await OktaGroupTagMapFactory.create_async(group_id=group.id, tag_id=tag.id) + req = await _role_request(u, group, role_members=("blocked_owner",)) + + completed = mocker.patch.object(get_notification_hook(), "access_role_request_completed") + + await DeleteGroup(group=req.requester_role_id, sync_to_okta=False).execute() + + assert completed.call_count == 1 + assert {user.id for user in completed.call_args.kwargs["approvers"]} == {u["admin"].id} + + +async def test_delete_app_close_notifications_use_reviewers_assigned_before_deletion( + db: Db, mocker: MockerFixture +) -> None: + """Deleting an app deletes its owners group too; the close notification for + a pending request on another of its groups still goes to the app owners.""" + from api.operations import DeleteApp + from api.plugins import get_notification_hook + from api.services import okta + + u = await _users("requester", "app_owner") + app, _ = await _app(u["app_owner"]) + group = await AppGroupFactory.create_async(app_id=app.id, is_owner=False) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + + mocker.patch.object(okta, "delete_group") + completed = mocker.patch.object(get_notification_hook(), "access_request_completed") + + await DeleteApp(app=app.id).execute() + + assert completed.call_count == 1 + assert completed.call_args.kwargs["access_request"].id == req.id + assert {user.id for user in completed.call_args.kwargs["approvers"]} == {u["app_owner"].id} + + +async def test_unmanage_group_close_notification_uses_reviewers_assigned_before_unmanaging( + db: Db, mocker: MockerFixture +) -> None: + """Unmanaging a group ends ownership granted through a role; the close + notification goes to the role member who owned the group while the request + was open.""" + from api.extensions import db as ext_db + from api.operations import UnmanageGroup + from api.plugins import get_notification_hook + + u = await _users("requester", "role_owner") + group = await OktaGroupFactory.create_async() + role = await RoleGroupFactory.create_async() + await _join(u["role_owner"], role) + role_map = await RoleGroupMapFactory.create_async(role_group_id=role.id, group_id=group.id, is_owner=True) + await _own(u["role_owner"], group, role_group_map_id=role_map.id) + req = await AccessRequestFactory.create_async(requester_user_id=u["requester"].id, requested_group_id=group.id) + assert [user.id for user in await get_assigned_reviewers(req)] == [u["role_owner"].id] + + group.is_managed = False + await ext_db.session.commit() + completed = mocker.patch.object(get_notification_hook(), "access_request_completed") + + await UnmanageGroup(group=group.id).execute() + + assert completed.call_count == 1 + assert {user.id for user in completed.call_args.kwargs["approvers"]} == {u["role_owner"].id} + + +_LIST_ROUTE = {AccessRequest: "access_requests", RoleRequest: "role_requests", GroupRequest: "group_requests"} + + +async def _assigned_via_list( + client: AsyncClient, url_for: Callable[..., str], scenario: Scenario, user: OktaUser +) -> bool: + """Whether the request shows in `user`'s "Assigned to Me" list.""" + rep = await client.get( + url_for(_LIST_ROUTE[type(scenario.request)]), params={"assignee_user_id": user.id, "size": 100} + ) + assert rep.status_code == 200 + return scenario.request.id in {item["id"] for item in rep.json()["items"]} + + +_RESOLVE_ROUTE = { + AccessRequest: ("access_request_by_id_put", "access_request_id", ResolveAccessRequestBodyFactory), + RoleRequest: ("role_request_by_id_put", "role_request_id", ResolveRoleRequestBodyFactory), + GroupRequest: ("group_request_by_id_put", "group_request_id", ResolveGroupRequestBodyFactory), +} + + +async def _permitted_by_resolve_routes( + app: FastAPI, client: AsyncClient, url_for: Callable[..., str], scenario: Scenario +) -> set[str]: + """The scenario users the resolve route lets approve the request, plus the role approval's self-add check.""" + request = scenario.request + route, param, body_factory = _RESOLVE_ROUTE[type(request)] + url = url_for(route, **{param: request.id}) + body = body_factory.json(approved=True, reason="approve") + # Read everything up front: a refused call rolls back the shared session, + # which expires the objects this loop would otherwise read. + users = [(name, user.id, user.email) for name, user in scenario.users.items()] + role_check = ( + (request.requester_role_id, request.requested_group_id, request.request_ownership) + if isinstance(request, RoleRequest) + else None + ) + permitted: set[str] = set() + prev_email = app.state.current_user_email + try: + for user_name, user_id, email in users: + app.state.current_user_email = email + rep = await client.put(url, json=body) + # 404 is the authentication dependency refusing a deleted user. + assert rep.status_code in {403, 404, 409}, (user_name, rep.status_code, rep.text) + if rep.status_code != 409: + continue + if role_check is not None: + role_id, group_id, ownership = role_check + valid, _ = await CheckForSelfAdd( + group=role_id, + current_user=user_id, + members_to_add=[] if ownership else [group_id], + owners_to_add=[group_id] if ownership else [], + ).execute_for_role() + if not valid: + continue + permitted.add(user_name) + finally: + app.state.current_user_email = prev_email + return permitted + + +@pytest.mark.parametrize("build", SCENARIOS, ids=lambda b: b.__name__) +async def test_reviewer_directions_agree( + db: Db, + app: FastAPI, + client: AsyncClient, + url_for: Callable[..., str], + build: Callable[[], Awaitable[Scenario]], +) -> None: + """Assigned by `get_assigned_reviewers` exactly when the "Assigned to Me" + list shows the request; listed by `get_eligible_reviewers_by_level` + exactly when permitted to approve; and the first level listed is the + assigned one.""" + scenario = await build() + assigned_names = _names(scenario, await get_assigned_reviewers(scenario.request)) + by_level = [_names(scenario, level.reviewers) for level in await get_eligible_reviewers_by_level(scenario.request)] + assert (by_level[0] if by_level else set()) == assigned_names + eligible = {name for names in by_level for name in names} + # Drop identity-map state so the routes read the database, then reload the + # objects this test reads directly. + db.session.expire_all() + for obj in (scenario.request, *scenario.users.values()): + await db.session.refresh(obj) + for name, user in scenario.users.items(): + assert (name in assigned_names) == await _assigned_via_list(client, url_for, scenario, user), name + # Each resolve route checks permission before it checks that the request is + # pending, so on a resolved request it answers 403 to a user it refuses and + # 409 to one it permits, without approving anything. + scenario.request.resolved_at = datetime.now(timezone.utc) + await db.session.commit() + assert eligible == await _permitted_by_resolve_routes(app, client, url_for, scenario) + + +_REVIEWERS_ROUTE = { + AccessRequest: ("access_request_reviewers", "access_request_id"), + RoleRequest: ("role_request_reviewers", "role_request_id"), + GroupRequest: ("group_request_reviewers", "group_request_id"), +} + + +@pytest.mark.parametrize( + "build", + [ + access_requester_is_sole_group_owner_and_an_app_owner, + role_tag_blocks_one_group_owner, + group_app_group_with_app_owners, + ], + ids=lambda b: b.__name__, +) +async def test_reviewers_route( + db: Db, client: AsyncClient, url_for: Callable[..., str], build: Callable[[], Awaitable[Scenario]] +) -> None: + scenario = await build() + name, param = _REVIEWERS_ROUTE[type(scenario.request)] + rep = await client.get(url_for(name, **{param: scenario.request.id})) + assert rep.status_code == 200 + body = rep.json() + by_id = {user.id: n for n, user in scenario.users.items()} + assert [ + (level["owner_level"], {by_id[r["id"]] for r in level["reviewers"]}) for level in body["reviewers_by_level"] + ] == scenario.expected_by_level + + +@pytest.mark.parametrize( + "build", + [ + access_requester_is_sole_group_owner_and_an_app_owner, + role_tag_blocks_one_group_owner, + group_app_group_with_app_owners, + ], + ids=lambda b: b.__name__, +) +async def test_reviewers_route_unknown_id( + db: Db, client: AsyncClient, url_for: Callable[..., str], build: Callable[[], Awaitable[Scenario]] +) -> None: + """The same route that answers for a known request answers 404 for an unknown id.""" + scenario = await build() + name, param = _REVIEWERS_ROUTE[type(scenario.request)] + assert (await client.get(url_for(name, **{param: scenario.request.id}))).status_code == 200 + rep = await client.get(url_for(name, **{param: "missing"})) + assert rep.status_code == 404 + assert rep.headers["content-type"].startswith("application/problem+json") + + +@pytest.mark.parametrize("model", [AccessRequest, RoleRequest, GroupRequest], ids=lambda m: m.__name__) +async def test_reviewers_route_requires_authentication( + app: FastAPI, client: AsyncClient, url_for: Callable[..., str], model: type +) -> None: + """The route declares `CurrentUserId`, so a failing authentication dependency rejects the call.""" + + def reject() -> str: + raise HTTPException(status_code=401, detail="Unauthenticated") + + app.dependency_overrides[get_current_user_id] = reject + name, param = _REVIEWERS_ROUTE[model] + rep = await client.get(url_for(name, **{param: "any"})) + assert rep.status_code == 401 diff --git a/tests/test_role_request.py b/tests/test_role_request.py index df410068..0ad1d256 100644 --- a/tests/test_role_request.py +++ b/tests/test_role_request.py @@ -20,7 +20,6 @@ RoleRequest, Tag, ) -from api.models.access_request import get_all_possible_request_approvers from api.operations import ( ApproveRoleRequest, CreateRoleRequest, @@ -28,6 +27,7 @@ ModifyRoleGroups, RejectRoleRequest, ) +from api.operations.constraints import CheckForSelfAdd from api.plugins import ConditionalAccessResponse, get_notification_hook from api.services import okta from tests.factories import ( @@ -709,38 +709,6 @@ async def test_create_app_role_request_notification( assert kwargs["requester"] == user -async def test_get_all_possible_role_request_approvers(app: FastAPI, mocker: MockerFixture, db: Db) -> None: - access_admin = ( - await db.session.scalars(select(OktaUser).where(OktaUser.email == settings.CURRENT_OKTA_USER_EMAIL)) - ).first() - - users = OktaUserFactory.batch(3) - db.session.add_all(users) - await db.session.commit() - - mocker.patch( - "api.models.access_request.get_group_managers", - return_value=[users[0], users[1]], - ) - - mocker.patch( - "api.models.access_request.get_app_managers", - return_value=[users[0], users[2]], - ) - - req = RoleRequest() - req.requested_group = AppGroupFactory.build() - - approvers = await get_all_possible_request_approvers(req) - - # Assert that the access admin and 3 users are returned with no duplicates - assert len(approvers) == 4 - assert access_admin in approvers - assert users[0] in approvers - assert users[1] in approvers - assert users[2] in approvers - - async def test_role_request_approvers_tagged( app: FastAPI, db: Db, @@ -811,10 +779,6 @@ async def test_resolve_app_role_request_notification( user: OktaUser, mocker: MockerFixture, ) -> None: - access_admin = ( - await db.session.scalars(select(OktaUser).where(OktaUser.email == settings.CURRENT_OKTA_USER_EMAIL)) - ).first() - app_owner_user1 = OktaUserFactory.build() app_owner_user2 = OktaUserFactory.build() app_owner_group = AppGroupFactory.build() @@ -890,11 +854,7 @@ async def test_resolve_app_role_request_notification( assert kwargs["role"] == role_group 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"] - assert app_owner_user2 in kwargs["approvers"] - assert user in kwargs["approvers"] + assert set(kwargs["approvers"]) == {app_owner_user1, app_owner_user2} # Reset the access request so we can test the reject path role_request.status = AccessRequestStatus.PENDING @@ -913,11 +873,7 @@ async def test_resolve_app_role_request_notification( assert kwargs["role"] == role_group 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"] - assert app_owner_user2 in kwargs["approvers"] - assert user in kwargs["approvers"] + assert set(kwargs["approvers"]) == {app_owner_user1, app_owner_user2} async def test_auto_resolve_create_role_request( @@ -1392,8 +1348,7 @@ async def test_role_request_approval_via_direct_add( 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"] + assert kwargs["approvers"] == [access_owner] group_url = url_for("api-groups.group_members_by_id", group_id=okta_group2.id) rep = await client.get(group_url) @@ -1526,16 +1481,14 @@ async def test_role_request_list_filters_by_role_and_group_together(client: Asyn assert [r["id"] for r in rep.json()["items"]] == [target_rr.id] -async def test_role_request_assignee_filter_excludes_propagated_self_add_role_target( +async def test_role_request_assignee_filter_ignores_propagated_self_add_tag_on_role_target( client: AsyncClient, db: Db, tag: Tag, url_for: Any ) -> None: - """`owned_groups_no_self_member` in the `assignee_user_id` branch of the - role-requests list must key off `effective_constraint`, which also sees - `disallow_self_add_membership` propagated onto a *role* the assignee owns - (because that role is itself a member of a tagged group), not only tags - applied directly to the role. Keying off only directly-applied tags would - offer the assignee a request their approval would then be blocked from - resolving.""" + """A `disallow_self_add_membership` tag that reaches the target only by + propagation (the target is a role that is itself a member of a tagged + group) does not block approval: `CheckForSelfAdd.execute_for_role` reads + only the target's own tags. So the request is listed for the target's + owner, who can approve it.""" role_owner = await OktaUserFactory.create_async() other_member = await OktaUserFactory.create_async() owned_role = await RoleGroupFactory.create_async() @@ -1544,9 +1497,8 @@ async def test_role_request_assignee_filter_excludes_propagated_self_add_role_ta # `role_owner` owns `owned_role`, and is separately a plain member of # `other_role` alongside `other_member` -- `other_member` is who actually - # submits the (fixture-inserted) request below, so the unconditional - # "never show the assignee their own requests" filter doesn't hide it - # for an unrelated reason. + # submits the (fixture-inserted) request below, so `role_owner` is not the + # requester, whom the reviewer rule never makes eligible. await ModifyGroupUsers(group=owned_role, owners_to_add=[role_owner.id], sync_to_okta=False).execute() await ModifyGroupUsers( group=other_role, members_to_add=[role_owner.id, other_member.id], sync_to_okta=False @@ -1576,19 +1528,20 @@ async def test_role_request_assignee_filter_excludes_propagated_self_add_role_ta assignee_user_id = role_owner.id pending_rr_id = pending_rr.id - # The test session already loaded `owned_role`'s (previously empty) - # `active_role_associated_group_member_mappings` collection earlier in - # this test (e.g. via factory/operation calls before the mapping row - # existed); expire it so the endpoint's own query -- sharing this same - # session -- repopulates it instead of reading the stale cached state. - # A real request never has this staleness since it starts a fresh session. + valid, _ = await CheckForSelfAdd( + group=other_role.id, current_user=role_owner.id, members_to_add=[owned_role.id] + ).execute_for_role() + assert valid + + # The endpoint shares this session; drop the role's cached (pre-mapping) + # collections so it reads the committed state, as a real request would. db.session.expire_all() list_url = url_for("api-role-requests.role_requests") rep = await client.get(list_url, params={"assignee_user_id": assignee_user_id, "status": "PENDING"}) assert rep.status_code == 200 ids = [r["id"] for r in rep.json()["items"]] - assert pending_rr_id not in ids + assert pending_rr_id in ids async def test_get_role_request_detail_requested_group_for_app_group( @@ -1737,16 +1690,11 @@ async def test_reject_role_request_notify_false_suppresses_completion( assert completed_spy.call_count == 0 -async def test_role_request_assignee_admin_branch_loads_role_target_propagation( +async def test_role_request_assignee_lists_unowned_role_target_for_admin( client: AsyncClient, db: Db, url_for: Any ) -> None: - """The Access-admin branch of the `assignee_user_id` role-requests list - evaluates `effective_constraint(DISALLOW_SELF_ADD_MEMBERSHIP, ...)` over - every pending request's `requested_group`. That key is in - `OWNER_SIDE_COUNTERPART`, so for a `RoleGroup` target the helper reads the - role's `active_role_associated_group_*_mappings` -- all `raise_on_sql`. - Without those loaders the endpoint 500s, and it does so whether or not the - role carries any tags, because propagation is consulted unconditionally. + """A role request whose target is an unowned role is assigned to Access + admins, and the `assignee_user_id` list shows it to them. A `RoleGroup` can become a `RoleRequest.requested_group` in production via `ModifyGroupType` converting a group that already has a pending request.""" @@ -1762,15 +1710,15 @@ async def test_role_request_assignee_admin_branch_loads_role_target_propagation( # produces when it converts a group with a pending role request. target_role = await RoleGroupFactory.create_async() - await RoleRequestFactory.create_async( + pending_rr = await RoleRequestFactory.create_async( requester_user_id=requester.id, requester_role_id=requester_role.id, requested_group_id=target_role.id, request_ownership=False, ) - - db.session.expire_all() + pending_rr_id = pending_rr.id list_url = url_for("api-role-requests.role_requests") rep = await client.get(list_url, params={"assignee_user_id": admin_id, "status": "PENDING"}) assert rep.status_code == 200 + assert pending_rr_id in [r["id"] for r in rep.json()["items"]]