Cleanup TOTP code to include Firebase User reference in MultiFactorSession. #7346

AngelAngelXie · 2023-06-02T18:39:28Z

Cleanup TOTP code to include Firebase User reference in MultiFactorSession.

Changed a field in MultiFactorSession from AuthInternal to UserInternal
fixed corresponding tests and functions to match the type change.

Ran the code in demo app and was able to complete TOTP enrollment.

…ssion

changeset-bot · 2023-06-02T18:39:31Z

⚠️ No Changeset found

Latest commit: 4ee9d7f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2023-06-02T19:08:10Z

Size Report ¹

Affected Products

`@firebase/auth`

Type	Base (`2ccc9dd`)	Merge (`e7e820e`)	Diff
browser	170 kB	171 kB	+53 B (+0.0%)
cordova	198 kB	198 kB	-5 B (-0.0%)
esm5	223 kB	223 kB	+53 B (+0.0%)
main	167 kB	168 kB	+53 B (+0.0%)
module	170 kB	171 kB	+53 B (+0.0%)
react-native	183 kB	183 kB	-5 B (-0.0%)

@firebase/auth/cordova
Type Base (2ccc9dd) Merge (e7e820e) Diff
browser 198 kB 198 kB -5 B (-0.0%)
module 198 kB 198 kB -5 B (-0.0%)
@firebase/auth/internal
Type Base (2ccc9dd) Merge (e7e820e) Diff
browser 181 kB 181 kB +53 B (+0.0%)
esm5 237 kB 237 kB +53 B (+0.0%)
main 204 kB 204 kB +53 B (+0.0%)
module 181 kB 181 kB +53 B (+0.0%)
@firebase/auth/react-native
Type Base (2ccc9dd) Merge (e7e820e) Diff
browser 183 kB 183 kB -5 B (-0.0%)
module 183 kB 183 kB -5 B (-0.0%)

`firebase`

