feat(auth): Adds support for importUsers API. #220

bojeil-google · 2018-03-01T04:09:03Z

This API can be used for bulk account import to Firebase Auth user database from external Auth systems using different hashing algorithms as well as other Firebase projects.
Unit tests and integration tests also added for this new API.

fix(auth): Fixes quota exceeded errors thrown in auth integration tests due to backend throttling.

This API can be used for bulk account import to Firebase Auth user database from external Auth systems using different hashing algorithms as well as other Firebase projects. fix(auth): Fixes quota exceeded errors thrown in auth integration tests due to backend throttling.

hiranya911

Code looks correct. Most of my comments are about style and readability.

hiranya911 · 2018-03-01T22:54:22Z

src/auth/auth.ts

  }
+
+  /**
+   * Imports the list of users provided to Firebase Auth users database. This is useful when


to Firebase Auth. sounds good enough I think. The word database in there might confuse somebody.

hiranya911 · 2018-03-01T22:54:41Z

src/auth/auth.ts

+   * When importing a list of password users, UserImportOptions are required to be specified.
+   *
+   * @param {UserImportRecord[]} users The list of user records to import to Firebase Auth
+   *     database.


Remove database from here too?

hiranya911 · 2018-03-01T23:00:19Z

src/auth/user-import-builder.ts

+    this.indexMap = {};
+
+    this.validateAndPopulateUsers();
+    this.validateAndPopulateOptions();


Can we refactor the code so the above two lines end up looking like:

this.validatedUsers = this.validateUsers(users); this.validatedOptions = this.validateOptions(options);

This is not as simple as you think, there are other private variables computed in the process of these. I prefer not to refactor this.

Ok, i made that change but it contains private variables within.

hiranya911 · 2018-03-01T23:03:22Z

src/auth/user-import-builder.ts

+          passwordHash: undefined,
+          salt: undefined,
+          lastLoginAt: undefined,
+          createdAt: undefined,


Can we not list these undefined fields here? Then you don't have to remove them later in the code.

I add them so i don't have to define this interface. anyway, i can remove them but will need to define this interface. In addition, other fields could be undefined as passed from developer and still need to be removed. Anyway, i will remove and define the interface even though there is little gained from this.

hiranya911 · 2018-03-01T23:06:24Z

src/auth/user-import-builder.ts

+}
+
+
+/** Callback function to validate an object. */


Document the contract of this function in more details. What should it do when an invalid user is encountered?

hiranya911 · 2018-03-02T00:13:04Z

src/auth/auth-api-request.ts

  }

