Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Fix impersonated service account parsing exception #1862

Merged
merged 14 commits into from
Jan 13, 2023
Merged
121 changes: 113 additions & 8 deletions src/app/credential-internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,107 @@ class RefreshToken {
}


/**
* Implementation of Credential that uses impersonated service account.
*/
export class ImpersonatedServiceAccountCredential implements Credential {

private readonly impersonatedServiceAccount: ImpersonatedServiceAccount;
private readonly httpClient: HttpClient;

/**
* Creates a new ImpersonatedServiceAccountCredential from the given parameters.
*
* @param impersonatedServiceAccountPathOrObject - Impersonated Service account json object or
* path to a service account json file.
* @param httpAgent - Optional http.Agent to use when calling the remote token server.
* @param implicit - An optinal boolean indicating whether this credential was implicitly
* discovered from the environment, as opposed to being explicitly specified by the developer.
*
* @constructor
*/
constructor(
impersonatedServiceAccountPathOrObject: string | object,
private readonly httpAgent?: Agent,
readonly implicit: boolean = false) {

this.impersonatedServiceAccount = (typeof impersonatedServiceAccountPathOrObject === 'string') ?
ImpersonatedServiceAccount.fromPath(impersonatedServiceAccountPathOrObject)
: new ImpersonatedServiceAccount(impersonatedServiceAccountPathOrObject);
this.httpClient = new HttpClient();
}

public getAccessToken(): Promise<GoogleOAuthAccessToken> {
const postData =
'client_id=' + this.impersonatedServiceAccount.clientId + '&' +
'client_secret=' + this.impersonatedServiceAccount.clientSecret + '&' +
'refresh_token=' + this.impersonatedServiceAccount.refreshToken + '&' +
'grant_type=refresh_token';
const request: HttpRequestConfig = {
method: 'POST',
url: `https://${REFRESH_TOKEN_HOST}${REFRESH_TOKEN_PATH}`,
headers: {
'Content-Type': 'application/x-www-form-urlencoded',
},
data: postData,
httpAgent: this.httpAgent,
};
return requestAccessToken(this.httpClient, request);
}
}

/**
* A struct containing the properties necessary to use impersonated service account JSON credentials.
*/
class ImpersonatedServiceAccount {

public readonly clientId: string;
public readonly clientSecret: string;
public readonly refreshToken: string;
public readonly type: string;

/*
* Tries to load a ImpersonatedServiceAccount from a path. Throws if the path doesn't exist or the
* data at the path is invalid.
*/
public static fromPath(filePath: string): ImpersonatedServiceAccount {
try {
return new ImpersonatedServiceAccount(JSON.parse(fs.readFileSync(filePath, 'utf8')));
} catch (error) {
// Throw a nicely formed error message if the file contents cannot be parsed
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
'Failed to parse impersonated service account file: ' + error,
);
}
}

constructor(json: object) {
const sourceCredentials = (json as {[key: string]: any})['source_credentials']
if (sourceCredentials) {
copyAttr(this, sourceCredentials, 'clientId', 'client_id');
copyAttr(this, sourceCredentials, 'clientSecret', 'client_secret');
copyAttr(this, sourceCredentials, 'refreshToken', 'refresh_token');
copyAttr(this, sourceCredentials, 'type', 'type');
}

let errorMessage;
if (!util.isNonEmptyString(this.clientId)) {
errorMessage = 'Impersonated Service Account must contain a "source_credentials.client_id" property.';
} else if (!util.isNonEmptyString(this.clientSecret)) {
errorMessage = 'Impersonated Service Account must contain a "source_credentials.client_secret" property.';
} else if (!util.isNonEmptyString(this.refreshToken)) {
errorMessage = 'Impersonated Service Account must contain a "source_credentials.refresh_token" property.';
} else if (!util.isNonEmptyString(this.type)) {
errorMessage = 'Impersonated Service Account must contain a "source_credentials.type" property.';
}

if (typeof errorMessage !== 'undefined') {
throw new FirebaseAppError(AppErrorCodes.INVALID_CREDENTIAL, errorMessage);
}
}
}

