feat(AF-622): out-of-office reviewer delegation - #723
Merged
Conversation
…ts queue GroupReviewController had no @PreAuthorize on any method and DefaultGroupReviewService.listPending had no eligibility filter, so any authenticated user could enumerate every pending grouped request in their org — name, description, risk and submitter included. Only the decision path was guarded, and only by an approver-rule match. Add PERM_QUERY_REVIEW on all three endpoints and in the service, and filter the queue with the same predicate the decision guard uses, so the queue can never list a group the caller would be rejected for. Prerequisite for reviewer delegation (#622): the invariant that a delegation never grants a permission the delegate lacks is unsatisfiable while the flow gates on no permission at all. BEHAVIOUR CHANGE: a user who could review grouped requests before and does not hold QUERY_REVIEW will now be rejected. Because approver eligibility is a union over member review plans and cannot be pushed into SQL, the queue filters after paging, so total_elements is an upper bound — the same trade-off GET /reviews/pending already makes. Also documents the /me/review-delegations and /admin/review-delegations endpoints ahead of their implementation, per CLAUDE.md.
Introduces review_delegations plus on-behalf-of provenance columns on the three decision tables, and the core.api surface the review flows will use: ReviewDelegationLookupService (hot path), ReviewDelegationService (CRUD), DelegatedIdentity, and ReviewCandidate. The table lives in core rather than workflow because all three review modules already depend on core.api, and because findPendingForReviewer — which must filter on these rows — lives in core. A workflow-owned entity would still resolve in that JPQL, since JPQL is a string rather than an import, so Modulith would not catch the coupling. ReviewCandidate exists so eligibility is evaluated per identity as a whole. A flow that checks several predicates must not satisfy one from delegator A and another from delegator B; that would synthesize an identity nobody actually holds. Delegation resolution is non-transitive by construction: the lookup reads the table once and never follows a delegator's own delegations. Enforcing that on write would be defeated by creating the two rows in the other order. Both parties' active flags and the window are re-checked at read time against the injected Clock, never current_timestamp, so a deactivation takes effect immediately without a cleanup job and tests can control time. Scope validation spans modules — an API_CONNECTOR scope resolves against apigov, which already depends on core — so ReviewDelegationScopeResolver inverts it: core collects implementations and each owning module supplies its own. An unregistered kind fails closed. No behaviour change yet; nothing consults the lookup service until the per-module eligibility wiring lands.
Query review (workflow) and API-request review (apigov) now resolve the caller's active delegations and evaluate eligibility per identity. Eligibility is checked per ReviewCandidate as a whole, never by OR-ing the predicates across identities: matching the approver rule as delegator A while matching the datasource-reviewer scope as delegator B would synthesize an identity neither of them holds. The self-approval ban covers both identities. A delegator who submitted the request is dropped from the candidate set rather than rejecting the call outright, so a reviewer who is independently eligible keeps their own authority. One authority, one vote. The UNIQUE (request, reviewer_id, stage) index keeps the acting human to a single decision, but cannot see that a delegator already voted personally, or that a delegate already voted for them — different reviewer_id. A service guard covers both orders. The pending-review JPQL now takes collections and uses an exists subquery instead of a join, dropping select distinct: the join fanned out one row per matching approver rule, which is why the count query was count(distinct q). The submitter exclusion stays scalar — widening it to cover delegators would hide requests the reviewer is eligible for in their own right, and that rule is per-identity. apigov gains approver-based eligibility, which it had none of: any holder of API_REQUEST_REVIEW could decide any pending request. It is opt-in by configuration — a connector with no review plan, or a plan with no approver rules, stays open to any permitted reviewer, because treating 'no plan' as 'nobody is an approver' would make every un-planned connector unreviewable on upgrade. Its permission check also moves into the service, since listPending is reachable from the dashboard module. BEHAVIOUR CHANGE: on a connector whose review plan configures approver rules, reviewers who do not match one can no longer decide its requests. recordRejection/recordChangesRequested gained overloads rather than default methods — a default method delegating to the proxied overload bypasses @transactional and fails with 'No active transaction'.
Grouped-request review folds delegated identities into the union its resolver builds across member plans. A scoped delegation covers a bundle when its resource is one of the members — consistent with that union, and never broader than what the delegator could do themselves. Adds the self-service /me/review-delegations endpoints and a read-only /admin/review-delegations listing behind QUERY_ADMIN, which is what lets an auditor interpret an on_behalf_of entry in a decision trail. Service-layer validation messages resolve through MessageSource rather than being built in Java, with the 13 new keys added to all seven locale files. The delegation cap is a config knob so the per-identity OR-tree the API-review queue builds stays bounded.
Adds the out-of-office card on /profile, a Delegated tag on the review queue, and on-behalf-of attribution in the approval timeline, so a delegated decision never reads as though the delegator acted themselves. Picking a delegate needed a new endpoint: the only user listing is gated on USER_MANAGE, which most reviewers do not hold, so the picker would have been empty for exactly the people the feature is for. /me/review-delegations/candidates returns id, email and display name only — no role, permission, or activity — and reviewers in an organization already see each other's names on timelines and queues. Error handling routes through apiErrorMessage so the server's localized reason survives; the backend names the specific rule that failed, which is more useful than a generic 'invalid delegation'. 37 locale keys across all seven files, and 13 new tests.
Covers the data model and its invariants, the eligibility rules and why each is enforced where it is, the security properties an auditor cares about, the profile card and queue tag, and the two behaviour changes this release makes to grouped-request and API-request review. The website gains an Out-of-office delegation section under Review workflows, with its cfg-review-delegation anchor registered in both frontend/src/config/docs.ts and website/app.js so the in-app 'View docs' buttons resolve. The new e2e spec covers the refusals that are cheap to assert against the bootstrap admin — self-delegation, an inverted window, the caller never appearing in their own candidate list — plus the profile card's empty states and the admin listing. Cross-user eligibility needs a second seeded reviewer, so it stays with the backend integration tests, which already exercise it against real Postgres.
… limit The form declared reason in its values type and sent it on submit, but never rendered an input, so the field was unreachable and the backend's @SiZe(max = 500) had no matching Form.Item rule — a validation-parity gap in the direction that only shows up as a 400 from the server.
backend/docker-compose-dev.yml and the PG18 volume-path edits in docs/09-deployment.md are an unrelated local fix that a broad `git add` pulled in. Restored to main; they belong in their own change.
Blockers: - ApiReviewerNotEligibleException had no @ExceptionHandler, so the headline new authorization check returned 500 with an ERROR-level log instead of the 403 its javadoc promises. Mapped in ApiGovExceptionHandler with an i18n key across all seven locales. - API_CONNECTOR-scoped delegations could never be created: core owns the table but cannot reach the connector catalog, and no apigov resolver existed, so validateScope 422'd every well-formed request while the whole read side branched on a row the write side could not produce. Adds ApiConnectorDelegationScopeResolver, and the frontend picker now offers both kinds instead of hardcoding one. - The one-authority-one-vote guard ran ahead of the idempotent-replay check and dropped the caller's OWN identity, so with min_approvals >= 2 a retry was rejected as ineligible rather than replaying — silently removing the documented wasIdempotentReplay contract, and via the missing handler surfacing as a 500. Decisions the acting user cast themselves are now excluded from the guard; both real cases it exists for still fire. Two tests were passing only on mock states a replay cannot occur in. - '(on behalf of X)' was a hardcoded English string in the approval timeline. Performance: the queue's actionability filter, delegated badge and stage were three full recomputations of the same plan/decisions/routing/scope lookups per row; they are now one pass returning the matched identity. Delegator identities and the admin listing's user lookups are batched. The apigov reach no longer filters to active connectors, which was hiding in-flight requests on a connector deactivated after submission. Also: the config knob moved to a @ConfigurationProperties record under accessflow.core.review-delegation, revoke no longer audits a no-op, delegated_for carries the email and display name the API spec promised, findActiveDelegateIds is gone (no caller, and its javadoc claimed one), and docs/04-api-spec.md's error table, revoke semantics, grouped-request scoping and response shapes now match the code. e2e: the new profile card's info Alert had silently defeated profile-api-keys.spec.ts's page-wide role=alert assertion — scoped to the API-keys card. reviews-delegation.spec.ts now seeds its own delegate via the invitation helpers, so create/revoke is actually covered instead of self-skipping.
revoke now reports whether it actually revoked, so the controller can skip auditing a no-op. A Mockito mock returns false by default, which made the test assert an audit row that the controller was correctly not writing. Adds the no-op case explicitly.
The admin listing's search() uses the (:param is null or column = :param) form, which can trip Hibernate's parameter-type inference on PostgreSQL for UUID binds — a runtime-only failure that the mocked unit tests cannot see. Also pins the half-open window boundary, the both-parties-active filter, the scope PG enum round-trip, and what countOpenForDelegator ignores. Drops the ReviewDelegationPage type, orphaned when the admin API module function was removed.
Contributor
Contributor
Coverage Report for Frontend Coverage (frontend)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Backend Test Results6 279 tests 6 279 ✅ 19m 27s ⏱️ Results for commit 9b4c315. ♻️ This comment has been updated with latest results. |
Contributor
The profile-page test asserted the delegate's display name, but inviteUserViaApi's no-roleId path posts a camelCase displayName that the snake_case API ignores, so an invited user has none and the table falls back to the email. The card, the delegation and the revoke flow all rendered correctly — only the assertion was wrong. Also confirms revocation by the row's action disappearing rather than by matching 'Revoked' text, which is ambiguous once retries leave several retired rows behind, and switches the Popconfirm to the primary-button idiom the other specs use.
The coverage report showed ReviewDelegateCandidate and DelegateCandidateResponse at 0% — the whole delegate-picker path shipped untested, despite listDelegateCandidates carrying real logic: it excludes the caller, drops deactivated members, and sorts by display name falling back to email for invited users who have none (which would NPE a naive comparator). Also pins ApiReviewerNotEligibleException to 403. Without that mapping the feature's own authorization check falls through to the global catch-all and answers 500 with an ERROR-level log on every denial — the exact regression the review caught, now guarded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements out-of-office reviewer delegation from #622. This is PR 1 of 2 — idle-reviewer escalation and nudge reminders follow in a second PR, which will close the issue.
A reviewer names a delegate for a window. During it the delegate is an eligible approver everywhere the delegator was — query review, governed API requests, and grouped requests — and every decision records both identities plus the delegation that authorised it.
Safety properties, enforced in the service layer
Permissioncheck runs before delegation is resolved, so it widens which requests an already-permitted reviewer may act on, never whether they may review.ReviewCandidate— matching an approver rule as delegator A while matching the datasource-reviewer scope as delegator B would synthesize an identity nobody holds.UNIQUE (request, reviewer_id, stage)index stays keyed on the acting reviewer, so a delegate covering two absent approvers cannot satisfymin_approvals = 2alone. A service guard covers what the index cannot see — the delegator voting personally and their delegate then voting for them, in either order.Behaviour changes
Three, all deliberate and documented in
docs/07-security.md:QUERY_REVIEW.GroupReviewControllerhad no@PreAuthorizeon any method andlistPendinghad no eligibility filter, so any authenticated user could enumerate every pending bundle in their org. Landed as the branch's first, independently revertible commit — a user who could review bundles before and lacks the permission will now be rejected.API_REQUEST_REVIEWcould decide any pending request. Opt-in by configuration — a connector with no review plan, or a plan with no approver rules, behaves exactly as before, because treating "no plan" as "nobody is an approver" would make every un-planned connector unreviewable on upgrade.total_elementson the grouped-request queue is now an upper bound. Approver eligibility there is a union over member review plans and cannot be pushed into SQL, so filtering happens after paging — the same trade-offGET /reviews/pendingalready makes.Schema
V142createsreview_delegations;V143adds nullableon_behalf_of_user_id+delegation_idtoreview_decisions,api_review_decisionsandgroup_review_decisions. Both are additive, no existing migration touched, noALTER TYPE … ADD VALUE.The table lives in
core, notworkflowas the issue suggested: all three review modules already depend oncore.api, andQueryRequestRepository.findPendingForReviewer— which must filter on these rows — lives incore. A workflow-owned entity would still resolve in that JPQL and Spring Modulith would not catch it, since JPQL is a string rather than an import.Docs updated
docs/03-data-model.md,docs/04-api-spec.md,docs/05-backend.md,docs/06-frontend.md,docs/07-security.md,docs/09-deployment.md,docs/12-roadmap.md,README.md,website/index.html,website/docs/configuration/review-workflows/index.html(+ thecfg-review-delegationanchor in bothfrontend/src/config/docs.tsandwebsite/app.js),website/README.md,website/sitemap.xml.Review notes
Four independent reviewers ran before this PR (
af-verifier,af-reviewer,af-java-reviewer,af-frontend-reviewer). Every blocker was fixed inee29b487; these are the surviving concerns a human reviewer should weigh.Deferred by design
GroupReviewPlanResolverissues a lookup per member — a 20-row page of 5-member bundles is materially more queries than before. The alternative is a per-page plan cache; the correctness fix (the queue previously leaked every bundle in the org) was the priority here, and the shape is now identical to the query-review queue's.apigovstill hard-codesSTAGE = 1, so an approver rule withstage > 1on a connector's plan is unreachable. Pre-existing; now visible because approver rules are read at all. Worth its own issue.Addressed after review
ReviewDelegationRepository.search's(:param is null or …)pattern had no integration test, so a Hibernate UUID parameter-inference failure would have surfaced only at runtime on the admin listing. Now covered byReviewDelegationRepositoryIntegrationTestagainst real PostgreSQL, along with the half-open window boundary, the both-parties-active filter and the scope PG-enum round-trip.Known gaps, called out in the API spec rather than papered over
delegated_forbadge field; only/reviews/pendingdoes. Likewiseapi_review_decisions/group_review_decisionspersist the provenance columns but their read models don't expose them yet.Not covered by e2e
Adjacent defect found, deliberately not fixed here
Permission.REVIEW_OVERRIDEis documented as "always an eligible approver" and honoured inaccess,lifecycle,attestation,requestgroupsandDefaultQueryCollaborationAccessService— but not inDefaultReviewService, so an admin not named in a review plan cannot approve a query. It sits on the exact method this PR modifies, but fixing it changes who may approve and is not part of this issue.Refs #622