From 8bd0c6cfef41a1eb2006f70ac5e36a22e19c0679 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Wed, 29 Apr 2026 21:54:50 +0200 Subject: [PATCH 1/2] Phase 2a: real Slack + Firestore implementations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the Phase 1 stubs with working code so SlackProposalHandler actually delivers DMs and tracks them across the propose/approve flow. Still gated by SLACK_NOTIFICATIONS_ENABLED (env var, default false in the wavemm-iam Terraform), so this commit alone changes no production behaviour — it just makes the flag's "on" path do real work. What this enables (with flag on): Alice submits MPA join → all qualified reviewers (after group expansion) receive a Slack DM with a Block Kit "Approve in JIT" card. Bob approves in JIT → sibling DMs update in place to "✅ Already approved by Bob — no action needed", and Alice receives a confirmation DM. ActivationSelfApproved still doesn't notify on Slack (audit sink covers it). SlackClient (proposal/SlackClient.java) - Slack.getInstance().methods(token) wraps the bot token. - lookupUserByEmail / postDirectMessage / updateMessage are async via the project's CompletableFutures.supplyAsync. - users_not_found returns null (normal outcome, not an error). - chat.update soft-fails (logs + swallows) — losing a sibling cosmetic update isn't worth failing the approval. SlackMessages (new) - Block Kit DSL templates for the three message shapes: review request, sibling-update, beneficiary confirmation. - escapeBlockquote() neutralises crafted justifications that try to break out of the >justification block. - reviewerSiblingUpdate intentionally omits the action button so a second reviewer can't double-approve via stale Slack message. SlackMessageRegistry (proposal/SlackMessageRegistry.java) - Real Firestore client (constructed in Application.java factory) over the named "pam-slack-bridge" database. - Document key is sha256(beneficiary | groupId | sorted recipients) — stable across propose / accept, order-independent on recipients (covered by tests). - TTL field aligns with JWT expiry; field is created by wavemm-iam TF. SlackProposalHandler (proposal/SlackProposalHandler.java) - onOperationProposed: GroupResolver expands group principals, looks up each user, posts DMs, records the registry. Aggregates per- reviewer failures: at least one DM landing keeps the request viable; zero landed throws so the requester knows. - onProposalApproved: looks up registry by request key, chat.updates siblings (skipping the approver themselves), DMs the beneficiary, deletes the registry entry. Beneficiary DM still fires on registry miss. - All Slack/Firestore failures logged at WARN, never propagated to JIT request thread except the "zero DMs landed" catastrophic case. Application.java (factory wiring) - The Slack branch in produceProposalHandler now also injects CloudIdentityGroupsClient (for group expansion) and constructs a Firestore client locally with the named DB id. Local construction keeps the IAM blast radius scoped to this factory branch — the Firestore client is never produced for project-wide use. Tests - TestSlackMessageRegistry: requestKey is deterministic + invariant to recipient ordering + distinguishes beneficiary/group/recipients. - TestSlackMessages: the JWT URL is on the primary action button (the only way the reviewer can act); sibling-update has no action button; blockquote injection is escaped. - TestSlackProposalHandler: orchestration with mocked deps — DMs all reviewers, skips on lookupByEmail miss, throws when zero land, on approval updates only siblings (not the approver), notifies beneficiary even on registry miss. Phase 2b (next, separate PR): - GroupsResource: GET /reviewers (returns qualified peers + suggested- from-team flag) + POST accepts selectedReviewers[] filter. - JoinOperation.propose overload accepting a reviewerFilter. - SLACK_COPY_LINK_ENABLED env-flag for the "show approval URL in response + opt-out of Slack DMs" UX, defaulting to off. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../solutions/jitaccess/web/Application.java | 23 +- .../jitaccess/web/proposal/SlackClient.java | 161 +++++----- .../web/proposal/SlackMessageRegistry.java | 161 ++++++---- .../jitaccess/web/proposal/SlackMessages.java | 212 +++++++++++++ .../web/proposal/SlackProposalHandler.java | 288 ++++++++++++++---- .../proposal/TestSlackMessageRegistry.java | 90 ++++++ .../web/proposal/TestSlackMessages.java | 151 +++++++++ .../proposal/TestSlackProposalHandler.java | 275 +++++++++++++++++ 8 files changed, 1153 insertions(+), 208 deletions(-) create mode 100644 sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessages.java create mode 100644 sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessageRegistry.java create mode 100644 sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java create mode 100644 sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java b/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java index 2dd12853..33539fdc 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java +++ b/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 ) { // @@ -288,16 +289,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)); diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java index fb826eeb..d72d1319 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackClient.java @@ -12,9 +12,19 @@ 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; @@ -22,24 +32,12 @@ /** * Thin wrapper around the Slack Web API. * - *

Responsibilities: - *

