Starts defining multi-tenancy APIs. This includes: #526

bojeil-google · 2019-05-08T02:01:31Z

Defining type definitions.
Adding tenantId to UserRecord and UserImportBuilder.
Adding new errors associated with tenant operations.
Defines the Tenant object.

As the changes are quite large. This will be split into multiple PRs.

- Defining type definitions. - Adding tenantId to UserRecord and UserImportBuilder. - Adding new errors associated with tenant operations. - Defines the Tenant object. As the changes are quite large. This will be split into multiple PRs.

hiranya911

I haven't gone through the tests yet, but the implementation looks pretty good. Just a few suggestions to make the code more TypeScript idiomatic. In general:

Reduce the use of any in method arguments, and use specific types/interfaces whenever possible. If you must use any, see {[key: string]: any} is a better alternative.
Double check that the exported things really need to be exported.

hiranya911 · 2019-05-08T22:47:14Z

src/auth/auth-config.ts

+        'INTERNAL ASSERT FAILED: Invalid email sign-in configuration response');
+    }
+    utils.addReadonlyGetter(this, 'enabled', response.allowPasswordSignup);
+    utils.addReadonlyGetter(this, 'passwordRequired', !response.enableEmailLinkSignin);


Assign to the attributes here.

this.enabled = response.allowPasswordSignup; this.passwordRequired = !response.enableEmailLinkSignin;

hiranya911 · 2019-05-08T22:48:24Z

src/auth/auth-config.ts

