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

Handle special case of application default credentials location #444

Merged
merged 4 commits into from Jan 19, 2019

Conversation

yinzara
Copy link
Contributor

@yinzara yinzara commented Jan 17, 2019

When the GOOGLE_APPLICATION_CREDENTIALS environment variable is pointed to
the refresh token file created by 'gcloud auth application-default login',
Firebase admin would error as it tried to parse it as a certificate.
This fix doesn't attempt to parse the file as a certificate if the
variable points to the refresh token file and instead just attempts
refresh token file parsing. Test case added, ran, and verified. Eslint rules passed.

@hiranya911
Copy link
Contributor

ADC protocol only requires that the variable point to a service account json file: https://cloud.google.com/docs/authentication/production

First, ADC checks to see if the environment variable GOOGLE_APPLICATION_CREDENTIALS is set. If the variable is set, ADC uses the service account file that the variable points to. The next section describes how to set the environment variable.

However, in practice many implementations are lenient about this, and allow both service accounts and refresh token files. I'm ok with making the Node.js implementation do the same.

But right now there's a CI failure, and we cannot proceed until that is fixed.

@yinzara
Copy link
Contributor Author

yinzara commented Jan 17, 2019

Thank you. This is important in environments where you may pass your local application default credentials into a docker container in a dev environment but pass a service account key in production.

I didn't realize you aren't able to test refresh token validity in the CI environment until I read the other tests better. I've rewritten the test to use Sinon and mock the readFileSync in the case where there is no refresh token present during the test. I've rebased into a single commit.

When the GOOGLE_APPLICATION_CREDENTIALS environment variable is pointed to
the refresh token file created by 'gcloud auth application-default login',
Firebase admin would error as it tried to parse it as a certificate.
This fix doesn't attempt to parse the file as a certificate if the
variable points to the refresh token file and instead just attempts
refresh token file parsing
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Couple of changes suggested. Also please update the CHANGELOG file.

@@ -355,7 +355,10 @@ export class ApplicationDefaultCredential implements Credential {
private credential_: Credential;

constructor(httpAgent?: Agent) {
if (process.env.GOOGLE_APPLICATION_CREDENTIALS) {
if (process.env.GOOGLE_APPLICATION_CREDENTIALS &&
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to implement this we should be consistent with other languages. Specifically, there's no reason to special case GCLOUD_CREDENTIALS_PATH. Rather, we should do something like:

if (process.env.GOOGLE_APPLICATION_CREDENTIALS) {
  const fileContent = readFile(process.env.GOOGLE_APPLICATION_CREDENTIALS);
  if (fileContent.type === 'authorized_user') {
    // create refresh token credential
  } else if (fileContent.type === 'service_account') {
   // create cert credential  
  } else {
   // throw error
  }
}

it('should parse valid token if app def creds point to default refresh token loc', () => {
process.env.GOOGLE_APPLICATION_CREDENTIALS = GCLOUD_CREDENTIAL_PATH;

let readFileSyncStub;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an fsStub variable already in scope for these tests. See if we can use that here.

@hiranya911 hiranya911 self-assigned this Jan 17, 2019
@yinzara
Copy link
Contributor Author

yinzara commented Jan 18, 2019

Updated. Quite a bit more code to handle that and still keep all existing test cases passing but I did it and added a few more to cover the other code paths I added.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

I think this can be simplified, if we don't try to merge the existing logic in the constructor with the new logic. See my comments.

if (refreshToken) {
this.credential_ = new RefreshTokenCredential(refreshToken, httpAgent);
return;
if (typeof process.env.GOOGLE_APPLICATION_CREDENTIALS === 'string' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be this complex. You only have to modify the first if block in the existing constructor. Rest of the constructor can remain unchanged. Something like:

constructor(httpAgent?: Agent) {
    if (process.env.GOOGLE_APPLICATION_CREDENTIALS) {
      this.credential_ = credentialFromFile(process.env.GOOGLE_APPLICATION_CREDENTIALS, httpAgent);
      return;
    }

    // Rest is unchanged
}

function credentialFromFile(filePath: string, httpAgent?: Agent): Credential {
  const credentialsFile = readCredentialFile(filePath);
  if (typeof credentialsFile !== 'object') {
    throw new FirebaseAppError(
      AppErrorCodes.INVALID_CREDENTIAL,
      'Failed to parse contents of the credentials file as an object',
    );
  }
  if (credentialsFile.type === 'service_account') {
    return new CertCredential(credentialsFile, httpAgent);
  }
  if (credentialsFile.type === 'authorized_user') {
    return new RefreshTokenCredential(credentialsFile, httpAgent);
  }
  throw new FirebaseAppError(
    AppErrorCodes.INVALID_CREDENTIAL,
    'Invalid contents in the credentials file',
  );
}

function readCredentialFile(filePath: string): {[key: string]: any} {
  if (typeof filePath !== 'string') {
      throw new FirebaseAppError(
        AppErrorCodes.INVALID_CREDENTIAL,
        'Failed to parse credentials file: TypeError: path must be a string',
      );
  }
  let fileText: string;
  try {
    fileText = fs.readFileSync(filePath, 'utf8');
  } catch (error) {
    throw new FirebaseAppError(
      AppErrorCodes.INVALID_CREDENTIAL,
      `Failed to read credentials from file ${filePath}: ` + error,
    );
  }
  try {
    return JSON.parse(fileText);
  } catch (error) {
    throw new FirebaseAppError(
      AppErrorCodes.INVALID_CREDENTIAL,
      'Failed to parse contents of the credentials file as an object: ' + error,
    );
  }
}

@@ -395,6 +408,24 @@ describe('Credential', () => {
privateKey: mockCertificateObject.private_key,
});
});

it('should parse valid token if app def creds point to default refresh token loc', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets spell out words in full: should create a RefreshTokenCredential if application default credentials variable point to gcloud credentials path.

@yinzara
Copy link
Contributor Author

yinzara commented Jan 18, 2019

Changes made as requested

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks @yinzara. The Code LGTM 👍 . Please address the 2 comments I've left on tests, and also update the CHANGELOG file, and then I can merge.

test/unit/auth/credential.spec.ts Outdated Show resolved Hide resolved
@hiranya911 hiranya911 merged commit d52d133 into firebase:master Jan 19, 2019
Copy link

@Zaikyaw Zaikyaw left a comment

Choose a reason for hiding this comment

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

Zaikyaw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants