-
Notifications
You must be signed in to change notification settings - Fork 373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(auth): Implement getUserByProviderId #769
Changes from 2 commits
fa9934b
b7458e2
4609191
a59ef60
e9197f9
8dbdd6e
40e0060
1cb1178
f6c19a9
8440262
88c5f5c
655b20f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest caps like line 1539. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
* | ||
* See [Retrieve user data](/docs/auth/admin/manage-users#retrieve_user_data) | ||
* for code samples and detailed documentation. | ||
* | ||
* @param providerId The provider ID, for example, "google.com" for the | ||
* Google provider. | ||
* @param providerUid The user identifier for the given provider. | ||
* | ||
* @return A promise fulfilled with the user data corresponding to the | ||
* given provider id. | ||
*/ | ||
getUserByProviderId(providerId: string, providerUid: string): Promise<admin.auth.UserRecord>; | ||
|
||
/** | ||
* Retrieves a list of users (single batch only) with a size of `maxResults` | ||
* starting from the offset as specified by `pageToken`. This is used to | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ const mockUserData = { | |
displayName: 'Random User ' + newUserUid, | ||
photoURL: 'http://www.example.com/' + newUserUid + '/photo.png', | ||
disabled: false, | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove the blank line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
}; | ||
const actionCodeSettings = { | ||
url: 'http://localhost/?a=1&b=2#c=3', | ||
|
@@ -168,6 +169,44 @@ describe('admin.auth', () => { | |
}); | ||
}); | ||
|
||
it('getUserByProviderId() returns a user record with the matching provider id', async () => { | ||
// TODO(rsgowman): Once we can link a provider id with a user, just do that | ||
// here instead of creating a new user. | ||
const importUser: admin.auth.UserImportRecord = { | ||
uid: 'uid', | ||
email: '[email protected]', | ||
phoneNumber: '+15555550000', | ||
emailVerified: true, | ||
disabled: false, | ||
metadata: { | ||
lastSignInTime: 'Thu, 01 Jan 1970 00:00:00 UTC', | ||
creationTime: 'Thu, 01 Jan 1970 00:00:00 UTC', | ||
toJSON: () => { throw new Error('Unimplemented'); }, | ||
}, | ||
providerData: [{ | ||
displayName: 'User Name', | ||
email: '[email protected]', | ||
phoneNumber: '+15555550000', | ||
photoURL: 'http://example.com/user', | ||
toJSON: () => { throw new Error('Unimplemented'); }, | ||
providerId: 'google.com', | ||
uid: 'google_uid', | ||
}], | ||
}; | ||
|
||
await safeDelete(importUser.uid); | ||
hiranya911 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
await admin.auth().importUsers([importUser]); | ||
|
||
try { | ||
await admin.auth().getUserByProviderId('google.com', 'google_uid') | ||
.then((userRecord) => { | ||
expect(userRecord.uid).to.equal(importUser.uid); | ||
}); | ||
} finally { | ||
await safeDelete(importUser.uid); | ||
} | ||
}); | ||
|
||
it('listUsers() returns up to the specified number of users', () => { | ||
const promises: Array<Promise<admin.auth.UserRecord>> = []; | ||
uids.forEach((uid) => { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for invalid provider id/uid here as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think they're already covered by the unit tests. Specifically, these ones: (Or have I missed something?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I meant to ask for unit tests in test/unit/auth/auth-api-request.spec.ts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.) |
||
return admin.auth().getUserByProviderId('google.com', nonexistentUid) | ||
.should.eventually.be.rejected.and.have.property('code', 'auth/user-not-found'); | ||
}); | ||
|
||
it('updateUser() fails when called with a non-existing UID', () => { | ||
return admin.auth().updateUser(nonexistentUid, { | ||
emailVerified: true, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1130,6 +1130,120 @@ AUTH_CONFIGS.forEach((testConfig) => { | |
}); | ||
}); | ||
|
||
describe('getUserByProviderId()', () => { | ||
const federatedId = 'google.com'; | ||
const federatedUid = 'google_uid'; | ||
const tenantId = testConfig.supportsTenantManagement ? undefined : TENANT_ID; | ||
const expectedGetAccountInfoResult = getValidGetAccountInfoResponse(tenantId); | ||
const expectedUserRecord = getValidUserRecord(expectedGetAccountInfoResult); | ||
const expectedError = new FirebaseAuthError(AuthClientErrorCode.USER_NOT_FOUND); | ||
|
||
// Stubs used to simulate underlying api calls. | ||
let stubs: sinon.SinonStub[] = []; | ||
beforeEach(() => sinon.spy(validator, 'isEmail')); | ||
afterEach(() => { | ||
(validator.isEmail as any).restore(); | ||
_.forEach(stubs, (stub) => stub.restore()); | ||
stubs = []; | ||
}); | ||
|
||
it('should be rejected given no federated id', () => { | ||
hiranya911 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
expect(() => (auth as any).getUserByProviderId()) | ||
.to.throw(FirebaseAuthError) | ||
.with.property('code', 'auth/invalid-provider-id'); | ||
}); | ||
|
||
it('should be rejected given an invalid federated id', () => { | ||
expect(() => auth.getUserByProviderId('', 'uid')) | ||
.to.throw(FirebaseAuthError) | ||
.with.property('code', 'auth/invalid-provider-id'); | ||
}); | ||
|
||
it('should be rejected given an invalid federated uid', () => { | ||
expect(() => auth.getUserByProviderId('id', '')) | ||
.to.throw(FirebaseAuthError) | ||
.with.property('code', 'auth/invalid-provider-id'); | ||
}); | ||
|
||
it('should be rejected given an app which returns null access tokens', () => { | ||
return nullAccessTokenAuth.getUserByProviderId(federatedId, federatedUid) | ||
.should.eventually.be.rejected.and.have.property('code', 'app/invalid-credential'); | ||
}); | ||
|
||
it('should be rejected given an app which returns invalid access tokens', () => { | ||
return malformedAccessTokenAuth.getUserByProviderId(federatedId, federatedUid) | ||
.should.eventually.be.rejected.and.have.property('code', 'app/invalid-credential'); | ||
}); | ||
|
||
it('should be rejected given an app which fails to generate access tokens', () => { | ||
return rejectedPromiseAccessTokenAuth.getUserByProviderId(federatedId, federatedUid) | ||
.should.eventually.be.rejected.and.have.property('code', 'app/invalid-credential'); | ||
}); | ||
|
||
it('should resolve with a UserRecord on success', () => { | ||
// Stub getAccountInfoByEmail to return expected result. | ||
const stub = sinon.stub(testConfig.RequestHandler.prototype, 'getAccountInfoByFederatedId') | ||
.resolves(expectedGetAccountInfoResult); | ||
stubs.push(stub); | ||
return auth.getUserByProviderId(federatedId, federatedUid) | ||
.then((userRecord) => { | ||
// Confirm underlying API called with expected parameters. | ||
expect(stub).to.have.been.calledOnce.and.calledWith(federatedId, federatedUid); | ||
// Confirm expected user record response returned. | ||
expect(userRecord).to.deep.equal(expectedUserRecord); | ||
}); | ||
}); | ||
|
||
describe('non-federated providers', () => { | ||
let invokeRequestHandlerStub: sinon.SinonStub; | ||
beforeEach(() => { | ||
invokeRequestHandlerStub = sinon.stub(testConfig.RequestHandler.prototype, 'invokeRequestHandler') | ||
.resolves({ | ||
// nothing here is checked; we just need enough to not crash. | ||
users: [{ | ||
localId: 1, | ||
}], | ||
}); | ||
|
||
}); | ||
afterEach(() => { | ||
invokeRequestHandlerStub.restore(); | ||
}); | ||
|
||
it('phone lookups should use phoneNumber field', async () => { | ||
await auth.getUserByProviderId('phone', '+15555550001'); | ||
expect(invokeRequestHandlerStub).to.have.been.calledOnce.and.calledWith( | ||
sinon.match.any, sinon.match.any, { | ||
phoneNumber: ['+15555550001'], | ||
}); | ||
}); | ||
|
||
it('email lookups should use email field', async () => { | ||
await auth.getUserByProviderId('email', '[email protected]'); | ||
expect(invokeRequestHandlerStub).to.have.been.calledOnce.and.calledWith( | ||
sinon.match.any, sinon.match.any, { | ||
email: ['[email protected]'], | ||
}); | ||
}); | ||
}); | ||
|
||
it('should throw an error when the backend returns an error', () => { | ||
// Stub getAccountInfoByFederatedId to throw a backend error. | ||
const stub = sinon.stub(testConfig.RequestHandler.prototype, 'getAccountInfoByFederatedId') | ||
.rejects(expectedError); | ||
stubs.push(stub); | ||
return auth.getUserByProviderId(federatedId, federatedUid) | ||
.then((userRecord) => { | ||
throw new Error('Unexpected success'); | ||
}, (error) => { | ||
// Confirm underlying API called with expected parameters. | ||
expect(stub).to.have.been.calledOnce.and.calledWith(federatedId, federatedUid); | ||
// Confirm expected error returned. | ||
expect(error).to.equal(expectedError); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('deleteUser()', () => { | ||
const uid = 'abcdefghijklmnopqrstuvwxyz'; | ||
const expectedDeleteAccountResult = {kind: 'identitytoolkit#DeleteAccountResponse'}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the final decision to call this method getUserByProviderId or getUserByProviderUid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!