diff --git a/src/commands/channel/channel.test.ts b/src/commands/channel/channel.test.ts index 4d7c2f2..890718f 100644 --- a/src/commands/channel/channel.test.ts +++ b/src/commands/channel/channel.test.ts @@ -3,7 +3,6 @@ import { createTestProgram, describeEmptyMachineOutput, } from '@doist/cli-core/testing' -import { CommsRequestError } from '@doist/comms-sdk' import { beforeEach, describe, expect, it, vi } from 'vitest' const apiMocks = vi.hoisted(() => ({ @@ -701,19 +700,6 @@ describe('channels delete', () => { expect(apiMocks.getCurrentWorkspaceId).not.toHaveBeenCalled() }) - it('translates a 403 from the API into a FORBIDDEN CliError', async () => { - const client = createClient() - client.channels.deleteChannel.mockRejectedValueOnce( - new CommsRequestError('Request failed with status 403', 403, {}), - ) - apiMocks.getCommsClient.mockResolvedValue(client) - - await expect(runChannelCommand(['delete', 'Engineering', '--yes'])).rejects.toHaveProperty( - 'code', - 'FORBIDDEN', - ) - }) - it('outputs JSON result with --yes --json', async () => { const client = createClient() apiMocks.getCommsClient.mockResolvedValue(client) diff --git a/src/commands/channel/delete.ts b/src/commands/channel/delete.ts index f4e1761..aaa47b7 100644 --- a/src/commands/channel/delete.ts +++ b/src/commands/channel/delete.ts @@ -1,4 +1,3 @@ -import { CommsRequestError } from '@doist/comms-sdk' import { getCommsClient } from '../../lib/api.js' import { CliError } from '../../lib/errors.js' import type { MutationOptions } from '../../lib/options.js' @@ -31,21 +30,7 @@ export async function deleteChannel(ref: string, options: DeleteChannelOptions): } const client = await getCommsClient() - try { - await client.channels.deleteChannel(channel.id) - } catch (error) { - if (error instanceof CommsRequestError && error.httpStatusCode === 403) { - throw new CliError( - 'FORBIDDEN', - `Comms 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 Comms web UI', - ], - ) - } - throw error - } + await client.channels.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 index 6bd618f..92d51a1 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -1,24 +1,42 @@ +import { CommsRequestError } from '@doist/comms-sdk' import { beforeEach, describe, expect, it, vi } from 'vitest' -// Mock the SDK so we can observe how `getWorkspaceUsers` invokes -// `client.workspaceUsers.getWorkspaceUsers`. The real filtering of -// removed users lives in the SDK (≥0.3.0), so the contract this test -// guards is "we pass `includeRemoved` through unchanged." +// Hoisted mocks — shared across both describe blocks. const getWorkspaceUsersMock = vi.hoisted(() => vi.fn().mockResolvedValue([])) +const sdkMocks = vi.hoisted(() => ({ + deleteChannel: vi.fn(), +})) -vi.mock('@doist/comms-sdk', () => ({ - CommsApi: class { +vi.mock('@doist/comms-sdk', () => { + class CommsApi { + channels = { deleteChannel: sdkMocks.deleteChannel } workspaceUsers = { getWorkspaceUsers: getWorkspaceUsersMock } - }, -})) + constructor(_token?: string) {} + } + return { + CommsApi, + CommsRequestError: class CommsRequestError 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' }), })) +// `channels.deleteChannel` is the only mutating method the 403-translation +// tests exercise; everything else (getWorkspaceUsers) stays on the read path. vi.mock('./permissions.js', () => ({ - ensureWriteAllowed: vi.fn(), - isMutatingMethod: vi.fn().mockReturnValue(false), + ensureWriteAllowed: vi.fn().mockResolvedValue(undefined), + isMutatingMethod: vi.fn((path: string) => path === 'channels.deleteChannel'), })) vi.mock('./spinner.js', () => ({ @@ -29,7 +47,8 @@ vi.mock('./progress.js', () => ({ getProgressTracker: () => ({ isEnabled: () => false, emitApiCall: vi.fn() }), })) -const { clearWorkspaceUserCache, getWorkspaceUsers } = await import('./api.js') +const { clearWorkspaceUserCache, createWrappedCommsClient, getWorkspaceUsers } = + await import('./api.js') describe('getWorkspaceUsers', () => { beforeEach(() => { @@ -73,3 +92,50 @@ describe('getWorkspaceUsers', () => { expect(getWorkspaceUsersMock).toHaveBeenCalledTimes(2) }) }) + +// ─── wrapResult — central 403 translation ──────────────────────────────────── + +describe('wrapResult — central 403 translation', () => { + beforeEach(() => { + sdkMocks.deleteChannel.mockReset() + }) + + it('translates a plain 403 into a FORBIDDEN CliError', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 403', 403, {}), + ) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({ + code: 'FORBIDDEN', + message: 'Comms refused this action: 403 Forbidden.', + hints: [ + 'You may not have permission for this action', + 'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong', + ], + }) + }) + + it('prefers INSUFFICIENT_SCOPE over FORBIDDEN when error_string indicates scope', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 403', 403, { + error_string: 'Insufficient scope provided: channels:write', + }), + ) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({ + code: 'INSUFFICIENT_SCOPE', + message: 'This action requires permissions your current token does not have.', + hints: ['Run `tdc auth login` to re-authenticate with the required scopes'], + }) + }) + + it('passes non-403 errors through untranslated', async () => { + const originalError = new CommsRequestError('Request failed with status 500', 500, {}) + sdkMocks.deleteChannel.mockRejectedValueOnce(originalError) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('CH500')).rejects.toBe(originalError) + }) +}) diff --git a/src/lib/api.ts b/src/lib/api.ts index ee54e3c..7cf4275 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -7,7 +7,7 @@ import { } from '@doist/comms-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' @@ -195,6 +195,12 @@ function wrapResult( ['Run `tdc auth login` to re-authenticate with the required scopes'], ) } + if (isForbidden(error)) { + throw new CliError('FORBIDDEN', 'Comms refused this action: 403 Forbidden.', [ + 'You may not have permission for this action', + 'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong', + ]) + } throw error }) diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index ee83e00..d383f68 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -1,7 +1,7 @@ import { CommsRequestError } from '@doist/comms-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 a 403 with undefined responseData', () => { + const error = new CommsRequestError('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 CommsRequestError('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 CommsRequestError('Request failed with status 401', 401, {}))).toBe( + false, + ) + expect(isForbidden(new CommsRequestError('Request failed with status 404', 404, {}))).toBe( + false, + ) + expect(isForbidden(new CommsRequestError('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) + }) + + // Exclusive with isInsufficientScope: a scope 403 must NOT also classify as + // a plain FORBIDDEN, so callers can check the two predicates in any order. + it('returns false for an "Insufficient scope" 403 (exclusive with isInsufficientScope)', () => { + const error = new CommsRequestError('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(false) + }) +}) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index fea38fd..4f9bd77 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -62,30 +62,43 @@ export type ErrorCode = // Escape hatch for dynamic codes | (string & {}) +function hasCommsStatusCode(error: unknown, status: number): error is { httpStatusCode: number } { + return ( + typeof error === 'object' && + error !== null && + 'httpStatusCode' in error && + typeof error.httpStatusCode === 'number' && + error.httpStatusCode === status + ) +} + /** * Check whether an error is a Comms API 403 "Insufficient scope" response. * Works with any error shaped like CommsRequestError (httpStatusCode + responseData). */ export function isInsufficientScope(error: unknown): boolean { - if ( - typeof error !== 'object' || - error === null || - !('httpStatusCode' in error) || - !('responseData' in error) - ) { - return false - } - const { httpStatusCode, responseData } = error as { - httpStatusCode: number - responseData: { error_string?: string } | undefined - } + if (!hasCommsStatusCode(error, 403)) return false + if (!('responseData' in error)) return false + const data = error.responseData return ( - httpStatusCode === 403 && - typeof responseData?.error_string === 'string' && - responseData.error_string.includes('Insufficient scope') + typeof data === 'object' && + data !== null && + 'error_string' in data && + typeof data.error_string === 'string' && + data.error_string.includes('Insufficient scope') ) } +/** + * Check whether an error is a plain workspace-permission 403 — i.e. a 403 that + * is NOT an OAuth-scope failure. Exclusive with `isInsufficientScope` so the + * two predicates can be checked in any order without downgrading a scope error + * to a generic `FORBIDDEN`. + */ +export function isForbidden(error: unknown): boolean { + return hasCommsStatusCode(error, 403) && !isInsufficientScope(error) +} + /** * Comms-flavoured CliError that preserves the historical positional * `(code, message, hints?, type?)` signature used across hundreds of call