chore: Move token verification logic to util #1189

lahirumaramba · 2021-03-08T20:42:50Z

Move FirebaseTokenVerifier to util
Include error type and codes to FirebaseTokenInfo to throw public error types from FirebaseTokenVerifier

hiranya911

Haven't looked at everything, but looks reasonable. Let's start off with some feedback on the high-level design.

src/utils/token-verifier.ts

src/utils/error.ts

src/auth/token-verifier-util.ts

… types.

hiranya911

Looks pretty good. Just a few suggestions for cleaning up the PR.

src/auth/token-verifier-util.ts

test/unit/auth/token-verifier.spec.ts

lahirumaramba · 2021-03-10T22:32:13Z

Merged the expired error code to error code config
Added validation for the new error code config and error type
Added unit tests for new validations

hiranya911

Thanks for making the changes. Looks mostly good. Couple of points to revise:

There's one remaining reference to auth from the utils token verifier.
Unit tests could use a bit more attention.

src/auth/token-verifier-util.ts

hiranya911 · 2021-03-10T23:19:42Z

src/utils/token-verifier.ts

@@ -213,9 +216,9 @@ export class FirebaseTokenVerifier {
          if (error) {
            if (error.name === 'TokenExpiredError') {
              const errorMessage = `${this.tokenInfo.jwtName} has expired. Get a fresh ${this.tokenInfo.shortName}` +
-                ` from your client app and try again (auth/${this.tokenInfo.expiredErrorCode.code}).` +
+                ` from your client app and try again (auth/${this.tokenInfo.errorCodeConfig.expiredError.code}).` +


There's a reference to auth here. Should probably get rid of it.

Oh good catch! I actually ended up finding more references to Auth. Ended up using generics in the verify token functions and moved the code to set the user id alias to one level up to auth.ts.
Let me know what you think. Thanks!

Re: the error code prefix. I just removed the auth prefix, but the error code feels a bit redundant here as the error code will also be included in the public error type.

This is something that will get cleaned up once we fix error handling for this SDK. For now this solution is adequate.

test/unit/auth/token-verifier.spec.ts

hiranya911 · 2021-03-10T23:23:30Z

test/unit/auth/token-verifier.spec.ts

+      });
+    });
+
+    const invalidErrorCodeConfigs = [null, NaN, 0, 1, true, false, [], {}, { a: 1 }, _.noop, '', 'test'];


These tests now belong in utils.

Moved the tests to test/utils

Let's not move the whole file. It still contains many auth-specific tests.

lahirumaramba · 2021-03-11T21:21:37Z

