feat(auth): include phoneNumber from PhoneMultiFactorInfo #7565

mnahkies · 2024-01-14T09:51:48Z

Description

Expose phoneNumber on the MFA hints to JS.

MultiFactorInfo has two sub-classes in the underlying SDK's - one for TOTP and one for Phone, where the Phone one additionally includes the phoneNumber registered to the factor.

Before login the phoneNumber is redacted, and after login it is unredacted.

This is useful for showing the user a hint as to which phone number they should check, or distinguishing between multiple phone factors.

Additionally rename enrollmentDate -> enrollmentTime on iOS to match Android/Web (ref: https://firebase.google.com/docs/reference/js/auth.multifactorinfo)

If we'd rather avoid the breaking change here, perhaps we could keep enrollmentDate as deprecated on iOS?

ref:

Release Summary

Expose phoneNumber on PhoneMultiFactorInfo
Rename enrollmentDate to enrollmentTime on iOS to match Android/Web

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
- Yes
My change supports the following platforms;
- Android
- iOS
My change includes tests;
- e2e tests added or updated in packages/\*\*/e2e
- jest tests added or updated in packages/\*\*/__tests__
I have updated TypeScript types that are affected by my change.
This is a breaking change;
- Yes
- [] No

Test Plan

Tested on Android emulator, observed:

Redacted phoneNumber included in hint on the login challenge
Unredacted phoneNumber included in hint after login (eg: showing a manage mfa UI)

NOT tested on iOS - ideally could do with some help here.

🔥

vercel · 2024-01-14T09:51:53Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-native-firebase	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 20, 2024 1:59am
react-native-firebase-next	❌ Failed (Inspect)			May 20, 2024 1:59am

CLAassistant · 2024-01-14T09:51:54Z

All committers have signed the CLA.

packages/auth/ios/RNFBAuth/RNFBAuthModule.m

mnahkies · 2024-01-14T09:58:26Z

Vercel – react-native-firebase-next — Deployment has failed

I don't seem to be able to access the logs for this 😞

packages/auth/ios/RNFBAuth/RNFBAuthModule.m

packages/auth/lib/index.d.ts

mikehardy

This looks pretty good - I left a few comments in form of code suggestions which could be adopted and this could merge, or if you've got thoughts + counter-proposals I'm happy to hear them

in general though I like the idea of backwards-compatibility to avoid the break, and following the same null-check idiom in the iOS code

packages/auth/ios/RNFBAuth/RNFBAuthModule.m

packages/auth/lib/index.d.ts

github-actions · 2024-02-21T19:35:56Z

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

mikehardy · 2024-02-21T19:58:47Z

Still very interested in getting this one in - just had a couple more things that were high priority slice into my queue in front of it - apologies

rename enrollmentDate -> enrollmentTime on iOS to match Android/Web (ref: https://firebase.google.com/docs/reference/js/auth.multifactorinfo) ref: - https://github.com/invertase/react-native-firebase/blob/main/packages/auth/android/src/main/java/io/invertase/firebase/auth/ReactNativeFirebaseAuthModule.java#L2500 - https://github.com/invertase/react-native-firebase/blob/main/packages/auth/lib/index.d.ts#L483 BREAKING CHANGE: enrollmentDate renamed to enrollmentTime on iOS split from invertase#7565

rename `enrollmentDate` -> `enrollmentTime` on iOS to match Android/Web (ref: https://firebase.google.com/docs/reference/js/auth.multifactorinfo) keep `enrollmentDate` around on ios for backwards compatibility, mark as deprecated. ref: - https://github.com/invertase/react-native-firebase/blob/main/packages/auth/android/src/main/java/io/invertase/firebase/auth/ReactNativeFirebaseAuthModule.java#L2500 - https://github.com/invertase/react-native-firebase/blob/main/packages/auth/lib/index.d.ts#L483 split from invertase#7565

github-actions · 2024-03-25T22:36:05Z

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

codecov · 2024-04-11T12:23:09Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 33.36%. Comparing base (a2e3206) to head (3e397a4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7565      +/-   ##
==========================================
+ Coverage   32.97%   33.36%   +0.39%     
==========================================
  Files         252      252              
  Lines       12530    12533       +3     
  Branches     1954     1956       +2     
==========================================
+ Hits         4131     4180      +49     
+ Misses       8305     8264      -41     
+ Partials       94       89       -5

…nrollmentTime` (#7653) rename `enrollmentDate` -> `enrollmentTime` on iOS to match Android/Web (ref: https://firebase.google.com/docs/reference/js/auth.multifactorinfo) keep `enrollmentDate` around on ios for backwards compatibility, mark as deprecated. ref: - https://github.com/invertase/react-native-firebase/blob/main/packages/auth/android/src/main/java/io/invertase/firebase/auth/ReactNativeFirebaseAuthModule.java#L2500 - https://github.com/invertase/react-native-firebase/blob/main/packages/auth/lib/index.d.ts#L483 split from #7565

mikehardy

I think if you add the nil check on phone number, allow it to return null in the JS-level typing, and remove the enrollmentTime/Date change (which I just merged separately) this can go in? I think that would satisfy all the comments here

Again, thanks for the patience, we're moving a bit now...

mikehardy · 2024-05-14T11:35:46Z

Oh of course the patch set generation fails this time because of some new CI features 🤦 - need to fix that, then rebase and re-run it, will be a bit, sorry

mikehardy · 2024-05-14T21:21:00Z

@yuvalhermelin-fijoya okay, fixed the patchset generation it will show up as an attachment here: https://github.com/invertase/react-native-firebase/actions/runs/9086347168?pr=7565

The auth one is the only one you are interested in, but the patchset contains all merged changes since last public release + this PR, so all of them are valid.

I don't anticipate that any of the code here will change, the holdup now is test process

mikehardy · 2024-05-17T20:47:11Z

I just fixed a major chunk of our test process issues with #7797
One more major chunk to truly verify this change - but I will likely defer the most difficult part of that change in it to get this through

mikehardy

This is looking good now - on course to land tonight and release, last bit of testing (enabling MFA tests for iOS) I'm going to defer as it implies a big shift to how we work since it's an auth-emulator vs cloud-auth thing

mikehardy · 2024-05-20T00:56:59Z

still need to merge the test tweak from #7565 (comment) then this can go

this is useful for indicating to users which phone number they have enrolled, and distinguishing between factors if there are multiple phone factors enrolled.

…n to match iOS WIP needs test

behavior should mirror what android native MFA does with this change

yuvalhermelin-fijoya · 2024-05-20T07:09:38Z

Thanks guys!

vercel bot had a problem deploying to Preview – react-native-firebase-next January 14, 2024 09:51 Failure

mnahkies commented Jan 14, 2024

View reviewed changes

packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved

vercel bot deployed to Preview – react-native-firebase January 14, 2024 09:55 View deployment

mnahkies marked this pull request as ready for review January 14, 2024 09:57

mnahkies commented Jan 14, 2024

View reviewed changes

packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved

mnahkies commented Jan 14, 2024

View reviewed changes

packages/auth/lib/index.d.ts Show resolved Hide resolved

mikehardy requested changes Jan 23, 2024

View reviewed changes

packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved

packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved

packages/auth/lib/index.d.ts Show resolved Hide resolved

mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. plugin: authentication Firebase Authentication labels Jan 23, 2024

mikehardy changed the title ~~feat!: include phoneNumber from PhoneMultiFactorInfo / enrollmentDate -> enrollmentTime~~ feat(auth): include phoneNumber from PhoneMultiFactorInfo Jan 23, 2024

mikehardy mentioned this pull request Feb 16, 2024

[🐛] 🔥 enrolledFactors not accessible on MultiFactorUser #7189

Closed

10 tasks

github-actions bot added the Stale label Feb 21, 2024

github-actions bot removed the Stale label Feb 21, 2024

mnahkies mentioned this pull request Feb 26, 2024

fix: ios enrollmentDate -> enrollmentTime #7653

Merged

8 tasks

github-actions bot added the Stale label Mar 25, 2024

github-actions bot closed this Apr 9, 2024

mikehardy reopened this Apr 11, 2024

github-actions bot removed the Stale label Apr 11, 2024

mikehardy requested changes Apr 12, 2024

View reviewed changes

mnahkies force-pushed the mn/feat/marshall-phone-number branch from 326e070 to 993c696 Compare April 20, 2024 10:26

vercel bot deployed to Preview May 14, 2024 11:41 View deployment

vercel bot deployed to Preview May 14, 2024 20:44 View deployment

mikehardy force-pushed the mn/feat/marshall-phone-number branch 2 times, most recently from d6fbbe8 to d3578b6 Compare May 14, 2024 20:48

vercel bot deployed to Preview May 14, 2024 20:54 View deployment

vercel bot deployed to Preview May 14, 2024 20:59 View deployment

mikehardy mentioned this pull request May 14, 2024

build(ci): fix patch set generation to handle modern react-native / yarn #7792

Merged

8 tasks

mikehardy force-pushed the mn/feat/marshall-phone-number branch from 4e8a1ff to 8792280 Compare May 14, 2024 21:19

vercel bot deployed to Preview May 14, 2024 21:26 View deployment

mikehardy force-pushed the mn/feat/marshall-phone-number branch from 8792280 to 34f4379 Compare May 20, 2024 00:14

mikehardy removed the Workflow: Waiting for User Response Blocked waiting for user response. label May 20, 2024

vercel bot deployed to Preview May 20, 2024 00:21 View deployment

mikehardy previously approved these changes May 20, 2024

View reviewed changes

mikehardy dismissed their stale review via 5d87679 May 20, 2024 01:47

mikehardy force-pushed the mn/feat/marshall-phone-number branch from 34f4379 to 5d87679 Compare May 20, 2024 01:47

vercel bot deployed to Preview May 20, 2024 01:52 View deployment

mnahkies and others added 5 commits May 19, 2024 20:55

feat: include phoneNumber from PhoneMultiFactorInfo

7656a9c

this is useful for indicating to users which phone number they have enrolled, and distinguishing between factors if there are multiple phone factors enrolled.

fix(auth, android): change error code for invalid multi-factor sessio…

938f2b1

…n to match iOS WIP needs test

chore(auth, ios): fix DLog formatting

a9b1fcc

chore(auth, ios): add dev logging in multi-factor area

b815ea3

fix(auth, ios): reject multi-factor API call if session not found

3e397a4

behavior should mirror what android native MFA does with this change

mikehardy force-pushed the mn/feat/marshall-phone-number branch from 5d87679 to 3e397a4 Compare May 20, 2024 01:55

vercel bot deployed to Preview May 20, 2024 01:59 View deployment

mikehardy merged commit 3d61e32 into invertase:main May 20, 2024
15 of 16 checks passed

mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): include phoneNumber from PhoneMultiFactorInfo #7565

feat(auth): include phoneNumber from PhoneMultiFactorInfo #7565

mnahkies commented Jan 14, 2024 •

edited

Loading

vercel bot commented Jan 14, 2024 •

edited

Loading

CLAassistant commented Jan 14, 2024 •

edited

Loading

mnahkies commented Jan 14, 2024

mikehardy left a comment

github-actions bot commented Feb 21, 2024

mikehardy commented Feb 21, 2024

github-actions bot commented Mar 25, 2024

codecov bot commented Apr 11, 2024 •

edited

Loading

mikehardy left a comment

mikehardy commented May 14, 2024

mikehardy commented May 14, 2024

mikehardy commented May 17, 2024

mikehardy left a comment

mikehardy commented May 20, 2024

yuvalhermelin-fijoya commented May 20, 2024

feat(auth): include phoneNumber from PhoneMultiFactorInfo #7565

feat(auth): include phoneNumber from PhoneMultiFactorInfo #7565

Conversation

mnahkies commented Jan 14, 2024 • edited Loading

Description

Release Summary

Checklist

Test Plan

vercel bot commented Jan 14, 2024 • edited Loading

CLAassistant commented Jan 14, 2024 • edited Loading

mnahkies commented Jan 14, 2024

mikehardy left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024

mikehardy commented Feb 21, 2024

github-actions bot commented Mar 25, 2024

codecov bot commented Apr 11, 2024 • edited Loading

Codecov Report

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented May 14, 2024

mikehardy commented May 14, 2024

mikehardy commented May 17, 2024

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented May 20, 2024

yuvalhermelin-fijoya commented May 20, 2024

mnahkies commented Jan 14, 2024 •

edited

Loading

vercel bot commented Jan 14, 2024 •

edited

Loading

CLAassistant commented Jan 14, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading