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