src/auth/auth.ts

      .then((decodedIdToken: DecodedIdToken) => {
+        decodedIdToken.uid = decodedIdToken.sub;


Moved the code to set the uid alias here. Another way is to add helper functions, verifySessionCookie() and verifyIdToken(), to auth/token-verifier. Let me know your thoughts.

How about extending the util.TokenVerifier as AuthTokenVerifier, and add the logic there?

lahirumaramba · 2021-03-11T21:46:11Z

src/utils/token-verifier.ts

+   * @return {Promise<T>} A promise fulfilled with the decoded claims of the Firebase Auth ID
+   *                           token.
+   */
+  public verifyJWT<T extends object>(jwtToken: string, isEmulator = false): Promise<T> {


@hiranya911 :
To make the type check tighter we can introduce a base type for returned tokens...

interface FirebaseToken { // registered JWT claims iss: string aud: string } interface DecodedIdToken extends FirebaseToken { ... } public verifyJWT<T extends FirebaseToken>

Let me know your thoughts. Thanks!

Yeah, this looks good to me. But I don't think we need a generic at all. Just make auth extend the TokenVerifier class, and transform the FirebaseToken into DecodedIdToken there.

// in utils interface DecodedToken { iss: string; aud: string; sub: string; } // in auth interface DecodedIdToken { iss: string; aud: string; sub: string; uid: string; } // at callsite const decodedToken: DecodedToken = await tokenVerifier.verifyToken(); const result: DecodedIdToken = {...decodedToken, uid: decodedToken.sub};

hiranya911

Lets find a more typesafe way to handle the returns. Plus separation of tests between auth and utils.

hiranya911 · 2021-03-11T21:45:14Z

src/auth/auth.ts

      .then((decodedIdToken: DecodedIdToken) => {
+        decodedIdToken.uid = decodedIdToken.sub;


How about extending the util.TokenVerifier as AuthTokenVerifier, and add the logic there?

hiranya911 · 2021-03-11T21:48:50Z

src/utils/token-verifier.ts

-    } else if (!validator.isNonNullObject(tokenInfo.expiredErrorCode) ||
-      !('code' in tokenInfo.expiredErrorCode)) {
-      throw new Error('The JWT expiration error code must be a non-null ErrorInfo object.');
+    } else if (!(typeof tokenInfo.errorType === 'function' && tokenInfo.errorType !== null)) {


typeof errorType === 'function' also asserts that errorType !== null. So the 2nd part of the conjunction is redundant.

hiranya911 · 2021-03-11T21:50:38Z

src/utils/token-verifier.ts

   *                           token.
   */
-  public verifyJWT(jwtToken: string, isEmulator = false): Promise<DecodedIdToken> {
+  public verifyJWT<T extends object>(jwtToken: string, isEmulator = false): Promise<T> {


This is breaking the type safety a bit. We should probably introduce a new interface for the return type of this class. And then convert it into an auth specific return type in auth/token-verifier.

hiranya911 · 2021-03-11T21:51:07Z

src/utils/token-verifier.ts

-            const decodedIdToken = (decodedToken as DecodedIdToken);
-            decodedIdToken.uid = decodedIdToken.sub;
-            resolve(decodedIdToken);
+            resolve(decodedToken as T);


Not particularly typesafe.

hiranya911 · 2021-03-11T21:53:45Z

src/utils/token-verifier.ts

+        'invalidCredential' in tokenInfo.errorCodeConfig ||
+        'internalError' in tokenInfo.errorCodeConfig ||
+        'expiredError' in tokenInfo.errorCodeConfig)) {
+      throw new Error('The provided error code config must be a non-null ErrorCodeInfo object.');


ErrorCodeConfig?

hiranya911 · 2021-03-11T22:02:56Z

test/unit/utils/token-verifier.spec.ts

@@ -30,10 +30,12 @@ import LegacyFirebaseTokenGenerator = require('firebase-token-generator');
 import * as mocks from '../../resources/mocks';
 import { FirebaseTokenGenerator, ServiceAccountSigner } from '../../../src/auth/token-generator';
 import * as verifier from '../../../src/utils/token-verifier';
-import * as verifierUtil from '../../../src/auth/token-verifier-util';
+import {


Sorry if I wasn't clear. But I didn't mean moving this entire file into utils. Some tests in this file are still very auth-specific, and therefore should be in auth. But the test cases that specifically test for the utils.token-verifier, should be moved into a new test module (like the invalidErrorTypes test cases).

I think a good rule of thumb is if a test requires an import from auth, then it probably belongs in auth.

lahirumaramba · 2021-03-24T23:07:58Z

Closing this as we decided to change the overall approach and design of the refactor. This work is replaced by #1204

lahirumaramba added the release:stage Stage a release candidate label Mar 8, 2021

chore: Move Token verification logic to util

8044f7d

lahirumaramba force-pushed the lm-refactor-verify branch from 534fcd6 to 8044f7d Compare March 8, 2021 20:47

lahirumaramba requested a review from hiranya911 March 8, 2021 20:47

hiranya911 suggested changes Mar 8, 2021

View reviewed changes

src/utils/token-verifier.ts Outdated Show resolved Hide resolved

src/utils/error.ts Outdated Show resolved Hide resolved

hiranya911 assigned lahirumaramba Mar 8, 2021

lahirumaramba commented Mar 9, 2021

View reviewed changes

src/auth/token-verifier-util.ts Outdated Show resolved Hide resolved

Moved Auth related items to token-verifier-util

6719cfa

lahirumaramba force-pushed the lm-refactor-verify branch from 7a1d9ea to 6719cfa Compare March 9, 2021 17:27

lahirumaramba added 2 commits March 9, 2021 13:50

Change the value errors in FirebaseTokenVerifier constructor to Error…

913ce37

… types.

Move the error type test into verifyJWT() tests

bba0b02

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Mar 9, 2021

hiranya911 suggested changes Mar 9, 2021

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Mar 9, 2021

lahirumaramba added 2 commits March 10, 2021 17:08

Merge expired error code to erro code config

27e0e6f

Fixed copyright years

5d3597a

lahirumaramba force-pushed the lm-refactor-verify branch from 1ac1020 to 5d3597a Compare March 10, 2021 22:28

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Mar 10, 2021

hiranya911 suggested changes Mar 10, 2021

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Mar 10, 2021

Remove all Auth dependencies from the token verifier

ac28a55

lahirumaramba commented Mar 11, 2021

View reviewed changes

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Mar 11, 2021

lahirumaramba commented Mar 11, 2021

View reviewed changes

hiranya911 suggested changes Mar 11, 2021

View reviewed changes

lahirumaramba assigned lahirumaramba and unassigned hiranya911 Mar 12, 2021

lahirumaramba closed this Mar 24, 2021

lahirumaramba deleted the lm-refactor-verify branch March 24, 2021 23:08

		.then((decodedIdToken: DecodedIdToken) => {
		decodedIdToken.uid = decodedIdToken.sub;

chore: Move token verification logic to util #1189

chore: Move token verification logic to util #1189

Uh oh!

Conversation

lahirumaramba commented Mar 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!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lahirumaramba commented Mar 10, 2021

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

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 Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

lahirumaramba commented Mar 24, 2021

Uh oh!

Uh oh!

lahirumaramba commented Mar 8, 2021 •

edited

Loading

lahirumaramba Mar 11, 2021 •

edited

Loading

hiranya911 Mar 11, 2021 •

edited

Loading