Phase 2a: real Slack DMs + Firestore registry + cross-reviewer chat.update

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

@@ -270,6 +270,7 @@ public GoogleCredentials produceApplicationCredentials() {
   public @NotNull ProposalHandler produceProposalHandler(
     @NotNull TokenSigner tokenSigner,
     @NotNull SecretManagerClient secretManagerClient,
+    @NotNull CloudIdentityGroupsClient groupsClient,
     @NotNull Executor executor
   ) {
     //
@@ -279,7 +280,23 @@ public GoogleCredentials produceApplicationCredentials() {
     // upstream factory behaviour below — this is the documented rollback
     // path.
     //
-    if (configuration.isSlackConfigured()) {
+    if (configuration.slackNotificationsEnabled) {
+      //
+      // Operator's intent is clear ("turn Slack on"), so fail loudly if any
+      // companion variable is missing rather than silently falling through
+      // to the SMTP/501 branches. Silent fall-through hides a misconfig
+      // that the operator will only notice when the next MPA request lands
+      // and no DM goes out.
+      //
+      if (configuration.slackBotTokenSecret.isEmpty()
+        || configuration.slackFirestoreDatabase.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.");
+      }
+
       try {
         var botToken = secretManagerClient.accessSecret(
           configuration.slackBotTokenSecret.get());
@@ -288,16 +305,30 @@ public GoogleCredentials produceApplicationCredentials() {
             "SLACK_BOT_TOKEN_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
+        // than producing a project-wide @Singleton to keep its IAM blast
+        // radius scoped to the Slack code path: only this factory branch,
+        // executed only when the flag is on, ever instantiates it.
+        //
+        var firestore = com.google.cloud.firestore.FirestoreOptions
+          .getDefaultInstance().toBuilder()
+          .setProjectId(runtime.projectId())
+          .setDatabaseId(configuration.slackFirestoreDatabase.get())
+          .setCredentials(runtime.applicationCredentials())
+          .build()
+          .getService();
+
         var slackClient = new SlackClient(botToken, executor, logger);
-        var registry = new SlackMessageRegistry(
-          configuration.slackFirestoreDatabase.get(),
-          executor,
-          logger);
+        var registry = new SlackMessageRegistry(firestore, executor, logger);
+        var groupResolver = new GroupResolver(groupsClient, executor);
 
         return new SlackProposalHandler(
           tokenSigner,
           slackClient,
           registry,
+          groupResolver,
           logger,
           new AbstractProposalHandler.Options(configuration.proposalTimeout),
           new SlackProposalHandler.Options(configuration.notificationTimeZone));

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

@@ -326,17 +326,6 @@ public ApplicationConfiguration(@NotNull Map<String, String> settingsData) {
     this.slackFirestoreDatabase = readStringSetting("SLACK_FIRESTORE_DATABASE");
   }
 
-  /**
-   * @return true iff the Slack code path is enabled and has the minimum
-   *         configuration needed to operate (bot token secret + Firestore
-   *         database id).
-   */
-  public boolean isSlackConfigured() {
-    return this.slackNotificationsEnabled
-      && this.slackBotTokenSecret.isPresent()
-      && this.slackFirestoreDatabase.isPresent();
-  }
-
   public boolean isSmtpConfigured() {
     return this.smtpSenderAddress.isPresent();
   }

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

@@ -12,34 +12,32 @@
 
 import com.google.common.base.Preconditions;
 import com.google.solutions.jitaccess.apis.Logger;
+import com.google.solutions.jitaccess.common.CompletableFutures;
+import com.slack.api.Slack;
+import com.slack.api.methods.MethodsClient;
+import com.slack.api.methods.SlackApiException;
+import com.slack.api.methods.response.chat.ChatPostMessageResponse;
+import com.slack.api.methods.response.chat.ChatUpdateResponse;
+import com.slack.api.methods.response.conversations.ConversationsOpenResponse;
+import com.slack.api.methods.response.users.UsersLookupByEmailResponse;
+import com.slack.api.model.block.LayoutBlock;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
+import java.io.IOException;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Executor;
 
 /**
  * Thin wrapper around the Slack Web API.
  *
- * <p>Responsibilities:
- * <ul>
- *   <li>Hold the cached bot token (resolved once at startup from Secret Manager).
- *   <li>Resolve email → Slack user id via {@code users.lookupByEmail}.
- *   <li>Post Block-Kit DMs and update them in place.
- * </ul>
- *
- * <p>All public methods are async — Slack outages must not block JIT request
- * threads. Failures are returned as failed futures and logged at
- * {@code WARNING}; the caller (SlackProposalHandler) decides whether to
- * surface or swallow them.
- *
- * <p>Phase 1: skeleton. Methods log "would call Slack" and return successful
- * stub futures so the DI graph wires up and integration tests can compile.
- * Phase 2 replaces the bodies with real {@code com.slack.api} calls.
+ * <p>All public methods are async. Failures bubble out as failed futures —
+ * the caller (SlackProposalHandler) decides whether to log+swallow or
+ * propagate. JIT request threads must never block on Slack.
  */
 public class SlackClient {
-  private final @NotNull String botToken;
+  private final @NotNull MethodsClient methods;
   private final @NotNull Executor executor;
   private final @NotNull Logger logger;
 
@@ -49,107 +47,110 @@ public SlackClient(
     @NotNull Logger logger
   ) {
     Preconditions.checkArgument(!botToken.isBlank(), "botToken must not be blank");
-    this.botToken = botToken;
+    this.methods = Slack.getInstance().methods(botToken);
     this.executor = executor;
     this.logger = logger;
   }
 
   /**
-   * Look up a Slack user id by email. Returns null if the user is not in the
-   * workspace (e.g. external collaborators, or the email is not registered).
-   *
-   * @param email user's primary email, case-insensitive on Slack's side.
+   * Look up a Slack user by email. Returns null when the email is not
+   * registered in the workspace (e.g. external collaborators) — that's
+   * a normal outcome, not an error.
    */
   public @NotNull CompletableFuture<@Nullable String> lookupUserByEmail(
     @NotNull String email
   ) {
-    return CompletableFuture.supplyAsync(() -> {
-      // TODO(phase-2): MethodsClient.usersLookupByEmail(...).getUser().getId()
-      this.logger.info(
-        "slack.lookupByEmail.stub",
-        "Phase 1 stub: would resolve %s",
-        email);
-      return null;
+    return CompletableFutures.supplyAsync(() -> {
+      try {
+        UsersLookupByEmailResponse response = this.methods.usersLookupByEmail(req -> req.email(email));
+        if (!response.isOk()) {
+          if ("users_not_found".equals(response.getError())) {
+            return null;
+          }
+          throw new IOException("users.lookupByEmail failed: " + response.getError());
+        }
+        return response.getUser() != null ? response.getUser().getId() : null;
+      }
+      catch (SlackApiException e) {
+        throw new IOException("Slack API error in users.lookupByEmail for " + email, e);
+      }
     }, this.executor);
   }
 
   /**
-   * Open a DM channel with the user and post a message. Returns the
-   * channel id and message timestamp so the caller can later
-   * {@link #updateMessage update} it in place.
-   *
-   * @param slackUserId resolved via {@link #lookupUserByEmail}
-   * @param blocksJson  Block Kit blocks array, serialised to JSON
-   * @param fallbackText plain-text fallback for notifications and a11y
+   * Open a DM channel with the user and post a Block Kit message. Returns
+   * (channelId, ts) for later {@link #updateMessage} calls.
    */
   public @NotNull CompletableFuture<PostedMessage> postDirectMessage(
     @NotNull String slackUserId,
-    @NotNull String blocksJson,
+    @NotNull List<LayoutBlock> blocks,
     @NotNull String fallbackText
   ) {
-    return CompletableFuture.supplyAsync(() -> {
-      // TODO(phase-2):
-      //   1. conversations.open(users=[slackUserId]) → channel.id
-      //   2. chat.postMessage(channel, blocks, text=fallbackText) → ts
-      this.logger.info(
-        "slack.postDirectMessage.stub",
-        "Phase 1 stub: would DM %s with %d blocks",
-        slackUserId,
-        blocksJson.length());
-      return new PostedMessage("STUB_CHANNEL", "0000000000.000000");
+    return CompletableFutures.supplyAsync(() -> {
+      try {
+        ConversationsOpenResponse open = this.methods.conversationsOpen(
+          req -> req.users(List.of(slackUserId)));
+        if (!open.isOk() || open.getChannel() == null) {
+          throw new IOException(
+            "conversations.open failed for user " + slackUserId + ": " + open.getError());
+        }
+        var channelId = open.getChannel().getId();
+
+        ChatPostMessageResponse post = this.methods.chatPostMessage(req -> req
+          .channel(channelId)
+          .blocks(blocks)
+          .text(fallbackText));
+        if (!post.isOk()) {
+          throw new IOException("chat.postMessage failed: " + post.getError());
+        }
+        return new PostedMessage(channelId, post.getTs());
+      }
+      catch (SlackApiException e) {
+        throw new IOException("Slack API error posting DM to " + slackUserId, e);
+      }
     }, this.executor);
   }
 
   /**
    * Replace the blocks of an already-posted message. Used to mark sibling
-   * reviewer DMs as "approved by X" without removing the original thread.
+   * reviewer DMs as "approved by X" without removing the original message.
+   * <p>
+   * Errors are logged at WARN and absorbed — losing a sibling update is not
+   * worth failing the approval flow over.
    */
   public @NotNull CompletableFuture<Void> updateMessage(
     @NotNull String channelId,
     @NotNull String messageTs,
-    @NotNull String blocksJson,
+    @NotNull List<LayoutBlock> blocks,
     @NotNull String fallbackText
   ) {
-    return CompletableFuture.runAsync(() -> {
-      // TODO(phase-2): chat.update(channel, ts, blocks, text=fallbackText)
-      this.logger.info(
-        "slack.updateMessage.stub",
-        "Phase 1 stub: would update %s/%s",
-        channelId,
-        messageTs);
+    return CompletableFutures.supplyAsync(() -> {
+      try {
+        ChatUpdateResponse response = this.methods.chatUpdate(req -> req
+          .channel(channelId)
+          .ts(messageTs)
+          .blocks(blocks)
+          .text(fallbackText));
+        if (!response.isOk()) {
+          this.logger.warn(
+            "slack.updateMessage.failed",
+            "chat.update failed for %s/%s: %s",
+            channelId, messageTs, response.getError());
+        }
+        return null;
+      }
+      catch (SlackApiException e) {
+        throw new IOException(
+          "Slack API error updating message " + channelId + "/" + messageTs, e);
+      }
     }, this.executor);
   }
 
   /**
    * A successfully posted Slack message, identified by (channel, timestamp).
-   * Slack's chat.update requires both fields.
    */
   public record PostedMessage(
     @NotNull String channelId,
     @NotNull String messageTs
   ) {}
-
-  /**
-   * Construction-time options for the Slack client.
-   *
-   * <p>Phase 1 only carries the bot token; Phase 2 will likely add timeouts,
-   * retry counts, and a deduplication window for chat.postMessage (Slack
-   * can deliver the same Block Kit interaction more than once).
-   */
-  public record Options(
-    @NotNull String botToken
-  ) {
-    public Options {
-      Preconditions.checkArgument(!botToken.isBlank(), "botToken must not be blank");
-    }
-
-    /**
-     * Helper to bundle the construction parameters that callers other than
-     * Application.java may want.
-     */
-    public List<String> debugFields() {
-      // Never include the token itself.
-      return List.of("botToken=<redacted>");
-    }
-  }
 }