+   *     EmailSignInConfig object.
+   * @constructor
+   */
+  constructor(response: any) {


Use type {[key: string]: any}

hiranya911 · 2019-05-08T22:49:11Z

src/auth/auth-config.ts

+   *
+   * @param {any} options The options object to validate.
+   */
+  public static validate(options: any) {


Use type {[key: string]: any}

hiranya911 · 2019-05-08T22:49:34Z

src/auth/auth-config.ts

+   * @param {any} options The options object to convert to a server request.
+   * @return {EmailSignInConfigServerRequest} The resulting server request.
+   */
+  public static buildServerRequest(options: any): EmailSignInConfigServerRequest {


Use type {[key: string]: any}

PS: Seeing how this get called, you can probably even annotate this with type EmailSignInProviderConfig. We still need to do the validations due to type erasure at runtime.

Changed to EmailSignInProviderConfig

hiranya911 · 2019-05-08T22:51:40Z

src/auth/auth-config.ts

+   *
+   * @param {any} options The options object to validate.
+   */
+  public static validate(options: any) {


Not sure if this is getting called from elsewhere. If not lets make it private.

hiranya911 · 2019-05-08T23:00:34Z

src/auth/tenant.ts

+   * @param {any} request The tenant options object to validate.
+   * @param {boolean} createRequest Whether this is a create request.
+   */
+  public static validate(request: any, createRequest: boolean) {


Can be private?

hiranya911 · 2019-05-08T23:06:19Z

src/auth/tenant.ts

+        'INTERNAL ASSERT FAILED: Invalid tenant response',
+      );
+    }
+    utils.addReadonlyGetter(this, 'tenantId', tenantId);


Directly assign to the readonly attributed you have defined above.

src/auth/tenant.ts

hiranya911 · 2019-05-08T23:08:56Z

src/auth/tenant.ts

+
+  /** @return {object} The plain object representation of the tenant. */
+  public toJSON(): object {
+    const json: any = {


Inline with return:

return { ... };

hiranya911 · 2019-05-08T23:10:32Z

src/auth/user-record.ts

@@ -187,6 +188,7 @@ export class UserRecord {
      validAfterTime = parseDate(response.validSince * 1000);
    }
    utils.addReadonlyGetter(this, 'tokensValidAfterTime', validAfterTime || undefined);
+    utils.addReadonlyGetter(this, 'tenantId', response.tenantId);


We can directly assign to this.tenantId here, but since we've already used the utils method in this constructor, it's ok to leave it as is.

Let's make the change in a separate PR.

hiranya911

Looks pretty good. Just a few nits.

hiranya911 · 2019-05-22T20:43:25Z

src/auth/tenant.ts

+    try {
+      this.emailSignInConfig = new EmailSignInConfig(response);
+    } catch (e) {
+      this.emailSignInConfig = undefined;


Might want to put a comment here to indicate why it's ok to ignore the error.

hiranya911 · 2019-05-22T20:45:47Z

src/auth/tenant.ts

+        `"${label}.displayName" must be a valid non-empty string.`,
+      );
+    }
+    // Validate type type if provided.


extraneous type

hiranya911 · 2019-05-22T20:48:22Z

src/auth/tenant.ts

+  public readonly tenantId: string;
+  public readonly type?: TenantType;
+  public readonly displayName?: string;
+  public readonly emailSignInConfig?: EmailSignInConfig | null;


Is this ever assigned null?

It used to but not anymore. Removed it.

hiranya911 · 2019-05-22T20:54:16Z

test/unit/auth/auth-config.spec.ts

+    });
+
+    it('should set readonly property "enabled" to false on allowPasswordSignup disabled', () => {
+      const validConfig2 = new EmailSignInConfig({


Nit: Give a more descriptive name to the variable that explains the context. Perhaps passwordSignupDisabledConfig?

hiranya911 · 2019-05-22T20:54:38Z

test/unit/auth/auth-config.spec.ts

+    });
+
+    it('should set readonly property "passwordRequired" to true on email link sign in disabled', () => {
+      const validConfig2 = new EmailSignInConfig({


const emailLinkSignInDisabledConfig

Switched to passwordSignupEnabledConfig.

hiranya911 · 2019-05-22T21:02:33Z

test/unit/auth/tenant.spec.ts

+        }).to.throw('"EmailSignInConfig.enabled" must be a boolean.');
+      });
+
+      it('should throw when type is specified', () => {


should throw when type is specified in an update request

hiranya911 · 2019-05-22T21:04:02Z

test/unit/auth/tenant.spec.ts

+      it('should throw when type is specified', () => {
+        const tenantOptionsClientRequest: TenantOptions = deepCopy(tenantOptions);
+        tenantOptionsClientRequest.type = 'lightweight';
+        expect(() => Tenant.buildServerRequest(tenantOptionsClientRequest, false))


Calls like this can be made more readable if you define a constant somewhere for false:

const createRequest = true; Tenant.buildServerRequest(tenantOptionsClientRequest, !createRequest)

hiranya911 · 2019-05-22T21:06:14Z

test/unit/auth/tenant.spec.ts

+        });
+      });
+
+      it('should throw on update with specified type', () => {


Seems like a duplicate.

hiranya911 · 2019-05-22T21:07:44Z

test/unit/auth/tenant.spec.ts

+        .to.equal('TENANT_ID');
+    });
+
+    it('should return the expected tenant ID from resource name with multiple tenants substring', () => {


The only difference from the previous test case is that this seems to use a different project ID. Was that intentional?

I updated the description. I am testing when the projectId contains the "tenants" substring.

hiranya911 · 2019-05-22T21:10:50Z

test/unit/auth/user-record.spec.ts

 * @return {object} A sample valid user response as returned from getAccountInfo
 *     endpoint.
 */
-function getValidUserResponse(): object {
-  return {
+function getValidUserResponse(tenantId?: string): any {


Use {[key: string}: any} instead of any.

hiranya911

LGTM 👍

hiranya911 · 2019-05-23T19:58:38Z

I see one of the GCB triggers are stuck. Let me know if that's somehow preventing you from being able to merge this.

bojeil-google · 2019-05-23T20:44:38Z

Yeah I am unable to squash and merge due to that.

bojeil-google added 2 commits May 7, 2019 18:47

Minor fixes and tweaks.

7770609

bojeil-google assigned hiranya911 May 8, 2019

hiranya911 suggested changes May 8, 2019

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 May 20, 2019

Addresses comments from review.

03926f0

bojeil-google assigned hiranya911 and unassigned bojeil-google May 21, 2019

hiranya911 suggested changes May 22, 2019

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 May 22, 2019

Addresses review comments.

d84dcd4

bojeil-google assigned hiranya911 and unassigned bojeil-google May 23, 2019

hiranya911 approved these changes May 23, 2019

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 May 23, 2019

bojeil-google assigned hiranya911 and unassigned bojeil-google May 23, 2019

hiranya911 merged commit f94423f into auth-multi-tenancy May 23, 2019

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

snyk-bot mentioned this pull request Dec 24, 2020

[Snyk] Security upgrade @google-cloud/firestore from 0.15.2 to 0.21.0 Jeremip11/firebase-admin-node#6

Open

snyk-bot mentioned this pull request Sep 6, 2021

[Snyk] Security upgrade @google-cloud/firestore from 0.15.2 to 0.21.0 Jeremip11/firebase-admin-node#9

Open

Jeremip11 mentioned this pull request Oct 27, 2023

[Snyk] Security upgrade @google-cloud/firestore from 0.15.2 to 0.21.0 Jeremip11/firebase-admin-node#16

Open

Jeremip11 mentioned this pull request Dec 14, 2023

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

Open

This was referenced Jan 1, 2024

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

Open

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

Open

Jeremip11 mentioned this pull request Mar 15, 2024

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

Open

Starts defining multi-tenancy APIs. This includes: #526

Starts defining multi-tenancy APIs. This includes: #526

Conversation

bojeil-google commented May 8, 2019

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

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

hiranya911 left a comment

Choose a reason for hiding this comment

hiranya911 commented May 23, 2019

bojeil-google commented May 23, 2019