[i,l,-r] Auto-narrow reviewers on empty picker selection (SECOP-952)#11
Merged
Merged
Conversation
When an MPA elevation request was submitted without picking specific
reviewers, JoinOperation.propose fell back to the full policy ACL and
SlackProposalHandler expanded any group in it to individuals — so a
role whose approver list names a broad group (e.g.
engineering@roles.wave.com) DM'd hundreds of people.
GroupsResource.post now resolves an effective reviewer filter when the
selection is empty:
- small, group-free approver set -> unchanged (DM everyone; no Cloud
Identity calls);
- broad set (contains a group, or > AUTO_NARROW_BROADCAST_LIMIT) ->
narrow to the requester's suggested teammates from
ReviewerCandidates;
- broad set with no teammate signal -> reject with a BadRequest so the
requester picks at least one reviewer, rather than broadcasting.
Same fail-safe if teammate resolution errors out.
Adds three TestGroupsResource cases covering the null-filter, narrow,
and reject paths.
phosphore
changed the title
Correctness fixes: - Resolve the reviewer filter inside the requiresApproval branch, so self-approve joins never pay reviewer resolution (or its failure modes) for a filter that would go unused. - notifyReviewers=false (copy-link) bypasses auto-narrow: it sends no DMs at all, so there is no broadcast to narrow; the pasted link keeps working for any qualified approver on broad ACLs. - Rate-limit the auto-narrow Cloud Identity fan-out with the same per-user limiter as GET /reviewers (blocking up to 1 s, then 429) so the POST path is not an unthrottled back door to the quota. - Catch UncheckedExecutionException from GroupResolver.expand; group expansion failures now hit the designed fail-safe instead of a 500. - Exclude the requester from the approver set BEFORE the broadcast-size guard (off-by-one: an ACL at the limit plus the requester was taking the expensive path). - On teammate-resolution failure with a group-FREE ACL, degrade to the bounded pre-SECOP-952 broadcast (null filter) instead of rejecting: the named approvers are already enumerated in the policy, and the "please pick someone" guidance is circular while the picker is degraded by the same outage. Group-bearing ACLs still reject. - Narrow to ReviewerCandidates' new uncapped `teammate` flag (score>0, capped at AUTO_NARROW_BROADCAST_LIMIT) instead of the `suggested` badge: the badge is a UI top-3 cap, and an audience of 3 was fragile (a couple of OOO teammates would strand the request) and coupled production fan-out to a presentation constant. Cleanups from the same review: - Extract approversFromPolicy()/reviewerCandidates() helpers (the qualified-approver extraction existed in three diverging copies). - Fix stale SELECTED_REVIEWERS_MAX javadoc (referenced the removed legacy DM-everyone path) and add the auto-narrow entry to GroupResolver's caller-audit list. Tests: 6 new TestGroupsResource cases (self-approve skip, copy-link bypass, requester-at-limit boundary, group-ACL narrowing, group expansion failure -> 400, group-free outage fallback -> null filter).
…uto-narrow-reviewers
phosphore
added a commit
that referenced
this pull request
Jul 9, 2026
…te, picker copy (SECOP-1093/1094/1095) (#13) * picker: update helper copy for auto-narrowed empty selection (SECOP-1095) Since SECOP-952 (#11) an empty selection no longer notifies every qualified peer on broad ACLs — it narrows to the requester's closest teammates (small group-free sets still notify everyone). Update the three places in the picker UI that still described the old behaviour: the hint text, the default-unchecked comment, and the degraded-picker notice. * slack: update the approver's own DM after they approve (SECOP-1094) The sibling-update loop skipped the approver, leaving their DM frozen with a stale action link. In the picked-single-reviewer flow (sole recipient) nothing changed on screen at all, which read as a failed approval (live-test feedback, 2026-07-09). Every recorded DM is now updated in place: siblings get the existing 'already approved by X' blocks, the approver gets a new 'you approved this request' variant. * security: make proposal approval JWTs single-use (SECOP-1093) Proposal tokens are stateless signed JWTs (~1h validity) with no consumption record, so one approval link authorized unlimited re-grants for the token lifetime, each resetting the membership clock — observed live during the SECOP-952 validation (an approver double-clicked and re-granted a lapsed 15-minute membership). - Proposal.id() surfaces the JWT jti (crypto-random, minted at propose). - ProposalHandler gains verifyNotConsumed/markConsumed hooks; defaults are no-ops so the mail/debug handlers keep upstream semantics. - SlackProposalHandler enforces via SlackMessageRegistry: consumption markers live in the same Firestore collection as DM registry entries (HMAC-keyed under the existing salt, namespaced with a "consumed|" prefix, reaped by the existing expires_at TTL policy at token expiry). - ProposalResource checks consumption on GET (used link renders the "URL no longer valid" banner) and POST (before the approval executes), and records it right after a successful approval. - Failure mode is deliberately fail-open with ERROR logs: the JWT signature remains the authoritative authorization; consumption is anti-replay defense-in-depth and approvals must not hard-depend on Firestore availability. * build: bump version to 2.3.0-wavemm.9
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.
Why
When an MPA elevation request is submitted without picking specific reviewers,
JoinOperation.proposefalls back to the full policy ACL andSlackProposalHandlerexpands any group in it to individuals. So a role whose approver list names a broad group (e.g.engineering@roles.wave.com) DMs hundreds of people — far too loud. Tracked in SECOP-952.What
When a join requires approval and the picker selection is empty,
GroupsResource.postresolves an effective reviewer filter:AUTO_NARROW_BROADCAST_LIMITafter excluding the requester, no group): unchanged — DM everyone. Makes zero Cloud Identity calls (cheap guard).*@roles.wave.comgroup, viaReviewerCandidates' new uncappedteammateflag), highest-affinity first, capped at the limit. Never fans out to the whole group.BadRequest("pick at least one reviewer") rather than broadcast.Deliberate non-narrowing paths:
requiresApproval=false) never touch reviewer resolution — resolved only inside the approval branch.notifyReviewers=false) bypasses narrowing — it sends no DMs at all, and the pasted link must keep working for any qualified approver.GroupResolver.expandwraps the failure inUncheckedExecutionException.GET /reviewers' per-user rate limiter (blocking up to 1 s, then 429), so the POST path is not an unthrottled back door to the Cloud Identity quota.Review
A max-effort adversarial review ran against the first revision (6 finder angles, independent verification of all 33 candidate findings); the 13 confirmed defects — early/eager resolution breaking self-approve and copy-link, the badge-cap coupling, the missing rate limit, the
UncheckedExecutionExceptionbypass, an off-by-one in the size guard, the circular outage fail-safe, a test gap on group-bearing ACLs, and stale docs — are all addressed in the second commit.Tests
Nine
TestGroupsResourcecases covering: null-filter (small set), narrow-to-teammates, reject-no-teammates, self-approve skip, copy-link bypass, requester-at-limit boundary, group-ACL narrowing (theengineering@headline scenario), group expansion failure → 400, and group-free outage fallback → null filter. Full unit suite green locally (1017 tests, 0 failures).Notes / follow-ups
view.js) — out of scope here.AUTO_NARROW_BROADCAST_LIMIT = 15is a judgment call; trivial to tune.🤖 Generated with Claude Code