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: multitenancy #677

Merged
merged 190 commits into from
Jul 19, 2023
Merged

feat: multitenancy #677

merged 190 commits into from
Jul 19, 2023

Conversation

alisher-aituarov
Copy link
Contributor

Summary of change

Multitenancy feature

Related issues

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

Documentation changes

(If relevant, please create a PR in our docs repo, or create a checklist here highlighting the necessary changes)

Checklist for important updates

  • Changelog has been updated
  • frontendDriverInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In package.json
    • In package-lock.json
    • In lib/ts/version.ts
  • Had run npm run build-pretty
  • Had installed and ran the pre-commit hook
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.
  • If added a new recipe interface, then make sure that the implementation of it uses NON arrow functions only (like someFunc: function () {..}).
  • If I added a new recipe, I also added the recipe entry point into the size-limit section of package.json with the size limit set to the current size rounded up.
  • If I added a new recipe, I also added the recipe entry point into the rollup.config.mjs

Remaining TODOs for this PR

@porcellus
Copy link
Collaborator

Please manually test/add test cases for combination recipes with disabled sub-recipe:

  • ThirdPartyEmailPassword is the only recipe initialized on the frontend with emailpassword disabled.
  • The tenant has emailpassword enabled (and optionally thirdparty as well)
  • Does login using email and password work in this case?
  • Do something like this for other combinations as well.

@alisher-aituarov
Copy link
Contributor Author

Please manually test/add test cases for combination recipes with disabled sub-recipe:

  • ThirdPartyEmailPassword is the only recipe initialized on the frontend with emailpassword disabled.
  • The tenant has emailpassword enabled (and optionally thirdparty as well)
  • Does login using email and password work in this case?
  • Do something like this for other combinations as well.

only thirdparty is rendered, using emailpassword and passwordless is not possible, i think thats what we need

Comment on lines 83 to 85
passwordless: { ...passwordless, enabled: false },
emailpassword: { ...emailPassword, enabled: true },
thirdparty: { ...thirdParty, enabled: true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems wrong..

Copy link
Collaborator

@porcellus porcellus May 25, 2023

Choose a reason for hiding this comment

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

Isn't this a testing change? It's still there.

lib/ts/recipe/recipeRouter/index.tsx Outdated Show resolved Hide resolved
@@ -41,11 +41,11 @@ const SignInAndUpTheme: React.FC<ThirdPartyEmailPasswordSignInAndUpThemeProps> =
const usesDynamicLoginMethods = SuperTokens.usesDynamicLoginMethods;
const dynamicLoginMethods = Multitenancy.getInstanceOrThrow().getLoadedDynamicLoginMethods();
const thirdPartyEnabled =
(props.config.disableEmailPassword === false && usesDynamicLoginMethods === false) ||
usesDynamicLoginMethods === false ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we not checking the provider count in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean like (usesDynamicLoginMethods === false || providers.length > 0) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

usesDynamicLoginMethods === false && providers.length > 0

SuperTokens.usesDynamicLoginMethods === false &&
config.thirdPartyConfig?.signInAndUpFeature.providers.length === 0
) {
throw new Error("ThirdParty signInAndUpFeature providers array cannot be empty.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be empty (which means the thirdparty section is disabled) if email password is not disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you want me to remove this check ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. This only needs to throw if all of the following is true:

  1. email password is disabled (this would be passwordless in the thirdpartypasswordless case)
  2. thirdparty provider list is empty
  3. not using dynamic login methods.

const dynamicLoginMethods = Multitenancy.getInstanceOrThrow().getLoadedDynamicLoginMethods();
const possiblyEnabledRecipes = {
thirdpartyemailpassword: {
enabled: dynamicLoginMethods?.["thirdparty"].enabled && dynamicLoginMethods["emailpassword"].enabled,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the second case in the ADR examples works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its working fine

return matching;
}
}

// Otherwise, If no recipe Id provided, or if no recipe id matches, return the first matching component.
return routeComponents[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please throw in this case:

  • we found no enabled recipes handling the current route
  • even if we render the route no actions on it will work since the recipe is disabled on the backend.

@@ -90,11 +90,12 @@ export const mergeProviders = ({
const providers: Pick<Provider, "id" | "buttonComponent" | "getButton">[] = [];

for (const tenantProvider of tenantProviders) {
// try finding exact match first
// try finding exact match or client provider that includes tenant id
let provider = clientProviders.find((provider) => {
const { id } = tenantProvider;
return provider.id === id || provider.id.includes(id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This includes is a bit strange to me here. Could you point me to the docs about this merging algorithm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can remove this include, i dont remember why i added this

Copy link
Collaborator

@porcellus porcellus left a comment

Choose a reason for hiding this comment

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

There are a few older comments that haven't been addressed.

@@ -24,7 +24,7 @@ export abstract class RecipeRouter {
}

const dynamicLoginMethods = Multitenancy.getInstanceOrThrow().getLoadedDynamicLoginMethods();
const possiblyEnabledRecipes = {
const componentMatchingRid = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const componentMatchingRid = {
const possiblyEnabledRecipes = {

@@ -33,20 +33,20 @@ export abstract class RecipeRouter {
},
...dynamicLoginMethods,
};
const components = routeComponents.filter((c) => c.matches());
const component = routeComponents.find((c) => c.matches());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const component = routeComponents.find((c) => c.matches());
const componentMatchingRid = routeComponents.find((c) => c.matches());

lib/ts/recipe/recipeRouter/index.tsx Outdated Show resolved Hide resolved
Alisher and others added 3 commits May 26, 2023 11:08
* wip: tests

* test: additional tests

* fix: picking providers logic & types

* test: adds test case

* fix test & remove redundant test case

* fix: minor cleanup/small fix + test extension

---------

Co-authored-by: Alisher <alisher@supertokens.com>
Co-authored-by: Mihaly Lengyel <mihaly@lengyel.tech>
@porcellus porcellus changed the base branch from feat/multitenancy to 0.33 June 22, 2023 22:56
@rishabhpoddar rishabhpoddar changed the base branch from 0.33 to 0.34 July 18, 2023 05:52
CHANGELOG.md Show resolved Hide resolved
@@ -36,23 +36,44 @@ supertokens.init({
providers: [
// We have provided you with development keys which you can use for testing.
// IMPORTANT: Please replace them with your own OAuth keys for production use.
Copy link
Member

Choose a reason for hiding this comment

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

undo all changes in example apps.

@rishabhpoddar rishabhpoddar merged commit 8fcfd18 into 0.34 Jul 19, 2023
3 of 23 checks passed
@rishabhpoddar rishabhpoddar deleted the feat/tp-multitenancy branch July 19, 2023 06:48
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.

None yet

3 participants