From 86bb37435c761b00149fd3eba6797149a270e87d Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 09:40:46 +0200 Subject: [PATCH 1/3] auto-narrow: rank auto-picked reviewers availability-first (SECOP-1098) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On an empty picker selection we auto-pick the requester's closest teammates; ranking them purely by team affinity meant an asleep or off-shift teammate ranked the same as one online now. Since the point of picking is to minimise how long the requester waits, reorder the affinity shortlist availability-first before capping at 5: tier 0: currently active on Slack tier 1: in a working-hours timezone within 2h of the requester tier 2: everyone else (stable sort, so affinity breaks ties within a tier) Wiring: ProposalHandler gains a rankReviewersByAvailability default (no-op passthrough — mail/debug keep pure affinity); SlackProposalHandler overrides it using users.getPresence + users.info (tz), reusing the SlackClient it already holds, so GroupsResource needs no new dependency. Guarantees: - availability is a tiebreaker, never a filter — an offline teammate is deprioritised, not dropped, so an off-hours request still lands in the closest teammates' DMs; - fail-open: any Slack enrichment error falls back to affinity order; - bounded: only a 15-teammate shortlist is enriched, behind the picker's existing per-user rate limiter. NOTE: presence/tz behaviour under the bot token wants live validation on a real workspace before this deploys (it's fail-open, so no risk to the base flow if it misbehaves). Version -> 2.3.0-wavemm.12. --- .../web/proposal/ProposalHandler.java | 25 ++++ .../jitaccess/web/proposal/SlackClient.java | 47 +++++++ .../web/proposal/SlackProposalHandler.java | 117 ++++++++++++++++++ .../jitaccess/web/rest/GroupsResource.java | 30 +++-- .../proposal/TestAbstractProposalHandler.java | 13 ++ .../proposal/TestSlackProposalHandler.java | 74 +++++++++++ .../web/rest/TestGroupsResource.java | 103 +++++++++++++++ 7 files changed, 399 insertions(+), 10 deletions(-) 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..732dfca6 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,51 @@ 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)); + return response.isOk() && "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() || 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..0640cf18 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,121 @@ 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 { + var requesterTz = timezoneOffsetOrNull(requester.email); + + // + // Enrich each shortlisted teammate: (active?, tzOffset). Bounded + // to the shortlist and gated by the picker's per-user rate limiter + // upstream, so a handful of extra Slack calls per empty-selection + // propose. Sequential is fine at this size. + // + var byEmail = new HashMap(); + for (var u : shortlist) { + byEmail.put(u.email, availabilityOf(u.email)); + } + + // + // Stable sort: keep affinity order (the shortlist's existing + // order) except promote (1) currently-active teammates, then + // (2) those in a working-hours timezone near the requester's. + // Comparator returns 0 for same tier so Java's stable sort + // preserves affinity 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); + this.logger.info( + "slack.rankReviewersByAvailability", + "Reordered %d candidate reviewer(s) availability-first for %s", + shortlist.size(), requester.email); + 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 + && Math.abs(a.tzOffsetSeconds() - requesterTz) <= TZ_PROXIMITY_SECONDS) { + return 1; + } + return 2; + } + + private @NotNull Availability availabilityOf(@NotNull String email) { + var userId = this.slackClient.lookupUserByEmail(email).join(); + if (userId == null) { + return new Availability(email, false, null); + } + var active = this.slackClient.isActive(userId).join(); + var tz = this.slackClient.timezoneOffsetSeconds(userId).join(); + return new Availability(email, active, tz); + } + + private @Nullable Integer timezoneOffsetOrNull(@NotNull String email) { + var userId = this.slackClient.lookupUserByEmail(email).join(); + return userId == null ? null : this.slackClient.timezoneOffsetSeconds(userId).join(); + } + + 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..bdf5f4d9 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,80 @@ 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); + } + // ------------------------------------------------------------------------- // 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)); From afb6ef74e6eba5fba0aaccef5cdf3f54fc84038f Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 11:23:10 +0200 Subject: [PATCH 2/3] availability ranking: parallel enrichment, tz wrap, scope visibility, per-candidate degradation (SECOP-1104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the four defects the adversarial review found in the SECOP-1098 ranking (all in this unmerged #16): - Latency: enrich the shortlist CONCURRENTLY (async lookup→presence+tz chains, joined once) instead of ~3×15 strictly-sequential blocking calls on the already-slow propose POST. - Per-candidate degradation: each chain .exceptionally → 'unknown' (tier 2); one bad candidate no longer aborts ranking for all (the outer fail-open now only catches systemic errors). - Timezone wrap: compare offsets on the 24h circle (min(d, 86400-d)) so antimeridian teammates with identical local clocks rank as close, not ~24h apart. - Scope-error visibility: SlackClient.isActive/timezoneOffsetSeconds WARN on !isOk (e.g. missing users:read), and the ranking INFO log carries the tier distribution (active/tzNear/other) so a silently inert ranking is detectable. Tests: antimeridian promotion, per-candidate degradation vs wholesale fallback. Suite 1050/1050. NOTE: still to do before #16 merges — rebase onto the SECOP-1100..1103 fixes (PR #17) and reconcile the version bump; and the cross-phase lookupUserByEmail cache (ranking re-looks-up winners the fan-out then looks up again) is deferred as a pure optimization. --- .../jitaccess/web/proposal/SlackClient.java | 23 +++- .../web/proposal/SlackProposalHandler.java | 106 +++++++++++++----- .../proposal/TestSlackProposalHandler.java | 57 ++++++++++ 3 files changed, 156 insertions(+), 30 deletions(-) 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 732dfca6..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 @@ -162,7 +162,19 @@ public SlackClient( try { UsersGetPresenceResponse response = this.methods.usersGetPresence(req -> req.user(slackUserId)); - return response.isOk() && "active".equals(response.getPresence()); + 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); @@ -182,7 +194,14 @@ public SlackClient( try { UsersInfoResponse response = this.methods.usersInfo(req -> req.user(slackUserId)); - if (!response.isOk() || response.getUser() == null) { + 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(); 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 0640cf18..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 @@ -721,25 +721,31 @@ public void releaseClaim(@NotNull Proposal proposal) { .toList(); try { - var requesterTz = timezoneOffsetOrNull(requester.email); - // - // Enrich each shortlisted teammate: (active?, tzOffset). Bounded - // to the shortlist and gated by the picker's per-user rate limiter - // upstream, so a handful of extra Slack calls per empty-selection - // propose. Sequential is fine at this size. + // 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 byEmail = new HashMap(); + var requesterTzFuture = timezoneOffsetFuture(requester.email); + var futures = new HashMap>(); for (var u : shortlist) { - byEmail.put(u.email, availabilityOf(u.email)); + 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 (1) currently-active teammates, then - // (2) those in a working-hours timezone near the requester's. - // Comparator returns 0 for same tier so Java's stable sort - // preserves affinity within a tier. + // 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( @@ -747,10 +753,23 @@ public void releaseClaim(@NotNull Proposal proposal) { 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 candidate reviewer(s) availability-first for %s", - shortlist.size(), requester.email); + "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) { @@ -775,26 +794,57 @@ private int availabilityTier(Availability a, Integer requesterTz) { if (a.active()) { return 0; } - if (a.tzOffsetSeconds() != null && requesterTz != null - && Math.abs(a.tzOffsetSeconds() - requesterTz) <= TZ_PROXIMITY_SECONDS) { - return 1; + 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; } - private @NotNull Availability availabilityOf(@NotNull String email) { - var userId = this.slackClient.lookupUserByEmail(email).join(); - if (userId == null) { - return new Availability(email, false, null); - } - var active = this.slackClient.isActive(userId).join(); - var tz = this.slackClient.timezoneOffsetSeconds(userId).join(); - return new Availability(email, active, tz); + /** + * 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 @Nullable Integer timezoneOffsetOrNull(@NotNull String email) { - var userId = this.slackClient.lookupUserByEmail(email).join(); - return userId == null ? null : this.slackClient.timezoneOffsetSeconds(userId).join(); + 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( 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 bdf5f4d9..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 @@ -455,6 +455,63 @@ public void rankReviewersByAvailability_singletonReturnedUnchanged() { 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). // ------------------------------------------------------------------------- From 84137398415e2a191e89af76972cfc8d38f2e2e5 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 16:12:00 +0200 Subject: [PATCH 3/3] build: 2.3.0-wavemm.13 (availability ranking on top of the review fixes) --- sources/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/pom.xml b/sources/pom.xml index 78a7a24b..bcae8fe4 100644 --- a/sources/pom.xml +++ b/sources/pom.xml @@ -24,7 +24,7 @@ 4.0.0 com.google.solutions jitaccess - 2.3.0-wavemm.12 + 2.3.0-wavemm.13 3.5.3 3.5.3