From f17f791801db1f43566e86b14dfb4de63193bf08 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Wed, 22 Jul 2026 10:24:31 +0200 Subject: [PATCH 1/2] Approval-link follow-ups: expired-link message, pending release on approval, canonical link host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related fixes around proposal tokens, prompted by the 2026-07-22 production incident where every overnight approval link 403'd with "The proposal token is invalid" (the tokens were merely expired — see trace db0322e6a6b1861fb22321941bf771e6): 1. accept() now distinguishes an expired token from a genuinely invalid one and says so, including the configured validity window and what to do next. The expiry probe parses the payload without signature verification and only ever selects the error message. 2. Approving a proposal now releases the SECOP-1101 duplicate-submit reservation (both key variants, since the variant isn't encoded in the token). Without this, raising APPROVAL_TIMEOUT would block re-requests of the same group for the full token lifetime after an approval. 3. New optional PUBLIC_BASE_URL setting + CanonicalLinkBuilder: approval links generated from requests on the app's *default* appspot host are rewritten to the canonical URL (pam.wavemm.net in prod). Versioned staging hosts (rev-…-dot-…appspot.com), custom domains, and localhost are never rewritten, so promote_traffic=false smoke tests keep versioned links. Full unit suite: 1070 tests, 0 failures (ITest* skipped, need GCP env). Co-Authored-By: Claude Fable 5 --- sources/mvnw | 0 .../solutions/jitaccess/web/Application.java | 6 +- .../web/ApplicationConfiguration.java | 31 ++++ .../jitaccess/web/CanonicalLinkBuilder.java | 88 ++++++++++ .../web/proposal/AbstractProposalHandler.java | 81 ++++++++- .../web/TestApplicationConfiguration.java | 44 +++++ .../web/TestCanonicalLinkBuilder.java | 154 ++++++++++++++++++ .../proposal/TestAbstractProposalHandler.java | 109 +++++++++++++ .../proposal/TestSlackProposalHandler.java | 34 ++++ 9 files changed, 542 insertions(+), 5 deletions(-) mode change 100644 => 100755 sources/mvnw create mode 100644 sources/src/main/java/com/google/solutions/jitaccess/web/CanonicalLinkBuilder.java create mode 100644 sources/src/test/java/com/google/solutions/jitaccess/web/TestCanonicalLinkBuilder.java diff --git a/sources/mvnw b/sources/mvnw old mode 100644 new mode 100755 diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java b/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java index 330328579..8d4a4bab8 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java @@ -243,9 +243,9 @@ public GoogleCredentials produceApplicationCredentials() { @Produces public @NotNull LinkBuilder produceLinkBuilder() { - return uriInfo -> uriInfo - .getBaseUriBuilder() - .scheme(runtime.type() == ApplicationRuntime.Type.DEVELOPMENT + return new CanonicalLinkBuilder( + configuration.publicBaseUrl.orElse(null), + runtime.type() == ApplicationRuntime.Type.DEVELOPMENT ? "http" : "https"); } diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java b/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java index dd77d3804..b667d7a26 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java @@ -26,6 +26,8 @@ import com.google.solutions.jitaccess.apis.OrganizationId; import org.jetbrains.annotations.NotNull; +import java.net.URI; +import java.net.URISyntaxException; import java.time.Duration; import java.time.ZoneId; import java.time.ZoneOffset; @@ -221,6 +223,17 @@ public class ApplicationConfiguration extends AbstractConfiguration { */ final boolean slackCopyLinkEnabled; + /** + * Canonical base URL for generated links, e.g. the approval links + * fanned out to reviewers (wavemm fork). When set, links built from + * requests that arrived on the app's default appspot host are + * rewritten to this URL. Versioned (…-dot-…) staging hosts, custom + * domains, and localhost are never rewritten — see + * {@link CanonicalLinkBuilder}. Optional; when unset, links inherit + * the requesting user's host (upstream behaviour). + */ + final @NotNull Optional publicBaseUrl; + public ApplicationConfiguration(@NotNull Map settingsData) { super(settingsData); @@ -352,6 +365,24 @@ public ApplicationConfiguration(@NotNull Map settingsData) { this.slackCopyLinkEnabled = readSetting( Boolean::parseBoolean, "SLACK_COPY_LINK_ENABLED") .orElse(false); + this.publicBaseUrl = readStringSetting("PUBLIC_BASE_URL") + .map(url -> { + try { + var uri = new URI(url); + if (uri.getHost() == null || + !("https".equalsIgnoreCase(uri.getScheme()) || + "http".equalsIgnoreCase(uri.getScheme()))) { + throw new URISyntaxException(url, "not an absolute http(s) URL"); + } + return uri; + } + catch (URISyntaxException e) { + throw new IllegalStateException( + "The environment variable 'PUBLIC_BASE_URL' must contain an " + + "absolute http(s) URL", + e); + } + }); } public boolean isSmtpConfigured() { diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/CanonicalLinkBuilder.java b/sources/src/main/java/com/google/solutions/jitaccess/web/CanonicalLinkBuilder.java new file mode 100644 index 000000000..a4e46534a --- /dev/null +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/CanonicalLinkBuilder.java @@ -0,0 +1,88 @@ +// +// Copyright 2026 Google LLC +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +package com.google.solutions.jitaccess.web; + +import jakarta.ws.rs.core.UriBuilder; +import jakarta.ws.rs.core.UriInfo; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.net.URI; +import java.util.Locale; + +/** + * Wavemm fork: link builder that rewrites generated links to a canonical + * public URL when — and only when — the request arrived on the app's + * default appspot host. + * + *

