From e97413b2ae5d60e16cd0c3fd8762cae2e71ee2f0 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Tue, 21 Jun 2022 15:41:17 -0700 Subject: [PATCH 1/2] fix: Requests Respect `config.projectIdRequired` === `false` --- src/nodejs-common/service.ts | 6 +- src/nodejs-common/util.ts | 116 +++++++++++++++++++++++++++------- test/nodejs-common/service.ts | 8 ++- test/nodejs-common/util.ts | 71 +++++++++++++++++++-- 4 files changed, 168 insertions(+), 33 deletions(-) diff --git a/src/nodejs-common/service.ts b/src/nodejs-common/service.ts index 4235ce832..5cadbba59 100644 --- a/src/nodejs-common/service.ts +++ b/src/nodejs-common/service.ts @@ -28,7 +28,7 @@ import { util, } from './util'; -const PROJECT_ID_TOKEN = '{{projectId}}'; +export const DEFAULT_PROJECT_ID_TOKEN = '{{projectId}}'; export interface StreamRequestOptions extends DecorateRequestOptions { shouldReturnStream: true; @@ -106,7 +106,7 @@ export class Service { this.globalInterceptors = arrify(options.interceptors_!); this.interceptors = []; this.packageJson = config.packageJson; - this.projectId = options.projectId || PROJECT_ID_TOKEN; + this.projectId = options.projectId || DEFAULT_PROJECT_ID_TOKEN; this.projectIdRequired = config.projectIdRequired !== false; this.providedUserAgent = options.userAgent; @@ -167,7 +167,7 @@ export class Service { protected async getProjectIdAsync(): Promise { const projectId = await this.authClient.getProjectId(); - if (this.projectId === PROJECT_ID_TOKEN && projectId) { + if (this.projectId === DEFAULT_PROJECT_ID_TOKEN && projectId) { this.projectId = projectId; } return this.projectId; diff --git a/src/nodejs-common/util.ts b/src/nodejs-common/util.ts index 720ff3ad2..8b1d6d990 100644 --- a/src/nodejs-common/util.ts +++ b/src/nodejs-common/util.ts @@ -18,7 +18,10 @@ * @module common/util */ -import {replaceProjectIdToken} from '@google-cloud/projectify'; +import { + replaceProjectIdToken, + MissingProjectIdError, +} from '@google-cloud/projectify'; import * as ent from 'ent'; import * as extend from 'extend'; import {AuthClient, GoogleAuth, GoogleAuthOptions} from 'google-auth-library'; @@ -29,6 +32,8 @@ import {Duplex, DuplexOptions, Readable, Transform, Writable} from 'stream'; import {teenyRequest} from 'teeny-request'; import {Interceptor} from './service-object'; import * as uuid from 'uuid'; +import {DEFAULT_PROJECT_ID_TOKEN} from './service'; + const packageJson = require('../../../package.json'); // eslint-disable-next-line @typescript-eslint/no-var-requires @@ -164,6 +169,11 @@ export interface MakeAuthenticatedRequestFactoryConfig * A new will be created if this is not set. */ authClient?: AuthClient | GoogleAuth; + + /** + * Determines if a projectId is required for authenticated requests. Defaults to `true`. + */ + projectIdRequired?: boolean; } export interface MakeAuthenticatedRequestOptions { @@ -592,7 +602,7 @@ export class Util { config: MakeAuthenticatedRequestFactoryConfig ) { const googleAutoAuthConfig = extend({}, config); - if (googleAutoAuthConfig.projectId === '{{projectId}}') { + if (googleAutoAuthConfig.projectId === DEFAULT_PROJECT_ID_TOKEN) { delete googleAutoAuthConfig.projectId; } @@ -650,7 +660,11 @@ export class Util { const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : undefined; - const onAuthenticated = ( + async function setProjectId() { + projectId = await authClient.getProjectId(); + } + + const onAuthenticated = async ( err: Error | null, authenticatedReqOpts?: DecorateRequestOptions ) => { @@ -667,16 +681,35 @@ export class Util { if (!err || autoAuthFailed) { try { + // Try with existing `projectId` value authenticatedReqOpts = util.decorateRequest( authenticatedReqOpts!, projectId ); + err = null; } catch (e) { - // A projectId was required, but we don't have one. - // Re-use the "Could not load the default credentials error" if - // auto auth failed. - err = err || (e as Error); + if (e instanceof MissingProjectIdError) { + // A `projectId` was required, but we don't have one. + try { + // Attempt to get the `projectId` + await setProjectId(); + + authenticatedReqOpts = util.decorateRequest( + authenticatedReqOpts!, + projectId + ); + + err = null; + } catch (e) { + // Re-use the "Could not load the default credentials error" if + // auto auth failed. + err = err || (e as Error); + } + } else { + // Some other error unrelated to missing `projectId` + err = err || (e as Error); + } } } @@ -715,23 +748,58 @@ export class Util { } }; - Promise.all([ - config.projectId && config.projectId !== '{{projectId}}' - ? // The user provided a project ID. We don't need to check with the - // auth client, it could be incorrect. - new Promise(resolve => resolve(config.projectId)) - : authClient.getProjectId(), - reqConfig.customEndpoint && reqConfig.useAuthWithCustomEndpoint !== true - ? // Using a custom API override. Do not use `google-auth-library` for - // authentication. (ex: connecting to a local Datastore server) - new Promise(resolve => resolve(reqOpts)) - : authClient.authorizeRequest(reqOpts), - ]) - .then(([_projectId, authorizedReqOpts]) => { - projectId = _projectId as string; - onAuthenticated(null, authorizedReqOpts as DecorateRequestOptions); - }) - .catch(onAuthenticated); + const prepareRequest = async () => { + try { + const getProjectId = async () => { + if ( + config.projectId && + config.projectId !== DEFAULT_PROJECT_ID_TOKEN + ) { + // The user provided a project ID. We don't need to check with the + // auth client, it could be incorrect. + return config.projectId; + } + + if (config.projectIdRequired === false) { + // A projectId is not required. Return the default. + return DEFAULT_PROJECT_ID_TOKEN; + } + + return setProjectId(); + }; + + const authorizeRequest = async () => { + if ( + reqConfig.customEndpoint && + !reqConfig.useAuthWithCustomEndpoint + ) { + // Using a custom API override. Do not use `google-auth-library` for + // authentication. (ex: connecting to a local Datastore server) + return reqOpts; + } else { + return authClient.authorizeRequest(reqOpts); + } + }; + + const [_projectId, authorizedReqOpts] = await Promise.all([ + getProjectId(), + authorizeRequest(), + ]); + + if (_projectId) { + projectId = _projectId; + } + + return onAuthenticated( + null, + authorizedReqOpts as DecorateRequestOptions + ); + } catch (e) { + return onAuthenticated(e as Error); + } + }; + + prepareRequest(); if (stream!) { return stream!; diff --git a/test/nodejs-common/service.ts b/test/nodejs-common/service.ts index 363164238..1ae57f9c7 100644 --- a/test/nodejs-common/service.ts +++ b/test/nodejs-common/service.ts @@ -22,7 +22,11 @@ import {Request} from 'teeny-request'; import {AuthClient, GoogleAuth, OAuth2Client} from 'google-auth-library'; import {Interceptor} from '../../src/nodejs-common'; -import {ServiceConfig, ServiceOptions} from '../../src/nodejs-common/service'; +import { + DEFAULT_PROJECT_ID_TOKEN, + ServiceConfig, + ServiceOptions, +} from '../../src/nodejs-common/service'; import { BodyResponseCallback, DecorateRequestOptions, @@ -228,7 +232,7 @@ describe('Service', () => { it('should default projectId with placeholder', () => { const service = new Service(fakeCfg, {}); - assert.strictEqual(service.projectId, '{{projectId}}'); + assert.strictEqual(service.projectId, DEFAULT_PROJECT_ID_TOKEN); }); it('should localize the projectIdRequired', () => { diff --git a/test/nodejs-common/util.ts b/test/nodejs-common/util.ts index c5a802c06..8f0afdb3e 100644 --- a/test/nodejs-common/util.ts +++ b/test/nodejs-common/util.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import {replaceProjectIdToken} from '@google-cloud/projectify'; +import { + MissingProjectIdError, + replaceProjectIdToken, +} from '@google-cloud/projectify'; import * as assert from 'assert'; import {describe, it, before, beforeEach, afterEach} from 'mocha'; import * as extend from 'extend'; @@ -45,6 +48,7 @@ import { ParsedHttpRespMessage, Util, } from '../../src/nodejs-common/util'; +import {DEFAULT_PROJECT_ID_TOKEN} from '../../src/nodejs-common/service'; // eslint-disable-next-line @typescript-eslint/no-var-requires const duplexify: DuplexifyConstructor = require('duplexify'); @@ -756,7 +760,7 @@ describe('common/util', () => { }); it('should not pass projectId token to google-auth-library', done => { - const config = {projectId: '{{projectId}}'}; + const config = {projectId: DEFAULT_PROJECT_ID_TOKEN}; sandbox.stub(fakeGoogleAuth, 'GoogleAuth').callsFake(config_ => { assert.strictEqual(config_.projectId, undefined); @@ -768,10 +772,10 @@ describe('common/util', () => { }); it('should not remove projectId from config object', done => { - const config = {projectId: '{{projectId}}'}; + const config = {projectId: DEFAULT_PROJECT_ID_TOKEN}; sandbox.stub(fakeGoogleAuth, 'GoogleAuth').callsFake(() => { - assert.strictEqual(config.projectId, '{{projectId}}'); + assert.strictEqual(config.projectId, DEFAULT_PROJECT_ID_TOKEN); setImmediate(done); return authClient; }); @@ -973,6 +977,65 @@ describe('common/util', () => { onAuthenticated: assert.ifError, }); }); + + it('should use default `projectId` and not call `authClient#getProjectId` when !`projectIdRequired`', done => { + const getProjectIdSpy = sandbox.spy(authClient, 'getProjectId'); + + sandbox.stub(fakeGoogleAuth, 'GoogleAuth').returns(authClient); + + const config = { + customEndpoint: true, + projectIdRequired: false, + }; + + stub('decorateRequest', (reqOpts, projectId) => { + assert.strictEqual(projectId, DEFAULT_PROJECT_ID_TOKEN); + }); + + const makeAuthenticatedRequest = + util.makeAuthenticatedRequestFactory(config); + + makeAuthenticatedRequest(reqOpts, { + onAuthenticated: e => { + assert.ifError(e); + assert(getProjectIdSpy.notCalled); + done(e); + }, + }); + }); + + it('should fallback to checking for a `projectId` on when missing a `projectId` when !`projectIdRequired`', done => { + const getProjectIdSpy = sandbox.spy(authClient, 'getProjectId'); + + sandbox.stub(fakeGoogleAuth, 'GoogleAuth').returns(authClient); + + const config = { + customEndpoint: true, + projectIdRequired: false, + }; + + const decorateRequestStub = sandbox.stub(util, 'decorateRequest'); + + decorateRequestStub.onFirstCall().callsFake(() => { + throw new MissingProjectIdError(); + }); + + decorateRequestStub.onSecondCall().callsFake((reqOpts, projectId) => { + assert.strictEqual(projectId, AUTH_CLIENT_PROJECT_ID); + return reqOpts; + }); + + const makeAuthenticatedRequest = + util.makeAuthenticatedRequestFactory(config); + + makeAuthenticatedRequest(reqOpts, { + onAuthenticated: e => { + assert.ifError(e); + assert(getProjectIdSpy.calledOnce); + done(e); + }, + }); + }); }); describe('authentication errors', () => { From eec865bf1845771c46dc5c89ee34380de6b5bfaa Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Tue, 21 Jun 2022 16:13:45 -0700 Subject: [PATCH 2/2] chore: `needs authentication` -> `authentication` --- test/nodejs-common/util.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/nodejs-common/util.ts b/test/nodejs-common/util.ts index 8f0afdb3e..732cc078e 100644 --- a/test/nodejs-common/util.ts +++ b/test/nodejs-common/util.ts @@ -905,7 +905,7 @@ describe('common/util', () => { }); }); - describe('needs authentication', () => { + describe('authentication', () => { it('should pass correct args to authorizeRequest', done => { const fake = extend(true, authClient, { authorizeRequest: async (rOpts: {}) => {