-
Notifications
You must be signed in to change notification settings - Fork 891
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
Add getIdTokenResult implementation #3014
Conversation
Binary Size ReportAffected SDKsNo changes between base commit (6b951a1) and head commit (f47ba5b). Test Logs
|
} | ||
|
||
export interface ParsedToken { | ||
[key: string]: string | ParsedToken; |
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.
a token can include another token?
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.
it can include a sub object yes
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.
recommend naming these differently then, ParsedToken should include standardized JWT fields on it like iat
, the nested object should be a FirebaseClaims
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-ish
I changed the index signature (which needs to cover all named types as well) to be string|object|undefined, the firebase specific type is then covered.
): Promise<IdTokenResult> { | ||
const token = await user.getIdToken(forceRefresh); | ||
const claims = parseClaims(token); | ||
// const firebase = claims?.firebase; |
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.
Remove?
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
|
||
function makeJWT(claims: object): string { | ||
const b64 = base64Encode(JSON.stringify(claims)); | ||
return `algorithm.${b64}.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.
Alex made the point later in the review that the middle part is often called payload. It's clear that this is base64 as well. Lets be consistent with whatever you choose!
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
@@ -0,0 +1,86 @@ | |||
/** |
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.
Any of this need javadoc?
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.
yeah that's the license
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.
Snarky but I meant the code itself. I was just pointing to the top of the file
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.
Ooooooh I misunderstood, my b
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.
I don't understand the github workflow and accidentally clicked "request changes". I'm going to switch my status to "approve with nits"
|
||
export interface ParsedToken { | ||
'exp'?: string; | ||
'auth_time'?: 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.
looks like auth_time is not a standard field, do we always have it present? https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields
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.
¯\_(ツ)_/¯ I'm just copying what the js version does. It's marked as ?:
optional so no it's not necessarily always there, that's why we do the assert
if (!idToken || !idToken['exp'] || !idToken['auth_time'] || !idToken['iat']) { |
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.
LGTM, but I still think we should not have nested PayloadToken, we should call it something else to make it clear that it's not a nested JWT
No description provided.