diff --git a/packages/core/src/helpers/oauth-request.test.ts b/packages/core/src/helpers/oauth-request.test.ts index 210573b8c..8234a9c49 100644 --- a/packages/core/src/helpers/oauth-request.test.ts +++ b/packages/core/src/helpers/oauth-request.test.ts @@ -17,7 +17,6 @@ import { doOAuthRequest, exchangeAuthorizationCode, getDatadogOAuthConfig, - getOAuthToken, readOAuthTokenFromKeychain, resolveOAuthToken, validateOAuthCallback, @@ -98,6 +97,14 @@ jest.mock('oauth4webapi', () => { }; }); +jest.mock('child_process', () => ({ + ...jest.requireActual('child_process'), + spawn: jest.fn(() => ({ + once: jest.fn(), + unref: jest.fn(), + })), +})); + jest.mock('@napi-rs/keyring', () => ({ AsyncEntry: class { private readonly key: string; @@ -172,6 +179,7 @@ describe('Core - OAuth', () => { afterEach(() => { nock.cleanAll(); nock.disableNetConnect(); + jest.restoreAllMocks(); }); describe('getDatadogOAuthConfig', () => { @@ -289,44 +297,20 @@ describe('Core - OAuth', () => { expect(token.expiresAt).toEqual(expect.any(Number)); }); - test('Should use cached access token when it is still valid', async () => { - await writeOAuthTokenToKeychain( - 'datadoghq.com', - { - accessToken: 'cached-token', - clientId: 'client-id', - expiresAt: Date.now() + 60 * 60 * 1000, - refreshToken: 'refresh-token', - site: 'datadoghq.com', - tokenType: 'bearer', - }, - createOAuthConfig(), - ); - - const token = await getOAuthToken('datadoghq.com', createOAuthConfig(), getMockLogger()); - - expect(token).toEqual({ - accessToken: 'cached-token', - expiresAt: expect.any(Number), - refreshToken: 'refresh-token', - site: 'datadoghq.com', - tokenType: 'bearer', - }); - }); - - test('Should refresh cached token when it is expired', async () => { + test('Should refresh and store cached token when it is expired and the request succeeds', async () => { const bodies: unknown[] = []; + const config = getDatadogOAuthConfig('datadoghq.com'); await writeOAuthTokenToKeychain( 'datadoghq.com', { accessToken: 'expired-token', - clientId: 'client-id', + clientId: config.clientId, expiresAt: Date.now() - 1000, refreshToken: 'old-refresh-token', site: 'datadoghq.com', tokenType: 'bearer', }, - createOAuthConfig(), + config, ); const scope = nock('https://api.datadoghq.com') .post('/oauth2/v1/token', (body) => { @@ -338,24 +322,26 @@ describe('Core - OAuth', () => { expires_in: 3600, token_type: 'Bearer', }); - - const token = await getOAuthToken('datadoghq.com', createOAuthConfig(), getMockLogger()); - const cachedToken = await readOAuthTokenFromKeychain('datadoghq.com', createOAuthConfig()); + const requestScope = nock('https://api.datadoghq.com') + .matchHeader('authorization', 'Bearer refreshed-token') + .get('/test') + .reply(200, 'ok'); + + const result = await doOAuthRequest({ + url: 'https://api.datadoghq.com/test', + auth: { site: 'datadoghq.com' }, + log: getMockLogger(), + }); + const cachedToken = await readOAuthTokenFromKeychain('datadoghq.com', config); expect(scope.isDone()).toBe(true); + expect(requestScope.isDone()).toBe(true); expect(normalizeFormBody(bodies[0])).toEqual({ - client_id: 'client-id', + client_id: config.clientId, grant_type: 'refresh_token', refresh_token: 'old-refresh-token', }); - expect(token).toEqual( - expect.objectContaining({ - accessToken: 'refreshed-token', - refreshToken: 'old-refresh-token', - site: 'datadoghq.com', - tokenType: 'bearer', - }), - ); + expect(result).toBe('ok'); expect(cachedToken).toEqual( expect.objectContaining({ accessToken: 'refreshed-token', @@ -389,6 +375,54 @@ describe('Core - OAuth', () => { ).resolves.toBeUndefined(); }); + test('Should delete malformed tokens from the OS credential store', async () => { + await writeOAuthTokenToKeychain( + 'datadoghq.com', + { + accessToken: 'cached-token', + clientId: 'client-id', + site: 'datadoghq.com', + }, + createOAuthConfig(), + ); + const [key] = mockKeyringStore.keys(); + mockKeyringStore.set(key, '{bad-json'); + + await expect( + readOAuthTokenFromKeychain('datadoghq.com', createOAuthConfig()), + ).resolves.toBeUndefined(); + expect(mockKeyringStore.size).toBe(0); + }); + + test('Should delete cached tokens with invalid shape from the OS credential store', async () => { + await writeOAuthTokenToKeychain( + 'datadoghq.com', + { + accessToken: 'cached-token', + clientId: 'client-id', + site: 'datadoghq.com', + }, + createOAuthConfig(), + ); + const [key] = mockKeyringStore.keys(); + mockKeyringStore.set( + key, + JSON.stringify({ + version: 1, + token: { + accessToken: '', + clientId: 'client-id', + site: 'datadoghq.com', + }, + }), + ); + + await expect( + readOAuthTokenFromKeychain('datadoghq.com', createOAuthConfig()), + ).resolves.toBeUndefined(); + expect(mockKeyringStore.size).toBe(0); + }); + test('Should authorize with PKCE using a local callback', async () => { const port = 18060; const redirectUri = `http://127.0.0.1:${port}`; @@ -420,6 +454,80 @@ describe('Core - OAuth', () => { expect(scope.isDone()).toBe(true); }); + test('Should store a new OAuth token after the OAuth request succeeds', async () => { + const site = 'ap2.datadoghq.com'; + const redirectUri = DEFAULT_OAUTH_REDIRECT_URI; + const authorizationUrlLogger = createAuthorizationUrlLogger(); + const logger = getMockLogger({ info: authorizationUrlLogger.info }); + const config = getDatadogOAuthConfig(site); + nock.enableNetConnect('localhost'); + const tokenScope = nock(`https://api.${site}`).post('/oauth2/v1/token').reply(200, { + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + token_type: 'Bearer', + }); + const requestScope = nock(`https://api.${site}`) + .matchHeader('authorization', 'Bearer new-access-token') + .get('/test') + .reply(200, 'ok'); + + const requestPromise = doOAuthRequest({ + url: `https://api.${site}/test`, + auth: { site }, + log: logger, + }); + requestPromise.catch(authorizationUrlLogger.reject); + + const authorizeUrl = await authorizationUrlLogger.url; + const response = await fetch( + `${redirectUri}?code=code&state=${authorizeUrl.searchParams.get('state')}`, + ); + expect(response.ok).toBe(true); + + await expect(requestPromise).resolves.toBe('ok'); + await expect(readOAuthTokenFromKeychain(site, config)).resolves.toMatchObject({ + accessToken: 'new-access-token', + refreshToken: 'new-refresh-token', + site, + }); + expect(tokenScope.isDone()).toBe(true); + expect(requestScope.isDone()).toBe(true); + }); + + test('Should not store a new OAuth token when the OAuth request is rejected', async () => { + const site = 'us5.datadoghq.com'; + const redirectUri = DEFAULT_OAUTH_REDIRECT_URI; + const authorizationUrlLogger = createAuthorizationUrlLogger(); + const logger = getMockLogger({ info: authorizationUrlLogger.info }); + const config = getDatadogOAuthConfig(site); + nock.enableNetConnect('localhost'); + const tokenScope = nock(`https://api.${site}`) + .post('/oauth2/v1/token') + .reply(200, { access_token: 'rejected-token', token_type: 'Bearer' }); + const requestScope = nock(`https://api.${site}`) + .matchHeader('authorization', 'Bearer rejected-token') + .get('/test') + .reply(401, { errors: [{ title: 'Unauthorized' }] }); + + const requestPromise = doOAuthRequest({ + url: `https://api.${site}/test`, + auth: { site }, + log: logger, + }); + requestPromise.catch(authorizationUrlLogger.reject); + + const authorizeUrl = await authorizationUrlLogger.url; + const response = await fetch( + `${redirectUri}?code=code&state=${authorizeUrl.searchParams.get('state')}`, + ); + expect(response.ok).toBe(true); + + await expect(requestPromise).rejects.toThrow('HTTP 401'); + await expect(readOAuthTokenFromKeychain(site, config)).resolves.toBeUndefined(); + expect(tokenScope.isDone()).toBe(true); + expect(requestScope.isDone()).toBe(true); + }); + test('Should resolve an OAuth token and pass it to doRequest.', async () => { const site = 'ap1.datadoghq.com'; const config = getDatadogOAuthConfig(site); @@ -450,23 +558,49 @@ describe('Core - OAuth', () => { }), }), ); + fetchMock.mockRestore(); }); - test('Should throw when OAuth token resolution does not return an access token.', async () => { - const site = 'ap2.datadoghq.com'; + test('Should delete cached OAuth tokens when an OAuth request is rejected', async () => { + const site = 'us2.ddog-gov.com'; const config = getDatadogOAuthConfig(site); await writeOAuthTokenToKeychain( site, { - accessToken: '', + accessToken: 'cached-token', clientId: config.clientId, site, }, config, ); - const fetchMock = jest - .spyOn(global, 'fetch') - .mockImplementation(() => Promise.resolve(new Response('{}'))); + const rejectedScope = nock(`https://api.${site}`) + .matchHeader('authorization', 'Bearer cached-token') + .get('/test') + .reply(403, { errors: [{ title: 'Forbidden' }] }); + + await expect( + doOAuthRequest({ + url: `https://api.${site}/test`, + auth: { site }, + log: getMockLogger(), + }), + ).rejects.toThrow('HTTP 403'); + await expect(readOAuthTokenFromKeychain(site, config)).resolves.toBeUndefined(); + expect(rejectedScope.isDone()).toBe(true); + + await writeOAuthTokenToKeychain( + site, + { + accessToken: 'replacement-token', + clientId: config.clientId, + site, + }, + config, + ); + const acceptedScope = nock(`https://api.${site}`) + .matchHeader('authorization', 'Bearer replacement-token') + .get('/test') + .reply(200, 'ok'); await expect( doOAuthRequest({ @@ -474,7 +608,7 @@ describe('Core - OAuth', () => { auth: { site }, log: getMockLogger(), }), - ).rejects.toThrow('OAuth authentication did not return an access token.'); - expect(fetchMock).not.toHaveBeenCalled(); + ).resolves.toBe('ok'); + expect(acceptedScope.isDone()).toBe(true); }); }); diff --git a/packages/core/src/helpers/oauth-request.ts b/packages/core/src/helpers/oauth-request.ts index c2680cd4f..57664adcb 100644 --- a/packages/core/src/helpers/oauth-request.ts +++ b/packages/core/src/helpers/oauth-request.ts @@ -9,7 +9,7 @@ import http from 'http'; import type { AuthOptionsWithDefaults, Logger, RequestOpts } from '../types'; -import { doRequest } from './request'; +import { RequestError, doRequest } from './request'; const KEYRING_PACKAGE_NAME = '@napi-rs/keyring'; @@ -381,6 +381,10 @@ const getErrorCode = (error: unknown) => const getErrorMessage = (error: unknown) => error instanceof Error ? error.message : String(error); +const isOAuthAuthError = (error: unknown) => { + return error instanceof RequestError && (error.statusCode === 401 || error.statusCode === 403); +}; + const isNoEntryError = (error: unknown) => { const message = getErrorMessage(error).toLowerCase(); return ( @@ -391,14 +395,29 @@ const isNoEntryError = (error: unknown) => { ); }; -const assertStoredOAuthCredential = (value: unknown): StoredOAuthCredential | undefined => { +const isOptionalString = (value: unknown) => value === undefined || typeof value === 'string'; + +const isOptionalNumber = (value: unknown) => value === undefined || typeof value === 'number'; + +const isNonEmptyString = (value: unknown): value is string => + typeof value === 'string' && value.length > 0; + +const assertStoredOAuthCredential = ( + value: unknown, + site: string, + options: Pick, +): StoredOAuthCredential | undefined => { if ( isObject(value) && value.version === 1 && isObject(value.token) && - typeof value.token.accessToken === 'string' && - typeof value.token.clientId === 'string' && - typeof value.token.site === 'string' + isNonEmptyString(value.token.accessToken) && + value.token.clientId === options.clientId && + value.token.site === site && + isOptionalNumber(value.token.expiresAt) && + isOptionalString(value.token.refreshToken) && + isOptionalString(value.token.scope) && + isOptionalString(value.token.tokenType) ) { return value as StoredOAuthCredential; } @@ -421,6 +440,19 @@ const secureStorageError = (operation: string, error: unknown) => }`, ); +const deleteOAuthCredentialEntry = async ( + entry: { deletePassword: () => Promise }, + operation: string, +) => { + try { + await entry.deletePassword(); + } catch (error) { + if (!isNoEntryError(error)) { + throw secureStorageError(operation, error); + } + } +}; + export const readOAuthTokenFromKeychain = async ( site: string, options: Pick, @@ -432,8 +464,21 @@ export const readOAuthTokenFromKeychain = async ( return undefined; } - const parsed: unknown = JSON.parse(raw); - return assertStoredOAuthCredential(parsed)?.token; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + await deleteOAuthCredentialEntry(entry, 'delete invalid'); + return undefined; + } + + const credential = assertStoredOAuthCredential(parsed, site, options); + if (!credential) { + await deleteOAuthCredentialEntry(entry, 'delete invalid'); + return undefined; + } + + return credential.token; } catch (error) { if (isNoEntryError(error)) { return undefined; @@ -461,14 +506,14 @@ export const deleteOAuthTokenFromKeychain = async ( site: string, options: Pick, ) => { + let entry: Awaited>; try { - const entry = await createOAuthCredentialEntry(site, options); - await entry.deletePassword(); + entry = await createOAuthCredentialEntry(site, options); } catch (error) { - if (!isNoEntryError(error)) { - throw secureStorageError('delete', error); - } + throw secureStorageError('delete', error); } + + await deleteOAuthCredentialEntry(entry, 'delete'); }; const isCachedTokenValid = (token: CachedOAuthToken) => @@ -529,7 +574,7 @@ const getCachedOAuthToken = async ( site: string, options: OAuthConfig, log: Logger, -): Promise => { +): Promise => { if (!options.cacheTokens) { return undefined; } @@ -548,7 +593,10 @@ const getCachedOAuthToken = async ( if (isCachedTokenValid(cachedToken)) { log.debug('Using cached Datadog OAuth access token.'); - return fromCachedToken(cachedToken); + return { + persistAfterSuccessfulRequest: false, + token: fromCachedToken(cachedToken), + }; } if (!cachedToken.refreshToken) { @@ -558,8 +606,10 @@ const getCachedOAuthToken = async ( try { log.debug('Refreshing cached Datadog OAuth access token.'); const refreshedToken = await refreshOAuthToken(site, options, cachedToken.refreshToken); - await saveOAuthToken(refreshedToken, options, log); - return refreshedToken; + return { + persistAfterSuccessfulRequest: true, + token: refreshedToken, + }; } catch (error) { log.warn( `Cached Datadog OAuth token could not be refreshed; starting browser authorization. ${ @@ -617,25 +667,13 @@ export const authorizeWithPKCE = async ( }); }; -export const getOAuthToken = async ( - site: string, - options: OAuthConfig, - log: Logger, -): Promise => { - const cachedToken = await getCachedOAuthToken(site, options, log); - if (cachedToken) { - return cachedToken; - } - - const token = await authorizeWithPKCE(site, options, log); - await saveOAuthToken(token, options, log, site); - return token; -}; - // Memoize per site+client for the lifetime of the process so concurrent requests // (and the sequential upload + release calls) share a single browser authorization. const tokenCache = new Map>(); +const getOAuthTokenCacheKey = (site: string, options: Pick) => + `${site}:${options.clientId}`; + const resolveOAuthTokenFromStorage = async ( site: string, options: OAuthConfig, @@ -643,16 +681,22 @@ const resolveOAuthTokenFromStorage = async ( ): Promise => { const cachedToken = await getCachedOAuthToken(site, options, log); if (cachedToken) { - return { persistAfterSuccessfulRequest: false, token: cachedToken }; + return cachedToken; } const token = await authorizeWithPKCE(site, options, log); return { persistAfterSuccessfulRequest: true, token }; }; -export const resolveOAuthToken = async (site: string, log: Logger): Promise => { +const invalidateOAuthToken = async (site: string, log: Logger) => { const options = getDatadogOAuthConfig(site); - const key = `${site}:${options.clientId}`; + tokenCache.delete(getOAuthTokenCacheKey(site, options)); + await deleteOAuthToken(site, options, log); +}; + +export const resolveOAuthToken = (site: string, log: Logger): Promise => { + const options = getDatadogOAuthConfig(site); + const key = getOAuthTokenCacheKey(site, options); let pending = tokenCache.get(key); if (!pending) { pending = resolveOAuthTokenFromStorage(site, options, log).catch((error) => { @@ -671,22 +715,34 @@ export type OAuthRequestOpts = Omit & { export const doOAuthRequest = async ({ auth, log, ...opts }: OAuthRequestOpts): Promise => { const { site } = auth; + const options = getDatadogOAuthConfig(site); const { persistAfterSuccessfulRequest, token } = await resolveOAuthToken(site, log); if (!token.accessToken) { throw new Error('OAuth authentication did not return an access token.'); } - const result = await doRequest({ - ...opts, - auth: { - accessToken: token.accessToken, - }, - }); + try { + const result = await doRequest({ + ...opts, + auth: { + accessToken: token.accessToken, + }, + }); - if (persistAfterSuccessfulRequest) { - await saveOAuthToken(token, getDatadogOAuthConfig(site), log, site); - } + if (persistAfterSuccessfulRequest) { + await saveOAuthToken(token, options, log, site); + } + + return result; + } catch (error) { + if (isOAuthAuthError(error)) { + log.warn( + `Datadog OAuth token was rejected by the API; deleting the cached token so the next command starts browser authorization. ${getErrorMessage(error)}`, + ); + await invalidateOAuthToken(site, log); + } - return result; + throw error; + } }; diff --git a/packages/core/src/helpers/request.test.ts b/packages/core/src/helpers/request.test.ts index dd377958b..b6cf08867 100644 --- a/packages/core/src/helpers/request.test.ts +++ b/packages/core/src/helpers/request.test.ts @@ -99,12 +99,22 @@ describe('Request Helpers', () => { }); test('Should bail on specific status', async () => { - const { doRequest } = await import('@dd/core/helpers/request'); + const { RequestError, doRequest } = await import('@dd/core/helpers/request'); const scope = nock(API_URL).post(API_PATH).reply(400, 'Bad Request'); - await expect(async () => { + let requestError: unknown; + try { await doRequest(requestOpts); - }).rejects.toThrow('HTTP 400 Bad Request'); + } catch (error) { + requestError = error; + } + + expect(requestError).toBeInstanceOf(RequestError); + expect(requestError).toMatchObject({ + statusCode: 400, + statusText: 'Bad Request', + }); + expect((requestError as Error).message).toContain('HTTP 400 Bad Request'); expect(scope.isDone()).toBe(true); }); diff --git a/packages/core/src/helpers/request.ts b/packages/core/src/helpers/request.ts index 9ea510976..179ed626e 100644 --- a/packages/core/src/helpers/request.ts +++ b/packages/core/src/helpers/request.ts @@ -7,6 +7,17 @@ import type { RequestInit } from 'undici-types'; import type { RequestOpts } from '../types'; +export class RequestError extends Error { + constructor( + message: string, + public statusCode: number, + public statusText: string, + ) { + super(message); + this.name = 'RequestError'; + } +} + const formatErrorEntry = (e: unknown): string => { if (e === null || typeof e !== 'object') { return ''; @@ -158,13 +169,18 @@ export const doRequest = async (opts: RequestOpts): Promise => { } catch { // Ignore if body cannot be read. } + const requestError = new RequestError( + errorMessage, + response.status, + response.statusText, + ); if (ERROR_CODES_NO_RETRY.includes(response.status)) { - bail(new Error(errorMessage)); + bail(requestError); // bail(error) throws so the return is never executed. return {} as T; } else { // Trigger the retry. - throw new Error(errorMessage); + throw requestError; } } diff --git a/packages/plugins/apps/README.md b/packages/plugins/apps/README.md index 16aa87e23..19c5dee4f 100644 --- a/packages/plugins/apps/README.md +++ b/packages/plugins/apps/README.md @@ -84,7 +84,7 @@ You can also set `DATADOG_APPS_AUTH_METHOD` or `DD_APPS_AUTH_METHOD` to `apiKey` When the method is `oauth`, the plugin derives OAuth client settings from the resolved Datadog site. The plugin reads tokens from the OS credential store, refreshes expired access tokens when a refresh token is available, and only starts browser authorization when no usable stored token exists. -For first-time authorization, the plugin starts a temporary local HTTP callback server, opens Datadog authorization in the browser, exchanges the authorization code with PKCE, and saves the returned token response for later uploads. +For first-time authorization, the plugin starts a temporary local HTTP callback server, opens Datadog authorization in the browser, and exchanges the authorization code with PKCE. The plugin saves the returned token response only after Datadog accepts an OAuth-authenticated plugin request. OAuth token and authorization URLs are derived from `auth.site`, so it must match your Datadog data center (e.g. `datadoghq.com`, `us5.datadoghq.com`, `datadoghq.eu`).