-
Notifications
You must be signed in to change notification settings - Fork 540
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: Changes for multitenancy dashboard #963
Conversation
* Add an API to fetch the coreConfig properties * Update changelog * Include plugin configs as well * Remove core_config_result from the final output * Hide protected config * Fix formatting changes * Add tests to ensure that the properties are same in the config and core config file * Match descriptions from devConfig.yaml as well * Refactor CoreConfig to return ConfigFieldInfo objects * Refactor enum property validation * Refactor move storage fields to API * Refactor getConfigFieldsInfo method and update CoreConfigListAPI * Add test to check protected properties are not present when saas secret is set * Add comments wherever necessary
## Unreleased | ||
|
||
- Add a new core API for fetching all the core properties | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schema changes + migration of schema + changes to multi tenancy + changes to how you configure a tenant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release prep at the end
src/main/java/io/supertokens/config/annotations/EnumProperty.java
Outdated
Show resolved
Hide resolved
if (getVersionFromRequest(req).greaterThanOrEqualTo(SemVer.v5_1)) { | ||
// if using first factors as a way to enable/disable recipes but the tenant was already created using an | ||
// old CDI, we automatically enable the recipes to make things work seamlessly | ||
if (hasFirstFactors && firstFactors != null) { | ||
if (List.of(firstFactors).contains("emailpassword") && emailPasswordEnabled == null) { | ||
emailPasswordEnabled = true; | ||
} | ||
if (List.of(firstFactors).contains("thirdparty") && thirdPartyEnabled == null) { | ||
thirdPartyEnabled = true; | ||
} | ||
if ((List.of(firstFactors).contains("otp-email") || List.of(firstFactors).contains("otp-phone") || List.of(firstFactors).contains("link-email") || List.of(firstFactors).contains("link-phone")) && passwordlessEnabled == null) { | ||
passwordlessEnabled = true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think by default, all these enabled booleans have to be empty anyway for >= 5.1 CDI! Why is this being done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just set all to true in this if block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do the if should be
- (>= 5.1 cdi && emailPasswordEnabled == null) => emailPasswordEnabled = true
same thing for third party and passwordless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no that might bring in un-intended side effects if the tenant is being updated with something else, like coreConfig only. This is specifically to handle tenants created using older CDI, and now updating enabled recipes using the firstFactors array.
); | ||
if (useFirstFactorsFromStaticIfEmpty == null) { | ||
useFirstFactorsFromStaticIfEmpty = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have to make this true for public tenant right? I think you need to refactor this whole thing to be more easy to read. Have separate branches in logic for if you are working with public tenant vs not, and what version of CDI it is. The code right now is very hard to follow and understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, it will be true if firstFactors is not passed at all. Whenever firstFactors is set, we set this boolean to false. Because this means that user want's to use that value for the tenant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done refactoring
if (getVersionFromRequest(req).lesserThan(SemVer.v5_1)) { | ||
result.get("thirdParty").getAsJsonObject().remove("useThirdPartyProvidersFromStaticConfigIfEmpty"); | ||
result.remove("useFirstFactorsFromStaticConfigIfEmpty"); | ||
} | ||
|
||
if (getVersionFromRequest(req).lesserThan(SemVer.v5_0)) { | ||
result.remove("firstFactors"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should make the toJson function only do this logic? Otherwise it is easy to miss doing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if (getVersionFromRequest(req).lesserThan(SemVer.v5_1)) { | ||
tenantConfigJson.remove("useFirstFactorsFromStaticConfigIfEmpty"); | ||
tenantConfigJson.get("thirdParty").getAsJsonObject().remove("useThirdPartyProvidersFromStaticConfigIfEmpty"); | ||
} | ||
|
||
if (getVersionFromRequest(req).lesserThan(SemVer.v5_0)) { | ||
tenantConfigJson.remove("firstFactors"); | ||
tenantConfigJson.remove("requiredSecondaryFactors"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should make the toJson function only do this logic? Otherwise it is easy to miss doing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if (getVersionFromRequest(req).lesserThan(SemVer.v5_1)) { | ||
tenantConfigJson.remove("useFirstFactorsFromStaticConfigIfEmpty"); | ||
tenantConfigJson.get("thirdParty").getAsJsonObject().remove("useThirdPartyProvidersFromStaticConfigIfEmpty"); | ||
} | ||
|
||
if (getVersionFromRequest(req).lesserThan(SemVer.v5_0)) { | ||
tenantConfigJson.remove("firstFactors"); | ||
tenantConfigJson.remove("requiredSecondaryFactors"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should make the toJson function only do this logic? Otherwise it is easy to miss doing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -112,9 +113,12 @@ protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IO | |||
tenantConfig.emailPasswordConfig, | |||
new ThirdPartyConfig( | |||
tenantConfig.thirdPartyConfig.enabled, | |||
getVersionFromRequest(req).lesserThan(SemVer.v5_1) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why checking version here? The value of useThirdPartyProvidersFromStaticConfigIfEmpty should be already set correctly based on when it was created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but when add/update/delete thirdParty config, we should set it to false for CDI >= 5.1. otherwise, preserve the older state
@@ -81,9 +82,13 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I | |||
config.tenantIdentifier, | |||
config.emailPasswordConfig, | |||
new ThirdPartyConfig( | |||
config.thirdPartyConfig.enabled, newProviders.toArray(new ThirdPartyConfig.Provider[0])), | |||
config.thirdPartyConfig.enabled, | |||
getVersionFromRequest(req).lesserThan(SemVer.v5_1) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why checking version here? The value of useThirdPartyProvidersFromStaticConfigIfEmpty should be already set correctly based on when it was created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but when add/update/delete thirdParty config, we should set it to false for CDI >= 5.1. otherwise, preserve the older state
Summary of change
Related issues
N/A
Test Plan
N/A
Documentation changes
N/A
Checklist for important updates
coreDriverInterfaceSupported.json
file has been updated (if needed)pluginInterfaceSupported.json
file has been updated (if needed)build.gradle
getPaidFeatureStats
function in FeatureFlag.java filebuild.gradle
, please make sure to add themin
implementationDependencies.json
.getValidFields
inio/supertokens/config/CoreConfig.java
if new aliases were added for any core config (similar to theaccess_token_signing_key_update_interval
config alias).git tag
) in the formatvX.Y.Z
, and then find thelatest branch (
git branch --all
) whoseX.Y
is greater than the latest released tag.app_id_to_user_id
table, make sure to delete from this table when deleting the user as well ifdeleteUserIdMappingToo
is false.Remaining TODOs for this PR