diff --git a/AGENTS.md b/AGENTS.md index 5a133729..edf30030 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -103,6 +103,7 @@ Flags are NOT global. Each command explicitly declares only the flags it needs v - **`durationFlag`** — `--duration` / `-D`. Use for long-running subscribe/stream commands that auto-exit after N seconds. - **`rewindFlag`** — `--rewind`. Use for subscribe commands that support message replay (default: 0). - **`timeRangeFlags`** — `--start`, `--end`. Use for history and stats commands. Parse with `parseTimestamp()` from `src/utils/time.ts`. Accepts ISO 8601, Unix ms, or relative (e.g., `"1h"`, `"30m"`, `"2d"`). +- **`forceFlag`** — `--force` / `-f`. Use for destructive commands (delete, revoke) that require user confirmation. When `--force` is provided, skip the interactive prompt. When `--json` is used without `--force`, fail with an error requiring `--force`. Use `promptForConfirmation()` from `src/utils/prompt-confirmation.js` for the interactive prompt — do NOT use `interactiveHelper.confirm()` (inquirer-based, inconsistent UX). - **`endpointFlag`** — `--endpoint`. Hidden, only on `accounts login` and `accounts switch`. **When creating a new command:** @@ -290,6 +291,10 @@ When adding COMMANDS sections in `src/help.ts`, use `chalk.bold()` for headers, - `--direction`: `"Direction of message retrieval"` or `"Direction of log retrieval"`, options `["backwards", "forwards"]`. - Channels use "publish", Rooms use "send" (matches SDK terminology) - Command descriptions: imperative mood, sentence case, no trailing period (e.g., `"Subscribe to presence events on a channel"`) +- **Destructive command confirmation pattern**: Commands that perform irreversible actions (delete, revoke) must use `...forceFlag` and `promptForConfirmation()`. The pattern: + 1. If `--json` without `--force`: `this.fail("The --force flag is required when using --json to confirm ", flags, component)` + 2. If no `--force` and not JSON: show what will be affected, then call `promptForConfirmation()` for yes/no + 3. If `--force`: skip prompt, proceed directly ## Ably Knowledge diff --git a/src/base-command.ts b/src/base-command.ts index bb8d9cf1..ba68a842 100644 --- a/src/base-command.ts +++ b/src/base-command.ts @@ -13,6 +13,7 @@ import { CommandError } from "./errors/command-error.js"; import { getFriendlyAblyErrorHint } from "./utils/errors.js"; import { coreGlobalFlags } from "./flags.js"; import { InteractiveHelper } from "./services/interactive-helper.js"; +import { promptForConfirmation } from "./utils/prompt-confirmation.js"; import { BaseFlags, CommandConfig } from "./types/cli.js"; import { JsonRecordType, @@ -1348,7 +1349,7 @@ export abstract class AblyBaseCommand extends InteractiveBaseCommand { "The configured API key appears to be invalid or revoked.", ); - const shouldRemove = await this.interactiveHelper.confirm( + const shouldRemove = await promptForConfirmation( "Would you like to remove this invalid key from your configuration?", ); diff --git a/src/commands/auth/keys/revoke.ts b/src/commands/auth/keys/revoke.ts index faf2090c..99b8366f 100644 --- a/src/commands/auth/keys/revoke.ts +++ b/src/commands/auth/keys/revoke.ts @@ -5,6 +5,7 @@ import { forceFlag } from "../../../flags.js"; import { formatCapabilities } from "../../../utils/key-display.js"; import { parseKeyIdentifier } from "../../../utils/key-parsing.js"; import { formatLabel, formatResource } from "../../../utils/output.js"; +import { promptForConfirmation } from "../../../utils/prompt-confirmation.js"; export default class KeysRevokeCommand extends ControlBaseCommand { static args = { @@ -73,8 +74,8 @@ export default class KeysRevokeCommand extends ControlBaseCommand { } if (!flags.force && !this.shouldOutputJson(flags)) { - const confirmed = await this.interactiveHelper.confirm( - "This will permanently revoke this key and any applications using it will stop working. Continue?", + const confirmed = await promptForConfirmation( + "\nThis will permanently revoke this key and any applications using it will stop working. Are you sure?", ); if (!confirmed) { @@ -109,7 +110,7 @@ export default class KeysRevokeCommand extends ControlBaseCommand { // Auto-remove in JSON mode — key is already revoked, can't be used this.configManager.removeApiKey(appId); } else { - const shouldRemove = await this.interactiveHelper.confirm( + const shouldRemove = await promptForConfirmation( "The revoked key was your current key for this app. Remove it from configuration?", ); diff --git a/src/commands/auth/revoke-token.ts b/src/commands/auth/revoke-token.ts index 696233d7..85198512 100644 --- a/src/commands/auth/revoke-token.ts +++ b/src/commands/auth/revoke-token.ts @@ -1,46 +1,77 @@ -import { Args, Flags } from "@oclif/core"; -import * as Ably from "ably"; +import { Flags } from "@oclif/core"; import * as https from "node:https"; import { AblyBaseCommand } from "../../base-command.js"; -import { productApiFlags } from "../../flags.js"; +import { forceFlag, productApiFlags } from "../../flags.js"; +import { formatLabel, formatResource } from "../../utils/output.js"; +import { promptForConfirmation } from "../../utils/prompt-confirmation.js"; export default class RevokeTokenCommand extends AblyBaseCommand { - static args = { - token: Args.string({ - description: "Token to revoke", - name: "token", - required: true, - }), - }; - - static description = "Revoke a token"; + static description = "Revoke tokens by client ID or revocation key"; static examples = [ - "$ ably auth revoke-token TOKEN", - "$ ably auth revoke-token TOKEN --client-id clientid", - "$ ably auth revoke-token TOKEN --json", - "$ ably auth revoke-token TOKEN --pretty-json", + `$ ably auth revoke-token --client-id "userClientId"`, + `$ ably auth revoke-token --client-id "userClientId" --force`, + `$ ably auth revoke-token --revocation-key group1`, + `$ ably auth revoke-token --client-id "userClientId" --allow-reauth-margin`, + `$ ably auth revoke-token --client-id "userClientId" --json --force`, ]; static flags = { ...productApiFlags, + ...forceFlag, app: Flags.string({ description: "The app ID or name (defaults to current app)", env: "ABLY_APP_ID", }), - "client-id": Flags.string({ char: "c", - description: "Client ID to revoke tokens for", + description: "Revoke all tokens issued to this client ID", + exclusive: ["revocation-key"], + }), + "revocation-key": Flags.string({ + char: "r", + description: + "Revoke all tokens matching this revocation key (JWT tokens only)", + exclusive: ["client-id"], + }), + "allow-reauth-margin": Flags.boolean({ + default: false, + description: + "[default: false] Delay enforcement by 30s so connected clients can obtain a new token before disconnection.", }), }; - // Property to store the Ably client - private ablyClient?: Ably.Realtime; - async run(): Promise { - const { args, flags } = await this.parse(RevokeTokenCommand); + const { flags } = await this.parse(RevokeTokenCommand); + + const clientId = flags["client-id"]; + const revocationKey = flags["revocation-key"]; + + // Require at least one target specifier + if (!clientId && !revocationKey) { + this.fail( + "Either --client-id or --revocation-key is required. See https://ably.com/docs/auth/revocation for details.", + flags, + "revokeToken", + ); + } + + // Build target specifier + const targetSpecifier = clientId + ? `clientId:${clientId}` + : `revocationKey:${revocationKey}`; + const targetLabel = clientId ? "Client ID" : "Revocation Key"; + const targetValue = (clientId ?? revocationKey)!; + + // JSON mode guard — fail fast before config lookup + if (!flags.force && this.shouldOutputJson(flags)) { + this.fail( + "The --force flag is required when using --json to confirm revocation", + flags, + "revokeToken", + ); + } // Get app and key const appAndKey = await this.ensureAppAndKey(flags); @@ -49,51 +80,49 @@ export default class RevokeTokenCommand extends AblyBaseCommand { } const { apiKey } = appAndKey; - const { token } = args; - - try { - // Create Ably Realtime client - const client = await this.createAblyRealtimeClient(flags); - if (!client) return; - this.ablyClient = client; + // Interactive confirmation + if (!flags.force && !this.shouldOutputJson(flags)) { + this.logToStderr(`\nYou are about to revoke all tokens matching:`); + this.logToStderr( + `${formatLabel(targetLabel)} ${formatResource(targetValue)}`, + ); - const clientId = flags["client-id"] || token; + const confirmed = await promptForConfirmation( + "\nThis will permanently revoke all matching tokens, and any applications using those tokens will need to be issued new tokens. Are you sure?", + ); - if (!flags["client-id"]) { - // We need to warn the user that we're using the token as a client ID - this.logWarning( - "Revoking a specific token is only possible if it has a client ID or revocation key.", - flags, - ); - this.logWarning( - "For advanced token revocation options, see: https://ably.com/docs/auth/revocation.", - flags, - ); - this.logWarning( - "Using the token argument as a client ID for this operation.", - flags, - ); + if (!confirmed) { + this.logWarning("Revocation cancelled.", flags); + return; } + } + try { // Extract the keyName (appId.keyId) from the API key const keyParts = apiKey.split(":"); if (keyParts.length !== 2) { this.fail( "Invalid API key format. Expected format: appId.keyId:secret", flags, - "tokenRevoke", + "revokeToken", ); } - const keyName = keyParts[0]!; // This gets the appId.keyId portion + const keyName = keyParts[0]!; const secret = keyParts[1]!; - // Create the properly formatted body for token revocation - const requestBody = { - targets: [`clientId:${clientId}`], + const requestBody: Record = { + targets: [targetSpecifier], }; + let reauthNote = ""; + if (flags["allow-reauth-margin"]) { + requestBody.allowReauthMargin = true; + reauthNote = + " Connected clients have a 30s grace period to obtain new tokens before disconnection."; + } + try { // Make direct HTTPS request to Ably REST API const response = await this.makeHttpRequest( @@ -101,33 +130,38 @@ export default class RevokeTokenCommand extends AblyBaseCommand { secret, requestBody, ); + const successMessage = `Tokens matching ${targetLabel.toLowerCase()} ${formatResource(targetValue)} have been revoked.${reauthNote}`; if (this.shouldOutputJson(flags)) { this.logJsonResult( { revocation: { - message: "Token revocation processed successfully", + allowReauthMargin: flags["allow-reauth-margin"], + message: successMessage, + target: targetSpecifier, response, }, }, flags, ); } else { - this.logSuccessMessage("Token successfully revoked.", flags); + this.logSuccessMessage(successMessage, flags); } } catch (requestError: unknown) { - // Handle specific API errors const error = requestError as Error; if (error.message && error.message.includes("token_not_found")) { - this.fail("Token not found or already revoked", flags, "tokenRevoke"); + this.fail( + "No matching tokens found or already revoked", + flags, + "revokeToken", + ); } else { throw requestError; } } } catch (error) { - this.fail(error, flags, "tokenRevoke"); + this.fail(error, flags, "revokeToken"); } - // Client cleanup is handled by base class finally() method } // Helper method to make a direct HTTP request to the Ably REST API diff --git a/src/commands/bench/publisher.ts b/src/commands/bench/publisher.ts index 8ca3702c..b734f15d 100644 --- a/src/commands/bench/publisher.ts +++ b/src/commands/bench/publisher.ts @@ -6,6 +6,7 @@ import Table from "cli-table3"; import { AblyBaseCommand } from "../../base-command.js"; import { clientIdFlag, productApiFlags } from "../../flags.js"; import { errorMessage } from "../../utils/errors.js"; +import { promptForConfirmation } from "../../utils/prompt-confirmation.js"; import type { BenchPresenceData } from "../../types/bench.js"; interface TestMetrics { @@ -388,7 +389,7 @@ export default class BenchPublisher extends AblyBaseCommand { `Found ${subscribers.length} subscribers present`, ); if (subscribers.length === 0 && !this.shouldOutputJson(flags)) { - const shouldContinue = await this.interactiveHelper.confirm( + const shouldContinue = await promptForConfirmation( "No subscribers found. Continue anyway?", ); if (!shouldContinue) { diff --git a/test/unit/commands/auth/revoke-token.test.ts b/test/unit/commands/auth/revoke-token.test.ts index 82dc2dde..d0bc6da3 100644 --- a/test/unit/commands/auth/revoke-token.test.ts +++ b/test/unit/commands/auth/revoke-token.test.ts @@ -1,23 +1,21 @@ +import { Readable } from "node:stream"; + import { describe, it, expect, beforeEach, afterEach } from "vitest"; import { runCommand } from "@oclif/test"; import nock from "nock"; import { getMockConfigManager } from "../../../helpers/mock-config-manager.js"; -import { getMockAblyRealtime } from "../../../helpers/mock-ably-realtime.js"; import { standardHelpTests, - standardArgValidationTests, standardFlagTests, } from "../../../helpers/standard-tests.js"; import { parseNdjsonLines } from "../../../helpers/ndjson.js"; describe("auth:revoke-token command", () => { - const mockToken = "test-token-12345"; const mockClientId = "test-client-id"; + const mockRevocationKey = "group1"; beforeEach(() => { nock.cleanAll(); - // Initialize the mock (command creates one but doesn't use it for HTTP) - getMockAblyRealtime(); }); afterEach(() => { @@ -26,15 +24,43 @@ describe("auth:revoke-token command", () => { standardHelpTests("auth:revoke-token", import.meta.url); - standardArgValidationTests("auth:revoke-token", import.meta.url, { - requiredArgs: ["test-token"], + describe("flag validation", () => { + it("should fail when neither --client-id nor --revocation-key is provided", async () => { + const { error } = await runCommand( + ["auth:revoke-token", "--force"], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toContain( + "Either --client-id or --revocation-key is required", + ); + }); + + it("should fail when both --client-id and --revocation-key are provided", async () => { + const { error } = await runCommand( + [ + "auth:revoke-token", + "--client-id", + mockClientId, + "--revocation-key", + mockRevocationKey, + "--force", + ], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toMatch( + /cannot also be provided when using.*--client-id|cannot also be provided when using.*--revocation-key/i, + ); + }); }); - describe("token revocation", () => { - it("should successfully revoke a token with client-id", async () => { + describe("token revocation by client ID", () => { + it("should successfully revoke tokens for a client ID", async () => { const mockConfig = getMockConfigManager(); const keyId = mockConfig.getKeyId()!; - // Mock the token revocation endpoint nock("https://rest.ably.io") .post(`/keys/${keyId}/revokeTokens`, { targets: [`clientId:${mockClientId}`], @@ -42,34 +68,35 @@ describe("auth:revoke-token command", () => { .reply(200, {}); const { stderr } = await runCommand( - ["auth:revoke-token", mockToken, "--client-id", mockClientId], + ["auth:revoke-token", "--client-id", mockClientId, "--force"], import.meta.url, ); - expect(stderr).toContain("Token successfully revoked"); + expect(stderr).toContain("have been revoked"); }); - it("should use token as client-id when --client-id not provided", async () => { + it("should include allowReauthMargin when --allow-reauth-margin is provided", async () => { const mockConfig = getMockConfigManager(); const keyId = mockConfig.getKeyId()!; - // When no client-id is provided, the token is used as the client-id nock("https://rest.ably.io") .post(`/keys/${keyId}/revokeTokens`, { - targets: [`clientId:${mockToken}`], + targets: [`clientId:${mockClientId}`], + allowReauthMargin: true, }) .reply(200, {}); const { stderr } = await runCommand( - ["auth:revoke-token", mockToken], + [ + "auth:revoke-token", + "--client-id", + mockClientId, + "--allow-reauth-margin", + "--force", + ], import.meta.url, ); - // Should show warnings about using token as client-id - expect(stderr).toContain( - "Revoking a specific token is only possible if it has a client ID", - ); - expect(stderr).toContain("Using the token argument as a client ID"); - expect(stderr).toContain("Token successfully revoked"); + expect(stderr).toContain("have been revoked"); }); it("should output JSON format when --json flag is used", async () => { @@ -82,7 +109,7 @@ describe("auth:revoke-token command", () => { .reply(200, { issuedBefore: 1234567890 }); const { stdout } = await runCommand( - ["auth:revoke-token", mockToken, "--client-id", mockClientId, "--json"], + ["auth:revoke-token", "--client-id", mockClientId, "--json", "--force"], import.meta.url, ); @@ -91,26 +118,51 @@ describe("auth:revoke-token command", () => { expect(result).toHaveProperty("revocation"); expect(result.revocation).toHaveProperty( "message", - "Token revocation processed successfully", + `Tokens matching client id ${mockClientId} have been revoked.`, + ); + expect(result.revocation).toHaveProperty( + "target", + `clientId:${mockClientId}`, ); expect(result.revocation).toHaveProperty("response"); }); + }); - it("should handle token not found error with special message", async () => { + describe("token revocation by revocation key", () => { + it("should successfully revoke tokens for a revocation key", async () => { + const mockConfig = getMockConfigManager(); + const keyId = mockConfig.getKeyId()!; + nock("https://rest.ably.io") + .post(`/keys/${keyId}/revokeTokens`, { + targets: [`revocationKey:${mockRevocationKey}`], + }) + .reply(200, {}); + + const { stderr } = await runCommand( + ["auth:revoke-token", "--revocation-key", mockRevocationKey, "--force"], + import.meta.url, + ); + + expect(stderr).toContain("have been revoked"); + }); + }); + + describe("error handling", () => { + it("should handle token not found error", async () => { const mockConfig = getMockConfigManager(); const keyId = mockConfig.getKeyId()!; - // The command handles token_not_found specifically in the response body nock("https://rest.ably.io") .post(`/keys/${keyId}/revokeTokens`) .reply(404, "token_not_found"); const { error } = await runCommand( - ["auth:revoke-token", mockToken, "--client-id", mockClientId], + ["auth:revoke-token", "--client-id", mockClientId, "--force"], import.meta.url, ); - // Command outputs error via fail - expect(error?.message).toContain("Token not found or already revoked"); + expect(error?.message).toContain( + "No matching tokens found or already revoked", + ); }); it("should handle authentication error (invalid API key)", async () => { @@ -121,7 +173,7 @@ describe("auth:revoke-token command", () => { .reply(401, { error: { message: "Unauthorized" } }); const { error } = await runCommand( - ["auth:revoke-token", mockToken, "--client-id", mockClientId], + ["auth:revoke-token", "--client-id", mockClientId, "--force"], import.meta.url, ); @@ -137,7 +189,7 @@ describe("auth:revoke-token command", () => { .reply(500, { error: "Internal Server Error" }); const { error } = await runCommand( - ["auth:revoke-token", mockToken, "--client-id", mockClientId], + ["auth:revoke-token", "--client-id", mockClientId, "--force"], import.meta.url, ); @@ -146,8 +198,65 @@ describe("auth:revoke-token command", () => { }); }); + describe("confirmation prompt", () => { + const originalStdin = process.stdin; + + function mockStdinAnswer(answer: string) { + const readable = new Readable({ read() {} }); + Object.defineProperty(process, "stdin", { + value: readable, + writable: true, + configurable: true, + }); + queueMicrotask(() => { + for (const chunk of [`${answer}\n`, null]) readable.push(chunk); + }); + } + + afterEach(() => { + Object.defineProperty(process, "stdin", { + value: originalStdin, + writable: true, + configurable: true, + }); + }); + + it("should require --force in JSON mode", async () => { + const { stdout } = await runCommand( + ["auth:revoke-token", "--client-id", mockClientId, "--json"], + import.meta.url, + ); + + const lines = parseNdjsonLines(stdout); + const errorLine = lines.find((r) => r.type === "error"); + expect(errorLine).toBeDefined(); + expect(errorLine!.error.message).toContain( + "The --force flag is required when using --json to confirm revocation", + ); + }); + + it("should cancel when user declines confirmation", async () => { + mockStdinAnswer("n"); + + const revokeNock = nock("https://rest.ably.io") + .post(/\/keys\/.*\/revokeTokens/) + .reply(200, {}); + + const { stderr } = await runCommand( + ["auth:revoke-token", "--client-id", mockClientId], + import.meta.url, + ); + + expect(stderr).toContain("Revocation cancelled"); + expect(revokeNock.isDone()).toBe(false); + }); + }); + standardFlagTests("auth:revoke-token", import.meta.url, [ "--client-id", + "--revocation-key", + "--allow-reauth-margin", "--json", + "--force", ]); });