Multi-tenancy changes to shared Auth API requests by bojeil-google · Pull Request #539 · firebase/firebase-admin-node

bojeil-google · 2019-05-24T08:14:45Z

Defines BaseFirebaseAuthRequestHandler class for sending Auth requests related to user management APIs and SAML/OIDC config mgmt APIs, link generation, etc.

Defines FirebaseAuthRequestHandler which extends the base class for project level only calls and which will also be extended to include tenant mgmt APIs.
Defines FirebaseTenantRequestHandler which extends the base class for tenant level only calls.

Unit tests have been modified to run tests on both subclasses. 99% of the unit test logic is the same. It has been tweaked to accept a common interface handler. 2 tests are run for each. One for FirebaseAuthRequestHandler and another for FirebaseTenantRequestHandler.

…s related to user management APIs and SAML/OIDC config mgmt APIs, link generation, etc. Defines FirebaseAuthRequestHandler which extends the base class for project level only calls and which will also be extended to include tenant mgmt APIs. Defines FirebaseTenantRequestHandler which extends the base class for tenant level only calls. Unit tests have been modified to run tests on both subclasses.

wuyanna · 2019-05-24T21:53:22Z

src/auth/auth-api-request.ts

    phoneNumber: true,
    customAttributes: true,
    validSince: true,
+    tenantId: true,


What does the boolean value mean here?

These are the list of allowed parameters to pass to the backend for create/edit requests (it's a quick way to generate a Set). If the developer passes other parameters, they will be either ignored or an error thrown.

wuyanna · 2019-05-24T21:58:30Z

src/auth/auth-api-request.ts

    }
  }
