-
Notifications
You must be signed in to change notification settings - Fork 406
feat(auth): Add ability to link a federated ID with the updateUser() method.
#770
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
Changes from 1 commit
59d93aa
c79f42d
dfa1fce
fe88272
a04e175
3393af3
ba5361f
a3f8f69
af5d012
37bd4e3
676b4a1
2167091
726cb85
7686113
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 |
|---|---|---|
|
|
@@ -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)) { | ||
|
Contributor
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. Aren't the validations at validateCreateEditRequest sufficient? Seems like the validation is added in two places.
Member
Author
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. (Missed this comment, sorry!) IIUC, the parameters are slightly different, so it doesn't quite work. (eg rawId). Some refactoring could be done though... I've added a TODO. |
||
| 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)) { | ||
|
Contributor
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. Isn't this check redundant, considering the condition gets checked at validation?
Member
Author
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.
Contributor
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. This will achieve the same result without the if condition I think:
Member
Author
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. The user could set this value to anything they like which could interact badly: request.deleteProvider = "foo"
request.providersToUnlink = [ "phone", "email" ]
request.deleteProvider = (request.deleteProvider || []).concat(request.providersToUnlink);
console.log(request.deleteProvider)
> ["foophone", "email"]Such use would be pretty questionable since Thinking about it a bit more though, another alternative would be to just set deleteProvider to undefined (or []) after we take the copy, thus ensuring any value that the user sets can't interfere with this value. I've left it as is for now, though could switch to the alternative if you'd prefer. |
||
| request.deleteProvider = []; | ||
| } | ||
| request.deleteProvider = request.deleteProvider.concat(request.providersToDelete); | ||
| delete request.providersToDelete; | ||
| } | ||
|
|
||
| // Rewrite photoURL to photoUrl. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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[]; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| * '[email protected]'. | ||
| */ | ||
| // TODO(rsgowman): This function could usefully be employed throughout this file. | ||
| function createTestUser(name: string): Promise<admin.auth.UserRecord> { | ||
| 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<void> { | ||
| return admin.auth().getUserByPhoneNumber(phoneNumber) | ||
| .then((userRecord) => { | ||
| return safeDelete(userRecord.uid); | ||
|
|
||
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.
Add deleteProvider as well?
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.
It's already present (line 263) since it was used previously to remove phone auth entries.