[i,l,-r] Auto-narrow reviewers on empty picker selection (SECOP-952)

sources/pom.xml

@@ -24,7 +24,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>com.google.solutions</groupId>
   <artifactId>jitaccess</artifactId>
-  <version>2.3.0-wavemm.7</version>
+  <version>2.3.0-wavemm.8</version>
   <properties>
     <surefire-plugin.version>3.5.3</surefire-plugin.version>
     <surefire-plugin.version>3.5.3</surefire-plugin.version>

sources/src/main/java/com/google/solutions/jitaccess/auth/GroupResolver.java

@@ -72,6 +72,14 @@
  *   <li>{@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. ✓
+ *   <li>{@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. ✓
  *   <li>{@link com.google.solutions.jitaccess.web.rest.GroupsResource#getReviewers}
  *       — picker candidate listing: best-effort, output is for UX
  *       only. ✓

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<Membership> 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 &gt; 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
   ) {}
 }

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;
@@ -180,15 +185,9 @@ public class GroupsResource {
       // here where we have a GroupResolver to expand groups.
       //
       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())
@@ -211,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<EndUserId> effectiveReviewers;
+        if (selectedReviewers != null && !selectedReviewers.isEmpty()) {
+          effectiveReviewers = selectedReviewers;
+        }
+        else if (!notifyReviewers) {
+          effectiveReviewers = null;
+        }
+        else {
+          effectiveReviewers = autoNarrowedReviewers(group, groupId);
+        }
+
         //
         // Approval required, propose to someone else.
         //
@@ -234,7 +262,7 @@ public class GroupsResource {
           joinOp,
           buildActionUri,
           new ProposalHandler.ProposeOptions(
-            selectedReviewers,
+            effectiveReviewers,
             notifyReviewers));
 
         // Plumb the notifyReviewers flag into the audit event so the
@@ -285,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;
 
@@ -370,6 +399,209 @@ private static boolean parseNotifyReviewers(@Nullable List<String> raw) {
     return Boolean.parseBoolean(raw.get(raw.size() - 1));
   }
 
+  /**
+   * 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.
+   *
+   * <p>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.
+   *
+   * <p>Sized to a generous single team: Wave teams are typically &lt;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 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.
+   *
+   * <p>Returns:
+   * <ul>
+   *   <li>{@code null} — no narrowing; {@code propose} uses the full
+   *       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.
+   *   <li>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.
+   * </ul>
+   *
+   * <p>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<EndUserId> autoNarrowedReviewers(
+    @NotNull JitGroupContext group,
+    @NotNull JitGroupId groupId
+  ) {
+    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 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-approver flows from touching the
+    // groups client at all).
+    //
+    if (approvers.isEmpty()
+      || (!hasGroupApprover && approvers.size() <= AUTO_NARROW_BROADCAST_LIMIT)) {
+      return null;
+    }
+
+    //
+    // 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<ReviewerCandidates.Candidate> candidates;
+    try {
+      candidates = reviewerCandidates().compute(requester, approvers);
+    }
+    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 "
+          + "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.");
+    }
+
+    //
+    // 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 (!teammates.isEmpty()) {
+      return teammates;
+    }
+
+    //
+    // 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.");
+  }
+
+  /**
+   * 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<IamPrincipalId> 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
@@ -455,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<ReviewerCandidates.Candidate> 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