Phase 1: Slack notifications scaffolding (feature-flagged, inert) by phosphore · Pull Request #4 · wavemm/jit-groups

phosphore · 2026-04-29T08:10:22Z

Summary

Phase 1 of Wave's Slack-notifications fork. Adds the DI graph, configuration
plumbing, and Terraform module toggle for a forthcoming Slack-based approval
flow that replaces the unconfigured SMTP path on the wavemm fork.

Feature-flagged off by default — SLACK_NOTIFICATIONS_ENABLED=false
short-circuits the new factory branch and falls through to the upstream
SMTP/Debug handlers, so this PR changes no runtime behaviour. Flipping the
flag on requires the matching infra PR
(wavemm/wavemm-iam#283)

a populated bot-token secret.

Full design, rollback, and 3-phase plan in
SLACK_INTEGRATION.md
(in the consumer repo, since wavemm-iam owns the integration's lifecycle).

What's in this PR

Java

ApplicationConfiguration: read SLACK_NOTIFICATIONS_ENABLED,
SLACK_BOT_TOKEN_SECRET, SLACK_FIRESTORE_DATABASE env vars +
isSlackConfigured() helper.
Application.produceProposalHandler: when the flag is on, fetch the bot
token via SecretManagerClient, construct SlackProposalHandler, return
it. Falls through to the existing SMTP/Debug branches otherwise. Fails
loudly on Slack init errors rather than silently dropping notifications
— operator likely thinks they're wired.
New SlackProposalHandler extends AbstractProposalHandler. Phase 1
overrides log structured would notify lines; Phase 2 will replace them
with real Block Kit DMs and chat.update sibling propagation.
New SlackClient: async wrapper for users.lookupByEmail /
chat.postMessage / chat.update. Phase 1 stubs return successful
futures so the DI graph wires up.
New SlackMessageRegistry: Firestore-backed (channel, ts) tracker
keyed by sha256(beneficiary, group, sorted recipients). Phase 1 stubs.

Build

pom.xml: com.slack.api:slack-api-client 1.45.3,
com.google.cloud:google-cloud-firestore 3.31.7. Both pulled in
unconditionally — they're trivially small compared to the existing
Google API client deps and the runtime gating happens in Java.

Terraform (jitgroups-appengine module)

New var.promote_traffic (default true). When false, the
google_app_engine_service_split_traffic resource is not managed and the
newly-deployed version stays at its versioned URL behind IAP without
receiving production traffic — Wave's staging mechanism per
SLACK_INTEGRATION.md.

Why a fork patch and not upstream

This is intentional Wave-specific behaviour:

SlackProposalHandler replaces rather than composes with
MailProposalHandler (Wave doesn't run SMTP).
The var.promote_traffic toggle is a Wave operations choice, not a
generic feature.

Both could plausibly be upstreamed once stabilised, but Phase 1 isn't the
moment to bikeshed that.

Test plan

CI build passes (mvn compile + tests). Local compile not validated —
host JDK is 8, this is JDK 17.
With SLACK_NOTIFICATIONS_ENABLED unset/false, the application
starts and produceProposalHandler returns the existing
MailProposalHandler / DebugProposalHandler / 501-throwing
handler path (no behavioural change).
With SLACK_NOTIFICATIONS_ENABLED=true but secrets unset, startup
fails fast with a clear message pointing at the rollback knob.
terraform validate of jitgroups-appengine module clean (verified
via the consumer repo's main.tf).

Follow-ups (separate PRs in this repo)

Phase 2: replace the three skeletons' stubs with real Slack/Firestore
calls, add Block Kit message templates, add the
GET /api/.../groups/{name}/reviewers endpoint and selectedReviewers
filter on the submit endpoint, unit tests.
Phase 3: vanilla-JS reviewer-picker UI in
src/main/resources/META-INF/resources/.

🤖 Generated with Claude Code

Adds the DI graph, configuration plumbing, and Terraform toggle for the forthcoming Slack-based approval flow. Ships with SLACK_NOTIFICATIONS_ENABLED defaulting to false so this commit is a no-op at runtime — upstream Mail/ Debug ProposalHandler factory branches remain unchanged when the flag is off. Java - ApplicationConfiguration: SLACK_NOTIFICATIONS_ENABLED, SLACK_BOT_TOKEN_SECRET, SLACK_FIRESTORE_DATABASE env vars + isSlackConfigured() helper. - Application.produceProposalHandler: when the flag is on, fetch the bot token via SecretManagerClient, construct SlackProposalHandler, return. Falls through to existing SMTP/Debug branches otherwise. Fails loudly on Slack init errors (operator likely thinks notifications are wired). - New SlackProposalHandler extends AbstractProposalHandler — Phase 1 overrides log structured "would notify" lines; Phase 2 will add real Block Kit DMs and chat.update sibling propagation. - New SlackClient — async wrapper for users.lookupByEmail / chat.postMessage / chat.update. Phase 1 stubs return successful futures so the DI graph wires up. - New SlackMessageRegistry — Firestore-backed (channel, ts) tracker keyed by sha256(beneficiary, group, sorted recipients). Phase 1 stubs. Build - pom.xml: com.slack.api:slack-api-client 1.45.3, com.google.cloud:google-cloud-firestore 3.31.7. Terraform (jitgroups-appengine module) - New var.promote_traffic (default true). When false, the google_app_engine_service_split_traffic resource is not managed and the newly-deployed version sits at its versioned URL behind IAP without receiving production traffic — the staging mechanism documented in SLACK_INTEGRATION.md. See infrastructure/jit-groups/SLACK_INTEGRATION.md (in wavemm-iam) for the full design, rollback playbook, and phase plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

alex-young

/followup Some small questions/improvement options

alex-young · 2026-04-29T15:25:31Z

+        // Fail loudly: a misconfigured Slack handler is worse than falling
+        // back, because the operator probably believes notifications are
+        // wired. Surfacing this at startup is preferable to silently
+        // dropping every approval request.


I think slack will silently fail if configuration vars aren't set because of the check in ApplicationConfiguration.java - but maybe you're ok with that, and the failure you're worried about is the variables being present but invalid?

Right — silent fall-through was the bug. The intent was "fail loud" only when the secret-fetch itself errored, but a partial misconfig (flag on, secret/database vars missing) skipped the whole branch silently. Fixed in #5: branch on slackNotificationsEnabled directly and throw at startup if either companion var is empty, with a message naming the missing var and the rollback knob. Dropped the isSlackConfigured() helper — its semantics were exactly the silent-fall-through case.

Commit: 5d55c69

alex-young · 2026-04-29T15:26:52Z

+    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);


If SlackMessageRegistry.requestKey took the proposal itself then we wouldn't have to repeat this logic.

Refactored in #5. Pulled the duplicated computation (beneficiary, groupId, expanded reviewer emails, SHA-256 key) into a single fingerprint(Proposal) helper used by both onOperationProposed and onProposalApproved. Removes the drift surface — the two sides have to compute the same key or the lookup misses on approval and siblings never update.

Commit: 5d55c69

alex-young · 2026-04-29T15:30:28Z

+    @NotNull AbstractProposalHandler.Options baseOptions,
+    @NotNull Options slackOptions
+  ) {
+    super(tokenSigner, new Random(), baseOptions);


What is this Random generator used for? Should it be a secure random generator? If it is token nonces then it should be crypto-random

Good catch — fixed in #5 (which builds on top of this). Switched to SecureRandom to match MailProposalHandler and DebugProposalHandler. JWT IDs are activation-token nonces, plain Random would have made them enumeration-vulnerable.

Commit: 5d55c69

alex-young · 2026-04-29T15:31:17Z

+  /**
+   * 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>");
+    }
+  }
+}


This seems like dead code?

Yep — the SlackClient.Options record was leftover scaffolding I forgot to remove when I switched the constructor to take the parameters directly. Already gone in #5 (the file gets a full rewrite to wire up the real com.slack.api calls).

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) <noreply@anthropic.com>

@alex-young

…pdate (#5) * Phase 2a: real Slack + Firestore implementations 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) <noreply@anthropic.com> * Address #4 review feedback from @alex-young 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The fork's CI was disabled before #4 and #5 merged, so this is the first run that actually exercised mvn test. It surfaced three real issues, all narrow: 1. Application.java: runtime.projectId() returns a ProjectId record, not a String. FirestoreOptions.setProjectId expects a String. Use .id() to extract. 2. SlackProposalHandler.java: GroupResolver.expand() is package-private in the upstream com.google.solutions.jitaccess.auth package. The handler in com.google.solutions.jitaccess.web.proposal can't call it across packages. Make expand() public — the class is already public and the method's signature is suitable for external use; the missing modifier looks like an oversight in upstream rather than a deliberate visibility boundary. 3. SlackProposalHandler.java: catch (CompletionException | RuntimeException) is illegal because CompletionException extends RuntimeException — Java's multi-catch forbids related types. Drop CompletionException from the alternatives; RuntimeException covers it. The extracted cause via e.getCause() still unwraps the original SlackApiException / IOException from inside CompletableFuture.join's wrapper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: add workflow_dispatch to enable manual runs Lets us re-trigger the build from the Actions tab without needing a push or PR. Also serves as the first CI run on the wavemm fork — when this PR opens, the workflow fires for the first time, exercising the Phase 1 (#4) and Phase 2a (#5) code that was merged before Actions was enabled on the fork. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: three compile errors caught by first CI run on the fork The fork's CI was disabled before #4 and #5 merged, so this is the first run that actually exercised mvn test. It surfaced three real issues, all narrow: 1. Application.java: runtime.projectId() returns a ProjectId record, not a String. FirestoreOptions.setProjectId expects a String. Use .id() to extract. 2. SlackProposalHandler.java: GroupResolver.expand() is package-private in the upstream com.google.solutions.jitaccess.auth package. The handler in com.google.solutions.jitaccess.web.proposal can't call it across packages. Make expand() public — the class is already public and the method's signature is suitable for external use; the missing modifier looks like an oversight in upstream rather than a deliberate visibility boundary. 3. SlackProposalHandler.java: catch (CompletionException | RuntimeException) is illegal because CompletionException extends RuntimeException — Java's multi-catch forbids related types. Drop CompletionException from the alternatives; RuntimeException covers it. The extracted cause via e.getCause() still unwraps the original SlackApiException / IOException from inside CompletableFuture.join's wrapper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

phosphore force-pushed the slack-integration branch from e31891c to 2412aee Compare April 29, 2026 10:14

phosphore merged commit fc2260d into master Apr 29, 2026

acanizares assigned alex-young Apr 29, 2026

acanizares added the review:pending label Apr 29, 2026

alex-young reviewed Apr 29, 2026

View reviewed changes

acanizares added review:needs-follow-up and removed review:pending labels Apr 29, 2026

phosphore mentioned this pull request Apr 29, 2026

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

Merged

3 tasks

phosphore mentioned this pull request Apr 30, 2026

[i,l,-r] ci: enable manual runs via workflow_dispatch #6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase 1: Slack notifications scaffolding (feature-flagged, inert)#4

Phase 1: Slack notifications scaffolding (feature-flagged, inert)#4
phosphore merged 1 commit into
masterfrom
slack-integration

phosphore commented Apr 29, 2026 •

edited

Loading

Uh oh!

alex-young left a comment

Uh oh!

alex-young Apr 29, 2026

Uh oh!

phosphore Apr 30, 2026

Uh oh!

alex-young Apr 29, 2026

Uh oh!

phosphore Apr 30, 2026

Uh oh!

alex-young Apr 29, 2026

Uh oh!

phosphore Apr 30, 2026

Uh oh!

alex-young Apr 29, 2026

Uh oh!

phosphore Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

phosphore commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's in this PR

Java

Build

Terraform (jitgroups-appengine module)

Why a fork patch and not upstream

Test plan

Follow-ups (separate PRs in this repo)

Uh oh!

alex-young left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phosphore commented Apr 29, 2026 •

edited

Loading