feat(auth): Add MFA info to UserRecord

jasperkuperus commented 3 years ago

Discussion

Issue: https://github.com/firebase/firebase-admin-go/issues/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.

bojeil-google commented 3 years ago

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.

hiranya911 commented 3 years ago

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

hiranya911 commented 3 years ago

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 commented 3 years ago

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

firebase / firebase-admin-go