diff --git a/CHANGELOG.md b/CHANGELOG.md index de699d0..72fec3a 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 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 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index b106ab7..973607b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,6 +40,12 @@ 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 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) - **Headers:** `Authorization: Bearer {jwt}`, `X-API-Version: 2.0`, `Content-Type: application/json` diff --git a/README.md b/README.md index ee4e54a..af9765c 100644 --- a/README.md +++ b/README.md @@ -118,11 +118,32 @@ 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 | 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 | Subcommand | Description | @@ -183,6 +204,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..fe903f7 100644 --- a/cmd/output_types.go +++ b/cmd/output_types.go @@ -49,9 +49,19 @@ 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. +// +// 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"` + 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 + 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..9091bd5 100644 --- a/cmd/revoke.go +++ b/cmd/revoke.go @@ -12,6 +12,20 @@ import ( "github.com/spf13/cobra" ) +// 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 type uiSessionSelector struct{} @@ -123,94 +137,103 @@ 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. + sessionIDs, done, err := resolveRevokeTargets(cmd, args, lister, elig, selector, confirmer, cspFilter, allFlag, yesFlag) + if err != nil || done { + return err + } - 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() + // 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) - sessions, err := lister.ListSessions(ctx, cspFilter) - if err != nil { - return fmt.Errorf("failed to list sessions: %w", err) - } + // A failing batch still returns the results already collected. + results, revokeErr := revokeInBatches(context.Background(), revoker, sessionIDs) - if len(sessions.Response) == 0 { - fmt.Fprintln(cmd.OutOrStdout(), "No active sessions to revoke.") - return nil - } + records, unattached := reconcileRevocations(sessionIDs, results) - 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 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) + } + + if revokeErr != nil { + return revokeErr + } + + if summary := summarizeRevocations(records); !summary.allAccepted() { + return fmt.Errorf("%w: %s", errRevocationIncomplete, summaryLine(summary)) } - // Call revoke API + return nil +} + +// 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, + lister sessionLister, + elig eligibilityLister, + selector sessionSelector, + confirmer confirmPrompter, + cspFilter *scamodels.CSP, + allFlag, yesFlag bool, +) (sessionIDs []string, done bool, err error) { + if len(args) > 0 { + // Direct mode: session IDs provided as arguments. + return args, 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, 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, true, nil + } + + selected := sessions.Response + if !allFlag { + nameMap := buildWorkspaceNameMap(ctx, elig, sessions.Response) + selected, err = selector.SelectSessions(sessions.Response, nameMap) + if err != nil { + 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, true, nil } - 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, true, fmt.Errorf("confirmation failed: %w", cerr) + } + if !confirmed { + fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.") + return nil, true, nil + } + } + + return sessionIDs, 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..7bf451f --- /dev/null +++ b/cmd/revoke_outcome_test.go @@ -0,0 +1,469 @@ +// 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" + + 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_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 + 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) { + t.Errorf("entry = %+v, want outcome=revoked", 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) + } + }, + }, + { + 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].Outcome != string(scamodels.OutcomeNotApplicable) || parsed[1].Reason == "" { + t.Errorf("entry = %+v, want outcome=not_applicable 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: "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) + } + // 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.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) { + t.Errorf("entry = %+v, want outcome=unknown", u) + } + }, + }, + { + 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) + } + // 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 new file mode 100644 index 0000000..327d799 --- /dev/null +++ b/cmd/revoke_reconcile.go @@ -0,0 +1,145 @@ +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 +} + +// 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) + // 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 + + 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, 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 +} + +func summarizeRevocations(records []revocationRecord) revocationSummary { + s := revocationSummary{requested: len(records)} + for _, r := range records { + switch r.Outcome { + case scamodels.OutcomeRevoked: + s.revoked++ + case scamodels.OutcomeInProgress: + 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..5fe4868 --- /dev/null +++ b/cmd/revoke_reconcile_test.go @@ -0,0 +1,237 @@ +// 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}, + }, + }, + { + 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}, + }, + }, + { + 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 { + t.Errorf("record[%d] = %+v, want %+v", i, got, want) + } + // 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) + } + } + + 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..4bbf692 --- /dev/null +++ b/cmd/revoke_render.go @@ -0,0 +1,91 @@ +package cmd + +import ( + "fmt" + "io" + + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +// 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 +// 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 { + 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) { + for _, r := range records { + fmt.Fprintf(w, " %s: %s\n", r.SessionID, describeRecord(r)) + } + + 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), + 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. The raw + // status is preserved for the operator. + out = append(out, revocationOutput{ + SessionID: u.SessionID, + Status: u.Status, + Outcome: string(scamodels.OutcomeUnknown), + 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..8241ffd 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) { + t.Errorf("entry = %+v, want outcome=revoked", 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) { + t.Errorf("entry = %+v, want outcome=unknown", parsed[0]) } } 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..a2fcda7 100644 --- a/internal/sca/models/revoke.go +++ b/internal/sca/models/revoke.go @@ -2,11 +2,55 @@ 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 + } +} + // 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..c074c89 100644 --- a/internal/sca/models/revoke_test.go +++ b/internal/sca/models/revoke_test.go @@ -116,6 +116,41 @@ func TestRevokeResponse_Mixed(t *testing.T) { } } +func TestClassifyRevocationStatus(t *testing.T) { + t.Parallel() + tests := []struct { + name string + status string + want RevocationOutcome + }{ + {"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 { + 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) + } + }) + } +} + +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{