feat(auth): Add API to look up users by federated ID. #340

nrsim · 2019-12-23T20:29:47Z

RELEASE NOTE: Added a new getUserByProviderUid() method to lookup user accounts by their providers

hiranya911 · 2019-12-23T20:53:54Z

In general we don't add APIs to this SDK that are not already present in the Node.js SDK.

bojeil-google · 2019-12-23T21:00:26Z

Hey @hiranya911, I plan to review the Node.js implementation these 2 weeks. It's in my backlog. I think @rsgowman's plan was to launch multiple languages together (including the Java implementation).

hiranya911 · 2019-12-23T21:11:07Z

Sounds good. I'll assign to @rsgowman for now. Do we have an API proposal for this?

bojeil-google · 2019-12-23T21:21:46Z

Yeah, link is at: https://docs.google.com/document/d/1OfN5XH6cYJ7y3AnNsocftQSrbMiMaLlPlkWWWsOpDQg/

The proposal is approved.

hiranya911 · 2019-12-23T21:27:16Z

That only has the bulk get and delete API as far as I can tell. I don't see getUserByFederatedId() in there.

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

rsgowman · 2019-12-23T21:41:35Z

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

@@ -605,6 +605,56 @@ protected UserRecord execute() throws FirebaseAuthException {
    };
  }

+  /**
+   * Gets the user data corresponding to the specified user federated identifier.


...the specified user's federated identifier.

(also below)

rsgowman · 2019-12-23T21:58:24Z

This is somewhat related to the bulk get, but is slightly different. The API doc for this feature is here: http://doc/1ikY2GoQZrwHNJ5mBa---t2o9R-MdjA4np4uW9SJ9dTg. It isn't approved yet (and thus we can't land this, but we don't want to delay starting on it while waiting for the API approval... especially this time of year.)

bojeil-google · 2019-12-23T21:59:58Z

Oh I didn't know that. I don't understand why this is being implemented before the other then. Definitely let's hold on this.

nrsim · 2019-12-27T18:08:38Z

We'll work on the Node implementation then. Sorry, I didn't know starting with the Node implementation was a must. I started with the Java implementation as the Node implementation was going to be tangled with bulk add/delete changes that are sent for review.

bojeil-google · 2020-01-13T05:12:47Z

Note I won't have time to review this PR. I will only have time to review the Node.js implementation. That should be the case going forward. @rsgowman and @hiranya911 reviews should be sufficient.

hiranya911

This requires some unit tests. So far it only has integration tests. See FirebaseUserManagerTest.

hiranya911 · 2020-01-31T19:22:30Z

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

    } finally {
-      auth.deleteUserAsync(userRecord.getUid()).get();
+      auth.deleteUserAsync(randomUser1.uid).get();


We should use bulk delete for this when it becomes available. Add a TODO so we don't forget.

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

google-cla · 2021-03-16T22:03:12Z

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

Implementation looks pretty solid. Just a few thoughts on cleaning up the tests a bit.

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

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

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

google-cla · 2021-03-31T19:50:41Z

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 pretty solid @rsgowman. Just one more comment concerning the integration tests (Sorry for not bringing that up earlier, but I completely forgot about the TemporaryUser abstraction that we now use in our tests).

hiranya911 · 2021-04-01T18:41:14Z

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

-      auth.deleteUserAsync(randomUser1.getUid()).get();
-      auth.deleteUserAsync(randomUser2.getUid()).get();
-      auth.deleteUserAsync(randomUser3.getUid()).get();
+      if (user1 != null) {


I just remembered that we now have a TemporaryUser abstraction that handles automatic clean up of user accounts created during testing. Can we tie createRandomUser() and importRandomUser() methods to that abstraction, and clean up this logic?

Here's an example of how it's being used:

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

Lines 446 to 449 in d8b1583

@Test

public void testCustomClaims() throws Exception {

UserRecord userRecord = temporaryUser.create(new UserRecord.CreateRequest());

String uid = userRecord.getUid();

google-cla · 2021-04-09T19:00:24Z

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 👍

Let me know when you want this merged, and I can perform a force merge to override the CLA bot.

Add API to look up users by federated ID.

5484ab1

nrsim requested review from hiranya911 and rsgowman December 23, 2019 20:29

hiranya911 assigned bojeil-google Dec 23, 2019

hiranya911 assigned rsgowman Dec 23, 2019

rsgowman reviewed Dec 23, 2019

View reviewed changes

Apply first round of feedback.

debda03

bojeil-google assigned hiranya911 and unassigned bojeil-google Jan 13, 2020

hiranya911 suggested changes Jan 31, 2020

View reviewed changes

hiranya911 assigned nrsim and unassigned hiranya911 Jan 31, 2020

samtstern mentioned this pull request Apr 1, 2020

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

Open

rsgowman reviewed Apr 1, 2020

View reviewed changes

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

rsgowman added 2 commits March 16, 2021 16:46

Review feedback

b3af254

Merge remote-tracking branch 'origin/master' into lookup-by-federatedid

c29a802

rsgowman assigned hiranya911 and unassigned rsgowman and nrsim Mar 16, 2021

hiranya911 suggested changes Mar 19, 2021

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Mar 19, 2021

hiranya911 changed the title ~~Add API to look up users by federated ID.~~ feat(auth): Add API to look up users by federated ID. Mar 19, 2021

hiranya911 added the release-note label Mar 19, 2021

review feedback

c1025e8

rsgowman assigned hiranya911 and unassigned rsgowman Mar 31, 2021

hiranya911 suggested changes Apr 1, 2021

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Apr 1, 2021

review feedback

f9b69a0

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 merged commit 589f08b into master Apr 20, 2021

hiranya911 deleted the lookup-by-federatedid branch April 20, 2021 21:28

	@Test
	public void testCustomClaims() throws Exception {
	UserRecord userRecord = temporaryUser.create(new UserRecord.CreateRequest());
	String uid = userRecord.getUid();

feat(auth): Add API to look up users by federated ID. #340

feat(auth): Add API to look up users by federated ID. #340

Uh oh!

Conversation

nrsim commented Dec 23, 2019 • edited by rsgowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 commented Dec 23, 2019

Uh oh!

bojeil-google commented Dec 23, 2019

Uh oh!

hiranya911 commented Dec 23, 2019

Uh oh!

bojeil-google commented Dec 23, 2019

Uh oh!

hiranya911 commented Dec 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rsgowman Dec 23, 2019

Choose a reason for hiding this comment

Uh oh!

rsgowman commented Dec 23, 2019

Uh oh!

bojeil-google commented Dec 23, 2019

Uh oh!

nrsim commented Dec 27, 2019

Uh oh!

bojeil-google commented Jan 13, 2020

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

google-cla bot commented Mar 16, 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!

Uh oh!

google-cla bot commented Mar 31, 2021

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

rsgowman Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

google-cla bot commented Apr 9, 2021

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrsim commented Dec 23, 2019 •

edited by rsgowman

Loading