[i,l,-r] Adversarial-review fixes: single-use race, dedupe redesign, IAM retry, requester honesty (SECOP-1100/1101/1102/1103)

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.11</version>
+  <version>2.3.0-wavemm.12</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/apis/clients/AbstractIamClient.java

@@ -45,11 +45,14 @@ public abstract class AbstractIamClient {
    * Bounded retries for the "member not in permitted organization"
    * propagation race (SECOP-1096). Separate budget from the
    * concurrency-control attempts so a slow propagation can't starve
-   * 412 handling. 3 attempts at 2s/4s/8s ≈ 14s covers the lag observed
-   * in production (a newly-created JIT group took ~15–26s to become
-   * resolvable by the org-policy member-domain checker).
+   * 412 handling. 4 attempts at 2s/4s/8s/16s ≈ 30s (SECOP-1102, up from
+   * 3/14s) covers the full lag observed in production — a newly-created
+   * JIT group took ~15–26s to become resolvable by the org-policy
+   * member-domain checker, and the old 14s budget failed the upper half
+   * of that range. This path only runs on the first-ever join of a new
+   * org-scoped role, so the rare worst-case wait is acceptable.
    */
-  private static final int MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS = 3;
+  private static final int MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS = 4;
 
   private static boolean isRoleNotGrantableErrorMessage(@Nullable String message)
   {
@@ -196,8 +199,25 @@ public void modifyIamPolicy(
             catch (InterruptedException ignored) {
             }
           }
-          else if (isMemberDomainPropagationError(e)
-            && memberDomainRetries < MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS) {
+          else if (isMemberDomainPropagationError(e)) {
+            if (memberDomainRetries >= MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS) {
+              //
+              // SECOP-1102: retries exhausted. After ~30s the membership
+              // still isn't resolvable, so this is almost certainly a
+              // genuine domain-restriction denial rather than propagation
+              // lag. Surface it honestly — the pre-existing outer
+              // 400->401 fallthrough would mislabel it
+              // NotAuthenticatedException("Not authenticated"), sending
+              // operators to debug auth/OAuth instead of the org policy.
+              //
+              throw new AccessDeniedException(String.format(
+                "Modifying the IAM policy of '%s' was denied by the "
+                  + "iam.allowedPolicyMemberDomains org policy: a bound "
+                  + "principal is outside the permitted organization "
+                  + "domains. (If this is a newly-created group, its "
+                  + "membership had not propagated after ~30s of retries.)",
+                fullResourcePath), e);
+            }
             //
             // SECOP-1096: a just-provisioned group isn't resolvable by
             // the org-policy member-domain checker yet. Back off longer
@@ -207,7 +227,7 @@ else if (isMemberDomainPropagationError(e)
             //
             memberDomainRetries++;
             try {
-              Thread.sleep(2000L << (memberDomainRetries - 1)); // 2s, 4s, 8s
+              Thread.sleep(2000L << (memberDomainRetries - 1)); // 2s, 4s, 8s, 16s
             }
             catch (InterruptedException ignored) {
             }

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

@@ -104,8 +104,9 @@ abstract void onProposalApproved(
     @NotNull ProposeOptions options
   ) throws AccessException {
 
+    var expiry = Instant.now().plus(this.options.tokenExpiry);
     var proposal = joinOperation.propose(
-      Instant.now().plus(this.options.tokenExpiry),
+      expiry,
       options.reviewerFilter());
 
     Preconditions.checkArgument(
@@ -115,6 +116,27 @@ abstract void onProposalApproved(
       !proposal.recipients().contains(proposal.user()),
       "Recipients must not contain the requesting user");
 
+    //
+    // SECOP-1101: reject duplicate in-flight requests BEFORE minting a
+    // token. Atomically reserve a pending marker; if one already exists
+    // (and hasn't expired), this is a re-submit of a request whose
+    // reviewers were already notified — throw rather than mint a second
+    // approvable token and fan out a second DM batch. Copy-link
+    // (notifyReviewers=false) is exempt: it sends no DMs and each call
+    // legitimately wants its own link. The reservation is released below
+    // if minting/notification fails, so a genuine retry isn't locked out.
+    //
+    boolean reserved = false;
+    if (options.notifyReviewers()) {
+      if (!reserveProposal(proposal, options.reviewersAutoSelected(), expiry)) {
+        throw new AccessDeniedException(
+          "You already have a pending approval request for this group. "
+            + "Wait for a reviewer to act on it, or for it to expire, "
+            + "before submitting another.");
+      }
+      reserved = true;
+    }
+
     //
     // Encode all inputs into a token and sign it.
     //
@@ -173,12 +195,51 @@ abstract void onProposalApproved(
 
       return proposalToken;
     }
-    catch (AccessException | IOException e) {
+    catch (AccessException | IOException | RuntimeException e) {
+      // SECOP-1101: minting/notification failed after we reserved —
+      // release the pending marker so a legitimate retry isn't blocked
+      // for the token lifetime.
+      if (reserved) {
+        releaseProposalReservation(proposal, options.reviewersAutoSelected());
+      }
+      if (e instanceof RuntimeException runtime) {
+        throw runtime;
+      }
       throw new AccessDeniedException(
         "Creating a proposal failed", e);
     }
   }
 
+  /**
+   * Wavemm fork (SECOP-1101): atomically reserve a pending-request
+   * marker before a token is minted, returning {@code false} if a live
+   * (non-expired) reservation already exists — i.e. this is a duplicate
+   * submit. Default no-op returning {@code true} (mail/debug don't
+   * dedupe). The key an implementation uses MUST be stable across the
+   * retries a requester makes: keyed on (beneficiary, group) when the
+   * reviewers were auto-selected (the picked set can shift between
+   * submits), and on (beneficiary, group, recipients) when the
+   * requester picked reviewers explicitly.
+   */
+  boolean reserveProposal(
+    @NotNull Proposal proposal,
+    boolean reviewersAutoSelected,
+    @NotNull Instant expiry
+  ) throws AccessException {
+    return true;
+  }
+
+  /**
+   * Wavemm fork (SECOP-1101): release a reservation taken by
+   * {@link #reserveProposal} when the propose failed after reserving.
+   * Best-effort; default no-op.
+   */
+  void releaseProposalReservation(
+    @NotNull Proposal proposal,
+    boolean reviewersAutoSelected
+  ) {
+  }
+
   @SuppressWarnings("unchecked")
   @Override
   public @NotNull Proposal accept(

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

@@ -107,14 +107,14 @@ public ProposeOptions(
   ) 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.
+   * 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
+   * render "this link has already been used" instead of an approval
+   * page whose submit would then fail. NOT the enforcement gate; that is
+   * {@link #claimForApproval} (a read here would be a TOCTOU).
    *
-   * <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.
+   * <p>Default no-op: handlers without a consumption store (mail, debug)
+   * keep upstream replayable semantics.
    *
    * @throws AccessException when the proposal was already consumed
    */
@@ -123,13 +123,34 @@ default void verifyNotConsumed(@NotNull Proposal proposal)
   }
 
   /**
-   * 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.
+   * Wavemm fork (SECOP-1100): atomically claim a proposal token for
+   * approval, so exactly one approval executes even under concurrent
+   * clicks on the same link. Called immediately BEFORE the approval
+   * executes; on {@code ALREADY_EXISTS} it throws, so the second
+   * concurrent (or any later) approver is rejected rather than
+   * re-granting. Replaces the previous post-execute {@code markConsumed}
+   * upsert, whose check-then-act window let concurrent approvers all
+   * pass.
+   *
+   * <p>Default no-op (mail/debug stay replayable). The Slack handler
+   * fails open on infrastructure errors — availability over
+   * replay-protection, matching SECOP-1093 — but treats an explicit
+   * already-claimed result as a hard reject.
+   *
+   * @throws AccessException when the token is already claimed/consumed
+   */
+  default void claimForApproval(@NotNull Proposal proposal)
+    throws AccessException {
+  }
+
+  /**
+   * Wavemm fork (SECOP-1100): release a claim taken by
+   * {@link #claimForApproval} when the approval did not complete (e.g.
+   * the execute failed before granting), so a legitimate retry can
+   * approve. Best-effort; a failed release leaves the token burned,
+   * which is the fail-safe direction.
    */
-  default void markConsumed(@NotNull Proposal proposal) {
+  default void releaseClaim(@NotNull Proposal proposal) {
   }
 
   /**