Adds support for Firebase Auth session management. #245

bojeil-google · 2018-03-28T20:31:33Z

This adds 2 new APIs:
admin.auth().createSessionCookie(idToken: string, sessionCookieOptions:
SessionCookieOptions): Promise
admin.auth().verifySessionCookie(sessionCookie: string, checkRevoked?:
boolean): Promise

Refactored token generator and split token verification to a new class
FirebaseTokenVerifier so it can be used to also verify session cookies.
Kept the same error handling for backward compatibility.
In the process, ported the same tests to token verifier.
Updated token generator ID token and session cookie verification to
check token verifier is called underneath with the expected parameters.

Added integration tests to test all common flows for session cookie
creation and verification.
Added mocks for session cookie JWTs.
Fixed error in URL validator.
Increased integration test timeout to 5 seconds from the default 2 seconds to accommodate user deletion throttling.

This adds 2 new APIs: admin.auth().createSessionCookie(idToken: string, sessionCookieOptions: SessionCookieOptions): Promise<string> admin.auth().verifySessionCookie(sessionCookie: string, checkRevoked?: boolean): Promise<DecodedIdToken> Refactored token generator and split token verification to a new class FirebaseTokenVerifier so it can be used to also verify session cookies. Kept the same error handling for backward compatibility. In the process, ported the same tests to token verifier. Updated token generator ID token and session cookie verification to check token verifier is called underneath with the expected parameters. Added integration tests to test all common flows for session cookie creation and verification. Added mocks for session cookie JWTs. Fixed error in URL validator.

hiranya911

Looks pretty good. Just a few things to clean up.

hiranya911 · 2018-03-28T21:28:37Z

src/auth/auth-api-request.ts

+        // Set response validator.
+        .setResponseValidator((response: any) => {
+          // Response should always contain the session cookie.
+          if (!response.sessionCookie || !validator.isNonEmptyString(response.sessionCookie)) {


isNonEmptyString check should be sufficient here.

hiranya911 · 2018-03-28T21:29:23Z

src/auth/auth-api-request.ts

+   * The session cookie JWT will have the same payload claims as the provided ID token.
+   *
+   * @param {string} idToken The Firebase ID token to exchange for a session cookie.
+   * @param {number} expiresIn The session cookie duration.


Please specify time unit.

hiranya911 · 2018-03-28T21:36:42Z

src/auth/auth.ts

+   *     verification.
+   */
+  private verifyDecodedJWTNotRevoked(
+      decodedIdToken: DecodedIdToken, revocationError: FirebaseAuthError): Promise<DecodedIdToken> {


Nit: Perhaps just take the error code as the second argument? You can instantiate the FirebaseAuthError in this function. It is a little weird to pass an exception as an argument.

hiranya911 · 2018-03-28T21:39:57Z

src/auth/auth.ts

+   */
+  public createSessionCookie(
+      idToken: string, sessionCookieOptions: SessionCookieOptions): Promise<string> {
+    return this.authRequestHandler.createSessionCookie(


Should we validate the existence of expiresIn here and throw an error? (since it's a required field)

Ok I added a check. Note this will return a rejected promise. We do this for all other APIs here. We don't directly throw an error and return a rejected promise after validation in auth-api-request.ts.

hiranya911 · 2018-03-28T21:40:31Z

src/auth/auth.ts

+  public createSessionCookie(
+      idToken: string, sessionCookieOptions: SessionCookieOptions): Promise<string> {
+    return this.authRequestHandler.createSessionCookie(
+      idToken, sessionCookieOptions && sessionCookieOptions.expiresIn);


Why not just sessionCookieOptions.expiresIn?

There is a risk that if sessionCookieOptions is passed as undefined, an error is thrown. So we have to keep this.

Ok I changed it since I validate this above.

hiranya911 · 2018-03-28T22:07:08Z

src/utils/error.ts

@@ -318,6 +318,10 @@ export class AuthClientErrorCode {
    code: 'claims-too-large',
    message: 'Developer claims maximum payload size exceeded.',
  };
+  public static ID_TOKEN_EXPIRED = {


Are we referencing these constants anywhere?

The backend error TOKEN_EXPIRED will map to this client facing error. I think you can see this error in action in the integration tests I added.

hiranya911 · 2018-03-28T22:11:05Z

test/integration/auth.spec.ts

+        })
+        .then((sessionCookie) => admin.auth().verifySessionCookie(sessionCookie))
+        .then((decodedIdToken) => {
+          // Check for expected expiration with +/-2 seconds of variation.


Lets be on the safe side and use a 5s interval for tests.

hiranya911 · 2018-03-28T22:12:21Z

test/integration/auth.spec.ts

+          return admin.auth().verifySessionCookie(currentSessionCookie, true)
+            .should.eventually.be.rejected.and.have.property('code', 'auth/session-cookie-revoked');
+        });
+    }).timeout(5000);


Can we drop .timeout(5000) calls now that we are setting it in package.json?

hiranya911 · 2018-03-28T22:13:50Z

test/integration/auth.spec.ts

+        });
+    }).timeout(5000);
+
+    it('fails when called with an invalid ID token', () => {


It's probably enough to have tests like these that do not make remote API calls as unit tests.

Ok removed these 2 tests.

hiranya911 · 2018-03-28T22:20:31Z

test/unit/auth/auth-api-request.spec.ts

+        .then((resp) => {
+          throw new Error('Unexpected success');
+        }, (error) => {
+          expect(error).to.deep.equal(expectedError);


Does the error have some code that we can test here?

The server will return the error code INVALID_ID_TOKEN which gets mapped to developer facing error AuthClientErrorCode.INVALID_ID_TOKEN. The deep equal assertion here should test for exact code match.

hiranya911

LGTM 👍

Thanks for making the changes.

bojeil-google · 2018-03-30T22:40:30Z

Thanks for the prompt review, @hiranya911 !

hiranya911 · 2018-04-02T20:23:20Z

@bojeil-google I forgot to mention this in the code review. Please add an entry to the CHANGELOG file as part of this PR.

bojeil-google · 2018-04-02T22:35:20Z

I updated the changelog file. PTAL.

bojeil-google requested a review from hiranya911 March 28, 2018 21:13

bojeil-google assigned hiranya911 Mar 28, 2018

hiranya911 suggested changes Mar 28, 2018

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 Mar 28, 2018

Addresses review comments.

377a359

bojeil-google assigned hiranya911 and unassigned bojeil-google Mar 30, 2018

hiranya911 approved these changes Mar 30, 2018

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 Mar 30, 2018

Updated Changelog.

fbd6622

bojeil-google added 2 commits April 5, 2018 10:13

Updates the verifySessionCookie info URL to the finalized one.

d510162

Merge branch 'master' into session-management

95b798e

bojeil-google merged commit 49d6bcd into master Apr 5, 2018

bojeil-google deleted the session-management branch June 9, 2018 01:22

takeratta mentioned this pull request Dec 14, 2023

[Snyk] Fix for 1 vulnerabilities takeratta/firebase-admin-node#33

Open

This was referenced Jan 1, 2024

[Snyk] Fix for 1 vulnerabilities takeratta/firebase-admin-node#34

Open

[Snyk] Fix for 1 vulnerabilities takeratta/firebase-admin-node#35

Open

takeratta mentioned this pull request Mar 18, 2024

[Snyk] Fix for 1 vulnerabilities takeratta/firebase-admin-node#36

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for Firebase Auth session management. #245

Adds support for Firebase Auth session management. #245

bojeil-google commented Mar 28, 2018

hiranya911 left a comment

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 Mar 28, 2018

bojeil-google Mar 30, 2018

hiranya911 left a comment

bojeil-google commented Mar 30, 2018

hiranya911 commented Apr 2, 2018

bojeil-google commented Apr 2, 2018

Adds support for Firebase Auth session management. #245

Adds support for Firebase Auth session management. #245

Conversation

bojeil-google commented Mar 28, 2018

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

bojeil-google commented Mar 30, 2018

hiranya911 commented Apr 2, 2018

bojeil-google commented Apr 2, 2018