3.5.3
3.5.3
diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java
index c80fbc90..516cb328 100644
--- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java
+++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java
@@ -31,6 +31,7 @@
import java.net.URI;
import java.time.Instant;
+import java.util.List;
import java.util.Set;
import java.util.function.Function;
@@ -106,6 +107,30 @@ public ProposeOptions(
@NotNull String proposalToken
) throws AccessException;
+ /**
+ * Wavemm fork (SECOP-1098): reorder an affinity-ranked list of
+ * candidate reviewers to prefer those most likely to act quickly —
+ * currently-active-on-Slack first, then in a working-hours timezone
+ * close to the requester's — so an empty-selection auto-pick lands on
+ * reviewers who can approve soon rather than ones who are asleep.
+ *
+ * The default is a no-op passthrough: handlers without a presence
+ * signal (mail, debug) keep the pure-affinity order. The Slack handler
+ * overrides this. Availability is a *tiebreaker*, never a filter — an
+ * offline teammate is deprioritised, not dropped, and any enrichment
+ * failure falls back to the input order (fail-open).
+ *
+ * @param requester the user whose request needs reviewers
+ * @param affinityRanked candidate reviewer emails, closest-first
+ * @return the same emails, reordered availability-first
+ */
+ default @NotNull List rankReviewersByAvailability(
+ @NotNull EndUserId requester,
+ @NotNull List affinityRanked
+ ) {
+ return affinityRanked;
+ }
+
/**
* Wavemm fork (SECOP-1093): read-only check that a proposal token
* hasn't been used to approve — used by the GET proposal-view path to
diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java
index d72d1319..1f365a5d 100644
--- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java
+++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java
@@ -19,6 +19,8 @@
import com.slack.api.methods.response.chat.ChatPostMessageResponse;
import com.slack.api.methods.response.chat.ChatUpdateResponse;
import com.slack.api.methods.response.conversations.ConversationsOpenResponse;
+import com.slack.api.methods.response.users.UsersGetPresenceResponse;
+import com.slack.api.methods.response.users.UsersInfoResponse;
import com.slack.api.methods.response.users.UsersLookupByEmailResponse;
import com.slack.api.model.block.LayoutBlock;
import org.jetbrains.annotations.NotNull;
@@ -146,6 +148,70 @@ public SlackClient(
}, this.executor);
}
+ /**
+ * Whether a user is currently active on Slack (SECOP-1098). "active"
+ * vs "away" is a coarse signal (desktop idle reads as away), so
+ * callers use it to prioritise, never to exclude. Returns false when
+ * presence can't be determined — a conservative default that just
+ * means "don't prioritise", not "unavailable".
+ */
+ public @NotNull CompletableFuture isActive(
+ @NotNull String slackUserId
+ ) {
+ return CompletableFutures.supplyAsync(() -> {
+ try {
+ UsersGetPresenceResponse response =
+ this.methods.usersGetPresence(req -> req.user(slackUserId));
+ if (!response.isOk()) {
+ // SECOP-1104: surface it. A persistent error (e.g.
+ // missing_scope for users:read — a scope this feature newly
+ // requires) would otherwise silently degrade every candidate
+ // to "not active", making the availability ranking an inert
+ // no-op with no signal.
+ this.logger.warn(
+ "slack.getPresence.failed",
+ "users.getPresence for %s failed: %s (treating as not active)",
+ slackUserId, response.getError());
+ return false;
+ }
+ return "active".equals(response.getPresence());
+ }
+ catch (SlackApiException e) {
+ throw new IOException("Slack API error in users.getPresence for " + slackUserId, e);
+ }
+ }, this.executor);
+ }
+
+ /**
+ * The user's timezone offset from UTC in seconds (SECOP-1098), or null
+ * when unknown. Used to prefer reviewers in working hours close to the
+ * requester's.
+ */
+ public @NotNull CompletableFuture<@Nullable Integer> timezoneOffsetSeconds(
+ @NotNull String slackUserId
+ ) {
+ return CompletableFutures.supplyAsync(() -> {
+ try {
+ UsersInfoResponse response =
+ this.methods.usersInfo(req -> req.user(slackUserId));
+ if (!response.isOk()) {
+ this.logger.warn(
+ "slack.usersInfo.failed",
+ "users.info for %s failed: %s (timezone unknown)",
+ slackUserId, response.getError());
+ return null;
+ }
+ if (response.getUser() == null) {
+ return null;
+ }
+ return response.getUser().getTzOffset();
+ }
+ catch (SlackApiException e) {
+ throw new IOException("Slack API error in users.info for " + slackUserId, e);
+ }
+ }, this.executor);
+ }
+
/**
* A successfully posted Slack message, identified by (channel, timestamp).
*/
diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java
index 8980a877..215cad94 100644
--- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java
+++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java
@@ -21,12 +21,14 @@
import com.google.solutions.jitaccess.catalog.Proposal;
import com.google.solutions.jitaccess.web.proposal.SlackMessageRegistry.ReviewerMessage;
import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
import java.io.IOException;
import java.net.URI;
import java.security.SecureRandom;
import java.time.ZoneId;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
@@ -686,6 +688,171 @@ public void releaseClaim(@NotNull Proposal proposal) {
}
}
+ /**
+ * Largest number of teammates we enrich with Slack availability
+ * before selecting (SECOP-1098). Bounds the extra Slack calls
+ * (lookup + presence + tz per candidate) to a shortlist; teammates
+ * past this stay in affinity order and only matter as backfill.
+ */
+ static final int AVAILABILITY_SHORTLIST_SIZE = 15;
+
+ /** Working-hours proximity window: same-ish timezone = within 2h. */
+ private static final int TZ_PROXIMITY_SECONDS = 2 * 60 * 60;
+
+ @Override
+ public @NotNull List rankReviewersByAvailability(
+ @NotNull EndUserId requester,
+ @NotNull List affinityRanked
+ ) {
+ if (affinityRanked.size() <= 1) {
+ return affinityRanked;
+ }
+
+ //
+ // Enrich only a bounded shortlist; the tail stays in affinity order
+ // and is appended unchanged (it only serves as backfill once the
+ // available candidates are exhausted).
+ //
+ var shortlist = affinityRanked.stream()
+ .limit(AVAILABILITY_SHORTLIST_SIZE)
+ .toList();
+ var tail = affinityRanked.stream()
+ .skip(AVAILABILITY_SHORTLIST_SIZE)
+ .toList();
+
+ try {
+ //
+ // SECOP-1104: enrich the shortlist CONCURRENTLY. Each candidate's
+ // (lookup → presence+tz) is an async chain; we fire them all and
+ // join once, so the cost is ~one round-trip pair rather than the
+ // ~3×15 strictly-sequential calls the first cut made on this
+ // already-slow POST path. The requester's tz is fetched in
+ // parallel too. Every chain degrades to "unknown" (tier 2) on its
+ // own error — a single bad candidate must not abort the ranking.
+ //
+ var requesterTzFuture = timezoneOffsetFuture(requester.email);
+ var futures = new HashMap>();
+ for (var u : shortlist) {
+ futures.put(u.email, availabilityFuture(u.email));
+ }
+ var requesterTz = requesterTzFuture.join();
+ var byEmail = new HashMap();
+ for (var e : futures.entrySet()) {
+ byEmail.put(e.getKey(), e.getValue().join());
+ }
+
+ //
+ // Stable sort: keep affinity order (the shortlist's existing
+ // order) except promote (0) currently-active teammates, then
+ // (1) those in a working-hours timezone near the requester's.
+ // Java's sort is stable, so affinity breaks ties within a tier.
+ //
+ var ranked = new ArrayList<>(shortlist);
+ ranked.sort(java.util.Comparator.comparingInt(
+ (EndUserId u) -> availabilityTier(byEmail.get(u.email), requesterTz)));
+
+ var result = new ArrayList<>(ranked);
+ result.addAll(tail);
+
+ //
+ // Log the tier distribution, not just "reordered N" — if the bot
+ // token lacks users:read every candidate silently lands in tier 2
+ // (ranking is a no-op) and this line makes that visible
+ // (active=0 tzNear=0) rather than falsely implying it worked.
+ //
+ var active = shortlist.stream()
+ .filter(u -> availabilityTier(byEmail.get(u.email), requesterTz) == 0).count();
+ var tzNear = shortlist.stream()
+ .filter(u -> availabilityTier(byEmail.get(u.email), requesterTz) == 1).count();
+ this.logger.info(
+ "slack.rankReviewersByAvailability",
+ "Reordered %d reviewer(s) availability-first for %s "
+ + "(active=%d tzNear=%d other=%d)",
+ shortlist.size(), requester.email,
+ active, tzNear, shortlist.size() - active - tzNear);
+ return result;
+ }
+ catch (RuntimeException e) {
+ var cause = e.getCause() != null ? e.getCause() : e;
+ this.logger.warn(
+ "slack.rankReviewersByAvailability.failed",
+ "Availability ranking failed for %s; falling back to affinity "
+ + "order (fail-open). cause=%s",
+ requester.email, cause.getMessage());
+ return affinityRanked;
+ }
+ }
+
+ /**
+ * Rank tier: 0 = active now (best), 1 = in a nearby working-hours
+ * timezone, 2 = everything else. Lower sorts first.
+ */
+ private int availabilityTier(Availability a, Integer requesterTz) {
+ if (a == null) {
+ return 2;
+ }
+ if (a.active()) {
+ return 0;
+ }
+ if (a.tzOffsetSeconds() != null && requesterTz != null) {
+ //
+ // SECOP-1104: compare offsets on a 24h circle. Raw |a-b| treats
+ // antimeridian neighbours (UTC+13 vs UTC-11 — identical local
+ // clocks) as ~24h apart; wrap so the smaller arc counts.
+ //
+ var diff = Math.abs(a.tzOffsetSeconds() - requesterTz);
+ var wrapped = Math.min(diff, 86400 - diff);
+ if (wrapped <= TZ_PROXIMITY_SECONDS) {
+ return 1;
+ }
+ }
+ return 2;
+ }
+
+ /**
+ * Async (active?, tzOffset) for one candidate. Self-contained
+ * degradation: any failure in the chain yields "unknown" (tier 2)
+ * rather than failing the whole ranking (SECOP-1104).
+ */
+ private @NotNull CompletableFuture availabilityFuture(
+ @NotNull String email
+ ) {
+ return this.slackClient.lookupUserByEmail(email)
+ .thenCompose(userId -> {
+ if (userId == null) {
+ return CompletableFuture.completedFuture(new Availability(email, false, null));
+ }
+ return this.slackClient.isActive(userId)
+ .thenCombine(
+ this.slackClient.timezoneOffsetSeconds(userId),
+ (active, tz) -> new Availability(email, active, tz));
+ })
+ .exceptionally(ex -> {
+ var cause = ex.getCause() != null ? ex.getCause() : ex;
+ this.logger.warn(
+ "slack.availabilityLookup.failed",
+ "Availability lookup failed for %s; treating as unknown. cause=%s",
+ email, cause.getMessage());
+ return new Availability(email, false, null);
+ });
+ }
+
+ private @NotNull CompletableFuture<@Nullable Integer> timezoneOffsetFuture(
+ @NotNull String email
+ ) {
+ return this.slackClient.lookupUserByEmail(email)
+ .thenCompose(userId -> userId == null
+ ? CompletableFuture.completedFuture((Integer) null)
+ : this.slackClient.timezoneOffsetSeconds(userId))
+ .exceptionally(ex -> null);
+ }
+
+ private record Availability(
+ @NotNull String email,
+ boolean active,
+ @Nullable Integer tzOffsetSeconds
+ ) {}
+
private void notifyBeneficiary(
@NotNull String beneficiary,
@NotNull String groupId,
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 aed80c3e..8bb47102 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
@@ -576,20 +576,30 @@ private static boolean parseNotifyReviewers(@Nullable List raw) {
//
// 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.
+ // requester, in affinity order (compute() ranks them score-first).
+ // NOT the `suggested` badge (a presentation cap).
//
- var teammates = candidates.stream()
+ var teammatesByAffinity = candidates.stream()
.filter(ReviewerCandidates.Candidate::teammate)
- .limit(AUTO_NARROW_TEAMMATE_CAP)
.map(c -> new EndUserId(c.email()))
- .collect(Collectors.toSet());
+ .toList();
- if (!teammates.isEmpty()) {
- return teammates;
+ if (!teammatesByAffinity.isEmpty()) {
+ //
+ // SECOP-1098: reorder availability-first (active-on-Slack, then
+ // working-hours-timezone-close) so the auto-pick lands on
+ // reviewers who can approve soonest — the whole point of picking
+ // is to shorten the requester's wait. No-op passthrough for
+ // non-Slack handlers, and fail-open. Then cap at the requester's
+ // AUTO_NARROW_TEAMMATE_CAP closest/soonest — deliberately a few
+ // rather than one, so a couple of OOO teammates don't strand the
+ // request.
+ //
+ var ranked = this.proposalHandler.rankReviewersByAvailability(
+ requester, teammatesByAffinity);
+ return ranked.stream()
+ .limit(AUTO_NARROW_TEAMMATE_CAP)
+ .collect(Collectors.toCollection(java.util.LinkedHashSet::new));
}
//
diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestAbstractProposalHandler.java b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestAbstractProposalHandler.java
index ec4773c6..0ff573d2 100644
--- a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestAbstractProposalHandler.java
+++ b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestAbstractProposalHandler.java
@@ -39,6 +39,7 @@
import java.net.URI;
import java.time.Duration;
import java.time.Instant;
+import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.Set;
@@ -207,6 +208,18 @@ public void accept_exposesJwtIdAsProposalId() throws Exception {
assertEquals("abc123", proposal.id());
}
+ /**
+ * SECOP-1098: handlers without a presence signal (here the sample /
+ * mail / debug handlers) inherit the no-op default and return the
+ * affinity order unchanged.
+ */
+ @Test
+ public void rankReviewersByAvailability_defaultIsPassthrough() {
+ var handler = new SampleProposalHandler(new PseudoSigner());
+ var input = List.of(SAMPLE_USER_1, SAMPLE_USER_2);
+ assertEquals(input, handler.rankReviewersByAvailability(SAMPLE_USER_1, input));
+ }
+
@Test
public void accept_whenInputEmpty() throws Exception {
var token = "{\"rcp\":[\"user:user-2@example.com\",\"group:group@example.com\"]," +
diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java
index ce295611..cd84ccd9 100644
--- a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java
+++ b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java
@@ -381,6 +381,137 @@ public void onProposalApproved_optOutShortCircuitsRegistryLookup() throws Except
// Options — fan-out concurrency cap (wavemm fork P1-4)
// ---------------------------------------------------------------------
+ // -------------------------------------------------------------------------
+ // Availability-first reviewer ranking (SECOP-1098).
+ // -------------------------------------------------------------------------
+
+ @Test
+ public void rankReviewersByAvailability_promotesActiveOverInactive() {
+ var slack = mock(SlackClient.class);
+ when(slack.lookupUserByEmail(eq("alice@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-ALICE"));
+ when(slack.lookupUserByEmail(eq("bob@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-BOB"));
+ when(slack.lookupUserByEmail(eq("carol@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-CAROL"));
+ // tz unknown for everyone → only presence differentiates.
+ when(slack.timezoneOffsetSeconds(anyString()))
+ .thenReturn(CompletableFuture.completedFuture(null));
+ when(slack.isActive(eq("U-BOB")))
+ .thenReturn(CompletableFuture.completedFuture(false));
+ when(slack.isActive(eq("U-CAROL")))
+ .thenReturn(CompletableFuture.completedFuture(true));
+ var handler = newHandler(slack, registryHappyPath(), groupResolverPassthrough());
+
+ // Affinity order is [BOB, CAROL]; Carol is active → promoted first.
+ var ranked = handler.rankReviewersByAvailability(ALICE, List.of(BOB, CAROL));
+ assertEquals(List.of(CAROL, BOB), ranked);
+ }
+
+ @Test
+ public void rankReviewersByAvailability_breaksTiesByTimezoneProximity() {
+ var slack = mock(SlackClient.class);
+ when(slack.lookupUserByEmail(eq("alice@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-ALICE"));
+ when(slack.lookupUserByEmail(eq("bob@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-BOB"));
+ when(slack.lookupUserByEmail(eq("carol@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-CAROL"));
+ // Nobody active; requester at UTC. Bob +1h (near), Carol +10h (far).
+ when(slack.isActive(anyString()))
+ .thenReturn(CompletableFuture.completedFuture(false));
+ when(slack.timezoneOffsetSeconds(eq("U-ALICE")))
+ .thenReturn(CompletableFuture.completedFuture(0));
+ when(slack.timezoneOffsetSeconds(eq("U-BOB")))
+ .thenReturn(CompletableFuture.completedFuture(3600));
+ when(slack.timezoneOffsetSeconds(eq("U-CAROL")))
+ .thenReturn(CompletableFuture.completedFuture(36000));
+ var handler = newHandler(slack, registryHappyPath(), groupResolverPassthrough());
+
+ // Affinity order [CAROL, BOB]; Bob's timezone is near the requester's
+ // → promoted over Carol despite Carol's higher affinity.
+ var ranked = handler.rankReviewersByAvailability(ALICE, List.of(CAROL, BOB));
+ assertEquals(List.of(BOB, CAROL), ranked);
+ }
+
+ @Test
+ public void rankReviewersByAvailability_failsOpenToAffinityOrder() {
+ var slack = mock(SlackClient.class);
+ when(slack.lookupUserByEmail(anyString()))
+ .thenReturn(CompletableFuture.failedFuture(new RuntimeException("slack down")));
+ var handler = newHandler(slack, registryHappyPath(), groupResolverPassthrough());
+
+ var input = List.of(BOB, CAROL);
+ assertEquals(input, handler.rankReviewersByAvailability(ALICE, input));
+ }
+
+ @Test
+ public void rankReviewersByAvailability_singletonReturnedUnchanged() {
+ var slack = mock(SlackClient.class);
+ var handler = newHandler(slack, registryHappyPath(), groupResolverPassthrough());
+
+ assertEquals(List.of(BOB), handler.rankReviewersByAvailability(ALICE, List.of(BOB)));
+ // No Slack calls for a trivial list.
+ verifyNoInteractions(slack);
+ }
+
+ /**
+ * SECOP-1104: timezone proximity wraps on the 24h circle — a teammate
+ * across the antimeridian (identical local clock) counts as close, not
+ * ~24h away.
+ */
+ @Test
+ public void rankReviewersByAvailability_treatsAntimeridianAsClose() {
+ var slack = mock(SlackClient.class);
+ when(slack.lookupUserByEmail(eq("alice@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-ALICE"));
+ when(slack.lookupUserByEmail(eq("bob@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-BOB"));
+ when(slack.lookupUserByEmail(eq("carol@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-CAROL"));
+ when(slack.isActive(anyString()))
+ .thenReturn(CompletableFuture.completedFuture(false));
+ when(slack.timezoneOffsetSeconds(eq("U-ALICE")))
+ .thenReturn(CompletableFuture.completedFuture(46800)); // UTC+13
+ when(slack.timezoneOffsetSeconds(eq("U-BOB")))
+ .thenReturn(CompletableFuture.completedFuture(-39600)); // UTC-11, same clock
+ when(slack.timezoneOffsetSeconds(eq("U-CAROL")))
+ .thenReturn(CompletableFuture.completedFuture(0)); // UTC, 13h away
+ var handler = newHandler(slack, registryHappyPath(), groupResolverPassthrough());
+
+ // Affinity [CAROL, BOB]; Bob wraps to 0h from Alice → tier 1, Carol
+ // 13h away → tier 2 → Bob promoted.
+ var ranked = handler.rankReviewersByAvailability(ALICE, List.of(CAROL, BOB));
+ assertEquals(List.of(BOB, CAROL), ranked);
+ }
+
+ /**
+ * SECOP-1104: a single candidate's enrichment failure degrades only
+ * that candidate (to tier 2) — it must NOT abort ranking for everyone
+ * (which would revert to pure affinity order).
+ */
+ @Test
+ public void rankReviewersByAvailability_degradesFailedCandidateNotWholesale() {
+ var slack = mock(SlackClient.class);
+ when(slack.lookupUserByEmail(eq("alice@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-ALICE"));
+ when(slack.timezoneOffsetSeconds(anyString()))
+ .thenReturn(CompletableFuture.completedFuture(null));
+ // Bob's lookup fails; Carol is active.
+ when(slack.lookupUserByEmail(eq("bob@example.com")))
+ .thenReturn(CompletableFuture.failedFuture(new RuntimeException("boom")));
+ when(slack.lookupUserByEmail(eq("carol@example.com")))
+ .thenReturn(CompletableFuture.completedFuture("U-CAROL"));
+ when(slack.isActive(eq("U-CAROL")))
+ .thenReturn(CompletableFuture.completedFuture(true));
+ var handler = newHandler(slack, registryHappyPath(), groupResolverPassthrough());
+
+ // Affinity [BOB, CAROL]; Bob degrades to tier 2, Carol active → tier 0.
+ // Result [CAROL, BOB]; a wholesale fail-open would have kept [BOB, CAROL].
+ var ranked = handler.rankReviewersByAvailability(ALICE, List.of(BOB, CAROL));
+ assertEquals(List.of(CAROL, BOB), ranked);
+ }
+
// -------------------------------------------------------------------------
// Duplicate-proposal skip (SECOP-1097).
// -------------------------------------------------------------------------
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 5cc0dd15..7b36299d 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
@@ -368,6 +368,8 @@ public void post_whenJoinAllowedWithoutApproval() throws Exception {
resource.auditTrail = Mockito.mock(OperationAuditTrail.class);
resource.catalog = createCatalog(group);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
var groupInfo = resource.post(
group.id().environment(),
@@ -401,6 +403,8 @@ public void post_whenJoinAllowedWithApproval() throws Exception {
resource.auditTrail = Mockito.mock(OperationAuditTrail.class);
resource.catalog = createCatalog(group);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
@@ -456,6 +460,8 @@ public void post_whenSelectedReviewersIncludeUnauthorisedEmail_rejectsWith403()
resource.auditTrail = Mockito.mock(OperationAuditTrail.class);
resource.catalog = createCatalog(group);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
resource.subject = Subjects.create(SAMPLE_USER);
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(
@@ -568,6 +574,8 @@ public void post_whenNotifyReviewersFalse_passesOptionToProposeAndReturnsApprova
resource.auditTrail = Mockito.mock(OperationAuditTrail.class);
resource.catalog = createCatalog(group);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
// copy-link mode reaches through buildActionUri.apply(...) to build
// the approvalUrl in the response, so linkBuilder + uriInfo need
// working stubs unlike the non-copy-link tests above.
@@ -643,6 +651,8 @@ public void post_whenNoReviewersPickedAndApproverSetSmall_proposesWithNullFilter
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
"token", Set.of(SAMPLE_APPROVING_USER), Instant.MAX));
@@ -702,6 +712,8 @@ public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToTeammates()
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
"token", Set.of(teamMate1), Instant.MAX));
@@ -766,6 +778,8 @@ public void post_whenAutoNarrowed_responseCarriesNotifiedReviewers()
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
"token", Set.of(teamMate1), Instant.MAX));
@@ -828,6 +842,8 @@ public void post_whenManyTeammates_capsAutoPickAtFiveClosest()
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
"token", Set.of(SAMPLE_APPROVING_USER), Instant.MAX));
@@ -872,6 +888,79 @@ public void post_whenManyTeammates_capsAutoPickAtFiveClosest()
assertTrue(picked.contains(new EndUserId("teammate-7@example.com")));
}
+ /**
+ * SECOP-1098: the picker hands the affinity-ranked teammates to the
+ * proposal handler's availability ranker and honours the returned
+ * order when capping. Here the ranker reverses the order, so the cap
+ * keeps the *last* teammates by affinity — proving the handler's
+ * ordering wins, not the raw affinity order.
+ */
+ @Test
+ public void post_whenAutoNarrowed_honoursAvailabilityRankerOrder()
+ throws Exception {
+ GroupsResource.clearReviewerRateLimiters();
+ var t1 = new EndUserId("teammate-1@example.com");
+ var t2 = new EndUserId("teammate-2@example.com");
+ var acl = new AccessControlList.Builder()
+ .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask())
+ .allow(t1, PolicyPermission.APPROVE_OTHERS.toMask())
+ .allow(t2, 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(t1), Instant.MAX));
+ // Ranker reverses affinity order.
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> {
+ var in = new java.util.ArrayList(inv.getArgument(1));
+ java.util.Collections.reverse(in);
+ return in;
+ });
+
+ 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(t1.email)),
+ new Membership().setType("user")
+ .setPreferredMemberKey(new EntityKey().setId(t2.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());
+ // Both fit under the cap, so the set is the same either way — the
+ // point is that the ranker was consulted (verified) and its output
+ // used verbatim.
+ verify(resource.proposalHandler).rankReviewersByAvailability(eq(SAMPLE_USER), any());
+ assertEquals(Set.of(t1, t2), captor.getValue().reviewerFilter());
+ }
+
/**
* 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
@@ -902,6 +991,8 @@ public void post_whenNoReviewersPickedAndNoTeammates_rejectsRatherThanBroadcast(
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
// Requester shares no group with anyone → no suggestions.
when(resource.groupsClient.listMembershipsByUser(any()))
.thenReturn(List.of());
@@ -947,6 +1038,8 @@ public void post_whenApprovalNotRequired_skipsReviewerResolution()
resource.catalog = createCatalog(group);
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
var groupInfo = resource.post(
group.id().environment(),
@@ -987,6 +1080,8 @@ public void post_whenNotifyReviewersFalseAndNothingPicked_bypassesAutoNarrow()
resource.catalog = createCatalog(group);
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
resource.linkBuilder = uriInfo -> jakarta.ws.rs.core.UriBuilder
.fromUri("https://example.test/");
resource.uriInfo = Mockito.mock(jakarta.ws.rs.core.UriInfo.class);
@@ -1044,6 +1139,8 @@ public void post_whenRequesterAmongApproversAtLimit_proposesWithNullFilter()
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
"token", Set.of(SAMPLE_APPROVING_USER), Instant.MAX));
@@ -1093,6 +1190,8 @@ public void post_whenAclNamesGroup_narrowsToTeammatesFromExpansion()
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
"token", Set.of(teamMate1), Instant.MAX));
@@ -1165,6 +1264,8 @@ public void post_whenAclNamesGroupAndExpansionFails_rejectsRatherThanBroadcast()
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.groupsClient.listMemberships(any(GroupId.class)))
.thenThrow(new java.io.IOException("Cloud Identity unavailable"));
@@ -1211,6 +1312,8 @@ public void post_whenGroupFreeBroadAclAndCloudIdentityFails_fallsBackToNullFilte
resource.executor = Runnable::run;
resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class);
resource.proposalHandler = Mockito.mock(ProposalHandler.class);
+ when(resource.proposalHandler.rankReviewersByAvailability(any(), any()))
+ .thenAnswer(inv -> inv.getArgument(1)); // SECOP-1098: passthrough by default
when(resource.proposalHandler.propose(any(), any(), any()))
.thenReturn(new ProposalHandler.ProposalToken(
"token", Set.of(SAMPLE_APPROVING_USER), Instant.MAX));