Custom Claims and List Users Support #92

hiranya911 · 2017-10-31T17:36:11Z

Adding two new operations to the FirebaseAuth API:

listUsersAsync(String pageToken, int maxResults): Facilitates paging through all the user accounts of a Firebase project.
setCustomClaimsAsync(String uid, Map<String, Object> claims): Sets custom attributes on a user account.

go/firebase-admin-custom-claims

…read factories used by the SDK

… in default thread managers to ensure clean JVM exit

…s for clarity

…base.

…to documentation and tests.

…-java into hkj-exec-cleanup

… minor changes

…e check until we have to serialize the update parameters

hiranya911 · 2017-10-31T17:40:54Z

@mikelehen This is a recently approved API (already released in Node.js). Please review when you get a chance. Not urgent as of now.

schmidt-sebastian · 2017-11-08T22:35:33Z

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

+
+/**
+ * Contains metadata associated with a Firebase user account, along with password hash and salt.
+ * Instances of this class are immutable and thread safe.


s/thread safe/thread-safe.

schmidt-sebastian · 2017-11-08T22:37:24Z

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

+    this.passwordSalt = response.getPasswordSalt();
+  }
+
+  @Nullable


Please add documentation here and below. Also explain when this might be null.

schmidt-sebastian · 2017-11-08T22:43:35Z

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

+      throw new FirebaseAuthException(LIST_USERS_ERROR,
+          "IO error while downloading user accounts.", e);
+    }
+    if (response == null) {


Nit: this might be more readable if you move this into the try block.

schmidt-sebastian · 2017-11-08T22:46:01Z

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

+  private ListUsersPage(@NonNull UserSource source, int maxResults, @Nullable String pageToken) {
+    this.source = source;
+    this.maxResults = maxResults;
+    this.currentBatch = source.fetch(maxResults, pageToken);


This is performing a synchronous network operation inside the constructor. Can you move this code to the accessors?

This class is much easier to reason about when it's immutable. Therefore I'd rather fetch the data upon construction, and assign to a final attribute. Also lazy loading is not a requirement for this class. FirebaseAuth.listUsers() and Page.getNextPage() are expected to make RPC calls, and return fully populated pages as the result.

We can move the RPC call out of the constructor, but since we create pages in 2 places the constructor seems like a natural place to place that bit of code. Alternatively we can try a private ensureCurrentBatch() method to load the data on demand, but we are not really saving anything since all operations on Page will have to call it.

This is generally frowned upon as it makes testing hard (See http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/).

One easy way to mitigate would be to create a factory method and pass in currentBatch.

schmidt-sebastian · 2017-11-08T22:49:53Z

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

+      try {
+        DownloadAccountResponse response = userManager.listUsers(maxResults, pageToken);
+        ImmutableList.Builder<ExportedUserRecord> builder = ImmutableList.builder();
+        if (response.getUsers() != null) {


s/getUsers/hasUsers()/?

schmidt-sebastian · 2017-11-08T22:51:07Z

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

+          }
+        }
+        String nextPageToken = response.getPageToken() != null
+            ? response.getPageToken() : END_OF_LIST;


Have you though about using "null" to mark the final page token and just validating that the end user never passes it in?

Users should be able to pass null here, which stands for the first page.

schmidt-sebastian · 2017-11-08T22:54:11Z

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

+      if (parsed.isEmpty()) {
+        this.customClaims = null;
+      } else {
+        this.customClaims = ImmutableMap.copyOf(parsed);


Is it possible to delete a single custom claim?

Something like:

Map<String, Object> copy = new HashMap(userRecord.getCustomClaims()); copy.remove("someClaim"); updateRequest.setCustomClaims(copy);

schmidt-sebastian · 2017-11-08T22:55:01Z

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

@@ -177,6 +194,16 @@ public UserMetadata getUserMetadata() {
  }

  /**
+   * Returns additional custom claims set on this user.


I don't understand what additional means in this context. I suppose it returns all the custom claims?

Removed additional

schmidt-sebastian · 2017-11-08T22:56:17Z

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

+   */
+  @Nullable
+  public Map<String,Object> getCustomClaims() {
+    return customClaims;


I think we should return an immutable copy here.

Also - can we return an empty map if there are no custom claims?

Setting to empty map when there are no claims. This always get set to ImmutableMap in the constructor.

schmidt-sebastian · 2017-11-08T22:56:48Z

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

+
+  private static String serializeCustomClaims(Map customClaims, JsonFactory jsonFactory) {
+    checkNotNull(jsonFactory, "JsonFactory must not be null");
+    if (customClaims == null || customClaims.isEmpty()) {


If you initialize customClaims to an empty map, this would go away as well.

This can be a value directly passed in by the user via UpdateRequest.setCustomClaims(). Therefore null values are allowed here.

…umentation and tests

hiranya911 · 2017-11-09T00:12:07Z

@schmidt-sebastian Made most of the suggested changes. Provided explanations on the rest. On to the next round.

schmidt-sebastian · 2017-11-09T19:21:26Z

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

+    ImmutableMap.Builder<String, Object> builder = ImmutableMap.<String, Object>builder()
+        .put("maxResults", maxResults);
+    if (pageToken != null) {
+      checkArgument(!pageToken.isEmpty(), "invalid end of list page token");


How about pageToken.equals(ListUsersPage.END_OF_LIST)? That way, that implementation detail only exists in one place.

Good call. Done.

schmidt-sebastian · 2017-11-09T19:23:03Z

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

+  private ListUsersPage(@NonNull UserSource source, int maxResults, @Nullable String pageToken) {
+    this.source = source;
+    this.maxResults = maxResults;
+    this.currentBatch = source.fetch(maxResults, pageToken);


This is generally frowned upon as it makes testing hard (See http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/).

One easy way to mitigate would be to create a factory method and pass in currentBatch.

schmidt-sebastian · 2017-11-09T19:27:27Z

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

+      update.getProperties(null);
+      fail("No error thrown for null JsonFactory");
+    } catch (Exception ignore) {
+      // expected


Are there any specifics you can verify here?

Done. Checking for NPE now.

hiranya911 · 2017-11-09T21:05:29Z

Made the suggested changes. Moved the RPC calls in ListUsersPage constructor to the PageFactory helper class,

schmidt-sebastian · 2017-11-09T23:55:15Z

Thanks so much! I am a big fan.

hiranya911 added 30 commits September 16, 2017 13:57

Implemented ThreadManager API for configuring the thread pools and th…

a2ee38c

…read factories used by the SDK

Giving all threads unique names; Updated documentation; Using daemons…

1cf4c3a

… in default thread managers to ensure clean JVM exit

Updated comments and documentation

8234ca1

Adding tests for options

a8fb84e

Test cases for basic ThreadManager API

913fda5

More test cases

c218f82

Made the executor service private in FirebaseApp; Refactored the test…

a803a4e

…s for clarity

Clean separation of long-lived and short-lived tasks of the SDK

458d0c2

Updated documentation; More tests; Starting token refresher from data…

b63b17a

…base.

Updated documentation and log statements

050bfa4

Removing test file

f06b9b3

Initializing executor in FirebaseApp constructor. Minor improvements …

846c93c

…to documentation and tests.

Merged with the latest ThreadManager impl

d3ea8e9

Merge branch 'hkj-exec-cleanup' of github.com:firebase/firebase-admin…

e06ae1d

…-java into hkj-exec-cleanup

Fixed token refresher stop() logic

dfca1e7

Updated documentation; Renamed submit() to submitCallable() and other…

b310b0a

… minor changes

Merging with latest base

ae32b93

Implemented custom claims support

6c6d44b

More test cases

6ecc866

Initial list users implementation

de99eae

Changed the setCustomClaims() API on UpdateRequest; Deferring the siz…

f58d41f

…e check until we have to serialize the update parameters

Cleaned up the listUser() impl

a5977be

Further developed the listUsers() API

ddd68d0

Updated tests and documentation

3a2478b

Updated documentation

770e63b

Merged with master

7df8719

Implementing batch get operation for users

4907345

Further cleaned up the iterator code

a841dc1

Further cleaned up the iterator code

5f2c906

Updated list user API

7a0ca59

hiranya911 requested a review from avishalom October 31, 2017 17:40

hiranya911 assigned mikelehen Oct 31, 2017

hiranya911 mentioned this pull request Nov 2, 2017

FR: Custom claims support #80

Closed

hiranya911 assigned schmidt-sebastian and unassigned mikelehen Nov 7, 2017

hiranya911 requested a review from schmidt-sebastian November 7, 2017 18:09

schmidt-sebastian reviewed Nov 8, 2017

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Nov 8, 2017

hiranya911 added 2 commits November 8, 2017 15:12

Merge branch 'master' into hkj-custom-claims

6d4af29

Using empty map in UserRecord when no claims are present; Updated doc…

95ca61e

…umentation and tests

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 Nov 9, 2017

schmidt-sebastian reviewed Nov 9, 2017

View reviewed changes

minor improvements

01460e0

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Nov 9, 2017

Removing RPC call from constructor

db05642

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 Nov 9, 2017

schmidt-sebastian approved these changes Nov 9, 2017

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Nov 10, 2017

hiranya911 mentioned this pull request Nov 28, 2017

Adding Java samples for listUsersAsync() and setCustomUserClaimsAsync() firebase/quickstart-java#15

Merged

hiranya911 merged commit acec5fe into master Dec 5, 2017

hiranya911 deleted the hkj-custom-claims branch December 5, 2017 18:38

Custom Claims and List Users Support #92

Custom Claims and List Users Support #92

Uh oh!

Conversation

hiranya911 commented Oct 31, 2017

Uh oh!

hiranya911 commented Oct 31, 2017

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!

schmidt-sebastian Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hiranya911 commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 commented Nov 9, 2017

Uh oh!

schmidt-sebastian commented Nov 9, 2017

Uh oh!

Uh oh!

schmidt-sebastian Nov 9, 2017 •

edited

Loading

schmidt-sebastian Nov 9, 2017 •

edited

Loading