+  if (typeof request.tenantId !== 'undefined' &&
+      !validator.isNonEmptyString(request.tenantId)) {


Shouldn't the check be the mismatch between tenantId in the request and the tenantId in the user?

For now, I am only doing basic validation (ensure tenant ID is non-empty string when provided). I am leaving the check to the backend to enforce the match. I wasn't sure that uploadAccount allows uploading users with different tenants. That is why I am not checking mismatch. For now, I will rely on backend check. I added the mismatch tenant ID backend error catching I think i can followup with a mismatch check client side.

There probably won't be a mismatch tenant ID backend error. For UploadAccount you can always rely on backend returning error messages in the response for specific failed users. For the other endpoints like SetAccountInfo and SignupNewUsers, the backend won't be able to return the error since it only receives one tenant_id from the request as I mentioned in a separate email thread. Adding the client side check in a follow-up change sounds good to me.

I see. Yeah I will add the mismatch check in a follow up.

hiranya911

Tests are somewhat complicated to review. I trust it's just the existing test cases configured to run with both handler implementations.

Just a couple of suggestions to improve readability and the design.

hiranya911 · 2019-05-28T21:09:46Z

src/auth/auth-api-request.ts

+  constructor(protected projectId: string, protected version: string, protected tenantId: string) {
+    super(projectId, version);
+    // Inject tenant path in URL.
+    this.urlFormat = this.urlFormat.replace(


It looks like urlFormat in the parent class is really just a constant. Can we just define a constant for this as well?

hiranya911 · 2019-05-28T21:11:57Z

src/auth/auth-api-request.ts

-  private readonly httpClient: AuthorizedHttpClient;
-  private readonly authUrlBuilder: AuthResourceUrlBuilder;
-  private readonly projectConfigUrlBuilder: AuthResourceUrlBuilder;
+export class BaseFirebaseAuthRequestHandler {


Can we make this class abstract and rename it to AbstractAuthRequestHandler?

getAuthUrlBuilder() and getProjectConfigUrlBuilder() could be abstract methods of the class.

hiranya911 · 2019-05-28T21:13:43Z

src/auth/auth-api-request.ts

+ * tenant management related APIs. This extends the BaseFirebaseAuthRequestHandler class and defines
+ * additional tenant management related APIs.
+ */
+export class FirebaseAuthRequestHandler extends BaseFirebaseAuthRequestHandler {


My suggestions for names would be AuthRequestHandler and TenantAwareAuthRequestHandler.

bojeil-google

Regarding the test changes, this is basically what I am doing:

Old test:

describe('FirebaseAuthRequestHandler', () => {
  describe('getAccountInfoByUid', () => {
    const path = '/v1/projects/project_id/accounts:lookup';
    const method = 'POST';
    it('should be fulfilled given a valid localId', () => {
      const expectedResult = utils.responseFrom({
        users : [
          {localId: 'uid'},
        ],
      });
      const data = {localId: ['uid']};
      const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedResult);
      stubs.push(stub);

      const requestHandler = new FirebaseAuthRequestHandler(mockApp);
      return requestHandler.getAccountInfoByUid('uid')
        .then((result) => {
          expect(result).to.deep.equal(expectedResult.data);
          expect(stub).to.have.been.calledOnce.and.calledWith(callParams(path, method, data));
        });
    });
  });
});

Changed test:

const AUTH_REQUEST_HANDLER_TESTS: HandlerTest[] = [
  {
    name: 'FirebaseAuthRequestHandler',
    init: (app: FirebaseApp) => {
      return new AuthRequestHandler(app);
    },
    path: (version: string, api: string, projectId: string) => {
      return `/${version}/projects/${projectId}${api}`;
    },
    supportsTenantManagement: true,
  },
  {
    name: 'FirebaseTenantRequestHandler',
    init: (app: FirebaseApp) => {
      return new TenantAwareAuthRequestHandler(app, TENANT_ID);
    },
    path: (version: string, api: string, projectId: string) => {
      return `/${version}/projects/${projectId}/tenants/${TENANT_ID}${api}`;
    },
    supportsTenantManagement: false,
  },
];

const TENANT_ID = 'tenantId';
AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
  describe(handler.name, () => {
    describe('getAccountInfoByUid', () => {
      const path = handler.path('v1', '/accounts:lookup', 'project_id');
      const method = 'POST';
      it('should be fulfilled given a valid localId', () => {
        const expectedResult = utils.responseFrom({
          users : [
            {localId: 'uid'},
          ],
        });
        const data = {localId: ['uid']};
        const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedResult);
        stubs.push(stub);

        const requestHandler = handler.init(mockApp);
        return requestHandler.getAccountInfoByUid('uid')
          .then((result) => {
            expect(result).to.deep.equal(expectedResult.data);
            expect(stub).to.have.been.calledOnce.and.calledWith(callParams(path, method, data));
          });
      });
  });
});

The main changes is that I introduced AUTH_REQUEST_HANDLER_TESTS to test both subclasses.
Each test I get the associated handler and get path to check in request by calling handler.path().
In addition, instead of initializing const requestHandler = new AuthRequestHandler(mockApp) manually, I use const requestHandler = handler.init(mockApp).

The logic is the same. However, GitHub's diff gets super confused from these changes. Note that Visual Studio Code does a great job catching the actual changes.

bojeil-google · 2019-06-01T01:16:01Z

src/auth/auth-api-request.ts

+  constructor(protected projectId: string, protected version: string, protected tenantId: string) {
+    super(projectId, version);
+    // Inject tenant path in URL.
+    this.urlFormat = this.urlFormat.replace(


bojeil-google · 2019-06-01T01:17:47Z

src/auth/auth-api-request.ts

    phoneNumber: true,
    customAttributes: true,
    validSince: true,
+    tenantId: true,


These are the list of allowed parameters to pass to the backend for create/edit requests (it's a quick way to generate a Set). If the developer passes other parameters, they will be either ignored or an error thrown.

bojeil-google · 2019-06-01T01:25:15Z

src/auth/auth-api-request.ts

    }
  }
+  if (typeof request.tenantId !== 'undefined' &&
+      !validator.isNonEmptyString(request.tenantId)) {


For now, I am only doing basic validation (ensure tenant ID is non-empty string when provided). I am leaving the check to the backend to enforce the match. I wasn't sure that uploadAccount allows uploading users with different tenants. That is why I am not checking mismatch. For now, I will rely on backend check. I added the mismatch tenant ID backend error catching I think i can followup with a mismatch check client side.

bojeil-google · 2019-06-01T02:36:48Z

src/auth/auth-api-request.ts

+ * tenant management related APIs. This extends the BaseFirebaseAuthRequestHandler class and defines
+ * additional tenant management related APIs.
+ */
+export class FirebaseAuthRequestHandler extends BaseFirebaseAuthRequestHandler {


bojeil-google · 2019-06-01T02:36:58Z

src/auth/auth-api-request.ts

-  private readonly httpClient: AuthorizedHttpClient;
-  private readonly authUrlBuilder: AuthResourceUrlBuilder;
-  private readonly projectConfigUrlBuilder: AuthResourceUrlBuilder;
+export class BaseFirebaseAuthRequestHandler {


hiranya911

Looks great. Just one suggestion to keep couple of attributes as readonly.

hiranya911 · 2019-06-03T20:57:29Z

src/auth/auth-api-request.ts

+  protected readonly projectId: string;
+  protected readonly httpClient: AuthorizedHttpClient;
+  protected authUrlBuilder: AuthResourceUrlBuilder;
+  protected projectConfigUrlBuilder: AuthResourceUrlBuilder;


We can keep these 2 properties readonly. Lets initialize them in the constructor of the abstract class, by calling the abstract methods:

this.authUrlBuilder = this.getAuthUrlBuilder(); this.projectConfigUrlBuilder = this.getProjectConfigUrlBuilder();

You can call the abstract methods newAuthUrlBuilder() and newProjectConfigUrlBuilder() if that sounds better.

hiranya911 · 2019-06-03T20:58:39Z

src/auth/auth-api-request.ts

+  /**
+   * @return {AuthResourceUrlBuilder} The Auth user management resource URL builder.
+   */
+  protected getAuthUrlBuilder(): AuthResourceUrlBuilder {


Once we make the authUrlBuilder readonly, these method implementation will become simplified:

protected getAuthUrlBuilder() { return new AuthResourceUrlBuilder(this.projectId, 'v1'); }

hiranya911

LGTM 👍

bojeil-google requested review from hiranya911 and wuyanna May 24, 2019 08:16

bojeil-google assigned wuyanna and hiranya911 May 24, 2019

wuyanna reviewed May 24, 2019

View reviewed changes

hiranya911 suggested changes May 28, 2019

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 and wuyanna May 29, 2019

bojeil-google added 2 commits May 31, 2019 19:41

Addresses review comments.

1b8b2ae

Comment clean up.

45e0153

bojeil-google commented Jun 1, 2019

View reviewed changes

bojeil-google assigned wuyanna and hiranya911 and unassigned bojeil-google Jun 1, 2019

wuyanna approved these changes Jun 3, 2019

View reviewed changes

hiranya911 suggested changes Jun 3, 2019

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 Jun 3, 2019

Address more comments.

b450bfb

hiranya911 approved these changes Jun 3, 2019

View reviewed changes

hiranya911 unassigned wuyanna Jun 3, 2019

hiranya911 merged commit 5c7d44d into auth-multi-tenancy Jun 3, 2019

bojeil-google deleted the temp-multi-tenancy2 branch June 4, 2019 08:22

snyk-bot mentioned this pull request Jun 12, 2021

[Snyk] Upgrade jsonwebtoken from 8.1.0 to 8.5.1 RebelTat/firebase-admin-node#19

Open

Comments

Conversation

bojeil-google commented May 24, 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!

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!

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!

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!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants