[i,l,-r] Phase 2b/3 + security hardening: reviewer picker UX + opt-out copy-link

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.2</version>
+  <version>2.3.0-wavemm.6</version>
   <properties>
     <surefire-plugin.version>3.5.3</surefire-plugin.version>
     <surefire-plugin.version>3.5.3</surefire-plugin.version>
@@ -207,6 +207,41 @@
           </execution>
         </executions>
       </plugin>
+      <plugin>
+        <!--
+          Wavemm fork P2-13: OWASP dependency-check. Surfaces known CVEs
+          in our (mostly Google-owned) transitive dep tree as part of
+          the Maven build so a CVE that lands in slack-api-client,
+          google-cloud-firestore, or any other dependency triggers a
+          build failure once the CVSS exceeds the configured threshold.
+
+          Run via `mvn org.owasp:dependency-check-maven:check` in CI or
+          locally; not bound to a default phase to keep `mvn test` fast.
+          A reviewer who runs the standard test target (`mvn -DskipITs
+          test`) won't pay the NVD-feed download cost.
+        -->
+        <groupId>org.owasp</groupId>
+        <artifactId>dependency-check-maven</artifactId>
+        <version>11.1.1</version>
+        <configuration>
+          <!-- Fail the build on CVSS >= 8 (high+). Lower-severity CVEs
+               surface in the report but don't break CI; we triage them
+               through the regular dependency-bump cadence. -->
+          <failBuildOnCVSS>8</failBuildOnCVSS>
+          <suppressionFiles>
+            <!-- Optional suppression file — created if/when we need to
+                 silence a reviewed-and-acknowledged finding. Absent by
+                 default; the plugin tolerates a missing file. -->
+            <suppressionFile>dependency-check-suppressions.xml</suppressionFile>
+          </suppressionFiles>
+          <!-- Skip unused analyzers to shorten a CI run from ~5 min
+               to ~2 min on a warm cache. -->
+          <nodeAnalyzerEnabled>false</nodeAnalyzerEnabled>
+          <nodeAuditAnalyzerEnabled>false</nodeAuditAnalyzerEnabled>
+          <retireJsAnalyzerEnabled>false</retireJsAnalyzerEnabled>
+          <assemblyAnalyzerEnabled>false</assemblyAnalyzerEnabled>
+        </configuration>
+      </plugin>
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-javadoc-plugin</artifactId>

sources/src/main/java/com/google/solutions/jitaccess/auth/GroupResolver.java

