feat(auth): Add bulk get/delete methods #726

rsgowman · 2019-12-12T23:19:28Z

This PR allows callers to retrieve a list of users by unique identifier (uid, email, phone, federated provider uid) as well as to delete a list of users.

RELEASE NOTE: Added getUsers() and deleteUsers() APIs for retrieving and deleting user accounts in bulk.

Avoids 'already-exists' errors. Note that these errors would occur not just for already existing uids, but also for email and phoneNumber fields.

hiranya911

Looks mostly good. Just a few suggestions and questions.

src/utils/error.ts

src/auth/auth.ts

src/auth/auth-api-request.ts

test/unit/auth/auth.spec.ts

test/integration/auth.spec.ts

hiranya911

LGTM 👍

Thanks @rsgowman

src/auth/auth-api-request.ts

test/integration/auth.spec.ts

bojeil-google

Thanks for putting this together. Working my way through it.

bojeil-google · 2020-01-10T08:12:39Z

src/auth/auth-api-request.ts

-  .setRequestValidator((request: any) => {
-    if (!request.localId && !request.email && !request.phoneNumber) {
+  .setRequestValidator((request: GetAccountInfoRequest) => {
+    if (!request.localId && !request.email && !request.phoneNumber && !request.federatedUserId) {


Do you validate the federatedUserId content?

Only that it's a non-empty string. (I'm not aware of a way to do any better...)

bojeil-google · 2020-01-10T08:13:58Z

src/auth/auth-api-request.ts

+ * Instantiates the getAccountInfo endpoint settings for use when fetching info
+ * for multiple accounts.
+ */
+export const FIREBASE_AUTH_GET_ACCOUNTS_INFO = new ApiSettings('/accounts:lookup', 'POST')


Why do you need to duplicate FIREBASE_AUTH_GET_ACCOUNT_INFO above? It is the same underlying API with similar request /response format.

The response validator differs. For singular lookups, a usernotfound error is raised if the user cannot be found. For bulk lookups, the return value is handled differently, so we don't want to throw this error.

An alternative would be to raise 'usernotfound' in all the calling locations rather than the response validator. This would make the FIREBASE_AUTH_GET_ACCOUNT[S]_INFO consistent (and match better with the actual backend api) but would involve duplicating the error handling code to a number of locations. (Which I don't object to. This file feels like the backend api, so removing things from it that adapt it to our client facing api seems to be a reasonable thing to do. The duplication could be minimized by using a helper.)

Thoughts?

Ok, I think this is fine.

bojeil-google · 2020-01-10T08:16:38Z

src/auth/auth-api-request.ts

+
+export const FIREBASE_AUTH_BATCH_DELETE_ACCOUNTS = new ApiSettings('/accounts:batchDelete', 'POST')
+  .setRequestValidator((request: BatchDeleteAccountsRequest) => {
+    if (!request.localIds) {


shouldn't you also validate the force field?

Shield generators back online.

bojeil-google · 2020-01-10T08:22:13Z

src/auth/auth-api-request.ts

+  .setResponseValidator((response: BatchDeleteAccountsResponse) => {
+    const errors = response.errors || [];
+    errors.forEach((batchDeleteErrorInfo) => {
+      if (batchDeleteErrorInfo.index === undefined) {


nit: typeof batchDeleteErrorInfo.index === 'undefined'

bojeil-google · 2020-01-10T08:26:23Z

src/index.d.ts

+     * @param uids The `uids` corresponding to the users to delete.
+     *
+     * @return A Promise that resolves to the total number of successful/failed
+     *     deletions, as well as the array of errors that correspond to the


corresponds

src/utils/error.ts

bojeil-google · 2020-01-10T08:35:02Z

src/auth/auth-api-request.ts

+
+  private static addProviderToRequest(id: ProviderIdentifier, request: GetAccountInfoRequest): GetAccountInfoRequest {
+    if (!validator.isNonEmptyString(id.providerUid) || !validator.isNonEmptyString(id.providerId)) {
+      throw new FirebaseAuthError(AuthClientErrorCode.INVALID_PROVIDER_ID);


I think invalid provider ID is different that invalid provider uid. The error is confusing here.

bojeil-google · 2020-01-10T08:36:03Z

src/auth/auth-api-request.ts

+  public getAccountInfoByIdentifiers(identifiers: UserIdentifier[]): Promise<object> {
+    if (identifiers.length === 0) {
+      return Promise.resolve({users: []});
+    } else if (identifiers.length > 100) {


Maybe define 100 as a constant like we do for other similar max fields?

Done. Also done for bulk delete (which was mistakenly set to 100 instead of 1000!)

bojeil-google · 2020-01-10T08:37:30Z

src/auth/auth-api-request.ts

+  public deleteAccounts(uids: string[], force: boolean): Promise<BatchDeleteAccountsResponse> {
+    if (uids.length === 0) {
+      return Promise.resolve({});
+    } else if (uids.length > 100) {


Same here regarding defining a const for this upper limit.

bojeil-google · 2020-01-10T08:40:10Z

src/auth/auth.ts

+         * Checks if the specified identifier is within the list of
+         * UserRecords.
+         */
+        const isUserFound = ((id: UserIdentifier, urs: UserRecord[]): boolean => {


what does urs stand for? Maybe pick a better name?

Done.
(urs => userRecords, ur => userRecord)

hiranya911

Looks pretty good. Just a couple of nits on documentation.

src/auth/auth.ts

src/index.d.ts

hiranya911

LGTM with a nit 👍

test/unit/auth/auth.spec.ts

cbonnie

Left small suggestion in addition to egilmore@'s comments. Otherwise LGTM.
(Covering on behalf of egilmore@)

cbonnie · 2020-03-02T23:38:30Z

test/integration/auth.spec.ts

+        // New users should not have a lastRefreshTime set.
+        expect(newUserRecord.metadata.lastRefreshTime).to.be.null;
+
+        // Login to cause the lastRefreshTime to be set.


Suggest:
"// Log in to set the lastRefreshTime."

cbonnie

Additional minor edits

cbonnie · 2020-03-03T01:22:20Z

src/index.d.ts

     */
    creationTime: string;

+    /**
+     * The time at which the user was last active (ID token refreshed), or null


"The time at which the user was last active (ID token refreshed), formatted as a UTC Date string (e.g., 'Sat, 03 Feb 2001 04:05:06 GMT'). Returns null if the user was never active."

cbonnie · 2020-03-03T01:24:18Z

src/index.d.ts

+    /**
+     * Deletes the users specified by the given uids.
+     *
+     * Deleting a non-existing user won't generate an error. (i.e. this method


Deleting a non-existing user won't generate an error (i.e. this method is idempotent).

…elete

hiranya911

LGTM

bojeil-google

Overall looks good. Only thing is that I don't see any tests for auth-api-request.ts

bojeil-google · 2020-05-04T17:41:43Z

src/auth/auth-api-request.ts

+/** Maximum allowed number of users to batch get at one time. */
+const MAX_GET_ACCOUNTS_BATCH_SIZE = 100;
+
+/** Maximum allowed numberof users to batch delete at one time. */


bojeil-google · 2020-05-04T17:44:23Z

src/auth/auth-api-request.ts

+
+export const FIREBASE_AUTH_BATCH_DELETE_ACCOUNTS = new ApiSettings('/accounts:batchDelete', 'POST')
+  .setRequestValidator((request: BatchDeleteAccountsRequest) => {
+    if (!request.localIds) {


bojeil-google · 2020-05-04T17:44:45Z

src/auth/auth-api-request.ts

+ * Instantiates the getAccountInfo endpoint settings for use when fetching info
+ * for multiple accounts.
+ */
+export const FIREBASE_AUTH_GET_ACCOUNTS_INFO = new ApiSettings('/accounts:lookup', 'POST')


Ok, I think this is fine.

bojeil-google · 2020-05-04T17:59:16Z

src/auth/identifier.ts

@@ -0,0 +1,78 @@
+/*!
+ * Copyright 2019 Google Inc.


bojeil-google · 2020-05-04T18:36:54Z

src/index.d.ts

@@ -604,7 +604,7 @@ declare namespace admin.credential {
   */
  interface Credential {

-    /**
+   /**


Comment alignment seem to be off here.

Done. (merge conflict artifact)

rsgowman

Done (incl auth-api-request.ts tests)

rsgowman · 2020-05-04T19:35:46Z

src/auth/auth-api-request.ts

+/** Maximum allowed number of users to batch get at one time. */
+const MAX_GET_ACCOUNTS_BATCH_SIZE = 100;
+
+/** Maximum allowed numberof users to batch delete at one time. */


rsgowman · 2020-05-04T19:37:42Z

src/auth/identifier.ts

@@ -0,0 +1,78 @@
+/*!
+ * Copyright 2019 Google Inc.


rsgowman · 2020-05-04T19:44:38Z

src/index.d.ts

@@ -604,7 +604,7 @@ declare namespace admin.credential {
   */
  interface Credential {

-    /**
+   /**


Done. (merge conflict artifact)

bojeil-google · 2020-05-04T20:49:05Z

Granted coverage is still adequate but the testing format does not align with the existing pattern used here. Personally I prefer sticking with the current pattern.

The pattern has been to test the RPC logic in auth-api-request.spec.ts
Then test the adequate auth-api-request calls are made in auth.spec.ts.

rsgowman · 2020-05-04T23:18:50Z

Granted coverage is still adequate but the testing format does not align with the existing pattern used here. Personally I prefer sticking with the current pattern.

The pattern has been to test the RPC logic in auth-api-request.spec.ts
Then test the adequate auth-api-request calls are made in auth.spec.ts.

Mostly done. I've moved some tests over entirely (where possible), kept some tests in auth.spec.ts (when those tests test behaviour entirely confined to auth.ts) and split some between the two (as auth.ts modifies the results from auth-api-request.ts, most notably adding a notFound parameter to the results as well as parsing everything in to a UserRecord.)

egilmorez

LG, thanks!

There's an instance of "deleteUsers()" in line is 680 of auth.d.ts that should be backticked, but we can get that later if it's a PITA.

rsgowman · 2020-05-05T20:56:37Z

LG, thanks!

There's an instance of "deleteUsers()" in line is 680 of auth.d.ts that should be backticked, but we can get that later if it's a PITA.

Not a problem; fixed.

bojeil-google · 2020-05-05T23:15:57Z

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

@@ -1727,6 +1920,35 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
      });
    });

+    describe('deleteAccounts', () => {


Can you add tests for rpc logic for this API?

hiranya911

Also list new types in docgen/content-sources/node/toc.yaml.

Sorry for not catching this earlier.

bojeil-google · 2020-05-06T17:30:28Z

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

@@ -1947,6 +1950,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
          .to.throw(FirebaseAuthError)
          .with.property('code', 'auth/invalid-uid');
      });
+
+      it('should be fulfilled given a valids uids', async () => {


given valid

hiranya911

LGTM 👍

rsgowman added 5 commits December 12, 2019 16:40

Add getUsers() bulk lookup method

16dd608

Add lastRefreshTime field to users returned via GetAccountInfo rpc

47f86ea

Add deleteUsers() bulk deletion method

75e0dcf

Add lookup by federated provider

7b214a8

Delete users before creating them.

b3efbf9

Avoids 'already-exists' errors. Note that these errors would occur not just for already existing uids, but also for email and phoneNumber fields.

rsgowman requested a review from hiranya911 December 13, 2019 14:55

rsgowman assigned hiranya911 Dec 13, 2019

hiranya911 suggested changes Dec 17, 2019

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Dec 17, 2019

hiranya911 requested review from bojeil-google and egilmorez December 17, 2019 19:57

hiranya911 assigned bojeil-google Dec 17, 2019

rsgowman added 2 commits December 18, 2019 14:57

review feedback

d65921f

oops

51a880c

rsgowman assigned hiranya911 and unassigned rsgowman Dec 18, 2019

Merge branch 'master' into rsgowman/bulk_get_delete

cb5086c

hiranya911 approved these changes Jan 6, 2020

View reviewed changes

src/auth/auth-api-request.ts Show resolved Hide resolved

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

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

bojeil-google suggested changes Jan 10, 2020

View reviewed changes

rsgowman added 2 commits January 10, 2020 11:59

review feedback 2

85bc39e

Merge branch 'master' into rsgowman/bulk_get_delete

8222139

hiranya911 changed the title ~~Add bulk get/delete methods~~ feat(auth): Add bulk get/delete methods Jan 18, 2020

hiranya911 added the release-note label Jan 18, 2020

hiranya911 suggested changes Jan 18, 2020

View reviewed changes

src/auth/auth.ts Outdated Show resolved Hide resolved

src/index.d.ts Outdated Show resolved Hide resolved

src/index.d.ts Outdated Show resolved Hide resolved

hiranya911 assigned egilmorez and unassigned hiranya911 Jan 18, 2020

review feedback 3

65451d0

rsgowman assigned hiranya911 Jan 20, 2020

hiranya911 approved these changes Jan 21, 2020

View reviewed changes

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

cbonnie approved these changes Mar 2, 2020

View reviewed changes

cbonnie approved these changes Mar 3, 2020

View reviewed changes

rsgowman added 3 commits May 1, 2020 17:16

comment cleanup

51ab780

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

fc493d6

…elete

lint

c585383

hiranya911 added the release:stage Stage a release candidate label May 4, 2020

trigger CI

ce69cf2

hiranya911 approved these changes May 4, 2020

View reviewed changes

bojeil-google suggested changes May 4, 2020

View reviewed changes

review feedback + auth-api-request tests

912c7e7

rsgowman commented May 4, 2020

View reviewed changes

hiranya911 removed the release:stage Stage a release candidate label May 4, 2020

move some tests

156e388

egilmorez approved these changes May 5, 2020

View reviewed changes

move more tests

5d4488b

bojeil-google suggested changes May 5, 2020

View reviewed changes

hiranya911 suggested changes May 5, 2020

View reviewed changes

Missing test + api doc toc

1f42a8a

bojeil-google approved these changes May 6, 2020

View reviewed changes

hiranya911 approved these changes May 6, 2020

View reviewed changes

hiranya911 assigned rsgowman May 6, 2020

rsgowman added 2 commits May 6, 2020 13:56

typo

10f95c4

remove unneeded toc entry

e8b8e65

rsgowman merged commit b590990 into master May 6, 2020

rsgowman deleted the rsgowman/bulk_get_delete branch May 6, 2020 21:43

samtstern mentioned this pull request May 7, 2020

Type issue in 8.12 conflicts with firebase-functions #880

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

feat(auth): Add bulk get/delete methods #726

feat(auth): Add bulk get/delete methods #726

Uh oh!

Conversation

rsgowman commented Dec 12, 2019 • edited by hiranya911 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!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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!

rsgowman commented Dec 12, 2019 •

edited by hiranya911

Loading

cbonnie left a comment •

edited

Loading