Repository navigation
fix(auth): support TOTP second factors when updating a user - #3264
Open
rootkiller6788 wants to merge 4 commits into
Open
rootkiller6788 wants to merge 4 commits into
rootkiller6788 wants to merge 4 commits into
Conversation
UpdateMultiFactorInfoRequest only had the phone variant, so there was no way to describe a TOTP factor in an update request at all. Add the missing member and export it, along with the totpInfo response type it refers to.
convertMultiFactorInfoToServerFormat() only knew about phone factors and threw for anything else, so passing a user's existing factors back into updateUser() failed as soon as one of them was TOTP (issue firebase#2995). Handle the totp case and let totpInfo through the request validator. Factors that only carry a secret still error out: the Admin SDK cannot enroll TOTP for a user, and quietly dropping the secret would leave them with a factor their authenticator app can no longer produce codes for.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for TOTP (Time-based One-Time Password) multi-factor authentication (MFA) in the Firebase Auth Admin SDK, allowing TOTP second factors to be validated, updated, and imported. The changes include adding the UpdateTotpMultiFactorInfoRequest interface, updating request validation and conversion logic, and adding corresponding unit tests. Feedback from the reviewer focuses on improving type safety and error handling: specifically, making the uid property required for TOTP updates, validating that mfaEnrollmentId is present during TOTP validation, and throwing a more descriptive error if totpInfo is missing rather than falling back to an unsupported factor error.
Follow-up to the TOTP update support: a TOTP factor is always carried over from a previously enrolled one, so the enrollment ID is strictly required. - UpdateTotpMultiFactorInfoRequest now declares uid as required. - validateAuthFactorInfo() rejects a TOTP factor that carries totpInfo but no mfaEnrollmentId, instead of forwarding it to the server.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2995.
When you update a user that already has TOTP enrolled, you normally do something like this:
because
enrolledFactorsreplaces the whole list. That blows up withauth/unsupported-second-factorthe moment one of those factors is TOTP, so there is no way to touch a TOTP user's factors from the Admin SDK at all.Three places had phone baked in:
UpdateMultiFactorInfoRequestinauth-config.tsonly listed the phone variant (that stray leading|on the union looked like a leftover of a longer list).convertMultiFactorInfoToServerFormat()threw for anything that wasn'tfactorId === 'phone'.validateAuthFactorInfo()treatedtotpInfoas an unknown key, deleted it, and then failed the request as invalid.TOTP factors are just a marker server side (
totpInfo: {}), and the fields the server hands back are the same ones it accepts, so for those it's a plain round trip.One thing I deliberately did not do: accept
{ factorId: 'totp', secret: '...' }. The Admin SDK can't enrol TOTP for a user, and turning a raw secret intototpInfowould leave them with a factor their authenticator app can no longer generate codes for, so those still get the existing error. That's also why the existing TOTP tests didn't need to change.Tests: added a mixed TOTP + phone update, a bad
totpInforejection, and animportUsers()case with a TOTP factor.npm run test:unitis 6069 passing,npm run lintclean. (The MachineLearning "should throw given invalid credential" test is flaky on a loaded machine and times out sometimes; it fails the same way on a clean checkout.)