From 8fb7f442b0af91ce5d1a45f2bdad673f72f3c62e Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Mon, 22 Jun 2026 15:38:58 +0100 Subject: [PATCH 1/3] Auto-narrow reviewers on empty picker selection (SECOP-952) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../jitaccess/web/rest/GroupsResource.java | 145 ++++++++++++++- .../web/rest/TestGroupsResource.java | 167 ++++++++++++++++++ 2 files changed, 305 insertions(+), 7 deletions(-) diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java index 923ebe9d..7ce28e74 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java @@ -172,13 +172,24 @@ public class GroupsResource { inputValues.remove(FIELD_NOTIFY_REVIEWERS)); // - // When the requester picked specific reviewers, validate every - // email is in the expanded qualified-peer set BEFORE calling - // propose. JoinOperation.propose trusts the filter is already - // an authorised subset (it short-circuits the recipients list - // to whatever we pass), so the security check needs to happen - // here where we have a GroupResolver to expand groups. + // Resolve the *effective* reviewer filter handed to + // JoinOperation.propose (ProposeOptions.reviewerFilter): // + // - Requester picked reviewers → validate every email is in the + // expanded qualified-peer set BEFORE calling propose. + // JoinOperation.propose trusts the filter is already an + // authorised subset (it short-circuits the recipients list to + // whatever we pass), so the security check has to happen here + // where we have a GroupResolver to expand groups. + // + // - Requester picked nobody → auto-narrow (SECOP-952). Rather + // than letting propose fall back to the entire policy ACL — + // which, when it names a broad group like + // engineering@roles.wave.com, expands to hundreds of Slack DMs + // — narrow to the requester's suggested teammates. We never + // broadcast to a large group on an empty selection. + // + Set effectiveReviewers; if (selectedReviewers != null && !selectedReviewers.isEmpty()) { var qualified = group.policy().effectiveAccessControlList() .allowedPrincipals(PolicyPermission.APPROVE_OTHERS.toMask()) @@ -202,6 +213,10 @@ public class GroupsResource { "Selected reviewers are not authorised to approve this request: " + String.join(", ", rejected)); } + effectiveReviewers = selectedReviewers; + } + else { + effectiveReviewers = autoNarrowedReviewers(group, groupId); } // @@ -234,7 +249,7 @@ public class GroupsResource { joinOp, buildActionUri, new ProposalHandler.ProposeOptions( - selectedReviewers, + effectiveReviewers, notifyReviewers)); // Plumb the notifyReviewers flag into the audit event so the @@ -370,6 +385,122 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { return Boolean.parseBoolean(raw.get(raw.size() - 1)); } + /** + * Largest expanded approver set we'll DM in full on an empty picker + * selection (SECOP-952). At or below this many individual reviewers, + * "notify everyone" is a small-team broadcast and stays the upstream + * default; above it — with no teammate to narrow to — we refuse to + * broadcast and require an explicit pick instead. + * + *

