From 3278ddf8c56fc6a9cd2de229c651f592e574afaf Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Tue, 10 Sep 2019 11:11:55 -0500 Subject: [PATCH] feat: add `setServiceUrl` method as a setter for the `serviceUrl` property * Change `url` to `serviceUrl` * Validate that `serviceUrl` is set in the `sendRequest` method * Make "auth type" case-insensitive * Throw an error for unsupported "auth types" - only default to 'iam' when no "auth type" is provided BREAKING CHANGE: The internal property `url` no longer exists on the `baseOptions` object, it has been renamed to `serviceUrl` --- .../get-authenticator-from-environment.ts | 18 ++++-- lib/base_service.ts | 31 +++++++--- lib/requestwrapper.ts | 14 ++++- migration-guide.md | 3 + test/unit/base-service.test.js | 56 +++++++++++++------ ...get-authenticator-from-environment.test.js | 15 +++-- test/unit/requestWrapper.test.js | 37 ++++++++++-- 7 files changed, 131 insertions(+), 43 deletions(-) diff --git a/auth/utils/get-authenticator-from-environment.ts b/auth/utils/get-authenticator-from-environment.ts index 2a98f12de..6365cf4c4 100644 --- a/auth/utils/get-authenticator-from-environment.ts +++ b/auth/utils/get-authenticator-from-environment.ts @@ -54,24 +54,34 @@ export function getAuthenticatorFromEnvironment(serviceName: string) { delete credentials.authDisableSsl; } + // default the auth type to `iam` if authType is undefined, or not a string + let { authType } = credentials; + if (!authType || typeof authType !== 'string') { + authType = 'iam'; + } + // create and return the appropriate authenticator let authenticator; - switch (credentials.authType) { - case 'noAuth': + // fold authType to lower case for case insensitivity + switch (authType.toLowerCase()) { + case 'noauth': authenticator = new NoAuthAuthenticator(); break; case 'basic': authenticator = new BasicAuthenticator(credentials); break; - case 'bearerToken': + case 'bearertoken': authenticator = new BearerTokenAuthenticator(credentials); break; case 'cp4d': authenticator = new CloudPakForDataAuthenticator(credentials); break; - default: // default the authentication type to iam + case 'iam': authenticator = new IamAuthenticator(credentials); + break; + default: + throw new Error('Invalid value for AUTH_TYPE: ' + authType); } return authenticator; diff --git a/lib/base_service.ts b/lib/base_service.ts index a3241e52a..ca72541cb 100644 --- a/lib/base_service.ts +++ b/lib/base_service.ts @@ -23,7 +23,8 @@ import { stripTrailingSlash } from './helper'; import { RequestWrapper } from './requestwrapper'; export interface UserOptions { - url?: string; + url?: string; // deprecated + serviceUrl?: string; version?: string; headers?: OutgoingHttpHeaders; disableSslVerification?: boolean; @@ -66,12 +67,17 @@ export class BaseService { const _options = {} as BaseServiceOptions; const options = extend({}, userOptions); - if (options.url) { - _options.url = stripTrailingSlash(options.url); + // for compatibility + if (options.url && !options.serviceUrl) { + options.serviceUrl = options.url; } - // check url for common user errors - const credentialProblems = checkCredentials(options, ['url']); + if (options.serviceUrl) { + _options.serviceUrl = stripTrailingSlash(options.serviceUrl); + } + + // check serviceUrl for common user errors + const credentialProblems = checkCredentials(options, ['serviceUrl']); if (credentialProblems) { throw new Error(credentialProblems); } @@ -84,7 +90,7 @@ export class BaseService { const serviceClass = this.constructor as typeof BaseService; this.baseOptions = extend( - { qs: {}, url: serviceClass.URL }, + { qs: {}, serviceUrl: serviceClass.URL }, options, this.readOptionsFromExternalConfig(), _options @@ -92,7 +98,7 @@ export class BaseService { this.requestWrapperInstance = new RequestWrapper(this.baseOptions); - // set authenticator + // enforce that an authenticator is set if (!options.authenticator) { throw new Error('Authenticator must be set.'); } @@ -109,6 +115,15 @@ export class BaseService { return this.authenticator; } + /** + * Set the service URL to send requests to. + * + * @param {string} the base URL for the service + */ + public setServiceUrl(url: string): void { + this.baseOptions.serviceUrl = url; + } + /** * Wrapper around `sendRequest` that enforces the request will be authenticated. * @@ -131,7 +146,7 @@ export class BaseService { const { url, disableSsl } = properties; if (url) { - results.url = url; + results.serviceUrl = url; } if (disableSsl === true) { results.disableSslVerification = disableSsl; diff --git a/lib/requestwrapper.ts b/lib/requestwrapper.ts index a5a6999a8..058d72248 100644 --- a/lib/requestwrapper.ts +++ b/lib/requestwrapper.ts @@ -122,8 +122,16 @@ export class RequestWrapper { */ public sendRequest(parameters, _callback) { const options = extend(true, {}, parameters.defaultOptions, parameters.options); - const { path, body, form, formData, qs, method } = options; - let { url, headers } = options; + const { path, body, form, formData, qs, method, serviceUrl } = options; + let { headers, url } = options; + + // validate service url parameter has been set + if (!serviceUrl || typeof serviceUrl !== 'string') { + return _callback(new Error('The service URL is required'), null); + } + + // use default service url if `url` parameter is not specified in generated method + url = url || serviceUrl; const multipartForm = new FormData(); @@ -189,7 +197,7 @@ export class RequestWrapper { // Add service default endpoint if options.url start with / if (url && url.charAt(0) === '/') { - url = parameters.defaultOptions.url + url; + url = serviceUrl + url; } let data = body; diff --git a/migration-guide.md b/migration-guide.md index 099e99f3b..0bafab3a4 100644 --- a/migration-guide.md +++ b/migration-guide.md @@ -31,3 +31,6 @@ const service = new MyService({ - `JwtTokenManagerV1` renamed to `JwtTokenManager` - Token managers no longer support the `accessToken` parameter. There is no need for a token manager when a user is managing their own token. This behavior is replaced by the `BearerTokenAuthenticator` class. - In the IAM Token Manager: the method `setAuthorizationInfo` is renamed to `setClientIdAndSecret` + +#### URL parameter name changed +The variable name for the stored, URL parameter has been changed from `url` to `serviceUrl`. Note that `url` can still be compatibility passed into the constructor as an alias for `serviceUrl`. However, if you try to access the `url` property directly in your code, this is a breaking change. diff --git a/test/unit/base-service.test.js b/test/unit/base-service.test.js index 31f6d7bcf..d7c3dc114 100644 --- a/test/unit/base-service.test.js +++ b/test/unit/base-service.test.js @@ -70,14 +70,34 @@ describe('Base Service', () => { }).toThrow(); }); - it('should strip trailing slash of url during instantiation', () => { + it('should strip trailing slash of serviceUrl during instantiation', () => { + const testService = new TestService({ + authenticator: AUTHENTICATOR, + serviceUrl: 'https://example.ibm.com/', + }); + + expect(testService.baseOptions.serviceUrl).toBe('https://example.ibm.com'); + }); + + it('should accept `url` instead of `serviceUrl` for compaitiblity', () => { const testService = new TestService({ authenticator: AUTHENTICATOR, - version: 'v1', url: 'https://example.ibm.com/', }); - expect(testService.baseOptions.url).toBe('https://example.ibm.com'); + expect(testService.baseOptions.serviceUrl).toBe('https://example.ibm.com'); + }); + + it('should support setting the service url after instantiation', () => { + const testService = new TestService({ + authenticator: AUTHENTICATOR, + }); + + expect(testService.baseOptions.serviceUrl).toBe(DEFAULT_URL); + + const newUrl = 'new.com'; + testService.setServiceUrl(newUrl); + expect(testService.baseOptions.serviceUrl).toBe(newUrl); }); it('should throw an error if an authenticator is not passed in', () => { @@ -120,7 +140,7 @@ describe('Base Service', () => { authenticator: AUTHENTICATOR, disableSslVerification: false, proxy: false, - url: DEFAULT_URL, + serviceUrl: DEFAULT_URL, qs: EMPTY_OBJECT, }); }); @@ -134,20 +154,20 @@ describe('Base Service', () => { expect(testService.baseOptions.qs).toEqual(EMPTY_OBJECT); }); - it('should use the default service url', () => { + it('should use the default service serviceUrl', () => { const testService = new TestService({ authenticator: AUTHENTICATOR, }); - expect(testService.baseOptions.url).toBe(DEFAULT_URL); + expect(testService.baseOptions.serviceUrl).toBe(DEFAULT_URL); }); - it('should read url and disableSslVerification from env', () => { - const url = 'abc123.com'; + it('should read serviceUrl and disableSslVerification from env', () => { + const serviceUrl = 'abc123.com'; const disableSsl = true; readExternalSourcesMock.mockImplementation(() => ({ - url, + url: serviceUrl, disableSsl, })); @@ -157,7 +177,7 @@ describe('Base Service', () => { const fromCredsFile = testService.readOptionsFromExternalConfig(); - expect(fromCredsFile.url).toBe(url); + expect(fromCredsFile.serviceUrl).toBe(serviceUrl); expect(fromCredsFile.disableSslVerification).toBe(disableSsl); expect(readExternalSourcesMock).toHaveBeenCalled(); expect(readExternalSourcesMock.mock.calls[0][0]).toBe(DEFAULT_NAME); @@ -170,12 +190,12 @@ describe('Base Service', () => { const testService = new TestService({ authenticator: AUTHENTICATOR, - url: 'withtrailingslash.com/api/', + serviceUrl: 'withtrailingslash.com/api/', proxy: false, }); expect(testService.baseOptions).toEqual({ - url: 'withtrailingslash.com/api', + serviceUrl: 'withtrailingslash.com/api', disableSslVerification: true, proxy: false, qs: EMPTY_OBJECT, @@ -192,11 +212,11 @@ describe('Base Service', () => { const parameters = { defaultOptions: { - url: DEFAULT_URL, + serviceUrl: DEFAULT_URL, Accept: 'application/json', }, options: { - url: '/v2/assistants/{assistant_id}/sessions', + serviceUrl: '/v2/assistants/{assistant_id}/sessions', method: 'POST', path: { id: '123', @@ -219,11 +239,11 @@ describe('Base Service', () => { const parameters = { defaultOptions: { - url: DEFAULT_URL, + serviceUrl: DEFAULT_URL, Accept: 'application/json', }, options: { - url: '/v2/assistants/{assistant_id}/sessions', + serviceUrl: '/v2/assistants/{assistant_id}/sessions', method: 'POST', path: { id: '123', @@ -268,11 +288,11 @@ describe('Base Service', () => { expect(testService.readOptionsFromExternalConfig()).toEqual(EMPTY_OBJECT); }); - it('should check url for common problems', () => { + it('should check serviceUrl for common problems', () => { expect(() => { new TestService({ authenticator: AUTHENTICATOR, - url: 'myapi.com/{instanceId}', + serviceUrl: 'myapi.com/{instanceId}', }); }).toThrow(/Revise these credentials/); }); diff --git a/test/unit/get-authenticator-from-environment.test.js b/test/unit/get-authenticator-from-environment.test.js index 51054a470..1e4801dee 100644 --- a/test/unit/get-authenticator-from-environment.test.js +++ b/test/unit/get-authenticator-from-environment.test.js @@ -31,8 +31,8 @@ describe('Get Authenticator From Environment Module', () => { expect(() => getAuthenticatorFromEnvironment(SERVICE_NAME)).toThrow(); }); - it('should get no auth authenticator', () => { - setUpNoauthPayload(); + it('should get no auth authenticator - tests case insentivity of auth type', () => { + setUpNoAuthPayload(); const authenticator = getAuthenticatorFromEnvironment(SERVICE_NAME); expect(authenticator).toBeInstanceOf(NoAuthAuthenticator); expect(readExternalSourcesMock).toHaveBeenCalled(); @@ -82,12 +82,19 @@ describe('Get Authenticator From Environment Module', () => { expect(authenticator).toBeInstanceOf(IamAuthenticator); expect(authenticator.apikey).toBe(APIKEY); }); + + it('should throw an error when an unsupported auth type is provided', () => { + readExternalSourcesMock.mockImplementation(() => ({ apikey: APIKEY, authType: 'unsupported' })); + expect(() => { + getAuthenticatorFromEnvironment(SERVICE_NAME); + }).toThrow(); + }); }); // mock payloads for the read-external-sources module -function setUpNoauthPayload() { +function setUpNoAuthPayload() { readExternalSourcesMock.mockImplementation(() => ({ - authType: 'noAuth', + authType: 'noauth', })); } diff --git a/test/unit/requestWrapper.test.js b/test/unit/requestWrapper.test.js index 2ece1d7a0..03a7a4486 100644 --- a/test/unit/requestWrapper.test.js +++ b/test/unit/requestWrapper.test.js @@ -59,7 +59,7 @@ describe('sendRequest', () => { formData: '', qs: {}, method: 'POST', - url: + serviceUrl: 'https://example.ibm.com/v1/environments/environment-id/configurations/configuration-id', headers: { 'test-header': 'test-header-value', @@ -97,7 +97,7 @@ describe('sendRequest', () => { formData: '', qs: {}, method: 'POST', - url: + serviceUrl: 'https://example.ibm.com/v1/environments/environment-id/configurations/configuration-id', headers: { 'test-header': 'test-header-value', @@ -124,7 +124,7 @@ describe('sendRequest', () => { version: '2017-10-15', }, method: 'POST', - url: 'https://example.ibm.com', + serviceUrl: 'https://example.ibm.com', headers: { 'test-header': 'test-header-value', }, @@ -185,7 +185,7 @@ describe('sendRequest', () => { version: '2017-10-15', }, method: 'POST', - url: 'https://example.ibm.com', + serviceUrl: 'https://example.ibm.com', headers: { 'test-header': 'test-header-value', }, @@ -270,7 +270,7 @@ describe('sendRequest', () => { version: '2017-10-15', }, method: 'POST', - url: 'https://example.ibm.com', + serviceUrl: 'https://example.ibm.com', headers: { 'test-header': 'test-header-value', }, @@ -319,6 +319,31 @@ describe('sendRequest', () => { }); }); + it('should call callback with an error if `serviceUrl` is not set', done => { + const parameters = { + defaultOptions: { + body: 'post=body', + formData: '', + qs: {}, + method: 'POST', + headers: { + 'test-header': 'test-header-value', + }, + responseType: 'buffer', + }, + }; + + mockAxiosInstance.mockResolvedValue(axiosResolveValue); + + requestWrapperInstance.sendRequest(parameters, (err, res) => { + // assert results + expect(err).toBeInstanceOf(Error); + expect(res).toBeNull(); + expect(mockAxiosInstance).not.toHaveBeenCalled(); + done(); + }); + }); + // Need to rewrite this to test instantiation with userOptions // it('should keep parameters in options that are not explicitly set in requestwrapper', done => { @@ -329,7 +354,7 @@ describe('sendRequest', () => { // qs: {}, // method: 'POST', // rejectUnauthorized: true, - // url: + // serviceUrl: // 'https://example.ibm.com/v1/environments/environment-id/configurations/configuration-id', // headers: { // 'test-header': 'test-header-value',