Generated links (e.g. the approval link fanned out to reviewers in + * Slack DMs) otherwise inherit whatever host the requester was browsing: + * a request submitted on the default appspot host sends every reviewer + * an appspot link even though the app has a custom domain. + * + *

Versioned App Engine hosts (rev-…-dot-…appspot.com) are deliberately + * left untouched: they are the staging surface (deploys with + * promote_traffic=false are smoke-tested on the versioned URL, see + * SLACK_INTEGRATION.md in wavemm-iam), and rewriting them would point + * staging-generated links at the traffic-promoted production version. + * Custom domains and localhost pass through unchanged, too. + */ +public class CanonicalLinkBuilder implements LinkBuilder { + private final @Nullable URI canonicalBaseUri; + private final @NotNull String fallbackScheme; + + public CanonicalLinkBuilder( + @Nullable URI canonicalBaseUri, + @NotNull String fallbackScheme + ) { + this.canonicalBaseUri = canonicalBaseUri; + this.fallbackScheme = fallbackScheme; + } + + @Override + public UriBuilder absoluteUriBuilder(@NotNull UriInfo uriInfo) { + if (this.canonicalBaseUri != null && + isDefaultAppspotHost(uriInfo.getBaseUri().getHost())) { + return UriBuilder.fromUri(this.canonicalBaseUri); + } + + return uriInfo + .getBaseUriBuilder() + .scheme(this.fallbackScheme); + } + + /** + * The app's default appspot hosts (project.appspot.com or + * project.REGION.r.appspot.com) route to the traffic-promoted version, + * so links on them can safely be canonicalized. Versioned hosts always + * contain a {@code -dot-} separator (VERSION-dot-SERVICE-dot-project…) + * and must never match. + */ + static boolean isDefaultAppspotHost(@Nullable String host) { + if (host == null) { + return false; + } + + var normalized = host.toLowerCase(Locale.ROOT); + return normalized.endsWith(".appspot.com") && !normalized.contains("-dot-"); + } +} diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java index b3077879c..6a5433b6c 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java @@ -22,6 +22,7 @@ package com.google.solutions.jitaccess.web.proposal; import com.google.api.client.json.GenericJson; +import com.google.api.client.json.gson.GsonFactory; import com.google.api.client.json.webtoken.JsonWebToken; import com.google.auth.oauth2.TokenVerifier; import com.google.common.base.Preconditions; @@ -37,6 +38,7 @@ import java.io.IOException; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.time.Duration; import java.time.Instant; import java.util.*; @@ -231,8 +233,11 @@ boolean reserveProposal( /** * Wavemm fork (SECOP-1101): release a reservation taken by - * {@link #reserveProposal} when the propose failed after reserving. - * Best-effort; default no-op. + * {@link #reserveProposal} — either because the propose failed after + * reserving, or because the proposal was approved and the request is + * no longer pending. Best-effort; default no-op. Implementations must + * tolerate keys that were never reserved (approval releases both key + * variants because the variant isn't recoverable from the token). */ void releaseProposalReservation( @NotNull Proposal proposal, @@ -251,6 +256,22 @@ void releaseProposalReservation( payload = this.tokenSigner.verify(proposalToken); } catch (TokenVerifier.VerificationException e) { + // + // Wavemm fork: expiry is the common verification failure + // (a reviewer clicking an approval link hours after it was + // minted) and deserves an actionable message. isProbablyExpired + // parses the payload without verifying the signature, so it may + // only ever pick the error message — both paths deny access. + // + if (isProbablyExpired(proposalToken)) { + throw new AccessDeniedException( + String.format( + "This approval link has expired: links can only be approved " + + "within %s of the request being made. Ask the requester " + + "to submit a new request.", + formatApproximateDuration(this.options.tokenExpiry)), + e); + } throw new AccessDeniedException("The proposal token is invalid", e); } @@ -321,11 +342,67 @@ public boolean notifyReviewers() { public void onCompleted( @NotNull JitGroupContext.ApprovalOperation op ) throws AccessException, IOException { + // + // The approval executed, so this request is no longer pending: + // release the duplicate-submit reservation (SECOP-1101) so the + // user can request the same group again without waiting for + // this token to expire — with APPROVAL_TIMEOUT well above the + // elevation duration, same-day re-requests are routine. The + // token doesn't record which key variant propose() reserved + // (auto-selected vs hand-picked reviewers), so release both; + // releases are idempotent and best-effort. + // + releaseProposalReservation(this, true); + releaseProposalReservation(this, false); onProposalApproved(op, this); } }; } + /** + * Best-effort check whether a token that failed verification is a + * well-formed JWT whose expiry has passed. The payload is parsed + * WITHOUT verifying the signature, so the result may only be used to + * choose an error message, never to authorize. + */ + private static boolean isProbablyExpired(@NotNull String jwt) { + try { + var parts = jwt.split("\\."); + if (parts.length != 3) { + return false; + } + + var payloadJson = new String( + Base64.getUrlDecoder().decode(parts[1]), + StandardCharsets.UTF_8); + try (var parser = new GsonFactory().createJsonParser(payloadJson)) { + var expiry = parser + .parse(JsonWebToken.Payload.class) + .getExpirationTimeSeconds(); + return expiry != null && Instant.now().getEpochSecond() > expiry; + } + } + catch (Exception e) { + return false; + } + } + + private static @NotNull String formatApproximateDuration( + @NotNull Duration duration + ) { + var minutes = duration.toMinutes(); + if (minutes == 0) { + return duration.toSeconds() + " seconds"; + } + else if (minutes % 60 == 0) { + var hours = minutes / 60; + return hours == 1 ? "1 hour" : hours + " hours"; + } + else { + return minutes == 1 ? "1 minute" : minutes + " minutes"; + } + } + static class Claims { static final String RECIPIENT = "rcp"; static final String GROUP_ID = "grp"; diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/TestApplicationConfiguration.java b/sources/src/test/java/com/google/solutions/jitaccess/web/TestApplicationConfiguration.java index f0135f472..90004cc90 100644 --- a/sources/src/test/java/com/google/solutions/jitaccess/web/TestApplicationConfiguration.java +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/TestApplicationConfiguration.java @@ -24,6 +24,7 @@ import com.google.solutions.jitaccess.apis.CustomerId; import org.junit.jupiter.api.Test; +import java.net.URI; import java.util.HashMap; import java.util.Map; @@ -180,4 +181,47 @@ public void smtp_whenProvided() { assertEquals("2", configuration.smtpExtraOptionsMap().get("b")); } + + // ------------------------------------------------------------------------- + // publicBaseUrl (wavemm fork). + // ------------------------------------------------------------------------- + + @Test + public void publicBaseUrl_whenNotSet() { + var configuration = new ApplicationConfiguration(createMandatorySettings()); + + assertFalse(configuration.publicBaseUrl.isPresent()); + } + + @Test + public void publicBaseUrl_whenSet() { + var settings = new HashMap<>(createMandatorySettings()); + settings.put("PUBLIC_BASE_URL", " https://pam.example.com "); + + var configuration = new ApplicationConfiguration(settings); + + assertEquals( + URI.create("https://pam.example.com"), + configuration.publicBaseUrl.get()); + } + + @Test + public void publicBaseUrl_whenSchemeNotHttp_thenThrows() { + var settings = new HashMap<>(createMandatorySettings()); + settings.put("PUBLIC_BASE_URL", "ftp://pam.example.com"); + + assertThrows( + IllegalStateException.class, + () -> new ApplicationConfiguration(settings)); + } + + @Test + public void publicBaseUrl_whenNotAbsolute_thenThrows() { + var settings = new HashMap<>(createMandatorySettings()); + settings.put("PUBLIC_BASE_URL", "pam.example.com"); + + assertThrows( + IllegalStateException.class, + () -> new ApplicationConfiguration(settings)); + } } diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/TestCanonicalLinkBuilder.java b/sources/src/test/java/com/google/solutions/jitaccess/web/TestCanonicalLinkBuilder.java new file mode 100644 index 000000000..90055df2c --- /dev/null +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/TestCanonicalLinkBuilder.java @@ -0,0 +1,154 @@ +// +// Copyright 2026 Google LLC +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +package com.google.solutions.jitaccess.web; + +import jakarta.ws.rs.core.UriBuilder; +import jakarta.ws.rs.core.UriInfo; +import org.jetbrains.annotations.NotNull; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import java.net.URI; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.when; + +public class TestCanonicalLinkBuilder { + private static final URI CANONICAL_URI = URI.create("https://pam.example.com"); + + private static UriInfo uriInfoFor(@NotNull String baseUri) { + var uriInfo = Mockito.mock(UriInfo.class); + when(uriInfo.getBaseUri()) + .thenReturn(URI.create(baseUri)); + when(uriInfo.getBaseUriBuilder()) + .thenAnswer(a -> UriBuilder.fromUri(baseUri)); + return uriInfo; + } + + // ------------------------------------------------------------------------- + // absoluteUriBuilder. + // ------------------------------------------------------------------------- + + @Test + public void absoluteUriBuilder_whenDefaultAppspotHost_thenUsesCanonicalUri() { + var linkBuilder = new CanonicalLinkBuilder(CANONICAL_URI, "https"); + + var uri = linkBuilder + .absoluteUriBuilder(uriInfoFor("https://project.nw.r.appspot.com/")) + .path("/") + .build(); + + assertEquals(URI.create("https://pam.example.com/"), uri); + } + + @Test + public void absoluteUriBuilder_whenLegacyAppspotHost_thenUsesCanonicalUri() { + var linkBuilder = new CanonicalLinkBuilder(CANONICAL_URI, "https"); + + var uri = linkBuilder + .absoluteUriBuilder(uriInfoFor("https://project.appspot.com/")) + .path("/") + .build(); + + assertEquals(URI.create("https://pam.example.com/"), uri); + } + + /** + * Versioned hosts are the staging surface (promote_traffic=false + * deploys are smoke-tested on them) — links they generate must keep + * pointing at the staged version, not the promoted one. + */ + @Test + public void absoluteUriBuilder_whenVersionedAppspotHost_thenKeepsRequestHost() { + var linkBuilder = new CanonicalLinkBuilder(CANONICAL_URI, "https"); + + var uri = linkBuilder + .absoluteUriBuilder(uriInfoFor( + "https://rev-abc123-dot-default-dot-project.nw.r.appspot.com/")) + .path("/") + .build(); + + assertEquals( + URI.create("https://rev-abc123-dot-default-dot-project.nw.r.appspot.com/"), + uri); + } + + @Test + public void absoluteUriBuilder_whenCustomDomain_thenKeepsRequestHost() { + var linkBuilder = new CanonicalLinkBuilder(CANONICAL_URI, "https"); + + var uri = linkBuilder + .absoluteUriBuilder(uriInfoFor("https://pam.other.example.com/")) + .path("/") + .build(); + + assertEquals(URI.create("https://pam.other.example.com/"), uri); + } + + @Test + public void absoluteUriBuilder_whenCanonicalUriNotConfigured_thenKeepsRequestHost() { + var linkBuilder = new CanonicalLinkBuilder(null, "https"); + + var uri = linkBuilder + .absoluteUriBuilder(uriInfoFor("http://project.nw.r.appspot.com/")) + .path("/") + .build(); + + assertEquals(URI.create("https://project.nw.r.appspot.com/"), uri); + } + + @Test + public void absoluteUriBuilder_whenDevelopment_thenKeepsHttpScheme() { + var linkBuilder = new CanonicalLinkBuilder(null, "http"); + + var uri = linkBuilder + .absoluteUriBuilder(uriInfoFor("http://localhost:8080/")) + .path("/") + .build(); + + assertEquals(URI.create("http://localhost:8080/"), uri); + } + + // ------------------------------------------------------------------------- + // isDefaultAppspotHost. + // ------------------------------------------------------------------------- + + @Test + public void isDefaultAppspotHost() { + assertTrue(CanonicalLinkBuilder.isDefaultAppspotHost( + "project.appspot.com")); + assertTrue(CanonicalLinkBuilder.isDefaultAppspotHost( + "project.nw.r.appspot.com")); + assertTrue(CanonicalLinkBuilder.isDefaultAppspotHost( + "PROJECT.NW.R.APPSPOT.COM")); + + assertFalse(CanonicalLinkBuilder.isDefaultAppspotHost(null)); + assertFalse(CanonicalLinkBuilder.isDefaultAppspotHost( + "rev-abc123-dot-default-dot-project.nw.r.appspot.com")); + assertFalse(CanonicalLinkBuilder.isDefaultAppspotHost( + "pam.example.com")); + assertFalse(CanonicalLinkBuilder.isDefaultAppspotHost( + "localhost")); + assertFalse(CanonicalLinkBuilder.isDefaultAppspotHost( + "project.appspot.com.evil.example.com")); + } +} 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 0ff573d23..6c718e0be 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 @@ -37,8 +37,11 @@ import java.io.IOException; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.time.Duration; import java.time.Instant; +import java.util.ArrayList; +import java.util.Base64; import java.util.List; import java.util.Map; import java.util.Random; @@ -258,6 +261,112 @@ public void accept_whenInputNotEmpty() throws Exception { assertEquals("value1", proposal.input().get("prop1")); } + /** + * Expired links get an actionable message instead of the generic + * "invalid" one. The expiry check parses the payload without + * verifying the signature (it only selects the message), so it must + * work on a token whose verification failed. + */ + @Test + public void accept_whenTokenExpired_thenThrowsWithExpiryMessage() throws Exception { + var signer = Mockito.mock(TokenSigner.class); + when(signer.verify(anyString())) + .thenThrow(new TokenVerifier.VerificationException("Token is expired")); + + var expiredJwt = String.join( + ".", + base64UrlEncode("{\"alg\":\"RS256\",\"typ\":\"JWT\"}"), + base64UrlEncode("{\"exp\":1000}"), + "untrusted-signature"); + + var proposalHandler = new SampleProposalHandler(signer); + var exception = assertThrows( + AccessDeniedException.class, + () -> proposalHandler.accept(expiredJwt)); + + assertTrue(exception.getMessage().contains("expired")); + assertTrue(exception.getMessage().contains("1 minute")); + } + + @Test + public void accept_whenTokenNotExpiredButInvalid_thenThrowsGenericMessage() throws Exception { + var signer = Mockito.mock(TokenSigner.class); + when(signer.verify(anyString())) + .thenThrow(new TokenVerifier.VerificationException("Invalid signature")); + + var unexpiredJwt = String.join( + ".", + base64UrlEncode("{\"alg\":\"RS256\",\"typ\":\"JWT\"}"), + base64UrlEncode( + "{\"exp\":" + Instant.now().plusSeconds(3600).getEpochSecond() + "}"), + "untrusted-signature"); + + var proposalHandler = new SampleProposalHandler(signer); + var exception = assertThrows( + AccessDeniedException.class, + () -> proposalHandler.accept(unexpiredJwt)); + + assertEquals("The proposal token is invalid", exception.getMessage()); + } + + @Test + public void accept_whenTokenMalformed_thenThrowsGenericMessage() throws Exception { + var signer = Mockito.mock(TokenSigner.class); + when(signer.verify(anyString())) + .thenThrow(new TokenVerifier.VerificationException("Token is expired")); + + var proposalHandler = new SampleProposalHandler(signer); + var exception = assertThrows( + AccessDeniedException.class, + () -> proposalHandler.accept("not-a-jwt")); + + assertEquals("The proposal token is invalid", exception.getMessage()); + } + + private static String base64UrlEncode(String json) { + return Base64.getUrlEncoder() + .withoutPadding() + .encodeToString(json.getBytes(StandardCharsets.UTF_8)); + } + + /** + * Approval must release the SECOP-1101 pending-request reservation + * (both key variants — the variant used at propose-time isn't encoded + * in the token) so the beneficiary can re-request the group without + * waiting out the token TTL. + */ + @Test + public void accept_onCompletedReleasesReservationAndNotifies() throws Exception { + var released = new ArrayList(); + var notified = new ArrayList(); + var handler = new SampleProposalHandler(new PseudoSigner()) { + @Override + void releaseProposalReservation( + @NotNull Proposal proposal, + boolean reviewersAutoSelected + ) { + released.add(reviewersAutoSelected); + } + + @Override + void onProposalApproved( + @NotNull JitGroupContext.ApprovalOperation operation, + @NotNull Proposal proposal + ) { + notified.add(proposal); + } + }; + + var token = "{\"rcp\":[\"user:user-2@example.com\"]," + + "\"grp\":\"jit-group:env.sys.group-1\"," + + "\"usr\":\"user:user-1@example.com\",\"inp\":{}}"; + var proposal = handler.accept(token); + proposal.onCompleted(Mockito.mock(JitGroupContext.ApprovalOperation.class)); + + assertEquals(List.of(true, false), released); + assertEquals(List.of(proposal), notified); + } + /** * Wavemm fork P1-2: a token without the `ntf` claim (legacy or * propose-time default) decodes with {@code notifyReviewers() = true}, 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 cd84ccd90..82aac97de 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 @@ -866,4 +866,38 @@ public void onOperationProposed_fanOutHonoursConcurrencyCap() throws Exception { "registry must record one entry per reviewer regardless of " + "fan-out concurrency cap"); } + + // ------------------------------------------------------------------------- + // releaseProposalReservation (SECOP-1101). + // ------------------------------------------------------------------------- + + /** + * Approval releases both pending-key variants (the variant used at + * propose-time isn't recoverable from the token), so both must map to + * a registry release with the right key derivation. + */ + @Test + public void releaseProposalReservation_releasesAutoAndPickedVariants() { + var registry = mock(SlackMessageRegistry.class); + when(registry.pendingKey(anyString(), anyString(), isNull())) + .thenReturn("pending-auto"); + when(registry.pendingKey(anyString(), anyString(), anyList())) + .thenReturn("pending-picked"); + when(registry.releasePendingProposal(anyString())) + .thenReturn(CompletableFuture.completedFuture(null)); + var handler = newHandler( + slackClientHappyPath(), registry, groupResolverPassthrough()); + + var proposal = proposalFor(ALICE, Set.of(BOB)); + handler.releaseProposalReservation(proposal, true); + handler.releaseProposalReservation(proposal, false); + + verify(registry).pendingKey( + eq("alice@example.com"), eq(GROUP.toString()), isNull()); + verify(registry).pendingKey( + eq("alice@example.com"), eq(GROUP.toString()), + eq(List.of("bob@example.com"))); + verify(registry).releasePendingProposal(eq("pending-auto")); + verify(registry).releasePendingProposal(eq("pending-picked")); + } } From 57bd0821fb89552a4460e7c05d5b0d0b20d2b874 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Wed, 22 Jul 2026 10:59:30 +0200 Subject: [PATCH 2/2] Bump version to 2.3.0-wavemm.14 Co-Authored-By: Claude Fable 5 --- sources/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/pom.xml b/sources/pom.xml index bcae8fe4a..292e1f2ba 100644 --- a/sources/pom.xml +++ b/sources/pom.xml @@ -24,7 +24,7 @@ 4.0.0 com.google.solutions jitaccess - 2.3.0-wavemm.13 + 2.3.0-wavemm.14 3.5.3 3.5.3