From 630b2ac5a374ca6cb918b6f284093415ec57c16c Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 00:21:49 +0200 Subject: [PATCH] auto-narrow: cap auto-picked teammates at 5 closest (SECOP-952) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first production broad-role request auto-picked 15 teammates — judged still too loud. Split the constant: AUTO_NARROW_TEAMMATE_CAP=5 now bounds the set picked on the requester's behalf (candidates are affinity-ranked, so this is their 5 closest colleagues), while AUTO_NARROW_BROADCAST_LIMIT=15 remains the tolerance for DMing an explicitly-named small ACL (or no-signal fallback) in full. --- sources/pom.xml | 2 +- .../jitaccess/web/rest/GroupsResource.java | 44 ++++++---- .../web/rest/TestGroupsResource.java | 83 +++++++++++++++++++ 3 files changed, 113 insertions(+), 16 deletions(-) diff --git a/sources/pom.xml b/sources/pom.xml index bd4ba3a1..f8a71feb 100644 --- a/sources/pom.xml +++ b/sources/pom.xml @@ -24,7 +24,7 @@ 4.0.0 com.google.solutions jitaccess - 2.3.0-wavemm.9 + 2.3.0-wavemm.10 3.5.3 3.5.3 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 eb5a2e41..49064af3 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 @@ -407,19 +407,33 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { * 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 * larger than this almost always means a broad group like - * {@code engineering@} crept into the ACL. + * {@code engineering@} crept into the ACL. Note this tolerance applies + * to approvers the policy names explicitly (or a no-teammate-signal + * fallback); the AUTO-PICKED teammate set has its own, much smaller + * cap — {@link #AUTO_NARROW_TEAMMATE_CAP}. */ static final int AUTO_NARROW_BROADCAST_LIMIT = 15; + /** + * Maximum number of teammates the auto-narrow picks on the + * requester's behalf (SECOP-952 follow-up). Candidates are ranked by + * team affinity, so this is "your {@value} closest colleagues" — the + * first production broad-role request auto-picked 15 and that was + * still judged too loud; when nobody is picked explicitly, a handful + * of close teammates is the intent, not a mid-sized broadcast. + * + *

Deliberately distinct from {@link + * ReviewerCandidates#SUGGESTED_BADGE_TOP_N} (a UI presentation cap: + * tying the DM audience to the badge would let a cosmetic tweak + * silently change production notification fan-out) and from + * {@link #AUTO_NARROW_BROADCAST_LIMIT} (the tolerance for DMing an + * explicitly-named small approver set in full). + */ + static final int AUTO_NARROW_TEAMMATE_CAP = 5; + /** * Compute the reviewer filter for an empty picker selection on a * join that requires approval (SECOP-952). The requester didn't pick @@ -442,7 +456,7 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { *

  • 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 + * capped at {@link #AUTO_NARROW_TEAMMATE_CAP}. Used when * the ACL contains a group (or many direct approvers) so we * avoid broadcasting to the whole group. * @@ -550,16 +564,16 @@ private static boolean parseNotifyReviewers(@Nullable List raw) { } // - // 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. + // Teammates = candidates sharing at least one small group with the + // requester, in affinity order (compute() ranks them score-first), + // capped at the requester's AUTO_NARROW_TEAMMATE_CAP closest. NOT + // the `suggested` badge (a presentation cap), and deliberately a + // few rather than one — a couple of OOO or offboarded teammates + // must not strand the request. // var teammates = candidates.stream() .filter(ReviewerCandidates.Candidate::teammate) - .limit(AUTO_NARROW_BROADCAST_LIMIT) + .limit(AUTO_NARROW_TEAMMATE_CAP) .map(c -> new EndUserId(c.email())) .collect(Collectors.toSet()); 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 059bbef6..5ef16e42 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 @@ -733,6 +733,89 @@ public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToTeammates() "empty selection on a broad set must narrow to teammates"); } + /** + * SECOP-952 follow-up: the auto-picked teammate set is capped at + * AUTO_NARROW_TEAMMATE_CAP (5) even when more teammates exist — the + * first production broad-role request auto-picked 15 and that was + * still too loud. Higher-affinity teammates (more shared small + * groups) win the cut. + */ + @Test + public void post_whenManyTeammates_capsAutoPickAtFiveClosest() + throws Exception { + GroupsResource.clearReviewerRateLimiters(); + + // 8 teammates; three of them share TWO small groups with the + // requester (higher affinity), five share one. A broad group-free + // ACL pushes the request onto the auto-narrow path. + var acl = new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()); + for (int i = 0; i < 8; i++) { + acl.allow(new EndUserId("teammate-" + i + "@example.com"), + 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(SAMPLE_APPROVING_USER), Instant.MAX)); + + var teamA = new GroupId("team-a@example.com"); // all 8 teammates + var teamB = new GroupId("team-b@example.com"); // teammates 5..7 only + when(resource.groupsClient.listMembershipsByUser(eq(SAMPLE_USER))) + .thenReturn(List.of( + new MembershipRelation().setGroupKey(new EntityKey().setId(teamA.email)), + new MembershipRelation().setGroupKey(new EntityKey().setId(teamB.email)))); + var teamAMembers = new java.util.ArrayList(); + for (int i = 0; i < 8; i++) { + teamAMembers.add(new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId("teammate-" + i + "@example.com"))); + } + when(resource.groupsClient.listMemberships(eq(teamA))).thenReturn(teamAMembers); + when(resource.groupsClient.listMemberships(eq(teamB))) + .thenReturn(List.of( + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId("teammate-5@example.com")), + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId("teammate-6@example.com")), + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId("teammate-7@example.com")))); + + 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()); + var picked = captor.getValue().reviewerFilter(); + assertEquals(GroupsResource.AUTO_NARROW_TEAMMATE_CAP, picked.size(), + "auto-pick must stop at the teammate cap"); + // The three double-affinity teammates must all make the cut; the + // remaining two slots go to single-affinity teammates. + assertTrue(picked.contains(new EndUserId("teammate-5@example.com"))); + assertTrue(picked.contains(new EndUserId("teammate-6@example.com"))); + assertTrue(picked.contains(new EndUserId("teammate-7@example.com"))); + } + /** * 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