-
Notifications
You must be signed in to change notification settings - Fork 376
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
Conversation
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'; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/auth/token-generator.ts
Outdated
); | ||
} | ||
|
||
public sign(buffer: Buffer): Promise<Buffer> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/auth/token-generator.ts
Outdated
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise<Buffer>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/auth/token-generator.ts
Outdated
* | ||
* @returns {Promise<string>} A promise that resolves with a service account ID. | ||
*/ | ||
getAccount(): Promise<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getAccountId()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/auth/token-verifier.ts
Outdated
/** | ||
* Creates a new FirebaseTokenVerifier to verify Firebase ID tokens. | ||
* | ||
* @param projectId Project ID string. |
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'explicit service account id'
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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 Tokens decoded via https://jwt.io/ JWT HeaderBefore:
After:
JWT BodyBefore:
After:
|
Resolves #224 |
There was a problem hiding this 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.
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.
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