- * - *

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. - * - *

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. + *

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 postDirectMessage( @NotNull String slackUserId, - @NotNull String blocksJson, + @NotNull List 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. + *

+ * Errors are logged at WARN and absorbed — losing a sibling update is not + * worth failing the approval flow over. */ public @NotNull CompletableFuture updateMessage( @NotNull String channelId, @NotNull String messageTs, - @NotNull String blocksJson, + @NotNull List 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. - * - *

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 debugFields() { - // Never include the token itself. - return List.of("botToken="); - } - } } diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java index a7a9ee58..ab4148aa 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessageRegistry.java @@ -10,18 +10,26 @@ package com.google.solutions.jitaccess.web.proposal; +import com.google.cloud.Timestamp; +import com.google.cloud.firestore.CollectionReference; +import com.google.cloud.firestore.Firestore; import com.google.common.base.Preconditions; import com.google.solutions.jitaccess.apis.Logger; +import com.google.solutions.jitaccess.common.CompletableFutures; import org.jetbrains.annotations.NotNull; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.time.Instant; +import java.util.ArrayList; +import java.util.HashMap; import java.util.HexFormat; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; /** @@ -29,54 +37,56 @@ * fingerprint of the join request. * *

Why this exists: when one reviewer approves in JIT, we need to update - * the sibling reviewers' DMs in place (chat.update) to "✅ Approved by X". - * The JIT JWT contains the recipients but not their Slack message - * timestamps, so we record (channel, ts) at notification time and look it up - * at approval time. + * the sibling reviewers' DMs in place ({@code chat.update}) to "Approved + * by X". The JIT JWT contains the recipients but not their Slack message + * timestamps, so we record (channel, ts) at notification time and look it + * up at approval time. * - *

Storage: a named Firestore database in the same GCP project as JIT. - * The collection is {@value #COLLECTION}; each document key is a SHA-256 of - * (beneficiary, group, sorted recipients). Documents carry a TTL field - * {@code expiresAt} aligned with the JWT expiry; Firestore TTL deletes - * stale entries best-effort within ~24 h. - * - *

Phase 1: all methods are stubs that log + return empty. Phase 2 wires - * in the real Firestore client. + *

Storage: a named Firestore Native database in the same GCP project as + * JIT. The collection is {@value #COLLECTION}; each document key is the + * SHA-256 of (beneficiary, group, sorted recipient emails). Each document + * carries a TTL field {@code expires_at} aligned with the JWT expiry; the + * Firestore TTL policy (managed in wavemm-iam Terraform) auto-deletes + * stale entries — best-effort, can lag up to 24h. */ public class SlackMessageRegistry { static final String COLLECTION = "requests"; + static final String FIELD_REVIEWERS = "reviewers"; + static final String FIELD_EXPIRES_AT = "expires_at"; - private final @NotNull String databaseId; + private final @NotNull Firestore firestore; private final @NotNull Executor executor; private final @NotNull Logger logger; public SlackMessageRegistry( - @NotNull String databaseId, + @NotNull Firestore firestore, @NotNull Executor executor, @NotNull Logger logger ) { - Preconditions.checkArgument(!databaseId.isBlank(), "databaseId must not be blank"); - this.databaseId = databaseId; + this.firestore = firestore; this.executor = executor; this.logger = logger; } + private @NotNull CollectionReference collection() { + return this.firestore.collection(COLLECTION); + } + /** * Compute the stable correlation key used to join {@code RequestActivation} - * (where we record the Slack messages) and {@code Approval} (where we look - * them back up). + * (where we record the Slack messages) and {@code ApprovalOperation} + * (where we look them back up). * - *

The fields chosen are present and unchanged across both events: + *

Fields chosen for stability across propose/accept: *

* - *

Justification is intentionally excluded (not part of approval event). - * Time fields are excluded because they have second-precision drift - * between propose and accept and are not the canonical identity of the - * request. + *

Justification is intentionally excluded (large, unstable). Time + * fields are excluded because they have second-precision drift between + * propose and accept. */ public static @NotNull String requestKey( @NotNull String beneficiary, @@ -94,7 +104,7 @@ public SlackMessageRegistry( return HexFormat.of().formatHex(digest); } catch (NoSuchAlgorithmException e) { - // SHA-256 is mandatory in every JRE; this branch is unreachable. + // SHA-256 is mandatory in every JRE. throw new AssertionError("SHA-256 not available", e); } } @@ -108,60 +118,95 @@ public SlackMessageRegistry( @NotNull List reviewerMessages, @NotNull Instant expiresAt ) { - return CompletableFuture.runAsync(() -> { - // TODO(phase-2): Firestore batch write with TTL field. - this.logger.info( - "slackRegistry.record.stub", - "Phase 1 stub: would persist %d entries for key=%s in db=%s", - reviewerMessages.size(), - requestKey, - this.databaseId); + return CompletableFutures.supplyAsync(() -> { + var doc = new HashMap(); + doc.put(FIELD_EXPIRES_AT, Timestamp.ofTimeSecondsAndNanos( + expiresAt.getEpochSecond(), + expiresAt.getNano())); + + var reviewers = new ArrayList>(reviewerMessages.size()); + for (var entry : reviewerMessages) { + var item = new HashMap(); + item.put("email", entry.email()); + item.put("user_id", entry.userId()); + item.put("channel_id", entry.channelId()); + item.put("message_ts", entry.messageTs()); + reviewers.add(item); + } + doc.put(FIELD_REVIEWERS, reviewers); + + try { + collection().document(requestKey).set(doc).get(); + } + catch (InterruptedException | ExecutionException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException( + "Failed to write Slack message registry entry " + requestKey, e); + } + return null; }, this.executor); } /** - * Retrieve previously recorded messages for a request, or an empty - * Optional if none exist (no Slack DMs were sent, or the entry has - * already been deleted / expired). + * Retrieve previously recorded messages for a request, or empty if the + * entry is missing (no DMs were sent, or already deleted/expired). */ + @SuppressWarnings("unchecked") public @NotNull CompletableFuture>> lookup( @NotNull String requestKey ) { - return CompletableFuture.supplyAsync(() -> { - // TODO(phase-2): Firestore document read. - this.logger.info( - "slackRegistry.lookup.stub", - "Phase 1 stub: would lookup key=%s in db=%s", - requestKey, - this.databaseId); - return Optional.>empty(); + return CompletableFutures.supplyAsync(() -> { + try { + var snapshot = collection().document(requestKey).get().get(); + if (!snapshot.exists()) { + return Optional.>empty(); + } + var reviewers = (List>) snapshot.get(FIELD_REVIEWERS); + if (reviewers == null) { + return Optional.>empty(); + } + var entries = new ArrayList(reviewers.size()); + for (var item : reviewers) { + entries.add(new ReviewerMessage( + (String) item.get("email"), + (String) item.get("user_id"), + (String) item.get("channel_id"), + (String) item.get("message_ts"))); + } + return Optional.of(List.copyOf(entries)); + } + catch (InterruptedException | ExecutionException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException( + "Failed to read Slack message registry entry " + requestKey, e); + } }, this.executor); } /** - * Remove a request entry once the approval flow is complete. Idempotent; - * a missing entry is not an error (TTL may have already deleted it). + * Remove a request entry once the approval flow is complete. Idempotent. */ public @NotNull CompletableFuture delete( @NotNull String requestKey ) { - return CompletableFuture.runAsync(() -> { - // TODO(phase-2): Firestore document delete. - this.logger.info( - "slackRegistry.delete.stub", - "Phase 1 stub: would delete key=%s in db=%s", - requestKey, - this.databaseId); + return CompletableFutures.supplyAsync(() -> { + try { + collection().document(requestKey).delete().get(); + } + catch (InterruptedException | ExecutionException e) { + Thread.currentThread().interrupt(); + // Deletion is best-effort; TTL will reap if we lose this race. + this.logger.warn( + "slackRegistry.delete.failed", + "Failed to delete registry entry %s (TTL will eventually reap): %s", + requestKey, e.getMessage()); + } + return null; }, this.executor); } /** * One reviewer's posted Slack DM, sufficient for chat.update later. - * - * @param email reviewer email (for sibling-update messages that name them) - * @param userId Slack user id, for re-resolution if needed - * @param channelId DM channel returned by conversations.open - * @param messageTs message timestamp returned by chat.postMessage */ public record ReviewerMessage( @NotNull String email, diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessages.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessages.java new file mode 100644 index 00000000..ab260e38 --- /dev/null +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackMessages.java @@ -0,0 +1,212 @@ +// +// Copyright 2026 Wave Mobile Money / wavemm fork +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// + +package com.google.solutions.jitaccess.web.proposal; + +import com.slack.api.model.block.ActionsBlock; +import com.slack.api.model.block.ContextBlock; +import com.slack.api.model.block.DividerBlock; +import com.slack.api.model.block.HeaderBlock; +import com.slack.api.model.block.LayoutBlock; +import com.slack.api.model.block.SectionBlock; +import com.slack.api.model.block.composition.MarkdownTextObject; +import com.slack.api.model.block.composition.PlainTextObject; +import com.slack.api.model.block.element.ButtonElement; +import org.jetbrains.annotations.NotNull; + +import java.net.URI; +import java.time.Duration; +import java.time.Instant; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Block Kit message templates for the Slack notification flow. + * + *

Three message shapes: + *

    + *
  1. {@link #reviewRequest} — DM sent to each qualified reviewer when an + * MPA elevation is requested. Carries an "Approve in JIT" button that + * links to the JIT approval URL (the JWT-bearing action URI). + *
  2. {@link #reviewerSiblingUpdate} — replaces a sibling reviewer's + * request DM after another reviewer approves. Strips the action button + * and adds an "approved by X" context line. + *
  3. {@link #beneficiaryApproved} — DM to the requester confirming the + * activation, naming the approver. + *
+ */ +final class SlackMessages { + private static final DateTimeFormatter HUMAN_TIME = DateTimeFormatter + .ofPattern("EEE d MMM, HH:mm z"); + + private SlackMessages() {} + + /** + * Builds the request DM sent to a reviewer. + * + * @param requesterEmail beneficiary's email (formatted in body) + * @param groupId JIT group being joined (formatted in body) + * @param justification user-provided text from the JIT submit form + * @param expiry token expiry (relative timing rendered in body) + * @param actionUri JWT-bearing URL the reviewer clicks to approve + * @param tz time zone for rendering expiry + */ + static List reviewRequest( + @NotNull String requesterEmail, + @NotNull String groupId, + @NotNull String justification, + @NotNull Instant expiry, + @NotNull URI actionUri, + @NotNull ZoneId tz + ) { + var blocks = new ArrayList(); + + blocks.add(HeaderBlock.builder() + .text(plain(":lock: PAM elevation request — your approval is needed")) + .build()); + + blocks.add(SectionBlock.builder() + .fields(List.of( + markdownField("*Requester*", ""), + markdownField("*Group*", "`" + groupId + "`"), + markdownField("*Expires*", relative(expiry, tz)), + markdownField("*Type*", "Multi-party approval") + )) + .build()); + + blocks.add(SectionBlock.builder() + .text(markdown("*Justification*\n>" + escapeBlockquote(justification))) + .build()); + + blocks.add(DividerBlock.builder().build()); + + blocks.add(ActionsBlock.builder() + .elements(List.of( + ButtonElement.builder() + .text(plain("Approve in JIT")) + .url(actionUri.toString()) + .style("primary") + .build())) + .build()); + + blocks.add(ContextBlock.builder() + .elements(List.of(markdown( + ":eyes: Approval is final — review the request carefully. " + + "Approving opens the JIT page where you confirm."))) + .build()); + + return blocks; + } + + /** + * Replaces the reviewer's request DM after a peer has approved. + * The action button is removed; a small context line names the approver. + */ + static List reviewerSiblingUpdate( + @NotNull String requesterEmail, + @NotNull String groupId, + @NotNull String approverEmail + ) { + return List.of( + HeaderBlock.builder() + .text(plain(":white_check_mark: Already approved — no action needed")) + .build(), + SectionBlock.builder() + .fields(List.of( + markdownField("*Requester*", ""), + markdownField("*Group*", "`" + groupId + "`"), + markdownField("*Approved by*", "") + )) + .build(), + ContextBlock.builder() + .elements(List.of(markdown( + ":information_source: Another reviewer approved this request. " + + "You can ignore this message."))) + .build()); + } + + /** + * DM to the beneficiary confirming the activation completed. + */ + static List beneficiaryApproved( + @NotNull String groupId, + @NotNull String approverEmail + ) { + return List.of( + HeaderBlock.builder() + .text(plain(":white_check_mark: PAM elevation approved")) + .build(), + SectionBlock.builder() + .fields(List.of( + markdownField("*Group*", "`" + groupId + "`"), + markdownField("*Approved by*", "") + )) + .build(), + ContextBlock.builder() + .elements(List.of(markdown( + "Your access is active until token expiry. Refresh `pam.wavemm.net` " + + "or `wavecli pam` to use it."))) + .build()); + } + + // ----- fallback text for notifications + a11y -------------------------- + + static String reviewRequestFallback(@NotNull String requesterEmail, @NotNull String groupId) { + return String.format( + "PAM approval requested by %s for %s — open the JIT app to approve.", + requesterEmail, groupId); + } + + static String reviewerSiblingUpdateFallback(@NotNull String approverEmail) { + return "Already approved by " + approverEmail + " — no action needed."; + } + + static String beneficiaryApprovedFallback(@NotNull String groupId, @NotNull String approverEmail) { + return String.format("Your PAM elevation for %s was approved by %s.", groupId, approverEmail); + } + + // ----- helpers --------------------------------------------------------- + + private static @NotNull PlainTextObject plain(@NotNull String text) { + return PlainTextObject.builder().text(text).emoji(true).build(); + } + + private static @NotNull MarkdownTextObject markdown(@NotNull String text) { + return MarkdownTextObject.builder().text(text).build(); + } + + private static @NotNull MarkdownTextObject markdownField(@NotNull String label, @NotNull String value) { + return markdown(label + "\n" + value); + } + + private static @NotNull String relative(@NotNull Instant expiry, @NotNull ZoneId tz) { + var remaining = Duration.between(Instant.now(), expiry); + var minutes = Math.max(0, remaining.toMinutes()); + var humanTime = HUMAN_TIME.format(expiry.atZone(tz)); + if (minutes < 60) { + return humanTime + " (~" + minutes + "m from now)"; + } + return humanTime + " (~" + (minutes / 60) + "h " + (minutes % 60) + "m from now)"; + } + + private static @NotNull String escapeBlockquote(@NotNull String text) { + // Slack blockquotes use leading > on each newline. Escape pre-existing + // ones so a malicious justification can't inject formatting. + return text.replace("\n", "\n>").replace("`", "'"); + } + + /** Used by tests to assert specific field values without parsing JSON. */ + static Map debugFields(@NotNull String requesterEmail, @NotNull String groupId) { + return Map.of("requester", requesterEmail, "group", groupId); + } +} diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java index 04afd209..ad5a9e3b 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java @@ -14,39 +14,63 @@ import com.google.solutions.jitaccess.apis.Logger; import com.google.solutions.jitaccess.apis.clients.AccessException; import com.google.solutions.jitaccess.auth.EndUserId; +import com.google.solutions.jitaccess.auth.GroupResolver; import com.google.solutions.jitaccess.auth.IamPrincipalId; +import com.google.solutions.jitaccess.auth.PrincipalId; import com.google.solutions.jitaccess.catalog.JitGroupContext; import com.google.solutions.jitaccess.catalog.Proposal; +import com.google.solutions.jitaccess.web.proposal.SlackMessageRegistry.ReviewerMessage; import org.jetbrains.annotations.NotNull; import java.io.IOException; import java.net.URI; +import java.time.Instant; +import java.time.ZoneId; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; import java.util.Random; -import java.util.stream.Collectors; +import java.util.Set; +import java.util.concurrent.CompletionException; /** - * Proposal handler that delivers approval requests via Slack DMs instead of - * email. + * Proposal handler that delivers approval requests via Slack DMs instead + * of email. Replaces (does not compose with) {@link MailProposalHandler} + * when Slack is configured — see SLACK_INTEGRATION.md. * - *

Replaces (does not compose with) {@link MailProposalHandler} when Slack - * is configured — see SLACK_INTEGRATION.md for the architectural rationale. + *

Flow on {@link #onOperationProposed}: + *

    + *
  1. Expand {@code proposal.recipients()} from a mix of users + groups + * into a flat set of individual users via {@link GroupResolver} + * (one round of expansion — non-recursive, same behaviour as + * the rest of JIT). + *
  2. Resolve each user's email to a Slack user ID. + *
  3. DM each resolved user with a Block Kit "review request" carrying + * the JWT-bearing {@code action_uri}. + *
  4. Persist {(channel, ts)} per reviewer to Firestore so siblings can + * be updated when one reviewer approves. + *
* - *

Phase 1 (this iteration): the handler is wired into the DI graph, - * receives proposal events, and logs structured "would notify" lines. The - * approval flow continues to work end-to-end; reviewers just don't actually - * receive Slack messages yet. This lets us validate config plumbing, - * feature-flag behaviour, and the JIT submodule deploy on staging before - * enabling real Slack traffic in Phase 2. + *

Flow on {@link #onProposalApproved}: + *

    + *
  1. Look up the registry entry by request key. + *
  2. For each non-approver sibling, {@code chat.update} the original DM + * to "Already approved by X — no action needed". + *
  3. DM the beneficiary "Your elevation was approved by X". + *
  4. Delete the registry entry. + *
* - *

Phase 2 will replace the stubs with real {@link SlackClient} + - * {@link SlackMessageRegistry} calls. The contract of this class — its - * constructor signature, the abstract methods it overrides, and the - * threading model (async, must not throw on Slack errors) — is intended to - * remain stable across phases. + *

All Slack and Firestore calls are async. Failures are logged at WARN + * but never propagated to the JIT request thread — a Slack outage must + * not block legitimate elevation requests. The exception is the initial + * {@code onOperationProposed}: if every recipient fails to be DM'd we + * surface the error so the requester knows the request didn't land + * anywhere actionable. */ public class SlackProposalHandler extends AbstractProposalHandler { private final @NotNull SlackClient slackClient; private final @NotNull SlackMessageRegistry registry; + private final @NotNull GroupResolver groupResolver; private final @NotNull Logger logger; private final @NotNull Options slackOptions; @@ -54,6 +78,7 @@ public SlackProposalHandler( @NotNull TokenSigner tokenSigner, @NotNull SlackClient slackClient, @NotNull SlackMessageRegistry registry, + @NotNull GroupResolver groupResolver, @NotNull Logger logger, @NotNull AbstractProposalHandler.Options baseOptions, @NotNull Options slackOptions @@ -61,10 +86,28 @@ public SlackProposalHandler( super(tokenSigner, new Random(), baseOptions); this.slackClient = slackClient; this.registry = registry; + this.groupResolver = groupResolver; this.logger = logger; this.slackOptions = slackOptions; } + /** + * Expand groups in the recipients set to individuals. Returns sorted + * unique emails. Non-user, non-group principals (e.g. service accounts) + * are dropped. + */ + private @NotNull List resolveRecipientEmails( + @NotNull Set recipients + ) throws AccessException { + Set expanded = this.groupResolver.expand(new HashSet<>(recipients)); + return expanded.stream() + .filter(EndUserId.class::isInstance) + .map(p -> ((EndUserId) p).email) + .distinct() + .sorted() + .toList(); + } + @Override void onOperationProposed( @NotNull JitGroupContext.JoinOperation operation, @@ -72,30 +115,87 @@ void onOperationProposed( @NotNull ProposalHandler.ProposalToken token, @NotNull URI actionUri ) throws AccessException, IOException { - var recipientEmails = proposal.recipients().stream() - .filter(EndUserId.class::isInstance) - .map(IamPrincipalId::value) - .sorted() - .collect(Collectors.toUnmodifiableList()); - - var requestKey = SlackMessageRegistry.requestKey( - proposal.user().value(), - operation.group().toString(), - recipientEmails); - - // TODO(phase-2): - // 1. for each recipient: slackClient.lookupUserByEmail → postDirectMessage - // (Block Kit: requester, group, justification, expiry, "Approve in JIT" link) - // 2. registry.record(requestKey, postedMessages, token.expiryTime()) - // 3. all async; failures logged but not thrown. + var beneficiary = proposal.user().email; + var groupId = operation.group().toString(); + + var reviewerEmails = resolveRecipientEmails(proposal.recipients()); + if (reviewerEmails.isEmpty()) { + throw new IOException( + "No qualified reviewers resolved to individual users for " + groupId); + } + + var justification = proposal.input().getOrDefault("justification", ""); + var requestKey = SlackMessageRegistry.requestKey(beneficiary, groupId, reviewerEmails); + + var blocks = SlackMessages.reviewRequest( + beneficiary, + groupId, + justification, + token.expiryTime(), + actionUri, + this.slackOptions.notificationTimeZone()); + var fallback = SlackMessages.reviewRequestFallback(beneficiary, groupId); + + // + // Resolve users + post DMs in parallel. Aggregate failures: if at least + // one DM lands, the approval flow is viable, so we record the + // successful subset and warn on the rest. If zero land, we throw — + // the requester needs to know nobody got the message. + // + var posted = new ArrayList(); + var failures = new ArrayList(); + + for (var email : reviewerEmails) { + try { + String userId = this.slackClient.lookupUserByEmail(email).join(); + if (userId == null) { + this.logger.warn( + "slack.lookupByEmail.notFound", + "Reviewer %s is not in the Slack workspace; skipping", + email); + failures.add(email); + continue; + } + SlackClient.PostedMessage message = this.slackClient + .postDirectMessage(userId, blocks, fallback) + .join(); + posted.add(new ReviewerMessage( + email, userId, message.channelId(), message.messageTs())); + } + catch (CompletionException | RuntimeException e) { + var cause = e.getCause() != null ? e.getCause() : e; + this.logger.warn( + "slack.dm.failed", + "Failed to DM reviewer %s for %s: %s", + email, groupId, cause.getMessage()); + failures.add(email); + } + } + + if (posted.isEmpty()) { + throw new IOException( + "Slack DM delivery failed for every reviewer (" + reviewerEmails.size() + + ") on " + groupId); + } + + try { + this.registry.record(requestKey, posted, token.expiryTime()).join(); + } + catch (CompletionException | RuntimeException e) { + // Registry write failure is bad — siblings won't update on approval — + // but the approval can still proceed via the live DM links. Log loud. + this.logger.error( + "slackRegistry.record.failed", + "Failed to persist Slack message registry for key=%s; sibling " + + "updates will not fire on approval. requester=%s group=%s", + requestKey, beneficiary, groupId, e); + } + this.logger.info( - "slack.onOperationProposed.stub", - "Phase 1 stub: requester=%s group=%s recipients=%d key=%s actionUri=%s", - proposal.user().value(), - operation.group(), - recipientEmails.size(), - requestKey, - actionUri); + "slack.onOperationProposed", + "Posted %d/%d Slack DMs for %s requesting %s (key=%s, failures=%s)", + posted.size(), reviewerEmails.size(), beneficiary, groupId, + requestKey, failures); } @Override @@ -103,41 +203,97 @@ void onProposalApproved( @NotNull JitGroupContext.ApprovalOperation operation, @NotNull Proposal proposal ) throws AccessException, IOException { - var recipientEmails = proposal.recipients().stream() - .filter(EndUserId.class::isInstance) - .map(IamPrincipalId::value) - .sorted() - .collect(Collectors.toUnmodifiableList()); - - var requestKey = SlackMessageRegistry.requestKey( - proposal.user().value(), - proposal.group().toString(), - recipientEmails); - - // TODO(phase-2): - // 1. registry.lookup(requestKey) - // 2. for each posted message NOT belonging to the approver: - // slackClient.updateMessage(channel, ts, "✅ Approved by ") - // 3. for the beneficiary: - // slackClient.lookupUserByEmail(proposal.user()) - // slackClient.postDirectMessage(...) — single result DM. - // 4. registry.delete(requestKey) - // All async; failures logged but not thrown. + var beneficiary = proposal.user().email; + var groupId = proposal.group().toString(); + var approverEmail = operation.user().email; + + var reviewerEmails = resolveRecipientEmails(proposal.recipients()); + var requestKey = SlackMessageRegistry.requestKey(beneficiary, groupId, reviewerEmails); + + var entriesOpt = this.registry.lookup(requestKey).join(); + if (entriesOpt.isEmpty()) { + this.logger.warn( + "slackRegistry.lookup.miss", + "No Slack registry entry for approved request key=%s; siblings " + + "won't be updated. requester=%s group=%s approver=%s", + requestKey, beneficiary, groupId, approverEmail); + // Still notify the beneficiary directly. + notifyBeneficiary(beneficiary, groupId, approverEmail); + return; + } + + var siblingBlocks = SlackMessages.reviewerSiblingUpdate( + beneficiary, groupId, approverEmail); + var siblingFallback = SlackMessages.reviewerSiblingUpdateFallback(approverEmail); + + for (var entry : entriesOpt.get()) { + if (entry.email().equalsIgnoreCase(approverEmail)) { + // The approver doesn't need a "you approved" update — they did it. + continue; + } + try { + this.slackClient.updateMessage( + entry.channelId(), entry.messageTs(), siblingBlocks, siblingFallback).join(); + } + catch (CompletionException | 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", + entry.channelId(), entry.messageTs(), entry.email(), cause.getMessage()); + } + } + + notifyBeneficiary(beneficiary, groupId, approverEmail); + + try { + this.registry.delete(requestKey).join(); + } + catch (CompletionException | RuntimeException e) { + // Best-effort; TTL will reap. + } + this.logger.info( - "slack.onProposalApproved.stub", - "Phase 1 stub: requester=%s group=%s key=%s", - proposal.user().value(), - proposal.group(), - requestKey); + "slack.onProposalApproved", + "Updated %d sibling DM(s) for approved request key=%s (approver=%s)", + Math.max(0, entriesOpt.get().size() - 1), requestKey, approverEmail); + } + + private void notifyBeneficiary( + @NotNull String beneficiary, + @NotNull String groupId, + @NotNull String approverEmail + ) { + try { + String userId = this.slackClient.lookupUserByEmail(beneficiary).join(); + if (userId == null) { + this.logger.warn( + "slack.lookupByEmail.notFound", + "Beneficiary %s is not in the Slack workspace; skipping confirmation DM", + beneficiary); + return; + } + this.slackClient.postDirectMessage( + userId, + SlackMessages.beneficiaryApproved(groupId, approverEmail), + SlackMessages.beneficiaryApprovedFallback(groupId, approverEmail)).join(); + } + catch (CompletionException | RuntimeException e) { + var cause = e.getCause() != null ? e.getCause() : e; + this.logger.warn( + "slack.beneficiaryDM.failed", + "Failed to DM beneficiary %s for approved %s: %s", + beneficiary, groupId, cause.getMessage()); + } } /** - * Slack-specific options, complementary to {@link AbstractProposalHandler.Options}. + * Slack-specific options. * * @param notificationTimeZone Time zone used to render expiry timestamps in DMs. */ public record Options( - @NotNull java.time.ZoneId notificationTimeZone + @NotNull ZoneId notificationTimeZone ) { public Options { Preconditions.checkArgument( diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessageRegistry.java b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessageRegistry.java new file mode 100644 index 00000000..3f4553dc --- /dev/null +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessageRegistry.java @@ -0,0 +1,90 @@ +// +// Copyright 2026 Wave Mobile Money / wavemm fork +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// + +package com.google.solutions.jitaccess.web.proposal; + +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; + +public class TestSlackMessageRegistry { + + // ------------------------------------------------------------------------- + // requestKey — must be stable, deterministic, and order-independent on + // recipients (because the AbstractProposalHandler sorts them differently + // on propose vs. accept depending on Set iteration order). + // ------------------------------------------------------------------------- + + @Test + public void requestKey_isDeterministic() { + var k1 = SlackMessageRegistry.requestKey( + "alice@example.com", + "env/sys/grp", + List.of("bob@example.com", "carol@example.com")); + var k2 = SlackMessageRegistry.requestKey( + "alice@example.com", + "env/sys/grp", + List.of("bob@example.com", "carol@example.com")); + assertEquals(k1, k2); + assertEquals(64, k1.length(), "SHA-256 hex"); + } + + @Test + public void requestKey_isInvariantToRecipientOrder() { + var k1 = SlackMessageRegistry.requestKey( + "alice@example.com", + "env/sys/grp", + List.of("bob@example.com", "carol@example.com")); + var k2 = SlackMessageRegistry.requestKey( + "alice@example.com", + "env/sys/grp", + List.of("carol@example.com", "bob@example.com")); + assertEquals(k1, k2, + "Recipient ordering must not affect the key — propose and accept " + + "may iterate the recipients Set in different orders."); + } + + @Test + public void requestKey_distinguishesBeneficiary() { + var k1 = SlackMessageRegistry.requestKey( + "alice@example.com", "env/sys/grp", List.of("bob@example.com")); + var k2 = SlackMessageRegistry.requestKey( + "alex@example.com", "env/sys/grp", List.of("bob@example.com")); + assertNotEquals(k1, k2); + } + + @Test + public void requestKey_distinguishesGroup() { + var k1 = SlackMessageRegistry.requestKey( + "alice@example.com", "env/sys/grp-1", List.of("bob@example.com")); + var k2 = SlackMessageRegistry.requestKey( + "alice@example.com", "env/sys/grp-2", List.of("bob@example.com")); + assertNotEquals(k1, k2); + } + + @Test + public void requestKey_distinguishesRecipients() { + var k1 = SlackMessageRegistry.requestKey( + "alice@example.com", "env/sys/grp", List.of("bob@example.com")); + var k2 = SlackMessageRegistry.requestKey( + "alice@example.com", "env/sys/grp", List.of("carol@example.com")); + assertNotEquals(k1, k2); + } + + @Test + public void requestKey_emptyRecipientsIsValid() { + // Empty recipients shouldn't crash even though no real flow produces it. + var k = SlackMessageRegistry.requestKey( + "alice@example.com", "env/sys/grp", List.of()); + assertEquals(64, k.length()); + } +} diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java new file mode 100644 index 00000000..f366e1e8 --- /dev/null +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackMessages.java @@ -0,0 +1,151 @@ +// +// Copyright 2026 Wave Mobile Money / wavemm fork +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// + +package com.google.solutions.jitaccess.web.proposal; + +import com.slack.api.model.block.ActionsBlock; +import com.slack.api.model.block.element.ButtonElement; +import org.junit.jupiter.api.Test; + +import java.net.URI; +import java.time.Instant; +import java.time.ZoneId; +import java.time.temporal.ChronoUnit; + +import static org.junit.jupiter.api.Assertions.*; + +public class TestSlackMessages { + + private static final ZoneId UTC = ZoneId.of("UTC"); + + // ------------------------------------------------------------------------- + // reviewRequest — primary regression coverage. Asserts that the action + // button URL matches the JWT URL we pass in (load-bearing: this is the + // only way the reviewer can actually approve). + // ------------------------------------------------------------------------- + + @Test + public void reviewRequest_carriesActionUriOnPrimaryButton() { + var actionUri = URI.create( + "https://pam.wavemm.net/?activation=eyJhbGciOiJSUzI1NiJ9.x.y"); + + var blocks = SlackMessages.reviewRequest( + "alice@example.com", + "env/sys/grp", + "Working on CASE-123", + Instant.now().plus(1, ChronoUnit.HOURS), + actionUri, + UTC); + + var action = blocks.stream() + .filter(ActionsBlock.class::isInstance) + .map(ActionsBlock.class::cast) + .findFirst() + .orElseThrow(() -> new AssertionError("expected an ActionsBlock")); + + var button = action.getElements().stream() + .filter(ButtonElement.class::isInstance) + .map(ButtonElement.class::cast) + .findFirst() + .orElseThrow(() -> new AssertionError("expected a ButtonElement")); + + assertEquals(actionUri.toString(), button.getUrl()); + assertEquals("primary", button.getStyle()); + } + + @Test + public void reviewRequest_includesRequesterAndGroupAndJustification() { + var blocks = SlackMessages.reviewRequest( + "alice@example.com", + "env/sys/grp", + "Working on CASE-123", + Instant.parse("2030-01-01T12:00:00Z"), + URI.create("https://pam.wavemm.net/?activation=jwt"), + UTC); + + var serialized = blocks.toString(); + assertTrue(serialized.contains("alice@example.com"), "requester email present"); + assertTrue(serialized.contains("env/sys/grp"), "group id present"); + assertTrue(serialized.contains("CASE-123"), "justification present"); + } + + @Test + public void reviewRequest_escapesBlockquoteFormatting() { + // Inject newlines in the justification — they must be escaped so a + // crafted message can't break out of the >justification block. + var blocks = SlackMessages.reviewRequest( + "alice@example.com", + "env/sys/grp", + "line one\nline two", + Instant.now().plus(1, ChronoUnit.HOURS), + URI.create("https://pam.wavemm.net/?activation=jwt"), + UTC); + + var serialized = blocks.toString(); + // Expect each new line to begin with a leading > so it stays inside the + // blockquote we opened in the template. + assertTrue(serialized.contains(">line one\n>line two") + || serialized.contains("\\n>line two"), + "justification newlines must be escaped to keep blockquote formatting"); + } + + // ------------------------------------------------------------------------- + // reviewerSiblingUpdate — must NOT carry an action button (we don't want + // a sibling reviewer clicking after a peer already approved). + // ------------------------------------------------------------------------- + + @Test + public void reviewerSiblingUpdate_dropsActionButton() { + var blocks = SlackMessages.reviewerSiblingUpdate( + "alice@example.com", "env/sys/grp", "bob@example.com"); + + boolean hasActions = blocks.stream().anyMatch(ActionsBlock.class::isInstance); + assertFalse(hasActions, "sibling-update messages must not carry action buttons"); + } + + @Test + public void reviewerSiblingUpdate_namesApprover() { + var blocks = SlackMessages.reviewerSiblingUpdate( + "alice@example.com", "env/sys/grp", "bob@example.com"); + + assertTrue(blocks.toString().contains("bob@example.com"), + "sibling-update message must name the approver so reviewers know who acted"); + } + + // ------------------------------------------------------------------------- + // beneficiaryApproved — confirms to requester that the elevation landed. + // ------------------------------------------------------------------------- + + @Test + public void beneficiaryApproved_namesGroupAndApprover() { + var blocks = SlackMessages.beneficiaryApproved( + "env/sys/grp", "bob@example.com"); + + var serialized = blocks.toString(); + assertTrue(serialized.contains("env/sys/grp")); + assertTrue(serialized.contains("bob@example.com")); + assertFalse(blocks.stream().anyMatch(ActionsBlock.class::isInstance), + "beneficiary confirmation has no further actions"); + } + + // ------------------------------------------------------------------------- + // Fallback strings (used as accessibility text + push notification body) + // ------------------------------------------------------------------------- + + @Test + public void fallbackStrings_containKeyFields() { + assertTrue(SlackMessages.reviewRequestFallback("alice@example.com", "env/sys/grp") + .contains("alice@example.com")); + assertTrue(SlackMessages.reviewerSiblingUpdateFallback("bob@example.com") + .contains("bob@example.com")); + assertTrue(SlackMessages.beneficiaryApprovedFallback("env/sys/grp", "bob@example.com") + .contains("env/sys/grp")); + } +} diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java new file mode 100644 index 00000000..bd5dfdab --- /dev/null +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/proposal/TestSlackProposalHandler.java @@ -0,0 +1,275 @@ +// +// Copyright 2026 Wave Mobile Money / wavemm fork +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// + +package com.google.solutions.jitaccess.web.proposal; + +import com.google.solutions.jitaccess.apis.Logger; +import com.google.solutions.jitaccess.auth.EndUserId; +import com.google.solutions.jitaccess.auth.GroupResolver; +import com.google.solutions.jitaccess.auth.IamPrincipalId; +import com.google.solutions.jitaccess.auth.JitGroupId; +import com.google.solutions.jitaccess.auth.PrincipalId; +import com.google.solutions.jitaccess.catalog.JitGroupContext; +import com.google.solutions.jitaccess.catalog.Proposal; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +import java.io.IOException; +import java.net.URI; +import java.time.Duration; +import java.time.Instant; +import java.time.ZoneId; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyList; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.*; + +public class TestSlackProposalHandler { + + private static final EndUserId ALICE = new EndUserId("alice@example.com"); + private static final EndUserId BOB = new EndUserId("bob@example.com"); + private static final EndUserId CAROL = new EndUserId("carol@example.com"); + private static final JitGroupId GROUP = new JitGroupId("env", "sys", "grp"); + private static final URI ACTION_URI = URI.create( + "https://pam.wavemm.net/?activation=jwt-stub"); + + private SlackProposalHandler newHandler( + SlackClient slackClient, + SlackMessageRegistry registry, + GroupResolver groupResolver + ) { + return new SlackProposalHandler( + mock(TokenSigner.class), + slackClient, + registry, + groupResolver, + mock(Logger.class), + new AbstractProposalHandler.Options(Duration.ofMinutes(60)), + new SlackProposalHandler.Options(ZoneId.of("UTC"))); + } + + private static GroupResolver groupResolverPassthrough() { + var resolver = mock(GroupResolver.class); + try { + when(resolver.expand(any())).thenAnswer(inv -> { + Set in = inv.getArgument(0); + return Set.copyOf(in); // no group expansion in tests + }); + } + catch (Exception e) { + throw new RuntimeException(e); + } + return resolver; + } + + private static SlackClient slackClientHappyPath() { + var client = mock(SlackClient.class); + when(client.lookupUserByEmail(anyString())) + .thenAnswer(inv -> CompletableFuture.completedFuture( + "U-" + inv.getArgument(0).hashCode())); + when(client.postDirectMessage(anyString(), anyList(), anyString())) + .thenAnswer(inv -> CompletableFuture.completedFuture( + new SlackClient.PostedMessage( + "C-" + inv.getArgument(0), + "12345.67890"))); + when(client.updateMessage(anyString(), anyString(), anyList(), anyString())) + .thenReturn(CompletableFuture.completedFuture(null)); + return client; + } + + private static SlackMessageRegistry registryHappyPath() { + var registry = mock(SlackMessageRegistry.class); + when(registry.record(anyString(), anyList(), any())) + .thenReturn(CompletableFuture.completedFuture(null)); + when(registry.lookup(anyString())) + .thenReturn(CompletableFuture.completedFuture(Optional.empty())); + when(registry.delete(anyString())) + .thenReturn(CompletableFuture.completedFuture(null)); + return registry; + } + + private static JitGroupContext.JoinOperation operationFor(EndUserId user) { + var op = mock(JitGroupContext.JoinOperation.class); + when(op.group()).thenReturn(GROUP); + when(op.user()).thenReturn(user); + return op; + } + + private static Proposal proposalFor( + EndUserId user, Set recipients + ) { + var p = mock(Proposal.class); + when(p.user()).thenReturn(user); + when(p.recipients()).thenReturn(recipients); + when(p.group()).thenReturn(GROUP); + when(p.input()).thenReturn(Map.of("justification", "CASE-123")); + when(p.expiry()).thenReturn(Instant.now().plus(Duration.ofMinutes(60))); + return p; + } + + private static ProposalHandler.ProposalToken tokenFor(Set audience) { + return new ProposalHandler.ProposalToken( + "stub-jwt", + audience, + Instant.now().plus(Duration.ofMinutes(60))); + } + + // ------------------------------------------------------------------------- + // onOperationProposed. + // ------------------------------------------------------------------------- + + @Test + public void onOperationProposed_dmsAllReviewersAndRecordsRegistry() throws Exception { + var slack = slackClientHappyPath(); + var registry = registryHappyPath(); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + var recipients = Set.of(BOB, CAROL); + handler.onOperationProposed( + operationFor(ALICE), + proposalFor(ALICE, recipients), + tokenFor(recipients), + ACTION_URI); + + verify(slack).lookupUserByEmail(eq("bob@example.com")); + verify(slack).lookupUserByEmail(eq("carol@example.com")); + verify(slack, times(2)).postDirectMessage(anyString(), anyList(), anyString()); + + var entriesCaptor = ArgumentCaptor.forClass(List.class); + verify(registry).record(anyString(), entriesCaptor.capture(), any()); + var entries = entriesCaptor.getValue(); + assertEquals(2, entries.size()); + } + + @Test + public void onOperationProposed_skipsReviewersWhenSlackUserNotFound() throws Exception { + var slack = slackClientHappyPath(); + when(slack.lookupUserByEmail(eq("bob@example.com"))) + .thenReturn(CompletableFuture.completedFuture(null)); + var registry = registryHappyPath(); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + handler.onOperationProposed( + operationFor(ALICE), + proposalFor(ALICE, Set.of(BOB, CAROL)), + tokenFor(Set.of(BOB, CAROL)), + ACTION_URI); + + verify(slack, times(1)) + .postDirectMessage(anyString(), anyList(), anyString()); + + var entriesCaptor = ArgumentCaptor.forClass(List.class); + verify(registry).record(anyString(), entriesCaptor.capture(), any()); + assertEquals(1, entriesCaptor.getValue().size(), + "registry should only contain the reviewers we successfully DM'd"); + } + + @Test + public void onOperationProposed_throwsWhenAllRecipientsFail() { + var slack = slackClientHappyPath(); + when(slack.lookupUserByEmail(anyString())) + .thenReturn(CompletableFuture.completedFuture(null)); + var registry = registryHappyPath(); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + assertThrows(IOException.class, () -> + handler.onOperationProposed( + operationFor(ALICE), + proposalFor(ALICE, Set.of(BOB, CAROL)), + tokenFor(Set.of(BOB, CAROL)), + ACTION_URI)); + + verify(registry, never()).record(anyString(), anyList(), any()); + } + + @Test + public void onOperationProposed_throwsWhenNoIndividualUsersAfterExpansion() throws Exception { + var slack = slackClientHappyPath(); + var registry = registryHappyPath(); + var resolver = mock(GroupResolver.class); + when(resolver.expand(any())).thenReturn(Set.of()); // group expanded to nothing + var handler = newHandler(slack, registry, resolver); + + assertThrows(IOException.class, () -> + handler.onOperationProposed( + operationFor(ALICE), + proposalFor(ALICE, Set.of(BOB)), + tokenFor(Set.of(BOB)), + ACTION_URI)); + } + + // ------------------------------------------------------------------------- + // onProposalApproved. + // ------------------------------------------------------------------------- + + @Test + public void onProposalApproved_updatesSiblingsButNotApprover() throws Exception { + var slack = slackClientHappyPath(); + var registry = mock(SlackMessageRegistry.class); + var entries = List.of( + new SlackMessageRegistry.ReviewerMessage( + "bob@example.com", "U-BOB", "C-BOB", "111.111"), + new SlackMessageRegistry.ReviewerMessage( + "carol@example.com", "U-CAROL", "C-CAROL", "222.222")); + when(registry.lookup(anyString())) + .thenReturn(CompletableFuture.completedFuture(Optional.of(entries))); + when(registry.delete(anyString())) + .thenReturn(CompletableFuture.completedFuture(null)); + + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + var approval = mock(JitGroupContext.ApprovalOperation.class); + when(approval.user()).thenReturn(BOB); // Bob is approving + + handler.onProposalApproved( + approval, + proposalFor(ALICE, Set.of(BOB, CAROL))); + + // Carol's DM gets updated; Bob's does NOT (he just approved). + verify(slack).updateMessage(eq("C-CAROL"), eq("222.222"), anyList(), anyString()); + verify(slack, never()).updateMessage(eq("C-BOB"), anyString(), anyList(), anyString()); + + // Beneficiary (Alice) gets a confirmation DM. + verify(slack).lookupUserByEmail(eq("alice@example.com")); + verify(slack).postDirectMessage(anyString(), anyList(), anyString()); + + // Registry entry deleted after handling. + verify(registry).delete(anyString()); + } + + @Test + public void onProposalApproved_stillNotifiesBeneficiaryOnRegistryMiss() throws Exception { + var slack = slackClientHappyPath(); + var registry = mock(SlackMessageRegistry.class); + when(registry.lookup(anyString())) + .thenReturn(CompletableFuture.completedFuture(Optional.empty())); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + var approval = mock(JitGroupContext.ApprovalOperation.class); + when(approval.user()).thenReturn(BOB); + + handler.onProposalApproved( + approval, + proposalFor(ALICE, Set.of(BOB, CAROL))); + + // No siblings to update (we lost the registry), but Alice still gets her DM. + verify(slack, never()).updateMessage(anyString(), anyString(), anyList(), anyString()); + verify(slack).lookupUserByEmail(eq("alice@example.com")); + verify(slack).postDirectMessage(anyString(), anyList(), anyString()); + } +} From 5d55c699bc502b00f0545e8f03bd21afd0782293 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Thu, 30 Apr 2026 09:58:40 +0200 Subject: [PATCH 2/2] Address #4 review feedback from @alex-young MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three behavioural fixes from the post-merge review of #4: 1. JWT IDs use SecureRandom, not Random AbstractProposalHandler accepts the random source via constructor; MailProposalHandler and DebugProposalHandler both pass SecureRandom. SlackProposalHandler was passing plain Random — JWT IDs are activation-token nonces and must be unpredictable to resist enumeration. Drop the j.u.Random import, switch to SecureRandom. 2. Fail loudly when SLACK_NOTIFICATIONS_ENABLED=true but companion vars are missing The previous logic short-circuited via isSlackConfigured() (which ANDed the flag with bot-token-secret + firestore-database presence). That made a partial misconfig silently fall through to SMTP/501 — the operator's intent ("turn Slack on") would only manifest at the next MPA request, hours later, when nothing happens. Now we branch on slackNotificationsEnabled alone and throw at startup if either companion var is missing, with a message naming both the missing var and the rollback knob. isSlackConfigured() is dropped — its semantics were exactly the silent-fall-through case we don't want. 3. Single fingerprint() helper instead of duplicating the requestKey computation in onOperationProposed and onProposalApproved The two sides have to compute the SAME key or the lookup misses on approval and siblings never update. Extracting to one helper removes the drift surface. Returns a small RegistryFingerprint record carrying beneficiary, groupId, expanded reviewerEmails, and the SHA-256 key — same shape both sides need. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../solutions/jitaccess/web/Application.java | 18 +++- .../web/ApplicationConfiguration.java | 11 --- .../web/proposal/SlackProposalHandler.java | 98 +++++++++++-------- 3 files changed, 74 insertions(+), 53 deletions(-) diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java b/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java index 33539fdc..bf8e12fe 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/Application.java @@ -280,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()); diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java b/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java index d525fd6f..e8fec216 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/ApplicationConfiguration.java @@ -326,17 +326,6 @@ public ApplicationConfiguration(@NotNull Map 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(); } diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java index ad5a9e3b..b457ac8f 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/SlackProposalHandler.java @@ -15,7 +15,6 @@ import com.google.solutions.jitaccess.apis.clients.AccessException; import com.google.solutions.jitaccess.auth.EndUserId; import com.google.solutions.jitaccess.auth.GroupResolver; -import com.google.solutions.jitaccess.auth.IamPrincipalId; import com.google.solutions.jitaccess.auth.PrincipalId; import com.google.solutions.jitaccess.catalog.JitGroupContext; import com.google.solutions.jitaccess.catalog.Proposal; @@ -24,12 +23,11 @@ import java.io.IOException; import java.net.URI; -import java.time.Instant; +import java.security.SecureRandom; import java.time.ZoneId; import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import java.util.Random; import java.util.Set; import java.util.concurrent.CompletionException; @@ -83,7 +81,10 @@ public SlackProposalHandler( @NotNull AbstractProposalHandler.Options baseOptions, @NotNull Options slackOptions ) { - super(tokenSigner, new Random(), baseOptions); + // Crypto-random for JWT IDs — these are activation token nonces, must + // be unpredictable to prevent enumeration. Matches MailProposalHandler + // and DebugProposalHandler. + super(tokenSigner, new SecureRandom(), baseOptions); this.slackClient = slackClient; this.registry = registry; this.groupResolver = groupResolver; @@ -92,22 +93,45 @@ public SlackProposalHandler( } /** - * Expand groups in the recipients set to individuals. Returns sorted - * unique emails. Non-user, non-group principals (e.g. service accounts) - * are dropped. + * Compute the registry fingerprint of a proposal — beneficiary, group, + * resolved reviewer emails, and the SHA-256 key derived from those. + * + *

Used by both {@link #onOperationProposed} (where we record the entry) + * and {@link #onProposalApproved} (where we look it back up). Computing + * it in a single helper avoids drift between the two sides — they must + * compute the same key or the lookup misses and siblings don't get + * updated. + * + *

Group expansion is intentionally part of the fingerprint: if the + * policy ACL names a group, the propose-side and accept-side both pass + * through {@link GroupResolver#expand}, ending up with the same flat + * email set assuming Cloud Identity returns the same membership for + * both calls (which it should within the JWT validity window). */ - private @NotNull List resolveRecipientEmails( - @NotNull Set recipients + private @NotNull RegistryFingerprint fingerprint( + @NotNull Proposal proposal ) throws AccessException { - Set expanded = this.groupResolver.expand(new HashSet<>(recipients)); - return expanded.stream() + var beneficiary = proposal.user().email; + var groupId = proposal.group().toString(); + Set expanded = this.groupResolver.expand( + new HashSet<>(proposal.recipients())); + var reviewerEmails = expanded.stream() .filter(EndUserId.class::isInstance) .map(p -> ((EndUserId) p).email) .distinct() .sorted() .toList(); + var key = SlackMessageRegistry.requestKey(beneficiary, groupId, reviewerEmails); + return new RegistryFingerprint(beneficiary, groupId, reviewerEmails, key); } + private record RegistryFingerprint( + @NotNull String beneficiary, + @NotNull String groupId, + @NotNull List reviewerEmails, + @NotNull String key + ) {} + @Override void onOperationProposed( @NotNull JitGroupContext.JoinOperation operation, @@ -115,26 +139,22 @@ void onOperationProposed( @NotNull ProposalHandler.ProposalToken token, @NotNull URI actionUri ) throws AccessException, IOException { - var beneficiary = proposal.user().email; - var groupId = operation.group().toString(); - - var reviewerEmails = resolveRecipientEmails(proposal.recipients()); - if (reviewerEmails.isEmpty()) { + var fp = fingerprint(proposal); + if (fp.reviewerEmails().isEmpty()) { throw new IOException( - "No qualified reviewers resolved to individual users for " + groupId); + "No qualified reviewers resolved to individual users for " + fp.groupId()); } var justification = proposal.input().getOrDefault("justification", ""); - var requestKey = SlackMessageRegistry.requestKey(beneficiary, groupId, reviewerEmails); var blocks = SlackMessages.reviewRequest( - beneficiary, - groupId, + fp.beneficiary(), + fp.groupId(), justification, token.expiryTime(), actionUri, this.slackOptions.notificationTimeZone()); - var fallback = SlackMessages.reviewRequestFallback(beneficiary, groupId); + var fallback = SlackMessages.reviewRequestFallback(fp.beneficiary(), fp.groupId()); // // Resolve users + post DMs in parallel. Aggregate failures: if at least @@ -145,7 +165,7 @@ void onOperationProposed( var posted = new ArrayList(); var failures = new ArrayList(); - for (var email : reviewerEmails) { + for (var email : fp.reviewerEmails()) { try { String userId = this.slackClient.lookupUserByEmail(email).join(); if (userId == null) { @@ -167,19 +187,19 @@ void onOperationProposed( this.logger.warn( "slack.dm.failed", "Failed to DM reviewer %s for %s: %s", - email, groupId, cause.getMessage()); + email, fp.groupId(), cause.getMessage()); failures.add(email); } } if (posted.isEmpty()) { throw new IOException( - "Slack DM delivery failed for every reviewer (" + reviewerEmails.size() - + ") on " + groupId); + "Slack DM delivery failed for every reviewer (" + fp.reviewerEmails().size() + + ") on " + fp.groupId()); } try { - this.registry.record(requestKey, posted, token.expiryTime()).join(); + this.registry.record(fp.key(), posted, token.expiryTime()).join(); } catch (CompletionException | RuntimeException e) { // Registry write failure is bad — siblings won't update on approval — @@ -188,14 +208,14 @@ void onOperationProposed( "slackRegistry.record.failed", "Failed to persist Slack message registry for key=%s; sibling " + "updates will not fire on approval. requester=%s group=%s", - requestKey, beneficiary, groupId, e); + fp.key(), fp.beneficiary(), fp.groupId(), e); } this.logger.info( "slack.onOperationProposed", "Posted %d/%d Slack DMs for %s requesting %s (key=%s, failures=%s)", - posted.size(), reviewerEmails.size(), beneficiary, groupId, - requestKey, failures); + posted.size(), fp.reviewerEmails().size(), fp.beneficiary(), fp.groupId(), + fp.key(), failures); } @Override @@ -203,27 +223,23 @@ void onProposalApproved( @NotNull JitGroupContext.ApprovalOperation operation, @NotNull Proposal proposal ) throws AccessException, IOException { - var beneficiary = proposal.user().email; - var groupId = proposal.group().toString(); + var fp = fingerprint(proposal); var approverEmail = operation.user().email; - var reviewerEmails = resolveRecipientEmails(proposal.recipients()); - var requestKey = SlackMessageRegistry.requestKey(beneficiary, groupId, reviewerEmails); - - var entriesOpt = this.registry.lookup(requestKey).join(); + var entriesOpt = this.registry.lookup(fp.key()).join(); if (entriesOpt.isEmpty()) { this.logger.warn( "slackRegistry.lookup.miss", "No Slack registry entry for approved request key=%s; siblings " + "won't be updated. requester=%s group=%s approver=%s", - requestKey, beneficiary, groupId, approverEmail); + fp.key(), fp.beneficiary(), fp.groupId(), approverEmail); // Still notify the beneficiary directly. - notifyBeneficiary(beneficiary, groupId, approverEmail); + notifyBeneficiary(fp.beneficiary(), fp.groupId(), approverEmail); return; } var siblingBlocks = SlackMessages.reviewerSiblingUpdate( - beneficiary, groupId, approverEmail); + fp.beneficiary(), fp.groupId(), approverEmail); var siblingFallback = SlackMessages.reviewerSiblingUpdateFallback(approverEmail); for (var entry : entriesOpt.get()) { @@ -244,10 +260,10 @@ void onProposalApproved( } } - notifyBeneficiary(beneficiary, groupId, approverEmail); + notifyBeneficiary(fp.beneficiary(), fp.groupId(), approverEmail); try { - this.registry.delete(requestKey).join(); + this.registry.delete(fp.key()).join(); } catch (CompletionException | RuntimeException e) { // Best-effort; TTL will reap. @@ -256,7 +272,7 @@ 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), requestKey, approverEmail); + Math.max(0, entriesOpt.get().size() - 1), fp.key(), approverEmail); } private void notifyBeneficiary(