From d60610c3cad9936969746b52f90ea2f685ca15e2 Mon Sep 17 00:00:00 2001 From: Ross Stenersen Date: Wed, 10 Jan 2024 13:25:19 -0600 Subject: [PATCH] fix: decouple axios version from core SDK --- .changeset/friendly-turtles-kneel.md | 6 + src/authenticator.ts | 50 +++---- src/endpoint-client.ts | 12 +- test/unit/authenticator.test.ts | 57 ++------ test/unit/endpoint-client.test.ts | 195 ++++++++++++++++++++------- 5 files changed, 191 insertions(+), 129 deletions(-) create mode 100644 .changeset/friendly-turtles-kneel.md diff --git a/.changeset/friendly-turtles-kneel.md b/.changeset/friendly-turtles-kneel.md new file mode 100644 index 00000000..953d70b0 --- /dev/null +++ b/.changeset/friendly-turtles-kneel.md @@ -0,0 +1,6 @@ +--- +"@smartthings/core-sdk": patch +--- + +Decouple from axios. Dependents of the core SDK no longer need to use the same version of +axios, or even use axios at all. diff --git a/src/authenticator.ts b/src/authenticator.ts index a6a160f6..667849ad 100644 --- a/src/authenticator.ts +++ b/src/authenticator.ts @@ -1,7 +1,7 @@ import axios, { AxiosResponse, AxiosRequestConfig } from 'axios' import { MutexInterface } from 'async-mutex' -import { EndpointClientConfig } from './endpoint-client' +import { EndpointClientConfig, HttpClientHeaders } from './endpoint-client' /** @@ -13,24 +13,17 @@ import { EndpointClientConfig } from './endpoint-client' export interface Authenticator { login?(): Promise logout?(): Promise - refresh?(requestConfig: AxiosRequestConfig, clientConfig: EndpointClientConfig): Promise + refresh?(clientConfig: EndpointClientConfig): Promise acquireRefreshMutex?(): Promise /** - * Performs required authentication steps to add credentials to the axios config, typically via Bearer Auth headers. - * Expected to call other functions such as @see refresh as needed to return valid credentials. - * - * @param requestConfig AxiosRequestConfig to add credentials to and return otherwise unmodified - */ - authenticate(requestConfig: AxiosRequestConfig): Promise - - /** - * Performs required authentication steps and returns credentials as a string value + * Performs required authentication steps and returns credentials as a set of HTTP headers which + * must be included in authenticated requests. * Expected to perform any required steps (such as token refresh) needed to return valid credentials. * * @returns {string} valid auth token */ - authenticateGeneric?(): Promise + authenticate(): Promise } @@ -38,12 +31,8 @@ export interface Authenticator { * For use in tests or on endpoints that don't need any authentication. */ export class NoOpAuthenticator implements Authenticator { - authenticate(requestConfig: AxiosRequestConfig): Promise { - return Promise.resolve(requestConfig) - } - - authenticateGeneric(): Promise { - return Promise.resolve('') + authenticate(): Promise { + return Promise.resolve({}) } } @@ -57,7 +46,8 @@ export class BearerTokenAuthenticator implements Authenticator { // simple } - authenticate(requestConfig: AxiosRequestConfig): Promise { + authenticate(): Promise { + /* return Promise.resolve({ ...requestConfig, headers: { @@ -65,10 +55,8 @@ export class BearerTokenAuthenticator implements Authenticator { Authorization: `Bearer ${this.token}`, }, }) - } - - authenticateGeneric(): Promise { - return Promise.resolve(this.token) + */ + return Promise.resolve({ Authorization: `Bearer ${this.token}` }) } } @@ -101,17 +89,11 @@ export class RefreshTokenAuthenticator implements Authenticator { // simple } - authenticate(requestConfig: AxiosRequestConfig): Promise { - return Promise.resolve({ - ...requestConfig, - headers: { - ...requestConfig.headers, - Authorization: `Bearer ${this.token}`, - }, - }) + authenticate(): Promise { + return Promise.resolve({ Authorization: `Bearer ${this.token}` }) } - async refresh(requestConfig: AxiosRequestConfig, clientConfig: EndpointClientConfig): Promise { + async refresh(clientConfig: EndpointClientConfig): Promise { const refreshData: RefreshData = await this.tokenStore.getRefreshData() const headers = { 'Content-Type': 'application/x-www-form-urlencoded', @@ -133,8 +115,8 @@ export class RefreshTokenAuthenticator implements Authenticator { refreshToken: response.data.refresh_token, } this.token = authData.authToken - requestConfig.headers = { ...(requestConfig.headers ?? {}), Authorization: `Bearer ${this.token}` } - return this.tokenStore.putAuthData(authData) + await this.tokenStore.putAuthData(authData) + return { Authorization: `Bearer ${this.token}` } } throw Error(`error ${response.status} refreshing token, with message ${response.data}`) diff --git a/src/endpoint-client.ts b/src/endpoint-client.ts index 39270959..56a69aff 100644 --- a/src/endpoint-client.ts +++ b/src/endpoint-client.ts @@ -174,7 +174,7 @@ export class EndpointClient { } } - let axiosConfig: AxiosRequestConfig = { + const axiosConfig: AxiosRequestConfig = { url: this.url(path), method, headers: options?.headerOverrides ? { ...headers, ...options.headerOverrides } : headers, @@ -183,7 +183,8 @@ export class EndpointClient { paramsSerializer: params => qs.stringify(params, { indices: false }), } - axiosConfig = await this.config.authenticator.authenticate(axiosConfig) + const authHeaders = await this.config.authenticator.authenticate() + axiosConfig.headers = { ...axiosConfig.headers, ...authHeaders } if (this.logger.isDebugEnabled()) { this.logger.debug(`making axios request: ${scrubConfig(axiosConfig)}`) @@ -208,6 +209,7 @@ export class EndpointClient { return response.data } catch (error: any) { if (this.logger.isTraceEnabled()) { + // https://www.npmjs.com/package/axios#handling-errors if (error.response) { // server responded with non-200 response code this.logger.trace(`axios response ${error.response.status}: data=${JSON.stringify(error.response.data)}`) @@ -222,14 +224,16 @@ export class EndpointClient { if (this.config.authenticator.acquireRefreshMutex) { const release = await this.config.authenticator.acquireRefreshMutex() try { - await this.config.authenticator.refresh(axiosConfig, this.config) + const newAuthHeaders = await this.config.authenticator.refresh(this.config) + axiosConfig.headers = { ...axiosConfig.headers, ...newAuthHeaders } const response = await axios.request(axiosConfig) return response.data } finally { release() } } else { - await this.config.authenticator.refresh(axiosConfig, this.config) + const newAuthHeaders = await this.config.authenticator.refresh(this.config) + axiosConfig.headers = { ...axiosConfig.headers, ...newAuthHeaders } const response = await axios.request(axiosConfig) return response.data } diff --git a/test/unit/authenticator.test.ts b/test/unit/authenticator.test.ts index 7dfb325f..eecd8d93 100644 --- a/test/unit/authenticator.test.ts +++ b/test/unit/authenticator.test.ts @@ -28,50 +28,24 @@ describe('authenticators', () => { jest.clearAllMocks() }) - const config = { url: 'https://api.smartthings.com', headers: { Test: 'test' } } + test('NoOpAuthenticator returns no headers', async () => { + const authenticator = new NoOpAuthenticator() - describe('NoOpAuthenticator', () => { - test('authenticate returns config unchanged', async () => { - const authenticator = new NoOpAuthenticator() - const data = await authenticator.authenticate(config) - - expect(data).toBe(config) - }) - - test('authenticateGeneric returns empty string for token', async () => { - const authenticator = new NoOpAuthenticator() - const token = await authenticator.authenticateGeneric() - - expect(token).toBe('') - }) + expect(await authenticator.authenticate()).toStrictEqual({}) }) - describe('BearerTokenAuthenticator', () => { - test('authenticate adds header with specified token', async () => { - const authenticator = new BearerTokenAuthenticator('a-bearer-token') - const data = await authenticator.authenticate(config) + test('BearerTokenAuthenticator returns header with specified token', async () => { + const authenticator = new BearerTokenAuthenticator('a-bearer-token') - expect(data.url).toBe(config.url) - expect(data.headers?.Authorization).toBe('Bearer a-bearer-token') - expect(data.headers?.Test).toBe('test') - }) - - test('authenticateGeneric returns specified token', async () => { - const authenticator = new BearerTokenAuthenticator('a-bearer-token') - const token = await authenticator.authenticateGeneric() - - expect(token).toBe('a-bearer-token') - }) + expect(await authenticator.authenticate()).toStrictEqual({ Authorization: 'Bearer a-bearer-token' }) }) describe('RefreshTokenAuthenticator', () => { - test('authenticate adds header with specified token', async () => { + test('authenticate returns header with specified token', async () => { const tokenStore = new TokenStore() const authenticator = new RefreshTokenAuthenticator('a-refreshable-bearer-token', tokenStore) - const data = await authenticator.authenticate(config) - expect(data.url).toBe(config.url) - expect(data.headers?.Authorization).toBe('Bearer a-refreshable-bearer-token') + expect(await authenticator.authenticate()).toStrictEqual({ Authorization: 'Bearer a-refreshable-bearer-token' }) }) test('refresh updates token', async () => { @@ -86,7 +60,8 @@ describe('authenticators', () => { const tokenStore = new TokenStore() const authenticator = new RefreshTokenAuthenticator('a-refreshable-bearer-token', tokenStore) const endpointConfig = { urlProvider: defaultSmartThingsURLProvider, authenticator } - await authenticator.refresh(config, endpointConfig) + + expect(await authenticator.refresh(endpointConfig)).toStrictEqual({ Authorization: 'Bearer the-access-token' }) expect(axios.request).toHaveBeenCalledTimes(1) expect(axios.request).toHaveBeenCalledWith({ @@ -114,7 +89,7 @@ describe('authenticators', () => { const endpointConfig = { urlProvider: defaultSmartThingsURLProvider, authenticator } let message try { - await authenticator.refresh(config, endpointConfig) + await authenticator.refresh(endpointConfig) // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (error: any) { message = error.message @@ -133,10 +108,7 @@ describe('authenticators', () => { const authenticator = new SequentialRefreshTokenAuthenticator('a-bearer-token', tokenStore, mutex) test('authenticate adds header with specified token', async () => { - const data = await authenticator.authenticate(config) - - expect(data.url).toBe(config.url) - expect(data.headers?.Authorization).toBe('Bearer a-bearer-token') + expect(await authenticator.authenticate()).toStrictEqual({ Authorization: 'Bearer a-bearer-token' }) }) describe('refresh', () => { @@ -151,7 +123,7 @@ describe('authenticators', () => { const endpointConfig = { urlProvider: defaultSmartThingsURLProvider, authenticator } it('updates token', async () => { - await authenticator.refresh(config, endpointConfig) + await authenticator.refresh(endpointConfig) expect(axios.request).toHaveBeenCalledTimes(1) expect(axios.request).toHaveBeenCalledWith({ @@ -169,8 +141,7 @@ describe('authenticators', () => { }) it('works on request with no existing headers', async () => { - const configWithoutHeaders = { url: 'https://api.smartthings.com' } - await authenticator.refresh(configWithoutHeaders, endpointConfig) + expect(await authenticator.refresh(endpointConfig)).toStrictEqual({ Authorization: 'Bearer the-access-token' }) expect(axios.request).toHaveBeenCalledTimes(1) expect(axios.request).toHaveBeenCalledWith({ diff --git a/test/unit/endpoint-client.test.ts b/test/unit/endpoint-client.test.ts index 0d22c635..a4ed8e39 100644 --- a/test/unit/endpoint-client.test.ts +++ b/test/unit/endpoint-client.test.ts @@ -1,12 +1,12 @@ import { Mutex } from 'async-mutex' -import axios, { AxiosRequestConfig, AxiosResponse } from 'axios' +import axios, { AxiosError, AxiosRequestConfig, AxiosResponse } from 'axios' import qs from 'qs' import { AuthData, Authenticator, BearerTokenAuthenticator, NoOpAuthenticator, RefreshData, RefreshTokenAuthenticator, RefreshTokenStore, SequentialRefreshTokenAuthenticator } from '../../src/authenticator' -import { defaultSmartThingsURLProvider, EndpointClient, EndpointClientConfig, parseWarningHeader } from '../../src/endpoint-client' -import { NoLogLogger } from '../../src/logger' +import { defaultSmartThingsURLProvider, EndpointClient, EndpointClientConfig, HttpClientHeaders, parseWarningHeader } from '../../src/endpoint-client' +import { Logger, NoLogLogger } from '../../src/logger' jest.mock('axios') @@ -305,6 +305,110 @@ describe('EndpointClient', () => { })).rejects.toThrow('skipping request; dry run mode') expect(mockRequest).toHaveBeenCalledTimes(0) }) + + describe('logging', () => { + const isDebugEnabledMock = jest.fn().mockReturnValue(true) + const isTraceEnabledMock = jest.fn().mockReturnValue(true) + const debugMock = jest.fn() + const traceMock = jest.fn() + const mockLogger = { + isDebugEnabled: isDebugEnabledMock, + isTraceEnabled: isTraceEnabledMock, + debug: debugMock, + trace: traceMock, + } as unknown as Logger + + beforeEach(() => { + client = buildClient({ ...configWithoutHeaders, headers, logger: mockLogger }) + }) + + it('logs axios request at debug level', async () => { + await client.request('GET', 'my/path') + + expect(isDebugEnabledMock).toHaveBeenCalledTimes(1) + expect(debugMock).toHaveBeenCalledTimes(1) + expect(debugMock).toHaveBeenCalledWith('making axios request: {' + + '"url":"https://api.smartthings.com/base/path/my/path",' + + '"method":"GET",' + + '"headers":{' + + '"Content-Type":"application/json;charset=utf-8",' + + '"Accept":"application/json",' + + '"Authorization":"(redacted)"' + + '}' + + '}') + }) + + it('logs successful axios response at trace level', async () => { + await client.request('GET', 'my/path') + + expect(isTraceEnabledMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledWith('axios response 200: data={"status":"ok"}') + }) + + it('logs failed axios response at trace level', async () => { + mockRequest.mockImplementationOnce(() => { throw Error('things done broke!') }) + + await expect(client.request('GET', 'my/path')).rejects.toThrow('things done broke!') + + expect(isTraceEnabledMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledWith('error making request: things done broke!') + }) + + it('logs failed axios response with response info', async () => { + const error = Error('error message') as AxiosError + error.response = { + status: 400, + data: 'body data', + } as AxiosResponse + mockRequest.mockImplementationOnce(() => { throw error }) + + await expect(client.request('GET', 'my/path')).rejects.toThrow('error message: "body data"') + + expect(isTraceEnabledMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledWith('axios response 400: data="body data"') + }) + + it('logs failed axios response with response info', async () => { + const error = Error('error message') as AxiosError + error.response = { + status: 400, + data: 'body data', + } as AxiosResponse + mockRequest.mockImplementationOnce(() => { throw error }) + + await expect(client.request('GET', 'my/path')).rejects.toThrow('error message: "body data"') + + expect(isTraceEnabledMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledWith('axios response 400: data="body data"') + }) + + it('logs request info when no response received from server', async () => { + const error = Error('error message') as AxiosError + error.request = { info: 'about request' } + mockRequest.mockImplementationOnce(() => { throw error }) + + await expect(client.request('GET', 'my/path')).rejects.toThrow('error message') + + expect(isTraceEnabledMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledWith('no response from server for request {"info":"about request"}') + }) + + it('logs error when setting up request failed', async () => { + const error = Error('error message') as AxiosError + mockRequest.mockImplementationOnce(() => { throw error }) + + await expect(client.request('GET', 'my/path')).rejects.toThrow('error message') + + expect(isTraceEnabledMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledTimes(1) + expect(traceMock).toHaveBeenCalledWith('error making request: error message') + }) + }) }) describe('get', () => { @@ -453,6 +557,43 @@ describe('EndpointClient', () => { expect(response.status).toBe('ok') }) + describe('getPagedItems', () => { + const getMock = jest.fn() + const client = new EndpointClient('paged-thing', { + authenticator: new NoOpAuthenticator(), + logger: new NoLogLogger(), + }) + client.get = getMock + + const item1 = { name: 'item-1' } + const item2 = { name: 'item-2' } + + it('uses single get when full results returned in one go', async () => { + getMock.mockResolvedValueOnce({ + items: [item1, item2], + }) + + expect(await client.getPagedItems()).toEqual([item1, item2]) + + expect(getMock).toHaveBeenCalledTimes(1) + expect(getMock).toHaveBeenCalledWith(undefined, undefined, undefined) + }) + + it('combines multiple pages', async () => { + const params = { paramName: 'param-value' } + const options = { dryRun: false } + getMock + .mockResolvedValueOnce({ items: [item1], _links: { next: { href: 'next-url' } } }) + .mockResolvedValueOnce({ items: [item2] }) + + expect(await client.getPagedItems('first-url', params, options)).toEqual([item1, item2]) + + expect(getMock).toHaveBeenCalledTimes(2) + expect(getMock).toHaveBeenCalledWith('first-url', params, options) + expect(getMock).toHaveBeenCalledWith('next-url', undefined, options) + }) + }) + test('expired token request', async () => { mockRequest .mockImplementationOnce(() => Promise.reject( @@ -522,6 +663,7 @@ describe('EndpointClient', () => { expect(threwError).toBe(true) }) + // TODO: this could probably be merged into request.logging above describe('request logging', () => { jest.spyOn(NoLogLogger.prototype, 'isDebugEnabled').mockReturnValue(true) const debugSpy = jest.spyOn(NoLogLogger.prototype, 'debug') @@ -544,14 +686,8 @@ describe('EndpointClient', () => { it('fully redacts Auth header when Bearer is not present', async () => { const basicAuth = Buffer.from('username:password', 'ascii').toString('base64') class BasicAuthenticator implements Authenticator { - authenticate(requestConfig: AxiosRequestConfig): Promise { - return Promise.resolve({ - ...requestConfig, - headers: { - ...requestConfig.headers, - Authorization: `Basic ${basicAuth}`, - }, - }) + authenticate(): Promise { + return Promise.resolve({ Authorization: `Basic ${basicAuth}` }) } } const config: EndpointClientConfig = { @@ -566,42 +702,5 @@ describe('EndpointClient', () => { expect(debugSpy).not.toBeCalledWith(expect.stringContaining(basicAuth)) expect(debugSpy).toBeCalledWith(expect.stringContaining('Authorization":"(redacted)"')) }) - - describe('getPagedItems', () => { - const getMock = jest.fn() - const client = new EndpointClient('paged-thing', { - authenticator: new NoOpAuthenticator(), - logger: new NoLogLogger(), - }) - client.get = getMock - - const item1 = { name: 'item-1' } - const item2 = { name: 'item-2' } - - it('uses single get when full results returned in one go', async () => { - getMock.mockResolvedValueOnce({ - items: [item1, item2], - }) - - expect(await client.getPagedItems()).toEqual([item1, item2]) - - expect(getMock).toHaveBeenCalledTimes(1) - expect(getMock).toHaveBeenCalledWith(undefined, undefined, undefined) - }) - - it('combines multiple pages', async () => { - const params = { paramName: 'param-value' } - const options = { dryRun: false } - getMock - .mockResolvedValueOnce({ items: [item1], _links: { next: { href: 'next-url' } } }) - .mockResolvedValueOnce({ items: [item2] }) - - expect(await client.getPagedItems('first-url', params, options)).toEqual([item1, item2]) - - expect(getMock).toHaveBeenCalledTimes(2) - expect(getMock).toHaveBeenCalledWith('first-url', params, options) - expect(getMock).toHaveBeenCalledWith('next-url', undefined, options) - }) - }) }) })