feat(auth): Implement getUserByProviderId #769
Conversation
src/auth/auth.ts
Outdated
| * @return A promise fulfilled with the user data corresponding to the | ||
| * given provider id. | ||
| */ | ||
| public getUserByProviderId(providerId: string, providerUid: string): Promise<UserRecord> { |
There was a problem hiding this comment.
Was the final decision to call this method getUserByProviderId or getUserByProviderUid?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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!
test/integration/auth.spec.ts
Outdated
| @@ -66,6 +66,7 @@ const mockUserData = { | |||
| displayName: 'Random User ' + newUserUid, | |||
| photoURL: 'http://www.example.com/' + newUserUid + '/photo.png', | |||
| disabled: false, | |||
|
|
|||
test/integration/auth.spec.ts
Outdated
| @@ -356,6 +395,11 @@ describe('admin.auth', () => { | |||
| .should.eventually.be.rejected.and.have.property('code', 'auth/user-not-found'); | |||
| }); | |||
|
|
|||
| it('getUserByProviderId() fails when called with a non-existing federated id', () => { | |||
There was a problem hiding this comment.
Add unit tests for invalid provider id/uid here as well?
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
Sorry, I meant to ask for unit tests in test/unit/auth/auth-api-request.spec.ts.
There was a problem hiding this comment.
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.)
src/auth/auth.ts
Outdated
| * @return A promise fulfilled with the user data corresponding to the | ||
| * given provider id. | ||
| */ | ||
| public getUserByProviderUid(providerId: string, providerUid: string): Promise<UserRecord> { |
There was a problem hiding this comment.
Here too. Perhaps provider and providerUid?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
test/integration/auth.spec.ts
Outdated
| @@ -356,6 +394,11 @@ describe('admin.auth', () => { | |||
| .should.eventually.be.rejected.and.have.property('code', 'auth/user-not-found'); | |||
| }); | |||
|
|
|||
| it('getUserByProviderUid() fails when called with a non-existing federated id', () => { | |||
hiranya911
changed the title
src/index.d.ts
Outdated
| @@ -1530,6 +1530,21 @@ declare namespace admin.auth { | |||
| */ | |||
| getUserByPhoneNumber(phoneNumber: string): Promise<admin.auth.UserRecord>; | |||
|
|
|||
| /** | |||
| * Gets the user data for the user corresponding to a given provider id. | |||
There was a problem hiding this comment.
Suggest caps like line 1539.
|
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. ℹ️ Googlers: Go here for more info. |
RELEASE NOTE: Added a new
getUserByProviderId()to lookup user accounts by their providers.