Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support mfa un-enrollment #7566

Closed
wants to merge 2 commits into from

Conversation

mnahkies
Copy link
Contributor

Description

implements the unenroll method, receiving either a factor uid or factor info, and then calling the underlying unenroll by factor uid overload to avoid marshaling the factor info object.

ref:

Related issues

#7483

Release Summary

Support un-enrolling MFA factors

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:

  • Failure due to needing to re-authenticate raises the correct exception
  • Success path correctly removes the factor

I've not tested that a auth state change event is emitted correctly yet.


🔥

implements the unenroll method, receiving either a factor uid or factor info,
and then calling the underlying unenroll by factor uid overload to avoid
marshalling the factor info object.
Copy link

vercel bot commented Jan 14, 2024

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 Jan 14, 2024 10:15am
react-native-firebase-next ❌ Failed (Inspect) Jan 14, 2024 10:15am

@@ -965,6 +965,25 @@ - (void)invalidate {
}];
}

RCT_EXPORT_METHOD(unenrollMultiFactor
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS implementation is untested, but should be roughly correct. If someone could give it a whirl that would be helpful

@mikehardy
Copy link
Collaborator

Just a note that I really appreciate this work @mnahkies - and I will collaborate with you to get your PRs incorporated.

I'm looking at it with an eye towards supporting TOTP as well, and I noticed that our e2e testing had all the iOS tests disabled which was very worrisome as I'd like to add e2e tests for these methods as well.

So far I haven't been able to get the local firebase auth emulator to work correctly (investigation on this branch: https://github.com/invertase/react-native-firebase/tree/%40mikehardy/auth-mfa-ios-e2e-enable) and my next step is to restore the previous non-emulator approach for auth testing where needed - this is a slightly larger job but the auth emulator apparently doesn't support TOTP with no ETA on support so I think this will be needed anyway.

That work is blocking my review + approval of these items because I want to make sure the MFA auth APIs have good testing.

But they will go in, and if you need them to work right now you may confidently use patch-package and either local patches or the sets that our CI generates, knowing we'll get these through eventually

Thanks again

@mnahkies
Copy link
Contributor Author

@mikehardy no problem - I've been a bit swamped the past few weeks and haven't had a chance to come back to these yet either, but do plan to polish them up still. I've also got appetite to implement TOTP support, but just limited on time right now

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants