Skip to content

Adds multi-factor support for updateUser #706

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

Merged
merged 2 commits into from
Nov 19, 2019
Merged

Conversation

bojeil-google
Copy link
Contributor

Adds multi-factor support for updateUser
This will allow the ability to update the list of second factors on an existing user record.

Adds related unit and integration tests for this operation.

This will allow the ability to update the list of second factors on an existing user record.

Adds related unit and integration tests for this operation.
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks great. Just a couple of nits and questions.

@@ -51,6 +51,15 @@ export interface UpdateRequest {
password?: string;
phoneNumber?: string | null;
photoURL?: string | null;
multiFactor?: {
enrolledFactors: Array<{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the SecondFactor type as used in UserImportBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a local SecondFactor type. Didn't want to risk getting in a possible circular dependency situation.

@@ -1049,6 +1059,28 @@ export abstract class AbstractAuthRequestHandler {
request.disableUser = request.disabled;
delete request.disabled;
}
// Construct mfa related user data.
if (validator.isNonNullObject(request.multiFactor)) {
if (request.multiFactor.enrolledFactors === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this is slightly more clear:

request.mfa = {};
const enrollments = [];
if (validator.isArray(request.multiFactor.enrolledFactors)) {
   // iterate and push
}

if (enrollments.length > 0) {
  request.mfa.enrollments = enrollments;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm it is a bit more complicated as passing request.mfa = {} will clear all existing second factors on the user.
We should only pass that if the developers passing explicitly an empty array for enrolledFactors or null for enrolledFactors.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM with a question.

@@ -1000,6 +1020,16 @@ declare namespace admin.auth {
* to the tenant corresponding to that `TenantAwareAuth` instance's tenant ID.
*/
tenantId?: string | null;

multiFactor?: {
enrolledFactors: Array<{
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to reduce this duplication by introducing an interface? Like the SecondFactor type used in the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think we can define an interface for this. I can do it in the next PR. This is going to be modified and more documentation added later anyway.

@bojeil-google bojeil-google merged commit 70b7da9 into auth-mfa Nov 19, 2019
@bojeil-google bojeil-google deleted the auth-mfa-temp branch November 20, 2019 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants