From 9c0c61f3539a8f201cfc81fc1e99a93aa3caa48e Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Thu, 30 Apr 2026 16:02:09 +0200 Subject: [PATCH] fix(slack): @Singleton producer, exclude requester from peers, log root causes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small fixes shaken out of the first end-to-end test on the staged version: 1. produceProposalHandler is now @Singleton Previously @Dependent (CDI default for @Produces with no scope), so every request that needed a ProposalHandler was rebuilding everything the factory constructs — including a fresh Firestore client and its gRPC ManagedChannel. Each abandoned channel surfaced as an ERROR- severity "ManagedChannel allocation site" stack trace in stderr when gRPC's leak detector eventually fired, polluting logs. Singleton- scope means one construction per JVM, no leak. 2. Filter the requester out of the expanded reviewer set AbstractProposalHandler / JoinOperation.propose drops the requester at the *principal* level (group ≠ user), so when the policy ACL names a group that the requester is in (the typical "team approves team" case used by 0-test-mpa-flow), the requester re-appears after GroupResolver.expand. We'd send them a DM about their own request. Drop them with a case-insensitive email match. 3. Log the root cause before re-throwing in onOperationProposed AbstractProposalHandler.propose catches AccessException | IOException and rethrows a generic AccessDeniedException("Creating a proposal failed", e), which surfaces as a 403 in the JIT UI with no detail. The first E2E test hit a Slack 'missing_scope' on users.lookupByEmail — the per-reviewer slack.dm.failed log captured the symptom, but the IOException("Slack DM delivery failed for every reviewer") that propagated up was emitted with no log of its own. Add explicit error logs at the three throw sites: - slack.fingerprint.failed (AccessException from GroupResolver) - slack.noReviewers (zero individuals after expansion) - slack.allDmsFailed (every per-reviewer DM failed) 4. Bump pom version to 2.3.0-wavemm.2 Forces a new App Engine version_id on the next deploy, so the bot token re-fetched from Secret Manager (rotated to add users:read.email scope) actually takes effect. App Engine doesn't allow mutating env vars on an existing version, and our version_id derives from the source ZIP hash — without the bump TF would fall into the same delete-and-recreate-with-same-id 409 trap. Co-Authored-By: Claude Opus 4.7 (1M context) --- sources/pom.xml | 2 +- .../solutions/jitaccess/web/Application.java | 1 + .../web/proposal/SlackProposalHandler.java | 42 ++++++++++++++++++- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/sources/pom.xml b/sources/pom.xml index 7c9142f8..abdad859 100644 --- a/sources/pom.xml +++ b/sources/pom.xml @@ -24,7 +24,7 @@ 4.0.0 com.google.solutions jitaccess - 2.3.0-wavemm.1 + 2.3.0-wavemm.2 3.5.3 3.5.3 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 c6389710..c9be87f8 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 @@ -267,6 +267,7 @@ public GoogleCredentials produceApplicationCredentials() { } @Produces + @Singleton public @NotNull ProposalHandler produceProposalHandler( @NotNull TokenSigner tokenSigner, @NotNull SecretManagerClient secretManagerClient, 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 93e256b4..c9a62353 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 @@ -117,6 +117,13 @@ public SlackProposalHandler( var reviewerEmails = expanded.stream() .filter(EndUserId.class::isInstance) .map(p -> ((EndUserId) p).email) + // Drop the requester from the expanded reviewer set. The upstream + // `JoinOperation.propose` filter excludes the requester at *principal* + // level (group ≠ user), so when the policy ACL names a group that the + // requester also belongs to (the typical "team approves team" pattern), + // the requester sneaks back in after group expansion. Without this we'd + // DM the requester their own approval request. + .filter(email -> !email.equalsIgnoreCase(beneficiary)) .distinct() .sorted() .toList(); @@ -138,8 +145,34 @@ void onOperationProposed( @NotNull ProposalHandler.ProposalToken token, @NotNull URI actionUri ) throws AccessException, IOException { - var fp = fingerprint(proposal); + RegistryFingerprint fp; + try { + fp = fingerprint(proposal); + } + catch (AccessException e) { + // Surface the underlying cause; AbstractProposalHandler will wrap + // this into AccessDeniedException("Creating a proposal failed", e) + // which the user sees as a 403 with no detail. Logging here makes + // sure the actual reason (typically a Cloud Identity Groups API + // permission gap) is visible in Cloud Logging. + this.logger.error( + "slack.fingerprint.failed", + "Resolving reviewers via GroupResolver failed for requester=%s " + + "group=%s — likely a Cloud Identity Groups API permission gap " + + "on the JIT service account.", + proposal.user().email, + proposal.group().toString(), + e); + throw e; + } + if (fp.reviewerEmails().isEmpty()) { + this.logger.error( + "slack.noReviewers", + "Group %s expanded to zero individual users (after excluding the " + + "requester %s). Either the policy ACL is misconfigured or the " + + "approver group has no listable members.", + fp.groupId(), fp.beneficiary()); throw new IOException( "No qualified reviewers resolved to individual users for " + fp.groupId()); } @@ -192,6 +225,13 @@ void onOperationProposed( } if (posted.isEmpty()) { + this.logger.error( + "slack.allDmsFailed", + "Slack DM delivery failed for every one of %d reviewer(s) on %s. " + + "See preceding slack.dm.failed entries for the per-reviewer " + + "cause (typical: missing Slack scope, invalid bot token, or " + + "user not in the workspace).", + fp.reviewerEmails().size(), fp.groupId()); throw new IOException( "Slack DM delivery failed for every reviewer (" + fp.reviewerEmails().size() + ") on " + fp.groupId());