FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Adds ability to import users with phone second factors. by bojeil-google · Pull Request #699 · firebase/firebase-admin-node · GitHub

/ firebase-admin-node Public

Adds ability to import users with phone second factors. - #699

Merged
bojeil-google merged 2 commits into
auth-mfafrom
auth-mfa-temp
Nov 14, 2019
ConversationCommits (2)ChecksFiles changed
Merged

Adds ability to import users with phone second factors.#699
bojeil-google merged 2 commits into
auth-mfafrom
auth-mfa-temp

Conversation

bojeil-google commented Nov 9, 2019

Copy link
Copy Markdown
Contributor

WIP: Adds ability to import users with phone second factors.

hiranya911 suggested changes Nov 11, 2019
View reviewed changes

hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks pretty good. Just a few suggestions for improvement.

Comment thread src/auth/auth-api-request.ts Outdated
);
}
if (typeof request.displayName !== 'undefined' &&
typeof request.displayName !== 'string') {

hiranya911 Nov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

!validator.isString()

bojeil-google Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done

Comment thread src/auth/auth-api-request.ts Outdated
});
}
// mfaInfo has to be an array of valid AuthFactorInfo requests.
if (typeof request.mfaInfo !== 'undefined' &&

hiranya911 Nov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

May be this is clearer:

if (request.mfaInfo) {
  if (!validator.isArray(request.mfaInfo) {
    throw ...
  }

  request.mfaInfo.forEach...
}

bojeil-google Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done

Comment thread src/auth/user-import-builder.ts
let enrolledAt;
if (typeof multiFactorInfo.enrollmentTime !== 'undefined') {
if (validator.isUTCDateString(multiFactorInfo.enrollmentTime)) {
enrolledAt = new Date(multiFactorInfo.enrollmentTime).toISOString();

hiranya911 Nov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is there a timezone conversion happening here? Perhaps add a comment.

bojeil-google Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added comment.

Comment thread test/unit/auth/user-import-builder.spec.ts
phoneNumber: '+16505557348',
displayName: 'Spouse\'s phone number',
factorId: 'phone',
enrollmentTime: 'invalid',

hiranya911 Nov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I expected this to throw. How is it getting silently ignored?

bojeil-google Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually it only gets ignored when building the request. The client side errors are cached and will be injected in the response alongside server side response. Check test should return the expected response for import with client side errors. I added a test for this.

Comment thread test/unit/auth/user-import-builder.spec.ts
uid: 'enrolledSecondFactor1',
secret: 'SECRET',
displayName: 'Google Authenticator on personal phone',
factorId: 'totp',

hiranya911 Nov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Expected to throw.

It looks like there's some pre-existing logic in the UserImportBuilder to handle these errors, and ignore them. May be we should at least log a warning when this happens, so a simple typo in the user code doesn't go completely unnoticed.

bojeil-google Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Same here. The client side errors are cached and will be injected in the response alongside server side response. Check test should return the expected response for import with client side errors. I added a test for this too.

hiranya911 approved these changes Nov 14, 2019
View reviewed changes

hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks. LGTM 👍

bojeil-google commented Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the quick and helpful review!

bojeil-google merged commit 5ac091b into auth-mfa Nov 14, 2019
bojeil-google deleted the auth-mfa-temp branch November 18, 2019 07:44
This was referenced Feb 1, 2020
Open
Open
snyk-bot mentioned this pull request Aug 17, 2020
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
hiranya911 hiranya911 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

bojeil-google

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL