[i,l,-r] Slack approval follow-ups: single-use JWTs, approver DM update, picker copy (SECOP-1093/1094/1095)

sources/pom.xml

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

sources/src/main/java/com/google/solutions/jitaccess/catalog/Proposal.java

@@ -87,6 +87,18 @@ default boolean notifyReviewers() {
     return true;
   }
 
+  /**
+   * Wavemm fork (SECOP-1093): stable, unpredictable identifier of this
+   * proposal — for token-backed proposals, the JWT {@code jti} claim.
+   * Used to record consumption so an approval link can be used exactly
+   * once. {@code null} when the proposal implementation has no such
+   * identifier; consumers must treat that as "consumption tracking not
+   * available" and skip enforcement rather than fail.
+   */
+  default String id() {
+    return null;
+  }
+
   /**
    * Invoked when the proposal was completed successfully.
    */

sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java

@@ -206,6 +206,13 @@ abstract void onProposalApproved(
         return user;
       }
 
+      @Override
+      public String id() {
+        // The jti minted at propose-time: 6 crypto-random bytes, base64.
+        // Anchors single-use consumption tracking (SECOP-1093).
+        return payload.getJwtId();
+      }
+
       @Override
       public @NotNull JitGroupId group() {
         return group;

sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java

@@ -92,6 +92,32 @@ record ProposeOptions(
     @NotNull String proposalToken
   ) throws AccessException;
 
+  /**
+   * Wavemm fork (SECOP-1093): reject proposals whose token was already
+   * used to approve. Proposal tokens are stateless JWTs — without a
+   * consumption record, one approval link authorizes unlimited re-grants
+   * for the token lifetime (~1h), each resetting the membership clock.
+   *
+   * <p>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.

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<Boolean> isProposalConsumed(
+    @NotNull String consumptionKey
+  ) {
+    return CompletableFutures.supplyAsync(() -> {
+      try {
+        return collection().document(consumptionKey).get().get().exists();
+      }
+      catch (InterruptedException | ExecutionException e) {
+        Thread.currentThread().interrupt();
+        throw new RuntimeException(
+          "Failed to read proposal consumption marker " + consumptionKey, e);
+      }
+    }, this.executor);
+  }
+
+  /**
+   * Record that a proposal was used to approve (SECOP-1093). The marker
+   * lives in the same collection as request entries so the existing
+   * Firestore TTL policy on {@value #FIELD_EXPIRES_AT} reaps it — the
+   * expiry passed here must be the token expiry, since a marker is only
+   * meaningful while the token it blocks is still valid.
+   */
+  public @NotNull CompletableFuture<Void> markProposalConsumed(
+    @NotNull String consumptionKey,
+    @NotNull Instant expiresAt
+  ) {
+    return CompletableFutures.supplyAsync(() -> {
+      var doc = new HashMap<String, Object>();
+      doc.put(FIELD_EXPIRES_AT, Timestamp.ofTimeSecondsAndNanos(
+        expiresAt.getEpochSecond(),
+        expiresAt.getNano()));
+      doc.put(FIELD_CONSUMED, true);
+      try {
+        collection().document(consumptionKey).set(doc).get();
+      }
+      catch (InterruptedException | ExecutionException e) {
+        Thread.currentThread().interrupt();
+        throw new RuntimeException(
+          "Failed to write proposal consumption marker " + consumptionKey, e);
+      }
+      return null;
+    }, this.executor);
+  }
+
   /**
    * Remove a request entry once the approval flow is complete. Idempotent.
    */

sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessages.java

@@ -166,6 +166,34 @@ static List<LayoutBlock> reviewerSiblingUpdate(
         .build());
   }
 
+  /**
+   * Replaces the approver's OWN request DM after they approve
+   * (SECOP-1094). Without this the approver's message keeps its stale
+   * action link and nothing on their screen acknowledges the approval —
+   * in the picked-single-reviewer flow that reads as "did that even
+   * work?". The action link is removed, same as the sibling update.
+   */
+  static List<LayoutBlock> reviewerApprovedByYou(
+    @NotNull String requesterEmail,
+    @NotNull String groupId
+  ) {
+    return List.of(
+      HeaderBlock.builder()
+        .text(plain(":white_check_mark: You approved this request"))
+        .build(),
+      SectionBlock.builder()
+        .fields(List.of(
+          markdownField("*Requester*", "<mailto:" + requesterEmail + "|" + requesterEmail + ">"),
+          markdownField("*Group*", "`" + groupId + "`")
+        ))
+        .build(),
+      ContextBlock.builder()
+        .elements(List.of(markdown(
+          ":information_source: Nothing more to do — the requester has "
+            + "been notified.")))
+        .build());
+  }
+
   /**
    * DM to the beneficiary confirming the activation completed.
    */
@@ -202,6 +230,10 @@ static String reviewerSiblingUpdateFallback(@NotNull String approverEmail) {
     return "Already approved by " + approverEmail + " — no action needed.";
   }
 
+  static String reviewerApprovedByYouFallback(@NotNull String requesterEmail) {
+    return "You approved " + requesterEmail + "'s request — nothing more to do.";
+  }
+
   static String beneficiaryApprovedFallback(@NotNull String groupId, @NotNull String approverEmail) {
     return String.format("Your PAM elevation for %s was approved by %s.", groupId, approverEmail);
   }

sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java

@@ -12,6 +12,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.solutions.jitaccess.apis.Logger;
+import com.google.solutions.jitaccess.apis.clients.AccessDeniedException;
 import com.google.solutions.jitaccess.apis.clients.AccessException;
 import com.google.solutions.jitaccess.auth.EndUserId;
 import com.google.solutions.jitaccess.auth.GroupResolver;
@@ -53,8 +54,10 @@
  * <p>Flow on {@link #onProposalApproved}:
  * <ol>
  *   <li>Look up the registry entry by request key.
- *   <li>For each non-approver sibling, {@code chat.update} the original DM
- *       to "Already approved by X — no action needed".
+ *   <li>{@code chat.update} every recorded DM in place: non-approver
+ *       siblings get "Already approved by X — no action needed", the
+ *       approver's own message gets "You approved this request"
+ *       (SECOP-1094).
  *   <li>DM the beneficiary "Your elevation was approved by X".
  *   <li>Delete the registry entry.
  * </ol>
@@ -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(

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,