Bulk User Import API #174

hiranya911 · 2018-05-08T20:26:21Z

Introduces the following new APIs for importing users to Firebase Auth in bulk (1000 users per API call):

FirebaseAuth.importUsers(List<UserImportRecord> users) FirebaseAuth.importUsers(List<UserImportRecord> users, UserImportOptions options) FirebaseAuth.importUsersAsync(List<UserImportRecord> users) FirebaseAuth.importUsersAsync(List<UserImportRecord> users, UserImportOptions options)

Also add the UserImportHash and UserImportResult APIs.

go/firebase-admin-user-import

…onsistency

* Removed the deprecated FirebaseCredential API * Removing the deprecated Task API

* Dropping support for GAE 7 * Removed GaeThreadFactory, GaeExecutorService and RevivingScheduledExecutor * Removed the deprecated FirebaseCredential API * Removing GAE java7 related APIs (GaeThreadFactory, RevivingScheduledExecutor) * Removed GaePlatform implementation * Added FirebaseScheduledExecutor * Updated documentation * Some minor nits from code reviews * Calling super method in DefaultRunLoop executor

* Dropping support for GAE 7 * Removed GaeThreadFactory, GaeExecutorService and RevivingScheduledExecutor * Removed the deprecated FirebaseCredential API * Removing GAE java7 related APIs (GaeThreadFactory, RevivingScheduledExecutor) * Removed GaePlatform implementation * Added FirebaseScheduledExecutor * Updated documentation * Removing LogWrapper API * Removing PrefixedLogger * Removing test config file * Updated CHANGELOG * Minor clean ups pointed out in the code review

schmidt-sebastian

This is pretty much LGTM. I mostly compared it to the API approval doc and it looks like 2600 lines of sound code.

Thanks for adding these exhaustive tests!

schmidt-sebastian · 2018-05-16T19:16:46Z

src/main/java/com/google/firebase/auth/FirebaseUserManager.java

