Skip to content
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

Merged
merged 5 commits into from
Mar 30, 2021

Conversation

jasperkuperus
Copy link
Contributor

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.UserRecord

Testing

  • I've updated the existing tests to also incorporate MFA. On the existing 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.
  • Unfortunately, I wasn't able to get the integration tests running locally, so I'm not sure whether I did break something there. I did manage to test the code in an actual project with a small local setup and verified that it works.

API Changes

  • This PR does not change the public API. The REST API already returns the mfaInfo section. This PR merely adds the code to pick it up into Golang structs.
@jasperkuperus jasperkuperus changed the base branch from master to dev March 9, 2021 10:59
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.

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?

auth/user_mgt.go Show resolved Hide resolved
@bojeil-google
Copy link
Contributor

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.

@jasperkuperus jasperkuperus removed their assignment Mar 16, 2021
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.

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.

auth/user_mgt.go Outdated Show resolved Hide resolved
auth/user_mgt.go Outdated Show resolved Hide resolved
testdata/list_users.json Outdated Show resolved Hide resolved
testdata/get_user.json Outdated Show resolved Hide resolved
auth/user_mgt.go Outdated Show resolved Hide resolved
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.

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.

@hiranya911
Copy link
Contributor

API review process for this has been initiated. Should have some updates by next week.

@hiranya911
Copy link
Contributor

This API has been approved with some minor changes. The change being removal of the MultiFactorID type and the related Phone constant (we will use FactorID string instead).

@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.

@jasperkuperus
Copy link
Contributor Author

jasperkuperus commented Mar 30, 2021

@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! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants