[i,l,-r] SECOP-952 follow-ups: requester visibility, double-submit guard, org-scoped-role retry (SECOP-1096/1097/1099)

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.10</version>
+  <version>2.3.0-wavemm.11</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

@@ -41,12 +41,59 @@
 public abstract class AbstractIamClient {
   private static final int MAX_SET_IAM_POLICY_ATTEMPTS = 4;
 
+  /**
+   * 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).
+   */
+  private static final int MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS = 3;
+
   private static boolean isRoleNotGrantableErrorMessage(@Nullable String message)
   {
     return message != null &&
       (message.contains("not supported") || message.contains("does not exist"));
   }
 
+  /**
+   * True for the transient 400 raised when a binding names a principal
+   * the {@code iam.allowedPolicyMemberDomains} org-policy checker can't
+   * yet resolve — the dominant cause being a Google group created
+   * moments earlier (JIT lazily provisions the group on first join,
+   * then immediately tries to bind it org-wide). Membership propagation
+   * clears within tens of seconds, so this is retryable; a genuinely
+   * disallowed member simply exhausts the bounded retries and surfaces
+   * as before. Deliberately narrow (both the constraint id AND the
+   * status) so we never silently swallow a real domain-restriction
+   * denial as "transient".
+   */
+  static boolean isMemberDomainPropagationError(@NotNull GoogleJsonResponseException e) {
+    if (e.getStatusCode() != 400) {
+      return false;
+    }
+    var sb = new StringBuilder();
+    if (e.getMessage() != null) {
+      sb.append(e.getMessage());
+    }
+    if (e.getDetails() != null) {
+      if (e.getDetails().getMessage() != null) {
+        sb.append('\n').append(e.getDetails().getMessage());
+      }
+      if (e.getDetails().getErrors() != null) {
+        e.getDetails().getErrors().forEach(err -> {
+          if (err.getMessage() != null) {
+            sb.append('\n').append(err.getMessage());
+          }
+        });
+      }
+    }
+    var haystack = sb.toString().toLowerCase();
+    return haystack.contains("allowedpolicymemberdomains")
+      || haystack.contains("not in permitted organization");
+  }
+
   /**
    * Create a new, API-specific client.
    */
@@ -99,6 +146,7 @@ public void modifyIamPolicy(
       // IAM policies use optimistic concurrency control, so we might need to perform
       // multiple attempts to update the policy.
       //
+      int memberDomainRetries = 0;
       for (int attempt = 0; attempt < MAX_SET_IAM_POLICY_ATTEMPTS; attempt++) {
         //
         // Read current version of policy.
@@ -148,6 +196,23 @@ public void modifyIamPolicy(
             catch (InterruptedException ignored) {
             }
           }
+          else if (isMemberDomainPropagationError(e)
+            && memberDomainRetries < MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS) {
+            //
+            // SECOP-1096: a just-provisioned group isn't resolvable by
+            // the org-policy member-domain checker yet. Back off longer
+            // than the 412 case (propagation is tens of seconds, not
+            // milliseconds) and retry. Don't spend a concurrency-control
+            // attempt on it — decrement so 412 retries stay available.
+            //
+            memberDomainRetries++;
+            try {
+              Thread.sleep(2000L << (memberDomainRetries - 1)); // 2s, 4s, 8s
+            }
+            catch (InterruptedException ignored) {
+            }
+            attempt--;
+          }
           else {
             throw (GoogleJsonResponseException) e.fillInStackTrace();
           }

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

@@ -68,6 +68,23 @@ abstract void onOperationProposed(
     @NotNull URI actionUri
     ) throws AccessException, IOException;
 
+  /**
+   * Options-aware variant (wavemm fork, SECOP-1099). The base
+   * implementation ignores the options so existing handlers (mail,
+   * debug) stay unchanged; handlers that care (Slack) override this
+   * one — e.g. to tell the requester who was auto-notified when the
+   * reviewer set was picked on their behalf.
+   */
+  void onOperationProposed(
+    @NotNull JitGroupContext.JoinOperation operation,
+    @NotNull Proposal proposal,
+    @NotNull ProposalHandler.ProposalToken token,
+    @NotNull URI actionUri,
+    @NotNull ProposalHandler.ProposeOptions options
+  ) throws AccessException, IOException {
+    onOperationProposed(operation, proposal, token, actionUri);
+  }
+
   /**
    * Notify relevant users about the completion of a proposal.
    */
@@ -150,7 +167,8 @@ abstract void onProposalApproved(
           joinOperation,
           proposal,
           proposalToken,
-          buildActionUri.apply(proposalToken.value()));
+          buildActionUri.apply(proposalToken.value()),
+          options);
       }
 
       return proposalToken;

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

@@ -76,13 +76,27 @@ public interface ProposalHandler {
    *                       individuals selected by the requester
    * @param notifyReviewers when false, the proposal token is generated
    *                        but no Slack/email notification is delivered
+   * @param reviewersAutoSelected wavemm fork (SECOP-1099): true when the
+   *                              filter was picked by the auto-narrow on
+   *                              the requester's behalf (empty picker
+   *                              selection) rather than by the requester.
+   *                              Handlers use this to tell the requester
+   *                              who was notified and to teach the picker.
    */
   record ProposeOptions(
     @Nullable Set<EndUserId> reviewerFilter,
-    boolean notifyReviewers
+    boolean notifyReviewers,
+    boolean reviewersAutoSelected
   ) {
+    public ProposeOptions(
+      @Nullable Set<EndUserId> reviewerFilter,
+      boolean notifyReviewers
+    ) {
+      this(reviewerFilter, notifyReviewers, false);
+    }
+
     public static final @NotNull ProposeOptions DEFAULT =
-      new ProposeOptions(null, true);
+      new ProposeOptions(null, true, false);
   }
 
   /**

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

@@ -194,6 +194,39 @@ static List<LayoutBlock> reviewerApprovedByYou(
         .build());
   }
 
+  /**
+   * DM to the requester right after an empty-selection propose
+   * (SECOP-1099): names the auto-selected reviewers and teaches the
+   * picker. Only sent when the reviewer set was picked on the
+   * requester's behalf — requesters who picked already know.
+   */
+  static List<LayoutBlock> beneficiaryProposed(
+    @NotNull String groupId,
+    @NotNull List<String> notifiedEmails
+  ) {
+    var names = String.join(", ", notifiedEmails);
+    return List.of(
+      HeaderBlock.builder()
+        .text(plain(":incoming_envelope: Request sent for approval"))
+        .build(),
+      SectionBlock.builder()
+        .fields(List.of(
+          markdownField("*Group*", "`" + groupId + "`")
+        ))
+        .build(),
+      SectionBlock.builder()
+        .text(markdown(
+          "Since you didn't pick anyone specifically, we sent it to "
+            + "*what we think are your closest teammates*:\n"
+            + truncateForBlock(names)))
+        .build(),
+      ContextBlock.builder()
+        .elements(List.of(markdown(
+          ":bulb: Next time you can choose who reviews it — use the "
+            + "\"Select reviewers\" picker on the request page.")))
+        .build());
+  }
+
   /**
    * DM to the beneficiary confirming the activation completed.
    */
@@ -234,6 +267,12 @@ static String reviewerApprovedByYouFallback(@NotNull String requesterEmail) {
     return "You approved " + requesterEmail + "'s request — nothing more to do.";
   }
 
+  static String beneficiaryProposedFallback(@NotNull String groupId) {
+    return String.format(
+      "Your request for %s was sent to your closest teammates for approval.",
+      groupId);
+  }
+
   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

@@ -162,6 +162,19 @@ void onOperationProposed(
     @NotNull Proposal proposal,
     @NotNull ProposalHandler.ProposalToken token,
     @NotNull URI actionUri
+  ) throws AccessException, IOException {
+    onOperationProposed(
+      operation, proposal, token, actionUri,
+      ProposalHandler.ProposeOptions.DEFAULT);
+  }
+
+  @Override
+  void onOperationProposed(
+    @NotNull JitGroupContext.JoinOperation operation,
+    @NotNull Proposal proposal,
+    @NotNull ProposalHandler.ProposalToken token,
+    @NotNull URI actionUri,
+    @NotNull ProposalHandler.ProposeOptions options
   ) throws AccessException, IOException {
     RegistryFingerprint fp;
     try {
@@ -195,6 +208,35 @@ void onOperationProposed(
         "No qualified reviewers resolved to individual users for " + fp.groupId());
     }
 
+    //
+    // SECOP-1097: skip the fan-out if an equivalent request is already
+    // in flight. The registry key is (beneficiary, group, recipients),
+    // so a live entry means the same person already has reviewer DMs
+    // out for the same group+reviewers — a duplicate submit (multi-tab,
+    // or a retry after a slow first POST). Re-sending would double every
+    // reviewer's DMs and leave a second approvable token dangling.
+    // Fail-open: a registry read error falls through to the normal
+    // fan-out (a duplicate DM batch beats dropping a real request).
+    //
+    try {
+      if (this.registry.lookup(fp.key()).join().isPresent()) {
+        this.logger.info(
+          "slack.onOperationProposed.duplicateSkipped",
+          "A live proposal already exists for %s requesting %s (key=%s); "
+            + "skipping duplicate reviewer notification.",
+          fp.beneficiary(), fp.groupId(), fp.key());
+        return;
+      }
+    }
+    catch (RuntimeException e) {
+      var cause = e.getCause() != null ? e.getCause() : e;
+      this.logger.warn(
+        "slack.duplicateCheck.failed",
+        "Duplicate-proposal check failed for %s on %s; proceeding with "
+          + "notification (fail-open). cause=%s",
+        fp.beneficiary(), fp.groupId(), cause.getMessage());
+    }
+
     var justification = proposal.input().getOrDefault("justification", "");
 
     var blocks = SlackMessages.reviewRequest(
@@ -353,11 +395,42 @@ void onOperationProposed(
         fp.key(), fp.beneficiary(), fp.groupId(), e);
     }
 
+    //
+    // SECOP-1099: when the reviewer set was picked on the requester's
+    // behalf (empty picker selection), tell them who we notified —
+    // people watch Slack, not the PAM UI, and the message doubles as
+    // the picker explainer. Best-effort: a failure here must not fail
+    // the propose that already landed reviewer DMs.
+    //
+    if (options.reviewersAutoSelected() && !posted.isEmpty()) {
+      var notifiedEmails = posted.stream()
+        .map(ReviewerMessage::email)
+        .sorted()
+        .toList();
+      try {
+        var beneficiaryUserId =
+          this.slackClient.lookupUserByEmail(fp.beneficiary()).join();
+        if (beneficiaryUserId != null) {
+          this.slackClient.postDirectMessage(
+            beneficiaryUserId,
+            SlackMessages.beneficiaryProposed(fp.groupId(), notifiedEmails),
+            SlackMessages.beneficiaryProposedFallback(fp.groupId())).join();
+        }
+      }
+      catch (RuntimeException e) {
+        var cause = e.getCause() != null ? e.getCause() : e;
+        this.logger.warn(
+          "slack.beneficiaryProposedDM.failed",
+          "Failed to DM requester %s the auto-selected reviewer list for %s: %s",
+          fp.beneficiary(), fp.groupId(), cause.getMessage());
+      }
+    }
+
     this.logger.info(
       "slack.onOperationProposed",
-      "Posted %d/%d Slack DMs for %s requesting %s (key=%s, failures=%s)",
+      "Posted %d/%d Slack DMs for %s requesting %s (key=%s, autoSelected=%s, failures=%s)",
       posted.size(), fp.reviewerEmails().size(), fp.beneficiary(), fp.groupId(),
-      fp.key(), failures);
+      fp.key(), options.reviewersAutoSelected(), failures);
   }
 
   @Override