Skip to content
Prev Previous commit
Next Next commit
getAccountByFederatedId -> getAccountByProviderId
  • Loading branch information
rsgowman committed Jan 20, 2020
commit b7458e23c6f191f483f39e87b46a4de2d72a1a32
23 changes: 16 additions & 7 deletions src/auth/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,20 +201,29 @@ export class BaseAuth<T extends AbstractAuthRequestHandler> {
}

/**
* Gets the user data for the user corresponding to a given federated id.
* Gets the user data for the user corresponding to a given provider id.
*
* See [Retrieve user data](/docs/auth/admin/manage-users#retrieve_user_data)
* for code samples and detailed documentation.
*
* @param federatedId The provider ID, for example, "google.com" for the
* @param providerId The provider ID, for example, "google.com" for the
* Google provider.
* @param federatedUid The user identifier for the given provider.
* @param providerUid The user identifier for the given provider.
*
* @return A promise fulfilled with the user data corresponding to the
* provided federated id.
*/
public getUserByFederatedId(federatedId: string, federatedUid: string): Promise<UserRecord> {
return this.authRequestHandler.getAccountInfoByFederatedId(federatedId, federatedUid)
* given provider id.
*/
public getUserByProviderId(providerId: string, providerUid: string): Promise<UserRecord> {
Copy link
Contributor

@nrsim nrsim Jan 21, 2020

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?

Copy link
Member Author

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.)

Copy link

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

Copy link
Member Author

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!

// Although we don't really advertise it, we want to also handle
// non-federated idps with this call. So if we detect one of them, we'll
// reroute this request appropriately.
if (providerId === 'phone') {
return this.getUserByPhoneNumber(providerUid);
} else if (providerId === 'email') {
return this.getUserByEmail(providerUid);
}

return this.authRequestHandler.getAccountInfoByFederatedId(providerId, providerUid)
.then((response: any) => {
// Returns the user record populated with server response.
return new UserRecord(response.users[0]);
Expand Down
10 changes: 5 additions & 5 deletions src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1531,19 +1531,19 @@ declare namespace admin.auth {
getUserByPhoneNumber(phoneNumber: string): Promise<admin.auth.UserRecord>;

/**
* Gets the user data for the user corresponding to a given federated id.
* Gets the user data for the user corresponding to a given provider id.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest caps like line 1539.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 federatedId The provider ID, for example, "google.com" for the
* @param providerId The provider ID, for example, "google.com" for the
* Google provider.
* @param federatedUid The user identifier for the given provider.
* @param providerUid The user identifier for the given provider.
*
* @return A promise fulfilled with the user data corresponding to the
* provided federated id.
* given provider id.
*/
getUserByFederatedId(federatedId: string, federatedUid: string): Promise<admin.auth.UserRecord>;
getUserByProviderId(providerId: string, providerUid: string): Promise<admin.auth.UserRecord>;

/**
* Retrieves a list of users (single batch only) with a size of `maxResults`
Expand Down
8 changes: 4 additions & 4 deletions test/integration/auth.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ describe('admin.auth', () => {
});
});

it('getUserByFederatedId() returns a user record with the matching federated id', async () => {
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 = {
Expand Down Expand Up @@ -198,7 +198,7 @@ describe('admin.auth', () => {
await admin.auth().importUsers([importUser]);

try {
await admin.auth().getUserByFederatedId('google.com', 'google_uid')
await admin.auth().getUserByProviderId('google.com', 'google_uid')
.then((userRecord) => {
expect(userRecord.uid).to.equal(importUser.uid);
});
Expand Down Expand Up @@ -395,8 +395,8 @@ describe('admin.auth', () => {
.should.eventually.be.rejected.and.have.property('code', 'auth/user-not-found');
});

it('getUserByFederatedId() fails when called with a non-existing federated id', () => {
return admin.auth().getUserByFederatedId('google.com', nonexistentUid)
it('getUserByProviderId() fails when called with a non-existing federated id', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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:
'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?)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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');
});

Expand Down
51 changes: 42 additions & 9 deletions test/unit/auth/auth.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1130,7 +1130,7 @@ AUTH_CONFIGS.forEach((testConfig) => {
});
});

describe('getUserByFederatedId()', () => {
describe('getUserByProviderId()', () => {
const federatedId = 'google.com';
const federatedUid = 'google_uid';
const tenantId = testConfig.supportsTenantManagement ? undefined : TENANT_ID;
Expand All @@ -1148,35 +1148,35 @@ AUTH_CONFIGS.forEach((testConfig) => {
});

it('should be rejected given no federated id', () => {
expect(() => (auth as any).getUserByFederatedId())
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.getUserByFederatedId('', 'uid'))
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.getUserByFederatedId('id', ''))
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.getUserByFederatedId(federatedId, federatedUid)
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.getUserByFederatedId(federatedId, federatedUid)
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.getUserByFederatedId(federatedId, federatedUid)
return rejectedPromiseAccessTokenAuth.getUserByProviderId(federatedId, federatedUid)
.should.eventually.be.rejected.and.have.property('code', 'app/invalid-credential');
});

Expand All @@ -1185,7 +1185,7 @@ AUTH_CONFIGS.forEach((testConfig) => {
const stub = sinon.stub(testConfig.RequestHandler.prototype, 'getAccountInfoByFederatedId')
.resolves(expectedGetAccountInfoResult);
stubs.push(stub);
return auth.getUserByFederatedId(federatedId, federatedUid)
return auth.getUserByProviderId(federatedId, federatedUid)
.then((userRecord) => {
// Confirm underlying API called with expected parameters.
expect(stub).to.have.been.calledOnce.and.calledWith(federatedId, federatedUid);
Expand All @@ -1194,12 +1194,45 @@ AUTH_CONFIGS.forEach((testConfig) => {
});
});

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.getUserByFederatedId(federatedId, federatedUid)
return auth.getUserByProviderId(federatedId, federatedUid)
.then((userRecord) => {
throw new Error('Unexpected success');
}, (error) => {
Expand Down