+  /**
+   * Imports the list of users provided to Firebase Auth users database. This is useful when


Lets drop database from the text. Here and elsewhere.

hiranya911 · 2018-03-02T00:16:43Z

src/auth/user-import-builder.ts

+   * Returns the corresponding constructed uploadAccount request.
+   * @return {UploadAccountRequest} The constructed uploadAccount request.
+   */
+  public generateRequest(): UploadAccountRequest {


I think buildRequest is a better name for this.

hiranya911 · 2018-03-02T00:23:48Z

test/integration/auth.spec.ts

+      return safeDelete(randomUid);
+    });
+
+    it('successfully imports users with HMAC_SHA256 hashed passwords', () => {


Is it possible to organize these tests by creating an array of test fixtures? For example:

interface UserImportTest { name string importOptions UserImportOptions passwordHash string } const fixtures: Array<UserImportTest> = [ { name: 'hmac', importOptions: {...}, passwordHash: crypto.createHmac(...), }, ... ]; fixtures.forEach((fixture) => { it(...) });

Ok, changing it as described. The passwordHash will be a function so i don't need to hardcode the parameters.

hiranya911 · 2018-03-02T00:25:42Z

test/integration/auth.spec.ts

  photoURL: 'http://www.example.com/' + newUserUid + '/photo.png',
  disabled: false,
 };
+let deleteQueue = Promise.resolve();


I overwrite this at the bottom every time I queue a single delete:

// Suppress errors in delete queue to not spill over to next item in queue. deleteQueue = deletePromise.catch((error) => { // Do nothing. }); return deletePromise;

hiranya911 · 2018-03-02T00:31:54Z

src/auth/user-import-builder.ts

+   * @return {UserImportResult} The user import result based on the returned failed
+   *     uploadAccount response.
+   */
+  public generateResponse(


buildResponse

googlebot · 2018-03-08T04:32:52Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

hiranya911

Looks pretty good. Just a few nits to address, and then I can LGTM.

hiranya911 · 2018-03-08T18:29:27Z

src/auth/auth.ts

+  /**
+   * Imports the list of users provided to Firebase Auth. This is useful when
+   * migrating from an external authentication system without having to use the Firebase CLI SDK.
+   * A maximum list of 1000 users are allowed to be imported one at a time.


Nit: At most 1000 users are....

hiranya911 · 2018-03-08T18:32:34Z

src/utils/error.ts

  };
+  public static INVALID_HASH_ALGORITHM = {
+    code: 'invalid-hash-algorithm',
+    message: 'The hash algorithm must be a string matching the list of supported algorithms.',


Nit: The hash algorithm must match one of the strings in the list of...

hiranya911 · 2018-03-08T18:37:51Z

test/integration/auth.spec.ts

+        computePasswordHash: (userImportTest: UserImportTest): Buffer => {
+          return Buffer.from(scryptPasswordHash, 'base64');
+        },
+        rawPassword: scryptRawPassword,


Can these two also be password and NaCl like the other cases? In that case you can extract these fields out of the fixture and just make them shared constants.

I modified the tests to all use the same rawPassword and rawSalt constants. However, it is not easy to generate the scrypt (internal version) password hash on the fly. The only way to do it is via: https://github.com/firebase/scrypt

hiranya911 · 2018-03-08T18:41:08Z

test/integration/auth.spec.ts

 }
+
+/**
+ * Safe deletes a specificed user identified by uid. This API chains all delete


Nit: Safely...

hiranya911 · 2018-03-08T18:53:42Z

src/auth/user-import-builder.ts

  cpuMemCost?: number;
  parallelization?: number;
  blockSize?: number;
  dkLen?: number;


I missed this in the previous review. Shall we spell this out like all the others?

This is the backend name. We call this derivedKeyLength (the developer facing one). Check UserImportOptions inteface.

hiranya911 · 2018-03-08T19:00:27Z

src/auth/user-import-builder.ts

-      return;
+  private populateOptions(
+      options: UserImportOptions, requiresHashOptions: boolean): UploadAccountOptions {
+    let populatedOptions: UploadAccountOptions = {};


Can this be just let populatedOptions: UploadAccountOptions;?

I actually return this when hash options are required but that is fine, i will return {} directly.

hiranya911 · 2018-03-08T19:12:14Z

src/auth/user-import-builder.ts

      }
      index++;
    });
+    return populatedUsers;


If you wanted, you can just return a single array from this method containing both users and errors:

const populatedUsers = []; users.forEach((user, index) => { try { const result = populateUploadAccountUser(user, userValidator); populatedUsers.push(result); } catch (error) { populatedUsers.push({index, error}); } }) return populatedUsers;

Then in the caller (constructor) you can make all the state changes:

const results = populateUsers(...); this.validatedUsers = // filter out the users from results this.indexMap = // construct the index map this.requireHashOptions = // check if at least one user requires hash

I don't feel strongly about this, so I'll leave it to you.

It is not easy constructing the indexMap after this completes. I have to remember the map original locations of the users to their new locations in the request sent to backend.

hiranya911 · 2018-03-08T19:13:08Z

src/auth/user-import-builder.ts

    let index = 0;
-    this.users.forEach((user) => {
+    const populatedUsers: UploadAccountUser[] = [];
+    users.forEach((user) => {


You could do users.forEach((user, index) => { here and avoid having to maintain you own index variable.

hiranya911 · 2018-03-08T19:17:29Z

If you wish to amend the author of old commits, here's a useful guide: https://www.git-tower.com/learn/git/faq/change-author-name-email

You basically has to perform an interactive rebase, and amend each commit with the incorrect email.

hiranya911

LGTM

OAuth providers. This helps test provider emails are correctly set.

hiranya911 · 2018-04-06T18:41:29Z

Related to #158

bojeil-google added 3 commits February 28, 2018 20:04

Merge branch 'master' into multiple-import

f0c9668

Fixed linter errors.

f4794ac

hiranya911 suggested changes Mar 2, 2018

View reviewed changes

Addresses review comments.

f94af91

Bassam Ojeil and others added 6 commits March 7, 2018 20:48

Additional fixes added.

95fd564

Improved comment.

9fd5f9c

More comment tweaks.

2a17380

Addressed more comments.

c1b3e4e

More fixes.

ea3418e

More fixes.

19a0ceb

bojeil-google assigned hiranya911 Mar 8, 2018

hiranya911 reviewed Mar 8, 2018

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 Mar 8, 2018

bojeil-google added 2 commits March 8, 2018 14:09

Addressed review comments.

cf8011a

More review comments addressed.

2ab33f1

bojeil-google assigned hiranya911 and unassigned bojeil-google Mar 8, 2018

hiranya911 approved these changes Mar 8, 2018

View reviewed changes

bojeil-google added 4 commits March 8, 2018 17:14

Adds email to provider in Auth integration importUsers test for multiple

616d3f4

OAuth providers. This helps test provider emails are correctly set.

Adds customClaims integration test to importUsers test.

9fa2ac1

Merge branch 'master' into multiple-import

57c76f9

Updates package-lock.json.

a43371f

hiranya911 merged commit 83309bb into master Mar 14, 2018

hiranya911 deleted the multiple-import branch March 14, 2018 21:04

This was referenced Mar 19, 2018

Bump firebase-admin from 5.8.1 to 5.11.0 raqbit/Raqbot#42

Closed

Bump firebase-admin from 5.5.1 to 5.11.0 dyaa/loopback-connector-firestore#47

Closed

takeratta mentioned this pull request Nov 29, 2023

[Snyk] Fix for 19 vulnerabilities takeratta/firebase-admin-node#30

Open

Uh oh!

feat(auth): Adds support for importUsers API. #220

feat(auth): Adds support for importUsers API. #220

Uh oh!

Conversation

bojeil-google commented Mar 1, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Mar 8, 2018

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!

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