Populates UserRecord with multi-factor related info #681
Conversation
… GetAccountInfo server response. First step in supporting multi-factor authentication with SMS as a second factor.
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
Looks pretty good. My main concerns are around some of the names. There are just many similar sounding names, and it makes it a little difficult to follow. My primary suggestions are:
- Embed
MfaandMetadatatypes directly intoGetAccountInfoUserResponseand remove them. - Rename
MfaInfotoAuthFactorInfo. - Rename
MultiFactorInfotoAuthFactorandPhoneMultiFactorInfoto justPhoneFactor.
bojeil-google
left a comment
bojeil-google
left a comment
There was a problem hiding this comment.
Thanks for the review and feedback. I made most of the requested changes except some of the public API renames as they have been approved by the API council and already implemented in the client SDKs (currently in alpha phase). It is hard to change it at this point across all platforms.
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
LGTM with a question.
| } else { | ||
| private initFromServerResponse(response: AuthFactorInfo) { | ||
| const factorId = this.getFactorId(response); | ||
| if (!factorId || !response || !response.mfaEnrollmentId) { |
There was a problem hiding this comment.
Should we check response before passing it to getFactorId()?
There was a problem hiding this comment.
I have added a check for this. I also added a test when undefined is passed to the constructor that the expected error is caught and thrown.
2 participants
Updates UserRecord to parse multi-factor related information from the GetAccountInfo server response.
First step in supporting multi-factor authentication with SMS as a second factor.