+
+ if (hasPassword) {
+ checkArgument(options != null && options.getHash() != null,
+ "UserImportHash option is required when at least one user has a password");


I would suggest that you explain how the UserImportHash can be set here:

"UserImportHash option is required when at least one user has a password. Provide a UserImportHash via UserImportOptions.withHash()."

schmidt-sebastian · 2018-05-16T19:20:45Z

src/main/java/com/google/firebase/auth/ImportUserRecord.java

+ * @return This builder.
+ */
+ public Builder addUserProvider(@NonNull UserProvider provider) {
+ this.userProviders.add(provider);


Can you explain to me (and maybe everyone in the comment) what the intended use for this is? How does it interact with the other fields in this builder, especially the overlapping ones?

schmidt-sebastian · 2018-05-16T19:25:26Z

src/main/java/com/google/firebase/auth/UserImportOptions.java

+ * @return A non-null {@link UserImportOptions}.
+ */
+ public UserImportOptions build() {
+ return new UserImportOptions(this);


Add verification here that the hash is not null.

schmidt-sebastian · 2018-05-16T19:25:53Z

src/main/java/com/google/firebase/auth/UserImportOptions.java

+
+ Map<String, Object> getProperties() {
+ ImmutableMap.Builder<String, Object> properties = ImmutableMap.builder();
+ if (hash != null) {


You can mark hash @nonnull and remove this check.

schmidt-sebastian · 2018-05-16T19:28:07Z

src/main/java/com/google/firebase/auth/UserImportResult.java

+ /**
+ * Returns the number of users that were imported successfully.
+ *
+ * @return an integer (possibly zero).


Here an below: You should update this to describe that the number represents. It's already clear that you are returning an integer.

schmidt-sebastian · 2018-05-16T19:37:11Z

src/main/java/com/google/firebase/auth/hash/BasicHash.java

+ BasicHash(String name, Builder builder) {
+ super(name);
+ checkArgument(builder.rounds >= 0 && builder.rounds <= 120000,
+ "A non-empty key is required for HMAC algorithms");


This error message doesn't match what it is verifying.

schmidt-sebastian · 2018-05-16T20:30:02Z

src/main/java/com/google/firebase/auth/hash/Scrypt.java

+ * Firebase Auth. See {@link StandardScrypt} for the standard Scrypt algorithm. Can be used as an
+ * instance of {@link com.google.firebase.auth.UserImportHash} when importing users.
+ */
+public final class Scrypt extends UserImportHash {


Can we make this inherit BasicHash and re-use setRounds()? On that note, should BasicHash be renamed to better reflect what it does?

It seems there are 5 types of hash algorithms that we need to support:

HMAC hashes

MD5, SHA, PBKDF etc

Scrypt

StandardScrypt

Bcrypt

I refer to the 2nd category as basic hashes for the lack of a better name. I think it makes sense to not have Scrypt extend from BasicHash (the allowed integer ranges are different). But I'm certainly open to calling BasicHash something else. Any suggestions?

What do you think of making BasicHash a "RepeatableHash" interface? You will have to verify the range in each implementing class, but it's a nicely named interface and would convey some property about the hash functions that adhere to it.

Thanks for the suggestion. I renamed BasicHash to RepeatableHash, and passed range interval as constructor parameters. This enables us to reuse it in Scrypt as well.

schmidt-sebastian · 2018-05-16T20:31:48Z

src/main/java/com/google/firebase/auth/internal/UploadAccountResponse.java

+import com.google.api.client.util.Key;
+import java.util.List;
+
+public class UploadAccountResponse {


Please add a some JavaDoc snippet, even for internal classes.

schmidt-sebastian · 2018-05-16T20:34:14Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

+ UserRecord savedUser = auth.getUserAsync(randomId).get();
+ assertEquals(userEmail, savedUser.getEmail());
+ String idToken = signInWithPassword(userEmail, "password");
+ assertTrue(!Strings.isNullOrEmpty(idToken));


assertFalse

schmidt-sebastian · 2018-05-16T20:36:59Z

src/test/java/com/google/firebase/auth/FirebaseUserManagerTest.java

+ FirebaseAuth.getInstance().importUsersAsync(users);
+ fail("No error thrown for missing hash option");
+ } catch (IllegalArgumentException expected) {
+ // expected


Please verify the error message. That will also make it super obvious what this test is verifying.

… minor nits

hiranya911 · 2018-05-17T21:58:39Z

@schmidt-sebastian thanks for the thorough review. Addressed pretty much all the comments, and left one follow up question (about renaming BasicHash).

schmidt-sebastian · 2018-05-22T18:28:05Z

src/main/java/com/google/firebase/auth/hash/RepeatableHash.java

- checkArgument(builder.rounds >= 0 && builder.rounds <= 120000,
- "Rounds value must be between 0 and 120000 (inclusive).");
+ checkArgument(builder.rounds >= min && builder.rounds <= max,
+ "Rounds value must be between %s and %s (inclusive).", min, max);


Make this "%d".

Guava only accepts %s: https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Preconditions.html

schmidt-sebastian · 2018-05-22T18:28:35Z

src/main/java/com/google/firebase/auth/hash/RepeatableHash.java

 import com.google.common.collect.ImmutableMap;
 import com.google.firebase.auth.UserImportHash;
 import java.util.Map;



Can you add a short class comment here?

hiranya911 added 28 commits February 21, 2018 17:28

Adding the sync APIs for FirebaseAuth

fd3dd9d

Added more tests; Added sync APIs for FirebaseMessaging

ac3453b

Removing Task references from database, iid and fcm APIs

ded167e

Fixing a typo

765f209

Minor code clean up

01ce79e

Merged with master (auth unit test improvements)

3913688

Updated javadocs; Renamed internal helpers of FirebaseMessaging for c…

d5ec1c3

…onsistency

Removed the deprecated FirebaseCredential API (#149)

f8f33b4

Removing the Task API (#152)

27eddb6

* Removed the deprecated FirebaseCredential API * Removing the deprecated Task API

Merged with master

3979c02

merged with master

ef036bb

Initial code for the importUsers() API

0523bf3

Added documentation and more tests

0db88c5

Added integration tests; Scrypt hash; more documentation

fe3cc03

Added more tests

46e51ab

Merge branch 'master' into v6

5ede505

updated test

efc9016

Merged with master

dbc1492

Merged with v6

0633d37

Adding other hash types

2514703

Added the remaining hash algorithms

ebe905c

Renamed to ImportUserRecord

1182576

Updated documentation

421a86c

Merged with master

7b085df

Some minor cleanup

501faf8

Updated documentation

dccc454

hiranya911 requested a review from schmidt-sebastian May 14, 2018 20:07

hiranya911 assigned schmidt-sebastian May 14, 2018

hiranya911 requested a review from avishalom May 14, 2018 20:08

schmidt-sebastian reviewed May 16, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian May 17, 2018

Updated documentation; Made hash required in UserImportOptions; Other…

a4cf116

… minor nits

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 May 17, 2018

hiranya911 removed the request for review from avishalom May 18, 2018 17:12

Renamed BasicHash to RepeatableHash; Made rounds range test configurable

9130ab2

schmidt-sebastian approved these changes May 22, 2018

View reviewed changes

Added javadoc comment

3f30183

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian May 22, 2018

hiranya911 merged commit d6b3d49 into master May 22, 2018

hiranya911 deleted the hkj-user-import branch May 22, 2018 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Bulk User Import API #174

Bulk User Import API #174

Uh oh!

hiranya911 commented May 8, 2018

schmidt-sebastian left a comment

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 22, 2018

hiranya911 May 22, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

schmidt-sebastian May 16, 2018

hiranya911 May 17, 2018

hiranya911 commented May 17, 2018

schmidt-sebastian May 22, 2018

hiranya911 May 22, 2018

schmidt-sebastian May 22, 2018

hiranya911 May 22, 2018

Labels

2 participants

Uh oh!

Bulk User Import API #174

Bulk User Import API #174

Uh oh!

Conversation

hiranya911 commented May 8, 2018

schmidt-sebastian 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 commented May 17, 2018

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

Labels

2 participants