diff --git a/src/commands/channel/channel.test.ts b/src/commands/channel/channel.test.ts index 4357864..47f5d3e 100644 --- a/src/commands/channel/channel.test.ts +++ b/src/commands/channel/channel.test.ts @@ -582,19 +582,6 @@ describe('tw channel delete', () => { expect(apiMocks.deleteChannel).not.toHaveBeenCalled() }) - it('translates a 403 from the API into a FORBIDDEN CliError', async () => { - const apiError = Object.assign(new Error('Request failed with status 403'), { - httpStatusCode: 403, - responseData: {}, - }) - apiMocks.deleteChannel.mockRejectedValueOnce(apiError) - const program = createProgram() - - await expect( - program.parseAsync(['node', 'tw', 'channel', 'delete', 'Engineering', '--yes']), - ).rejects.toMatchObject({ code: 'FORBIDDEN' }) - }) - it('outputs JSON result with --yes --json', async () => { const program = createProgram() const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) diff --git a/src/commands/channel/delete.ts b/src/commands/channel/delete.ts index dbf6c9f..7b29baf 100644 --- a/src/commands/channel/delete.ts +++ b/src/commands/channel/delete.ts @@ -6,15 +6,6 @@ import { resolveChannelRef } from '../../lib/refs.js' type DeleteChannelOptions = MutationOptions & { yes?: boolean } -function isForbidden(error: unknown): boolean { - return ( - typeof error === 'object' && - error !== null && - 'httpStatusCode' in error && - (error as { httpStatusCode: number }).httpStatusCode === 403 - ) -} - export async function deleteChannelCommand( ref: string, options: DeleteChannelOptions, @@ -42,21 +33,7 @@ export async function deleteChannelCommand( return } - try { - await deleteChannel(channel.id) - } catch (error) { - if (isForbidden(error)) { - throw new CliError( - 'FORBIDDEN', - `Twist refused to delete "${channel.name}" (id:${channel.id}): 403 Forbidden.`, - [ - 'Channel deletion is typically restricted to workspace admins', - 'Ask a workspace admin to delete it, or use the Twist web UI', - ], - ) - } - throw error - } + await deleteChannel(channel.id) if (options.json) { console.log(formatJson({ id: channel.id, deleted: true })) diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts new file mode 100644 index 0000000..3445fa0 --- /dev/null +++ b/src/lib/api.test.ts @@ -0,0 +1,88 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const sdkMocks = vi.hoisted(() => ({ + deleteChannel: vi.fn(), +})) + +vi.mock('@doist/twist-sdk', () => { + class TwistApi { + channels = { deleteChannel: sdkMocks.deleteChannel } + constructor(_token: string) {} + } + return { + TwistApi, + TwistRequestError: class TwistRequestError extends Error { + constructor( + message: string, + public httpStatusCode: number, + public responseData?: unknown, + ) { + super(message) + } + }, + } +}) + +vi.mock('./auth.js', () => ({ + getApiToken: vi.fn().mockResolvedValue('test-token'), + getAuthMetadata: vi.fn().mockResolvedValue({ authMode: 'full' }), +})) + +vi.mock('./spinner.js', () => ({ + withSpinner: (_config: unknown, fn: () => Promise) => fn(), +})) + +describe('wrapResult — central 403 translation', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.resetModules() + }) + + it('translates a plain 403 into a FORBIDDEN CliError', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + Object.assign(new Error('Request failed with status 403'), { + httpStatusCode: 403, + responseData: {}, + }), + ) + + const { createWrappedTwistClient } = await import('./api.js') + const client = createWrappedTwistClient('test-token') + + await expect(client.channels.deleteChannel(500)).rejects.toMatchObject({ + code: 'FORBIDDEN', + }) + }) + + it('prefers INSUFFICIENT_SCOPE over FORBIDDEN when error_string indicates scope', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + Object.assign(new Error('Request failed with status 403'), { + httpStatusCode: 403, + responseData: { error_string: 'Insufficient scope provided: channels:write' }, + }), + ) + + const { createWrappedTwistClient } = await import('./api.js') + const client = createWrappedTwistClient('test-token') + + await expect(client.channels.deleteChannel(500)).rejects.toMatchObject({ + code: 'INSUFFICIENT_SCOPE', + }) + }) + + it('passes non-403 errors through untranslated', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + Object.assign(new Error('Request failed with status 500'), { + httpStatusCode: 500, + responseData: {}, + }), + ) + + const { createWrappedTwistClient } = await import('./api.js') + const client = createWrappedTwistClient('test-token') + + await expect(client.channels.deleteChannel(500)).rejects.toThrow( + 'Request failed with status 500', + ) + }) +}) diff --git a/src/lib/api.ts b/src/lib/api.ts index 5c241b0..b341841 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -9,7 +9,7 @@ import { } from '@doist/twist-sdk' import { getApiToken } from './auth.js' import { getConfig, updateConfig } from './config.js' -import { CliError, isInsufficientScope } from './errors.js' +import { CliError, isForbidden, isInsufficientScope } from './errors.js' import { ensureWriteAllowed, isMutatingMethod } from './permissions.js' import { getProgressTracker } from './progress.js' import { withSpinner } from './spinner.js' @@ -194,6 +194,12 @@ function wrapResult( ['Run `tw auth login` to re-authenticate with the required scopes'], ) } + if (isForbidden(error)) { + throw new CliError('FORBIDDEN', 'Twist refused this action: 403 Forbidden.', [ + 'You may not have permission for this action — workspace admins can perform it', + 'Or re-authenticate with `tw auth login` if your scopes look wrong', + ]) + } throw error }) diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index 8823451..203123e 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -1,7 +1,7 @@ import { TwistRequestError } from '@doist/twist-sdk' import { describe, expect, it } from 'vitest' -import { isInsufficientScope } from './errors.js' +import { isForbidden, isInsufficientScope } from './errors.js' describe('isInsufficientScope', () => { it('returns true for a 403 with "Insufficient scope" error_string', () => { @@ -38,3 +38,48 @@ describe('isInsufficientScope', () => { expect(isInsufficientScope('string')).toBe(false) }) }) + +describe('isForbidden', () => { + it('returns true for any 403 — even without responseData', () => { + const error = new TwistRequestError('Request failed with status 403', 403, undefined) + expect(isForbidden(error)).toBe(true) + }) + + it('returns true for a 403 with an arbitrary error_string', () => { + const error = new TwistRequestError('Request failed with status 403', 403, { + error_code: 100, + error_string: 'Access denied', + }) + expect(isForbidden(error)).toBe(true) + }) + + it('returns false for non-403 status codes', () => { + expect(isForbidden(new TwistRequestError('Request failed with status 401', 401, {}))).toBe( + false, + ) + expect(isForbidden(new TwistRequestError('Request failed with status 404', 404, {}))).toBe( + false, + ) + expect(isForbidden(new TwistRequestError('Request failed with status 500', 500, {}))).toBe( + false, + ) + }) + + it('returns false for plain errors and non-object values', () => { + expect(isForbidden(new Error('something'))).toBe(false) + expect(isForbidden(null)).toBe(false) + expect(isForbidden(undefined)).toBe(false) + expect(isForbidden('string')).toBe(false) + }) + + // Precedence: both predicates fire on a scope 403, so callers must test + // isInsufficientScope first (the central handler in api.ts does). + it('fires alongside isInsufficientScope for an "Insufficient scope" 403', () => { + const error = new TwistRequestError('Request failed with status 403', 403, { + error_code: 109, + error_string: 'Insufficient scope provided: user:write', + }) + expect(isInsufficientScope(error)).toBe(true) + expect(isForbidden(error)).toBe(true) + }) +}) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 22b1578..0a95ef7 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -12,6 +12,7 @@ export type ErrorCode = // Auth & permissions | 'AUTH_FAILED' | 'AUTH_MIGRATION_PENDING' + | 'FORBIDDEN' | 'INSUFFICIENT_SCOPE' | 'INVALID_TOKEN' | 'NO_TOKEN' @@ -86,6 +87,21 @@ export function isInsufficientScope(error: unknown): boolean { ) } +/** + * Check whether an error is any Twist API 403 response. + * + * Precedence: callers must test `isInsufficientScope` first so OAuth-scope + * 403s keep their dedicated `INSUFFICIENT_SCOPE` code and hints; `isForbidden` + * is the catch-all fallback for plain workspace-permission 403s. + */ +export function isForbidden(error: unknown): boolean { + if (typeof error !== 'object' || error === null || !('httpStatusCode' in error)) { + return false + } + const { httpStatusCode } = error as { httpStatusCode: number } + return httpStatusCode === 403 +} + /** * Twist-flavoured CliError that preserves the historical positional * `(code, message, hints?, type?)` signature used across hundreds of call