From 59d93aa84b9d0924f6e05f4bb36f38f73af055d5 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Mon, 20 Jan 2020 16:25:39 -0500 Subject: [PATCH 01/10] Add ability to link a federated id with the updateUser() method. --- src/auth/auth-api-request.ts | 54 +++++++- src/auth/auth.ts | 44 ++++++ src/auth/user-record.ts | 37 ++++++ src/index.d.ts | 46 +++++++ test/integration/auth.spec.ts | 120 ++++++++++++++--- test/unit/auth/auth.spec.ts | 244 +++++++++++++++++++++++++++++++--- 6 files changed, 500 insertions(+), 45 deletions(-) diff --git a/src/auth/auth-api-request.ts b/src/auth/auth-api-request.ts index 3b3f7a7f21..31ac661448 100755 --- a/src/auth/auth-api-request.ts +++ b/src/auth/auth-api-request.ts @@ -265,6 +265,8 @@ function validateCreateEditRequest(request: any, uploadAccountRequest: boolean = phoneNumber: true, customAttributes: true, validSince: true, + // Pass linkProviderUserInfo only for updates (i.e. not for uploads.) + linkProviderUserInfo: !uploadAccountRequest, // Pass tenantId only for uploadAccount requests. tenantId: uploadAccountRequest, passwordHash: uploadAccountRequest, @@ -410,6 +412,11 @@ function validateCreateEditRequest(request: any, uploadAccountRequest: boolean = validateProviderUserInfo(providerUserInfoEntry); }); } + + // linkProviderUserInfo must be a (single) UserInfo value. + if (typeof request.linkProviderUserInfo !== 'undefined') { + validateProviderUserInfo(request.linkProviderUserInfo); + } } @@ -961,6 +968,31 @@ export abstract class AbstractAuthRequestHandler { 'Properties argument must be a non-null object.', ), ); + } else if (validator.isNonNullObject(properties.providerToLink)) { + if (!validator.isNonEmptyString(properties.providerToLink.providerId)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + 'providerToLink.providerId of properties argument must be a non-empty string.'); + } + if (!validator.isNonEmptyString(properties.providerToLink.uid)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + 'providerToLink.uid of properties argument must be a non-empty string.'); + } + } else if (typeof properties.providersToDelete !== 'undefined') { + if (!validator.isNonEmptyArray(properties.providersToDelete)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + 'providersToDelete of properties argument must be a non-empty array of strings.'); + } + + properties.providersToDelete.forEach((providerId) => { + if (!validator.isNonEmptyString(providerId)) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + 'providersToDelete of properties argument must be a non-empty array of strings.'); + } + }); } // Build the setAccountInfo request. @@ -995,13 +1027,25 @@ export abstract class AbstractAuthRequestHandler { // It will be removed from the backend request and an additional parameter // deleteProvider: ['phone'] with an array of providerIds (phone in this case), // will be passed. - // Currently this applies to phone provider only. if (request.phoneNumber === null) { - request.deleteProvider = ['phone']; + request.deleteProvider ? request.deleteProvider.push('phone') : request.deleteProvider = ['phone']; delete request.phoneNumber; - } else { - // Doesn't apply to other providers in admin SDK. - delete request.deleteProvider; + } + + if (typeof(request.providerToLink) !== 'undefined') { + request.linkProviderUserInfo = deepCopy(request.providerToLink); + delete request.providerToLink; + + request.linkProviderUserInfo.rawId = request.linkProviderUserInfo.uid; + delete request.linkProviderUserInfo.uid; + } + + if (typeof(request.providersToDelete) !== 'undefined') { + if (!validator.isArray(request.deleteProvider)) { + request.deleteProvider = []; + } + request.deleteProvider = request.deleteProvider.concat(request.providersToDelete); + delete request.providersToDelete; } // Rewrite photoURL to photoUrl. diff --git a/src/auth/auth.ts b/src/auth/auth.ts index eddea69cb3..9f5657244f 100755 --- a/src/auth/auth.ts +++ b/src/auth/auth.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import {deepCopy} from '../utils/deep-copy'; import {UserRecord, CreateRequest, UpdateRequest} from './user-record'; import {FirebaseApp} from '../firebase-app'; import {FirebaseTokenGenerator, cryptoSignerFromApp} from './token-generator'; @@ -279,6 +280,49 @@ export class BaseAuth { * @return {Promise} A promise that resolves with the modified user record. */ public updateUser(uid: string, properties: UpdateRequest): Promise { + // Although we don't really advertise it, we want to also handle linking of + // non-federated idps with this call. So if we detect one of them, we'll + // adjust the properties parameter appropriately. This *does* imply that a + // conflict could arise, e.g. if the user provides a phoneNumber property, + // but also provides a providerToLink with a 'phone' provider id. In that + // case, we'll throw an error. + properties = deepCopy(properties); + if (validator.isNonNullObject(properties) && typeof properties.providerToLink !== 'undefined') { + if (properties.providerToLink.providerId === 'email') { + if (typeof properties.email !== 'undefined') { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + "Both UpdateRequest.email and UpdateRequest.providerToLink.providerId='email' were set. To " + + 'link to the email/password provider, only specify the UpdateRequest.email field.'); + } + properties.email = properties.providerToLink.uid; + delete properties.providerToLink; + } else if (properties.providerToLink.providerId === 'phone') { + if (typeof properties.phoneNumber !== 'undefined') { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + "Both UpdateRequest.phoneNumber and UpdateRequest.providerToLink.providerId='phone' were set. To " + + 'link to a phone provider, only specify the UpdateRequest.phoneNumber field.'); + } + properties.phoneNumber = properties.providerToLink.uid; + delete properties.providerToLink; + } + } + if (validator.isNonNullObject(properties) && typeof properties.providersToDelete !== 'undefined') { + if (properties.providersToDelete.indexOf('phone') !== -1) { + // If we've been told to unlink the phone provider both via setting + // phoneNumber to null *and* by setting providersToDelete to include + // 'phone', then we'll reject that. Though it might also be reasonable + // to relax this restriction and just unlink it. + if (properties.phoneNumber === null) { + throw new FirebaseAuthError( + AuthClientErrorCode.INVALID_ARGUMENT, + "Both UpdateRequest.phoneNumber=null and UpdateRequest.providersToDelete=['phone'] were set. To " + + 'unlink from a phone provider, only specify the UpdateRequest.phoneNumber=null field.'); + } + } + } + return this.authRequestHandler.updateExistingAccount(uid, properties) .then((existingUid) => { // Return the corresponding user record. diff --git a/src/auth/user-record.ts b/src/auth/user-record.ts index f8c3adafc7..025b66ff2d 100644 --- a/src/auth/user-record.ts +++ b/src/auth/user-record.ts @@ -50,6 +50,8 @@ export interface UpdateRequest { password?: string; phoneNumber?: string | null; photoURL?: string | null; + providerToLink?: UserProvider; + providersToDelete?: string[]; } /** Parameters for create user operation */ @@ -132,6 +134,41 @@ export class UserInfo { } } +/** + * Represents a user identity provider that can be associated with a Firebase user. + */ +interface UserProvider { + /** + * The user identifier for the linked provider. + */ + uid?: string; + + /** + * The display name for the linked provider. + */ + displayName?: string; + + /** + * The email for the linked provider. + */ + email?: string; + + /** + * The phone number for the linked provider. + */ + phoneNumber?: string; + + /** + * The photo URL for the linked provider. + */ + photoURL?: string; + + /** + * The linked provider ID (for example, "google.com" for the Google provider). + */ + providerId?: string; +} + /** * User record class that defines the Firebase user object populated from * the Firebase Auth getAccountInfo response. diff --git a/src/index.d.ts b/src/index.d.ts index 5177647da2..ef8b3d8e98 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -545,6 +545,42 @@ declare namespace admin.auth { toJSON(): Object; } + /** + * Represents a user identity provider that can be associated with a Firebase user. + */ + interface UserProvider { + + /** + * The user identifier for the linked provider. + */ + uid?: string; + + /** + * The display name for the linked provider. + */ + displayName?: string; + + /** + * The email for the linked provider. + */ + email?: string; + + /** + * The phone number for the linked provider. + */ + phoneNumber?: string; + + /** + * The photo URL for the linked provider. + */ + photoURL?: string; + + /** + * The linked provider ID (for example, "google.com" for the Google provider). + */ + providerId?: string; + } + /** * Interface representing a user. */ @@ -686,6 +722,16 @@ declare namespace admin.auth { * The user's photo URL. */ photoURL?: string | null; + + /** + * Links this user to the specified federated provider. + */ + providerToLink?: UserProvider; + + /** + * Unlinks this user from the specified federated providers. + */ + providersToDelete?: string[]; } /** diff --git a/test/integration/auth.spec.ts b/test/integration/auth.spec.ts index b78e1e6c43..68d2e50579 100755 --- a/test/integration/auth.spec.ts +++ b/test/integration/auth.spec.ts @@ -323,22 +323,106 @@ describe('admin.auth', () => { }); }); - it('updateUser() updates the user record with the given parameters', () => { - const updatedDisplayName = 'Updated User ' + newUserUid; - return admin.auth().updateUser(newUserUid, { - email: updatedEmail, - phoneNumber: updatedPhone, - emailVerified: true, - displayName: updatedDisplayName, - }) - .then((userRecord) => { - expect(userRecord.emailVerified).to.be.true; - expect(userRecord.displayName).to.equal(updatedDisplayName); - // Confirm expected email. - expect(userRecord.email).to.equal(updatedEmail); - // Confirm expected phone number. - expect(userRecord.phoneNumber).to.equal(updatedPhone); + describe('updateUser()', () => { + /** + * Creates a new user for testing purposes. The user's uid will be + * '$name_$tenRandomChars' and email will be + * '$name_$tenRandomChars@example.com'. + */ + // TODO(rsgowman): This function could usefully be employed throughout this file. + function createTestUser(name: string): Promise { + const tenRandomChars = generateRandomString(10); + return admin.auth().createUser({ + uid: name + '_' + tenRandomChars, + displayName: name, + email: name + '_' + tenRandomChars + '@example.com', + }); + } + + let updateUser: admin.auth.UserRecord; + before(async () => { + updateUser = await createTestUser('UpdateUser'); + }); + + after(() => { + return safeDelete(updateUser.uid); + }); + + it('updates the user record with the given parameters', async () => { + const updatedDisplayName = 'Updated User ' + updateUser.uid; + const userRecord = await admin.auth().updateUser(updateUser.uid, { + email: updatedEmail, + phoneNumber: updatedPhone, + emailVerified: true, + displayName: updatedDisplayName, + }); + + expect(userRecord.emailVerified).to.be.true; + expect(userRecord.displayName).to.equal(updatedDisplayName); + // Confirm expected email. + expect(userRecord.email).to.equal(updatedEmail); + // Confirm expected phone number. + expect(userRecord.phoneNumber).to.equal(updatedPhone); + }); + + it('can link/unlink with a federated provider', async () => { + const federatedUid = 'google_uid_' + generateRandomString(10); + let userRecord = await admin.auth().updateUser(updateUser.uid, { + providerToLink: { + providerId: 'google.com', + uid: federatedUid, + }, + }); + + let providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); + let providerIds = userRecord.providerData.map((userInfo) => userInfo.providerId); + expect(providerUids).to.deep.include(federatedUid); + expect(providerIds).to.deep.include('google.com'); + + userRecord = await admin.auth().updateUser(updateUser.uid, { + providersToDelete: ['google.com'], + }); + + providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); + providerIds = userRecord.providerData.map((userInfo) => userInfo.providerId); + expect(providerUids).to.not.deep.include(federatedUid); + expect(providerIds).to.not.deep.include('google.com'); + }); + + it('can unlink multiple providers at once, incl a non-federated provider', async () => { + await deletePhoneNumberUser('+15555550001'); + + const googleFederatedUid = 'google_uid_' + generateRandomString(10); + const facebookFederatedUid = 'facebook_uid_' + generateRandomString(10); + + let userRecord = await admin.auth().updateUser(updateUser.uid, { + phoneNumber: '+15555550001', + providerToLink: { + providerId: 'google.com', + uid: googleFederatedUid, + }, }); + userRecord = await admin.auth().updateUser(updateUser.uid, { + providerToLink: { + providerId: 'facebook.com', + uid: facebookFederatedUid, + }, + }); + + let providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); + let providerIds = userRecord.providerData.map((userInfo) => userInfo.providerId); + expect(providerUids).to.deep.include.members([googleFederatedUid, facebookFederatedUid, '+15555550001']); + expect(providerIds).to.deep.include.members(['google.com', 'facebook.com', 'phone']); + + userRecord = await admin.auth().updateUser(updateUser.uid, { + providersToDelete: ['google.com', 'facebook.com', 'phone'], + }); + + providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); + providerIds = userRecord.providerData.map((userInfo) => userInfo.providerId); + expect(providerUids).to.not.deep.include.members([googleFederatedUid, facebookFederatedUid, '+15555550001']); + expect(providerIds).to.not.deep.include.members(['google.com', 'facebook.com', 'phone']); + }); }); it('getUser() fails when called with a non-existing UID', () => { @@ -1615,11 +1699,11 @@ function testImportAndSignInUser( /** * Helper function that deletes the user with the specified phone number * if it exists. - * @param {string} phoneNumber The phone number of the user to delete. - * @return {Promise} A promise that resolves when the user is deleted + * @param phoneNumber The phone number of the user to delete. + * @return A promise that resolves when the user is deleted * or is found not to exist. */ -function deletePhoneNumberUser(phoneNumber: string) { +function deletePhoneNumberUser(phoneNumber: string): Promise { return admin.auth().getUserByPhoneNumber(phoneNumber) .then((userRecord) => { return safeDelete(userRecord.uid); diff --git a/test/unit/auth/auth.spec.ts b/test/unit/auth/auth.spec.ts index 0451be7288..fb1e67ff4f 100755 --- a/test/unit/auth/auth.spec.ts +++ b/test/unit/auth/auth.spec.ts @@ -222,6 +222,8 @@ function getSAMLConfigServerResponse(providerId: string): SAMLConfigServerRespon } +const INVALID_PROVIDER_IDS = [ + undefined, null, NaN, 0, 1, true, false, '', [], [1, 'a'], {}, { a: 1 }, _.noop]; const TENANT_ID = 'tenantId'; const AUTH_CONFIGS: AuthTest[] = [ { @@ -1358,6 +1360,10 @@ AUTH_CONFIGS.forEach((testConfig) => { emailVerified: expectedUserRecord.emailVerified, password: 'password', phoneNumber: expectedUserRecord.phoneNumber, + providerToLink: { + providerId: 'google.com', + uid: 'google_uid', + }, }; // Stubs used to simulate underlying api calls. let stubs: sinon.SinonStub[] = []; @@ -1401,10 +1407,204 @@ AUTH_CONFIGS.forEach((testConfig) => { }) .catch((error) => { expect(error).to.have.property('code', 'auth/argument-error'); - expect(validator.isNonNullObject).to.have.been.calledOnce.and.calledWith(null); + expect(validator.isNonNullObject).to.have.been.calledWith(null); + }); + }); + + const invalidUpdateRequests: UpdateRequest[] = [ + { providerToLink: { uid: 'google_uid' } }, + { providerToLink: { providerId: 'google.com' } }, + { providerToLink: { providerId: 'google.com', uid: '' } }, + { providerToLink: { providerId: '', uid: 'google_uid' } }, + ]; + invalidUpdateRequests.forEach((invalidUpdateRequest) => { + it('should be rejected given an UpdateRequest with an invalid providerToLink parameter', () => { + expect(() => { + auth.updateUser(uid, invalidUpdateRequest); + }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); + }); + }); + + it('should rename providerToLink property to linkProviderUserInfo', async () => { + const invokeRequestHandlerStub = sinon.stub(testConfig.RequestHandler.prototype, 'invokeRequestHandler') + .resolves({ + localId: uid, + }); + + // Stub getAccountInfoByUid to return a valid result (unchecked; we + // just need it to be valid so as to not crash.) + const getUserStub = sinon.stub(testConfig.RequestHandler.prototype, 'getAccountInfoByUid') + .resolves(expectedGetAccountInfoResult); + + stubs.push(invokeRequestHandlerStub); + stubs.push(getUserStub); + + await auth.updateUser(uid, { + providerToLink: { + providerId: 'google.com', + uid: 'google_uid', + }, + }); + + expect(invokeRequestHandlerStub).to.have.been.calledOnce.and.calledWith( + sinon.match.any, sinon.match.any, { + localId: uid, + linkProviderUserInfo: { + providerId: 'google.com', + rawId: 'google_uid', + }, + }); + }); + + it('should be rejected given an empty providersToDelete list', () => { + expect(() => { + auth.updateUser(uid, { + providersToDelete: [], + }); + }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); + }); + + INVALID_PROVIDER_IDS.forEach((invalidProviderId) => { + it('should be rejected given a deleteProvider list with an invalid provider ID ' + + JSON.stringify(invalidProviderId), () => { + expect(() => { + auth.updateUser(uid, { + providersToDelete: [ invalidProviderId as any ], + }); + }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); + }); + }); + + it('should merge deletion of phone provider with the providersToDelete list', async () => { + const invokeRequestHandlerStub = sinon.stub(testConfig.RequestHandler.prototype, 'invokeRequestHandler') + .resolves({ + localId: uid, + }); + + // Stub getAccountInfoByUid to return a valid result (unchecked; we + // just need it to be valid so as to not crash.) + const getUserStub = sinon.stub(testConfig.RequestHandler.prototype, 'getAccountInfoByUid') + .resolves(expectedGetAccountInfoResult); + + stubs.push(invokeRequestHandlerStub); + stubs.push(getUserStub); + + await auth.updateUser(uid, { + phoneNumber: null, + providersToDelete: [ 'google.com' ], + }); + + expect(invokeRequestHandlerStub).to.have.been.calledOnce.and.calledWith( + sinon.match.any, sinon.match.any, { + localId: uid, + deleteProvider: [ 'phone', 'google.com' ], }); }); + describe('non-federated providers', () => { + let invokeRequestHandlerStub: sinon.SinonStub; + let getAccountInfoByUidStub: 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, + }], + }); + + getAccountInfoByUidStub = sinon.stub(testConfig.RequestHandler.prototype, 'getAccountInfoByUid') + .resolves({ + // nothing here is checked; we just need enough to not crash. + users: [{ + localId: 1, + }], + }); + }); + afterEach(() => { + invokeRequestHandlerStub.restore(); + getAccountInfoByUidStub.restore(); + }); + + + it('specifying both email and providerId=email should be rejected', () => { + expect(() => { + auth.updateUser(uid, { + email: 'user@example.com', + providerToLink: { + providerId: 'email', + uid: 'user@example.com', + }, + }); + }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); + }); + + it('specifying both phoneNumber and providerId=phone should be rejected', () => { + expect(() => { + auth.updateUser(uid, { + phoneNumber: '+15555550001', + providerToLink: { + providerId: 'phone', + uid: '+15555550001', + }, + }); + }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); + }); + + it('email linking should use email field', async () => { + await auth.updateUser(uid, { + providerToLink: { + providerId: 'email', + uid: 'user@example.com', + }, + }); + expect(invokeRequestHandlerStub).to.have.been.calledOnce.and.calledWith( + sinon.match.any, sinon.match.any, { + localId: uid, + email: 'user@example.com', + }); + }); + + it('phone linking should use phoneNumber field', async () => { + await auth.updateUser(uid, { + providerToLink: { + providerId: 'phone', + uid: '+15555550001', + }, + }); + expect(invokeRequestHandlerStub).to.have.been.calledOnce.and.calledWith( + sinon.match.any, sinon.match.any, { + localId: uid, + phoneNumber: '+15555550001', + }); + }); + + it('specifying both phoneNumber=null and providersToDelete=phone should be rejected', () => { + expect(() => { + auth.updateUser(uid, { + phoneNumber: null, + providersToDelete: ['phone'], + }); + }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); + }); + + it('doesnt mutate the properties parameter', async () => { + const properties: UpdateRequest = { + providerToLink: { + providerId: 'email', + uid: 'user@example.com', + }, + }; + await auth.updateUser(uid, properties); + expect(properties).to.deep.equal({ + providerToLink: { + providerId: 'email', + uid: 'user@example.com', + }, + }); + }); + }); + it('should be rejected given an app which returns null access tokens', () => { return nullAccessTokenAuth.updateUser(uid, propertiesToEdit) .should.eventually.be.rejected.and.have.property('code', 'app/invalid-credential'); @@ -2217,9 +2417,7 @@ AUTH_CONFIGS.forEach((testConfig) => { .should.eventually.be.rejected.and.have.property('code', 'auth/invalid-provider-id'); }); - const invalidProviderIds = [ - undefined, null, NaN, 0, 1, true, false, '', [], [1, 'a'], {}, { a: 1 }, _.noop]; - invalidProviderIds.forEach((invalidProviderId) => { + INVALID_PROVIDER_IDS.forEach((invalidProviderId) => { it(`should be rejected given an invalid provider ID "${JSON.stringify(invalidProviderId)}"`, () => { return (auth as Auth).getProviderConfig(invalidProviderId as any) .then(() => { @@ -2590,15 +2788,16 @@ AUTH_CONFIGS.forEach((testConfig) => { .should.eventually.be.rejected.and.have.property('code', 'auth/invalid-provider-id'); }); - it('should be rejected given an invalid provider ID', () => { - const invalidProviderId = ''; - return (auth as Auth).deleteProviderConfig(invalidProviderId) - .then(() => { - throw new Error('Unexpected success'); - }) - .catch((error) => { - expect(error).to.have.property('code', 'auth/invalid-provider-id'); - }); + INVALID_PROVIDER_IDS.forEach((invalidProviderId) => { + it(`should be rejected given an invalid provider ID "${JSON.stringify(invalidProviderId)}"`, () => { + return (auth as Auth).deleteProviderConfig(invalidProviderId as any) + .then(() => { + throw new Error('Unexpected success'); + }) + .catch((error) => { + expect(error).to.have.property('code', 'auth/invalid-provider-id'); + }); + }); }); it('should be rejected given an app which returns null access tokens', () => { @@ -2709,15 +2908,16 @@ AUTH_CONFIGS.forEach((testConfig) => { .should.eventually.be.rejected.and.have.property('code', 'auth/invalid-provider-id'); }); - it('should be rejected given an invalid provider ID', () => { - const invalidProviderId = ''; - return (auth as Auth).updateProviderConfig(invalidProviderId, oidcConfigOptions) - .then(() => { - throw new Error('Unexpected success'); - }) - .catch((error) => { - expect(error).to.have.property('code', 'auth/invalid-provider-id'); - }); + INVALID_PROVIDER_IDS.forEach((invalidProviderId) => { + it(`should be rejected given an invalid provider ID "${JSON.stringify(invalidProviderId)}"`, () => { + return (auth as Auth).updateProviderConfig(invalidProviderId as any, oidcConfigOptions) + .then(() => { + throw new Error('Unexpected success'); + }) + .catch((error) => { + expect(error).to.have.property('code', 'auth/invalid-provider-id'); + }); + }); }); it('should be rejected given no options', () => { From c79f42d6fbec7b93b1ea7700052e842e28ad7baa Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 28 Jan 2020 10:41:27 -0500 Subject: [PATCH 02/10] review feedback --- src/auth/auth-api-request.ts | 8 ++++---- src/index.d.ts | 4 ++-- test/integration/auth.spec.ts | 21 +++++++++++++++++---- test/unit/auth/auth.spec.ts | 8 -------- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/auth/auth-api-request.ts b/src/auth/auth-api-request.ts index 31ac661448..45fb6a6f40 100755 --- a/src/auth/auth-api-request.ts +++ b/src/auth/auth-api-request.ts @@ -413,7 +413,7 @@ function validateCreateEditRequest(request: any, uploadAccountRequest: boolean = }); } - // linkProviderUserInfo must be a (single) UserInfo value. + // linkProviderUserInfo must be a (single) UserProvider value. if (typeof request.linkProviderUserInfo !== 'undefined') { validateProviderUserInfo(request.linkProviderUserInfo); } @@ -980,17 +980,17 @@ export abstract class AbstractAuthRequestHandler { 'providerToLink.uid of properties argument must be a non-empty string.'); } } else if (typeof properties.providersToDelete !== 'undefined') { - if (!validator.isNonEmptyArray(properties.providersToDelete)) { + if (!validator.isArray(properties.providersToDelete)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, - 'providersToDelete of properties argument must be a non-empty array of strings.'); + 'providersToDelete of properties argument must be an array of strings.'); } properties.providersToDelete.forEach((providerId) => { if (!validator.isNonEmptyString(providerId)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, - 'providersToDelete of properties argument must be a non-empty array of strings.'); + 'providersToDelete of properties argument must be an array of strings.'); } }); } diff --git a/src/index.d.ts b/src/index.d.ts index ef8b3d8e98..7048b7d58d 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -724,12 +724,12 @@ declare namespace admin.auth { photoURL?: string | null; /** - * Links this user to the specified federated provider. + * Links this user to the specified provider. */ providerToLink?: UserProvider; /** - * Unlinks this user from the specified federated providers. + * Unlinks this user from the specified providers. */ providersToDelete?: string[]; } diff --git a/test/integration/auth.spec.ts b/test/integration/auth.spec.ts index 68d2e50579..2bf71e6e0e 100755 --- a/test/integration/auth.spec.ts +++ b/test/integration/auth.spec.ts @@ -366,17 +366,17 @@ describe('admin.auth', () => { }); it('can link/unlink with a federated provider', async () => { - const federatedUid = 'google_uid_' + generateRandomString(10); + const googleFederatedUid = 'google_uid_' + generateRandomString(10); let userRecord = await admin.auth().updateUser(updateUser.uid, { providerToLink: { providerId: 'google.com', - uid: federatedUid, + uid: googleFederatedUid, }, }); let providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); let providerIds = userRecord.providerData.map((userInfo) => userInfo.providerId); - expect(providerUids).to.deep.include(federatedUid); + expect(providerUids).to.deep.include(googleFederatedUid); expect(providerIds).to.deep.include('google.com'); userRecord = await admin.auth().updateUser(updateUser.uid, { @@ -385,7 +385,7 @@ describe('admin.auth', () => { providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); providerIds = userRecord.providerData.map((userInfo) => userInfo.providerId); - expect(providerUids).to.not.deep.include(federatedUid); + expect(providerUids).to.not.deep.include(googleFederatedUid); expect(providerIds).to.not.deep.include('google.com'); }); @@ -423,6 +423,19 @@ describe('admin.auth', () => { expect(providerUids).to.not.deep.include.members([googleFederatedUid, facebookFederatedUid, '+15555550001']); expect(providerIds).to.not.deep.include.members(['google.com', 'facebook.com', 'phone']); }); + + it('noops successfully when given an empty providersToDelete list', async () => { + const userRecord = await createTestUser('NoopWithEmptyProvidersToDeleteUser'); + try { + const updatedUserRecord = await admin.auth().updateUser(userRecord.uid, { + providersToDelete: [], + }); + + expect(updatedUserRecord).to.deep.equal(userRecord); + } finally { + safeDelete(userRecord.uid); + } + }); }); it('getUser() fails when called with a non-existing UID', () => { diff --git a/test/unit/auth/auth.spec.ts b/test/unit/auth/auth.spec.ts index fb1e67ff4f..05d108ed38 100755 --- a/test/unit/auth/auth.spec.ts +++ b/test/unit/auth/auth.spec.ts @@ -1456,14 +1456,6 @@ AUTH_CONFIGS.forEach((testConfig) => { }); }); - it('should be rejected given an empty providersToDelete list', () => { - expect(() => { - auth.updateUser(uid, { - providersToDelete: [], - }); - }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); - }); - INVALID_PROVIDER_IDS.forEach((invalidProviderId) => { it('should be rejected given a deleteProvider list with an invalid provider ID ' + JSON.stringify(invalidProviderId), () => { From dfa1fced9f4985373de5f0db1bca301a896cd073 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Thu, 19 Mar 2020 15:22:56 -0400 Subject: [PATCH 03/10] more details on the docs --- src/index.d.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/index.d.ts b/src/index.d.ts index 7048b7d58d..1a7f2be718 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -725,6 +725,16 @@ declare namespace admin.auth { /** * Links this user to the specified provider. + * + * Linking a provider to an existing user account does not invalidate the + * refresh token of that account. In other words, the existing account + * would continue to be able to access resources, despite not having used + * the newly linked provider to login. If you wish to force the user to + * authenticate with this new provider, you need to (a) revoke their + * refresh token (see + * https://firebase.google.com/docs/auth/admin/manage-sessions#revoke_refresh_tokens), + * and (b) ensure no other authentication methods are present on this + * account. */ providerToLink?: UserProvider; From a04e1755795924561c627119fbfd007410324dcc Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 22 Dec 2020 09:27:29 -0500 Subject: [PATCH 04/10] providersToDelete -> providersToUnlink --- src/auth/auth-api-request.ts | 16 ++++++++-------- src/auth/auth.ts | 8 ++++---- src/auth/index.ts | 2 +- test/integration/auth.spec.ts | 8 ++++---- test/unit/auth/auth.spec.ts | 10 +++++----- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/auth/auth-api-request.ts b/src/auth/auth-api-request.ts index 9beb001ca8..c739e7007f 100644 --- a/src/auth/auth-api-request.ts +++ b/src/auth/auth-api-request.ts @@ -1310,18 +1310,18 @@ export abstract class AbstractAuthRequestHandler { AuthClientErrorCode.INVALID_ARGUMENT, 'providerToLink.uid of properties argument must be a non-empty string.'); } - } else if (typeof properties.providersToDelete !== 'undefined') { - if (!validator.isArray(properties.providersToDelete)) { + } else if (typeof properties.providersToUnlink !== 'undefined') { + if (!validator.isArray(properties.providersToUnlink)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, - 'providersToDelete of properties argument must be an array of strings.'); + 'providersToUnlink of properties argument must be an array of strings.'); } - properties.providersToDelete.forEach((providerId) => { + properties.providersToUnlink.forEach((providerId) => { if (!validator.isNonEmptyString(providerId)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, - 'providersToDelete of properties argument must be an array of strings.'); + 'providersToUnlink of properties argument must be an array of strings.'); } }); } @@ -1371,12 +1371,12 @@ export abstract class AbstractAuthRequestHandler { delete request.linkProviderUserInfo.uid; } - if (typeof(request.providersToDelete) !== 'undefined') { + if (typeof(request.providersToUnlink) !== 'undefined') { if (!validator.isArray(request.deleteProvider)) { request.deleteProvider = []; } - request.deleteProvider = request.deleteProvider.concat(request.providersToDelete); - delete request.providersToDelete; + request.deleteProvider = request.deleteProvider.concat(request.providersToUnlink); + delete request.providersToUnlink; } // Rewrite photoURL to photoUrl. diff --git a/src/auth/auth.ts b/src/auth/auth.ts index d737114dc2..f639588782 100644 --- a/src/auth/auth.ts +++ b/src/auth/auth.ts @@ -396,16 +396,16 @@ export class BaseAuth implements BaseAuthI delete properties.providerToLink; } } - if (validator.isNonNullObject(properties) && typeof properties.providersToDelete !== 'undefined') { - if (properties.providersToDelete.indexOf('phone') !== -1) { + if (validator.isNonNullObject(properties) && typeof properties.providersToUnlink !== 'undefined') { + if (properties.providersToUnlink.indexOf('phone') !== -1) { // If we've been told to unlink the phone provider both via setting - // phoneNumber to null *and* by setting providersToDelete to include + // phoneNumber to null *and* by setting providersToUnlink to include // 'phone', then we'll reject that. Though it might also be reasonable // to relax this restriction and just unlink it. if (properties.phoneNumber === null) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, - "Both UpdateRequest.phoneNumber=null and UpdateRequest.providersToDelete=['phone'] were set. To " + "Both UpdateRequest.phoneNumber=null and UpdateRequest.providersToUnlink=['phone'] were set. To " + 'unlink from a phone provider, only specify the UpdateRequest.phoneNumber=null field.'); } } diff --git a/src/auth/index.ts b/src/auth/index.ts index 41dde14fb6..1c7cdaf09f 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -439,7 +439,7 @@ export namespace auth { /** * Unlinks this user from the specified providers. */ - providersToDelete?: string[]; + providersToUnlink?: string[]; } /** diff --git a/test/integration/auth.spec.ts b/test/integration/auth.spec.ts index e8a11998f6..8f784a6c0f 100644 --- a/test/integration/auth.spec.ts +++ b/test/integration/auth.spec.ts @@ -624,7 +624,7 @@ describe('admin.auth', () => { expect(providerIds).to.deep.include('google.com'); userRecord = await admin.auth().updateUser(updateUser.uid, { - providersToDelete: ['google.com'], + providersToUnlink: ['google.com'], }); providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); @@ -659,7 +659,7 @@ describe('admin.auth', () => { expect(providerIds).to.deep.include.members(['google.com', 'facebook.com', 'phone']); userRecord = await admin.auth().updateUser(updateUser.uid, { - providersToDelete: ['google.com', 'facebook.com', 'phone'], + providersToUnlink: ['google.com', 'facebook.com', 'phone'], }); providerUids = userRecord.providerData.map((userInfo) => userInfo.uid); @@ -668,11 +668,11 @@ describe('admin.auth', () => { expect(providerIds).to.not.deep.include.members(['google.com', 'facebook.com', 'phone']); }); - it('noops successfully when given an empty providersToDelete list', async () => { + it('noops successfully when given an empty providersToUnlink list', async () => { const userRecord = await createTestUser('NoopWithEmptyProvidersToDeleteUser'); try { const updatedUserRecord = await admin.auth().updateUser(userRecord.uid, { - providersToDelete: [], + providersToUnlink: [], }); expect(updatedUserRecord).to.deep.equal(userRecord); diff --git a/test/unit/auth/auth.spec.ts b/test/unit/auth/auth.spec.ts index b0e58d6f49..1ba752c614 100644 --- a/test/unit/auth/auth.spec.ts +++ b/test/unit/auth/auth.spec.ts @@ -1617,13 +1617,13 @@ AUTH_CONFIGS.forEach((testConfig) => { + JSON.stringify(invalidProviderId), () => { expect(() => { auth.updateUser(uid, { - providersToDelete: [ invalidProviderId as any ], + providersToUnlink: [ invalidProviderId as any ], }); }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); }); }); - it('should merge deletion of phone provider with the providersToDelete list', async () => { + it('should merge deletion of phone provider with the providersToUnlink list', async () => { const invokeRequestHandlerStub = sinon.stub(testConfig.RequestHandler.prototype, 'invokeRequestHandler') .resolves({ localId: uid, @@ -1639,7 +1639,7 @@ AUTH_CONFIGS.forEach((testConfig) => { await auth.updateUser(uid, { phoneNumber: null, - providersToDelete: [ 'google.com' ], + providersToUnlink: [ 'google.com' ], }); expect(invokeRequestHandlerStub).to.have.been.calledOnce.and.calledWith( @@ -1727,11 +1727,11 @@ AUTH_CONFIGS.forEach((testConfig) => { }); }); - it('specifying both phoneNumber=null and providersToDelete=phone should be rejected', () => { + it('specifying both phoneNumber=null and providersToUnlink=phone should be rejected', () => { expect(() => { auth.updateUser(uid, { phoneNumber: null, - providersToDelete: ['phone'], + providersToUnlink: ['phone'], }); }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error'); }); From 3393af3dba4db579aac7c97b6956525858421435 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 22 Dec 2020 10:00:04 -0500 Subject: [PATCH 05/10] add todo re refactoring --- src/auth/auth-api-request.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/auth/auth-api-request.ts b/src/auth/auth-api-request.ts index c739e7007f..e94149c94b 100644 --- a/src/auth/auth-api-request.ts +++ b/src/auth/auth-api-request.ts @@ -1300,6 +1300,8 @@ export abstract class AbstractAuthRequestHandler { ), ); } else if (validator.isNonNullObject(properties.providerToLink)) { + // TODO(rsgowman): These checks overlap somewhat with + // validateProviderUserInfo. It may be possible to refactor a bit. if (!validator.isNonEmptyString(properties.providerToLink.providerId)) { throw new FirebaseAuthError( AuthClientErrorCode.INVALID_ARGUMENT, From a3f8f69e03bc79df7f7e212d58880fe46189c8aa Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Thu, 4 Feb 2021 13:48:15 -0500 Subject: [PATCH 06/10] api-extractor --- etc/firebase-admin.api.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/etc/firebase-admin.api.md b/etc/firebase-admin.api.md index b5b810a5e6..e7064c47ea 100644 --- a/etc/firebase-admin.api.md +++ b/etc/firebase-admin.api.md @@ -250,6 +250,7 @@ export namespace auth { expiresIn: number; } export interface Tenant { + anonymousSignInEnabled?: boolean; displayName?: string; emailSignInConfig?: { enabled: boolean; @@ -300,6 +301,7 @@ export namespace auth { photoURL?: string | null; } export interface UpdateTenantRequest { + anonymousSignInEnabled?: boolean; displayName?: string; emailSignInConfig?: EmailSignInProviderConfig; multiFactorConfig?: MultiFactorConfig; From af5d012e6663c2e0ee44e3d951c0671a16966986 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Thu, 4 Feb 2021 13:54:40 -0500 Subject: [PATCH 07/10] api-extractor in the right branch. :) --- etc/firebase-admin.api.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/etc/firebase-admin.api.md b/etc/firebase-admin.api.md index e7064c47ea..c11e31e0d1 100644 --- a/etc/firebase-admin.api.md +++ b/etc/firebase-admin.api.md @@ -250,7 +250,6 @@ export namespace auth { expiresIn: number; } export interface Tenant { - anonymousSignInEnabled?: boolean; displayName?: string; emailSignInConfig?: { enabled: boolean; @@ -299,9 +298,10 @@ export namespace auth { password?: string; phoneNumber?: string | null; photoURL?: string | null; + providersToUnlink?: string[]; + providerToLink?: UserProvider; } export interface UpdateTenantRequest { - anonymousSignInEnabled?: boolean; displayName?: string; emailSignInConfig?: EmailSignInProviderConfig; multiFactorConfig?: MultiFactorConfig; @@ -364,6 +364,14 @@ export namespace auth { creationTime?: string; lastSignInTime?: string; } + export interface UserProvider { + displayName?: string; + email?: string; + phoneNumber?: string; + photoURL?: string; + providerId?: string; + uid?: string; + } export interface UserProviderRequest { displayName?: string; email?: string; @@ -392,6 +400,7 @@ export namespace auth { tokensValidAfterTime?: string; uid: string; } + {}; } // @public (undocumented) From 37bd4e3c7921cb828562be5122b03daac1ef77ff Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Mon, 8 Feb 2021 15:48:55 -0500 Subject: [PATCH 08/10] review feedback --- etc/firebase-admin.api.md | 1 - src/auth/auth.ts | 5 +++-- test/unit/auth/auth.spec.ts | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/etc/firebase-admin.api.md b/etc/firebase-admin.api.md index c11e31e0d1..19a2f30a2e 100644 --- a/etc/firebase-admin.api.md +++ b/etc/firebase-admin.api.md @@ -400,7 +400,6 @@ export namespace auth { tokensValidAfterTime?: string; uid: string; } - {}; } // @public (undocumented) diff --git a/src/auth/auth.ts b/src/auth/auth.ts index c4fa0d5e3c..2432c44fae 100644 --- a/src/auth/auth.ts +++ b/src/auth/auth.ts @@ -359,7 +359,8 @@ export class BaseAuth implements BaseAuthI // but also provides a providerToLink with a 'phone' provider id. In that // case, we'll throw an error. properties = deepCopy(properties); - if (validator.isNonNullObject(properties) && typeof properties.providerToLink !== 'undefined') { + + if (properties?.providerToLink) { if (properties.providerToLink.providerId === 'email') { if (typeof properties.email !== 'undefined') { throw new FirebaseAuthError( @@ -380,7 +381,7 @@ export class BaseAuth implements BaseAuthI delete properties.providerToLink; } } - if (validator.isNonNullObject(properties) && typeof properties.providersToUnlink !== 'undefined') { + if (properties?.providersToUnlink) { if (properties.providersToUnlink.indexOf('phone') !== -1) { // If we've been told to unlink the phone provider both via setting // phoneNumber to null *and* by setting providersToUnlink to include diff --git a/test/unit/auth/auth.spec.ts b/test/unit/auth/auth.spec.ts index 0332ce6a89..c17efe67c2 100644 --- a/test/unit/auth/auth.spec.ts +++ b/test/unit/auth/auth.spec.ts @@ -1674,7 +1674,6 @@ AUTH_CONFIGS.forEach((testConfig) => { getAccountInfoByUidStub.restore(); }); - it('specifying both email and providerId=email should be rejected', () => { expect(() => { auth.updateUser(uid, { From 21670911c6c717decc98ba9b3a57e9dbac31b11c Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 9 Feb 2021 15:43:59 -0500 Subject: [PATCH 09/10] add back api-extractor's magic {} --- etc/firebase-admin.api.md | 1 + 1 file changed, 1 insertion(+) diff --git a/etc/firebase-admin.api.md b/etc/firebase-admin.api.md index cd8260d351..9cbece07df 100644 --- a/etc/firebase-admin.api.md +++ b/etc/firebase-admin.api.md @@ -401,6 +401,7 @@ export namespace auth { tokensValidAfterTime?: string; uid: string; } + {}; } // @public (undocumented) From 768611305baced20e8a4d32ced2c4790e556e989 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 9 Feb 2021 17:38:31 -0500 Subject: [PATCH 10/10] doc nit --- src/auth/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/auth/index.ts b/src/auth/index.ts index fa32ad7cd3..7817435c14 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -427,7 +427,7 @@ export namespace auth { * Linking a provider to an existing user account does not invalidate the * refresh token of that account. In other words, the existing account * would continue to be able to access resources, despite not having used - * the newly linked provider to login. If you wish to force the user to + * the newly linked provider to log in. If you wish to force the user to * authenticate with this new provider, you need to (a) revoke their * refresh token (see * https://firebase.google.com/docs/auth/admin/manage-sessions#revoke_refresh_tokens),