Repository navigation
Conversation
The JS SDK's IdTokenResult has signInSecondFactor, but the interop type did not declare it, so the web implementation always returned null.
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds support for the signInSecondFactor property in IdTokenResult for the Firebase Auth Web package, mapping it from the underlying JS object and adding unit tests to verify its behavior. The review feedback points out a violation of the repository's style guide regarding null safety, specifically the use of the non-null assertion operator (!) in the new test file, and suggests using flow analysis instead.
Comment on lines
+38
to
+41
| final user = auth_interop.User.getInstance( | ||
| jsUser as auth_js_interop.UserJsImpl, | ||
| )!; | ||
| return user.getIdTokenResult(false); |
Contributor
There was a problem hiding this comment.
The repository style guide prohibits the use of the ! operator. Please use flow analysis or a null check instead.
final user = auth_interop.User.getInstance(
jsUser as auth_js_interop.UserJsImpl,
);
if (user == null) {
throw StateError('User instance should not be null');
}
return user.getIdTokenResult(false);References
- Null Safety: NO
!operator. Use?and flow analysis (e.g.if (x != null)). (link)
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.
Description
On web,
IdTokenResult.signInSecondFactoris alwaysnull, even after a multi-factor sign-in (for example email and password plus a TOTP code throughMultiFactorResolver.resolveSignIn). The value is present in the token:claims['firebase']['sign_in_second_factor']is'totp'. Android and iOS already return it (PigeonParser.kt, PigeonParser.swift).#17589 added
signInSecondFactorto the platform interface, butfirebase_auth_webnever reads it:IdTokenResultImpldoes not declare the property,IdTokenResultwrapper has no getter for it,convertWebIdTokenResultdoes not pass it toInternalIdTokenResult.This PR declares the property as
JSString?, adds the getter and passes the value through. In the JS SDK the property is typedsignInSecondFactor: string | nulland is set tonullwhen the token has no second factor (@firebase/auth1.13.8, the versionfirebase13.1.0 depends on, whichfirebase_core_webtargets). A session without a second factor therefore still returnsnull, as on Android and iOS.The new browser test feeds a JS object shaped like the SDK's
IdTokenResultthroughUser.getIdTokenResultandconvertWebIdTokenResult, and checks both a'totp'value andnull. Onmainthe first case fails withExpected: 'totp' Actual: <null>.Run locally in
packages/firebase_auth/firebase_auth_web:flutter test --platform=chromeandflutter test --platform=chrome --wasm(all 24 tests pass),dart analyze . --fatal-infosanddart format. I did not run it against a real multi-factor sign-in; the multi-factor e2e tests in the example app are skipped on web.Related Issues
Follow-up to #17589, which added
signInSecondFactorto the platform interface without the web implementation. There is no separate issue for this.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).This will ensure a smooth and quick review process. Updating the
pubspec.yamland changelogs is not required.///).melos run analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?