@@ -36,8 +36,57 @@
 import java.util.concurrent.Executor;
 
 /**
- * Expands group memberships using the Cloud Identity
- * Groups API.
+ * Expands group memberships using the Cloud Identity Groups API.
+ *
+ * <p><b>Visibility &amp; trust boundary (wavemm fork P1-7).</b> The
+ * output of {@link #expand} is a flat set of {@link EndUserId}/{@link
+ * GroupId} principals derived from policy-ACL-supplied group ids. It
+ * is NOT itself an authorisation decision — it answers the question
+ * "who are the (one-level-resolved) members of these groups?", nothing
+ * more. Callers that use it to populate audit fields, message
+ * recipients, or picker candidate lists are fine; callers that use it
+ * to decide whether a specific principal can perform a privileged
+ * action MUST cross-check against the policy ACL, because:
+ *
+ * <ul>
+ *   <li>resolution is one level deep (nested groups are not flattened
+ *       — see method comment), so the absence of a user from
+ *       {@code expand(...)} does NOT prove they aren't an effective
+ *       member;</li>
+ *   <li>an empty/failed Cloud Identity response is silently equivalent
+ *       to "no members" inside this method (the per-group lambda in
+ *       {@link com.google.solutions.jitaccess.web.proposal.ReviewerCandidates}
+ *       turns errors into empty lists for picker robustness; do not
+ *       copy that pattern when correctness matters);</li>
+ *   <li>service-account members of approver groups are silently
+ *       dropped (see {@link #principalFromMembership}), so any
+ *       authorisation decision derived from {@code expand} must also
+ *       account for non-EndUser principals via the policy ACL
+ *       directly.</li>
+ * </ul>
+ *
+ * <p><b>Audit of in-tree callers</b> (re-verify when adding a new
+ * one):
+ *
+ * <ul>
+ *   <li>{@link com.google.solutions.jitaccess.web.rest.GroupsResource#post}
+ *       — selectedReviewers subset check: combines {@code expand} with
+ *       a separate ACL lookup, NOT a sole authorisation source. ✓
+ *   <li>{@link com.google.solutions.jitaccess.web.rest.GroupsResource#getReviewers}
+ *       — picker candidate listing: best-effort, output is for UX
+ *       only. ✓
+ *   <li>{@link com.google.solutions.jitaccess.web.proposal.ReviewerCandidates#compute}
+ *       — picker UI: same as above. ✓
+ *   <li>{@link com.google.solutions.jitaccess.web.proposal.SlackProposalHandler#fingerprint}
+ *       — registry key + DM dispatch: NOT an authorisation source;
+ *       the JWT verified by {@code JitGroupContext.approve} is. ✓
+ * </ul>
+ *
+ * <p>Any new caller that uses {@code expand} for an authorisation
+ * decision must add itself here AND ensure a separate ACL check is
+ * still performed. The catalog-side defense in {@link
+ * com.google.solutions.jitaccess.catalog.JitGroupContext.JoinOperation#propose(java.time.Instant,
+ * java.util.Set)} is the trust-boundary backstop that catches a forgotten check.
  */
 public class GroupResolver {
   private final @NotNull CloudIdentityGroupsClient groupsClient;

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

@@ -29,6 +29,7 @@
 import com.google.solutions.jitaccess.catalog.policy.*;
 import com.google.solutions.jitaccess.catalog.provisioning.Provisioner;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
 import java.io.IOException;
 import java.time.Instant;
@@ -326,6 +327,49 @@ public boolean requiresApproval() {
      */
     public @NotNull Proposal propose(
       @NotNull Instant expiry
+    ) throws AccessException {
+      return propose(expiry, null);
+    }
+
+    /**
+     * Propose this operation for someone else to approve, optionally
+     * narrowing the set of recipients.
+     *
+     * <p>The picker UX (added in the wavemm fork) lets the requester
+     * choose specific reviewers rather than blasting every qualified
+     * peer. When {@code reviewerFilter} is non-null, the recipient set
+     * REPLACES the policy-derived qualified set with the filter — group
+     * principals from the ACL are dropped so the picker doesn't get
+     * defeated by post-handler group expansion.
+     *
+     * <p>When {@code reviewerFilter} is null the behaviour is identical
+     * to the upstream single-arg {@link #propose(Instant)} method.
+     *
+     * <p><b>SECURITY (defense-in-depth, wavemm fork P1-6):</b> the catalog
+     * enforces what it can locally — every {@code EndUserId} in the
+     * filter must either appear directly in the policy ACL, OR the ACL
+     * must contain at least one {@link GroupId} approver into which the
+     * caller has already verified the filter expands. The full subset-
+     * after-group-expansion check still lives at the REST layer
+     * ({@code GroupsResource.post}) where {@link
+     * com.google.solutions.jitaccess.auth.GroupResolver} is available;
+     * forcing the catalog to depend on Cloud Identity would be the
+     * wrong layering. But a future caller that forgets the REST-layer
+     * check still cannot smuggle in a principal who has no chance of
+     * being a real approver — the catalog rejects filter entries that
+     * are not in the direct ACL and have no group approver to
+     * conceivably belong to.
+     *
+     * <p>This is a partial defense: an ACL of
+     * {@code group:engineering@} still allows the catalog to honour a
+     * filter naming any email (because the email <i>could</i> be in
+     * the group, and the catalog can't tell from here). The REST
+     * layer's expansion check is what closes that gap — see {@link
+     * com.google.solutions.jitaccess.web.proposal.ReviewerCandidates}.
+     */
+    public @NotNull Proposal propose(
+      @NotNull Instant expiry,
+      @Nullable Set<EndUserId> reviewerFilter
     ) throws AccessException {
       if (!this.requiresApproval) {
         throw new IllegalStateException(
@@ -362,6 +406,65 @@ public boolean requiresApproval() {
           "There are no principals that could approve the request to join this group");
       }
 
+      //
+      // Apply the requester-supplied filter, if any.
+      //
+      // When a filter is set, it REPLACES the policy-derived approvers
+      // set with the requester's selection. We deliberately don't keep
+      // GroupId entries from the original ACL alongside the filtered
+      // EndUserIds — otherwise a typical Wave ACL of
+      // {group:engineering_platform_security@} would let the group
+      // pass through unchanged, the SlackProposalHandler would expand
+      // it back to every member, and the picker would be a UX lie
+      // (user selects "only Adam", every team-mate gets DM'd anyway).
+      //
+      // The REST layer (GroupsResource) is responsible for validating
+      // that every email in the filter is in the expanded qualified-
+      // peer set BEFORE calling propose. The defense-in-depth check
+      // below catches the case where a future caller forgets that
+      // step — we reject filter entries that are not in the direct
+      // ACL when the ACL has no group approvers, since in that case
+      // the catalog has full visibility into who's authorised.
+      //
+      Set<IamPrincipalId> filteredApprovers;
+      if (reviewerFilter == null || reviewerFilter.isEmpty()) {
+        filteredApprovers = approvers;
+      } else {
+        var directAclEndUsers = approvers.stream()
+          .filter(EndUserId.class::isInstance)
+          .map(EndUserId.class::cast)
+          .collect(Collectors.toSet());
+        var aclHasGroupApprover = approvers.stream()
+          .anyMatch(GroupId.class::isInstance);
+
+        // Reject filter entries the catalog can prove are not
+        // authorised. If the ACL has no group approver every entry
+        // must be in directAclEndUsers; if there's a group approver
+        // we can't prove it without expansion, so we trust the REST
+        // layer's check (which has run, modulo a calling-code bug).
+        if (!aclHasGroupApprover) {
+          var bogus = reviewerFilter.stream()
+            .filter(u -> !directAclEndUsers.contains(u))
+            .map(u -> u.email)
+            .toList();
+          if (!bogus.isEmpty()) {
+            throw new AccessDeniedException(
+              "Selected reviewers are not authorised to approve this "
+                + "request: " + String.join(", ", bogus));
+          }
+        }
+
+        filteredApprovers = reviewerFilter.stream()
+          .map(u -> (IamPrincipalId) u)
+          .collect(Collectors.toSet());
+      }
+
+      if (filteredApprovers.isEmpty()) {
+        throw new AccessDeniedException(
+          "None of the selected reviewers are authorised to approve this request");
+      }
+
+      var finalApprovers = filteredApprovers;
       return new Proposal() {
         @Override
         public @NotNull EndUserId user() {
@@ -375,7 +478,7 @@ public boolean requiresApproval() {
 
         @Override
         public @NotNull Set<IamPrincipalId> recipients() {
-          return approvers;
+          return finalApprovers;
         }
 
         @Override

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

@@ -58,6 +58,35 @@ public interface Proposal {
    */
   @NotNull Map<String, String> input();
 
+  /**
+   * Wavemm fork: whether the requester opted in to having the proposal
+   * handler deliver out-of-band notification (Slack DMs / email) to
+   * reviewers, vs. opting out to copy and share the approval URL
+   * manually.
+   *
+   * <p>When false:
+   * <ul>
+   *   <li>{@code AbstractProposalHandler.propose} skips the
+   *       {@code onOperationProposed} hook — no DMs, no Firestore
+   *       registry entry.
+   *   <li>{@code SlackProposalHandler.onProposalApproved} short-
+   *       circuits the sibling-update path (there are no siblings to
+   *       update because no DMs were sent), keeping only the
+   *       beneficiary-confirmation DM. The previous "no Slack
+   *       registry entry" warning becomes a routine INFO log.
+   * </ul>
+   *
+   * <p>The flag is encoded as a JWT claim so it survives the
+   * propose → accept round-trip and the approver doesn't need to
+   * re-derive the requester's intent from the request side.
+   *
+   * <p>Default: {@code true}, matching upstream behaviour where
+   * notification is always attempted.
+   */
+  default boolean notifyReviewers() {
+    return true;
+  }
+
   /**
    * Invoked when the proposal was completed successfully.
    */

sources/src/main/java/com/google/solutions/jitaccess/web/Application.java

@@ -219,6 +219,13 @@ public UserResource.Options produceUserResourceOptions() {
       runtime.type() == ApplicationRuntime.Type.DEVELOPMENT);
   }
 
+  @Produces
+  @Singleton
+  public com.google.solutions.jitaccess.web.rest.GroupsResource.Options produceGroupsResourceOptions() {
+    return new com.google.solutions.jitaccess.web.rest.GroupsResource.Options(
+      configuration.slackCopyLinkEnabled);
+  }
+
   @Produces
   public GoogleCredentials produceApplicationCredentials() {
     return runtime.applicationCredentials();
@@ -290,12 +297,13 @@ public GoogleCredentials produceApplicationCredentials() {
       // and no DM goes out.
       //
       if (configuration.slackBotTokenSecret.isEmpty()
-        || configuration.slackFirestoreDatabase.isEmpty()) {
+        || configuration.slackFirestoreDatabase.isEmpty()
+        || configuration.slackRegistryKeySaltSecret.isEmpty()) {
         throw new IllegalStateException(
-          "SLACK_NOTIFICATIONS_ENABLED=true requires both SLACK_BOT_TOKEN_SECRET "
-            + "and SLACK_FIRESTORE_DATABASE. Either provide them or set "
-            + "SLACK_NOTIFICATIONS_ENABLED=false to restore the upstream "
-            + "notification path.");
+          "SLACK_NOTIFICATIONS_ENABLED=true requires SLACK_BOT_TOKEN_SECRET, "
+            + "SLACK_FIRESTORE_DATABASE, and SLACK_REGISTRY_KEY_SALT_SECRET. "
+            + "Either provide them or set SLACK_NOTIFICATIONS_ENABLED=false "
+            + "to restore the upstream notification path.");
       }
 
       try {
@@ -306,6 +314,19 @@ public GoogleCredentials produceApplicationCredentials() {
             "SLACK_BOT_TOKEN_SECRET points to an empty secret value");
         }
 
+        //
+        // Wavemm fork P2-11: HMAC salt for SlackMessageRegistry document
+        // keys. Read at the same lifecycle point as the bot token so a
+        // misconfigured (missing/empty) salt fails app startup loudly,
+        // not silently at the first elevation request.
+        //
+        var registryKeySalt = secretManagerClient.accessSecret(
+          configuration.slackRegistryKeySaltSecret.get());
+        if (registryKeySalt == null || registryKeySalt.isBlank()) {
+          throw new IllegalStateException(
+            "SLACK_REGISTRY_KEY_SALT_SECRET points to an empty secret value");
+        }
+
         //
         // Build a Firestore client targeting the named database that
         // wavemm-iam Terraform provisions. We construct it locally rather
@@ -322,7 +343,8 @@ public GoogleCredentials produceApplicationCredentials() {
           .getService();
 
         var slackClient = new SlackClient(botToken, executor, logger);
-        var registry = new SlackMessageRegistry(firestore, executor, logger);
+        var registry = new SlackMessageRegistry(
+          firestore, executor, logger, registryKeySalt);
         var groupResolver = new GroupResolver(groupsClient, executor);
 
         return new SlackProposalHandler(
@@ -391,7 +413,8 @@ else if (runtime.type() == ApplicationRuntime.Type.DEVELOPMENT) {
         @Override
         public @NotNull ProposalHandler.ProposalToken propose(
           @NotNull JitGroupContext.JoinOperation joinOperation,
-          @NotNull Function<String, URI> buildActionUri
+          @NotNull Function<String, URI> buildActionUri,
+          @NotNull ProposalHandler.ProposeOptions options
           ) {
           throw new UnsupportedOperationException(
             "Approvals are not supported because the SMTP configuration is incomplete");

sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java

@@ -197,6 +197,30 @@ public class ApplicationConfiguration extends AbstractConfiguration {
    */
   final @NotNull Optional<String> slackFirestoreDatabase;
 
+  /**
+   * Secret Manager resource path of the HMAC salt used to compute the
+   * registry document key (wavemm fork P2-11). Required when
+   * {@link #slackNotificationsEnabled} is true. Format:
+   * projects/X/secrets/Y/versions/Z. The salt MUST stay constant for
+   * the lifetime of any in-flight JWT — rotating it strands open
+   * approval requests because the propose-time and accept-time keys
+   * stop matching. Rotate during a quiet period (e.g. between
+   * deploys) and follow the secret-rotation runbook in
+   * SLACK_INTEGRATION.md.
+   */
+  final @NotNull Optional<String> slackRegistryKeySaltSecret;
+
+  /**
+   * When true, the {@code POST /api/.../groups/{name}} response includes
+   * the JWT-bearing approval URL so the requester can copy it manually,
+   * and the form accepts {@code notifyReviewers=false} to skip the
+   * automated Slack DM delivery entirely (the JWT is still generated and
+   * signed). Independent of {@link #slackNotificationsEnabled} —
+   * disabling this flag just hides the affordance; Slack DMs continue to
+   * fire as long as Slack is configured.
+   */
+  final boolean slackCopyLinkEnabled;
+
   public ApplicationConfiguration(@NotNull Map<String, String> settingsData) {
     super(settingsData);
 
@@ -324,6 +348,10 @@ public ApplicationConfiguration(@NotNull Map<String, String> settingsData) {
       .orElse(false);
     this.slackBotTokenSecret = readStringSetting("SLACK_BOT_TOKEN_SECRET");
     this.slackFirestoreDatabase = readStringSetting("SLACK_FIRESTORE_DATABASE");
+    this.slackRegistryKeySaltSecret = readStringSetting("SLACK_REGISTRY_KEY_SALT_SECRET");
+    this.slackCopyLinkEnabled = readSetting(
+      Boolean::parseBoolean, "SLACK_COPY_LINK_ENABLED")
+      .orElse(false);
   }
 
   public boolean isSmtpConfigured() {