Allow createCustomToken() to work with tenant-aware auth #708

rsgowman · 2019-11-28T16:24:35Z

No description provided.

rsgowman · 2019-11-28T16:31:43Z

src/auth/token-generator.ts

+    return this.createCustomTokenInternal(uid, tenantId, developerClaims);
+  }
+
+  private createCustomTokenInternal(


Alternatively to having two separate createCustomToken*() methods, we could have one with an options object, i.e.:

interface CreateCustomTokenOptions { tenantId?: string; developerClaims?: {[key: string]: any}; } public createCustomToken(uid: string, options?: CreateCustomTokenOptions) { ... }

rsgowman · 2019-11-28T16:37:41Z

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

@@ -293,22 +294,22 @@ describe('FirebaseTokenGenerator', () => {
    it('should throw given no uid', () => {
      expect(() => {
        (tokenGenerator as any).createCustomToken();
-      }).to.throw('First argument to createCustomToken() must be a non-empty string uid');
+      }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error');


I've switched many of the error check tests from 'verify the error message' to 'verify the error code'. Personally, I prefer just verifying the code, as the error message itself often changes which makes the tests a little fragile. But this is hardly a universal opinion. I'm happy to add the error message back in if you'd prefer.

(Obviously, these changes in particular were caused by me tweaking the error message slightly and having a bunch of tests fail as a result.)

rsgowman · 2019-11-28T16:41:32Z

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

@@ -466,4 +467,215 @@ describe('FirebaseTokenGenerator', () => {
        });
    });
  });
+
+  describe('createCustomTokenWithTenantId()', () => {


These tests largely duplicate the createCustomToken() tests above, but validate that the path with tenantIds also works. Roughly half of these tests are pretty low (though not quite no) value and I'd be happy to remove them. i.e. the tests that validate the uid handling is correct, even when a tennantid is provided, e.g. 'should throw given an empty string uid'.

Opinions welcome.

You can follow a similar approach to other tests for common API used in a tenant and non-tenant context where shared code is used. This avoids the duplication and makes it easier to make changes in the future.

You mean like this?

firebase-admin-node/test/unit/auth/auth.spec.ts

Line 244 in bba61e1

AUTH_CONFIGS.forEach((testConfig) => {

The problem here is that:
a) We have a TokenGenerator (rather than an Auth object) and the TokenGenerator is not tenant aware.
b) The API for creating a custom token differs based on whether you want a tenantid included or not.

This problem is the more general problem of testing method overloads; I'm not aware of a great solution. Here's the obvious:
a) Just duplicate the tests. (i.e. as currently implemented)
b) Mock the implementation method and just verify that createCustomToken() pipes it's arguments through successfully. Possibly more pragmatic, but requires you to peek at the implementation (and to assume it won't change.)

In our case, we could also:
c) Make TokenGenerator tenant aware. Then we could use the same approach as for Auth instances. It seems a little odd... but I think it's workable with some refactoring around how the Auth instance creates its TokenGenerator.
d) Iterate over a list of functions, like this (which is pretty ugly):

const createTokenFns = [ (uid: string, tenantId: string, developerClaims?: {[key: string]: any}): Promise<string> => { return tokenGenerator.createCustomToken(string, developerClaims); } (uid: string, tenantId: string, developerClaims?: {[key: string]: any}): Promise<string> => { return tokenGenerator.createCustomTokenWithTenantId(string, tenantId, developerClaims); }]; createTokensFns.forEach((fn) => { describe("create custom token, possibly tenant aware", () => { it('should work', () => { expect(fn('uid', 'tenantid_that_might_be_ignored')).should.eventually.be.fulfilled; }); }); });

I don't like (d); I mildly don't like (b). (c) doesn't seem so bad though. wdyt?

TokenGenerator is created per Auth and TenantAwareAuth instance. The tenant ID in that case does not change after creation (could be set in the constructor). TokenGenerator has only one public method createCustomToken that mints tokens that are tenant aware. Every call of that should also set the tenant ID in the minted JWT. So maybe (c) is a good choice. But I am not sure how much work is involved in this. Do you want to explore and make an assessment?

It's not too much at all. Done.

test/integration/auth.spec.ts

bojeil-google

Thanks for putting this together. Took a quick look. Will take a more thorough look tomorrow.

bojeil-google · 2019-12-10T08:35:05Z

src/auth/token-generator.ts

+  /**
+   * Creates a new Firebase Auth Custom token.
+   *
+   * @param {string} uid The user ID to use for the generated Firebase Auth Custom token.


I know this is the current pattern but we should move away from re-stating the types, eg. string in the javadocs.
This is according to the internal typescript readability guideline.

Very happy to do this. (Done.) Obviously, I've only done this for the methods I've touched in this PR (createCustomToken*).

bojeil-google · 2019-12-10T08:42:52Z

test/unit/auth/auth.spec.ts

-        });
+        expect(() => {
+          mockCredentialAuth.createCustomToken(mocks.uid, mocks.developerClaims);
+        }).not.to.throw;


Seems to be a mismatch with the test description (should throw if...)

Huh; that's odd. Seems to go back to #285 which is literally called 'Support for creating custom tokens without service account credentials'. So the test was adjusted as part of that, but the test description was not. I've fixed it.

But wait! That's wrong too, since this test can't call "the IAM service in the cloud when the service account credentials are not provided." So instead of throwing immediately, it now returns a failed future. Fixed (for real this time) by ensuring it does fail, but not immediately. (Might be worth checking with @hiranya911 to see if this captures the original intent.)

bojeil-google · 2019-12-10T08:45:25Z

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

@@ -466,4 +467,215 @@ describe('FirebaseTokenGenerator', () => {
        });
    });
  });
+
+  describe('createCustomTokenWithTenantId()', () => {


You can follow a similar approach to other tests for common API used in a tenant and non-tenant context where shared code is used. This avoids the duplication and makes it easier to make changes in the future.

…tomtoken

Cuts down on the test duplication.

…tomtoken

bojeil-google

Latest changes look good. Just some minor comments.

bojeil-google · 2019-12-12T09:20:06Z

src/auth/auth.ts

-  constructor(app: FirebaseApp, protected readonly authRequestHandler: T) {
-    const cryptoSigner = cryptoSignerFromApp(app);
-    this.tokenGenerator = new FirebaseTokenGenerator(cryptoSigner);
+  constructor(app: FirebaseApp, protected readonly authRequestHandler: T, tokenGenerator?: FirebaseTokenGenerator) {


We should update the javadocs with the right number of parameters

Done. (Also removed {types})

bojeil-google · 2019-12-12T09:21:50Z

src/auth/token-generator.ts

    if (!validator.isNonNullObject(signer)) {
      throw new FirebaseAuthError(
        AuthClientErrorCode.INVALID_CREDENTIAL,
        'INTERNAL ASSERT: Must provide a CryptoSigner to use FirebaseTokenGenerator.',
      );
    }
+    if (tenantId !== undefined && !validator.isNonEmptyString(tenantId)) {


nit: typeof tenantId !== 'undefined'

bojeil-google · 2019-12-12T09:23:00Z

src/auth/token-generator.ts

+    if (tenantId !== undefined && !validator.isNonEmptyString(tenantId)) {
+      throw new FirebaseAuthError(
+        AuthClientErrorCode.INVALID_ARGUMENT,
+        '`tenantId` argument must be a non-empty string uid.');


Maybe remove uid mention as it may confuse the developer since we use it to refer to the user unique identifier?
tenantId argument must be a non-empty string.

Done. (I had copied an error message from somewhere else and adjusted it, but didn't remove the uid part.)

bojeil-google · 2019-12-12T09:31:17Z

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

-      expect(() => {
-        tokenGenerator.createCustomToken(uid);
-      }).not.to.throw();
+  const tokenGeneratorConfigs = [{


You can also declare an interface for this.

I don't think that's necessary; the type checker will still enforce everything via it's type deduction. (And I verified that, because I initially got it wrong.) If you want it anyways for doc purposes, or any other reason, I'm happy to add it in though.

bojeil-google · 2019-12-12T09:36:40Z

test/integration/auth.spec.ts

+
+      it('createCustomToken() mints a JWT that can be used to sign in tenant users', async () => {
+        try {
+          firebase.auth!().tenantId = createdTenantId;


Style guide does not seem to recommend non-nullability assertions without a reason.
https://g3doc.corp.google.com/javascript/typescript/g3doc/styleguide.md?cl=head#type-and-non-nullability-assertions

It allows it when the reason is "obvious". So I'd argue:
a) It's "obvious" that this is present because we've imported '@firebase/auth'.
b) It's overly verbose to comment each case (or to expect(firebase.auth).to.be.ok) each time.)
c) Worst case, something goes wrong and the test fails. Which is exactly what we'd want (albeit, the cause might not be as obvious as ideal.)

A possible compromise: It's reasonably obvious that this is the client auth instance, but not perfectly obvious. We could make a helper, and that helper could do the expect dance since it wouldn't clutter the tests. The helper would make it more obvious that this is the client instance (rather than admin.auth), so it'd serve that purpose too. (Done.)

bojeil-google · 2019-12-13T09:20:39Z

test/integration/auth.spec.ts

@@ -91,6 +91,10 @@ function randomOidcProviderId(): string {
  return 'oidc.' + generateRandomString(10, false).toLowerCase();
 }

+function client_auth(): FirebaseAuth {


Please use camel case clientAuth for function names here and elsewhere.

rsgowman self-assigned this Nov 28, 2019

Allow createCustomToken() to work with tenant-aware auth

8b02f7a

rsgowman force-pushed the rsgowman/tenant_customtoken branch from 5e6cb16 to 8b02f7a Compare November 28, 2019 16:30

rsgowman commented Nov 28, 2019

View reviewed changes

rsgowman assigned bojeil-google and unassigned rsgowman Nov 28, 2019

rsgowman requested review from bojeil-google and wuyanna November 28, 2019 16:43

wuyanna suggested changes Dec 4, 2019

View reviewed changes

test/integration/auth.spec.ts Outdated Show resolved Hide resolved

rsgowman added 2 commits December 5, 2019 11:33

Missed an await()

4648c08

remove unnecessary test

08dfb17

wuyanna approved these changes Dec 7, 2019

View reviewed changes

bojeil-google suggested changes Dec 10, 2019

View reviewed changes

rsgowman added 6 commits December 10, 2019 14:36

Merge remote-tracking branch 'origin/master' into rsgowman/tenant_cus…

53e6e6a

…tomtoken

Merge remote-tracking branch 'origin/master' into rsgowman/tenant_cus…

565989d

…tomtoken

review feedback

e749059

oops

a971e4a

Option c: Tenant aware TokenGenerator.

f9d8d66

Cuts down on the test duplication.

Merge remote-tracking branch 'origin/master' into rsgowman/tenant_cus…

cf4f87a

…tomtoken

bojeil-google suggested changes Dec 12, 2019

View reviewed changes

rsgowman added 3 commits December 12, 2019 11:49

more review feedback

f6937e4

firebase.auth!() -> client_auth()

0bccb66

Merge branch 'master' into rsgowman/tenant_customtoken

3f33a5e

bojeil-google suggested changes Dec 13, 2019

View reviewed changes

client_auth => clientAuth

0067272

bojeil-google approved these changes Dec 14, 2019

View reviewed changes

bojeil-google assigned rsgowman and unassigned bojeil-google Dec 14, 2019

Merge branch 'master' into rsgowman/tenant_customtoken

c70bc3b

rsgowman merged commit 99d3214 into master Dec 16, 2019

rsgowman deleted the rsgowman/tenant_customtoken branch December 16, 2019 20:52

This was referenced Feb 1, 2020

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

Open

[Snyk] Fix for 1 vulnerabilities Jeremip11/firebase-admin-node#2

Open

This was referenced Feb 27, 2020

Multi-tenancy firebase/firebase-admin-dotnet#152

Closed

TenantClient lacks custom tokens minting capabilities firebase/firebase-admin-go#337

Closed

snyk-bot mentioned this pull request Aug 17, 2020

[Snyk] Security upgrade @google-cloud/storage from 1.7.0 to 3.0.0 takeratta/firebase-admin-node#13

Open

Allow createCustomToken() to work with tenant-aware auth #708

Allow createCustomToken() to work with tenant-aware auth #708

Uh oh!

Conversation

rsgowman commented Nov 28, 2019

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!

Uh oh!

bojeil-google 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!

bojeil-google 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!

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!

Uh oh!