-
Notifications
You must be signed in to change notification settings - Fork 248
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 MFA info to UserRecord #422
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jasperkuperus for the PR. It looks mostly good. I've suggested some changes to the public API.
@bojeil-google how do you feel about just releasing the changes to UserRecord
without the support for updating MFA settings?
I think it's fine to do so, as long as we keep track of the missing functionality (updating MFA settings). Ultimately, we want to make sure we have feature parity across all languages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes @jasperkuperus. This is starting to shape up nicely. Just a few more changes needed to get the public API sorted out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jasperkuperus. This LGTM 👍
I do need to get this API approved through our internal API review process. I'll merge when I have the required approvals.
API review process for this has been initiated. Should have some updates by next week. |
This API has been approved with some minor changes. The change being removal of the @jasperkuperus would you like to push the change to this PR? If you're busy I can merge this as-is, and implement the required changes on top of that. |
@hiranya911 It's a small change, but I have a rather busy week in front of me. If you could be so kind to merge it and implement the change on top of that, it'd be of great help, thanks! 😊 |
Discussion
Issue: #421
This PR implements a small part of Multi Factor Authentication (MFA). I do realize this isn't a full implementation, unfortunately I don't have time for that. This PR only implements unmarshalling the MFA info from the REST response into the
UserRecord
struct.For structure and naming within the
UserRecord
, I've decided to be consistent with the Node.js client library, which already has this implemented: https://firebase.google.com/docs/reference/admin/node/admin.auth.UserRecordTesting
testUser
, I've added MFA info. Next to that, I've introduced a test user without MFA. In the list endpoints, I replaced every third user with a non MFA user, so the list tests both users with and without MFA info available. All unit tests should be OK.API Changes
mfaInfo
section. This PR merely adds the code to pick it up into Golang structs.