Adds ability to import users with phone second factors.#699
Adds ability to import users with phone second factors.#699bojeil-google merged 2 commits intoauth-mfafrom
Conversation
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
Looks pretty good. Just a few suggestions for improvement.
src/auth/auth-api-request.ts
Outdated
| ); | ||
| } | ||
| if (typeof request.displayName !== 'undefined' && | ||
| typeof request.displayName !== 'string') { |
src/auth/auth-api-request.ts
Outdated
| }); | ||
| } | ||
| // mfaInfo has to be an array of valid AuthFactorInfo requests. | ||
| if (typeof request.mfaInfo !== 'undefined' && |
There was a problem hiding this comment.
May be this is clearer:
if (request.mfaInfo) {
if (!validator.isArray(request.mfaInfo) {
throw ...
}
request.mfaInfo.forEach...
}
| let enrolledAt; | ||
| if (typeof multiFactorInfo.enrollmentTime !== 'undefined') { | ||
| if (validator.isUTCDateString(multiFactorInfo.enrollmentTime)) { | ||
| enrolledAt = new Date(multiFactorInfo.enrollmentTime).toISOString(); |
There was a problem hiding this comment.
Is there a timezone conversion happening here? Perhaps add a comment.
There was a problem hiding this comment.
Added comment.
| phoneNumber: '+16505557348', | ||
| displayName: 'Spouse\'s phone number', | ||
| factorId: 'phone', | ||
| enrollmentTime: 'invalid', |
There was a problem hiding this comment.
I expected this to throw. How is it getting silently ignored?
There was a problem hiding this comment.
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.
| uid: 'enrolledSecondFactor1', | ||
| secret: 'SECRET', | ||
| displayName: 'Google Authenticator on personal phone', | ||
| factorId: 'totp', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for the quick and helpful review! |
2 participants
WIP: Adds ability to import users with phone second factors.