Phase 2a: real Slack DMs + Firestore registry + cross-reviewer chat.update by phosphore · Pull Request #5 · wavemm/jit-groups

phosphore · 2026-04-29T19:56:53Z

Summary

Replaces the Phase 1 stubs (#4) with working implementations so the
SlackProposalHandler actually delivers DMs and tracks them across the
propose/approve flow. Still gated by SLACK_NOTIFICATIONS_ENABLED
(default false in
wavemm-iam),
so this PR alone changes no production behaviour — it just makes the
flag's "on" path do real work.

Full design + decision log:
infrastructure/jit-groups/SLACK_INTEGRATION.md
in the consumer repo.

End-to-end flow this enables (with flag on)

Alice submits an MPA elevation request in JIT.
JIT computes qualified reviewers from the policy ACL, expands any
group:*@roles.wave.com principals to individuals via
CloudIdentityGroupsClient (one round, non-recursive — same as the
rest of JIT), resolves each email to a Slack user via
users.lookupByEmail, and posts a Block Kit DM with an "Approve in
JIT" button.
Bob (one of the reviewers) clicks the button → JIT verifies the JWT
→ Bob clicks Approve in JIT → access is granted.
The handler's onProposalApproved looks up the request in
Firestore, chat.updates every sibling reviewer's DM to "✅ Already
approved by Bob — no action needed" (drops the action button so they
can't double-approve), and DMs Alice "Your PAM elevation was
approved by Bob".
ActivationSelfApproved is intentionally not surfaced on Slack —
the BigQuery audit sink already covers it.

What's in this PR

`SlackClient` (real)

Slack.getInstance().methods(token) wraps the bot token.
lookupUserByEmail / postDirectMessage / updateMessage async via
CompletableFutures.supplyAsync (the project's helper).
users_not_found resolves to null (normal — external collaborators
not in the workspace).
chat.update soft-fails (logs WARN, returns success) — losing a
cosmetic sibling update isn't worth failing approvals.

`SlackMessages` (new file, Block Kit DSL)

Three message shapes: reviewRequest, reviewerSiblingUpdate,
beneficiaryApproved.
escapeBlockquote() neutralises crafted justifications that would
otherwise break out of the >justification block.
reviewerSiblingUpdate intentionally omits the primary button so a
reviewer can't double-approve via a stale Slack message.

`SlackMessageRegistry` (real Firestore)

Targets the named DB pam-slack-bridge (provisioned in wavemm-iam TF).
Document key:
sha256(beneficiary | groupId | sorted recipient emails) — stable
across propose/accept and order-independent on the recipient set
(covered by TestSlackMessageRegistry).
TTL field expires_at aligned with JWT expiry; the TTL policy is
managed in wavemm-iam TF, so the registry self-cleans.

`SlackProposalHandler` (real orchestration)

onOperationProposed: GroupResolver expands group principals,
resolves each user, posts DMs, records the registry. Per-reviewer
failures are aggregated: at least one DM landing keeps the request
viable; zero landed throws so the requester knows.
onProposalApproved: looks up registry, chat.updates every
sibling (skipping the approver themselves), DMs the beneficiary,
deletes the entry. Beneficiary DM still fires on registry miss.
All Slack/Firestore failures logged at WARN, never propagated except
the catastrophic "zero DMs landed".

`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 is
intentional — the Firestore client is never produced for project-wide
use, keeping its IAM blast radius scoped to this factory branch.

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.

What's deferred to Phase 2b (next PR)

GroupsResource: new GET /api/.../groups/{name}/reviewers endpoint
returning candidate reviewers + a "suggested from your team" flag.
GroupsResource.post accepting selectedReviewers[] to narrow the
notification set.
JitGroupContext.JoinOperation.propose overload taking an optional
reviewer filter.
SLACK_COPY_LINK_ENABLED env-flag (default off) for a future "copy
approval link from JIT UI / opt-out of Slack DMs" affordance —
surfaces the JWT URL in the POST response, lets the requester
manually share it instead of relying on automated Slack delivery.

Phase 3 (separate PR after 2b): vanilla-JS picker UI in view.js /
index.html / model.js.

Test plan

mvn test (CI will run; local JDK is 8, can't compile here).
After merge + deploy: with flag still off, JIT behaves identically
to today (MPA throws 501 because SMTP not configured + flag off).
After merge: flip flag on a non-promoted staged version, populate
pam-slack-bot-token, submit MPA → DMs land → approve in JIT →
siblings update + beneficiary confirms.

🤖 Generated with Claude Code

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>

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>

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>

alex-young

/followup

alex-young · 2026-04-30T14:58:09Z

+    // 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<ReviewerMessage>();
+    var failures = new ArrayList<String>();
+
+    for (var email : fp.reviewerEmails()) {
+      try {
+        String userId = this.slackClient.lookupUserByEmail(email).join();


This doesn't look like parallel code - you join() each future in the loop, so I think this is serial?
Is there some kind of awaitAll we could use instead?

alex-young · 2026-04-30T14:58:36Z

+        }
+        SlackClient.PostedMessage message = this.slackClient
+          .postDirectMessage(userId, blocks, fallback)
+          .join();


Same parallel vs serial issue here with .join() in a loop.

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

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


Can you explain this a bit more?

My understanding is that we are will be listing the members of a team group, e.g. security-team@roles.wave.com or similar managed by wavemm-iam. Is the theory here that these rarely update so the chance of an update coinciding with a PAM request is low?

Does this really need to be part of the key? Could we not use the group itself for this? My understanding is that the key is needed to locate the firebase doc corresponding to the request, and that this doc is what then contains the emails/slackIDs/threadIDs. I'm not at all sure of this though!

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

+    // 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"),


This seems odd - should we not expect one stable output?

phosphore and others added 2 commits April 29, 2026 21:54

phosphore merged commit 5928ce3 into master Apr 30, 2026

acanizares assigned alex-young Apr 30, 2026

acanizares added the review:pending label Apr 30, 2026

phosphore mentioned this pull request Apr 30, 2026

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

Merged

alex-young reviewed Apr 30, 2026

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Phase 2a: real Slack DMs + Firestore registry + cross-reviewer chat.update#5
phosphore merged 2 commits into
masterfrom
slack-integration-phase2

phosphore commented Apr 29, 2026

Uh oh!

alex-young left a comment

Uh oh!

alex-young Apr 30, 2026

Uh oh!

alex-young Apr 30, 2026

Uh oh!

alex-young Apr 30, 2026

Uh oh!

alex-young Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

phosphore commented Apr 29, 2026

Summary

End-to-end flow this enables (with flag on)

What's in this PR

SlackClient (real)

SlackMessages (new file, Block Kit DSL)

SlackMessageRegistry (real Firestore)

SlackProposalHandler (real orchestration)

Application.java (factory wiring)

Tests

What's deferred to Phase 2b (next PR)

Test plan

Uh oh!

alex-young left a comment

Choose a reason for hiding this comment

Uh oh!

alex-young Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

alex-young Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

alex-young Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

alex-young Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`SlackClient` (real)

`SlackMessages` (new file, Block Kit DSL)

`SlackMessageRegistry` (real Firestore)

`SlackProposalHandler` (real orchestration)

`Application.java` (factory wiring)