feat(auth): Link federatedid #345

nrsim · 2020-01-09T19:35:51Z

RELEASE NOTE: Add ability to link a federated ID with the updateUserAsync() method.

rsgowman · 2020-01-09T20:02:58Z

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

+
+    UpdateRequest deleteProvider(String providerId) {
+      checkProviderId(providerId);
+      properties.put("deleteProvider", ImmutableList.of(providerId));


This would overwrite previous values such that:

UpdateRequest ur = ...; ur.deleteProvider("google.com").deleteProvider("facebook.com")

would result in only the facebook.com provider being deleted.

Better might be: UpdateRequest setDeleteProviders(Iterable<String> providerIds). You could optionally add UpdateRequest addDeleteProvider(String) and/or UpdateRequest addDeleteProviders(Iterable<String>).

See further discussion here: http://go/java-practices/builders#special. (Second bullet point).

rsgowman · 2020-01-09T20:06:02Z

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

@@ -528,6 +532,17 @@ UpdateRequest setValidSince(long epochSeconds) {
      return this;
    }

+    UpdateRequest linkProvider(@NonNull UserProvider userProvider) {


For our builders (which UpdateRequest effectively is), use setProperty() rather than property(). (So setLinkProvider() here.) Both for consistency with the existing type, (eg setValidSince) and also for consistency with our 'best practices' guide. (http://go/java-practices/builders#api, last bullet point.) Note that this directly conflicts with EJ (as pointed out by the practices doc.)

rsgowman · 2020-01-09T20:08:02Z

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

@@ -528,6 +532,17 @@ UpdateRequest setValidSince(long epochSeconds) {
      return this;
    }

+    UpdateRequest linkProvider(@NonNull UserProvider userProvider) {


Note that this part of the api design is currently in flux, so it may change a bit yet. Sorry! :(

No action required (yet).

rsgowman · 2020-01-09T20:10:40Z

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

@@ -528,6 +532,17 @@ UpdateRequest setValidSince(long epochSeconds) {
      return this;
    }

+    UpdateRequest linkProvider(@NonNull UserProvider userProvider) {


Should be public (and probably have some javadoc).

rsgowman · 2020-01-09T20:10:54Z

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

+      return this;
+    }
+
+    UpdateRequest deleteProvider(String providerId) {


public/javadoc

rsgowman · 2020-01-09T20:17:16Z

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

+      assertEquals(1, userRecord.getProviderData().length);
+      assertTrue(userRecord.getCustomClaims().isEmpty());
+
+    } finally {


Good catch.

rsgowman

Other than a few nits, this lgtm. We'll need to wait for the api review before we can make further progress.

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

Michael-Frank · 2021-03-02T08:44:21Z

any progress on this? This feature would be really great to have.

rsgowman · 2021-03-02T16:02:14Z

any progress on this? This feature would be really great to have.

Yes: We finally got all the approvals necessary to launch this and have started to do so. The node.js port went out last month (https://firebase.google.com/support/release-notes/admin/node#version_950_-_10_february_2021) and the other ports, incl this one, should be released in the coming weeks.

google-cla · 2021-03-17T21:27:32Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

hiranya911

Looks good! Just a couple of nits.

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

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

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

hiranya911 · 2021-03-19T00:26:37Z

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

+          @SuppressWarnings("unchecked")
+          Iterable<String> deleteProviderIterable = (Iterable<String>)deleteProvider;
+
+          copy.put("deleteProvider", new ImmutableList.Builder<String>()


Should we handle the double specification case?

user.updateRequest() .setPhoneNumber(null) .setProvidersToUnlink(ImmutableList.of("phone"))

I remember handling this case is other languages.

Done. Note that this results in the IllegalArgumentException being wrapped in an ExecutionException, which is a little awkward. (Option 1)

Option 2:
We can resolve that by pulling the property calculation out of the async portion. It's a bit more disruptive, and diverges from how get/posts are handled throughout the rest of the code base. Roughly:

FirebaseUserManager.java: ... - void updateUser(UserRecord.UpdateRequest request JsonFactory jsonFactory) { + void updateUser(Map<String, Object> payload) { ...

(NB: This is not a public interface.)

Option 3:
Alternatively, we could just call request.getProperties() before the async portion and ignore the results. (i.e. call it twice.)

I've left it as option 1 for now. lmk if you'd prefer 2 or 3.

Would it make sense to enforce this constraint in the setters? We'd have to do so in both setPhoneNumber() and setProvidersToUnlink(), but I think we can reduce the duplication via some sort of a helper function. That would make argument validation consistent with the pattern currently used in this class.

Yeah, that's better; done. I've removed the logic here, i.e. if we somehow get into a bad state, we won't notice it. (But now we shouldn't get into that state.)

I didn't create a helper; there wasn't much duplication (though the exception itself is duplicated). If we set the value first and then checked, it would work out better... but that risks leaving the object in a bad state (eg if user catches and ignores the exception and then proceeds to use the object anyways.)

google-cla · 2021-03-31T19:20:17Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

hiranya911

LGTM with a suggestion on argument validation.

hiranya911 · 2021-04-01T19:48:17Z

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

+          @SuppressWarnings("unchecked")
+          Iterable<String> deleteProviderIterable = (Iterable<String>)deleteProvider;
+
+          copy.put("deleteProvider", new ImmutableList.Builder<String>()


Would it make sense to enforce this constraint in the setters? We'd have to do so in both setPhoneNumber() and setProvidersToUnlink(), but I think we can reduce the duplication via some sort of a helper function. That would make argument validation consistent with the pattern currently used in this class.

google-cla · 2021-04-09T18:55:46Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

nrsim requested a review from rsgowman January 9, 2020 19:36

Add API to link/unlink provider info to/from user record.

52608d5

nrsim force-pushed the link-federatedid branch from ecd0525 to 52608d5 Compare January 9, 2020 19:46

rsgowman reviewed Jan 9, 2020

View reviewed changes

rsgowman assigned nrsim Jan 9, 2020

Make changes in respond to first round of feedback.

dface3a

nrsim assigned rsgowman Jan 9, 2020

rsgowman reviewed Jan 9, 2020

View reviewed changes

src/main/java/com/google/firebase/auth/UserRecord.java Show resolved Hide resolved

src/main/java/com/google/firebase/auth/UserRecord.java Outdated Show resolved Hide resolved

src/main/java/com/google/firebase/auth/UserRecord.java Show resolved Hide resolved

rsgowman removed their assignment Jan 15, 2020

samtstern mentioned this pull request Apr 1, 2020

Snippets for new Admin auth features firebase/snippets-java#6

Open

rsgowman added 3 commits March 17, 2021 17:00

review feedback

a9e09c2

Merge remote-tracking branch 'origin/master' into link-federatedid

e5847c2

Update API to reflect changes made during api review

0bcaf69

rsgowman requested a review from hiranya911 March 17, 2021 21:27

rsgowman assigned hiranya911 and unassigned nrsim Mar 17, 2021

hiranya911 suggested changes Mar 19, 2021

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Mar 19, 2021

review feedback

cf34cf1

rsgowman assigned hiranya911 and unassigned rsgowman Mar 31, 2021

hiranya911 approved these changes Apr 1, 2021

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Apr 1, 2021

review feedback

e18af15

rsgowman assigned hiranya911 and unassigned rsgowman Apr 9, 2021

hiranya911 approved these changes Apr 19, 2021

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Apr 19, 2021

hiranya911 changed the title ~~Link federatedid~~ feat(auth): Link federatedid Apr 20, 2021

hiranya911 added the release-note label Apr 20, 2021

hiranya911 merged commit 40c25e8 into master Apr 20, 2021

hiranya911 deleted the link-federatedid branch April 20, 2021 21:29

feat(auth): Link federatedid #345

feat(auth): Link federatedid #345

Uh oh!

Conversation

nrsim commented Jan 9, 2020 • edited by rsgowman Loading Uh oh! There was an error while loading. Please reload this page.

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!

rsgowman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Michael-Frank commented Mar 2, 2021

Uh oh!

rsgowman commented Mar 2, 2021

Uh oh!

google-cla bot commented Mar 17, 2021

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!

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!

google-cla bot commented Mar 31, 2021

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented Apr 9, 2021

Uh oh!

Uh oh!

nrsim commented Jan 9, 2020 •

edited by rsgowman

Loading