Sized to a generous single team: Wave teams are typically <15 + * (see {@link ReviewerCandidates#SUGGESTION_GROUP_MAX_SIZE}), so a set + * larger than this almost always means a broad group like + * {@code engineering@} crept into the ACL. + */ + static final int AUTO_NARROW_BROADCAST_LIMIT = 15; + + /** + * Compute the reviewer filter for an empty picker selection + * (SECOP-952). The requester didn't pick anyone, so instead of + * letting {@link JitGroupContext.JoinOperation#propose} fall back to + * the entire policy ACL — which DMs every member of any group in it — + * we narrow to the requester's likely teammates. + * + *

Returns: + *

    + *
  • {@code null} — no narrowing; {@code propose} uses the full + * policy ACL. Used when the approver set is small and contains + * no group, i.e. a handful of named individuals where DMing + * everyone isn't "loud". This path makes no Cloud Identity + * calls. + *
  • a non-empty subset — the requester's suggested teammates + * ({@link ReviewerCandidates.Candidate#suggested()}), used when + * the ACL contains a group (or many direct approvers) so we + * avoid broadcasting to the whole group. + *
+ * + *

Throws {@link BadRequestException} when the approver set is large + * AND no teammate can be auto-selected: we won't silently DM everyone, + * so we ask the requester to pick at least one reviewer. That is the + * only case an empty selection is rejected — it preserves the + * "never broadcast to a large group" guarantee without forcing a pick + * in the common small-team / clear-teammate cases. The same + * fail-safe applies if teammate resolution errors out (Cloud Identity + * outage / permission gap): we'd rather ask for a pick than broadcast. + */ + private @Nullable Set autoNarrowedReviewers( + @NotNull JitGroupContext group, + @NotNull JitGroupId groupId + ) { + var rawApprovers = group.policy().effectiveAccessControlList() + .allowedPrincipals(PolicyPermission.APPROVE_OTHERS.toMask()) + .stream() + .filter(p -> p instanceof com.google.solutions.jitaccess.auth.IamPrincipalId) + .map(p -> (com.google.solutions.jitaccess.auth.IamPrincipalId) p) + .collect(Collectors.toSet()); + + var hasGroupApprover = rawApprovers.stream() + .anyMatch(com.google.solutions.jitaccess.auth.GroupId.class::isInstance); + + // + // Cheap guard: no group in the ACL and only a few named approvers → + // DMing all of them is not a broad broadcast. Keep the upstream + // DM-everyone behaviour and skip the Cloud Identity round-trips + // entirely (this also keeps no-approval / no-approver flows from + // touching the groups client at all). + // + if (rawApprovers.isEmpty() + || (!hasGroupApprover && rawApprovers.size() <= AUTO_NARROW_BROADCAST_LIMIT)) { + return null; + } + + var requester = this.subject.user(); + var qualified = rawApprovers.stream() + .filter(p -> !p.equals(requester)) + .collect(Collectors.toCollection(HashSet::new)); + + List candidates; + try { + var resolver = new GroupResolver(this.groupsClient, this.executor); + candidates = new ReviewerCandidates(resolver, this.groupsClient, this.executor) + .compute(requester, qualified); + } + catch (com.google.solutions.jitaccess.apis.clients.AccessException + | java.io.IOException e) { + // Fail safe = never broadcast. If teammates can't be computed, + // don't silently DM the whole group — require an explicit pick. + this.logger.warn( + EventIds.API_JOIN_GROUP, + "Auto-narrow of reviewers failed for %s on %s; requiring an " + + "explicit selection rather than broadcasting. cause=%s", + requester.email, groupId, e); + throw new BadRequestException( + "Couldn't determine close-team reviewers automatically. Please " + + "select at least one reviewer for this request."); + } + + var suggested = candidates.stream() + .filter(ReviewerCandidates.Candidate::suggested) + .map(c -> new EndUserId(c.email())) + .collect(Collectors.toSet()); + + if (!suggested.isEmpty()) { + return suggested; + } + + // + // No teammate signal at all. If the expanded set is still small, + // DMing everyone is acceptable; otherwise refuse to broadcast. + // + if (candidates.size() <= AUTO_NARROW_BROADCAST_LIMIT) { + return null; + } + throw new BadRequestException( + "This role's approver list expands to " + candidates.size() + + " people and none share a team with you. Please pick at least " + + "one reviewer so we don't notify everyone."); + } + /** * Per-user rate limit on {@link #getReviewers}. Each {@link * ReviewerCandidates#compute} call is expensive — one diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java index 4e043116..cb6c2845 100644 --- a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java @@ -21,10 +21,14 @@ package com.google.solutions.jitaccess.web.rest; +import com.google.api.services.cloudidentity.v1.model.EntityKey; +import com.google.api.services.cloudidentity.v1.model.Membership; +import com.google.api.services.cloudidentity.v1.model.MembershipRelation; import com.google.solutions.jitaccess.apis.Logger; import com.google.solutions.jitaccess.apis.OrganizationId; import com.google.solutions.jitaccess.apis.ProjectId; import com.google.solutions.jitaccess.apis.clients.AccessDeniedException; +import com.google.solutions.jitaccess.apis.clients.CloudIdentityGroupsClient; import com.google.solutions.jitaccess.apis.clients.GroupKey; import com.google.solutions.jitaccess.auth.*; import com.google.solutions.jitaccess.catalog.*; @@ -609,6 +613,169 @@ public void post_whenNotifyReviewersFalse_passesOptionToProposeAndReturnsApprova "copy-link mode + JOIN_PROPOSED must surface the approval URL"); } + // ------------------------------------------------------------------------- + // Wavemm fork: auto-narrow on empty selection (SECOP-952). + // ------------------------------------------------------------------------- + + /** + * SECOP-952: a small, group-free approver set on an empty picker + * selection keeps the upstream "DM everyone" behaviour — propose is + * called with a {@code null} reviewer filter, and we don't touch the + * Cloud Identity groups client at all. + */ + @Test + public void post_whenNoReviewersPickedAndApproverSetSmall_proposesWithNullFilter() + throws Exception { + var group = Policies.createJitGroupPolicy( + "g-1", + new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(SAMPLE_APPROVING_USER, PolicyPermission.APPROVE_OTHERS.toMask()) + .build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + when(resource.proposalHandler.propose(any(), any(), any())) + .thenReturn(new ProposalHandler.ProposalToken( + "token", Set.of(SAMPLE_APPROVING_USER), Instant.MAX)); + + resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>()); + + var captor = org.mockito.ArgumentCaptor.forClass( + ProposalHandler.ProposeOptions.class); + verify(resource.proposalHandler).propose(any(), any(), captor.capture()); + assertNull(captor.getValue().reviewerFilter(), + "a small group-free approver set must not be narrowed"); + + // Cheap-path guarantee: no Cloud Identity round-trips for the + // common small-team case. + verifyNoInteractions(resource.groupsClient); + } + + /** + * SECOP-952 core fix: on an empty selection, when the approver set is + * large enough to be a broadcast, narrow to the requester's suggested + * teammates rather than DMing the whole set. Here two of the many + * direct approvers share a small group with the requester, so propose + * receives exactly those two. + */ + @Test + public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToSuggested() + throws Exception { + var teamMate1 = new EndUserId("teammate-1@example.com"); + var teamMate2 = new EndUserId("teammate-2@example.com"); + + // A broad, group-free approver set (> AUTO_NARROW_BROADCAST_LIMIT) + // so the auto-narrow path engages. + var acl = new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(teamMate1, PolicyPermission.APPROVE_OTHERS.toMask()) + .allow(teamMate2, PolicyPermission.APPROVE_OTHERS.toMask()); + for (int i = 0; i < 20; i++) { + acl.allow(new EndUserId("approver-" + i + "@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()); + } + var group = Policies.createJitGroupPolicy( + "g-1", + acl.build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + when(resource.proposalHandler.propose(any(), any(), any())) + .thenReturn(new ProposalHandler.ProposalToken( + "token", Set.of(teamMate1), Instant.MAX)); + + // The requester shares one small team group with teamMate1/2. + var teamGroup = new GroupId("team@example.com"); + when(resource.groupsClient.listMembershipsByUser(eq(SAMPLE_USER))) + .thenReturn(List.of(new MembershipRelation() + .setGroupKey(new EntityKey().setId(teamGroup.email)))); + when(resource.groupsClient.listMemberships(eq(teamGroup))) + .thenReturn(List.of( + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId(teamMate1.email)), + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId(teamMate2.email)))); + + resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>()); + + var captor = org.mockito.ArgumentCaptor.forClass( + ProposalHandler.ProposeOptions.class); + verify(resource.proposalHandler).propose(any(), any(), captor.capture()); + assertEquals( + Set.of(teamMate1, teamMate2), + captor.getValue().reviewerFilter(), + "empty selection on a broad set must narrow to suggested teammates"); + } + + /** + * SECOP-952 safety net: on an empty selection, when the approver set + * is large AND the requester shares no team with anyone, we refuse to + * broadcast — the submission is rejected so the requester picks at + * least one reviewer. propose is never reached. + */ + @Test + public void post_whenNoReviewersPickedAndNoTeammates_rejectsRatherThanBroadcast() + throws Exception { + var acl = new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()); + for (int i = 0; i < 20; i++) { + acl.allow(new EndUserId("approver-" + i + "@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()); + } + var group = Policies.createJitGroupPolicy( + "g-1", + acl.build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + // Requester shares no group with anyone → no suggestions. + when(resource.groupsClient.listMembershipsByUser(any())) + .thenReturn(List.of()); + + assertThrows( + jakarta.ws.rs.BadRequestException.class, + () -> resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>())); + + verify(resource.proposalHandler, never()).propose(any(), any(), any()); + } + //--------------------------------------------------------------------------- // getReviewers — rate limit (wavemm fork P1-4). //--------------------------------------------------------------------------- From 0385b56bc9e357f6decc346b083c71027b7442b9 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Thu, 9 Jul 2026 11:41:12 +0200 Subject: [PATCH 2/3] Address max-effort review findings on auto-narrow (SECOP-952) 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). --- .../jitaccess/auth/GroupResolver.java | 8 + .../web/proposal/ReviewerCandidates.java | 14 +- .../jitaccess/web/rest/GroupsResource.java | 288 ++++++++++------ .../web/rest/TestGroupsResource.java | 325 +++++++++++++++++- 4 files changed, 529 insertions(+), 106 deletions(-) diff --git a/sources/src/main/java/com/google/solutions/jitaccess/auth/GroupResolver.java b/sources/src/main/java/com/google/solutions/jitaccess/auth/GroupResolver.java index b3dca246..c31c4a12 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/auth/GroupResolver.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/auth/GroupResolver.java @@ -72,6 +72,14 @@ *

  • {@link com.google.solutions.jitaccess.web.rest.GroupsResource#post} * — selectedReviewers subset check: combines {@code expand} with * a separate ACL lookup, NOT a sole authorisation source. ✓ + *
  • {@code GroupsResource#autoNarrowedReviewers} (also reached + * from {@code post}) — empty-selection auto-narrow (SECOP-952): + * its output becomes {@code ProposeOptions.reviewerFilter}, i.e. + * the proposal recipient set. Degraded {@code expand} output can + * only SHRINK that set (silently-dropped members yield fewer + * teammates), never widen it beyond the qualified peers, so the + * failure mode is fail-closed: fewer people notified/able to + * approve, backstopped by the catalog-side subset defense. ✓ *
  • {@link com.google.solutions.jitaccess.web.rest.GroupsResource#getReviewers} * — picker candidate listing: best-effort, output is for UX * only. ✓ diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ReviewerCandidates.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ReviewerCandidates.java index 5c047e3c..74e18b33 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ReviewerCandidates.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ReviewerCandidates.java @@ -163,7 +163,8 @@ public ReviewerCandidates( .map(u -> new Candidate( u.email, u.email, // displayName: same as email until we wire Directory.users.get - badgeSet.contains(u))) + badgeSet.contains(u), + teamScore.getOrDefault(u, 0) > 0)) .toList(); } @@ -276,11 +277,18 @@ private record GroupMembers(@NotNull GroupId group, @NotNull List me * Directory API for actual display names * @param suggested true if this candidate is in the top * {@link #SUGGESTED_BADGE_TOP_N} ranked teammates - * (rendered with the "team" badge in the UI) + * (rendered with the "team" badge in the UI). A + * presentation cap — do not derive dispatch + * decisions from it; use {@link #teammate()}. + * @param teammate true if this candidate shares at least one small + * group with the requester (team score > 0), + * uncapped. This is the signal the empty-selection + * auto-narrow (SECOP-952) dispatches DMs on. */ public record Candidate( @NotNull String email, @NotNull String displayName, - boolean suggested + boolean suggested, + boolean teammate ) {} } diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java index 7ce28e74..eb5a2e41 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java @@ -24,11 +24,15 @@ import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.util.concurrent.RateLimiter; +import com.google.common.util.concurrent.UncheckedExecutionException; import com.google.solutions.jitaccess.apis.Logger; import com.google.solutions.jitaccess.apis.clients.AccessDeniedException; +import com.google.solutions.jitaccess.apis.clients.AccessException; import com.google.solutions.jitaccess.apis.clients.CloudIdentityGroupsClient; import com.google.solutions.jitaccess.auth.EndUserId; +import com.google.solutions.jitaccess.auth.GroupId; import com.google.solutions.jitaccess.auth.GroupResolver; +import com.google.solutions.jitaccess.auth.IamPrincipalId; import com.google.solutions.jitaccess.auth.JitGroupId; import com.google.solutions.jitaccess.auth.Principal; import com.google.solutions.jitaccess.auth.Subject; @@ -53,6 +57,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.io.IOException; import java.net.URI; import java.time.Duration; import java.time.Instant; @@ -172,34 +177,17 @@ public class GroupsResource { inputValues.remove(FIELD_NOTIFY_REVIEWERS)); // - // Resolve the *effective* reviewer filter handed to - // JoinOperation.propose (ProposeOptions.reviewerFilter): + // When the requester picked specific reviewers, validate every + // email is in the expanded qualified-peer set BEFORE calling + // propose. JoinOperation.propose trusts the filter is already + // an authorised subset (it short-circuits the recipients list + // to whatever we pass), so the security check needs to happen + // here where we have a GroupResolver to expand groups. // - // - Requester picked reviewers → validate every email is in the - // expanded qualified-peer set BEFORE calling propose. - // JoinOperation.propose trusts the filter is already an - // authorised subset (it short-circuits the recipients list to - // whatever we pass), so the security check has to happen here - // where we have a GroupResolver to expand groups. - // - // - Requester picked nobody → auto-narrow (SECOP-952). Rather - // than letting propose fall back to the entire policy ACL — - // which, when it names a broad group like - // engineering@roles.wave.com, expands to hundreds of Slack DMs - // — narrow to the requester's suggested teammates. We never - // broadcast to a large group on an empty selection. - // - Set effectiveReviewers; if (selectedReviewers != null && !selectedReviewers.isEmpty()) { - var qualified = group.policy().effectiveAccessControlList() - .allowedPrincipals(PolicyPermission.APPROVE_OTHERS.toMask()) - .stream() - .filter(p -> !p.equals(this.subject.user())) - .filter(p -> p instanceof com.google.solutions.jitaccess.auth.IamPrincipalId) - .map(p -> (com.google.solutions.jitaccess.auth.IamPrincipalId) p) - .collect(Collectors.toCollection(HashSet::new)); - var resolver = new GroupResolver(this.groupsClient, this.executor); - var allowedEmails = new ReviewerCandidates(resolver, this.groupsClient, this.executor) + var qualified = approversFromPolicy(group); + qualified.remove(this.subject.user()); + var allowedEmails = reviewerCandidates() .compute(this.subject.user(), qualified) .stream() .map(c -> c.email().toLowerCase()) @@ -213,10 +201,6 @@ public class GroupsResource { "Selected reviewers are not authorised to approve this request: " + String.join(", ", rejected)); } - effectiveReviewers = selectedReviewers; - } - else { - effectiveReviewers = autoNarrowedReviewers(group, groupId); } // @@ -226,6 +210,35 @@ public class GroupsResource { Inputs.copyValues(inputValues, joinOp.input()); if (joinOp.requiresApproval()) { + // + // Resolve the reviewer filter handed to propose + // (ProposeOptions.reviewerFilter). Resolved here — after + // requiresApproval is known — so self-approve joins never pay + // reviewer resolution (or its failure modes) for a filter + // that would go unused. + // + // - Reviewers picked → use them (validated above). + // - notifyReviewers=false (copy-link) → nobody is DM'd at + // all, so there is no broadcast to narrow; keep the + // upstream full-ACL recipient semantics so the pasted + // link works for any qualified approver. + // - Nothing picked → auto-narrow (SECOP-952). Rather than + // letting propose fall back to the entire policy ACL — + // which, when it names a broad group like + // engineering@roles.wave.com, expands to hundreds of + // Slack DMs — narrow to the requester's teammates. + // + Set effectiveReviewers; + if (selectedReviewers != null && !selectedReviewers.isEmpty()) { + effectiveReviewers = selectedReviewers; + } + else if (!notifyReviewers) { + effectiveReviewers = null; + } + else { + effectiveReviewers = autoNarrowedReviewers(group, groupId); + } + // // Approval required, propose to someone else. // @@ -300,10 +313,11 @@ public class GroupsResource { /** * Cap on the number of selected reviewers a single picker submission - * can carry. Sane upper bound — ACLs that grant APPROVE_OTHERS to - * groups larger than this number still pass through the legacy "DM - * everyone" path because the picker can't fit them all on screen - * anyway. + * can carry — a sane upper bound on client-supplied fan-out (the + * picker can't usefully render a selection this large anyway). What + * happens when NOTHING is selected is governed separately by + * {@link #autoNarrowedReviewers} and + * {@link #AUTO_NARROW_BROADCAST_LIMIT}. */ static final int SELECTED_REVIEWERS_MAX = 50; @@ -386,11 +400,18 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { } /** - * Largest expanded approver set we'll DM in full on an empty picker - * selection (SECOP-952). At or below this many individual reviewers, - * "notify everyone" is a small-team broadcast and stays the upstream - * default; above it — with no teammate to narrow to — we refuse to - * broadcast and require an explicit pick instead. + * Largest approver set (requester excluded) we'll DM in full on an + * empty picker selection (SECOP-952). At or below this many + * individual reviewers, "notify everyone" is a small-team broadcast + * and stays the upstream default; above it we narrow to teammates, + * and — with no teammate to narrow to — refuse to broadcast and + * require an explicit pick instead. + * + *

    Also caps how many teammates the auto-narrowed filter itself + * carries. Deliberately distinct from {@link + * ReviewerCandidates#SUGGESTED_BADGE_TOP_N}, which is a UI + * presentation cap: tying the DM audience to the badge would let a + * cosmetic tweak silently change production notification fan-out. * *

    Sized to a generous single team: Wave teams are typically <15 * (see {@link ReviewerCandidates#SUGGESTION_GROUP_MAX_SIZE}), so a set @@ -400,75 +421,124 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { static final int AUTO_NARROW_BROADCAST_LIMIT = 15; /** - * Compute the reviewer filter for an empty picker selection - * (SECOP-952). The requester didn't pick anyone, so instead of - * letting {@link JitGroupContext.JoinOperation#propose} fall back to - * the entire policy ACL — which DMs every member of any group in it — - * we narrow to the requester's likely teammates. + * Compute the reviewer filter for an empty picker selection on a + * join that requires approval (SECOP-952). The requester didn't pick + * anyone, so instead of letting {@link + * JitGroupContext.JoinOperation#propose} fall back to the entire + * policy ACL — which DMs every member of any group in it — we narrow + * to the requester's teammates. * *

    Returns: *

      *
    • {@code null} — no narrowing; {@code propose} uses the full - * policy ACL. Used when the approver set is small and contains - * no group, i.e. a handful of named individuals where DMing - * everyone isn't "loud". This path makes no Cloud Identity - * calls. - *
    • a non-empty subset — the requester's suggested teammates - * ({@link ReviewerCandidates.Candidate#suggested()}), used when + * policy ACL. Used when the approver set (requester excluded) + * is small and contains no group, i.e. a handful of named + * individuals where DMing everyone isn't "loud". This path + * makes no Cloud Identity calls. Also the degraded outcome for + * group-free ACLs when Cloud Identity is unavailable — the + * named individuals are already enumerated in the policy, so + * the bounded broadcast beats dead-ending the requester whose + * picker is degraded too. + *
    • a non-empty subset — the requester's teammates + * ({@link ReviewerCandidates.Candidate#teammate()}, i.e. + * sharing at least one small group), highest-affinity first, + * capped at {@link #AUTO_NARROW_BROADCAST_LIMIT}. Used when * the ACL contains a group (or many direct approvers) so we * avoid broadcasting to the whole group. *
    * - *

    Throws {@link BadRequestException} when the approver set is large - * AND no teammate can be auto-selected: we won't silently DM everyone, - * so we ask the requester to pick at least one reviewer. That is the - * only case an empty selection is rejected — it preserves the - * "never broadcast to a large group" guarantee without forcing a pick - * in the common small-team / clear-teammate cases. The same - * fail-safe applies if teammate resolution errors out (Cloud Identity - * outage / permission gap): we'd rather ask for a pick than broadcast. + *

    Throws {@link BadRequestException} when we won't pick for the + * requester and won't broadcast either: a large approver set with no + * teammate signal, or a group-bearing ACL whose expansion failed + * (a group's membership is unknowable without Cloud Identity, so + * there is no bounded fallback). Throws a 429 + * {@link WebApplicationException} when the per-user rate limit is + * exhausted — this path performs the same Cloud Identity fan-out + * that {@link #getReviewers} throttles, and must not become an + * unthrottled back door to it. */ private @Nullable Set autoNarrowedReviewers( @NotNull JitGroupContext group, @NotNull JitGroupId groupId ) { - var rawApprovers = group.policy().effectiveAccessControlList() - .allowedPrincipals(PolicyPermission.APPROVE_OTHERS.toMask()) - .stream() - .filter(p -> p instanceof com.google.solutions.jitaccess.auth.IamPrincipalId) - .map(p -> (com.google.solutions.jitaccess.auth.IamPrincipalId) p) - .collect(Collectors.toSet()); + var approvers = approversFromPolicy(group); + if (approvers.isEmpty()) { + // + // Nobody holds APPROVE_OTHERS; pass through and let propose() + // surface its canonical "no principals could approve" error. + // + return null; + } - var hasGroupApprover = rawApprovers.stream() - .anyMatch(com.google.solutions.jitaccess.auth.GroupId.class::isInstance); + var requester = this.subject.user(); + // Exclude the requester BEFORE the size guard: propose() and the + // Slack fan-out both drop them, so they must not count toward the + // broadcast size either. + approvers.remove(requester); + + var hasGroupApprover = approvers.stream() + .anyMatch(GroupId.class::isInstance); // // Cheap guard: no group in the ACL and only a few named approvers → // DMing all of them is not a broad broadcast. Keep the upstream // DM-everyone behaviour and skip the Cloud Identity round-trips - // entirely (this also keeps no-approval / no-approver flows from - // touching the groups client at all). + // entirely (this also keeps no-approver flows from touching the + // groups client at all). // - if (rawApprovers.isEmpty() - || (!hasGroupApprover && rawApprovers.size() <= AUTO_NARROW_BROADCAST_LIMIT)) { + if (approvers.isEmpty() + || (!hasGroupApprover && approvers.size() <= AUTO_NARROW_BROADCAST_LIMIT)) { return null; } - var requester = this.subject.user(); - var qualified = rawApprovers.stream() - .filter(p -> !p.equals(requester)) - .collect(Collectors.toCollection(HashSet::new)); + // + // Same per-user rate limit as getReviewers — compute() below is + // the identical listMembershipsByUser + parallel listMemberships + // fan-out, and without a limiter this POST path would be an + // unthrottled back door to the Cloud Identity quota. The blocking + // acquire (1 s = exactly one permit interval at + // REVIEWERS_RATE_PER_SECOND) means the normal picker-GET-then- + // submit sequence never trips it, while sustained polling gets + // 429s and holds a request thread for at most a second. + // + if (!rateLimiterFor(requester.email) + .tryAcquire(1, java.util.concurrent.TimeUnit.SECONDS)) { + this.logger.warn( + EventIds.API_JOIN_GROUP, + "Auto-narrow rate limit exceeded by %s on %s", + requester.email, groupId); + throw new WebApplicationException( + "Too many requests; please retry shortly.", 429); + } List candidates; try { - var resolver = new GroupResolver(this.groupsClient, this.executor); - candidates = new ReviewerCandidates(resolver, this.groupsClient, this.executor) - .compute(requester, qualified); + candidates = reviewerCandidates().compute(requester, approvers); } - catch (com.google.solutions.jitaccess.apis.clients.AccessException - | java.io.IOException e) { - // Fail safe = never broadcast. If teammates can't be computed, - // don't silently DM the whole group — require an explicit pick. + catch (AccessException | IOException | UncheckedExecutionException e) { + // NB. GroupResolver.expand wraps I/O failures from group + // expansion in UncheckedExecutionException — it must be + // caught here too, or expansion failures bypass this + // fail-safe and surface as a 500. + if (!hasGroupApprover) { + // + // Group-free ACL: the approver set is exactly the named + // individuals above, and DMing them requires no Cloud + // Identity at all. Degrade to that bounded, pre-SECOP-952 + // broadcast rather than dead-ending the requester — during a + // Cloud Identity outage the picker is degraded too, so + // "please pick someone" would be circular guidance. + // + this.logger.warn( + EventIds.API_JOIN_GROUP, + "Auto-narrow of reviewers failed for %s on %s; falling back " + + "to notifying all %d policy-named approvers. cause=%s", + requester.email, groupId, approvers.size(), e); + return null; + } + // Group-bearing ACL: membership is unknowable without Cloud + // Identity, so there is no bounded fallback. Fail safe = never + // broadcast; require an explicit pick. this.logger.warn( EventIds.API_JOIN_GROUP, "Auto-narrow of reviewers failed for %s on %s; requiring an " @@ -479,13 +549,22 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { + "select at least one reviewer for this request."); } - var suggested = candidates.stream() - .filter(ReviewerCandidates.Candidate::suggested) + // + // Teammates = every candidate sharing at least one small group + // with the requester, in affinity order (compute() ranks them + // score-first). NOT the `suggested` badge: that is capped at + // ReviewerCandidates.SUGGESTED_BADGE_TOP_N for presentation, and + // an audience of 3 is fragile — a couple of OOO or offboarded + // teammates would strand the request. + // + var teammates = candidates.stream() + .filter(ReviewerCandidates.Candidate::teammate) + .limit(AUTO_NARROW_BROADCAST_LIMIT) .map(c -> new EndUserId(c.email())) .collect(Collectors.toSet()); - if (!suggested.isEmpty()) { - return suggested; + if (!teammates.isEmpty()) { + return teammates; } // @@ -501,6 +580,28 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { + "one reviewer so we don't notify everyone."); } + /** + * Principals holding APPROVE_OTHERS in the group's effective ACL — + * the raw qualified-approver set (requester NOT excluded). Shared by + * the picked-reviewer validation, the empty-selection auto-narrow, + * and the picker listing so the three stay in lockstep. + */ + private @NotNull Set approversFromPolicy( + @NotNull JitGroupContext group + ) { + return group.policy().effectiveAccessControlList() + .allowedPrincipals(PolicyPermission.APPROVE_OTHERS.toMask()) + .stream() + .filter(p -> p instanceof IamPrincipalId) + .map(p -> (IamPrincipalId) p) + .collect(Collectors.toCollection(HashSet::new)); + } + + private @NotNull ReviewerCandidates reviewerCandidates() { + var resolver = new GroupResolver(this.groupsClient, this.executor); + return new ReviewerCandidates(resolver, this.groupsClient, this.executor); + } + /** * Per-user rate limit on {@link #getReviewers}. Each {@link * ReviewerCandidates#compute} call is expensive — one @@ -586,23 +687,14 @@ static void clearReviewerRateLimiters() { throw new WebApplicationException( "Too many requests; please retry shortly.", 429); } - var qualified = group.policy().effectiveAccessControlList() - .allowedPrincipals(PolicyPermission.APPROVE_OTHERS.toMask()) - .stream() - .filter(p -> !p.equals(requester)) - .filter(p -> p instanceof com.google.solutions.jitaccess.auth.IamPrincipalId) - .map(p -> (com.google.solutions.jitaccess.auth.IamPrincipalId) p) - .collect(Collectors.toCollection(HashSet::new)); - - var resolver = new GroupResolver(this.groupsClient, this.executor); - var helper = new ReviewerCandidates(resolver, this.groupsClient, this.executor); + var qualified = approversFromPolicy(group); + qualified.remove(requester); List candidates; try { - candidates = helper.compute(requester, qualified); + candidates = reviewerCandidates().compute(requester, qualified); } - catch (com.google.solutions.jitaccess.apis.clients.AccessException - | java.io.IOException e) { + catch (AccessException | IOException e) { // Picker is best-effort: a Cloud Identity outage or a missing // group-membership permission must not block the elevation // request. Return the degraded shape and let the frontend diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java index cb6c2845..059bbef6 100644 --- a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java @@ -666,14 +666,15 @@ public void post_whenNoReviewersPickedAndApproverSetSmall_proposesWithNullFilter /** * SECOP-952 core fix: on an empty selection, when the approver set is - * large enough to be a broadcast, narrow to the requester's suggested - * teammates rather than DMing the whole set. Here two of the many - * direct approvers share a small group with the requester, so propose + * large enough to be a broadcast, narrow to the requester's teammates + * rather than DMing the whole set. Here two of the many direct + * approvers share a small group with the requester, so propose * receives exactly those two. */ @Test - public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToSuggested() + public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToTeammates() throws Exception { + GroupsResource.clearReviewerRateLimiters(); var teamMate1 = new EndUserId("teammate-1@example.com"); var teamMate2 = new EndUserId("teammate-2@example.com"); @@ -729,7 +730,7 @@ public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToSuggested() assertEquals( Set.of(teamMate1, teamMate2), captor.getValue().reviewerFilter(), - "empty selection on a broad set must narrow to suggested teammates"); + "empty selection on a broad set must narrow to teammates"); } /** @@ -741,6 +742,7 @@ public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToSuggested() @Test public void post_whenNoReviewersPickedAndNoTeammates_rejectsRatherThanBroadcast() throws Exception { + GroupsResource.clearReviewerRateLimiters(); var acl = new AccessControlList.Builder() .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()); for (int i = 0; i < 20; i++) { @@ -776,6 +778,319 @@ public void post_whenNoReviewersPickedAndNoTeammates_rejectsRatherThanBroadcast( verify(resource.proposalHandler, never()).propose(any(), any(), any()); } + /** + * SECOP-952 review fix: reviewer resolution must not run for joins + * that require no approval. A self-approve join on a group whose ACL + * also names a broad approver set executes immediately — no Cloud + * Identity calls, no proposal, and in particular no auto-narrow + * rejection. (resource.subject is deliberately left unset: any + * reviewer resolution on this path would NPE and fail the test.) + */ + @Test + public void post_whenApprovalNotRequired_skipsReviewerResolution() + throws Exception { + var acl = new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(SAMPLE_USER, PolicyPermission.APPROVE_SELF.toMask()); + for (int i = 0; i < 20; i++) { + acl.allow(new EndUserId("approver-" + i + "@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()); + } + var group = Policies.createJitGroupPolicy( + "g-1", + acl.build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + + var groupInfo = resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>()); + + assertEquals(GroupsResource.JoinStatusInfo.JOIN_COMPLETED, groupInfo.join().status()); + verify(resource.proposalHandler, never()).propose(any(), any(), any()); + verifyNoInteractions(resource.groupsClient); + } + + /** + * SECOP-952 review fix: notifyReviewers=false (copy-link) sends no + * DMs at all, so an empty selection must bypass auto-narrow — the + * proposal keeps the upstream full-ACL recipients (null filter) so + * the pasted link works for any qualified approver, and no Cloud + * Identity call is made. Before this fix, a broad group-bearing ACL + * would have auto-narrowed (or rejected) a flow that notifies + * nobody. (resource.subject deliberately unset, as above.) + */ + @Test + public void post_whenNotifyReviewersFalseAndNothingPicked_bypassesAutoNarrow() + throws Exception { + var group = Policies.createJitGroupPolicy( + "g-1", + new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(new GroupId("engineering@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()) + .build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(true); // copy-link mode on + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + resource.linkBuilder = uriInfo -> jakarta.ws.rs.core.UriBuilder + .fromUri("https://example.test/"); + resource.uriInfo = Mockito.mock(jakarta.ws.rs.core.UriInfo.class); + when(resource.proposalHandler.propose(any(), any(), any())) + .thenReturn(new ProposalHandler.ProposalToken( + "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ0ZXN0In0.", + Set.of(SAMPLE_APPROVING_USER), Instant.MAX)); + + var inputs = new MultivaluedHashMap(); + inputs.put(GroupsResource.FIELD_NOTIFY_REVIEWERS, List.of("false")); + + resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + inputs); + + var captor = org.mockito.ArgumentCaptor.forClass( + ProposalHandler.ProposeOptions.class); + verify(resource.proposalHandler).propose(any(), any(), captor.capture()); + assertNull(captor.getValue().reviewerFilter(), + "copy-link with an empty selection must not be auto-narrowed"); + assertFalse(captor.getValue().notifyReviewers()); + verifyNoInteractions(resource.groupsClient); + } + + /** + * SECOP-952 review fix (off-by-one): the broadcast-size guard must + * count actual reviewers, i.e. exclude the requester. An ACL with + * AUTO_NARROW_BROADCAST_LIMIT approvers PLUS the requester holding + * APPROVE_OTHERS notifies exactly the limit — it must take the cheap + * null-filter path, not the Cloud Identity-dependent one. + */ + @Test + public void post_whenRequesterAmongApproversAtLimit_proposesWithNullFilter() + throws Exception { + var acl = new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(SAMPLE_USER, PolicyPermission.APPROVE_OTHERS.toMask()); + for (int i = 0; i < GroupsResource.AUTO_NARROW_BROADCAST_LIMIT; i++) { + acl.allow(new EndUserId("approver-" + i + "@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()); + } + var group = Policies.createJitGroupPolicy( + "g-1", + acl.build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + when(resource.proposalHandler.propose(any(), any(), any())) + .thenReturn(new ProposalHandler.ProposalToken( + "token", Set.of(SAMPLE_APPROVING_USER), Instant.MAX)); + + resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>()); + + var captor = org.mockito.ArgumentCaptor.forClass( + ProposalHandler.ProposeOptions.class); + verify(resource.proposalHandler).propose(any(), any(), captor.capture()); + assertNull(captor.getValue().reviewerFilter(), + "the requester must not count toward the broadcast size"); + verifyNoInteractions(resource.groupsClient); + } + + /** + * SECOP-952 headline scenario: the ACL names a GROUP approver (the + * engineering@ case). Even though the ACL itself is tiny, the group + * makes its true size unknowable, so auto-narrow must expand it and + * narrow to the requester's teammates from the expansion. + */ + @Test + public void post_whenAclNamesGroup_narrowsToTeammatesFromExpansion() + throws Exception { + GroupsResource.clearReviewerRateLimiters(); + var teamMate1 = new EndUserId("teammate-1@example.com"); + var teamMate2 = new EndUserId("teammate-2@example.com"); + var approverGroup = new GroupId("engineering@example.com"); + + var group = Policies.createJitGroupPolicy( + "g-1", + new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(approverGroup, PolicyPermission.APPROVE_OTHERS.toMask()) + .build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + when(resource.proposalHandler.propose(any(), any(), any())) + .thenReturn(new ProposalHandler.ProposalToken( + "token", Set.of(teamMate1), Instant.MAX)); + + // engineering@ expands to the two teammates plus strangers. + when(resource.groupsClient.listMemberships(eq(approverGroup))) + .thenReturn(List.of( + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId(teamMate1.email)), + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId(teamMate2.email)), + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId("stranger-1@example.com")), + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId("stranger-2@example.com")))); + + // The requester shares one small team group with the teammates. + var teamGroup = new GroupId("team@example.com"); + when(resource.groupsClient.listMembershipsByUser(eq(SAMPLE_USER))) + .thenReturn(List.of(new MembershipRelation() + .setGroupKey(new EntityKey().setId(teamGroup.email)))); + when(resource.groupsClient.listMemberships(eq(teamGroup))) + .thenReturn(List.of( + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId(teamMate1.email)), + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId(teamMate2.email)))); + + resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>()); + + var captor = org.mockito.ArgumentCaptor.forClass( + ProposalHandler.ProposeOptions.class); + verify(resource.proposalHandler).propose(any(), any(), captor.capture()); + assertEquals( + Set.of(teamMate1, teamMate2), + captor.getValue().reviewerFilter(), + "a group-bearing ACL must be expanded and narrowed to teammates"); + } + + /** + * SECOP-952 review fix: GroupResolver.expand wraps I/O failures from + * group expansion in UncheckedExecutionException. The fail-safe must + * catch it and reject with the 400 guidance — not surface a 500. A + * group-bearing ACL has no bounded fallback, so rejection (rather + * than broadcast) is the correct outcome. + */ + @Test + public void post_whenAclNamesGroupAndExpansionFails_rejectsRatherThanBroadcast() + throws Exception { + GroupsResource.clearReviewerRateLimiters(); + var group = Policies.createJitGroupPolicy( + "g-1", + new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(new GroupId("engineering@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()) + .build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + when(resource.groupsClient.listMemberships(any(GroupId.class))) + .thenThrow(new java.io.IOException("Cloud Identity unavailable")); + + assertThrows( + jakarta.ws.rs.BadRequestException.class, + () -> resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>())); + + verify(resource.proposalHandler, never()).propose(any(), any(), any()); + } + + /** + * SECOP-952 review fix: when the broad ACL is GROUP-FREE, the named + * approvers are already enumerated in the policy and notifying them + * needs no Cloud Identity. If teammate resolution fails (outage / + * permission gap), degrade to that bounded pre-SECOP-952 broadcast + * (null filter) instead of rejecting — the "please pick someone" + * guidance would be circular while the picker is degraded too. + */ + @Test + public void post_whenGroupFreeBroadAclAndCloudIdentityFails_fallsBackToNullFilter() + throws Exception { + GroupsResource.clearReviewerRateLimiters(); + var acl = new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()); + for (int i = 0; i < 20; i++) { + acl.allow(new EndUserId("approver-" + i + "@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()); + } + var group = Policies.createJitGroupPolicy( + "g-1", + acl.build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + when(resource.proposalHandler.propose(any(), any(), any())) + .thenReturn(new ProposalHandler.ProposalToken( + "token", Set.of(SAMPLE_APPROVING_USER), Instant.MAX)); + when(resource.groupsClient.listMembershipsByUser(any())) + .thenThrow(new AccessDeniedException("Cloud Identity unavailable")); + + resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>()); + + var captor = org.mockito.ArgumentCaptor.forClass( + ProposalHandler.ProposeOptions.class); + verify(resource.proposalHandler).propose(any(), any(), captor.capture()); + assertNull(captor.getValue().reviewerFilter(), + "a group-free ACL must degrade to the bounded full broadcast, not reject"); + } + //--------------------------------------------------------------------------- // getReviewers — rate limit (wavemm fork P1-4). //--------------------------------------------------------------------------- From 40be8f66343ea46fb054b177efb937ec9257e3e9 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Thu, 9 Jul 2026 12:10:25 +0200 Subject: [PATCH 3/3] build: bump version to 2.3.0-wavemm.8 --- sources/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/pom.xml b/sources/pom.xml index c2135ab2..3cc881d2 100644 --- a/sources/pom.xml +++ b/sources/pom.xml @@ -24,7 +24,7 @@ 4.0.0 com.google.solutions jitaccess - 2.3.0-wavemm.7 + 2.3.0-wavemm.8 3.5.3 3.5.3