diff --git a/sources/pom.xml b/sources/pom.xml index 4fcaed6f..78a7a24b 100644 --- a/sources/pom.xml +++ b/sources/pom.xml @@ -24,7 +24,7 @@ 4.0.0 com.google.solutions jitaccess - 2.3.0-wavemm.11 + 2.3.0-wavemm.12 3.5.3 3.5.3 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 4b9f1fbb..8cd7dbbd 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 @@ -45,11 +45,14 @@ public abstract class AbstractIamClient { * 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). + * 412 handling. 4 attempts at 2s/4s/8s/16s ≈ 30s (SECOP-1102, up from + * 3/14s) covers the full lag observed in production — a newly-created + * JIT group took ~15–26s to become resolvable by the org-policy + * member-domain checker, and the old 14s budget failed the upper half + * of that range. This path only runs on the first-ever join of a new + * org-scoped role, so the rare worst-case wait is acceptable. */ - private static final int MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS = 3; + private static final int MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS = 4; private static boolean isRoleNotGrantableErrorMessage(@Nullable String message) { @@ -196,8 +199,25 @@ public void modifyIamPolicy( catch (InterruptedException ignored) { } } - else if (isMemberDomainPropagationError(e) - && memberDomainRetries < MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS) { + else if (isMemberDomainPropagationError(e)) { + if (memberDomainRetries >= MAX_MEMBER_DOMAIN_PROPAGATION_ATTEMPTS) { + // + // SECOP-1102: retries exhausted. After ~30s the membership + // still isn't resolvable, so this is almost certainly a + // genuine domain-restriction denial rather than propagation + // lag. Surface it honestly — the pre-existing outer + // 400->401 fallthrough would mislabel it + // NotAuthenticatedException("Not authenticated"), sending + // operators to debug auth/OAuth instead of the org policy. + // + throw new AccessDeniedException(String.format( + "Modifying the IAM policy of '%s' was denied by the " + + "iam.allowedPolicyMemberDomains org policy: a bound " + + "principal is outside the permitted organization " + + "domains. (If this is a newly-created group, its " + + "membership had not propagated after ~30s of retries.)", + fullResourcePath), e); + } // // SECOP-1096: a just-provisioned group isn't resolvable by // the org-policy member-domain checker yet. Back off longer @@ -207,7 +227,7 @@ else if (isMemberDomainPropagationError(e) // memberDomainRetries++; try { - Thread.sleep(2000L << (memberDomainRetries - 1)); // 2s, 4s, 8s + Thread.sleep(2000L << (memberDomainRetries - 1)); // 2s, 4s, 8s, 16s } catch (InterruptedException ignored) { } 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 24302282..b3077879 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 @@ -104,8 +104,9 @@ abstract void onProposalApproved( @NotNull ProposeOptions options ) throws AccessException { + var expiry = Instant.now().plus(this.options.tokenExpiry); var proposal = joinOperation.propose( - Instant.now().plus(this.options.tokenExpiry), + expiry, options.reviewerFilter()); Preconditions.checkArgument( @@ -115,6 +116,27 @@ abstract void onProposalApproved( !proposal.recipients().contains(proposal.user()), "Recipients must not contain the requesting user"); + // + // SECOP-1101: reject duplicate in-flight requests BEFORE minting a + // token. Atomically reserve a pending marker; if one already exists + // (and hasn't expired), this is a re-submit of a request whose + // reviewers were already notified — throw rather than mint a second + // approvable token and fan out a second DM batch. Copy-link + // (notifyReviewers=false) is exempt: it sends no DMs and each call + // legitimately wants its own link. The reservation is released below + // if minting/notification fails, so a genuine retry isn't locked out. + // + boolean reserved = false; + if (options.notifyReviewers()) { + if (!reserveProposal(proposal, options.reviewersAutoSelected(), expiry)) { + throw new AccessDeniedException( + "You already have a pending approval request for this group. " + + "Wait for a reviewer to act on it, or for it to expire, " + + "before submitting another."); + } + reserved = true; + } + // // Encode all inputs into a token and sign it. // @@ -173,12 +195,51 @@ abstract void onProposalApproved( return proposalToken; } - catch (AccessException | IOException e) { + catch (AccessException | IOException | RuntimeException e) { + // SECOP-1101: minting/notification failed after we reserved — + // release the pending marker so a legitimate retry isn't blocked + // for the token lifetime. + if (reserved) { + releaseProposalReservation(proposal, options.reviewersAutoSelected()); + } + if (e instanceof RuntimeException runtime) { + throw runtime; + } throw new AccessDeniedException( "Creating a proposal failed", e); } } + /** + * Wavemm fork (SECOP-1101): atomically reserve a pending-request + * marker before a token is minted, returning {@code false} if a live + * (non-expired) reservation already exists — i.e. this is a duplicate + * submit. Default no-op returning {@code true} (mail/debug don't + * dedupe). The key an implementation uses MUST be stable across the + * retries a requester makes: keyed on (beneficiary, group) when the + * reviewers were auto-selected (the picked set can shift between + * submits), and on (beneficiary, group, recipients) when the + * requester picked reviewers explicitly. + */ + boolean reserveProposal( + @NotNull Proposal proposal, + boolean reviewersAutoSelected, + @NotNull Instant expiry + ) throws AccessException { + return true; + } + + /** + * Wavemm fork (SECOP-1101): release a reservation taken by + * {@link #reserveProposal} when the propose failed after reserving. + * Best-effort; default no-op. + */ + void releaseProposalReservation( + @NotNull Proposal proposal, + boolean reviewersAutoSelected + ) { + } + @SuppressWarnings("unchecked") @Override public @NotNull Proposal accept( 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 6da6dee6..c80fbc90 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 @@ -107,14 +107,14 @@ public ProposeOptions( ) throws AccessException; /** - * Wavemm fork (SECOP-1093): reject proposals whose token was already - * used to approve. Proposal tokens are stateless JWTs — without a - * consumption record, one approval link authorizes unlimited re-grants - * for the token lifetime (~1h), each resetting the membership clock. + * Wavemm fork (SECOP-1093): read-only check that a proposal token + * hasn't been used to approve — used by the GET proposal-view path to + * render "this link has already been used" instead of an approval + * page whose submit would then fail. NOT the enforcement gate; that is + * {@link #claimForApproval} (a read here would be a TOCTOU). * - *

The default is a no-op: handlers without a consumption store - * (mail, debug) keep the upstream replayable semantics. The Slack - * handler enforces via its Firestore registry. + *

Default no-op: handlers without a consumption store (mail, debug) + * keep upstream replayable semantics. * * @throws AccessException when the proposal was already consumed */ @@ -123,13 +123,34 @@ default void verifyNotConsumed(@NotNull Proposal proposal) } /** - * Wavemm fork (SECOP-1093): record that this proposal was used to - * approve, so subsequent {@link #verifyNotConsumed} calls reject it. - * Best-effort by contract — implementations must not fail the - * approval that just succeeded; a missed mark merely restores the - * upstream replayable behaviour for this one token. + * Wavemm fork (SECOP-1100): atomically claim a proposal token for + * approval, so exactly one approval executes even under concurrent + * clicks on the same link. Called immediately BEFORE the approval + * executes; on {@code ALREADY_EXISTS} it throws, so the second + * concurrent (or any later) approver is rejected rather than + * re-granting. Replaces the previous post-execute {@code markConsumed} + * upsert, whose check-then-act window let concurrent approvers all + * pass. + * + *

Default no-op (mail/debug stay replayable). The Slack handler + * fails open on infrastructure errors — availability over + * replay-protection, matching SECOP-1093 — but treats an explicit + * already-claimed result as a hard reject. + * + * @throws AccessException when the token is already claimed/consumed + */ + default void claimForApproval(@NotNull Proposal proposal) + throws AccessException { + } + + /** + * Wavemm fork (SECOP-1100): release a claim taken by + * {@link #claimForApproval} when the approval did not complete (e.g. + * the execute failed before granting), so a legitimate retry can + * approve. Best-effort; a failed release leaves the token burned, + * which is the fail-safe direction. */ - default void markConsumed(@NotNull Proposal proposal) { + default void releaseClaim(@NotNull Proposal proposal) { } /** 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 6ae4a614..6229a88b 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 @@ -57,6 +57,7 @@ public class SlackMessageRegistry { static final String FIELD_REVIEWERS = "reviewers"; static final String FIELD_EXPIRES_AT = "expires_at"; static final String FIELD_CONSUMED = "consumed"; + static final String FIELD_PENDING = "pending"; private final @NotNull Firestore firestore; private final @NotNull Executor executor; @@ -146,6 +147,116 @@ public SlackMessageRegistry( return hmacHex("consumed|" + proposalId); } + /** + * Key of the pending-request reservation (SECOP-1101), used to reject + * duplicate in-flight requests. Namespaced "pending|" (distinct from + * request/consumption docs) and HMAC'd like the others. + * + *

When {@code recipientEmails} is null the key is keyed only on + * (beneficiary, group) — for auto-selected reviewers, whose exact set + * can shift between a requester's re-submits (e.g. presence-ranked), + * so a re-submit is still recognised as the same request. When + * non-null (the requester picked reviewers), the recipients are part + * of the key: deliberately picking a different set is a new request. + */ + public @NotNull String pendingKey( + @NotNull String beneficiary, + @NotNull String groupId, + @org.jetbrains.annotations.Nullable List recipientEmails + ) { + var canonical = new StringBuilder("pending|") + .append(beneficiary).append('|').append(groupId); + if (recipientEmails != null) { + canonical.append('|') + .append(String.join(",", recipientEmails.stream().sorted().toList())); + } + return hmacHex(canonical.toString()); + } + + /** + * Atomically reserve a pending-request marker (SECOP-1101). Returns + * {@code true} if this caller took the reservation, {@code false} if a + * live (non-expired) one already exists. Firestore {@code create()} + * makes concurrent first-time reservations mutually exclusive (fixes + * the lookup/record TOCTOU); an existing-but-EXPIRED marker is treated + * as free and overwritten (fixes the up-to-24h stale-entry lockout, + * since the TTL reaper lags — we compare {@code expires_at} to now + * rather than trusting deletion). + */ + public @NotNull CompletableFuture reservePendingProposal( + @NotNull String pendingKey, + @NotNull Instant expiresAt + ) { + return CompletableFutures.supplyAsync(() -> { + var doc = new HashMap(); + doc.put(FIELD_EXPIRES_AT, Timestamp.ofTimeSecondsAndNanos( + expiresAt.getEpochSecond(), expiresAt.getNano())); + doc.put(FIELD_PENDING, true); + var ref = collection().document(pendingKey); + try { + ref.create(doc).get(); + return true; + } + catch (ExecutionException e) { + if (!isAlreadyExists(e.getCause())) { + throw new RuntimeException( + "Failed to reserve pending proposal " + pendingKey, e); + } + // A marker exists — reuse it only if it's a live request. + try { + var snapshot = ref.get().get(); + var existingExpiry = snapshot.getTimestamp(FIELD_EXPIRES_AT); + var live = existingExpiry != null + && existingExpiry.toDate().toInstant().isAfter(Instant.now()); + if (live) { + return false; + } + // Stale (or malformed) — take it over. + ref.set(doc).get(); + return true; + } + catch (ExecutionException e2) { + throw new RuntimeException( + "Failed to reconcile stale pending reservation " + pendingKey, e2); + } + catch (InterruptedException e2) { + Thread.currentThread().interrupt(); + throw new RuntimeException( + "Interrupted reconciling pending reservation " + pendingKey, e2); + } + } + catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException( + "Interrupted reserving pending proposal " + pendingKey, e); + } + }, this.executor); + } + + /** + * Release a pending reservation (SECOP-1101) so a legitimate retry can + * proceed after a failed propose. Best-effort; TTL reaps otherwise. + */ + public @NotNull CompletableFuture releasePendingProposal( + @NotNull String pendingKey + ) { + return CompletableFutures.supplyAsync(() -> { + try { + collection().document(pendingKey).delete().get(); + } + catch (ExecutionException e) { + this.logger.warn( + "slackRegistry.releasePending.failed", + "Failed to release pending reservation %s (TTL will reap): %s", + pendingKey, e.getMessage()); + } + catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return null; + }, this.executor); + } + private @NotNull String hmacHex(@NotNull String canonical) { try { var mac = Mac.getInstance("HmacSHA256"); @@ -259,13 +370,23 @@ public SlackMessageRegistry( } /** - * Record that a proposal was used to approve (SECOP-1093). The marker - * lives in the same collection as request entries so the existing - * Firestore TTL policy on {@value #FIELD_EXPIRES_AT} reaps it — the - * expiry passed here must be the token expiry, since a marker is only + * Atomically claim a proposal for approval (SECOP-1100). Uses + * Firestore {@code create()}, which fails if the document already + * exists, so exactly one concurrent caller wins the claim — the + * primitive that makes "approve exactly once" hold under simultaneous + * approver clicks, unlike the previous upsert {@code set()} where the + * second writer silently overwrote the first. + * + *

The marker lives in the same collection as request entries so the + * existing Firestore TTL on {@value #FIELD_EXPIRES_AT} reaps it — + * {@code expiresAt} must be the token expiry, since the claim is only * meaningful while the token it blocks is still valid. + * + * @return {@code true} if this caller created the claim, {@code false} + * if it already existed (someone else is approving / has + * approved this token) */ - public @NotNull CompletableFuture markProposalConsumed( + public @NotNull CompletableFuture claimProposalConsumption( @NotNull String consumptionKey, @NotNull Instant expiresAt ) { @@ -276,17 +397,60 @@ public SlackMessageRegistry( expiresAt.getNano())); doc.put(FIELD_CONSUMED, true); try { - collection().document(consumptionKey).set(doc).get(); + collection().document(consumptionKey).create(doc).get(); + return true; } - catch (InterruptedException | ExecutionException e) { + catch (ExecutionException e) { + if (isAlreadyExists(e.getCause())) { + return false; + } + throw new RuntimeException( + "Failed to claim proposal consumption marker " + consumptionKey, e); + } + catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new RuntimeException( - "Failed to write proposal consumption marker " + consumptionKey, e); + "Interrupted claiming proposal consumption marker " + consumptionKey, e); + } + }, this.executor); + } + + /** + * Release a previously-claimed proposal (SECOP-1100), so a legitimate + * retry can approve after the claimed attempt failed before granting. + * Best-effort: if the delete fails the claim stays and the token is + * burned — the fail-safe direction (deny replay over permit it). + */ + public @NotNull CompletableFuture releaseProposalConsumption( + @NotNull String consumptionKey + ) { + return CompletableFutures.supplyAsync(() -> { + try { + collection().document(consumptionKey).delete().get(); + } + catch (ExecutionException e) { + this.logger.warn( + "slackRegistry.releaseClaim.failed", + "Failed to release consumption claim %s; the token stays " + + "burned until TTL reaps it: %s", + consumptionKey, e.getMessage()); + } + catch (InterruptedException e) { + Thread.currentThread().interrupt(); } return null; }, this.executor); } + private static boolean isAlreadyExists(java.lang.Throwable cause) { + if (cause instanceof com.google.api.gax.rpc.AlreadyExistsException) { + return true; + } + return cause != null + && cause.getMessage() != null + && cause.getMessage().toUpperCase().contains("ALREADY_EXISTS"); + } + /** * Remove a request entry once the approval flow is complete. Idempotent. */ 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 f24cb1cd..68288f20 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 @@ -189,8 +189,8 @@ static List reviewerApprovedByYou( .build(), ContextBlock.builder() .elements(List.of(markdown( - ":information_source: Nothing more to do — the requester has " - + "been notified."))) + ":information_source: Nothing more to do — the request is " + + "approved and their access is now active."))) .build()); } 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 8277cee1..8980a877 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,34 +208,11 @@ 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()); - } + // SECOP-1101: duplicate rejection has moved to reserveProposal(), + // which runs BEFORE the token is minted (in AbstractProposalHandler. + // propose) and reserves atomically — replacing the previous + // lookup-then-skip here, which ran after minting (so it left a + // second live token) and was a non-atomic TOCTOU. var justification = proposal.input().getOrDefault("justification", ""); @@ -437,6 +414,30 @@ void onOperationProposed( void onProposalApproved( @NotNull JitGroupContext.ApprovalOperation operation, @NotNull Proposal proposal + ) throws AccessException, IOException { + // + // SECOP-1100: this hook runs INSIDE ApprovalOperation.execute(), + // AFTER the membership has been granted. Anything thrown here would + // propagate out of execute() as a 500 even though access is already + // live — and, worse, would skip the caller's markConsumed/audit, + // leaving the token replayable. Notification is strictly + // post-completion best-effort: swallow everything, log loud. + // + try { + notifyProposalApproved(operation, proposal); + } + catch (Exception e) { + this.logger.error( + "slack.onProposalApproved.failed", + "Post-approval Slack notification failed for group=%s approver=%s; " + + "the approval itself succeeded and is unaffected. cause=%s", + proposal.group(), operation.user().email, e.getMessage()); + } + } + + private void notifyProposalApproved( + @NotNull JitGroupContext.ApprovalOperation operation, + @NotNull Proposal proposal ) throws AccessException, IOException { var fp = fingerprint(proposal); var approverEmail = operation.user().email; @@ -558,20 +559,25 @@ public void verifyNotConsumed(@NotNull Proposal proposal) } /** - * SECOP-1093: record consumption after a successful approval. - * Best-effort by contract — the approval already succeeded, so a - * failed write only restores replayability for this one token; log - * loud and move on. + * SECOP-1100: atomically claim the token before the approval executes. + * The Firestore {@code create()} is the concurrency gate — if two + * approvers click the same link at once, exactly one create succeeds + * and the other is rejected here, before either grants. Fail-open on + * infrastructure errors (availability over replay-protection, same + * tradeoff as the SECOP-1093 read), but an explicit already-claimed + * result is a hard reject. */ @Override - public void markConsumed(@NotNull Proposal proposal) { + public void claimForApproval(@NotNull Proposal proposal) + throws AccessException { var proposalId = proposal.id(); if (proposalId == null) { return; } + boolean claimed; try { - this.registry - .markProposalConsumed( + claimed = this.registry + .claimProposalConsumption( this.registry.consumptionKey(proposalId), proposal.expiry()) .join(); @@ -579,10 +585,104 @@ public void markConsumed(@NotNull Proposal proposal) { catch (RuntimeException e) { var cause = e.getCause() != null ? e.getCause() : e; this.logger.error( - "slack.consumption.markFailed", - "Failed to record proposal consumption for id=%s; this token " - + "remains replayable until it expires at %s. cause=%s", - proposalId, proposal.expiry(), cause.getMessage()); + "slack.consumption.claimFailed", + "Failed to claim proposal for id=%s; allowing the approval " + + "(fail-open) — replay protection degraded until Firestore " + + "recovers. cause=%s", + proposalId, cause.getMessage()); + return; + } + if (!claimed) { + throw new AccessDeniedException( + "This approval link has already been used. If further access is " + + "needed, ask the requester to submit a new request."); + } + } + + /** + * SECOP-1101: atomically reserve a pending-request marker before a + * token is minted. Keyed on (beneficiary, group) for auto-selected + * reviewers (presence/affinity may pick a different set on a + * re-submit, but it's the same request) and additionally on the + * recipient set when the requester picked reviewers. Fail-open on + * infrastructure errors — a Firestore outage must not block + * elevations (a rare duplicate DM batch beats dropping real requests). + */ + @Override + boolean reserveProposal( + @NotNull Proposal proposal, + boolean reviewersAutoSelected, + @NotNull java.time.Instant expiry + ) { + try { + return this.registry + .reservePendingProposal(pendingKeyFor(proposal, reviewersAutoSelected), expiry) + .join(); + } + catch (RuntimeException e) { + var cause = e.getCause() != null ? e.getCause() : e; + this.logger.error( + "slack.reserveProposal.failed", + "Failed to reserve pending request for %s on %s; allowing " + + "(fail-open) — duplicate protection degraded. cause=%s", + proposal.user().email, proposal.group(), cause.getMessage()); + return true; + } + } + + @Override + void releaseProposalReservation( + @NotNull Proposal proposal, + boolean reviewersAutoSelected + ) { + try { + this.registry + .releasePendingProposal(pendingKeyFor(proposal, reviewersAutoSelected)) + .join(); + } + catch (RuntimeException e) { + // Best-effort; TTL reaps. + } + } + + private @NotNull String pendingKeyFor( + @NotNull Proposal proposal, + boolean reviewersAutoSelected + ) { + List recipientEmails = reviewersAutoSelected + ? null + : proposal.recipients().stream() + .filter(EndUserId.class::isInstance) + .map(p -> ((EndUserId) p).email) + .sorted() + .toList(); + return this.registry.pendingKey( + proposal.user().email, proposal.group().toString(), recipientEmails); + } + + /** + * SECOP-1100: release a claim when the approval didn't complete, so a + * legitimate retry can approve. Best-effort — a failed release leaves + * the token burned (fail-safe). + */ + @Override + public void releaseClaim(@NotNull Proposal proposal) { + var proposalId = proposal.id(); + if (proposalId == null) { + return; + } + try { + this.registry + .releaseProposalConsumption(this.registry.consumptionKey(proposalId)) + .join(); + } + catch (RuntimeException e) { + var cause = e.getCause() != null ? e.getCause() : e; + this.logger.warn( + "slack.consumption.releaseFailed", + "Failed to release claim for id=%s; token stays burned until " + + "TTL. cause=%s", + proposalId, cause.getMessage()); } } diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java index 341cf9c1..e752a245 100644 --- a/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java +++ b/sources/src/main/java/com/google/solutions/jitaccess/web/rest/ProposalResource.java @@ -131,14 +131,6 @@ public class ProposalResource { groupId.environment().equals(environment), "The token must match the environment"); - // SECOP-1093: proposal tokens approve exactly once. The check - // runs BEFORE the approval executes; the marker is written right - // after it succeeds. - this.proposalHandler.verifyNotConsumed(proposal); - - // - // Attempt to approve. - // var group = this.catalog .group(proposal.group()) .orElseThrow(() -> NOT_FOUND); @@ -146,9 +138,25 @@ public class ProposalResource { var approveOp = group.approve(proposal); Inputs.copyValues(inputValues, approveOp.input()); - var principal = approveOp.execute(); + // + // SECOP-1100: proposal tokens approve exactly once. Atomically + // claim the token immediately BEFORE executing — a concurrent + // second click loses the claim and is rejected here rather than + // executing a duplicate grant. If the execute fails before + // granting, release the claim so a legitimate retry works. (The + // GET path uses verifyNotConsumed for a friendly banner; the + // claim here is the actual gate.) + // + this.proposalHandler.claimForApproval(proposal); + Principal principal; + try { + principal = approveOp.execute(); + } + catch (Exception e) { + this.proposalHandler.releaseClaim(proposal); + throw e; + } this.auditTrail.joinExecuted(approveOp, principal); - this.proposalHandler.markConsumed(proposal); return ProposalInfo.create( group, diff --git a/sources/src/main/resources/META-INF/resources/index.html b/sources/src/main/resources/META-INF/resources/index.html index d72c155b..0a4bdfbe 100644 --- a/sources/src/main/resources/META-INF/resources/index.html +++ b/sources/src/main/resources/META-INF/resources/index.html @@ -760,8 +760,9 @@

Please wait...

— pick one or more teammates (those tagged team are likely on yours). - Leave all unchecked to notify your closest - qualified teammates automatically. + Leave all unchecked and we'll pick close + teammates for you — or, on small teams, + notify every qualified approver. 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. + // SECOP-1099: name the reviewers this request was sent + // to, and when the set was auto-picked, teach the picker. + // SECOP-1103: phrase as "requested" not "delivered" — + // this list is who we asked; a reviewer who isn't in + // Slack simply won't get a DM, and the Slack DM is the + // delivery-accurate surface. 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 " + + ? "We asked 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(", "); + : "Reviewers requested: " + groupInfo.join.notifiedReviewers.join(", "); } this.membership.addRow({ primary: "Your request to join the group was sent for approval", 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 4e491518..ce295611 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 @@ -37,7 +37,9 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.*; public class TestSlackProposalHandler { @@ -384,57 +386,86 @@ public void onProposalApproved_optOutShortCircuitsRegistryLookup() throws Except // ------------------------------------------------------------------------- /** - * 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. + * SECOP-1101: reserveProposal takes an atomic pending reservation and + * reports duplicates. Auto-selected reviewers key on (beneficiary, + * group) only — recipients are NOT part of the key, so a re-submit + * whose picked set shifted is still recognised as the same request. */ @Test - public void onOperationProposed_whenLiveEntryExists_skipsFanOut() - throws Exception { - var slack = slackClientHappyPath(); + public void reserveProposal_autoSelected_keysOnBeneficiaryAndGroup() { 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()); + when(registry.pendingKey(eq("alice@example.com"), anyString(), isNull())) + .thenReturn("pending-key"); + when(registry.reservePendingProposal(eq("pending-key"), any())) + .thenReturn(CompletableFuture.completedFuture(true)); + var handler = newHandler( + slackClientHappyPath(), registry, groupResolverPassthrough()); - var recipients = Set.of(BOB, CAROL); - handler.onOperationProposed( - operationFor(ALICE), proposalFor(ALICE, recipients), - tokenFor(recipients), ACTION_URI); + var reserved = handler.reserveProposal( + proposalFor(ALICE, Set.of(BOB, CAROL)), + /*reviewersAutoSelected*/ true, + Instant.now().plus(Duration.ofHours(1))); - // No DMs posted, no new registry write. - verify(slack, never()).postDirectMessage(anyString(), anyList(), anyString()); - verify(registry, never()).record(anyString(), anyList(), any()); + assertTrue(reserved); + // recipients omitted from the key (null) for the auto-selected case. + verify(registry).pendingKey(eq("alice@example.com"), anyString(), isNull()); } /** - * SECOP-1097 fail-open: a registry read error must not drop a real - * request — the fan-out proceeds. + * SECOP-1101: a picked reviewer set is part of the key (non-null + * recipient list) — deliberately choosing different reviewers is a + * new request. */ @Test - public void onOperationProposed_whenDuplicateCheckErrors_proceeds() - throws Exception { - var slack = slackClientHappyPath(); + public void reserveProposal_userPicked_keysOnRecipients() { var registry = mock(SlackMessageRegistry.class); - when(registry.requestKey(anyString(), anyString(), anyList())) - .thenReturn("k"); - when(registry.lookup(eq("k"))) + when(registry.pendingKey(eq("alice@example.com"), anyString(), anyList())) + .thenReturn("pending-key"); + when(registry.reservePendingProposal(anyString(), any())) + .thenReturn(CompletableFuture.completedFuture(true)); + var handler = newHandler( + slackClientHappyPath(), registry, groupResolverPassthrough()); + + handler.reserveProposal( + proposalFor(ALICE, Set.of(BOB, CAROL)), + /*reviewersAutoSelected*/ false, + Instant.now().plus(Duration.ofHours(1))); + + verify(registry).pendingKey(eq("alice@example.com"), anyString(), + argThat(list -> list != null && list.size() == 2)); + } + + /** SECOP-1101: a live reservation → duplicate → reserveProposal false. */ + @Test + public void reserveProposal_reportsDuplicate() { + var registry = mock(SlackMessageRegistry.class); + when(registry.pendingKey(anyString(), anyString(), any())) + .thenReturn("pending-key"); + when(registry.reservePendingProposal(anyString(), any())) + .thenReturn(CompletableFuture.completedFuture(false)); + var handler = newHandler( + slackClientHappyPath(), registry, groupResolverPassthrough()); + + assertFalse(handler.reserveProposal( + proposalFor(ALICE, Set.of(BOB)), true, + Instant.now().plus(Duration.ofHours(1)))); + } + + /** SECOP-1101 fail-open: a reservation error must not block the request. */ + @Test + public void reserveProposal_failsOpenOnRegistryError() { + var registry = mock(SlackMessageRegistry.class); + when(registry.pendingKey(anyString(), anyString(), any())) + .thenReturn("pending-key"); + when(registry.reservePendingProposal(anyString(), any())) .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); + var handler = newHandler( + slackClientHappyPath(), registry, groupResolverPassthrough()); - verify(slack, atLeastOnce()).postDirectMessage(anyString(), anyList(), anyString()); + assertTrue(handler.reserveProposal( + proposalFor(ALICE, Set.of(BOB)), true, + Instant.now().plus(Duration.ofHours(1)))); } // ------------------------------------------------------------------------- @@ -558,12 +589,16 @@ public void verifyNotConsumed_failsOpenOnRegistryError() throws Exception { handler.verifyNotConsumed(proposal); // must not throw } + /** + * SECOP-1100: claimForApproval writes an atomic claim with the token + * expiry and returns normally when this caller wins the create. + */ @Test - public void markConsumed_writesMarkerWithTokenExpiry() { + public void claimForApproval_claimsWithTokenExpiry() throws Exception { var registry = mock(SlackMessageRegistry.class); when(registry.consumptionKey(eq("jti-1"))).thenReturn("consumption-key"); - when(registry.markProposalConsumed(anyString(), any())) - .thenReturn(CompletableFuture.completedFuture(null)); + when(registry.claimProposalConsumption(anyString(), any())) + .thenReturn(CompletableFuture.completedFuture(true)); var handler = newHandler( slackClientHappyPath(), registry, groupResolverPassthrough()); @@ -572,16 +607,60 @@ public void markConsumed_writesMarkerWithTokenExpiry() { when(proposal.id()).thenReturn("jti-1"); when(proposal.expiry()).thenReturn(expiry); - handler.markConsumed(proposal); + handler.claimForApproval(proposal); // must not throw when it wins - verify(registry).markProposalConsumed(eq("consumption-key"), eq(expiry)); + verify(registry).claimProposalConsumption(eq("consumption-key"), eq(expiry)); } + /** + * SECOP-1100: losing the atomic claim (create returned false → someone + * else is approving / already approved) is a hard reject. + */ @Test - public void markConsumed_swallowsRegistryErrors() { + public void claimForApproval_rejectsWhenClaimLost() { var registry = mock(SlackMessageRegistry.class); when(registry.consumptionKey(anyString())).thenReturn("consumption-key"); - when(registry.markProposalConsumed(anyString(), any())) + when(registry.claimProposalConsumption(anyString(), any())) + .thenReturn(CompletableFuture.completedFuture(false)); + var handler = newHandler( + slackClientHappyPath(), registry, groupResolverPassthrough()); + + var proposal = proposalFor(ALICE, Set.of(BOB)); + when(proposal.id()).thenReturn("jti-1"); + when(proposal.expiry()).thenReturn(Instant.now().plus(Duration.ofHours(1))); + + assertThrows( + AccessDeniedException.class, + () -> handler.claimForApproval(proposal)); + } + + /** + * SECOP-1100: a Firestore error during the claim fails open — an + * approval must not hard-depend on Firestore (replay protection + * degrades, logged loud). Must not throw. + */ + @Test + public void claimForApproval_failsOpenOnRegistryError() throws Exception { + var registry = mock(SlackMessageRegistry.class); + when(registry.consumptionKey(anyString())).thenReturn("consumption-key"); + when(registry.claimProposalConsumption(anyString(), any())) + .thenReturn(CompletableFuture.failedFuture( + new RuntimeException("firestore unavailable"))); + var handler = newHandler( + slackClientHappyPath(), registry, groupResolverPassthrough()); + + var proposal = proposalFor(ALICE, Set.of(BOB)); + when(proposal.id()).thenReturn("jti-1"); + when(proposal.expiry()).thenReturn(Instant.now().plus(Duration.ofHours(1))); + + handler.claimForApproval(proposal); // fail-open: no throw + } + + @Test + public void releaseClaim_deletesMarkerAndSwallowsErrors() { + var registry = mock(SlackMessageRegistry.class); + when(registry.consumptionKey(eq("jti-1"))).thenReturn("consumption-key"); + when(registry.releaseProposalConsumption(anyString())) .thenReturn(CompletableFuture.failedFuture( new RuntimeException("firestore unavailable"))); var handler = newHandler( @@ -590,7 +669,8 @@ public void markConsumed_swallowsRegistryErrors() { var proposal = proposalFor(ALICE, Set.of(BOB)); when(proposal.id()).thenReturn("jti-1"); - handler.markConsumed(proposal); // must not throw + handler.releaseClaim(proposal); // must not throw + verify(registry).releaseProposalConsumption(eq("consumption-key")); } @Test diff --git a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestProposalResource.java b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestProposalResource.java index 67d43326..9d681f5c 100644 --- a/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestProposalResource.java +++ b/sources/src/test/java/com/google/solutions/jitaccess/web/rest/TestProposalResource.java @@ -509,11 +509,11 @@ public void post_whenApprovalNotAllowed() throws Exception { //--------------------------------------------------------------------------- /** - * SECOP-1093: an already-consumed token must be rejected BEFORE the - * approval executes — no membership, no audit event, no re-mark. + * SECOP-1100: a token whose claim is already held must be rejected + * BEFORE the approval executes — no membership, no audit event. */ @Test - public void post_whenProposalAlreadyConsumed_rejectsBeforeApproving() + public void post_whenProposalAlreadyClaimed_rejectsBeforeApproving() throws Exception { var group = Policies.createJitGroupPolicy( "g-1", @@ -531,7 +531,7 @@ public void post_whenProposalAlreadyConsumed_rejectsBeforeApproving() new EndUserId("other@example.com"), Set.of(SAMPLE_USER)); doThrow(new AccessDeniedException("This approval link has already been used")) - .when(resource.proposalHandler).verifyNotConsumed(any(Proposal.class)); + .when(resource.proposalHandler).claimForApproval(any(Proposal.class)); assertThrows( AccessDeniedException.class, @@ -543,15 +543,14 @@ public void post_whenProposalAlreadyConsumed_rejectsBeforeApproving() verify(resource.auditTrail, never()).joinExecuted( any(JitGroupContext.ApprovalOperation.class), any(Principal.class)); - verify(resource.proposalHandler, never()).markConsumed(any(Proposal.class)); } /** - * SECOP-1093: the happy path checks consumption before approving and - * records consumption only after the approval succeeded. + * SECOP-1100: the happy path CLAIMS before executing (the atomic + * gate), then executes and audits. The claim precedes joinExecuted. */ @Test - public void post_whenApprovalSucceeds_marksProposalConsumed() + public void post_whenApprovalSucceeds_claimsBeforeExecuting() throws Exception { var group = Policies.createJitGroupPolicy( "g-1", @@ -575,13 +574,15 @@ public void post_whenApprovalSucceeds_marksProposalConsumed() new MultivaluedHashMap<>()); var order = inOrder(resource.proposalHandler, resource.auditTrail); - order.verify(resource.proposalHandler).verifyNotConsumed(any(Proposal.class)); + order.verify(resource.proposalHandler).claimForApproval(any(Proposal.class)); order.verify(resource.auditTrail).joinExecuted( any(JitGroupContext.ApprovalOperation.class), any(Principal.class)); - order.verify(resource.proposalHandler).markConsumed(any(Proposal.class)); + // Success path never releases the claim. + verify(resource.proposalHandler, never()).releaseClaim(any(Proposal.class)); } + /** * SECOP-1093: viewing an already-used link fails like an expired one, * instead of rendering an approval page whose submit would then fail.