feat(fac): Add custom TTL options for App Check #1363

lahirumaramba · 2021-07-08T19:26:47Z

Add new AppCheckTokenOptions type
Change the public API createToken(...) to accept optional AppCheckTokenOptions
Add validations for custom ttl
Move transformMillisecondsToSecondsString() to utils
Add unit tests. ~~(Integration tests will follow in a separate PR)~~
Add integration tests.

RELEASE NOTE: The createToken() API now supports configuring the TTL of the returned Firebase App Check Token.

hiranya911

LGTM with some suggestions.

hiranya911 · 2021-07-09T17:01:16Z

src/app-check/index.ts

  }

+  /**
+   * Interface representing an App Check token options.


Remove "an"?

hiranya911 · 2021-07-09T17:03:19Z

src/app-check/token-generator.ts

+        'AppCheckTokenOptions must be a non-null object.');
+    }
+    if (typeof options.ttlMillis !== 'undefined') {
+      if (!validator.isNumber(options.ttlMillis) || options.ttlMillis < 0) {


Negative check is redundant due to the following check.

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.

hiranya911 · 2021-07-09T17:08:43Z

test/unit/app-check/token-generator.spec.ts

+    [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 })


We should probably decode the token and verify the expected TTL is set.

Good call. Thanks!

hiranya911 · 2021-07-09T17:12:40Z

src/app-check/token-generator.ts

        aud: FIREBASE_APP_CHECK_AUDIENCE,
        exp: iat + ONE_HOUR_IN_SECONDS,
        iat,
+        ...customOptions,


Just curious. Why is ttl separate from exp?

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 · 2021-07-09T21:08:59Z

Thanks! Updated the code with PR fixes.

Adding @kevinthecheung to review the reference docs. Thank you!

hiranya911

Thanks. LGTM with a comment.

hiranya911 · 2021-07-09T22:23:55Z

test/unit/app-check/token-generator.spec.ts

-        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'],


One entry per line for clarity:

[ [], [], ... ]

kevinthecheung · 2021-07-10T00:42:05Z

src/app-check/index.ts

+     * 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.


Suggested change

* 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 3 times, most recently from 8d6197a to 89b759c Compare July 8, 2021 19:49

lahirumaramba requested a review from hiranya911 July 8, 2021 19:52

lahirumaramba assigned hiranya911 Jul 8, 2021

lahirumaramba requested a review from weixifan July 8, 2021 19:56

lahirumaramba force-pushed the lm-fac-config-ttl branch from 89b759c to 8dab1b2 Compare July 8, 2021 20:11

weixifan approved these changes Jul 9, 2021

View reviewed changes

hiranya911 approved these changes Jul 9, 2021

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Jul 9, 2021

hiranya911 reviewed Jul 9, 2021

View reviewed changes

lahirumaramba force-pushed the lm-fac-config-ttl branch from 644ca9a to 4090f74 Compare July 9, 2021 21:07

lahirumaramba requested a review from kevinthecheung July 9, 2021 21:08

lahirumaramba assigned kevinthecheung Jul 9, 2021

lahirumaramba added the release:stage Stage a release candidate label Jul 9, 2021

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Jul 9, 2021

hiranya911 approved these changes Jul 9, 2021

View reviewed changes

hiranya911 removed their assignment Jul 9, 2021

kevinthecheung approved these changes Jul 10, 2021

View reviewed changes

lahirumaramba assigned lahirumaramba and unassigned kevinthecheung Jul 12, 2021

lahirumaramba added 4 commits July 12, 2021 16:08

Add custom ttl options for App Check

765afcc

PR fixes

9e1154e

Add integration tests

53226ab

PR fixes

752437d

lahirumaramba force-pushed the lm-fac-config-ttl branch from 9ab022c to 752437d Compare July 12, 2021 20:08

lahirumaramba changed the title ~~Add custom TTL options for App Check~~ feat(fac): Add custom TTL options for App Check Jul 14, 2021

davidhin mentioned this pull request Jun 26, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#681

Open

davidhin mentioned this pull request Jul 6, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#682

Open

This was referenced Jul 17, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#685

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#687

Open

This was referenced Aug 10, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#697

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#700

Open

This was referenced Aug 28, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#707

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#709

Open

This was referenced Sep 5, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#711

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#712

Open

[Snyk] Upgrade: , dotenv, express, firebase-admin, nodemon, query-string davidhin/vuldoctor#713

Open

This was referenced Sep 13, 2024

[Snyk] Upgrade: , dotenv, express, firebase-admin, nodemon, query-string davidhin/vuldoctor#716

Open

[Snyk] Upgrade: , dotenv, express, firebase-admin, nodemon, query-string davidhin/vuldoctor#717

Open

This was referenced Oct 26, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#729

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#731

Open

davidhin mentioned this pull request Nov 9, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#732

Open

feat(fac): Add custom TTL options for App Check #1363

feat(fac): Add custom TTL options for App Check #1363

Uh oh!

Conversation

lahirumaramba commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahirumaramba commented Jul 9, 2021

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lahirumaramba commented Jul 8, 2021 •

edited

Loading