feat(fac): Add custom TTL options for App Check#1363
Conversation
lahirumaramba
force-pushed
the
lm-fac-config-ttl
branch
3 times, most recently
from
8d6197a to
89b759c
Compare
lahirumaramba
force-pushed
the
lm-fac-config-ttl
branch
from
89b759c to
8dab1b2
Compare
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
LGTM with some suggestions.
src/app-check/index.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Interface representing an App Check token options. |
src/app-check/token-generator.ts
Outdated
| 'AppCheckTokenOptions must be a non-null object.'); | ||
| } | ||
| if (typeof options.ttlMillis !== 'undefined') { | ||
| if (!validator.isNumber(options.ttlMillis) || options.ttlMillis < 0) { |
There was a problem hiding this comment.
Negative check is redundant due to the following check.
There was a problem hiding this comment.
I added the negative check here to specify that the ttl should be a non-negative duration in the error message. I agree though that it makes more sense to include/check that as part of the range validation below. Will update the code.
| [THIRTY_MIN_IN_MS, THIRTY_MIN_IN_MS + 1, SEVEN_DAYS_IN_MS / 2, SEVEN_DAYS_IN_MS - 1, SEVEN_DAYS_IN_MS] | ||
| .forEach((ttlMillis) => { | ||
| it('should be fulfilled with a Firebase Custom JWT with a valid custom ttl' + JSON.stringify(ttlMillis), () => { | ||
| return tokenGenerator.createCustomToken(APP_ID, { ttlMillis }) |
There was a problem hiding this comment.
We should probably decode the token and verify the expected TTL is set.
| aud: FIREBASE_APP_CHECK_AUDIENCE, | ||
| exp: iat + ONE_HOUR_IN_SECONDS, | ||
| iat, | ||
| ...customOptions, |
There was a problem hiding this comment.
Just curious. Why is ttl separate from exp?
There was a problem hiding this comment.
According to go/fac-configurable-ttls we are not exposing the option to use exp at all to keep the interface simple. So in this case a custom ttl will override exp.
lahirumaramba
force-pushed
the
lm-fac-config-ttl
branch
from
644ca9a to
4090f74
Compare
|
Thanks! Updated the code with PR fixes. Adding @kevinthecheung to review the reference docs. Thank you! |
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
Thanks. LGTM with a comment.
| it('should be fulfilled with a Firebase Custom JWT with a valid custom ttl' + JSON.stringify(ttlMillis), () => { | ||
| return tokenGenerator.createCustomToken(APP_ID, { ttlMillis }) | ||
| .should.eventually.be.a('string').and.not.be.empty; | ||
| [[THIRTY_MIN_IN_MS, '1800s'], [THIRTY_MIN_IN_MS + 1, '1800.001000000s'], |
There was a problem hiding this comment.
One entry per line for clarity:
[
[],
[],
...
]
src/app-check/index.ts
Outdated
| * The length of time measured in milliseconds starting from when the server | ||
| * mints the token for which the returned FAC token will be valid. | ||
| * This value must be in milliseconds and between 30 minutes and 7 days, inclusive. |
There was a problem hiding this comment.
| * The length of time measured in milliseconds starting from when the server | |
| * mints the token for which the returned FAC token will be valid. | |
| * This value must be in milliseconds and between 30 minutes and 7 days, inclusive. | |
| * The length of time, in milliseconds, for which the App Check token will | |
| * be valid. This value must be between 30 minutes and 7 days, inclusive. |
lahirumaramba
force-pushed
the
lm-fac-config-ttl
branch
from
9ab022c to
752437d
Compare
lahirumaramba
changed the title
4 participants
AppCheckTokenOptionstypecreateToken(...)to accept optionalAppCheckTokenOptionsttltransformMillisecondsToSecondsString()toutils(Integration tests will follow in a separate PR)RELEASE NOTE: The
createToken()API now supports configuring theTTLof the returned Firebase App Check Token.