-
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 5 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. | ||
*/ | ||
getUserByProviderUid(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 |
---|---|---|
|
@@ -168,6 +168,44 @@ describe('admin.auth', () => { | |
}); | ||
}); | ||
|
||
it('getUserByProviderUid() 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 randomUid = 'import_' + generateRandomString(20).toLowerCase(); | ||
const importUser: admin.auth.UserImportRecord = { | ||
uid: randomUid, | ||
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 admin.auth().importUsers([importUser]); | ||
|
||
try { | ||
await admin.auth().getUserByProviderUid('google.com', 'google_uid') | ||
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. Should we also call this API with email and phone and see if we get the expected result? 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 we've already got that on lines 210,217. Or were you referring to something else? 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 was referring to testing invocations like 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. Ah, I understand now. Yeah; we should have those too. Done. 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. Thanks. LGTM 👍 |
||
.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 +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', () => { | ||
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. s/federated/provider/ 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. |
||
return admin.auth().getUserByProviderUid('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('getUserByProviderUid()', () => { | ||
const providerId = 'google.com'; | ||
const providerUid = '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 provider id', () => { | ||
expect(() => (auth as any).getUserByProviderUid()) | ||
.to.throw(FirebaseAuthError) | ||
.with.property('code', 'auth/invalid-provider-id'); | ||
}); | ||
|
||
it('should be rejected given an invalid provider id', () => { | ||
expect(() => auth.getUserByProviderUid('', 'uid')) | ||
.to.throw(FirebaseAuthError) | ||
.with.property('code', 'auth/invalid-provider-id'); | ||
}); | ||
|
||
it('should be rejected given an invalid provider uid', () => { | ||
expect(() => auth.getUserByProviderUid('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.getUserByProviderUid(providerId, providerUid) | ||
.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.getUserByProviderUid(providerId, providerUid) | ||
.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.getUserByProviderUid(providerId, providerUid) | ||
.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, 'getAccountInfoByFederatedUid') | ||
.resolves(expectedGetAccountInfoResult); | ||
stubs.push(stub); | ||
return auth.getUserByProviderUid(providerId, providerUid) | ||
.then((userRecord) => { | ||
// Confirm underlying API called with expected parameters. | ||
expect(stub).to.have.been.calledOnce.and.calledWith(providerId, providerUid); | ||
// 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.getUserByProviderUid('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.getUserByProviderUid('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 getAccountInfoByFederatedUid to throw a backend error. | ||
const stub = sinon.stub(testConfig.RequestHandler.prototype, 'getAccountInfoByFederatedUid') | ||
.rejects(expectedError); | ||
stubs.push(stub); | ||
return auth.getUserByProviderUid(providerId, providerUid) | ||
.then((userRecord) => { | ||
throw new Error('Unexpected success'); | ||
}, (error) => { | ||
// Confirm underlying API called with expected parameters. | ||
expect(stub).to.have.been.calledOnce.and.calledWith(providerId, providerUid); | ||
// 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.
Here too. Perhaps
provider
andproviderUid
?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.
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 simplyuid
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel pretty positive about
providerId
anduid
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.