feat(auth): Implement getUserByProviderId by rsgowman · Pull Request #769 · firebase/firebase-admin-node · GitHub

RELEASE NOTE: Added a new getUserByProviderId() to lookup user accounts by their providers.

Was the final decision to call this method getUserByProviderId or getUserByProviderUid?

Technically neither, since the final decision hasn't been made yet. :)

But you're quite right that the current state uses Uid. Fixed. (Nice catch.)

@rsgowman Is it solidated? The released name also conflict with the method name I see here

https://firebase.google.com/support/release-notes/admin/node#9.5.0

Hi @Thaina; the final decision here was to use Uid, i.e. getUserByProviderUid. The release notes are incorrect (caused by my description of this PR not being updated to reflect this change). I'll see if I can get that fixed. Thanks for noticing!

nit: remove the blank line

Done.

Add unit tests for invalid provider id/uid here as well?

I think they're already covered by the unit tests. Specifically, these ones:
'should be rejected given no federated id'
'should be rejected given an invalid federated id'
'should be rejected given an invalid federated uid'

(Or have I missed something?)

Sorry, I meant to ask for unit tests in test/unit/auth/auth-api-request.spec.ts.

Oh; I hadn't noticed that. Done.

(More thought required, but I suspect some of those tests may be redundant since we're already effectively testing them from the api level. It might be possible to eliminate some of them.)

LGTM overall.

Looks great. Just a few comments to improve readability.

Here too. Perhaps provider and providerUid?

This is what was agreed to in the api review, so I'm reluctant to change that. But if we were to do so:

providerId has prior art (UserINfo, AuthProviderConfig, getProviderConfig, deleteProviderConfig, updateProviderConfig) so I'd prefer to leave that alone.

providerUid however does not. Although a bit overloaded, we could use simply uid here instead. I think it's sufficiently clear.

If we do want to change this, we should also revisit the bulk get PR which uses similar names.

wdyt?

I feel pretty positive about providerId and uid. If we are in agreement, let's go ahead and implement that change. This is a non-breaking change for Node.js (but may be a breaking change for languages like C# that allow named arguments in method calls).

I'm not a fan of uid, as it may be confused with Firebase uid (see getUser(uid: string), for example). Having a different name for provider uid would make the distinction explicit.

I'm not 100% sure where this ended up, but at this point, I'm going to go ahead and use Hiranya's suggestion. (But I'm not going to attempt revisiting the bulk get PR.)

However, I haven't changed UserIdentifier.providerUid.

LGTM with the argument rename in the public API pending.

s/federated/provider/

Done.

LG from the doc perspective with a tiny nit. Thanks!

Suggest caps like line 1539.

Done.

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. 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 then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

rsgowman added 2 commits January 20, 2020 16:25

Implement getUserByFederatedId()

fa9934b

getAccountByFederatedId -> getAccountByProviderId

b7458e2

rsgowman requested a review from nrsim January 20, 2020 21:37

rsgowman assigned nrsim Jan 20, 2020

nrsim reviewed Jan 21, 2020

View reviewed changes

Comment thread src/auth/auth.ts

nrsim reviewed Jan 21, 2020

View reviewed changes

nrsim assigned rsgowman and unassigned nrsim Jan 21, 2020

rsgowman assigned nrsim and unassigned rsgowman Jan 22, 2020

review feedback

4609191

nrsim approved these changes Jan 22, 2020

View reviewed changes

nrsim assigned rsgowman and unassigned nrsim Jan 22, 2020

hiranya911 suggested changes Feb 3, 2020

View reviewed changes

hiranya911 requested a review from egilmorez February 3, 2020 22:14

hiranya911 assigned egilmorez and unassigned rsgowman Feb 3, 2020

review feedback 2

a59ef60

rsgowman assigned hiranya911 Feb 5, 2020

review feedback 3

e9197f9

rsgowman mentioned this pull request Feb 5, 2020

feat(auth): Implement getUserByProviderUid. firebase/firebase-admin-dotnet#142

Open

hiranya911 approved these changes Feb 6, 2020

View reviewed changes

hiranya911 removed their assignment Feb 6, 2020

hiranya911 changed the title Implement getUserByProviderId feat(auth): Implement getUserByProviderId Feb 6, 2020

hiranya911 added the release-note label Feb 6, 2020

egilmorez approved these changes Feb 19, 2020

View reviewed changes

davidhin mentioned this pull request Jul 6, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#682

Open

This was referenced Jul 17, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#685

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#687

Open

This was referenced Aug 10, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#697

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#700

Open

This was referenced Aug 28, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#707

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#709

Open

This was referenced Sep 5, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#711

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#712

Open

This was referenced Oct 26, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#729

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#731

Open

davidhin mentioned this pull request Nov 9, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#732

Open

+                 * @return A promise fulfilled with the user data corresponding to the
+                 *   given provider id.
+                 */
+                public getUserByProviderId(providerId: string, providerUid: string): Promise<UserRecord> {

+                 * @return A promise fulfilled with the user data corresponding to the
+                 *   given provider id.
+                 */
+                public getUserByProviderUid(providerId: string, providerUid: string): Promise<UserRecord> {

                 displayName: 'Random User ' + newUserUid,
                 photoURL: 'http://www.example.com/' + newUserUid + '/photo.png',
                 disabled: false,

                     .should.eventually.be.rejected.and.have.property('code', 'auth/user-not-found');
                 });
+                it('getUserByProviderId() fails when called with a non-existing federated id', () => {

                   getUserByPhoneNumber(phoneNumber: string): Promise<admin.auth.UserRecord>;
+                  /**
+                   * Gets the user data for the user corresponding to a given provider id.

feat(auth): Implement getUserByProviderId by rsgowman · Pull Request #769 · firebase/firebase-admin-node · GitHub

Uh oh!

feat(auth): Implement getUserByProviderId - #769

feat(auth): Implement getUserByProviderId#769rsgowman merged 12 commits intomasterfirebase/firebase-admin-node:masterfrom rsgowman/getAccountByProviderIdfirebase/firebase-admin-node:rsgowman/getAccountByProviderIdCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

Uh oh!

nrsim Jan 21, 2020 • 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!

nrsim left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

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!

Uh oh!

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!

egilmorez 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!

google-cla Bot commented Dec 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat(auth): Implement getUserByProviderId#769
rsgowman merged 12 commits into
masterfrom
rsgowman/getAccountByProviderId

rsgowman commented Jan 20, 2020 •
edited by hiranya911
Loading

nrsim Jan 21, 2020 •
edited
Loading