The default is a no-op: handlers without a consumption store
+ * (mail, debug) keep the upstream replayable semantics. The Slack
+ * handler enforces via its Firestore registry.
+ *
+ * @throws AccessException when the proposal was already consumed
+ */
+ default void verifyNotConsumed(@NotNull Proposal proposal)
+ throws AccessException {
+ }
+
+ /**
+ * Wavemm fork (SECOP-1093): record that this proposal was used to
+ * approve, so subsequent {@link #verifyNotConsumed} calls reject it.
+ * Best-effort by contract — implementations must not fail the
+ * approval that just succeeded; a missed mark merely restores the
+ * upstream replayable behaviour for this one token.
+ */
+ default void markConsumed(@NotNull Proposal proposal) {
+ }
+
/**
* Token that encodes all information about a proposal in a tamper-proof
* way, suitable for exchanging in URLs and/or email messages.
diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java
index ad6ccfc2..6ae4a614 100644
--- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java
+++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java
@@ -56,6 +56,7 @@ public class SlackMessageRegistry {
static final String COLLECTION = "requests";
static final String FIELD_REVIEWERS = "reviewers";
static final String FIELD_EXPIRES_AT = "expires_at";
+ static final String FIELD_CONSUMED = "consumed";
private final @NotNull Firestore firestore;
private final @NotNull Executor executor;
@@ -131,6 +132,21 @@ public SlackMessageRegistry(
beneficiary,
groupId,
String.join(",", sorted));
+ return hmacHex(canonical);
+ }
+
+ /**
+ * Key of the consumption marker for a proposal JWT (SECOP-1093).
+ * Namespaced with a prefix so it can never collide with a
+ * {@link #requestKey} in the shared collection, and HMAC'd for the
+ * same reason request keys are: a Firestore reader without the salt
+ * can't correlate markers with tokens they've seen.
+ */
+ public @NotNull String consumptionKey(@NotNull String proposalId) {
+ return hmacHex("consumed|" + proposalId);
+ }
+
+ private @NotNull String hmacHex(@NotNull String canonical) {
try {
var mac = Mac.getInstance("HmacSHA256");
mac.init(new SecretKeySpec(this.hmacKey, "HmacSHA256"));
@@ -223,6 +239,54 @@ public SlackMessageRegistry(
}, this.executor);
}
+ /**
+ * Check whether a proposal was already used to approve (SECOP-1093).
+ * Missing document → not consumed.
+ */
+ public @NotNull CompletableFuture Flow on {@link #onProposalApproved}:
*
*
@@ -400,21 +403,29 @@ void onProposalApproved(
var siblingBlocks = SlackMessages.reviewerSiblingUpdate(
fp.beneficiary(), fp.groupId(), approverEmail);
var siblingFallback = SlackMessages.reviewerSiblingUpdateFallback(approverEmail);
+ // SECOP-1094: the approver's own DM gets a distinct "you approved
+ // this" update instead of being skipped — leaving it frozen (with a
+ // stale action link) reads as a failure, especially when the
+ // approver was the only recipient and no sibling update happens.
+ var approverBlocks = SlackMessages.reviewerApprovedByYou(
+ fp.beneficiary(), fp.groupId());
+ var approverFallback = SlackMessages.reviewerApprovedByYouFallback(fp.beneficiary());
for (var entry : entriesOpt.get()) {
- if (entry.email().equalsIgnoreCase(approverEmail)) {
- // The approver doesn't need a "you approved" update — they did it.
- continue;
- }
+ var isApprover = entry.email().equalsIgnoreCase(approverEmail);
try {
this.slackClient.updateMessage(
- entry.channelId(), entry.messageTs(), siblingBlocks, siblingFallback).join();
+ entry.channelId(),
+ entry.messageTs(),
+ isApprover ? approverBlocks : siblingBlocks,
+ isApprover ? approverFallback : siblingFallback).join();
}
catch (RuntimeException e) {
var cause = e.getCause() != null ? e.getCause() : e;
this.logger.warn(
"slack.siblingUpdate.failed",
- "Failed to chat.update sibling DM %s/%s for %s: %s",
+ "Failed to chat.update %s DM %s/%s for %s: %s",
+ isApprover ? "approver" : "sibling",
entry.channelId(), entry.messageTs(), entry.email(), cause.getMessage());
}
}
@@ -430,8 +441,76 @@ void onProposalApproved(
this.logger.info(
"slack.onProposalApproved",
- "Updated %d sibling DM(s) for approved request key=%s (approver=%s)",
- Math.max(0, entriesOpt.get().size() - 1), fp.key(), approverEmail);
+ "Updated %d reviewer DM(s) for approved request key=%s (approver=%s)",
+ entriesOpt.get().size(), fp.key(), approverEmail);
+ }
+
+ /**
+ * SECOP-1093: reject approval links that were already used. Fail-open
+ * on Firestore errors — the JWT signature remains the authoritative
+ * authorization; consumption is anti-replay defense-in-depth, and an
+ * approval must not hard-depend on Firestore availability. Failures
+ * are logged at ERROR so an outage window (during which tokens
+ * temporarily regain the upstream replayable semantics) is visible.
+ */
+ @Override
+ public void verifyNotConsumed(@NotNull Proposal proposal)
+ throws AccessException {
+ var proposalId = proposal.id();
+ if (proposalId == null) {
+ // Proposal implementation without a stable id — nothing to check.
+ return;
+ }
+ boolean consumed;
+ try {
+ consumed = this.registry
+ .isProposalConsumed(this.registry.consumptionKey(proposalId))
+ .join();
+ }
+ catch (RuntimeException e) {
+ var cause = e.getCause() != null ? e.getCause() : e;
+ this.logger.error(
+ "slack.consumption.checkFailed",
+ "Failed to check proposal consumption for id=%s; allowing the "
+ + "approval (fail-open) — replay protection is degraded until "
+ + "Firestore recovers. cause=%s",
+ proposalId, cause.getMessage());
+ return;
+ }
+ if (consumed) {
+ throw new AccessDeniedException(
+ "This approval link has already been used. If further access is "
+ + "needed, ask the requester to submit a new request.");
+ }
+ }
+
+ /**
+ * SECOP-1093: record consumption after a successful approval.
+ * Best-effort by contract — the approval already succeeded, so a
+ * failed write only restores replayability for this one token; log
+ * loud and move on.
+ */
+ @Override
+ public void markConsumed(@NotNull Proposal proposal) {
+ var proposalId = proposal.id();
+ if (proposalId == null) {
+ return;
+ }
+ try {
+ this.registry
+ .markProposalConsumed(
+ this.registry.consumptionKey(proposalId),
+ proposal.expiry())
+ .join();
+ }
+ catch (RuntimeException e) {
+ var cause = e.getCause() != null ? e.getCause() : e;
+ this.logger.error(
+ "slack.consumption.markFailed",
+ "Failed to record proposal consumption for id=%s; this token "
+ + "remains replayable until it expires at %s. cause=%s",
+ proposalId, proposal.expiry(), cause.getMessage());
+ }
}
private void notifyBeneficiary(
diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java
index 429d92b4..341cf9c1 100644
--- a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java
+++ b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java
@@ -85,6 +85,11 @@ public class ProposalResource {
proposal.group().environment().equals(environment),
"The token must match the environment");
+ // SECOP-1093: an already-used approval link renders the same
+ // "URL no longer valid" banner as an expired one, instead of an
+ // approval page whose submit would then fail.
+ this.proposalHandler.verifyNotConsumed(proposal);
+
return this.catalog
.group(proposal.group())
.map(grp -> ProposalInfo.create(
@@ -126,6 +131,11 @@ public class ProposalResource {
groupId.environment().equals(environment),
"The token must match the environment");
+ // SECOP-1093: proposal tokens approve exactly once. The check
+ // runs BEFORE the approval executes; the marker is written right
+ // after it succeeds.
+ this.proposalHandler.verifyNotConsumed(proposal);
+
//
// Attempt to approve.
//
@@ -138,6 +148,7 @@ public class ProposalResource {
var principal = approveOp.execute();
this.auditTrail.joinExecuted(approveOp, principal);
+ this.proposalHandler.markConsumed(proposal);
return ProposalInfo.create(
group,
diff --git a/sources/src/main/resources/META-INF/resources/index.html b/sources/src/main/resources/META-INF/resources/index.html
index 9503e1c7..8d699eea 100644
--- a/sources/src/main/resources/META-INF/resources/index.html
+++ b/sources/src/main/resources/META-INF/resources/index.html
@@ -745,7 +745,8 @@ Please wait...
— pick one or more teammates (those tagged
team are likely on yours).
- Leave all unchecked to notify every qualified peer.
+ Leave all unchecked to notify your closest
+ qualified teammates automatically.
Please wait...
.attr('name', 'selectedReviewers')
.attr('value', c.email || '');
// Default unchecked even for suggested. The picker is
- // an opt-in narrowing tool: an untouched form must
- // continue to mean "notify every qualified peer", same
- // as before the picker existed. The badge remains as
- // a visual hint of who likely makes a good reviewer.
+ // an opt-in narrowing tool: an untouched form means
+ // "let the server pick" — since SECOP-952 that is the
+ // requester's closest teammates (small approver sets
+ // still notify everyone). The badge remains a visual
+ // hint of who likely makes a good reviewer.
const display = $('')
.addClass('jit-reviewer-display')
.text(c.displayName || c.email || '');
@@ -852,8 +854,9 @@ Please wait...
.addClass('jit-reviewer-picker-hint')
.text(
'Couldn\'t load the list of qualified reviewers ' +
- '(Cloud Identity API hiccup). Submitting will fall ' +
- 'back to notifying every qualified peer per the policy.'));
+ '(Cloud Identity API hiccup). You can still submit — ' +
+ 'reviewers are chosen automatically server-side, or ' +
+ 'you may be asked to retry once suggestions are back.'));
form.append(notice);
}
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 7f0d3c32..ec4773c6 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
@@ -189,6 +189,24 @@ public void accept_whenTokenVerificationFails() throws Exception {
() -> proposalHandler.accept("token"));
}
+ /**
+ * SECOP-1093: accept() must surface the JWT jti as {@code Proposal.id()}
+ * so the consumption registry can key on it.
+ */
+ @Test
+ public void accept_exposesJwtIdAsProposalId() throws Exception {
+ var token = "{\"jti\":\"abc123\"," +
+ "\"rcp\":[\"user:user-2@example.com\"]," +
+ "\"grp\":\"jit-group:env.sys.group-1\"," +
+ "\"usr\":\"user:user-1@example.com\",\"inp\":{}}";
+
+ var signer = new PseudoSigner();
+ var proposalHandler = new SampleProposalHandler(signer);
+ var proposal = proposalHandler.accept(token);
+
+ assertEquals("abc123", proposal.id());
+ }
+
@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/TestSlackMessageRegistry.java b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessageRegistry.java
index a6097a21..0844366c 100644
--- a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessageRegistry.java
+++ b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessageRegistry.java
@@ -70,6 +70,27 @@ public void requestKey_isInvariantToRecipientOrder() {
+ "may iterate the recipients Set in different orders.");
}
+ // -------------------------------------------------------------------------
+ // consumptionKey (SECOP-1093) — deterministic, salted, and namespaced
+ // away from request keys since both live in the same collection.
+ // -------------------------------------------------------------------------
+
+ @Test
+ public void consumptionKey_isDeterministicAndSaltDependent() {
+ var registry = newRegistry(SALT_A);
+ assertEquals(
+ registry.consumptionKey("jti-1"),
+ registry.consumptionKey("jti-1"));
+ assertNotEquals(
+ registry.consumptionKey("jti-1"),
+ registry.consumptionKey("jti-2"));
+ assertNotEquals(
+ registry.consumptionKey("jti-1"),
+ newRegistry(SALT_B).consumptionKey("jti-1"),
+ "an attacker without the salt must not be able to precompute keys");
+ assertEquals(64, registry.consumptionKey("jti-1").length(), "HMAC-SHA-256 hex");
+ }
+
@Test
public void requestKey_distinguishesBeneficiary() {
var registry = newRegistry(SALT_A);
diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java
index 9921cc94..6155dd7d 100644
--- a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java
+++ b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java
@@ -162,6 +162,23 @@ public void reviewerSiblingUpdate_namesApprover() {
"sibling-update message must name the approver so reviewers know who acted");
}
+ // -------------------------------------------------------------------------
+ // reviewerApprovedByYou — replaces the approver's own DM (SECOP-1094).
+ // -------------------------------------------------------------------------
+
+ @Test
+ public void reviewerApprovedByYou_dropsActionButtonAndNamesRequester() {
+ var blocks = SlackMessages.reviewerApprovedByYou(
+ "alice@example.com", "env/sys/grp");
+
+ assertFalse(blocks.stream().anyMatch(ActionsBlock.class::isInstance),
+ "the approver's post-approval message must not carry the action link");
+ var serialized = blocks.toString();
+ assertTrue(serialized.contains("alice@example.com"));
+ assertTrue(serialized.contains("env/sys/grp"));
+ assertTrue(serialized.contains("You approved"));
+ }
+
// -------------------------------------------------------------------------
// beneficiaryApproved — confirms to requester that the elevation landed.
// -------------------------------------------------------------------------
@@ -188,6 +205,8 @@ public void fallbackStrings_containKeyFields() {
.contains("alice@example.com"));
assertTrue(SlackMessages.reviewerSiblingUpdateFallback("bob@example.com")
.contains("bob@example.com"));
+ assertTrue(SlackMessages.reviewerApprovedByYouFallback("alice@example.com")
+ .contains("alice@example.com"));
assertTrue(SlackMessages.beneficiaryApprovedFallback("env/sys/grp", "bob@example.com")
.contains("env/sys/grp"));
}
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 a95fa2a0..3297d59c 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
@@ -11,6 +11,7 @@
package com.google.solutions.jitaccess.web.proposal;
import com.google.solutions.jitaccess.apis.Logger;
+import com.google.solutions.jitaccess.apis.clients.AccessDeniedException;
import com.google.solutions.jitaccess.auth.EndUserId;
import com.google.solutions.jitaccess.auth.GroupResolver;
import com.google.solutions.jitaccess.auth.IamPrincipalId;
@@ -231,7 +232,7 @@ public void onOperationProposed_throwsWhenNoIndividualUsersAfterExpansion() thro
// -------------------------------------------------------------------------
@Test
- public void onProposalApproved_updatesSiblingsButNotApprover() throws Exception {
+ public void onProposalApproved_updatesSiblingsAndApproverDistinctly() throws Exception {
var slack = slackClientHappyPath();
var registry = mock(SlackMessageRegistry.class);
var entries = List.of(
@@ -255,9 +256,18 @@ public void onProposalApproved_updatesSiblingsButNotApprover() throws Exception
approval,
proposalFor(ALICE, Set.