/**
* Checks if the given credential was loaded via the application default credentials mechanism. This
* includes all ComputeEngineCredential instances, and the ServiceAccountCredential and RefreshTokenCredential
Expand All @@ -377,20 +478,19 @@ class RefreshToken {
export function isApplicationDefault(credential?: Credential): boolean {
return credential instanceof ComputeEngineCredential ||
(credential instanceof ServiceAccountCredential && credential.implicit) ||
(credential instanceof RefreshTokenCredential && credential.implicit);
(credential instanceof RefreshTokenCredential && credential.implicit) ||
(credential instanceof ImpersonatedServiceAccountCredential && credential.implicit);
}

export function getApplicationDefault(httpAgent?: Agent): Credential {
if (process.env.GOOGLE_APPLICATION_CREDENTIALS) {
return credentialFromFile(process.env.GOOGLE_APPLICATION_CREDENTIALS, httpAgent);
return credentialFromFile(process.env.GOOGLE_APPLICATION_CREDENTIALS, httpAgent, false)!;
}

// It is OK to not have this file. If it is present, it must be valid.
if (GCLOUD_CREDENTIAL_PATH) {
const refreshToken = readCredentialFile(GCLOUD_CREDENTIAL_PATH, true);
if (refreshToken) {
return new RefreshTokenCredential(refreshToken, httpAgent, true);
}
const credential = credentialFromFile(GCLOUD_CREDENTIAL_PATH, httpAgent, true);
if (credential) return credential
}

return new ComputeEngineCredential(httpAgent);
Expand Down Expand Up @@ -474,9 +574,10 @@ function getDetailFromResponse(response: HttpResponse): string {
return response.text || 'Missing error payload';
}

function credentialFromFile(filePath: string, httpAgent?: Agent): Credential {
const credentialsFile = readCredentialFile(filePath);
function credentialFromFile(filePath: string, httpAgent?: Agent, ignoreMissing?: boolean): Credential | null {
const credentialsFile = readCredentialFile(filePath, ignoreMissing);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I am wrong here, but it looks like by setting ignoreMissing we turn off the Failed to read credentials from file error. I think if GOOGLE_APPLICATION_CREDENTIALS is set and getApplicationDefault() is called we should still throw if it is an incorrect file path, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, so I passed ignoreMissing as false when check GOOGLE_APPLICATION_CREDENTIALS on line 486. So we still can encounter FirebaseAppError when an incorrect file path is passed.

if (typeof credentialsFile !== 'object' || credentialsFile === null) {
if (ignoreMissing) { return null; }
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
'Failed to parse contents of the credentials file as an object',
Expand All @@ -491,6 +592,10 @@ function credentialFromFile(filePath: string, httpAgent?: Agent): Credential {
return new RefreshTokenCredential(credentialsFile, httpAgent, true);
}

if (credentialsFile.type === 'impersonated_service_account') {
return new ImpersonatedServiceAccountCredential(credentialsFile, httpAgent, true)
}

throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
'Invalid contents in the credentials file',
Expand Down
11 changes: 11 additions & 0 deletions test/resources/mock.impersonated_key.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"delegates": [],
"service_account_impersonation_url": "",
"source_credentials": {
"client_id": "client_id",
"client_secret": "client_secret",
"refresh_token": "refresh_token",
"type": "authorized_user"
},
"type": "impersonated_service_account"
}
44 changes: 43 additions & 1 deletion test/unit/app/credential-internal.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {
} from '../../../src/app/index';
import {
RefreshTokenCredential, ServiceAccountCredential,
ComputeEngineCredential, getApplicationDefault, isApplicationDefault
ComputeEngineCredential, getApplicationDefault, isApplicationDefault, ImpersonatedServiceAccountCredential
} from '../../../src/app/credential-internal';
import { HttpClient } from '../../../src/utils/api-request';
import { Agent } from 'https';
Expand All @@ -59,6 +59,17 @@ const MOCK_REFRESH_TOKEN_CONFIG = {
type: 'authorized_user',
refresh_token: 'test_token',
};
const MOCK_IMPERSONATED_TOKEN_CONFIG = {
delegates: [],
service_account_impersonation_url: '',
source_credentials: {
client_id: 'test_client_id',
client_secret: 'test_client_secret',
refresh_token: 'test_refresh_token',
type: 'authorized_user'
},
type: 'impersonated_service_account'
}

const ONE_HOUR_IN_SECONDS = 60 * 60;
const FIVE_MINUTES_IN_SECONDS = 5 * 60;
Expand Down Expand Up @@ -424,6 +435,13 @@ describe('Credential', () => {
expect(c).to.be.an.instanceof(ServiceAccountCredential);
});

it('should return a ImpersonatedCredential with impersonated GOOGLE_APPLICATION_CREDENTIALS set', () => {
process.env.GOOGLE_APPLICATION_CREDENTIALS
= path.resolve(__dirname, '../../resources/mock.impersonated_key.json');
const c = getApplicationDefault();
expect(c).to.be.an.instanceof(ImpersonatedServiceAccountCredential);
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add another test to httpAgent:

  it('ImpersonatedServiceAccountCredential should use the provided HTTP Agent', () => {
    const agent = new Agent();
    const c = new ImpersonatedServiceAccountCredential(MOCK_IMPERSONATED_TOKEN_CONFIG, agent);
    return c.getAccessToken().then((token) => {
      expect(token.access_token).to.equal(expectedToken);
      expect(stub).to.have.been.calledOnce;
      expect(stub.args[0][0].httpAgent).to.equal(agent);
    });
  });

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think we can improve the test coverage for ImpersonatedServiceAccountCredential. Please see describe('RefreshTokenCredential', () => { for an example.

Let's also add another test in describe('isApplicationDefault()', () => {

  it('should return true for ImpersonatedServiceAccountCredential loaded from GOOGLE_APPLICATION_CREDENTIALS', () => {
    process.env.GOOGLE_APPLICATION_CREDENTIALS = path.resolve(__dirname, '../../resources/mock.impersonated_key.json');
    const c = getApplicationDefault();
    expect(c).is.instanceOf(ImpersonatedServiceAccountCredential);
    expect(isApplicationDefault(c)).to.be.true;
  });

 it('should return false for explicitly loaded ImpersonatedServiceAccountCredential', () => {
    const c = new ImpersonatedServiceAccountCredential(mockImpersonatedAccount);
    expect(isApplicationDefault(c)).to.be.false;
  });

it('should throw if explicitly pointing to an invalid path', () => {
process.env.GOOGLE_APPLICATION_CREDENTIALS = 'invalidpath';
expect(() => getApplicationDefault()).to.throw(Error);
Expand Down Expand Up @@ -538,6 +556,15 @@ describe('Credential', () => {
expect(isApplicationDefault(c)).to.be.true;
});

it('should return true for ImpersonatedServiceAccountCredential loaded from GOOGLE_APPLICATION_CREDENTIALS', () => {
process.env.GOOGLE_APPLICATION_CREDENTIALS = path.resolve(
__dirname, '../../resources/mock.impersonated_key.json'
);
const c = getApplicationDefault();
expect(c).is.instanceOf(ImpersonatedServiceAccountCredential);
expect(isApplicationDefault(c)).to.be.true;
});

it('should return true for credential loaded from gcloud SDK', () => {
if (!fs.existsSync(GCLOUD_CREDENTIAL_PATH)) {
// tslint:disable-next-line:no-console
Expand Down Expand Up @@ -570,6 +597,11 @@ describe('Credential', () => {
expect(isApplicationDefault(c)).to.be.false;
});

it('should return false for explicitly loaded ImpersonatedServiceAccountCredential', () => {
const c = new ImpersonatedServiceAccountCredential(MOCK_IMPERSONATED_TOKEN_CONFIG);
expect(isApplicationDefault(c)).to.be.false;
});

it('should return false for custom credential', () => {
const c: Credential = {
getAccessToken: () => {
Expand Down Expand Up @@ -636,5 +668,15 @@ describe('Credential', () => {
expect(stub.args[0][0].httpAgent).to.equal(agent);
});
});

it('ImpersonatedServiceAccountCredential should use the provided HTTP Agent', () => {
const agent = new Agent();
const c = new ImpersonatedServiceAccountCredential(MOCK_IMPERSONATED_TOKEN_CONFIG, agent);
return c.getAccessToken().then((token) => {
expect(token.access_token).to.equal(expectedToken);
expect(stub).to.have.been.calledOnce;
expect(stub.args[0][0].httpAgent).to.equal(agent);
});
});
});
});