Skip to content

Commit

Permalink
fix: Requests Respect config.projectIdRequired === false (#1988)
Browse files Browse the repository at this point in the history 
* fix: Requests Respect `config.projectIdRequired` === `false`

* chore: `needs authentication` -> `authentication`
  • Loading branch information
@danielbankhead
danielbankhead committed Jun 22, 2022
1 parent 71a61ec commit 8813369
Show file tree
Hide file tree
Showing 4 changed files with 169 additions and 34 deletions.
6 changes: 3 additions & 3 deletions src/nodejs-common/service.ts
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -167,7 +167,7 @@ export class Service {

protected async getProjectIdAsync(): Promise<string> {
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;
Expand Down
116 changes: 92 additions & 24 deletions src/nodejs-common/util.ts
View file
Expand Up @@ -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';
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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
) => {
Expand All @@ -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);
}
}
}

Expand Down Expand Up @@ -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!;
Expand Down
8 changes: 6 additions & 2 deletions test/nodejs-common/service.ts
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -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', () => {
Expand Down
73 changes: 68 additions & 5 deletions test/nodejs-common/util.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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);
Expand All @@ -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;
});
Expand Down Expand Up @@ -901,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: {}) => {
Expand Down Expand Up @@ -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', () => {
Expand Down

0 comments on commit 8813369

Please sign in to comment.