Type	Base (`2ccc9dd`)	Merge (`e7e820e`)	Diff
firebase-auth-compat.js	132 kB	132 kB	-5 B (-0.0%)
firebase-auth-cordova.js	147 kB	147 kB	-5 B (-0.0%)
firebase-auth-react-native.js	160 kB	160 kB	-5 B (-0.0%)
firebase-auth.js	127 kB	127 kB	+50 B (+0.0%)
firebase-compat.js	773 kB	773 kB	-5 B (-0.0%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/0uKXBPUkPe.html

google-oss-bot · 2023-06-02T19:19:01Z

Size Analysis Report ¹

Affected Products

@firebase/auth
- TotpMultiFactorGenerator
  Size
  Type Base (2ccc9dd) Merge (ce8ffe5) Diff
  size 37.5 kB 37.6 kB +54 B (+0.1%)
  size-with-ext-deps 58.3 kB 58.4 kB +54 B (+0.1%)
- multiFactor
  Size
  Type Base (2ccc9dd) Merge (ce8ffe5) Diff
  size 37.3 kB 37.3 kB -5 B (-0.0%)
  size-with-ext-deps 58.1 kB 58.1 kB -5 B (-0.0%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/xyn2FTowSo.html

packages/auth/src/mfa/assertions/totp.test.ts

prameshj

Thanks for fixing this!

packages/auth/src/mfa/assertions/totp.test.ts

prameshj · 2023-06-02T21:18:52Z

packages/auth/src/mfa/mfa_session.ts

  ) {}

  static _fromIdtoken(
    idToken: string,
-    auth?: AuthInternal
+    user?: UserInternal


I wonder if we can make "user" a required parameter here. The only place this is invoked without the "user" is the fromJSON method -

firebase-js-sdk/packages/auth/src/mfa/mfa_session.ts

Line 81 in 88f010a

return MultiFactorSessionImpl._fromIdtoken(

But I don't see this method being used anywhere. @sam-gc do you know why we have these methods?

I wonder if we can make "user" a required parameter here. The only place this is invoked without the "user" is the fromJSON method -

firebase-js-sdk/packages/auth/src/mfa/mfa_session.ts

Line 81 in 88f010a

return MultiFactorSessionImpl._fromIdtoken(

But I don't see this method being used anywhere. Sam do you know why we have these methods?

toJSON can be invoked when an end user calls JSON.stringify(), but it's unclear where fromJSON is used.. since JSON.parse won't automatically call it and it is not in the expected reviver format - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#using_the_reviver_parameter

But i think it is ok to leave it be, in case users are manually calling this function somewhere.

packages/auth/src/platform_browser/mfa/assertions/phone.test.ts

prameshj

Please update the PR description to include any manual testing results with the demo app.

renkelvin

Thanks, looks good to me. Please update the PR description to provide a bit more details.

prameshj

LGTM, except for the one comment about user param in _fromIdToken.

Let's also verify this in the demo app to make sure it works end to end. Thanks!

…ssion. (#7346) * cleanup TOTP code to include Firebase user reference in MultiFactorSession * removed helper comments * Added fakeUid const to organize code * edited test to match changes in MultiFactorSession * resolved TestAuth incompatible type error * resolved more TestAuth incompatible type error * resubmitting * fixed formatting & resolved issue with undefined * fixed format issue * fixed formatting * tried to resolve issue with user being undefined * fixed if statement formatting * addressed feedback: changed expect to compare user * fixed formatting * fixed the issue with undefined * instantiate user * organized import statements * changed import formatting * resubmitting for import formatting issue * ran yarn format

AngelAngelXie added 12 commits June 1, 2023 18:53

cleanup TOTP code to include Firebase user reference in MultiFactorSe…

bd8d738

…ssion

removed helper comments

39abaa7

Added fakeUid const to organize code

e375934

edited test to match changes in MultiFactorSession

69534c7

resolved TestAuth incompatible type error

3109463

resolved more TestAuth incompatible type error

c451871

resubmitting

b505093

fixed formatting & resolved issue with undefined

262e7d1

fixed format issue

1aca301

fixed formatting

48d281a

tried to resolve issue with user being undefined

17da4cf

fixed if statement formatting

88f010a

AngelAngelXie requested review from lisajian, prameshj, renkelvin, sam-gc and a team as code owners June 2, 2023 18:39

renkelvin reviewed Jun 2, 2023

View reviewed changes

packages/auth/src/mfa/assertions/totp.test.ts Outdated Show resolved Hide resolved

packages/auth/src/mfa/assertions/totp.test.ts Outdated Show resolved Hide resolved

prameshj reviewed Jun 2, 2023

View reviewed changes

AngelAngelXie added 7 commits June 2, 2023 22:46

addressed feedback: changed expect to compare user

b5e9cad

fixed formatting

adb34a7

fixed the issue with undefined

a18bb1b

instantiate user

af8ea3f

organized import statements

6379962

changed import formatting

1b7b274

resubmitting for import formatting issue

3d39898

ran yarn format

4ee9d7f

This comment was marked as duplicate.

Sign in to view

renkelvin approved these changes Jun 5, 2023

View reviewed changes

prameshj reviewed Jun 6, 2023

View reviewed changes

AngelAngelXie changed the title ~~Firebase cleanup ax~~ Cleanup TOTP code to include Firebase User reference in MultiFactorSession. Jun 7, 2023

AngelAngelXie merged commit f949ee9 into master Jun 9, 2023

AngelAngelXie deleted the firebase-cleanup-ax branch June 9, 2023 17:30

firebase locked and limited conversation to collaborators Jul 10, 2023

Cleanup TOTP code to include Firebase User reference in MultiFactorSession. #7346

Cleanup TOTP code to include Firebase User reference in MultiFactorSession. #7346

Uh oh!

Conversation

AngelAngelXie commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

google-oss-bot commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

@firebase/auth

@firebase/auth/cordova

@firebase/auth/internal

@firebase/auth/react-native

firebase

Test Logs

Uh oh!

google-oss-bot commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Analysis Report 1

Affected Products

@firebase/auth

TotpMultiFactorGenerator

multiFactor

Test Logs

Uh oh!

Uh oh!

Uh oh!

prameshj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prameshj Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

prameshj Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prameshj left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

renkelvin left a comment

Choose a reason for hiding this comment

Uh oh!

prameshj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AngelAngelXie commented Jun 2, 2023 •

edited

Loading

changeset-bot bot commented Jun 2, 2023 •

edited

Loading

google-oss-bot commented Jun 2, 2023 •

edited

Loading

Size Report ¹

`@firebase/auth`

`@firebase/auth/cordova`

`@firebase/auth/internal`

`@firebase/auth/react-native`

`firebase`

google-oss-bot commented Jun 2, 2023 •

edited

Loading

Size Analysis Report ¹

`@firebase/auth`

`TotpMultiFactorGenerator`

`multiFactor`