From d3796a8701cd0e2fe2aadc61ac9d613f15df319d Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:48:22 +0200 Subject: [PATCH 1/3] fix(revoke): reconcile revocation outcomes and fail closed on exit code `grant revoke` printed the raw revocation status without inspecting it and exited 0 on any HTTP 200. A live AWS session returning the undocumented REVOCATION_NOT_APPLICABLE therefore reported success while the session stayed live, and a response missing rows for requested sessions did the same. Outcomes are now reconciled against the *requested* session IDs rather than the returned rows: a requested session with no row is unknown, duplicate rows resolve worst-outcome-wins, and rows for unrequested or empty IDs satisfy nothing. Statuses are classified into an outcome enum (revoked/in_progress/not_applicable/unknown) that fails closed on anything outside the spec's two documented values. Exit 0 only when every requested session was accepted; partial results exit 1 after printing the full per-session breakdown. The request is also chunked to the spec's `sessionIds` cap of 100 per call, so `--all` works on tenants with more than 100 active sessions; a mid-sequence batch error keeps the outcomes already collected. Messaging is provider-neutral: observing REVOCATION_NOT_APPLICABLE proves only that the service declined to act, not why, so no AWS/STS mechanism is claimed. The raw status token is kept in parentheses for grepping. The best-effort "expires in ~Xm" note is a standalone informational clause, never a causal one, and is unavailable in direct mode where grant has no session metadata. JSON gains per-entry outcome/accepted/complete/reason with one entry per requested session, and is emitted before the error so `-o json` stays valid on exit 1. There is deliberately no `revoked` boolean: in-progress means accepted, not finished. --- CHANGELOG.md | 4 + CLAUDE.md | 4 + README.md | 18 +- cmd/output_types.go | 13 +- cmd/revoke.go | 204 ++++++++----- cmd/revoke_batch.go | 48 +++ cmd/revoke_batch_test.go | 174 +++++++++++ cmd/revoke_outcome_test.go | 454 +++++++++++++++++++++++++++++ cmd/revoke_reconcile.go | 148 ++++++++++ cmd/revoke_reconcile_test.go | 236 +++++++++++++++ cmd/revoke_render.go | 129 ++++++++ cmd/revoke_test.go | 42 ++- cmd/status.go | 7 +- cmd/test_helpers.go | 15 + cmd/test_mocks.go | 6 + internal/sca/models/revoke.go | 58 +++- internal/sca/models/revoke_test.go | 43 +++ internal/sca/service_test.go | 28 ++ 18 files changed, 1549 insertions(+), 82 deletions(-) create mode 100644 cmd/revoke_batch.go create mode 100644 cmd/revoke_batch_test.go create mode 100644 cmd/revoke_outcome_test.go create mode 100644 cmd/revoke_reconcile.go create mode 100644 cmd/revoke_reconcile_test.go create mode 100644 cmd/revoke_render.go diff --git a/CHANGELOG.md b/CHANGELOG.md index de699d0..bef63fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ All notable changes to this project will be documented in this file. - End-to-end self-update tests that replace a real, running binary (`internal/selfupdate/e2e_test.go`, build tag `selfupdate_e2e`). They compile two fixture binaries from a dependency-free module, execute one, and swap it through grant's own apply path while a process is still running from that image — so the Windows file-locking semantics behind the two-rename swap are actually exercised, not just the bookkeeping. Success and rollback paths are both covered, and the rolled-back binary is asserted to still run. No network access is required - CI runs the new self-update end-to-end tests on **both** `ubuntu-latest` and `windows-latest`, closing the gap left by the Windows CI leg added in 0.8.0, which only ran `go build` and `go test` and never exercised a binary replacing itself on Windows +### Fixed + +- `grant revoke` now exits 1 when any requested session was not revoked instead of reporting success; check scripts relying on exit 0. + ## [0.8.0] - 2026-08-14 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index b106ab7..e575be5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,6 +40,10 @@ Custom `SCAAccessService` follows SDK conventions: - `POST /api/access/elevate` — request JIT elevation (AWS responses include `accessCredentials` JSON string) - `GET /api/access/sessions` — list active sessions - `POST /api/access/sessions/revoke` — revoke sessions by ID (request: `sessionIds[]`, response: `SessionRevocationInfo[]`) + - `sessionIds` is capped at `maxItems: 100` per request, so `cmd/revoke_batch.go` chunks the requested set into sequential ≤100-ID calls and aggregates; a mid-sequence batch error keeps the outcomes already collected + - `revocationStatus`: the spec enum is only `SUCCESSFULLY_REVOKED` and `REVOCATION_IN_PROGRESS`. The live API also returns the **undocumented** `REVOCATION_NOT_APPLICABLE` (in neither the spec nor the SDK), so the status set is open and `ClassifyRevocationStatus` (`internal/sca/models/revoke.go`) **fails closed** — anything unrecognized, including `""`, is `OutcomeUnknown` and counts as a failure. Match is exact; case variants are unknown + - Outcomes are reconciled against the **requested** session IDs, never the returned rows (`cmd/revoke_reconcile.go`): a requested session with no row is `unknown`, duplicate rows resolve worst-outcome-wins, and rows for unrequested or empty IDs satisfy nothing. Exit 0 only when every requested session was accepted (`revoked` or `in_progress`); partial → exit 1 + - Never attribute a cause for `REVOCATION_NOT_APPLICABLE` (e.g. an AWS/STS story). Observing the status does not prove the reason, and grant supports Azure/AWS/GCP plus group sessions. Render provider-neutral text and keep the raw token - `GET /api/access/{CSP}/eligibility/groups` — list eligible Entra ID groups (response: `groupId`/`groupName`/`directoryId`) - `POST /api/access/elevate/groups` — request group membership elevation (response wrapped in `response` key, same as cloud elevation) - **Headers:** `Authorization: Bearer {jwt}`, `X-API-Version: 2.0`, `Content-Type: application/json` diff --git a/README.md b/README.md index ee4e54a..75b5f8a 100644 --- a/README.md +++ b/README.md @@ -118,11 +118,26 @@ Running `grant` with no subcommand elevates cloud permissions (the core behavior | `logout` | Clear cached tokens from keyring | | `status` | Show auth state and active sessions | | `favorites` | Manage saved role favorites (`add`/`list`/`remove`) | -| `revoke` | Revoke sessions (interactive, by ID, or `--all`) | +| `revoke` | Revoke sessions (interactive, by ID, or `--all`) — see exit codes below | | `request` | Manage access requests through an approval workflow (see subcommands below) | | `update` | Self-update to the latest release from GitHub | | `version` | Print version information | +### `grant revoke` exit codes + +`revoke` reports its outcome against the sessions you **requested**, not against +whatever rows the service happened to return. + +| Exit | Meaning | +|------|---------| +| 0 | Every requested session was **accepted** for revocation. Some may still be in progress — accepted is not the same as finished, and `revoke` says which is which per session. | +| 1 | At least one requested session was not accepted, or the service returned no result for it. The full per-session breakdown is printed before the command exits. | + +A partial result exits 1 on purpose, so `grant revoke --all && echo safe` cannot +print `safe` while a session survives. With `--output json` the per-session +outcome (`revoked`, `in_progress`, `not_applicable`, `unknown`) plus the +`accepted` and `complete` flags are emitted on stdout even on exit 1. + ### `grant request` subcommands | Subcommand | Description | @@ -183,6 +198,7 @@ favorites: | "No eligible targets found" | Verify SCA policies with your Idira admin; try without `--provider` to see all targets | | "Failed to elevate" | Check `grant status` for active sessions; verify target/role names | | `grant env` errors for Azure/GCP | `env` is AWS-only — Azure and GCP return no credentials, use `grant` directly | +| `grant revoke` reports "NOT revoked" | The service returned a status other than a successful or in-progress revocation (the raw value is shown in parentheses, e.g. `REVOCATION_NOT_APPLICABLE`), or returned no result for that session. The session may still be active — check `grant status` and raise the raw status with your Idira admin | | Permission denied accessing keyring (Linux) | Install and start `gnome-keyring` or `kwalletmanager` | ## Development diff --git a/cmd/output_types.go b/cmd/output_types.go index 77fea39..79a9a75 100644 --- a/cmd/output_types.go +++ b/cmd/output_types.go @@ -49,9 +49,18 @@ type statusOutput struct { } // revocationOutput is the JSON representation of a revocation result. +// There is one entry per *requested* session, in requested order, plus any +// results the service returned that could not be attributed to a request. +// There is deliberately no "revoked" boolean: an in-progress revocation is +// accepted but not complete, and a boolean cannot say that. type revocationOutput struct { - SessionID string `json:"sessionId"` - Status string `json:"status"` + SessionID string `json:"sessionId"` + Status string `json:"status"` // raw API value; "" when no row was returned + Outcome string `json:"outcome"` // revoked | in_progress | not_applicable | unknown + Accepted bool `json:"accepted"` // the service accepted the command + Complete bool `json:"complete"` // revocation confirmed finished + Reason string `json:"reason,omitempty"` // explanation when not confirmed revoked + Unexpected bool `json:"unexpected,omitempty"` } // favoriteOutput is the JSON representation of a saved favorite. diff --git a/cmd/revoke.go b/cmd/revoke.go index 0238faa..f26c108 100644 --- a/cmd/revoke.go +++ b/cmd/revoke.go @@ -4,7 +4,9 @@ import ( "context" "errors" "fmt" + "time" + "github.com/aaearon/grant-cli/internal/cache" "github.com/aaearon/grant-cli/internal/config" scamodels "github.com/aaearon/grant-cli/internal/sca/models" "github.com/aaearon/grant-cli/internal/ui" @@ -12,6 +14,11 @@ import ( "github.com/spf13/cobra" ) +// errRevocationIncomplete is returned when at least one requested session was +// not accepted for revocation. grant fails closed: a security-remediation +// command must not exit 0 while access may still be live. +var errRevocationIncomplete = errors.New("not all requested sessions were revoked") + // uiSessionSelector wraps ui.SelectSessions to implement sessionSelector type uiSessionSelector struct{} @@ -66,7 +73,13 @@ func NewRevokeCommand() *cobra.Command { cachedLister := buildCachedLister(cfg, false, svc, nil) - return runRevoke(cmd, args, ispAuth, svc, cachedLister, svc, &uiSessionSelector{}, &uiConfirmPrompter{}, profile) + // Session timestamp tracker for the best-effort expiry note (may be nil). + var tracker *cache.Store + if cacheDir, err := cache.CacheDir(); err == nil { + tracker = cache.NewStore(cacheDir, 25*time.Hour) + } + + return runRevoke(cmd, args, ispAuth, svc, cachedLister, svc, &uiSessionSelector{}, &uiConfirmPrompter{}, profile, tracker, time.Now) }) } @@ -78,9 +91,25 @@ func NewRevokeCommandWithDeps( revoker sessionRevoker, selector sessionSelector, confirmer confirmPrompter, +) *cobra.Command { + return newRevokeCommandWithClock(auth, lister, elig, revoker, selector, confirmer, nil, time.Now) +} + +// newRevokeCommandWithClock is NewRevokeCommandWithDeps plus a session +// timestamp tracker and an injectable clock, so expiry notes are deterministic +// in tests. +func newRevokeCommandWithClock( + auth authLoader, + lister sessionLister, + elig eligibilityLister, + revoker sessionRevoker, + selector sessionSelector, + confirmer confirmPrompter, + tracker *cache.Store, + now func() time.Time, ) *cobra.Command { return newRevokeCommand(func(cmd *cobra.Command, args []string) error { - return runRevoke(cmd, args, auth, lister, elig, revoker, selector, confirmer, nil) + return runRevoke(cmd, args, auth, lister, elig, revoker, selector, confirmer, nil, tracker, now) }) } @@ -94,6 +123,8 @@ func runRevoke( selector sessionSelector, confirmer confirmPrompter, profile *sdkmodels.IdsecProfile, + tracker *cache.Store, + now func() time.Time, ) error { allFlag, _ := cmd.Flags().GetBool("all") yesFlag, _ := cmd.Flags().GetBool("yes") @@ -123,94 +154,119 @@ func runRevoke( return fmt.Errorf("not authenticated, run 'grant login' first: %w", err) } - // Determine session IDs to revoke - var sessionIDs []string + // Determine which sessions to revoke. metadata stays empty in direct mode, + // where grant only has bare session IDs. + sessionIDs, metadata, done, err := resolveRevokeTargets(cmd, args, lister, elig, selector, confirmer, cspFilter, allFlag, yesFlag) + if err != nil || done { + return err + } + + // The requested set is the source of truth for every count and for the exit + // code, so deduplicate it before sending and before reconciling. + sessionIDs = dedupeSessionIDs(sessionIDs) + if len(sessionIDs) == 0 { + fmt.Fprintln(cmd.OutOrStdout(), "No sessions selected.") + return nil + } - if len(args) > 0 { - // Direct mode: session IDs provided as arguments - sessionIDs = args - } else { - // All or interactive mode: need to list sessions first - ctx, cancel := context.WithTimeout(context.Background(), apiTimeout) - defer cancel() + // A failing batch still returns the results already collected. + results, revokeErr := revokeInBatches(context.Background(), revoker, sessionIDs) - sessions, err := lister.ListSessions(ctx, cspFilter) - if err != nil { - return fmt.Errorf("failed to list sessions: %w", err) - } + records, unattached := reconcileRevocations(sessionIDs, results) - if len(sessions.Response) == 0 { - fmt.Fprintln(cmd.OutOrStdout(), "No active sessions to revoke.") - return nil + if isJSONOutput() { + if err := writeJSON(cmd.OutOrStdout(), buildRevocationJSON(records, unattached)); err != nil { + return err } + } else { + // Always print the full breakdown before returning an error, so a + // non-zero exit is never opaque. + renderRevocationResults(cmd.OutOrStdout(), records, unattached, expiryHinter{ + metadata: metadata, + timestamps: sessionTimestamps(tracker), + now: now, + }) + } - if allFlag { - // Collect all session IDs - for _, s := range sessions.Response { - sessionIDs = append(sessionIDs, s.SessionID) - } - - // Confirm unless --yes - if !yesFlag { - confirmed, err := confirmer.ConfirmRevocation(len(sessionIDs)) - if err != nil { - return fmt.Errorf("confirmation failed: %w", err) - } - if !confirmed { - fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.") - return nil - } - } - } else { - // Interactive mode - nameMap := buildWorkspaceNameMap(ctx, elig, sessions.Response) - - selected, err := selector.SelectSessions(sessions.Response, nameMap) - if err != nil { - return fmt.Errorf("session selection failed: %w", err) - } - - for _, s := range selected { - sessionIDs = append(sessionIDs, s.SessionID) - } - - // Confirm - if !yesFlag { - confirmed, err := confirmer.ConfirmRevocation(len(sessionIDs)) - if err != nil { - return fmt.Errorf("confirmation failed: %w", err) - } - if !confirmed { - fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.") - return nil - } - } - } + if revokeErr != nil { + return revokeErr } - // Call revoke API + if summary := summarizeRevocations(records); !summary.allAccepted() { + return fmt.Errorf("%w: %s", errRevocationIncomplete, summaryLine(summary)) + } + + return nil +} + +// sessionTimestamps reads local elevation timestamps, tolerating a nil tracker. +func sessionTimestamps(tracker *cache.Store) map[string]time.Time { + if tracker == nil { + return nil + } + return cache.SessionTimestamps(tracker) +} + +// resolveRevokeTargets determines the session IDs to revoke, along with session +// metadata when grant listed the sessions itself. done reports that the command +// has already finished (nothing to revoke, or the user declined). +func resolveRevokeTargets( + cmd *cobra.Command, + args []string, + lister sessionLister, + elig eligibilityLister, + selector sessionSelector, + confirmer confirmPrompter, + cspFilter *scamodels.CSP, + allFlag, yesFlag bool, +) (sessionIDs []string, metadata map[string]scamodels.SessionInfo, done bool, err error) { + if len(args) > 0 { + // Direct mode: session IDs provided as arguments, no metadata available. + return args, nil, false, nil + } + + // All or interactive mode: list sessions first. ctx, cancel := context.WithTimeout(context.Background(), apiTimeout) defer cancel() - result, err := revoker.RevokeSessions(ctx, &scamodels.RevokeRequest{ - SessionIDs: sessionIDs, - }) + sessions, err := lister.ListSessions(ctx, cspFilter) if err != nil { - return err + return nil, nil, true, fmt.Errorf("failed to list sessions: %w", err) } - // Display results - if isJSONOutput() { - out := make([]revocationOutput, len(result.Response)) - for i, r := range result.Response { - out[i] = revocationOutput{SessionID: r.SessionID, Status: r.RevocationStatus} + if len(sessions.Response) == 0 { + fmt.Fprintln(cmd.OutOrStdout(), "No active sessions to revoke.") + return nil, nil, true, nil + } + + metadata = make(map[string]scamodels.SessionInfo, len(sessions.Response)) + for _, s := range sessions.Response { + metadata[s.SessionID] = s + } + + selected := sessions.Response + if !allFlag { + nameMap := buildWorkspaceNameMap(ctx, elig, sessions.Response) + selected, err = selector.SelectSessions(sessions.Response, nameMap) + if err != nil { + return nil, nil, true, fmt.Errorf("session selection failed: %w", err) } - return writeJSON(cmd.OutOrStdout(), out) } - for _, r := range result.Response { - fmt.Fprintf(cmd.OutOrStdout(), " %s: %s\n", r.SessionID, r.RevocationStatus) + for _, s := range selected { + sessionIDs = append(sessionIDs, s.SessionID) } - return nil + if !yesFlag { + confirmed, cerr := confirmer.ConfirmRevocation(len(sessionIDs)) + if cerr != nil { + return nil, nil, true, fmt.Errorf("confirmation failed: %w", cerr) + } + if !confirmed { + fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.") + return nil, nil, true, nil + } + } + + return sessionIDs, metadata, false, nil } diff --git a/cmd/revoke_batch.go b/cmd/revoke_batch.go new file mode 100644 index 0000000..96dfeff --- /dev/null +++ b/cmd/revoke_batch.go @@ -0,0 +1,48 @@ +package cmd + +import ( + "context" + + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +// chunkSessionIDs splits ids into consecutive slices of at most size, +// preserving order. +func chunkSessionIDs(ids []string, size int) [][]string { + if len(ids) == 0 || size <= 0 { + return nil + } + chunks := make([][]string, 0, (len(ids)+size-1)/size) + for start := 0; start < len(ids); start += size { + end := start + size + if end > len(ids) { + end = len(ids) + } + chunks = append(chunks, ids[start:end]) + } + return chunks +} + +// revokeInBatches revokes ids in sequential batches of at most +// scamodels.MaxRevokeBatchSize, the API's cap on the request body. +// +// On a batch failure it returns the results already collected together with the +// error, so outcomes from earlier batches are never discarded: aborting +// silently would under-report revocations that actually happened. +func revokeInBatches(ctx context.Context, revoker sessionRevoker, ids []string) ([]scamodels.RevocationResult, error) { + var results []scamodels.RevocationResult + + for _, chunk := range chunkSessionIDs(ids, scamodels.MaxRevokeBatchSize) { + batchCtx, cancel := context.WithTimeout(ctx, apiTimeout) + resp, err := revoker.RevokeSessions(batchCtx, &scamodels.RevokeRequest{SessionIDs: chunk}) + cancel() + if err != nil { + return results, err + } + if resp != nil { + results = append(results, resp.Response...) + } + } + + return results, nil +} diff --git a/cmd/revoke_batch_test.go b/cmd/revoke_batch_test.go new file mode 100644 index 0000000..88f7039 --- /dev/null +++ b/cmd/revoke_batch_test.go @@ -0,0 +1,174 @@ +// NOTE: Do not use t.Parallel() in cmd/ tests due to package-level state +// (verbose, passedArgValidation) that is mutated during test execution. +package cmd + +import ( + "context" + "errors" + "fmt" + "testing" + + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +func makeSessionIDs(n int) []string { + ids := make([]string, n) + for i := range ids { + ids[i] = fmt.Sprintf("s%03d", i) + } + return ids +} + +func TestChunkSessionIDs(t *testing.T) { + tests := []struct { + name string + count int + wantChunks []int + }{ + {"empty", 0, nil}, + {"single", 1, []int{1}}, + {"exactly one full batch", 100, []int{100}}, + {"one over a batch", 101, []int{100, 1}}, + {"two and a half batches", 250, []int{100, 100, 50}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ids := makeSessionIDs(tt.count) + chunks := chunkSessionIDs(ids, scamodels.MaxRevokeBatchSize) + + if len(chunks) != len(tt.wantChunks) { + t.Fatalf("got %d chunks, want %d", len(chunks), len(tt.wantChunks)) + } + + var flat []string + for i, c := range chunks { + if len(c) != tt.wantChunks[i] { + t.Errorf("chunk[%d] size = %d, want %d", i, len(c), tt.wantChunks[i]) + } + if len(c) > scamodels.MaxRevokeBatchSize { + t.Errorf("chunk[%d] exceeds the API limit: %d", i, len(c)) + } + flat = append(flat, c...) + } + + if len(flat) != len(ids) { + t.Fatalf("flattened chunks have %d IDs, want %d", len(flat), len(ids)) + } + for i := range ids { + if flat[i] != ids[i] { + t.Fatalf("order not preserved at %d: got %q, want %q", i, flat[i], ids[i]) + } + } + }) + } +} + +func TestRevokeInBatches_AggregatesAcrossChunks(t *testing.T) { + ids := makeSessionIDs(250) + + revoker := &mockSessionRevoker{ + revokeFunc: func(ctx context.Context, req *scamodels.RevokeRequest) (*scamodels.RevokeResponse, error) { + results := make([]scamodels.RevocationResult, len(req.SessionIDs)) + for i, id := range req.SessionIDs { + results[i] = scamodels.RevocationResult{SessionID: id, RevocationStatus: scamodels.RevocationSuccessful} + } + return &scamodels.RevokeResponse{Response: results}, nil + }, + } + + results, err := revokeInBatches(t.Context(), revoker, ids) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(revoker.calls) != 3 { + t.Fatalf("got %d revoke calls, want 3", len(revoker.calls)) + } + for i, call := range revoker.calls { + if len(call) > scamodels.MaxRevokeBatchSize { + t.Errorf("call[%d] sent %d IDs, exceeding the API limit", i, len(call)) + } + } + if len(results) != 250 { + t.Fatalf("got %d results, want 250", len(results)) + } + for i, r := range results { + if r.SessionID != ids[i] { + t.Fatalf("result[%d] = %q, want %q", i, r.SessionID, ids[i]) + } + } +} + +func TestRevokeInBatches_SingleBatch(t *testing.T) { + revoker := &mockSessionRevoker{ + response: &scamodels.RevokeResponse{Response: []scamodels.RevocationResult{ + {SessionID: "s000", RevocationStatus: scamodels.RevocationSuccessful}, + }}, + } + + results, err := revokeInBatches(t.Context(), revoker, makeSessionIDs(1)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(revoker.calls) != 1 { + t.Fatalf("got %d revoke calls, want 1", len(revoker.calls)) + } + if len(results) != 1 { + t.Fatalf("got %d results, want 1", len(results)) + } +} + +func TestRevokeInBatches_ErrorMidSequenceKeepsEarlierResults(t *testing.T) { + ids := makeSessionIDs(250) + boom := errors.New("service unavailable") + + call := 0 + revoker := &mockSessionRevoker{ + revokeFunc: func(ctx context.Context, req *scamodels.RevokeRequest) (*scamodels.RevokeResponse, error) { + call++ + if call == 2 { + return nil, boom + } + results := make([]scamodels.RevocationResult, len(req.SessionIDs)) + for i, id := range req.SessionIDs { + results[i] = scamodels.RevocationResult{SessionID: id, RevocationStatus: scamodels.RevocationSuccessful} + } + return &scamodels.RevokeResponse{Response: results}, nil + }, + } + + results, err := revokeInBatches(t.Context(), revoker, ids) + if err == nil { + t.Fatal("expected an error from the failing batch") + } + if !errors.Is(err, boom) { + t.Errorf("error = %v, want it to wrap %v", err, boom) + } + // Chunk 1's outcomes must survive; aborting silently would under-report + // revocations that actually happened. + if len(results) != 100 { + t.Fatalf("got %d results, want the 100 from the first batch", len(results)) + } + if call != 2 { + t.Errorf("made %d calls, want 2 (stop after the failure)", call) + } +} + +func TestRevokeInBatches_NilResponseIsNotASuccess(t *testing.T) { + revoker := &mockSessionRevoker{} + + results, err := revokeInBatches(t.Context(), revoker, makeSessionIDs(2)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(results) != 0 { + t.Fatalf("got %d results, want 0", len(results)) + } + + // Reconciliation must then report both requested sessions as unknown. + records, _ := reconcileRevocations(makeSessionIDs(2), results) + if summarizeRevocations(records).allAccepted() { + t.Error("a nil response must not be reconciled as success") + } +} diff --git a/cmd/revoke_outcome_test.go b/cmd/revoke_outcome_test.go new file mode 100644 index 0000000..3ecf3a3 --- /dev/null +++ b/cmd/revoke_outcome_test.go @@ -0,0 +1,454 @@ +// NOTE: Do not use t.Parallel() in cmd/ tests due to package-level state +// (verbose, passedArgValidation) that is mutated during test execution. +package cmd + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "strings" + "testing" + "time" + + "github.com/aaearon/grant-cli/internal/cache" + scamodels "github.com/aaearon/grant-cli/internal/sca/models" + authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth" + commonmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/common" +) + +func testAuthLoader() *mockAuthLoader { + expiresIn := commonmodels.IdsecRFC3339Time(time.Now().Add(1 * time.Hour)) + return &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user@example.com", ExpiresIn: expiresIn}} +} + +func revokeResponse(pairs ...string) *scamodels.RevokeResponse { + results := make([]scamodels.RevocationResult, 0, len(pairs)/2) + for i := 0; i+1 < len(pairs); i += 2 { + results = append(results, scamodels.RevocationResult{SessionID: pairs[i], RevocationStatus: pairs[i+1]}) + } + return &scamodels.RevokeResponse{Response: results} +} + +// TestRevokeCommand_OutcomeClassification covers the exit-code contract: +// exit 0 only when every *requested* session was accepted by the service. +func TestRevokeCommand_OutcomeClassification(t *testing.T) { + tests := []struct { + name string + args []string + response *scamodels.RevokeResponse + wantErr bool + wantContain []string + wantNotContain []string + }{ + { + name: "all revoked exits zero", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful, "s2", scamodels.RevocationSuccessful), + wantErr: false, + wantContain: []string{"s1", "s2", "revoked", "SUCCESSFULLY_REVOKED", "2 of 2 requested sessions revoked"}, + }, + { + name: "all in progress exits zero but is not called revoked", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationInProgress, "s2", scamodels.RevocationInProgress), + wantErr: false, + wantContain: []string{"revocation in progress", "REVOCATION_IN_PROGRESS", "0 of 2 requested sessions revoked", "2 revocations in progress"}, + }, + { + name: "not applicable exits non-zero, provider-neutral wording", + args: []string{"s1"}, + response: revokeResponse("s1", scamodels.RevocationNotApplicable), + wantErr: true, + wantContain: []string{ + "NOT revoked", + "the service reported revocation is not applicable", + "REVOCATION_NOT_APPLICABLE", + "0 of 1 requested sessions revoked", + }, + // The status proves only that the service declined to act. Naming a + // mechanism grant never observed would be an invented explanation. + wantNotContain: []string{"STS", "temporary credentials", "AWS"}, + }, + { + name: "unknown status exits non-zero and keeps the raw token", + args: []string{"s1"}, + response: revokeResponse("s1", "TOTALLY_NEW_STATUS"), + wantErr: true, + wantContain: []string{"NOT revoked", "unexpected status", "TOTALLY_NEW_STATUS"}, + }, + { + name: "empty status exits non-zero", + args: []string{"s1"}, + response: revokeResponse("s1", ""), + wantErr: true, + wantContain: []string{"NOT revoked", "unexpected status"}, + }, + { + name: "requested two, one row returned", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful), + wantErr: true, + wantContain: []string{"s2", "no result returned", "1 of 2 requested sessions revoked"}, + }, + { + name: "empty response array reports every requested session", + args: []string{"s1", "s2"}, + response: &scamodels.RevokeResponse{Response: []scamodels.RevocationResult{}}, + wantErr: true, + wantContain: []string{"s1", "s2", "no result returned", "0 of 2 requested sessions revoked"}, + }, + { + name: "partial exits non-zero", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful, "s2", scamodels.RevocationNotApplicable), + wantErr: true, + wantContain: []string{"revoked (SUCCESSFULLY_REVOKED)", "NOT revoked", "1 of 2 requested sessions revoked", "1 not revoked"}, + }, + { + name: "partial with in-progress distinguishes the two", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationInProgress, "s2", scamodels.RevocationNotApplicable), + wantErr: true, + wantContain: []string{"0 of 2 requested sessions revoked", "1 revocation in progress", "1 not revoked"}, + }, + { + name: "unexpected ID is surfaced but satisfies nothing", + args: []string{"s1"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful, "zz", scamodels.RevocationSuccessful), + wantErr: false, + wantContain: []string{"unexpected result", "zz"}, + }, + { + name: "duplicate request IDs are deduplicated", + args: []string{"s1", "s1"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful), + wantErr: false, + wantContain: []string{"1 of 1 requested sessions revoked"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + revoker := &mockSessionRevoker{response: tt.response} + cmd := NewRevokeCommandWithDeps(testAuthLoader(), &mockSessionLister{}, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{}) + + output, err := executeCommand(cmd, tt.args...) + + if tt.wantErr && err == nil { + t.Errorf("expected an error (exit 1) but got none\noutput:\n%s", output) + } + if !tt.wantErr && err != nil { + t.Errorf("unexpected error: %v\noutput:\n%s", err, output) + } + for _, want := range tt.wantContain { + if !strings.Contains(output, want) { + t.Errorf("output missing %q\ngot:\n%s", want, output) + } + } + for _, notWant := range tt.wantNotContain { + if strings.Contains(output, notWant) { + t.Errorf("output must not contain %q\ngot:\n%s", notWant, output) + } + } + }) + } +} + +// TestRevokeCommand_ProviderNeutralWording asserts the not-applicable wording +// does not vary by CSP. +func TestRevokeCommand_ProviderNeutralWording(t *testing.T) { + render := func(csp scamodels.CSP) string { + lister := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + {SessionID: "s1", CSP: csp, WorkspaceID: "ws", RoleID: "Admin", SessionDuration: 3600}, + }, + Total: 1, + }} + revoker := &mockSessionRevoker{response: revokeResponse("s1", scamodels.RevocationNotApplicable)} + cmd := NewRevokeCommandWithDeps(testAuthLoader(), lister, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{}) + out, err := executeCommand(cmd, "--all", "--yes") + if err == nil { + t.Fatalf("expected an error for a not-applicable revocation (%s)", csp) + } + return out + } + + aws := render(scamodels.CSPAWS) + azure := render(scamodels.CSPAzure) + + const phrase = "the service reported revocation is not applicable" + if !strings.Contains(aws, phrase) || !strings.Contains(azure, phrase) { + t.Fatalf("expected identical provider-neutral wording\naws:\n%s\nazure:\n%s", aws, azure) + } + for _, banned := range []string{"STS", "temporary credentials"} { + if strings.Contains(aws, banned) { + t.Errorf("AWS output must not claim a mechanism grant did not observe (%q)\n%s", banned, aws) + } + } +} + +// TestRevokeCommand_BatchesOverAPILimit verifies --all across more than 100 +// sessions is chunked to the API's 100-ID cap and fully accounted for. +func TestRevokeCommand_BatchesOverAPILimit(t *testing.T) { + const total = 150 + + sessions := make([]scamodels.SessionInfo, total) + for i := range sessions { + sessions[i] = scamodels.SessionInfo{ + SessionID: fmt.Sprintf("s%03d", i), + CSP: scamodels.CSPAzure, + WorkspaceID: "ws", + RoleID: "Reader", + SessionDuration: 3600, + } + } + + revoker := &mockSessionRevoker{ + revokeFunc: func(ctx context.Context, req *scamodels.RevokeRequest) (*scamodels.RevokeResponse, error) { + if len(req.SessionIDs) > scamodels.MaxRevokeBatchSize { + t.Errorf("sent %d session IDs, exceeding the API cap of %d", len(req.SessionIDs), scamodels.MaxRevokeBatchSize) + } + results := make([]scamodels.RevocationResult, len(req.SessionIDs)) + for i, id := range req.SessionIDs { + results[i] = scamodels.RevocationResult{SessionID: id, RevocationStatus: scamodels.RevocationSuccessful} + } + return &scamodels.RevokeResponse{Response: results}, nil + }, + } + + lister := &mockSessionLister{sessions: &scamodels.SessionsResponse{Response: sessions, Total: total}} + cmd := NewRevokeCommandWithDeps(testAuthLoader(), lister, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{}) + + output, err := executeCommand(cmd, "--all", "--yes") + if err != nil { + t.Fatalf("unexpected error: %v\n%s", err, output) + } + if len(revoker.calls) != 2 { + t.Errorf("made %d revoke calls, want 2", len(revoker.calls)) + } + for _, s := range sessions { + if !strings.Contains(output, s.SessionID) { + t.Fatalf("output missing session %s", s.SessionID) + } + } + if !strings.Contains(output, "150 of 150 requested sessions revoked") { + t.Errorf("expected all 150 accounted for, got:\n%s", output) + } +} + +// TestRevokeCommand_BatchErrorKeepsEarlierOutcomes asserts a mid-sequence batch +// failure still reports what is known. +func TestRevokeCommand_BatchErrorKeepsEarlierOutcomes(t *testing.T) { + const total = 150 + + sessions := make([]scamodels.SessionInfo, total) + for i := range sessions { + sessions[i] = scamodels.SessionInfo{SessionID: fmt.Sprintf("s%03d", i), CSP: scamodels.CSPAzure, SessionDuration: 3600} + } + + call := 0 + revoker := &mockSessionRevoker{ + revokeFunc: func(ctx context.Context, req *scamodels.RevokeRequest) (*scamodels.RevokeResponse, error) { + call++ + if call == 2 { + return nil, errors.New("service unavailable") + } + results := make([]scamodels.RevocationResult, len(req.SessionIDs)) + for i, id := range req.SessionIDs { + results[i] = scamodels.RevocationResult{SessionID: id, RevocationStatus: scamodels.RevocationSuccessful} + } + return &scamodels.RevokeResponse{Response: results}, nil + }, + } + + lister := &mockSessionLister{sessions: &scamodels.SessionsResponse{Response: sessions, Total: total}} + cmd := NewRevokeCommandWithDeps(testAuthLoader(), lister, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{}) + + output, err := executeCommand(cmd, "--all", "--yes") + if err == nil { + t.Fatal("expected an error from the failing batch") + } + if !strings.Contains(output, "service unavailable") { + t.Errorf("expected the transport error to be reported, got:\n%s", output) + } + if !strings.Contains(output, "100 of 150 requested sessions revoked") { + t.Errorf("expected the first batch's outcomes to survive, got:\n%s", output) + } + if !strings.Contains(output, "s149") { + t.Errorf("expected the unattempted sessions to be reported, got:\n%s", output) + } +} + +// TestRevokeCommand_ExpiryNote covers the best-effort expiry hint. The clock is +// pinned; deriving the expectation from time.Now() would straddle a minute +// boundary and flake. +func TestRevokeCommand_ExpiryNote(t *testing.T) { + elevatedAt := time.Now().Add(-20 * time.Minute) + pinned := elevatedAt.Add(20 * time.Minute) + + tracker := cache.NewStore(t.TempDir(), 25*time.Hour) + if err := cache.RecordSession(tracker, "s1", elevatedAt); err != nil { + t.Fatalf("failed to seed tracker: %v", err) + } + + lister := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + {SessionID: "s1", CSP: scamodels.CSPAzure, WorkspaceID: "ws", RoleID: "Admin", SessionDuration: 3600}, + }, + Total: 1, + }} + revoker := &mockSessionRevoker{response: revokeResponse("s1", scamodels.RevocationNotApplicable)} + + cmd := newRevokeCommandWithClock(testAuthLoader(), lister, &mockEligibilityLister{}, revoker, + &mockSessionSelector{}, &mockConfirmPrompter{}, tracker, func() time.Time { return pinned }) + + output, err := executeCommand(cmd, "--all", "--yes") + if err == nil { + t.Fatal("expected an error for a not-applicable revocation") + } + if !strings.Contains(output, "expires in ~40m") { + t.Errorf("expected the expiry note, got:\n%s", output) + } +} + +// TestRevokeCommand_ExpiryNoteAbsentInDirectMode: direct mode has bare IDs and +// no session metadata, so no expiry can be claimed. +func TestRevokeCommand_ExpiryNoteAbsentInDirectMode(t *testing.T) { + elevatedAt := time.Now().Add(-20 * time.Minute) + tracker := cache.NewStore(t.TempDir(), 25*time.Hour) + if err := cache.RecordSession(tracker, "s1", elevatedAt); err != nil { + t.Fatalf("failed to seed tracker: %v", err) + } + + revoker := &mockSessionRevoker{response: revokeResponse("s1", scamodels.RevocationNotApplicable)} + cmd := newRevokeCommandWithClock(testAuthLoader(), &mockSessionLister{}, &mockEligibilityLister{}, revoker, + &mockSessionSelector{}, &mockConfirmPrompter{}, tracker, func() time.Time { return elevatedAt.Add(20 * time.Minute) }) + + output, err := executeCommand(cmd, "s1") + if err == nil { + t.Fatal("expected an error for a not-applicable revocation") + } + if strings.Contains(output, "expires in") { + t.Errorf("direct mode has no session metadata, so no expiry may be claimed:\n%s", output) + } +} + +func TestRevokeCommand_JSONOutcomes(t *testing.T) { + tests := []struct { + name string + args []string + response *scamodels.RevokeResponse + wantErr bool + check func(t *testing.T, parsed []revocationOutput) + }{ + { + name: "all revoked", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful, "s2", scamodels.RevocationSuccessful), + check: func(t *testing.T, parsed []revocationOutput) { + if len(parsed) != 2 { + t.Fatalf("got %d entries, want 2", len(parsed)) + } + for _, p := range parsed { + if p.Outcome != string(scamodels.OutcomeRevoked) || !p.Accepted || !p.Complete { + t.Errorf("entry = %+v, want outcome=revoked accepted=true complete=true", p) + } + if p.Status != scamodels.RevocationSuccessful { + t.Errorf("status = %q, want the raw API value", p.Status) + } + } + }, + }, + { + name: "in progress is accepted but not complete", + args: []string{"s1"}, + response: revokeResponse("s1", scamodels.RevocationInProgress), + check: func(t *testing.T, parsed []revocationOutput) { + if parsed[0].Outcome != string(scamodels.OutcomeInProgress) { + t.Errorf("outcome = %q, want in_progress", parsed[0].Outcome) + } + if !parsed[0].Accepted || parsed[0].Complete { + t.Errorf("entry = %+v, want accepted=true complete=false", parsed[0]) + } + }, + }, + { + name: "partial keeps a reason on the refused entry", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful, "s2", scamodels.RevocationNotApplicable), + wantErr: true, + check: func(t *testing.T, parsed []revocationOutput) { + if len(parsed) != 2 { + t.Fatalf("got %d entries, want 2", len(parsed)) + } + if parsed[1].Accepted || parsed[1].Reason == "" { + t.Errorf("entry = %+v, want accepted=false with a reason", parsed[1]) + } + }, + }, + { + name: "missing session appears with an empty status", + args: []string{"s1", "s2"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful), + wantErr: true, + check: func(t *testing.T, parsed []revocationOutput) { + if len(parsed) != 2 { + t.Fatalf("got %d entries, want one per requested session", len(parsed)) + } + if parsed[1].SessionID != "s2" || parsed[1].Status != "" || parsed[1].Outcome != string(scamodels.OutcomeUnknown) { + t.Errorf("entry = %+v, want s2 with empty status and outcome unknown", parsed[1]) + } + }, + }, + { + name: "unknown status preserves the raw value", + args: []string{"s1"}, + response: revokeResponse("s1", "TOTALLY_NEW_STATUS"), + wantErr: true, + check: func(t *testing.T, parsed []revocationOutput) { + if parsed[0].Status != "TOTALLY_NEW_STATUS" { + t.Errorf("status = %q, want the raw API value", parsed[0].Status) + } + if parsed[0].Outcome != string(scamodels.OutcomeUnknown) { + t.Errorf("outcome = %q, want unknown", parsed[0].Outcome) + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + revoker := &mockSessionRevoker{response: tt.response} + cmd := NewRevokeCommandWithDeps(testAuthLoader(), &mockSessionLister{}, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{}) + root := newTestRootCommand() + root.AddCommand(cmd) + + args := append([]string{"revoke"}, tt.args...) + args = append(args, "--yes", "--output", "json") + stdout, _, err := executeCommandStreams(root, args...) + + if tt.wantErr && err == nil { + t.Errorf("expected an error (exit 1) but got none") + } + if !tt.wantErr && err != nil { + t.Errorf("unexpected error: %v", err) + } + + // JSON must be complete and valid even when the command exits 1. + var parsed []revocationOutput + if uerr := json.Unmarshal([]byte(stdout), &parsed); uerr != nil { + t.Fatalf("invalid JSON on stdout: %v\n%s", uerr, stdout) + } + if strings.Contains(stdout, `"revoked":true`) || strings.Contains(stdout, `"revoked": true`) { + t.Errorf("JSON must never carry a revoked boolean:\n%s", stdout) + } + tt.check(t, parsed) + }) + } +} diff --git a/cmd/revoke_reconcile.go b/cmd/revoke_reconcile.go new file mode 100644 index 0000000..040ade1 --- /dev/null +++ b/cmd/revoke_reconcile.go @@ -0,0 +1,148 @@ +package cmd + +import ( + "fmt" + + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +// revocationRecord is the reconciled outcome for one *requested* session. +// There is exactly one record per requested session, whether or not the +// service returned a row for it. +type revocationRecord struct { + SessionID string + Status string // raw API value; "" when no row was returned + Outcome scamodels.RevocationOutcome + Reason string // why this is not a confirmed revocation; "" when revoked + Duplicate bool // the service returned more than one row for this ID +} + +// unattributedResult is a returned row that cannot be attributed to a +// requested session: either an ID that was never requested, or an empty ID. +type unattributedResult struct { + SessionID string + Status string +} + +// dedupeSessionIDs removes repeated IDs, preserving first-seen order. +func dedupeSessionIDs(ids []string) []string { + if len(ids) == 0 { + return nil + } + seen := make(map[string]bool, len(ids)) + out := make([]string, 0, len(ids)) + for _, id := range ids { + if seen[id] { + continue + } + seen[id] = true + out = append(out, id) + } + return out +} + +// outcomeRank orders outcomes best to worst so the worst can win when the +// service returns more than one row for the same session. +func outcomeRank(o scamodels.RevocationOutcome) int { + switch o { + case scamodels.OutcomeRevoked: + return 3 + case scamodels.OutcomeInProgress: + return 2 + case scamodels.OutcomeNotApplicable: + return 1 + default: + return 0 + } +} + +// reasonForOutcome explains a non-complete outcome in provider-neutral terms. +// It never attributes a cause grant did not observe. +func reasonForOutcome(outcome scamodels.RevocationOutcome, status string) string { + switch outcome { + case scamodels.OutcomeRevoked: + return "" + case scamodels.OutcomeInProgress: + return "accepted by the service, not yet confirmed complete" + case scamodels.OutcomeNotApplicable: + return "the service reported revocation is not applicable to this session" + default: + return fmt.Sprintf("unexpected status %q (treated as failure)", status) + } +} + +// reconcileRevocations joins the service's results onto the *requested* session +// IDs, which are the source of truth. A requested session with no returned row +// is an unknown outcome, not a success. The requested set is deduplicated, +// preserving first-seen order. +func reconcileRevocations(requested []string, results []scamodels.RevocationResult) ([]revocationRecord, []unattributedResult) { + ids := dedupeSessionIDs(requested) + + index := make(map[string]int, len(ids)) + records := make([]revocationRecord, len(ids)) + for i, id := range ids { + index[id] = i + records[i] = revocationRecord{ + SessionID: id, + Outcome: scamodels.OutcomeUnknown, + Reason: "no result returned by the service for this session", + } + } + + seen := make(map[string]bool, len(ids)) + var unattached []unattributedResult + + for _, r := range results { + i, ok := index[r.SessionID] + if !ok || r.SessionID == "" { + unattached = append(unattached, unattributedResult{SessionID: r.SessionID, Status: r.RevocationStatus}) + continue + } + + outcome := scamodels.ClassifyRevocationStatus(r.RevocationStatus) + if seen[r.SessionID] { + records[i].Duplicate = true + // Worst outcome wins: a later success must never mask an earlier failure. + if outcomeRank(outcome) >= outcomeRank(records[i].Outcome) { + continue + } + } + seen[r.SessionID] = true + + records[i].Status = r.RevocationStatus + records[i].Outcome = outcome + records[i].Reason = reasonForOutcome(outcome, r.RevocationStatus) + } + + return records, unattached +} + +// revocationSummary counts outcomes over the requested session set. +type revocationSummary struct { + requested int + revoked int + inProgress int + failed int +} + +// allAccepted reports whether every requested session was accepted by the +// service (revoked or in progress). An empty requested set is not a success: +// nothing was confirmed revoked. +func (s revocationSummary) allAccepted() bool { + return s.requested > 0 && s.failed == 0 +} + +func summarizeRevocations(records []revocationRecord) revocationSummary { + s := revocationSummary{requested: len(records)} + for _, r := range records { + switch { + case r.Outcome.Complete(): + s.revoked++ + case r.Outcome.Accepted(): + s.inProgress++ + default: + s.failed++ + } + } + return s +} diff --git a/cmd/revoke_reconcile_test.go b/cmd/revoke_reconcile_test.go new file mode 100644 index 0000000..5e71a3e --- /dev/null +++ b/cmd/revoke_reconcile_test.go @@ -0,0 +1,236 @@ +// NOTE: Do not use t.Parallel() in cmd/ tests due to package-level state +// (verbose, passedArgValidation) that is mutated during test execution. +package cmd + +import ( + "strings" + "testing" + + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +func TestDedupeSessionIDs(t *testing.T) { + tests := []struct { + name string + in []string + want []string + }{ + {"no duplicates", []string{"a", "b"}, []string{"a", "b"}}, + {"duplicates collapsed, first-seen order kept", []string{"b", "a", "b"}, []string{"b", "a"}}, + {"all duplicates", []string{"a", "a", "a"}, []string{"a"}}, + {"empty", nil, nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := dedupeSessionIDs(tt.in) + if len(got) != len(tt.want) { + t.Fatalf("dedupeSessionIDs(%v) = %v, want %v", tt.in, got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Fatalf("dedupeSessionIDs(%v) = %v, want %v", tt.in, got, tt.want) + } + } + }) + } +} + +func TestReconcileRevocations(t *testing.T) { + ok := func(id string) scamodels.RevocationResult { + return scamodels.RevocationResult{SessionID: id, RevocationStatus: scamodels.RevocationSuccessful} + } + notApplicable := func(id string) scamodels.RevocationResult { + return scamodels.RevocationResult{SessionID: id, RevocationStatus: scamodels.RevocationNotApplicable} + } + + tests := []struct { + name string + requested []string + results []scamodels.RevocationResult + wantRecords []revocationRecord + wantUnattached []unattributedResult + wantReasonSub map[string]string + }{ + { + name: "exact match", + requested: []string{"A", "B"}, + results: []scamodels.RevocationResult{ok("A"), ok("B")}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: scamodels.RevocationSuccessful, Outcome: scamodels.OutcomeRevoked}, + {SessionID: "B", Status: scamodels.RevocationSuccessful, Outcome: scamodels.OutcomeRevoked}, + }, + }, + { + name: "missing row is a failure", + requested: []string{"A", "B"}, + results: []scamodels.RevocationResult{ok("A")}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: scamodels.RevocationSuccessful, Outcome: scamodels.OutcomeRevoked}, + {SessionID: "B", Status: "", Outcome: scamodels.OutcomeUnknown}, + }, + wantReasonSub: map[string]string{"B": "no result returned"}, + }, + { + name: "duplicate rows, worst outcome wins", + requested: []string{"A"}, + results: []scamodels.RevocationResult{ok("A"), notApplicable("A")}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: scamodels.RevocationNotApplicable, Outcome: scamodels.OutcomeNotApplicable, Duplicate: true}, + }, + }, + { + name: "duplicate rows reversed, later success must not mask failure", + requested: []string{"A"}, + results: []scamodels.RevocationResult{notApplicable("A"), ok("A")}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: scamodels.RevocationNotApplicable, Outcome: scamodels.OutcomeNotApplicable, Duplicate: true}, + }, + }, + { + name: "row with empty session ID is unattributable", + requested: []string{"A"}, + results: []scamodels.RevocationResult{{SessionID: "", RevocationStatus: scamodels.RevocationSuccessful}}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: "", Outcome: scamodels.OutcomeUnknown}, + }, + wantUnattached: []unattributedResult{{SessionID: "", Status: scamodels.RevocationSuccessful}}, + }, + { + name: "unexpected ID satisfies nothing", + requested: []string{"A"}, + results: []scamodels.RevocationResult{ok("A"), ok("Z")}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: scamodels.RevocationSuccessful, Outcome: scamodels.OutcomeRevoked}, + }, + wantUnattached: []unattributedResult{{SessionID: "Z", Status: scamodels.RevocationSuccessful}}, + }, + { + name: "empty response array, all unknown", + requested: []string{"A", "B"}, + results: nil, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: "", Outcome: scamodels.OutcomeUnknown}, + {SessionID: "B", Status: "", Outcome: scamodels.OutcomeUnknown}, + }, + }, + { + name: "duplicate in request is deduped", + requested: []string{"A", "A", "B"}, + results: []scamodels.RevocationResult{ok("A"), ok("B")}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: scamodels.RevocationSuccessful, Outcome: scamodels.OutcomeRevoked}, + {SessionID: "B", Status: scamodels.RevocationSuccessful, Outcome: scamodels.OutcomeRevoked}, + }, + }, + { + name: "unknown status fails closed", + requested: []string{"A"}, + results: []scamodels.RevocationResult{{SessionID: "A", RevocationStatus: "TOTALLY_NEW_STATUS"}}, + wantRecords: []revocationRecord{ + {SessionID: "A", Status: "TOTALLY_NEW_STATUS", Outcome: scamodels.OutcomeUnknown}, + }, + wantReasonSub: map[string]string{"A": "unexpected status"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + records, unattached := reconcileRevocations(tt.requested, tt.results) + + if len(records) != len(tt.wantRecords) { + t.Fatalf("got %d records, want %d: %+v", len(records), len(tt.wantRecords), records) + } + for i, want := range tt.wantRecords { + got := records[i] + if got.SessionID != want.SessionID || got.Status != want.Status || + got.Outcome != want.Outcome || got.Duplicate != want.Duplicate { + t.Errorf("record[%d] = %+v, want %+v", i, got, want) + } + if !got.Outcome.Accepted() && got.Reason == "" { + t.Errorf("record[%d] (%s) has no reason for a non-accepted outcome", i, got.SessionID) + } + } + + for id, sub := range tt.wantReasonSub { + found := false + for _, r := range records { + if r.SessionID == id { + found = true + if !strings.Contains(r.Reason, sub) { + t.Errorf("reason for %s = %q, want substring %q", id, r.Reason, sub) + } + } + } + if !found { + t.Errorf("no record for %s", id) + } + } + + if len(unattached) != len(tt.wantUnattached) { + t.Fatalf("got %d unattributed results, want %d: %+v", len(unattached), len(tt.wantUnattached), unattached) + } + for i, want := range tt.wantUnattached { + if unattached[i] != want { + t.Errorf("unattributed[%d] = %+v, want %+v", i, unattached[i], want) + } + } + }) + } +} + +func TestSummarizeRevocations(t *testing.T) { + tests := []struct { + name string + records []revocationRecord + wantRevoked int + wantPending int + wantFailed int + wantAllOK bool + }{ + { + name: "all revoked", + records: []revocationRecord{ + {Outcome: scamodels.OutcomeRevoked}, {Outcome: scamodels.OutcomeRevoked}, + }, + wantRevoked: 2, wantAllOK: true, + }, + { + name: "in progress counts as accepted but not revoked", + records: []revocationRecord{ + {Outcome: scamodels.OutcomeRevoked}, {Outcome: scamodels.OutcomeInProgress}, + }, + wantRevoked: 1, wantPending: 1, wantAllOK: true, + }, + { + name: "partial fails", + records: []revocationRecord{ + {Outcome: scamodels.OutcomeRevoked}, {Outcome: scamodels.OutcomeNotApplicable}, + }, + wantRevoked: 1, wantFailed: 1, wantAllOK: false, + }, + { + name: "unknown fails", + records: []revocationRecord{{Outcome: scamodels.OutcomeUnknown}}, + wantFailed: 1, wantAllOK: false, + }, + { + name: "no records at all fails", + records: nil, + wantAllOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := summarizeRevocations(tt.records) + if s.revoked != tt.wantRevoked || s.inProgress != tt.wantPending || s.failed != tt.wantFailed { + t.Errorf("summary = %+v, want revoked=%d inProgress=%d failed=%d", + s, tt.wantRevoked, tt.wantPending, tt.wantFailed) + } + if s.allAccepted() != tt.wantAllOK { + t.Errorf("allAccepted() = %v, want %v", s.allAccepted(), tt.wantAllOK) + } + }) + } +} diff --git a/cmd/revoke_render.go b/cmd/revoke_render.go new file mode 100644 index 0000000..44a8d52 --- /dev/null +++ b/cmd/revoke_render.go @@ -0,0 +1,129 @@ +package cmd + +import ( + "fmt" + "io" + "time" + + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +// expiryHinter supplies the best-effort "expires in ~Xm" note. The hint needs +// both session metadata (for the duration) and a local elevation timestamp, so +// it is unavailable in direct mode, where grant only has bare session IDs. +// +// The note is purely informational. It never explains *why* a revocation was +// refused — grant has no evidence for that. +type expiryHinter struct { + metadata map[string]scamodels.SessionInfo + timestamps map[string]time.Time + now func() time.Time +} + +// note returns a parenthesised expiry clause, or "" when it is unknown. +func (h expiryHinter) note(sessionID string) string { + if h.now == nil || len(h.metadata) == 0 || len(h.timestamps) == 0 { + return "" + } + session, ok := h.metadata[sessionID] + if !ok { + return "" + } + remaining, ok := computeRemainingTimeAt([]scamodels.SessionInfo{session}, h.timestamps, h.now())[sessionID] + if !ok { + return "" + } + if remaining <= 0 { + return " (this session has already expired)" + } + totalMin := int(remaining.Minutes()) + if totalMin >= 60 { + return fmt.Sprintf(" (this session expires in ~%dh %dm)", totalMin/60, totalMin%60) + } + return fmt.Sprintf(" (this session expires in ~%dm)", totalMin) +} + +// describeRecord renders one reconciled record as a human-readable line body. +func describeRecord(r revocationRecord) string { + switch r.Outcome { + case scamodels.OutcomeRevoked: + return fmt.Sprintf("revoked (%s)", r.Status) + case scamodels.OutcomeInProgress: + return fmt.Sprintf("revocation in progress — %s (%s)", r.Reason, r.Status) + case scamodels.OutcomeNotApplicable: + return fmt.Sprintf("NOT revoked — %s (%s)", r.Reason, r.Status) + default: + return "NOT revoked — " + r.Reason + } +} + +// summaryLine states the outcome over the *requested* sessions, keeping the +// accepted-versus-complete distinction visible. It never claims that an +// accepted revocation is a finished one. +func summaryLine(s revocationSummary) string { + line := fmt.Sprintf("%d of %d requested sessions revoked", s.revoked, s.requested) + if s.inProgress > 0 { + line += fmt.Sprintf("; %d %s in progress", s.inProgress, plural(s.inProgress, "revocation", "revocations")) + } + if s.failed > 0 { + line += fmt.Sprintf("; %d not revoked", s.failed) + } + return line + "." +} + +func plural(n int, singular, plural string) string { + if n == 1 { + return singular + } + return plural +} + +// renderRevocationResults prints the full per-session breakdown. It is always +// called before the command returns its error, so a non-zero exit is never +// opaque. +func renderRevocationResults(w io.Writer, records []revocationRecord, unattached []unattributedResult, hints expiryHinter) { + for _, r := range records { + fmt.Fprintf(w, " %s: %s%s\n", r.SessionID, describeRecord(r), hints.note(r.SessionID)) + } + + for _, u := range unattached { + if u.SessionID == "" { + fmt.Fprintf(w, " ! unexpected result with an empty session ID (status %q); it cannot be attributed to any requested session\n", u.Status) + continue + } + fmt.Fprintf(w, " ! unexpected result for session %s (status %q); it was not requested and satisfies nothing\n", u.SessionID, u.Status) + } + + if len(records) > 0 { + fmt.Fprintf(w, "%s\n", summaryLine(summarizeRevocations(records))) + } +} + +// buildRevocationJSON builds the machine-readable output: one entry per +// requested session in requested order, followed by unattributable results. +func buildRevocationJSON(records []revocationRecord, unattached []unattributedResult) []revocationOutput { + out := make([]revocationOutput, 0, len(records)+len(unattached)) + for _, r := range records { + out = append(out, revocationOutput{ + SessionID: r.SessionID, + Status: r.Status, + Outcome: string(r.Outcome), + Accepted: r.Outcome.Accepted(), + Complete: r.Outcome.Complete(), + Reason: r.Reason, + }) + } + for _, u := range unattached { + outcome := scamodels.ClassifyRevocationStatus(u.Status) + out = append(out, revocationOutput{ + SessionID: u.SessionID, + Status: u.Status, + Outcome: string(outcome), + Accepted: false, + Complete: false, + Reason: "result was not requested and satisfies no requested session", + Unexpected: true, + }) + } + return out +} diff --git a/cmd/revoke_test.go b/cmd/revoke_test.go index 6295034..7d6e60f 100644 --- a/cmd/revoke_test.go +++ b/cmd/revoke_test.go @@ -727,7 +727,7 @@ func TestRevokeCommand_JSONOutput(t *testing.T) { elig := &mockEligibilityLister{} revoker := &mockSessionRevoker{response: &scamodels.RevokeResponse{ Response: []scamodels.RevocationResult{ - {SessionID: "s1", RevocationStatus: "Revoked"}, + {SessionID: "s1", RevocationStatus: scamodels.RevocationSuccessful}, }, }} selector := &mockSessionSelector{sessions: []scamodels.SessionInfo{ @@ -754,7 +754,45 @@ func TestRevokeCommand_JSONOutput(t *testing.T) { if parsed[0].SessionID != "s1" { t.Errorf("sessionId = %q, want s1", parsed[0].SessionID) } + if parsed[0].Status != scamodels.RevocationSuccessful { + t.Errorf("status = %q, want the raw API value", parsed[0].Status) + } + if parsed[0].Outcome != string(scamodels.OutcomeRevoked) || !parsed[0].Accepted || !parsed[0].Complete { + t.Errorf("entry = %+v, want outcome=revoked accepted=true complete=true", parsed[0]) + } +} + +// TestRevokeCommand_JSONOutput_UndocumentedStatus pins the fail-closed +// behavior: "Revoked" is not in the API's enum, so it must not read as success. +func TestRevokeCommand_JSONOutput_UndocumentedStatus(t *testing.T) { + now := time.Now() + expiresIn := commonmodels.IdsecRFC3339Time(now.Add(1 * time.Hour)) + + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user", ExpiresIn: expiresIn}} + revoker := &mockSessionRevoker{response: &scamodels.RevokeResponse{ + Response: []scamodels.RevocationResult{ + {SessionID: "s1", RevocationStatus: "Revoked"}, + }, + }} + + cmd := NewRevokeCommandWithDeps(auth, &mockSessionLister{}, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{confirmed: true}) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, _, err := executeCommandStreams(root, "revoke", "s1", "--yes", "--output", "json") + if err == nil { + t.Fatal("expected an error: an undocumented status must fail closed") + } + + var parsed []revocationOutput + if uerr := json.Unmarshal([]byte(stdout), &parsed); uerr != nil { + t.Fatalf("invalid JSON on stdout: %v\n%s", uerr, stdout) + } if parsed[0].Status != "Revoked" { - t.Errorf("status = %q, want Revoked", parsed[0].Status) + t.Errorf("status = %q, want the raw API value preserved", parsed[0].Status) + } + if parsed[0].Outcome != string(scamodels.OutcomeUnknown) || parsed[0].Accepted || parsed[0].Complete { + t.Errorf("entry = %+v, want outcome=unknown accepted=false complete=false", parsed[0]) } } diff --git a/cmd/status.go b/cmd/status.go index d5527ea..f72d915 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -178,11 +178,16 @@ func runStatus( // computeRemainingTime builds a sessionID -> remaining duration map from local timestamps. func computeRemainingTime(sessions []scamodels.SessionInfo, timestamps map[string]time.Time) map[string]time.Duration { + return computeRemainingTimeAt(sessions, timestamps, time.Now()) +} + +// computeRemainingTimeAt is computeRemainingTime with an explicit clock, so +// callers that need deterministic output can pin it. +func computeRemainingTimeAt(sessions []scamodels.SessionInfo, timestamps map[string]time.Time, now time.Time) map[string]time.Duration { if len(timestamps) == 0 { return nil } - now := time.Now() remaining := make(map[string]time.Duration) for _, s := range sessions { if elevatedAt, ok := timestamps[s.SessionID]; ok { diff --git a/cmd/test_helpers.go b/cmd/test_helpers.go index 71a23fa..4f47512 100644 --- a/cmd/test_helpers.go +++ b/cmd/test_helpers.go @@ -37,6 +37,21 @@ func executeCommand(cmd *cobra.Command, args ...string) (string, error) { return buf.String(), err } +// executeCommandStreams executes a command keeping stdout and stderr apart and +// writing no error text into either. Use it when a test needs to assert on the +// exact stdout payload (e.g. valid JSON) *and* on a returned error, which +// executeCommand cannot express because it merges the streams and appends the +// error text. +func executeCommandStreams(cmd *cobra.Command, args ...string) (stdout, stderr string, err error) { + var outBuf, errBuf bytes.Buffer + cmd.SetOut(&outBuf) + cmd.SetErr(&errBuf) + cmd.SetArgs(args) + + err = cmd.Execute() + return outBuf.String(), errBuf.String(), err +} + // executeWithHint simulates Execute() logic without os.Exit, returning the error output. // Used for testing the verbose hint behavior. func executeWithHint(cmd *cobra.Command, args []string) string { diff --git a/cmd/test_mocks.go b/cmd/test_mocks.go index cc219af..75e0495 100644 --- a/cmd/test_mocks.go +++ b/cmd/test_mocks.go @@ -91,9 +91,15 @@ type mockSessionRevoker struct { revokeFunc func(ctx context.Context, req *models.RevokeRequest) (*models.RevokeResponse, error) response *models.RevokeResponse revokeErr error + // calls records the session IDs sent on every invocation, so batching + // behavior can be asserted. + calls [][]string } func (m *mockSessionRevoker) RevokeSessions(ctx context.Context, req *models.RevokeRequest) (*models.RevokeResponse, error) { + if req != nil { + m.calls = append(m.calls, append([]string(nil), req.SessionIDs...)) + } if m.revokeFunc != nil { return m.revokeFunc(ctx, req) } diff --git a/internal/sca/models/revoke.go b/internal/sca/models/revoke.go index 8cdbdf2..d036835 100644 --- a/internal/sca/models/revoke.go +++ b/internal/sca/models/revoke.go @@ -2,11 +2,65 @@ package models import "encoding/json" +// Revocation status values returned in SessionRevocationInfo.revocationStatus. +// +// Only RevocationSuccessful and RevocationInProgress appear in the API spec's +// enum. RevocationNotApplicable is observed from the live API but documented +// nowhere (neither the OpenAPI spec nor the SDK), so the status set must be +// treated as open — see ClassifyRevocationStatus. const ( - RevocationSuccessful = "SUCCESSFULLY_REVOKED" - RevocationInProgress = "REVOCATION_IN_PROGRESS" + RevocationSuccessful = "SUCCESSFULLY_REVOKED" + RevocationInProgress = "REVOCATION_IN_PROGRESS" + RevocationNotApplicable = "REVOCATION_NOT_APPLICABLE" ) +// MaxRevokeBatchSize is the maximum number of session IDs accepted in a single +// revoke request (`sessionIds` has `maxItems: 100` in the API spec). +const MaxRevokeBatchSize = 100 + +// RevocationOutcome is the classified result of a revocation attempt for one +// session. It is deliberately not a boolean: "in progress" means the service +// accepted the command, not that the access is gone. +type RevocationOutcome string + +const ( + // OutcomeRevoked means revocation is confirmed complete. + OutcomeRevoked RevocationOutcome = "revoked" + // OutcomeInProgress means the service accepted the command but has not confirmed completion. + OutcomeInProgress RevocationOutcome = "in_progress" + // OutcomeNotApplicable means the service declined to act on the session. + OutcomeNotApplicable RevocationOutcome = "not_applicable" + // OutcomeUnknown covers unrecognized, empty and missing statuses. It fails closed. + OutcomeUnknown RevocationOutcome = "unknown" +) + +// ClassifyRevocationStatus maps a raw API status to an outcome. Matching is +// exact (the API uses SCREAMING_SNAKE_CASE); anything unrecognized, including +// the empty string and case variants, classifies as OutcomeUnknown so an +// unexpected value can never be read as success. +func ClassifyRevocationStatus(status string) RevocationOutcome { + switch status { + case RevocationSuccessful: + return OutcomeRevoked + case RevocationInProgress: + return OutcomeInProgress + case RevocationNotApplicable: + return OutcomeNotApplicable + default: + return OutcomeUnknown + } +} + +// Accepted reports whether the service accepted the revocation command. +func (o RevocationOutcome) Accepted() bool { + return o == OutcomeRevoked || o == OutcomeInProgress +} + +// Complete reports whether revocation is confirmed finished. +func (o RevocationOutcome) Complete() bool { + return o == OutcomeRevoked +} + // RevokeRequest is the request body for POST /api/access/sessions/revoke. type RevokeRequest struct { SessionIDs []string `json:"sessionIds"` diff --git a/internal/sca/models/revoke_test.go b/internal/sca/models/revoke_test.go index 9b68f04..1881586 100644 --- a/internal/sca/models/revoke_test.go +++ b/internal/sca/models/revoke_test.go @@ -116,6 +116,49 @@ func TestRevokeResponse_Mixed(t *testing.T) { } } +func TestClassifyRevocationStatus(t *testing.T) { + t.Parallel() + tests := []struct { + name string + status string + want RevocationOutcome + accepted bool + complete bool + }{ + {"documented success", RevocationSuccessful, OutcomeRevoked, true, true}, + {"documented in progress", RevocationInProgress, OutcomeInProgress, true, false}, + {"undocumented not applicable", RevocationNotApplicable, OutcomeNotApplicable, false, false}, + {"empty status fails closed", "", OutcomeUnknown, false, false}, + {"novel status fails closed", "TOTALLY_NEW_STATUS", OutcomeUnknown, false, false}, + {"lowercase variant fails closed", "successfully_revoked", OutcomeUnknown, false, false}, + {"mixed case variant fails closed", "Revoked", OutcomeUnknown, false, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := ClassifyRevocationStatus(tt.status) + if got != tt.want { + t.Errorf("ClassifyRevocationStatus(%q) = %q, want %q", tt.status, got, tt.want) + } + if got.Accepted() != tt.accepted { + t.Errorf("%q.Accepted() = %v, want %v", got, got.Accepted(), tt.accepted) + } + if got.Complete() != tt.complete { + t.Errorf("%q.Complete() = %v, want %v", got, got.Complete(), tt.complete) + } + }) + } +} + +func TestMaxRevokeBatchSize(t *testing.T) { + t.Parallel() + // The API spec caps sessionIds at maxItems: 100 on the request body. + if MaxRevokeBatchSize != 100 { + t.Errorf("MaxRevokeBatchSize = %d, want 100", MaxRevokeBatchSize) + } +} + func TestRevokeResponse_SnakeCase(t *testing.T) { t.Parallel() jsonInput := `{ diff --git a/internal/sca/service_test.go b/internal/sca/service_test.go index a441c36..78d3901 100644 --- a/internal/sca/service_test.go +++ b/internal/sca/service_test.go @@ -410,6 +410,34 @@ func TestRevokeSessions_HTTPError(t *testing.T) { } } +// TestRevokeSessions_UndocumentedStatusPassesThrough proves the decoder does +// not normalize or reject statuses outside the spec's enum. REVOCATION_NOT_APPLICABLE +// is returned by the live API but documented in neither the spec nor the SDK, +// and classification happens above this layer. +func TestRevokeSessions_UndocumentedStatusPassesThrough(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader( + `{"response":[{"sessionId":"session-1","revocationStatus":"REVOCATION_NOT_APPLICABLE"}]}`)), + }, + } + + svc := &SCAAccessService{httpClient: mock} + result, err := svc.RevokeSessions(t.Context(), &models.RevokeRequest{ + SessionIDs: []string{"session-1"}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(result.Response) != 1 { + t.Fatalf("expected 1 result, got %d", len(result.Response)) + } + if result.Response[0].RevocationStatus != models.RevocationNotApplicable { + t.Errorf("status = %q, want it passed through unaltered", result.Response[0].RevocationStatus) + } +} + func TestListSessions_Success(t *testing.T) { resp := models.SessionsResponse{ Response: []models.SessionInfo{ From 62d75707b9142ffe855663fdbb0a6c11f0b90e17 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:58:31 +0200 Subject: [PATCH 2/3] fix(revoke): make unattributed rows unknown and empty selection reachable Addresses two Codex review findings. An unattributed result row (an ID grant never requested, or an empty ID) was classified from its raw status, so it could report outcome "revoked" while both `accepted` and `complete` were false. Consumers keying on `outcome` would read success where consumers keying on the axes read failure. A row grant never asked about is not a success by any reading, so the outcome is now always `unknown`, agreeing with both axes; the raw status is still preserved for the operator. The zero-selected-sessions no-op was unreachable: it sat after the request was built, past the point where the confirmation prompt had already run. It now sits immediately after selection, so an empty selection is treated as cancellation without prompting and never reaches the API. --- cmd/revoke.go | 11 +++--- cmd/revoke_outcome_test.go | 71 ++++++++++++++++++++++++++++++++++++++ cmd/revoke_render.go | 8 +++-- 3 files changed, 84 insertions(+), 6 deletions(-) diff --git a/cmd/revoke.go b/cmd/revoke.go index f26c108..6648745 100644 --- a/cmd/revoke.go +++ b/cmd/revoke.go @@ -164,10 +164,6 @@ func runRevoke( // The requested set is the source of truth for every count and for the exit // code, so deduplicate it before sending and before reconciling. sessionIDs = dedupeSessionIDs(sessionIDs) - if len(sessionIDs) == 0 { - fmt.Fprintln(cmd.OutOrStdout(), "No sessions selected.") - return nil - } // A failing batch still returns the results already collected. results, revokeErr := revokeInBatches(context.Background(), revoker, sessionIDs) @@ -251,6 +247,13 @@ func resolveRevokeTargets( if err != nil { return nil, nil, true, fmt.Errorf("session selection failed: %w", err) } + // Selecting nothing is a deliberate no-op, equivalent to declining the + // confirmation. Handled here rather than after the request is built, so + // an empty selection can never be revoked as if it were a request. + if len(selected) == 0 { + fmt.Fprintln(cmd.OutOrStdout(), "No sessions selected.") + return nil, nil, true, nil + } } for _, s := range selected { diff --git a/cmd/revoke_outcome_test.go b/cmd/revoke_outcome_test.go index 3ecf3a3..d3983c4 100644 --- a/cmd/revoke_outcome_test.go +++ b/cmd/revoke_outcome_test.go @@ -338,6 +338,38 @@ func TestRevokeCommand_ExpiryNoteAbsentInDirectMode(t *testing.T) { } } +// TestRevokeCommand_EmptySelection: selecting nothing is a no-op, not a +// revocation of nothing. It must never reach the API. +func TestRevokeCommand_EmptySelection(t *testing.T) { + lister := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + {SessionID: "s1", CSP: scamodels.CSPAzure, WorkspaceID: "ws", RoleID: "Admin", SessionDuration: 3600}, + }, + Total: 1, + }} + revoker := &mockSessionRevoker{} + confirmer := &mockConfirmPrompter{ + confirmFunc: func(count int) (bool, error) { + t.Error("confirmation must not be requested when nothing was selected") + return false, nil + }, + } + + cmd := NewRevokeCommandWithDeps(testAuthLoader(), lister, &mockEligibilityLister{}, + revoker, &mockSessionSelector{sessions: nil}, confirmer) + + output, err := executeCommand(cmd) + if err != nil { + t.Fatalf("unexpected error: %v\n%s", err, output) + } + if !strings.Contains(output, "No sessions selected") { + t.Errorf("expected the no-op to be reported, got:\n%s", output) + } + if len(revoker.calls) != 0 { + t.Errorf("made %d revoke calls, want 0", len(revoker.calls)) + } +} + func TestRevokeCommand_JSONOutcomes(t *testing.T) { tests := []struct { name string @@ -405,6 +437,45 @@ func TestRevokeCommand_JSONOutcomes(t *testing.T) { } }, }, + { + name: "unattributed row is never a success, whatever its status", + args: []string{"s1"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful, "zz", scamodels.RevocationSuccessful), + check: func(t *testing.T, parsed []revocationOutput) { + if len(parsed) != 2 { + t.Fatalf("got %d entries, want the requested session plus the unattributed row", len(parsed)) + } + u := parsed[1] + if !u.Unexpected || u.SessionID != "zz" { + t.Fatalf("entry = %+v, want the unattributed zz row", u) + } + // outcome and the accepted/complete axes must agree: a row grant + // never requested is not a success by any reading. + if u.Outcome != string(scamodels.OutcomeUnknown) { + t.Errorf("outcome = %q, want unknown", u.Outcome) + } + if u.Accepted || u.Complete { + t.Errorf("entry = %+v, want accepted=false complete=false", u) + } + if u.Status != scamodels.RevocationSuccessful { + t.Errorf("status = %q, want the raw API value preserved", u.Status) + } + }, + }, + { + name: "unattributed row with an empty session ID is never a success", + args: []string{"s1"}, + response: revokeResponse("s1", scamodels.RevocationSuccessful, "", scamodels.RevocationInProgress), + check: func(t *testing.T, parsed []revocationOutput) { + u := parsed[len(parsed)-1] + if !u.Unexpected || u.SessionID != "" { + t.Fatalf("entry = %+v, want the unattributable empty-ID row", u) + } + if u.Outcome != string(scamodels.OutcomeUnknown) || u.Accepted || u.Complete { + t.Errorf("entry = %+v, want outcome=unknown accepted=false complete=false", u) + } + }, + }, { name: "unknown status preserves the raw value", args: []string{"s1"}, diff --git a/cmd/revoke_render.go b/cmd/revoke_render.go index 44a8d52..5a306dd 100644 --- a/cmd/revoke_render.go +++ b/cmd/revoke_render.go @@ -114,11 +114,15 @@ func buildRevocationJSON(records []revocationRecord, unattached []unattributedRe }) } for _, u := range unattached { - outcome := scamodels.ClassifyRevocationStatus(u.Status) + // The outcome is always unknown, whatever the raw status says. A row + // grant never asked for is not a success by any reading, and reporting + // outcome "revoked" alongside accepted=false/complete=false would let + // consumers keying on outcome and consumers keying on the axes disagree. + // The raw status is preserved for the operator. out = append(out, revocationOutput{ SessionID: u.SessionID, Status: u.Status, - Outcome: string(outcome), + Outcome: string(scamodels.OutcomeUnknown), Accepted: false, Complete: false, Reason: "result was not requested and satisfies no requested session", From 2f347951a945fe871d5119694fd54597d14caa80 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 09:01:28 +0200 Subject: [PATCH 3/3] refactor(revoke): reduce revocation output to a single outcome field Bounded simplification of the revocation reporting surface, applied before the JSON shape becomes a compatibility surface on release. Reconciliation, batching, provider-neutral messaging and the file split are unchanged. - Drop `accepted` and `complete` from `revocationOutput`. Both are derivable from `outcome`, and carrying two representations of one concept is what let the unattributed-row rows disagree with themselves in the first place. `outcome` is now the single classification field; the raw `status` stays in both text and JSON. `summarizeRevocations` switches on `outcome` directly, which leaves `RevocationOutcome.Accepted`/`.Complete` with no callers, so they go too. - Drop the expiry hint, the session-ID -> metadata map that fed it, and the `computeRemainingTimeAt` clock seam added for it. The hint rested on incomplete local evidence: it only existed when grant elevated the session on this machine and the tracker had not aged out, and never in direct mode. That makes it a poor fit for authoritative remediation output. `grant status` reports remaining time. `cmd/status.go` is back to its original form and unaffected. - Drop `revocationRecord.Duplicate`. It was set but never rendered and never read as a decision input; worst-outcome-wins already encodes what matters. Exit policy is settled explicitly rather than described as "fail closed", which it never was: exit 0 covers every requested session the service accepted, `REVOCATION_IN_PROGRESS` included, because that is a documented asynchronous success state and failing on it would make legitimate revocations look broken. Exit 1 covers refusals, unrecognized statuses and sessions with no returned row. Exit 0 therefore does not prove access is gone, only that nothing was refused or unaccounted for. Comments, README, CHANGELOG and CLAUDE.md now say exactly that. `ClassifyRevocationStatus` still fails closed; the command does not fail on in-progress. --- CHANGELOG.md | 2 +- CLAUDE.md | 4 +- README.md | 20 ++++--- cmd/output_types.go | 9 +-- cmd/revoke.go | 94 +++++++++--------------------- cmd/revoke_outcome_test.go | 86 +++++---------------------- cmd/revoke_reconcile.go | 21 +++---- cmd/revoke_reconcile_test.go | 11 ++-- cmd/revoke_render.go | 58 +++--------------- cmd/revoke_test.go | 8 +-- cmd/status.go | 7 +-- internal/sca/models/revoke.go | 10 ---- internal/sca/models/revoke_test.go | 28 ++++----- 13 files changed, 104 insertions(+), 254 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bef63fd..72fec3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ All notable changes to this project will be documented in this file. ### Fixed -- `grant revoke` now exits 1 when any requested session was not revoked instead of reporting success; check scripts relying on exit 0. +- `grant revoke` now exits 1 when the service refused, returned an unrecognized status for, or returned no result at all for any requested session, instead of reporting success; accepted-but-`in_progress` revocations still exit 0, and `--output json` gains a per-session `outcome` field — check scripts relying on exit 0. ## [0.8.0] - 2026-08-14 diff --git a/CLAUDE.md b/CLAUDE.md index e575be5..973607b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,7 +42,9 @@ Custom `SCAAccessService` follows SDK conventions: - `POST /api/access/sessions/revoke` — revoke sessions by ID (request: `sessionIds[]`, response: `SessionRevocationInfo[]`) - `sessionIds` is capped at `maxItems: 100` per request, so `cmd/revoke_batch.go` chunks the requested set into sequential ≤100-ID calls and aggregates; a mid-sequence batch error keeps the outcomes already collected - `revocationStatus`: the spec enum is only `SUCCESSFULLY_REVOKED` and `REVOCATION_IN_PROGRESS`. The live API also returns the **undocumented** `REVOCATION_NOT_APPLICABLE` (in neither the spec nor the SDK), so the status set is open and `ClassifyRevocationStatus` (`internal/sca/models/revoke.go`) **fails closed** — anything unrecognized, including `""`, is `OutcomeUnknown` and counts as a failure. Match is exact; case variants are unknown - - Outcomes are reconciled against the **requested** session IDs, never the returned rows (`cmd/revoke_reconcile.go`): a requested session with no row is `unknown`, duplicate rows resolve worst-outcome-wins, and rows for unrequested or empty IDs satisfy nothing. Exit 0 only when every requested session was accepted (`revoked` or `in_progress`); partial → exit 1 + - Outcomes are reconciled against the **requested** session IDs, never the returned rows (`cmd/revoke_reconcile.go`): a requested session with no row is `unknown`, duplicate rows resolve worst-outcome-wins, and rows for unrequested or empty IDs satisfy nothing + - **Exit policy (deliberate, not blanket "fail closed"):** exit 0 when every requested session was accepted, which *includes* `in_progress` — a documented async success state, so failing on it would make legitimate revocations look broken. Exit 1 on `not_applicable`, unrecognized statuses and missing rows. So exit 0 does not prove access is gone; only that nothing was refused or unaccounted for. State it that precisely in docs — `ClassifyRevocationStatus` fails closed, the *command* does not fail on in-progress + - `outcome` (`revoked`/`in_progress`/`not_applicable`/`unknown`) is the single classification field in the JSON. Do not add derived booleans (`accepted`, `complete`, `revoked`) beside it: two representations of one concept drift apart and disagree. Callers switch on `outcome` - Never attribute a cause for `REVOCATION_NOT_APPLICABLE` (e.g. an AWS/STS story). Observing the status does not prove the reason, and grant supports Azure/AWS/GCP plus group sessions. Render provider-neutral text and keep the raw token - `GET /api/access/{CSP}/eligibility/groups` — list eligible Entra ID groups (response: `groupId`/`groupName`/`directoryId`) - `POST /api/access/elevate/groups` — request group membership elevation (response wrapped in `response` key, same as cloud elevation) diff --git a/README.md b/README.md index 75b5f8a..af9765c 100644 --- a/README.md +++ b/README.md @@ -130,13 +130,19 @@ whatever rows the service happened to return. | Exit | Meaning | |------|---------| -| 0 | Every requested session was **accepted** for revocation. Some may still be in progress — accepted is not the same as finished, and `revoke` says which is which per session. | -| 1 | At least one requested session was not accepted, or the service returned no result for it. The full per-session breakdown is printed before the command exits. | - -A partial result exits 1 on purpose, so `grant revoke --all && echo safe` cannot -print `safe` while a session survives. With `--output json` the per-session -outcome (`revoked`, `in_progress`, `not_applicable`, `unknown`) plus the -`accepted` and `complete` flags are emitted on stdout even on exit 1. +| 0 | The service **accepted** revocation for every requested session. This includes sessions still `in_progress` — accepted is not the same as finished, and `revoke` says which is which per session. | +| 1 | At least one requested session was refused (`not_applicable`), came back with an unrecognized status, or had no result returned for it at all. The full per-session breakdown is printed before the command exits. | + +Precisely: exit 0 does **not** prove every session is gone, only that nothing was +refused, unrecognized or unaccounted for. An in-progress revocation is a +documented asynchronous success state, so it does not fail the command; run +`grant status` to see what is still live. What does fail the command is a +partial result, so `grant revoke --all && echo safe` cannot print `safe` while a +session was refused or silently dropped. + +With `--output json` each entry carries the raw `status` and a single +`outcome` field (`revoked`, `in_progress`, `not_applicable`, `unknown`), emitted +on stdout even on exit 1. ### `grant request` subcommands diff --git a/cmd/output_types.go b/cmd/output_types.go index 79a9a75..fe903f7 100644 --- a/cmd/output_types.go +++ b/cmd/output_types.go @@ -51,14 +51,15 @@ type statusOutput struct { // revocationOutput is the JSON representation of a revocation result. // There is one entry per *requested* session, in requested order, plus any // results the service returned that could not be attributed to a request. -// There is deliberately no "revoked" boolean: an in-progress revocation is -// accepted but not complete, and a boolean cannot say that. +// +// outcome is the single classification field, and deliberately not a boolean: +// an in-progress revocation is accepted but not complete, which no boolean can +// say. Derived flags are not emitted alongside it — two representations of one +// concept can drift apart and disagree. type revocationOutput struct { SessionID string `json:"sessionId"` Status string `json:"status"` // raw API value; "" when no row was returned Outcome string `json:"outcome"` // revoked | in_progress | not_applicable | unknown - Accepted bool `json:"accepted"` // the service accepted the command - Complete bool `json:"complete"` // revocation confirmed finished Reason string `json:"reason,omitempty"` // explanation when not confirmed revoked Unexpected bool `json:"unexpected,omitempty"` } diff --git a/cmd/revoke.go b/cmd/revoke.go index 6648745..9091bd5 100644 --- a/cmd/revoke.go +++ b/cmd/revoke.go @@ -4,9 +4,7 @@ import ( "context" "errors" "fmt" - "time" - "github.com/aaearon/grant-cli/internal/cache" "github.com/aaearon/grant-cli/internal/config" scamodels "github.com/aaearon/grant-cli/internal/sca/models" "github.com/aaearon/grant-cli/internal/ui" @@ -14,9 +12,18 @@ import ( "github.com/spf13/cobra" ) -// errRevocationIncomplete is returned when at least one requested session was -// not accepted for revocation. grant fails closed: a security-remediation -// command must not exit 0 while access may still be live. +// errRevocationIncomplete is returned when the service did not accept +// revocation for every requested session. +// +// Exit policy, precisely. A non-zero exit means at least one requested session +// was refused (REVOCATION_NOT_APPLICABLE), carried an unrecognized status, or +// had no result returned for it at all — unrecognized and missing statuses fail +// closed. A zero exit means every requested session was *accepted*, which +// includes REVOCATION_IN_PROGRESS: the service took the command and will act, +// but has not confirmed the access is gone. That is a documented asynchronous +// success state, so treating it as a failure would make legitimate revocations +// look broken; the per-session breakdown still distinguishes it from a +// confirmed revocation, and `grant status` shows what is still live. var errRevocationIncomplete = errors.New("not all requested sessions were revoked") // uiSessionSelector wraps ui.SelectSessions to implement sessionSelector @@ -73,13 +80,7 @@ func NewRevokeCommand() *cobra.Command { cachedLister := buildCachedLister(cfg, false, svc, nil) - // Session timestamp tracker for the best-effort expiry note (may be nil). - var tracker *cache.Store - if cacheDir, err := cache.CacheDir(); err == nil { - tracker = cache.NewStore(cacheDir, 25*time.Hour) - } - - return runRevoke(cmd, args, ispAuth, svc, cachedLister, svc, &uiSessionSelector{}, &uiConfirmPrompter{}, profile, tracker, time.Now) + return runRevoke(cmd, args, ispAuth, svc, cachedLister, svc, &uiSessionSelector{}, &uiConfirmPrompter{}, profile) }) } @@ -91,25 +92,9 @@ func NewRevokeCommandWithDeps( revoker sessionRevoker, selector sessionSelector, confirmer confirmPrompter, -) *cobra.Command { - return newRevokeCommandWithClock(auth, lister, elig, revoker, selector, confirmer, nil, time.Now) -} - -// newRevokeCommandWithClock is NewRevokeCommandWithDeps plus a session -// timestamp tracker and an injectable clock, so expiry notes are deterministic -// in tests. -func newRevokeCommandWithClock( - auth authLoader, - lister sessionLister, - elig eligibilityLister, - revoker sessionRevoker, - selector sessionSelector, - confirmer confirmPrompter, - tracker *cache.Store, - now func() time.Time, ) *cobra.Command { return newRevokeCommand(func(cmd *cobra.Command, args []string) error { - return runRevoke(cmd, args, auth, lister, elig, revoker, selector, confirmer, nil, tracker, now) + return runRevoke(cmd, args, auth, lister, elig, revoker, selector, confirmer, nil) }) } @@ -123,8 +108,6 @@ func runRevoke( selector sessionSelector, confirmer confirmPrompter, profile *sdkmodels.IdsecProfile, - tracker *cache.Store, - now func() time.Time, ) error { allFlag, _ := cmd.Flags().GetBool("all") yesFlag, _ := cmd.Flags().GetBool("yes") @@ -154,9 +137,8 @@ func runRevoke( return fmt.Errorf("not authenticated, run 'grant login' first: %w", err) } - // Determine which sessions to revoke. metadata stays empty in direct mode, - // where grant only has bare session IDs. - sessionIDs, metadata, done, err := resolveRevokeTargets(cmd, args, lister, elig, selector, confirmer, cspFilter, allFlag, yesFlag) + // Determine which sessions to revoke. + sessionIDs, done, err := resolveRevokeTargets(cmd, args, lister, elig, selector, confirmer, cspFilter, allFlag, yesFlag) if err != nil || done { return err } @@ -177,11 +159,7 @@ func runRevoke( } else { // Always print the full breakdown before returning an error, so a // non-zero exit is never opaque. - renderRevocationResults(cmd.OutOrStdout(), records, unattached, expiryHinter{ - metadata: metadata, - timestamps: sessionTimestamps(tracker), - now: now, - }) + renderRevocationResults(cmd.OutOrStdout(), records, unattached) } if revokeErr != nil { @@ -195,17 +173,8 @@ func runRevoke( return nil } -// sessionTimestamps reads local elevation timestamps, tolerating a nil tracker. -func sessionTimestamps(tracker *cache.Store) map[string]time.Time { - if tracker == nil { - return nil - } - return cache.SessionTimestamps(tracker) -} - -// resolveRevokeTargets determines the session IDs to revoke, along with session -// metadata when grant listed the sessions itself. done reports that the command -// has already finished (nothing to revoke, or the user declined). +// resolveRevokeTargets determines the session IDs to revoke. done reports that +// the command has already finished (nothing to revoke, or the user declined). func resolveRevokeTargets( cmd *cobra.Command, args []string, @@ -215,10 +184,10 @@ func resolveRevokeTargets( confirmer confirmPrompter, cspFilter *scamodels.CSP, allFlag, yesFlag bool, -) (sessionIDs []string, metadata map[string]scamodels.SessionInfo, done bool, err error) { +) (sessionIDs []string, done bool, err error) { if len(args) > 0 { - // Direct mode: session IDs provided as arguments, no metadata available. - return args, nil, false, nil + // Direct mode: session IDs provided as arguments. + return args, false, nil } // All or interactive mode: list sessions first. @@ -227,17 +196,12 @@ func resolveRevokeTargets( sessions, err := lister.ListSessions(ctx, cspFilter) if err != nil { - return nil, nil, true, fmt.Errorf("failed to list sessions: %w", err) + return nil, true, fmt.Errorf("failed to list sessions: %w", err) } if len(sessions.Response) == 0 { fmt.Fprintln(cmd.OutOrStdout(), "No active sessions to revoke.") - return nil, nil, true, nil - } - - metadata = make(map[string]scamodels.SessionInfo, len(sessions.Response)) - for _, s := range sessions.Response { - metadata[s.SessionID] = s + return nil, true, nil } selected := sessions.Response @@ -245,14 +209,14 @@ func resolveRevokeTargets( nameMap := buildWorkspaceNameMap(ctx, elig, sessions.Response) selected, err = selector.SelectSessions(sessions.Response, nameMap) if err != nil { - return nil, nil, true, fmt.Errorf("session selection failed: %w", err) + return nil, true, fmt.Errorf("session selection failed: %w", err) } // Selecting nothing is a deliberate no-op, equivalent to declining the // confirmation. Handled here rather than after the request is built, so // an empty selection can never be revoked as if it were a request. if len(selected) == 0 { fmt.Fprintln(cmd.OutOrStdout(), "No sessions selected.") - return nil, nil, true, nil + return nil, true, nil } } @@ -263,13 +227,13 @@ func resolveRevokeTargets( if !yesFlag { confirmed, cerr := confirmer.ConfirmRevocation(len(sessionIDs)) if cerr != nil { - return nil, nil, true, fmt.Errorf("confirmation failed: %w", cerr) + return nil, true, fmt.Errorf("confirmation failed: %w", cerr) } if !confirmed { fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.") - return nil, nil, true, nil + return nil, true, nil } } - return sessionIDs, metadata, false, nil + return sessionIDs, false, nil } diff --git a/cmd/revoke_outcome_test.go b/cmd/revoke_outcome_test.go index d3983c4..7bf451f 100644 --- a/cmd/revoke_outcome_test.go +++ b/cmd/revoke_outcome_test.go @@ -11,7 +11,6 @@ import ( "testing" "time" - "github.com/aaearon/grant-cli/internal/cache" scamodels "github.com/aaearon/grant-cli/internal/sca/models" authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth" commonmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/common" @@ -284,60 +283,6 @@ func TestRevokeCommand_BatchErrorKeepsEarlierOutcomes(t *testing.T) { } } -// TestRevokeCommand_ExpiryNote covers the best-effort expiry hint. The clock is -// pinned; deriving the expectation from time.Now() would straddle a minute -// boundary and flake. -func TestRevokeCommand_ExpiryNote(t *testing.T) { - elevatedAt := time.Now().Add(-20 * time.Minute) - pinned := elevatedAt.Add(20 * time.Minute) - - tracker := cache.NewStore(t.TempDir(), 25*time.Hour) - if err := cache.RecordSession(tracker, "s1", elevatedAt); err != nil { - t.Fatalf("failed to seed tracker: %v", err) - } - - lister := &mockSessionLister{sessions: &scamodels.SessionsResponse{ - Response: []scamodels.SessionInfo{ - {SessionID: "s1", CSP: scamodels.CSPAzure, WorkspaceID: "ws", RoleID: "Admin", SessionDuration: 3600}, - }, - Total: 1, - }} - revoker := &mockSessionRevoker{response: revokeResponse("s1", scamodels.RevocationNotApplicable)} - - cmd := newRevokeCommandWithClock(testAuthLoader(), lister, &mockEligibilityLister{}, revoker, - &mockSessionSelector{}, &mockConfirmPrompter{}, tracker, func() time.Time { return pinned }) - - output, err := executeCommand(cmd, "--all", "--yes") - if err == nil { - t.Fatal("expected an error for a not-applicable revocation") - } - if !strings.Contains(output, "expires in ~40m") { - t.Errorf("expected the expiry note, got:\n%s", output) - } -} - -// TestRevokeCommand_ExpiryNoteAbsentInDirectMode: direct mode has bare IDs and -// no session metadata, so no expiry can be claimed. -func TestRevokeCommand_ExpiryNoteAbsentInDirectMode(t *testing.T) { - elevatedAt := time.Now().Add(-20 * time.Minute) - tracker := cache.NewStore(t.TempDir(), 25*time.Hour) - if err := cache.RecordSession(tracker, "s1", elevatedAt); err != nil { - t.Fatalf("failed to seed tracker: %v", err) - } - - revoker := &mockSessionRevoker{response: revokeResponse("s1", scamodels.RevocationNotApplicable)} - cmd := newRevokeCommandWithClock(testAuthLoader(), &mockSessionLister{}, &mockEligibilityLister{}, revoker, - &mockSessionSelector{}, &mockConfirmPrompter{}, tracker, func() time.Time { return elevatedAt.Add(20 * time.Minute) }) - - output, err := executeCommand(cmd, "s1") - if err == nil { - t.Fatal("expected an error for a not-applicable revocation") - } - if strings.Contains(output, "expires in") { - t.Errorf("direct mode has no session metadata, so no expiry may be claimed:\n%s", output) - } -} - // TestRevokeCommand_EmptySelection: selecting nothing is a no-op, not a // revocation of nothing. It must never reach the API. func TestRevokeCommand_EmptySelection(t *testing.T) { @@ -387,8 +332,8 @@ func TestRevokeCommand_JSONOutcomes(t *testing.T) { t.Fatalf("got %d entries, want 2", len(parsed)) } for _, p := range parsed { - if p.Outcome != string(scamodels.OutcomeRevoked) || !p.Accepted || !p.Complete { - t.Errorf("entry = %+v, want outcome=revoked accepted=true complete=true", p) + if p.Outcome != string(scamodels.OutcomeRevoked) { + t.Errorf("entry = %+v, want outcome=revoked", p) } if p.Status != scamodels.RevocationSuccessful { t.Errorf("status = %q, want the raw API value", p.Status) @@ -404,9 +349,6 @@ func TestRevokeCommand_JSONOutcomes(t *testing.T) { if parsed[0].Outcome != string(scamodels.OutcomeInProgress) { t.Errorf("outcome = %q, want in_progress", parsed[0].Outcome) } - if !parsed[0].Accepted || parsed[0].Complete { - t.Errorf("entry = %+v, want accepted=true complete=false", parsed[0]) - } }, }, { @@ -418,8 +360,8 @@ func TestRevokeCommand_JSONOutcomes(t *testing.T) { if len(parsed) != 2 { t.Fatalf("got %d entries, want 2", len(parsed)) } - if parsed[1].Accepted || parsed[1].Reason == "" { - t.Errorf("entry = %+v, want accepted=false with a reason", parsed[1]) + if parsed[1].Outcome != string(scamodels.OutcomeNotApplicable) || parsed[1].Reason == "" { + t.Errorf("entry = %+v, want outcome=not_applicable with a reason", parsed[1]) } }, }, @@ -449,14 +391,11 @@ func TestRevokeCommand_JSONOutcomes(t *testing.T) { if !u.Unexpected || u.SessionID != "zz" { t.Fatalf("entry = %+v, want the unattributed zz row", u) } - // outcome and the accepted/complete axes must agree: a row grant - // never requested is not a success by any reading. + // A row grant never requested is not a success by any reading, + // whatever its raw status says. if u.Outcome != string(scamodels.OutcomeUnknown) { t.Errorf("outcome = %q, want unknown", u.Outcome) } - if u.Accepted || u.Complete { - t.Errorf("entry = %+v, want accepted=false complete=false", u) - } if u.Status != scamodels.RevocationSuccessful { t.Errorf("status = %q, want the raw API value preserved", u.Status) } @@ -471,8 +410,8 @@ func TestRevokeCommand_JSONOutcomes(t *testing.T) { if !u.Unexpected || u.SessionID != "" { t.Fatalf("entry = %+v, want the unattributable empty-ID row", u) } - if u.Outcome != string(scamodels.OutcomeUnknown) || u.Accepted || u.Complete { - t.Errorf("entry = %+v, want outcome=unknown accepted=false complete=false", u) + if u.Outcome != string(scamodels.OutcomeUnknown) { + t.Errorf("entry = %+v, want outcome=unknown", u) } }, }, @@ -516,8 +455,13 @@ func TestRevokeCommand_JSONOutcomes(t *testing.T) { if uerr := json.Unmarshal([]byte(stdout), &parsed); uerr != nil { t.Fatalf("invalid JSON on stdout: %v\n%s", uerr, stdout) } - if strings.Contains(stdout, `"revoked":true`) || strings.Contains(stdout, `"revoked": true`) { - t.Errorf("JSON must never carry a revoked boolean:\n%s", stdout) + // outcome is the single classification field. Booleans derived from + // it (a "revoked" flag, or accepted/complete axes) must not exist: + // two representations of one concept can disagree. + for _, banned := range []string{`"revoked":`, `"accepted"`, `"complete"`} { + if strings.Contains(stdout, banned) { + t.Errorf("JSON must not carry %s; outcome is the only classification field:\n%s", banned, stdout) + } } tt.check(t, parsed) }) diff --git a/cmd/revoke_reconcile.go b/cmd/revoke_reconcile.go index 040ade1..327d799 100644 --- a/cmd/revoke_reconcile.go +++ b/cmd/revoke_reconcile.go @@ -14,7 +14,6 @@ type revocationRecord struct { Status string // raw API value; "" when no row was returned Outcome scamodels.RevocationOutcome Reason string // why this is not a confirmed revocation; "" when revoked - Duplicate bool // the service returned more than one row for this ID } // unattributedResult is a returned row that cannot be attributed to a @@ -100,12 +99,9 @@ func reconcileRevocations(requested []string, results []scamodels.RevocationResu } outcome := scamodels.ClassifyRevocationStatus(r.RevocationStatus) - if seen[r.SessionID] { - records[i].Duplicate = true - // Worst outcome wins: a later success must never mask an earlier failure. - if outcomeRank(outcome) >= outcomeRank(records[i].Outcome) { - continue - } + // Worst outcome wins: a later success must never mask an earlier failure. + if seen[r.SessionID] && outcomeRank(outcome) >= outcomeRank(records[i].Outcome) { + continue } seen[r.SessionID] = true @@ -126,8 +122,9 @@ type revocationSummary struct { } // allAccepted reports whether every requested session was accepted by the -// service (revoked or in progress). An empty requested set is not a success: -// nothing was confirmed revoked. +// service, counting OutcomeInProgress as accepted. This is the exit-code +// predicate: see errRevocationIncomplete for the policy and its limits. +// An empty requested set is not a success: nothing was confirmed revoked. func (s revocationSummary) allAccepted() bool { return s.requested > 0 && s.failed == 0 } @@ -135,10 +132,10 @@ func (s revocationSummary) allAccepted() bool { func summarizeRevocations(records []revocationRecord) revocationSummary { s := revocationSummary{requested: len(records)} for _, r := range records { - switch { - case r.Outcome.Complete(): + switch r.Outcome { + case scamodels.OutcomeRevoked: s.revoked++ - case r.Outcome.Accepted(): + case scamodels.OutcomeInProgress: s.inProgress++ default: s.failed++ diff --git a/cmd/revoke_reconcile_test.go b/cmd/revoke_reconcile_test.go index 5e71a3e..5fe4868 100644 --- a/cmd/revoke_reconcile_test.go +++ b/cmd/revoke_reconcile_test.go @@ -76,7 +76,7 @@ func TestReconcileRevocations(t *testing.T) { requested: []string{"A"}, results: []scamodels.RevocationResult{ok("A"), notApplicable("A")}, wantRecords: []revocationRecord{ - {SessionID: "A", Status: scamodels.RevocationNotApplicable, Outcome: scamodels.OutcomeNotApplicable, Duplicate: true}, + {SessionID: "A", Status: scamodels.RevocationNotApplicable, Outcome: scamodels.OutcomeNotApplicable}, }, }, { @@ -84,7 +84,7 @@ func TestReconcileRevocations(t *testing.T) { requested: []string{"A"}, results: []scamodels.RevocationResult{notApplicable("A"), ok("A")}, wantRecords: []revocationRecord{ - {SessionID: "A", Status: scamodels.RevocationNotApplicable, Outcome: scamodels.OutcomeNotApplicable, Duplicate: true}, + {SessionID: "A", Status: scamodels.RevocationNotApplicable, Outcome: scamodels.OutcomeNotApplicable}, }, }, { @@ -144,11 +144,12 @@ func TestReconcileRevocations(t *testing.T) { for i, want := range tt.wantRecords { got := records[i] if got.SessionID != want.SessionID || got.Status != want.Status || - got.Outcome != want.Outcome || got.Duplicate != want.Duplicate { + got.Outcome != want.Outcome { t.Errorf("record[%d] = %+v, want %+v", i, got, want) } - if !got.Outcome.Accepted() && got.Reason == "" { - t.Errorf("record[%d] (%s) has no reason for a non-accepted outcome", i, got.SessionID) + // Anything short of a confirmed revocation must say why. + if got.Outcome != scamodels.OutcomeRevoked && got.Reason == "" { + t.Errorf("record[%d] (%s) has no reason for outcome %q", i, got.SessionID, got.Outcome) } } diff --git a/cmd/revoke_render.go b/cmd/revoke_render.go index 5a306dd..4bbf692 100644 --- a/cmd/revoke_render.go +++ b/cmd/revoke_render.go @@ -3,46 +3,10 @@ package cmd import ( "fmt" "io" - "time" scamodels "github.com/aaearon/grant-cli/internal/sca/models" ) -// expiryHinter supplies the best-effort "expires in ~Xm" note. The hint needs -// both session metadata (for the duration) and a local elevation timestamp, so -// it is unavailable in direct mode, where grant only has bare session IDs. -// -// The note is purely informational. It never explains *why* a revocation was -// refused — grant has no evidence for that. -type expiryHinter struct { - metadata map[string]scamodels.SessionInfo - timestamps map[string]time.Time - now func() time.Time -} - -// note returns a parenthesised expiry clause, or "" when it is unknown. -func (h expiryHinter) note(sessionID string) string { - if h.now == nil || len(h.metadata) == 0 || len(h.timestamps) == 0 { - return "" - } - session, ok := h.metadata[sessionID] - if !ok { - return "" - } - remaining, ok := computeRemainingTimeAt([]scamodels.SessionInfo{session}, h.timestamps, h.now())[sessionID] - if !ok { - return "" - } - if remaining <= 0 { - return " (this session has already expired)" - } - totalMin := int(remaining.Minutes()) - if totalMin >= 60 { - return fmt.Sprintf(" (this session expires in ~%dh %dm)", totalMin/60, totalMin%60) - } - return fmt.Sprintf(" (this session expires in ~%dm)", totalMin) -} - // describeRecord renders one reconciled record as a human-readable line body. func describeRecord(r revocationRecord) string { switch r.Outcome { @@ -57,9 +21,9 @@ func describeRecord(r revocationRecord) string { } } -// summaryLine states the outcome over the *requested* sessions, keeping the -// accepted-versus-complete distinction visible. It never claims that an -// accepted revocation is a finished one. +// summaryLine states the outcome over the *requested* sessions, keeping +// confirmed revocations separate from ones merely accepted. It never claims +// that an accepted revocation is a finished one. func summaryLine(s revocationSummary) string { line := fmt.Sprintf("%d of %d requested sessions revoked", s.revoked, s.requested) if s.inProgress > 0 { @@ -81,9 +45,9 @@ func plural(n int, singular, plural string) string { // renderRevocationResults prints the full per-session breakdown. It is always // called before the command returns its error, so a non-zero exit is never // opaque. -func renderRevocationResults(w io.Writer, records []revocationRecord, unattached []unattributedResult, hints expiryHinter) { +func renderRevocationResults(w io.Writer, records []revocationRecord, unattached []unattributedResult) { for _, r := range records { - fmt.Fprintf(w, " %s: %s%s\n", r.SessionID, describeRecord(r), hints.note(r.SessionID)) + fmt.Fprintf(w, " %s: %s\n", r.SessionID, describeRecord(r)) } for _, u := range unattached { @@ -108,23 +72,17 @@ func buildRevocationJSON(records []revocationRecord, unattached []unattributedRe SessionID: r.SessionID, Status: r.Status, Outcome: string(r.Outcome), - Accepted: r.Outcome.Accepted(), - Complete: r.Outcome.Complete(), Reason: r.Reason, }) } for _, u := range unattached { - // The outcome is always unknown, whatever the raw status says. A row - // grant never asked for is not a success by any reading, and reporting - // outcome "revoked" alongside accepted=false/complete=false would let - // consumers keying on outcome and consumers keying on the axes disagree. - // The raw status is preserved for the operator. + // The outcome is always unknown, whatever the raw status says: a row + // grant never asked for is not a success by any reading. The raw + // status is preserved for the operator. out = append(out, revocationOutput{ SessionID: u.SessionID, Status: u.Status, Outcome: string(scamodels.OutcomeUnknown), - Accepted: false, - Complete: false, Reason: "result was not requested and satisfies no requested session", Unexpected: true, }) diff --git a/cmd/revoke_test.go b/cmd/revoke_test.go index 7d6e60f..8241ffd 100644 --- a/cmd/revoke_test.go +++ b/cmd/revoke_test.go @@ -757,8 +757,8 @@ func TestRevokeCommand_JSONOutput(t *testing.T) { if parsed[0].Status != scamodels.RevocationSuccessful { t.Errorf("status = %q, want the raw API value", parsed[0].Status) } - if parsed[0].Outcome != string(scamodels.OutcomeRevoked) || !parsed[0].Accepted || !parsed[0].Complete { - t.Errorf("entry = %+v, want outcome=revoked accepted=true complete=true", parsed[0]) + if parsed[0].Outcome != string(scamodels.OutcomeRevoked) { + t.Errorf("entry = %+v, want outcome=revoked", parsed[0]) } } @@ -792,7 +792,7 @@ func TestRevokeCommand_JSONOutput_UndocumentedStatus(t *testing.T) { if parsed[0].Status != "Revoked" { t.Errorf("status = %q, want the raw API value preserved", parsed[0].Status) } - if parsed[0].Outcome != string(scamodels.OutcomeUnknown) || parsed[0].Accepted || parsed[0].Complete { - t.Errorf("entry = %+v, want outcome=unknown accepted=false complete=false", parsed[0]) + if parsed[0].Outcome != string(scamodels.OutcomeUnknown) { + t.Errorf("entry = %+v, want outcome=unknown", parsed[0]) } } diff --git a/cmd/status.go b/cmd/status.go index f72d915..d5527ea 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -178,16 +178,11 @@ func runStatus( // computeRemainingTime builds a sessionID -> remaining duration map from local timestamps. func computeRemainingTime(sessions []scamodels.SessionInfo, timestamps map[string]time.Time) map[string]time.Duration { - return computeRemainingTimeAt(sessions, timestamps, time.Now()) -} - -// computeRemainingTimeAt is computeRemainingTime with an explicit clock, so -// callers that need deterministic output can pin it. -func computeRemainingTimeAt(sessions []scamodels.SessionInfo, timestamps map[string]time.Time, now time.Time) map[string]time.Duration { if len(timestamps) == 0 { return nil } + now := time.Now() remaining := make(map[string]time.Duration) for _, s := range sessions { if elevatedAt, ok := timestamps[s.SessionID]; ok { diff --git a/internal/sca/models/revoke.go b/internal/sca/models/revoke.go index d036835..a2fcda7 100644 --- a/internal/sca/models/revoke.go +++ b/internal/sca/models/revoke.go @@ -51,16 +51,6 @@ func ClassifyRevocationStatus(status string) RevocationOutcome { } } -// Accepted reports whether the service accepted the revocation command. -func (o RevocationOutcome) Accepted() bool { - return o == OutcomeRevoked || o == OutcomeInProgress -} - -// Complete reports whether revocation is confirmed finished. -func (o RevocationOutcome) Complete() bool { - return o == OutcomeRevoked -} - // RevokeRequest is the request body for POST /api/access/sessions/revoke. type RevokeRequest struct { SessionIDs []string `json:"sessionIds"` diff --git a/internal/sca/models/revoke_test.go b/internal/sca/models/revoke_test.go index 1881586..c074c89 100644 --- a/internal/sca/models/revoke_test.go +++ b/internal/sca/models/revoke_test.go @@ -119,19 +119,17 @@ func TestRevokeResponse_Mixed(t *testing.T) { func TestClassifyRevocationStatus(t *testing.T) { t.Parallel() tests := []struct { - name string - status string - want RevocationOutcome - accepted bool - complete bool + name string + status string + want RevocationOutcome }{ - {"documented success", RevocationSuccessful, OutcomeRevoked, true, true}, - {"documented in progress", RevocationInProgress, OutcomeInProgress, true, false}, - {"undocumented not applicable", RevocationNotApplicable, OutcomeNotApplicable, false, false}, - {"empty status fails closed", "", OutcomeUnknown, false, false}, - {"novel status fails closed", "TOTALLY_NEW_STATUS", OutcomeUnknown, false, false}, - {"lowercase variant fails closed", "successfully_revoked", OutcomeUnknown, false, false}, - {"mixed case variant fails closed", "Revoked", OutcomeUnknown, false, false}, + {"documented success", RevocationSuccessful, OutcomeRevoked}, + {"documented in progress", RevocationInProgress, OutcomeInProgress}, + {"undocumented not applicable", RevocationNotApplicable, OutcomeNotApplicable}, + {"empty status fails closed", "", OutcomeUnknown}, + {"novel status fails closed", "TOTALLY_NEW_STATUS", OutcomeUnknown}, + {"lowercase variant fails closed", "successfully_revoked", OutcomeUnknown}, + {"mixed case variant fails closed", "Revoked", OutcomeUnknown}, } for _, tt := range tests { @@ -141,12 +139,6 @@ func TestClassifyRevocationStatus(t *testing.T) { if got != tt.want { t.Errorf("ClassifyRevocationStatus(%q) = %q, want %q", tt.status, got, tt.want) } - if got.Accepted() != tt.accepted { - t.Errorf("%q.Accepted() = %v, want %v", got, got.Accepted(), tt.accepted) - } - if got.Complete() != tt.complete { - t.Errorf("%q.Complete() = %v, want %v", got, got.Complete(), tt.complete) - } }) } }