Skip to content

feat(auth, multi-tenant): add multi-tenant (tenantID) support #5019

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

Merged
merged 5 commits into from
Mar 12, 2021

Conversation

xianlinbox
Copy link
Contributor

Description

When I working on the firebase auth, I found I can't set tenant id which is quite critical for working with Google Identity platform. In the repository discussion channel. there are 2 questions related with this also.

  1. Set tenantID on auth() #4462
  2. Multi Tenant Support #4964

Then I checked the Firebase iOS SDK, which actually provided tenantId for user to set it. So I created this PR to open this capability to React native Lib users.

Related issues

No

Release Summary

Support set tenant it in Firebase auth module.

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

yarn tests:jest

Test Suites: 8 passed, 8 total
Tests: 1 skipped, 95 passed, 96 total
Snapshots: 0 total
Time: 20.6 s


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Mar 12, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/FBEZQKXga5YfFB4izy4a4ZszyVvY
✅ Preview: https://react-native-f-git-fork-xianlinbox-set-tenant-for-auth-i-991157.vercel.app

@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #5019 (326942e) into master (d2b0074) will decrease coverage by 0.01%.
The diff coverage is 85.72%.

@@            Coverage Diff             @@
##           master    #5019      +/-   ##
==========================================
- Coverage   89.09%   89.09%   -0.00%     
==========================================
  Files         109      109              
  Lines        3721     3728       +7     
  Branches      348      350       +2     
==========================================
+ Hits         3315     3321       +6     
  Misses        365      365              
- Partials       41       42       +1     

@xianlinbox
Copy link
Contributor Author

@mikehardy here is the new PR with my correct email, I will sign CLA here. Thanks very much for reviewing the code.

@Ehesp
Copy link
Member

Ehesp commented Mar 12, 2021

Awesome thanks for this.

@mikehardy
Copy link
Collaborator

Thank you! We still have some underlying CI issues which are very new - just last couple days - I have hopes of triaging those so this is fully green but there should be nothing wrong with this PR as it is, I'm planning on merging it - cheers @xianlinbox

@mikehardy mikehardy added tools: ci Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Pending Merge Waiting on CI or similar labels Mar 12, 2021
@mikehardy mikehardy self-assigned this Mar 12, 2021
xianlinbox and others added 5 commits March 12, 2021 15:35
without this, the xcodebuild commands could fail and you would still
get a clean exit code, so CI (for example) would pass even though the
iOS build failed
this exposed most of the errors fixed in prior commits, and allows you
to see that setTenantId is working, but cannot be used against the emulator
and we have no valid tenant id in our test project so it is unknown if this
is truly working
@mikehardy
Copy link
Collaborator

There was an iOS compile error inadvertently hidden by the way we were calling xcodebuild

I just added these changes and pushed to your branch:

  • rebased to current master
  • fixed the incorrect use of unix pipes that hid compile errors
  • fixed the compile error
  • exposed the user.tenantId property for those that use multi-tenant
  • added a simple E2E test that will always run, and some commented out ones that implementors may play with if they have access to a live firebase project with multi-tenant setup (we do not)

If it passes CI I think it will be good to go

@mikehardy mikehardy removed tools: ci Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Pending Merge Waiting on CI or similar labels Mar 12, 2021
@mikehardy mikehardy merged commit 5ba603c into invertase:master Mar 12, 2021
@mikehardy
Copy link
Collaborator

This is out as of v11.1.0 - please test and if there's anything wrong let us know - cheers!

@kwong70
Copy link

kwong70 commented Jun 5, 2024

Do we need to manually set the tenantId or is it auto-populated when a user signs in?

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

Successfully merging this pull request may close these issues.

4 participants