Skip to content
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): Add ability to link a federated ID with the updateUser() method. #770

Merged
merged 14 commits into from
Feb 9, 2021
Merged
11 changes: 11 additions & 0 deletions etc/firebase-admin.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ export namespace auth {
password?: string;
phoneNumber?: string | null;
photoURL?: string | null;
providersToUnlink?: string[];
providerToLink?: UserProvider;
}
export interface UpdateTenantRequest {
anonymousSignInEnabled?: boolean;
Expand Down Expand Up @@ -365,6 +367,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;
Expand Down Expand Up @@ -393,6 +403,7 @@ export namespace auth {
tokensValidAfterTime?: string;
uid: string;
}
{};
hiranya911 marked this conversation as resolved.
Show resolved Hide resolved
}

// @public (undocumented)
Expand Down
57 changes: 52 additions & 5 deletions src/auth/auth-api-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,8 @@ function validateCreateEditRequest(request: any, writeOperationType: WriteOperat
phoneNumber: true,
customAttributes: true,
validSince: true,
// Pass linkProviderUserInfo only for updates (i.e. not for uploads.)
linkProviderUserInfo: !uploadAccountRequest,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add deleteProvider 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.

It's already present (line 263) since it was used previously to remove phone auth entries.

// Pass tenantId only for uploadAccount requests.
tenantId: uploadAccountRequest,
passwordHash: uploadAccountRequest,
Expand Down Expand Up @@ -551,6 +553,12 @@ function validateCreateEditRequest(request: any, writeOperationType: WriteOperat
validateProviderUserInfo(providerUserInfoEntry);
});
}

// linkProviderUserInfo must be a (single) UserProvider value.
if (typeof request.linkProviderUserInfo !== 'undefined') {
validateProviderUserInfo(request.linkProviderUserInfo);
}

// mfaInfo is used for importUsers.
// mfa.enrollments is used for setAccountInfo.
// enrollments has to be an array of valid AuthFactorInfo requests.
Expand Down Expand Up @@ -1306,6 +1314,33 @@ export abstract class AbstractAuthRequestHandler {
'Properties argument must be a non-null object.',
),
);
} else if (validator.isNonNullObject(properties.providerToLink)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

// 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,
'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.providersToUnlink !== 'undefined') {
if (!validator.isArray(properties.providersToUnlink)) {
throw new FirebaseAuthError(
AuthClientErrorCode.INVALID_ARGUMENT,
'providersToUnlink of properties argument must be an array of strings.');
}

properties.providersToUnlink.forEach((providerId) => {
if (!validator.isNonEmptyString(providerId)) {
throw new FirebaseAuthError(
AuthClientErrorCode.INVALID_ARGUMENT,
'providersToUnlink of properties argument must be an array of strings.');
}
});
}

// Build the setAccountInfo request.
Expand Down Expand Up @@ -1340,13 +1375,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.providersToUnlink) !== 'undefined') {
if (!validator.isArray(request.deleteProvider)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this check redundant, considering the condition gets checked at validation?

Copy link
Member Author

Choose a reason for hiding this comment

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

deleteProvider might not yet exist. (Or technically, the end user could set it to anything they like if they're using javascript rather than typescript.)

Copy link
Contributor

Choose a reason for hiding this comment

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

This will achieve the same result without the if condition I think:

request.deleteProvider = (request.deleteProvider || []).concat(request.providersToUnlink);

Copy link
Member Author

Choose a reason for hiding this comment

The 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 deleteProvider doesn't show up in the UpdateRequest interface... though that doesn't stop the user from setting it anyways for their own purposes. (Although we modify it here, we also take a copy first, so this won't impact the user's copy.) Given that, it seems easiest to just check to see if it's an array first.

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.providersToUnlink);
delete request.providersToUnlink;
}

// Rewrite photoURL to photoUrl.
Expand Down
45 changes: 45 additions & 0 deletions src/auth/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

import { deepCopy } from '../utils/deep-copy';
import { UserRecord } from './user-record';
import {
isUidIdentifier, isEmailIdentifier, isPhoneIdentifier, isProviderIdentifier,
Expand Down Expand Up @@ -381,6 +382,50 @@ export class BaseAuth<T extends AbstractAuthRequestHandler> implements BaseAuthI
* @return {Promise<UserRecord>} A promise that resolves with the modified user record.
*/
public updateUser(uid: string, properties: UpdateRequest): Promise<UserRecord> {
// 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 (properties?.providerToLink) {
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 (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
// '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.providersToUnlink=['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.
Expand Down
56 changes: 56 additions & 0 deletions src/auth/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,42 @@ export namespace auth {
phoneNumber: string;
}

/**
* 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.
*/
Expand Down Expand Up @@ -384,6 +420,26 @@ export namespace auth {
* The user's updated multi-factor related properties.
*/
multiFactor?: MultiFactorUpdateSettings;

/**
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

As a verb or infinitive, this is two words: "log in"

* 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;

/**
* Unlinks this user from the specified providers.
*/
providersToUnlink?: string[];
}

/**
Expand Down
Loading