From 855070d30edd4958ab341a9b872b28dccab32151 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 00:41:16 +0200 Subject: [PATCH 1/4] requester visibility: name the auto-selected reviewers (SECOP-1099) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the reviewer set is picked on the requester's behalf (empty picker selection), the requester now learns who was notified: - propose-time Slack DM ('sent to what we think are your closest teammates, since you didn't pick anyone specifically') that also teaches the picker — people watch DMs, not the PAM UI; - the JOIN_PROPOSED response carries notifiedReviewers + reviewersAutoSelected, rendered as a secondary line in the UI. Plumbing: ProposeOptions gains reviewersAutoSelected (back-compat constructor kept); AbstractProposalHandler gets an options-aware onOperationProposed overload defaulting to the legacy hook, so the mail/debug handlers are untouched. --- .../web/proposal/AbstractProposalHandler.java | 20 ++++++- .../web/proposal/ProposalHandler.java | 18 +++++- .../jitaccess/web/proposal/SlackMessages.java | 39 +++++++++++++ .../web/proposal/SlackProposalHandler.java | 48 +++++++++++++++- .../jitaccess/web/rest/GroupsResource.java | 42 +++++++++++--- .../resources/META-INF/resources/index.html | 13 +++++ .../web/proposal/TestSlackMessages.java | 20 +++++++ .../proposal/TestSlackProposalHandler.java | 53 ++++++++++++++++++ .../web/rest/TestGroupsResource.java | 56 +++++++++++++++++++ 9 files changed, 297 insertions(+), 12 deletions(-) diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java index 12aff3be..24302282 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/AbstractProposalHandler.java @@ -68,6 +68,23 @@ abstract void onOperationProposed( @NotNull URI actionUri ) throws AccessException, IOException; + /** + * Options-aware variant (wavemm fork, SECOP-1099). The base + * implementation ignores the options so existing handlers (mail, + * debug) stay unchanged; handlers that care (Slack) override this + * one — e.g. to tell the requester who was auto-notified when the + * reviewer set was picked on their behalf. + */ + void onOperationProposed( + @NotNull JitGroupContext.JoinOperation operation, + @NotNull Proposal proposal, + @NotNull ProposalHandler.ProposalToken token, + @NotNull URI actionUri, + @NotNull ProposalHandler.ProposeOptions options + ) throws AccessException, IOException { + onOperationProposed(operation, proposal, token, actionUri); + } + /** * Notify relevant users about the completion of a proposal. */ @@ -150,7 +167,8 @@ abstract void onProposalApproved( joinOperation, proposal, proposalToken, - buildActionUri.apply(proposalToken.value())); + buildActionUri.apply(proposalToken.value()), + options); } return proposalToken; diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java index 4b8a9738..6da6dee6 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/proposal/ProposalHandler.java @@ -76,13 +76,27 @@ public interface ProposalHandler { * individuals selected by the requester * @param notifyReviewers when false, the proposal token is generated * but no Slack/email notification is delivered + * @param reviewersAutoSelected wavemm fork (SECOP-1099): true when the + * filter was picked by the auto-narrow on + * the requester's behalf (empty picker + * selection) rather than by the requester. + * Handlers use this to tell the requester + * who was notified and to teach the picker. */ record ProposeOptions( @Nullable Set reviewerFilter, - boolean notifyReviewers + boolean notifyReviewers, + boolean reviewersAutoSelected ) { + public ProposeOptions( + @Nullable Set reviewerFilter, + boolean notifyReviewers + ) { + this(reviewerFilter, notifyReviewers, false); + } + public static final @NotNull ProposeOptions DEFAULT = - new ProposeOptions(null, true); + new ProposeOptions(null, true, false); } /** 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 index b55d592a..f24cb1cd 100644 --- 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 @@ -194,6 +194,39 @@ static List reviewerApprovedByYou( .build()); } + /** + * DM to the requester right after an empty-selection propose + * (SECOP-1099): names the auto-selected reviewers and teaches the + * picker. Only sent when the reviewer set was picked on the + * requester's behalf — requesters who picked already know. + */ + static List beneficiaryProposed( + @NotNull String groupId, + @NotNull List notifiedEmails + ) { + var names = String.join(", ", notifiedEmails); + return List.of( + HeaderBlock.builder() + .text(plain(":incoming_envelope: Request sent for approval")) + .build(), + SectionBlock.builder() + .fields(List.of( + markdownField("*Group*", "`" + groupId + "`") + )) + .build(), + SectionBlock.builder() + .text(markdown( + "Since you didn't pick anyone specifically, we sent it to " + + "*what we think are your closest teammates*:\n" + + truncateForBlock(names))) + .build(), + ContextBlock.builder() + .elements(List.of(markdown( + ":bulb: Next time you can choose who reviews it — use the " + + "\"Select reviewers\" picker on the request page."))) + .build()); + } + /** * DM to the beneficiary confirming the activation completed. */ @@ -234,6 +267,12 @@ static String reviewerApprovedByYouFallback(@NotNull String requesterEmail) { return "You approved " + requesterEmail + "'s request — nothing more to do."; } + static String beneficiaryProposedFallback(@NotNull String groupId) { + return String.format( + "Your request for %s was sent to your closest teammates for approval.", + groupId); + } + static String beneficiaryApprovedFallback(@NotNull String groupId, @NotNull String approverEmail) { return String.format("Your PAM elevation for %s was approved by %s.", groupId, approverEmail); } 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 55bc407e..21cd1518 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 @@ -162,6 +162,19 @@ void onOperationProposed( @NotNull Proposal proposal, @NotNull ProposalHandler.ProposalToken token, @NotNull URI actionUri + ) throws AccessException, IOException { + onOperationProposed( + operation, proposal, token, actionUri, + ProposalHandler.ProposeOptions.DEFAULT); + } + + @Override + void onOperationProposed( + @NotNull JitGroupContext.JoinOperation operation, + @NotNull Proposal proposal, + @NotNull ProposalHandler.ProposalToken token, + @NotNull URI actionUri, + @NotNull ProposalHandler.ProposeOptions options ) throws AccessException, IOException { RegistryFingerprint fp; try { @@ -353,11 +366,42 @@ void onOperationProposed( fp.key(), fp.beneficiary(), fp.groupId(), e); } + // + // SECOP-1099: when the reviewer set was picked on the requester's + // behalf (empty picker selection), tell them who we notified — + // people watch Slack, not the PAM UI, and the message doubles as + // the picker explainer. Best-effort: a failure here must not fail + // the propose that already landed reviewer DMs. + // + if (options.reviewersAutoSelected() && !posted.isEmpty()) { + var notifiedEmails = posted.stream() + .map(ReviewerMessage::email) + .sorted() + .toList(); + try { + var beneficiaryUserId = + this.slackClient.lookupUserByEmail(fp.beneficiary()).join(); + if (beneficiaryUserId != null) { + this.slackClient.postDirectMessage( + beneficiaryUserId, + SlackMessages.beneficiaryProposed(fp.groupId(), notifiedEmails), + SlackMessages.beneficiaryProposedFallback(fp.groupId())).join(); + } + } + catch (RuntimeException e) { + var cause = e.getCause() != null ? e.getCause() : e; + this.logger.warn( + "slack.beneficiaryProposedDM.failed", + "Failed to DM requester %s the auto-selected reviewer list for %s: %s", + fp.beneficiary(), fp.groupId(), cause.getMessage()); + } + } + this.logger.info( "slack.onOperationProposed", - "Posted %d/%d Slack DMs for %s requesting %s (key=%s, failures=%s)", + "Posted %d/%d Slack DMs for %s requesting %s (key=%s, autoSelected=%s, failures=%s)", posted.size(), fp.reviewerEmails().size(), fp.beneficiary(), fp.groupId(), - fp.key(), failures); + fp.key(), options.reviewersAutoSelected(), failures); } @Override diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java index 49064af3..aed80c3e 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/GroupsResource.java @@ -229,6 +229,7 @@ public class GroupsResource { // Slack DMs — narrow to the requester's teammates. // Set effectiveReviewers; + boolean reviewersAutoSelected = false; if (selectedReviewers != null && !selectedReviewers.isEmpty()) { effectiveReviewers = selectedReviewers; } @@ -237,6 +238,7 @@ else if (!notifyReviewers) { } else { effectiveReviewers = autoNarrowedReviewers(group, groupId); + reviewersAutoSelected = effectiveReviewers != null; } // @@ -263,7 +265,8 @@ else if (!notifyReviewers) { buildActionUri, new ProposalHandler.ProposeOptions( effectiveReviewers, - notifyReviewers)); + notifyReviewers, + reviewersAutoSelected)); // Plumb the notifyReviewers flag into the audit event so the // log-based BigQuery dashboard distinguishes copy-link @@ -282,12 +285,20 @@ else if (!notifyReviewers) { ? buildActionUri.apply(proposal.value()).toString() : null; + // SECOP-1099: surface who was notified so the requester learns + // the auto-pick happened and that the picker exists. + var notifiedReviewers = effectiveReviewers == null + ? null + : effectiveReviewers.stream().map(u -> u.email).sorted().toList(); + return GroupInfo.create( group, JoinInfo.forProposal( joinOp.input(), approvalUrl, - this.options.slackCopyLinkEnabled())); + this.options.slackCopyLinkEnabled(), + notifiedReviewers, + reviewersAutoSelected)); } else { // @@ -856,7 +867,16 @@ public record JoinInfo( /** Mirrors the SLACK_COPY_LINK_ENABLED env flag. The picker UI uses * this to decide whether to render the "Notify reviewers in Slack" * checkbox and the copy-approval-link affordance. */ - boolean copyLinkEnabled + boolean copyLinkEnabled, + /** SECOP-1099: emails the proposal was sent to — non-null only when + * status is JOIN_PROPOSED and a reviewer filter was applied (picked + * or auto-selected). Null when the proposal went to the raw policy + * ACL. */ + @Nullable List notifiedReviewers, + /** SECOP-1099: true when {@code notifiedReviewers} was picked on the + * requester's behalf by the auto-narrow (empty picker selection) — + * the UI uses this to render the "we picked for you" explainer. */ + boolean reviewersAutoSelected ) { static @NotNull GroupsResource.JoinInfo forJoinAnalysis( @NotNull JitGroupContext g, @@ -897,7 +917,9 @@ else if (joinOp.requiresApproval()) { .map(InputInfo::fromProperty) .toList(), null, - copyLinkEnabled); + copyLinkEnabled, + null, + false); } static @NotNull GroupsResource.JoinInfo forCompletedJoin( @@ -919,19 +941,23 @@ else if (joinOp.requiresApproval()) { .map(InputInfo::fromProperty) .toList(), null, + false, + null, false); } static @NotNull GroupsResource.JoinInfo forProposal( @NotNull List input ) { - return forProposal(input, null, false); + return forProposal(input, null, false, null, false); } static @NotNull GroupsResource.JoinInfo forProposal( @NotNull List input, @Nullable String approvalUrl, - boolean copyLinkEnabled + boolean copyLinkEnabled, + @Nullable List notifiedReviewers, + boolean reviewersAutoSelected ) { return new JoinInfo( JoinStatusInfo.JOIN_PROPOSED, @@ -944,7 +970,9 @@ else if (joinOp.requiresApproval()) { .map(InputInfo::fromProperty) .toList(), approvalUrl, - copyLinkEnabled); + copyLinkEnabled, + notifiedReviewers, + reviewersAutoSelected); } } diff --git a/sources/src/main/resources/META-INF/resources/index.html b/sources/src/main/resources/META-INF/resources/index.html index 8d699eea..84298f87 100644 --- a/sources/src/main/resources/META-INF/resources/index.html +++ b/sources/src/main/resources/META-INF/resources/index.html @@ -962,8 +962,21 @@

Please wait...

}); } else if (groupInfo.join.status == 'JOIN_PROPOSED') { + // SECOP-1099: name the notified reviewers, and when the + // set was auto-picked, teach the picker. + let secondary = undefined; + if (groupInfo.join.notifiedReviewers && + groupInfo.join.notifiedReviewers.length > 0) { + secondary = groupInfo.join.reviewersAutoSelected + ? "Sent to what we think are your closest teammates " + + "(since you didn't pick anyone specifically): " + + groupInfo.join.notifiedReviewers.join(", ") + + " — next time you can choose reviewers with the picker above." + : "Sent to: " + groupInfo.join.notifiedReviewers.join(", "); + } this.membership.addRow({ primary: "Your request to join the group was sent for approval", + secondary: secondary, icon: 'outgoing_mail' }); } 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 index 6155dd7d..fb1eaa2a 100644 --- 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 @@ -179,6 +179,26 @@ public void reviewerApprovedByYou_dropsActionButtonAndNamesRequester() { assertTrue(serialized.contains("You approved")); } + // ------------------------------------------------------------------------- + // beneficiaryProposed — tells the requester who was auto-notified + // (SECOP-1099). + // ------------------------------------------------------------------------- + + @Test + public void beneficiaryProposed_namesReviewersAndTeachesPicker() { + var blocks = SlackMessages.beneficiaryProposed( + "env/sys/grp", + java.util.List.of("bob@example.com", "carol@example.com")); + + var serialized = blocks.toString(); + assertTrue(serialized.contains("bob@example.com")); + assertTrue(serialized.contains("carol@example.com")); + assertTrue(serialized.contains("closest teammates")); + assertTrue(serialized.contains("Select reviewers"), + "the DM must teach the picker"); + assertFalse(blocks.stream().anyMatch(ActionsBlock.class::isInstance)); + } + // ------------------------------------------------------------------------- // beneficiaryApproved — confirms to requester that the elevation landed. // ------------------------------------------------------------------------- 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 index 3297d59c..aedc49f2 100644 --- 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 @@ -379,6 +379,59 @@ public void onProposalApproved_optOutShortCircuitsRegistryLookup() throws Except // Options — fan-out concurrency cap (wavemm fork P1-4) // --------------------------------------------------------------------- + // ------------------------------------------------------------------------- + // Requester visibility on auto-selected reviewers (SECOP-1099). + // ------------------------------------------------------------------------- + + /** + * SECOP-1099: when the reviewer set was auto-picked, the requester + * gets a propose-time DM naming who was notified. When they picked + * themselves (options.DEFAULT), no such DM — they already know. + */ + @Test + public void onOperationProposed_whenAutoSelected_dmsRequesterWithNotifiedList() + throws Exception { + var slack = slackClientHappyPath(); + var registry = registryHappyPath(); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + handler.onOperationProposed( + mock(JitGroupContext.JoinOperation.class), + proposalFor(ALICE, Set.of(BOB, CAROL)), + tokenFor(Set.of(BOB, CAROL)), + ACTION_URI, + new ProposalHandler.ProposeOptions( + Set.of(BOB, CAROL), true, /*reviewersAutoSelected*/ true)); + + // Reviewer DMs to Bob and Carol, plus one to Alice (the requester) + // naming the auto-selected set. + verify(slack).lookupUserByEmail(eq("alice@example.com")); + var fallbacks = ArgumentCaptor.forClass(String.class); + verify(slack, times(3)).postDirectMessage(anyString(), anyList(), fallbacks.capture()); + assertTrue(fallbacks.getAllValues().stream() + .anyMatch(f -> f.contains("closest teammates"))); + } + + @Test + public void onOperationProposed_whenUserPicked_noRequesterDm() + throws Exception { + var slack = slackClientHappyPath(); + var registry = registryHappyPath(); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + handler.onOperationProposed( + mock(JitGroupContext.JoinOperation.class), + proposalFor(ALICE, Set.of(BOB)), + tokenFor(Set.of(BOB)), + ACTION_URI, + new ProposalHandler.ProposeOptions( + Set.of(BOB), true, /*reviewersAutoSelected*/ false)); + + // Only Bob's reviewer DM; the requester is never looked up. + verify(slack, times(1)).postDirectMessage(anyString(), anyList(), anyString()); + verify(slack, never()).lookupUserByEmail(eq("alice@example.com")); + } + // ------------------------------------------------------------------------- // verifyNotConsumed / markConsumed (SECOP-1093). // ------------------------------------------------------------------------- diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java index 5ef16e42..5cc0dd15 100644 --- a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestGroupsResource.java @@ -731,6 +731,62 @@ public void post_whenNoReviewersPickedAndTeammatesExist_narrowsToTeammates() Set.of(teamMate1, teamMate2), captor.getValue().reviewerFilter(), "empty selection on a broad set must narrow to teammates"); + assertTrue(captor.getValue().reviewersAutoSelected(), + "the auto-narrow must flag the selection as made on the requester's behalf"); + } + + /** + * SECOP-1099: the JOIN_PROPOSED response tells the requester who was + * notified and that the choice was auto-made, so the UI can render + * the "we picked for you — use the picker next time" explainer. + */ + @Test + public void post_whenAutoNarrowed_responseCarriesNotifiedReviewers() + throws Exception { + GroupsResource.clearReviewerRateLimiters(); + var teamMate1 = new EndUserId("teammate-1@example.com"); + var acl = new AccessControlList.Builder() + .allow(SAMPLE_USER, PolicyPermission.JOIN.toMask()) + .allow(teamMate1, PolicyPermission.APPROVE_OTHERS.toMask()); + for (int i = 0; i < 20; i++) { + acl.allow(new EndUserId("approver-" + i + "@example.com"), + PolicyPermission.APPROVE_OTHERS.toMask()); + } + var group = Policies.createJitGroupPolicy( + "g-1", + acl.build(), + Map.of(Policy.ConstraintClass.JOIN, List.of(new ExpiryConstraint(Duration.ofMinutes(1))))); + + var resource = new GroupsResource(); + resource.options = new GroupsResource.Options(false); + resource.logger = Mockito.mock(Logger.class); + resource.auditTrail = Mockito.mock(OperationAuditTrail.class); + resource.catalog = createCatalog(group); + resource.subject = Subjects.create(SAMPLE_USER); + resource.executor = Runnable::run; + resource.groupsClient = Mockito.mock(CloudIdentityGroupsClient.class); + resource.proposalHandler = Mockito.mock(ProposalHandler.class); + when(resource.proposalHandler.propose(any(), any(), any())) + .thenReturn(new ProposalHandler.ProposalToken( + "token", Set.of(teamMate1), Instant.MAX)); + + var teamGroup = new GroupId("team@example.com"); + when(resource.groupsClient.listMembershipsByUser(eq(SAMPLE_USER))) + .thenReturn(List.of(new MembershipRelation() + .setGroupKey(new EntityKey().setId(teamGroup.email)))); + when(resource.groupsClient.listMemberships(eq(teamGroup))) + .thenReturn(List.of( + new Membership().setType("user") + .setPreferredMemberKey(new EntityKey().setId(teamMate1.email)))); + + var groupInfo = resource.post( + group.id().environment(), + group.id().system(), + group.id().name(), + new MultivaluedHashMap<>()); + + assertTrue(groupInfo.join().reviewersAutoSelected()); + assertEquals(List.of(teamMate1.email), groupInfo.join().notifiedReviewers()); } /** From 4662f2cab955d0354e3811df7d651310350e3d87 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 00:44:04 +0200 Subject: [PATCH 2/4] double-submit protection for elevation requests (SECOP-1097) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The elevation POST is slow on broad roles (Cloud Identity teammate fan-out + synchronous reviewer DM chains), so an impatient second click used to mint a second proposal token and a duplicate reviewer DM batch (observed live 2026-07-09: two identical 'Posted 15/15' 25s apart). - Frontend: ignore re-entrant submit clicks and disable the button while the POST is in flight. - Backend defense-in-depth: SlackProposalHandler skips the fan-out when a live registry entry already exists for the same (beneficiary, group, recipients) fingerprint — covers multi-tab / post-timeout retries the button guard can't. Fail-open on registry read errors. Not addressed here (bigger change, tracked in SECOP-1097): making the DM fan-out fully async so the POST returns before Slack delivery. --- .../web/proposal/SlackProposalHandler.java | 29 ++++++++++ .../resources/META-INF/resources/index.html | 17 +++++- .../proposal/TestSlackProposalHandler.java | 58 +++++++++++++++++++ 3 files changed, 103 insertions(+), 1 deletion(-) 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 21cd1518..8277cee1 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 @@ -208,6 +208,35 @@ void onOperationProposed( "No qualified reviewers resolved to individual users for " + fp.groupId()); } + // + // SECOP-1097: skip the fan-out if an equivalent request is already + // in flight. The registry key is (beneficiary, group, recipients), + // so a live entry means the same person already has reviewer DMs + // out for the same group+reviewers — a duplicate submit (multi-tab, + // or a retry after a slow first POST). Re-sending would double every + // reviewer's DMs and leave a second approvable token dangling. + // Fail-open: a registry read error falls through to the normal + // fan-out (a duplicate DM batch beats dropping a real request). + // + try { + if (this.registry.lookup(fp.key()).join().isPresent()) { + this.logger.info( + "slack.onOperationProposed.duplicateSkipped", + "A live proposal already exists for %s requesting %s (key=%s); " + + "skipping duplicate reviewer notification.", + fp.beneficiary(), fp.groupId(), fp.key()); + return; + } + } + catch (RuntimeException e) { + var cause = e.getCause() != null ? e.getCause() : e; + this.logger.warn( + "slack.duplicateCheck.failed", + "Duplicate-proposal check failed for %s on %s; proceeding with " + + "notification (fail-open). cause=%s", + fp.beneficiary(), fp.groupId(), cause.getMessage()); + } + var justification = proposal.input().getOrDefault("justification", ""); var blocks = SlackMessages.reviewRequest( diff --git a/sources/src/main/resources/META-INF/resources/index.html b/sources/src/main/resources/META-INF/resources/index.html index 84298f87..d72c155b 100644 --- a/sources/src/main/resources/META-INF/resources/index.html +++ b/sources/src/main/resources/META-INF/resources/index.html @@ -406,7 +406,20 @@

Please wait...

this.formFields = [] this.form = this.select('form'); - this.select('button[type="submit"]').unbind('click').click(async () => { + // SECOP-1097: guard against double-submit. The POST is slow + // on broad roles (Cloud Identity fan-out + reviewer DMs), and + // an impatient second click used to mint a second proposal + // token and a duplicate reviewer DM batch. Ignore re-entrant + // clicks and disable the button while the request is in flight. + this._submitting = false; + const submitButton = this.select('button[type="submit"]'); + submitButton.unbind('click').click(async () => { + if (this._submitting) { + return; + } + this._submitting = true; + submitButton.prop('disabled', true); + var dialog = new WaitDialog(); dialog.showAsync(); @@ -418,6 +431,8 @@

Please wait...

} finally { dialog.close(); + this._submitting = false; + submitButton.prop('disabled', false); } }); } 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 index aedc49f2..4e491518 100644 --- 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 @@ -379,6 +379,64 @@ public void onProposalApproved_optOutShortCircuitsRegistryLookup() throws Except // Options — fan-out concurrency cap (wavemm fork P1-4) // --------------------------------------------------------------------- + // ------------------------------------------------------------------------- + // Duplicate-proposal skip (SECOP-1097). + // ------------------------------------------------------------------------- + + /** + * SECOP-1097: if a live registry entry already exists for the same + * (beneficiary, group, recipients), the fan-out is skipped — no + * duplicate reviewer DMs on a double-submit. + */ + @Test + public void onOperationProposed_whenLiveEntryExists_skipsFanOut() + throws Exception { + var slack = slackClientHappyPath(); + var registry = mock(SlackMessageRegistry.class); + when(registry.requestKey(anyString(), anyString(), anyList())) + .thenReturn("dup-key"); + when(registry.lookup(eq("dup-key"))) + .thenReturn(CompletableFuture.completedFuture(Optional.of(List.of( + new SlackMessageRegistry.ReviewerMessage( + "bob@example.com", "U-BOB", "C-BOB", "111.111"))))); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + var recipients = Set.of(BOB, CAROL); + handler.onOperationProposed( + operationFor(ALICE), proposalFor(ALICE, recipients), + tokenFor(recipients), ACTION_URI); + + // No DMs posted, no new registry write. + verify(slack, never()).postDirectMessage(anyString(), anyList(), anyString()); + verify(registry, never()).record(anyString(), anyList(), any()); + } + + /** + * SECOP-1097 fail-open: a registry read error must not drop a real + * request — the fan-out proceeds. + */ + @Test + public void onOperationProposed_whenDuplicateCheckErrors_proceeds() + throws Exception { + var slack = slackClientHappyPath(); + var registry = mock(SlackMessageRegistry.class); + when(registry.requestKey(anyString(), anyString(), anyList())) + .thenReturn("k"); + when(registry.lookup(eq("k"))) + .thenReturn(CompletableFuture.failedFuture( + new RuntimeException("firestore down"))); + when(registry.record(anyString(), anyList(), any())) + .thenReturn(CompletableFuture.completedFuture(null)); + var handler = newHandler(slack, registry, groupResolverPassthrough()); + + var recipients = Set.of(BOB); + handler.onOperationProposed( + operationFor(ALICE), proposalFor(ALICE, recipients), + tokenFor(recipients), ACTION_URI); + + verify(slack, atLeastOnce()).postDirectMessage(anyString(), anyList(), anyString()); + } + // ------------------------------------------------------------------------- // Requester visibility on auto-selected reviewers (SECOP-1099). // ------------------------------------------------------------------------- From 1a7ca219f8659d60a96ff85aa41c38a974d33bd6 Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 00:47:53 +0200 Subject: [PATCH 3/4] iam: retry SetIamPolicy through new-group member-domain propagation (SECOP-1096) First-ever join of an org-scoped role failed with a 400: JIT lazily creates the Google group, then immediately binds it org-wide, but the iam.allowedPolicyMemberDomains checker can't resolve the seconds-old group yet ('not in permitted organization'). The error previously fell through to NotAuthenticatedException; the user's manual retry succeeded once propagation cleared (2026-07-09, serviceusage-admin). modifyIamPolicy now recognises that transient 400 and retries with backoff (2s/4s/8s, budget separate from the 412 concurrency-control retries so neither starves the other). A genuine domain-restriction denial simply exhausts the bounded retries and surfaces as before. Predicate is deliberately narrow (400 + constraint id / 'not in permitted organization') and unit-tested. --- .../apis/clients/AbstractIamClient.java | 65 +++++++++++++++++ .../apis/clients/TestAbstractIamClient.java | 72 +++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 sources/src/test/java/com/google/solutions/jitaccess/apis/clients/TestAbstractIamClient.java diff --git a/sources/src/main/java/com/google/solutions/jitaccess/apis/clients/AbstractIamClient.java b/sources/src/main/java/com/google/solutions/jitaccess/apis/clients/AbstractIamClient.java index d873502b..4b9f1fbb 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/apis/clients/AbstractIamClient.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/apis/clients/AbstractIamClient.java @@ -41,12 +41,59 @@ public abstract class AbstractIamClient { private static final int MAX_SET_IAM_POLICY_ATTEMPTS = 4; + /** + * Bounded retries for the "member not in permitted organization" + * propagation race (SECOP-1096). Separate budget from the + * concurrency-control attempts so a slow propagation can't starve + * 412 handling. 3 attempts at 2s/4s/8s ≈ 14s covers the lag observed + * in production (a newly-created JIT group took ~15–26s to become + * resolvable by the org-policy member-domain checker). + */ + private static final int MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS = 3; + private static boolean isRoleNotGrantableErrorMessage(@Nullable String message) { return message != null && (message.contains("not supported") || message.contains("does not exist")); } + /** + * True for the transient 400 raised when a binding names a principal + * the {@code iam.allowedPolicyMemberDomains} org-policy checker can't + * yet resolve — the dominant cause being a Google group created + * moments earlier (JIT lazily provisions the group on first join, + * then immediately tries to bind it org-wide). Membership propagation + * clears within tens of seconds, so this is retryable; a genuinely + * disallowed member simply exhausts the bounded retries and surfaces + * as before. Deliberately narrow (both the constraint id AND the + * status) so we never silently swallow a real domain-restriction + * denial as "transient". + */ + static boolean isMemberDomainPropagationError(@NotNull GoogleJsonResponseException e) { + if (e.getStatusCode() != 400) { + return false; + } + var sb = new StringBuilder(); + if (e.getMessage() != null) { + sb.append(e.getMessage()); + } + if (e.getDetails() != null) { + if (e.getDetails().getMessage() != null) { + sb.append('\n').append(e.getDetails().getMessage()); + } + if (e.getDetails().getErrors() != null) { + e.getDetails().getErrors().forEach(err -> { + if (err.getMessage() != null) { + sb.append('\n').append(err.getMessage()); + } + }); + } + } + var haystack = sb.toString().toLowerCase(); + return haystack.contains("allowedpolicymemberdomains") + || haystack.contains("not in permitted organization"); + } + /** * Create a new, API-specific client. */ @@ -99,6 +146,7 @@ public void modifyIamPolicy( // IAM policies use optimistic concurrency control, so we might need to perform // multiple attempts to update the policy. // + int memberDomainRetries = 0; for (int attempt = 0; attempt < MAX_SET_IAM_POLICY_ATTEMPTS; attempt++) { // // Read current version of policy. @@ -148,6 +196,23 @@ public void modifyIamPolicy( catch (InterruptedException ignored) { } } + else if (isMemberDomainPropagationError(e) + && memberDomainRetries < MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS) { + // + // SECOP-1096: a just-provisioned group isn't resolvable by + // the org-policy member-domain checker yet. Back off longer + // than the 412 case (propagation is tens of seconds, not + // milliseconds) and retry. Don't spend a concurrency-control + // attempt on it — decrement so 412 retries stay available. + // + memberDomainRetries++; + try { + Thread.sleep(2000L << (memberDomainRetries - 1)); // 2s, 4s, 8s + } + catch (InterruptedException ignored) { + } + attempt--; + } else { throw (GoogleJsonResponseException) e.fillInStackTrace(); } diff --git a/sources/src/test/java/com/google/solutions/jitaccess/apis/clients/TestAbstractIamClient.java b/sources/src/test/java/com/google/solutions/jitaccess/apis/clients/TestAbstractIamClient.java new file mode 100644 index 00000000..d02430d0 --- /dev/null +++ b/sources/src/test/java/com/google/solutions/jitaccess/apis/clients/TestAbstractIamClient.java @@ -0,0 +1,72 @@ +// +// 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.apis.clients; + +import com.google.api.client.googleapis.json.GoogleJsonError; +import com.google.api.client.googleapis.json.GoogleJsonResponseException; +import com.google.api.client.http.HttpHeaders; +import com.google.api.client.http.HttpResponseException; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Unit tests for {@link AbstractIamClient#isMemberDomainPropagationError} + * (SECOP-1096). The retry loop itself needs a live API and is covered by + * ITestAbstractIamClient; here we pin the classification predicate that + * decides whether a 400 is the transient new-group propagation race. + */ +public class TestAbstractIamClient { + + private static GoogleJsonResponseException error(int status, String detailMessage) { + var builder = new HttpResponseException.Builder(status, "status", new HttpHeaders()); + var details = new GoogleJsonError(); + if (detailMessage != null) { + details.setMessage(detailMessage); + } + return new GoogleJsonResponseException(builder, details); + } + + @Test + public void isMemberDomainPropagationError_whenDomainViolationOn400() { + assertTrue(AbstractIamClient.isMemberDomainPropagationError( + error(400, + "User jit.production.gcp.serviceusage-admin@roles.wave.com is not " + + "in permitted organization."))); + } + + @Test + public void isMemberDomainPropagationError_whenConstraintIdPresent() { + assertTrue(AbstractIamClient.isMemberDomainPropagationError( + error(400, "constraints/iam.allowedPolicyMemberDomains violated"))); + } + + @Test + public void isMemberDomainPropagationError_falseForUnrelated400() { + assertFalse(AbstractIamClient.isMemberDomainPropagationError( + error(400, "Invalid argument: role does not exist"))); + } + + @Test + public void isMemberDomainPropagationError_falseWhenNotA400() { + // Same message shape but a 403 is a real denial, not the race. + assertFalse(AbstractIamClient.isMemberDomainPropagationError( + error(403, "not in permitted organization"))); + } + + @Test + public void isMemberDomainPropagationError_falseWhenNoDetail() { + assertFalse(AbstractIamClient.isMemberDomainPropagationError(error(400, null))); + } +} From e3ef0203a1691a41f3c774a54b215c0cc2d6c54d Mon Sep 17 00:00:00 2001 From: Lorenzo Stella Date: Fri, 10 Jul 2026 00:49:10 +0200 Subject: [PATCH 4/4] build: bump version to 2.3.0-wavemm.11 --- sources/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/pom.xml b/sources/pom.xml index f8a71feb..4fcaed6f 100644 --- a/sources/pom.xml +++ b/sources/pom.xml @@ -24,7 +24,7 @@ 4.0.0 com.google.solutions jitaccess - 2.3.0-wavemm.10 + 2.3.0-wavemm.11 3.5.3 3.5.3