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

Support for creating custom tokens without service account credentials #285

Merged
merged 23 commits into from
Jul 12, 2018

Conversation

hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented May 31, 2018

Currently the SDK must be initialized with service account credentials in order to be able to call FirebaseAuth.createCustomToken(). With this PR, the SDK will attempt to sign custom tokens by calling the IAM service in the cloud when the service account credentials are not provided.

// Following will work in GCP managed runtimes where the Metadata service is present
admin.initializeApp();
admin.auth().createCustomToken(uid).then(...);
// Following will work anywhere
FirebaseApp.initializeApp({
  // need to only specify the service account email here...
  serviceAccountId: '[email protected]',
});
admin.auth().createCustomToken(uid).then(...);

As a part of this implementation I'm also refactoring the existing FirebaseTokenGenerator abstraction by removing all the token verification logic out of it.

go/firebase-admin-sign
go/firebase-admin-iam-sign

@hiranya911
Copy link
Contributor Author

Resolves #224

src/auth/auth.ts Outdated
@@ -26,6 +26,7 @@ import {

import * as utils from '../utils/index';
import * as validator from '../utils/validator';
import { FirebaseTokenVerifier, newSessionCookieVerifier, newIdTokenVerifier } from './token-verifier';
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename newSessionCookieVerifier and newIdTokenVerifier to createSessionCookieVerifier and createIdTokenVerifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

);
}

public sign(buffer: Buffer): Promise<Buffer> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add javadoc descriptions to all function declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* Cryptographically signs a buffer of data.
*
* @param {Buffer} buffer The data to be signed.
* @returns {Promise<object>} A promise that resolves with a base64-encoded signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Promise<Buffer>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*
* @returns {Promise<string>} A promise that resolves with a service account ID.
*/
getAccount(): Promise<string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

getAccountId()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/**
* Creates a new FirebaseTokenVerifier to verify Firebase ID tokens.
*
* @param projectId Project ID string.
Copy link
Contributor

Choose a reason for hiding this comment

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

 * @param {string} projectId Project ID string.
 * @return {FirebaseTokenVerifier}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


it('resolves when underlying idTokenVerifier.verifyJWT() resolves with expected result', () => {
idTokenVerifier.verifyJWT.withArgs(idToken).returns(Promise.resolve(decodedIdToken));
const auth = new Auth(mocks.app());
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. these tests should not depend on Auth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests basically check the verifyIdToken() and verifySessionCookie() methods on Auth. They are better off in auth.spec.

}).to.throw('Must provide a HTTP client to initialize IAMSigner');
});

describe('explicit service account', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

'explicit service account id'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


describe('explicit service account', () => {
const response = {signature: Buffer.from('testsignature').toString('base64')};
const input = Buffer.from('base64');
Copy link
Contributor

Choose a reason for hiding this comment

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

first encoding argument of toString() and the second argument of Buffer.from is `base64' making this confusing. Change to 'base64string' or something different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to input

});

describe('auto discovered service account', () => {
const input = Buffer.from('base64');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here on 'base64' first argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

});
});

it('should return the discovered service account', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test when getAccount() rejects with an error and confirm expected error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hiranya911
Copy link
Contributor Author

I'm posting a decoded custom token generated against the master branch and this branch for comparison. Notice that other than the order of some claims (which has no significance in JSON and JWT), nothing has changed. During the last code review round I noticed that the typ header was missing which I have now added (interestingly, Firebase Auth did not fail on that).

Tokens decoded via https://jwt.io/

JWT Header

Before:

{
  "alg": "RS256",
  "typ": "JWT"
}

After:

{
  "alg": "RS256",
  "typ": "JWT"
}

JWT Body

Before:

{
  "claims": {
    "isAdmin": true
  },
  "uid": "testuser",
  "iat": 1530216676,
  "exp": 1530220276,
  "aud": "https://identitytoolkit.googleapis.com/google.identity.identitytoolkit.v1.IdentityToolkit",
  "iss": "*******@*******.iam.gserviceaccount.com",
  "sub": "*******@*******.iam.gserviceaccount.com"
}

After:

{
  "aud": "https://identitytoolkit.googleapis.com/google.identity.identitytoolkit.v1.IdentityToolkit",
  "iat": 1530216493,
  "exp": 1530220093,
  "iss": "*******@*******.iam.gserviceaccount.com",
  "sub": "*******@*******.iam.gserviceaccount.com",
  "uid": "testuser",
  "claims": {
    "isAdmin": true
  }
}

@hiranya911
Copy link
Contributor Author

Resolves #224

Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. I would have preferred keeping the dependency on jsonwebtoken for minting custom tokens with service accounts but since this is undergoing a launchcal review, I leave the final say to reviewers.

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.

3 participants