diff --git a/app/client/src/ce/utils/adminSettingsHelpers.ts b/app/client/src/ce/utils/adminSettingsHelpers.ts index e0268e08578a..3dd9227b96aa 100644 --- a/app/client/src/ce/utils/adminSettingsHelpers.ts +++ b/app/client/src/ce/utils/adminSettingsHelpers.ts @@ -65,7 +65,11 @@ export const getWrapperCategory = ( return categories[subCategory || category]; }; -export const getFilteredGeneralCategories = (categories: Category[]) => { +export const getFilteredGeneralCategories = ( + categories: Category[], + // eslint-disable-next-line @typescript-eslint/no-unused-vars + featureFlags?: Record, +) => { return categories ?.map((category: Category) => { return category; diff --git a/app/client/src/pages/AdminSettings/LeftPane.tsx b/app/client/src/pages/AdminSettings/LeftPane.tsx index c410f3827b5c..57a99bd6a762 100644 --- a/app/client/src/pages/AdminSettings/LeftPane.tsx +++ b/app/client/src/pages/AdminSettings/LeftPane.tsx @@ -202,12 +202,17 @@ export default function LeftPane() { const isSuperUser = user?.isSuperUser; const organizationPermissions = useSelector(getOrganizationPermissions); const isFeatureEnabled = useFeatureFlag(FEATURE_FLAG.license_gac_enabled); + const isMultiOrgEnabled = useFeatureFlag( + FEATURE_FLAG.license_multi_org_enabled, + ); const isAuditLogsEnabled = getHasAuditLogsReadPermission( isFeatureEnabled, organizationPermissions, ); - const filteredGeneralCategories = getFilteredGeneralCategories(categories); + const filteredGeneralCategories = getFilteredGeneralCategories(categories, { + license_multi_org_enabled: isMultiOrgEnabled, + }); const filteredAclCategories = getFilteredAclCategories( aclCategories, diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelper.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelper.java new file mode 100644 index 000000000000..8a6684776e6c --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelper.java @@ -0,0 +1,5 @@ +package com.appsmith.server.helpers; + +import com.appsmith.server.helpers.ce.BlacklistedEnvVariableHelperCE; + +public interface BlacklistedEnvVariableHelper extends BlacklistedEnvVariableHelperCE {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelperImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelperImpl.java new file mode 100644 index 000000000000..40a49db7a4d2 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/BlacklistedEnvVariableHelperImpl.java @@ -0,0 +1,10 @@ +package com.appsmith.server.helpers; + +import com.appsmith.server.helpers.ce.BlacklistedEnvVariableHelperCEImpl; +import lombok.RequiredArgsConstructor; +import org.springframework.stereotype.Component; + +@Component +@RequiredArgsConstructor +public class BlacklistedEnvVariableHelperImpl extends BlacklistedEnvVariableHelperCEImpl + implements BlacklistedEnvVariableHelper {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCE.java new file mode 100644 index 000000000000..6c6d08a1e0c2 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCE.java @@ -0,0 +1,7 @@ +package com.appsmith.server.helpers.ce; + +import java.util.Set; + +public interface BlacklistedEnvVariableHelperCE { + Set getBlacklistedEnvVariableForAppsmithCloud(String organizationId); +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCEImpl.java new file mode 100644 index 000000000000..b6ddf865fb7b --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/BlacklistedEnvVariableHelperCEImpl.java @@ -0,0 +1,10 @@ +package com.appsmith.server.helpers.ce; + +import java.util.Set; + +public class BlacklistedEnvVariableHelperCEImpl implements BlacklistedEnvVariableHelperCE { + @Override + public Set getBlacklistedEnvVariableForAppsmithCloud(String organizationId) { + return Set.of(); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java index 974b359c4e86..e4ad7dc90d88 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManagerImpl.java @@ -3,6 +3,7 @@ import com.appsmith.server.configurations.CommonConfig; import com.appsmith.server.configurations.EmailConfig; import com.appsmith.server.configurations.GoogleRecaptchaConfig; +import com.appsmith.server.helpers.BlacklistedEnvVariableHelper; import com.appsmith.server.helpers.FileUtils; import com.appsmith.server.helpers.UserUtils; import com.appsmith.server.notifications.EmailSender; @@ -40,7 +41,8 @@ public EnvManagerImpl( UserUtils userUtils, OrganizationService organizationService, ObjectMapper objectMapper, - EmailService emailService) { + EmailService emailService, + BlacklistedEnvVariableHelper blacklistedEnvVariableHelper) { super( sessionUserService, @@ -58,6 +60,7 @@ public EnvManagerImpl( userUtils, organizationService, objectMapper, - emailService); + emailService, + blacklistedEnvVariableHelper); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCE.java index f415b642f6f0..2deabaded88f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCE.java @@ -11,12 +11,10 @@ public interface EnvManagerCE { - List transformEnvContent(String envContent, Map changes); + Mono> transformEnvContent(String envContent, Map changes); Mono applyChanges(Map changes, String originHeader); - Mono> applyChangesToEnvFileWithoutAclCheck(Map changes); - Mono applyChangesFromMultipartFormData(MultiValueMap formData, String originHeader); void setAnalyticsEventAction( diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java index 9af6b6f33ead..f3dd450a51b4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/EnvManagerCEImpl.java @@ -13,8 +13,8 @@ import com.appsmith.server.dtos.TestEmailConfigRequestDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.BlacklistedEnvVariableHelper; import com.appsmith.server.helpers.CollectionUtils; -import com.appsmith.server.helpers.FeatureFlagMigrationHelper; import com.appsmith.server.helpers.FileUtils; import com.appsmith.server.helpers.TextUtils; import com.appsmith.server.helpers.UserUtils; @@ -46,6 +46,8 @@ import org.springframework.util.StringUtils; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Schedulers; +import reactor.util.context.Context; import reactor.util.function.Tuple2; import reactor.util.function.Tuples; @@ -83,6 +85,7 @@ import static com.appsmith.server.constants.EnvVariables.APPSMITH_RECAPTCHA_SITE_KEY; import static com.appsmith.server.constants.EnvVariables.APPSMITH_REPLY_TO; import static com.appsmith.server.constants.EnvVariables.APPSMITH_SIGNUP_ALLOWED_DOMAINS; +import static com.appsmith.server.constants.ce.FieldNameCE.ORGANIZATION_ID; import static java.lang.Boolean.TRUE; @Slf4j @@ -113,6 +116,8 @@ public class EnvManagerCEImpl implements EnvManagerCE { private final EmailService emailService; + private final BlacklistedEnvVariableHelper blacklistedEnvVariableHelper; + /** * This regex pattern matches environment variable declarations like `VAR_NAME=value` or `VAR_NAME="value"` or just * `VAR_NAME=`. It also defines two named capture groups, `name` and `value`, for the variable's name and value @@ -139,7 +144,8 @@ public EnvManagerCEImpl( UserUtils userUtils, OrganizationService organizationService, ObjectMapper objectMapper, - EmailService emailService) { + EmailService emailService, + BlacklistedEnvVariableHelper blacklistedEnvVariableHelper) { this.sessionUserService = sessionUserService; this.userService = userService; @@ -157,6 +163,7 @@ public EnvManagerCEImpl( this.organizationService = organizationService; this.objectMapper = objectMapper; this.emailService = emailService; + this.blacklistedEnvVariableHelper = blacklistedEnvVariableHelper; } /** @@ -169,7 +176,7 @@ public EnvManagerCEImpl( * @return List of string lines for updated env file content. */ @Override - public List transformEnvContent(String envContent, Map changes) { + public Mono> transformEnvContent(String envContent, Map changes) { final Set variablesNotInWhitelist = new HashSet<>(changes.keySet()); final Set organizationConfigWhitelist = allowedOrganizationConfiguration(); @@ -178,44 +185,57 @@ public List transformEnvContent(String envContent, Map c // class. This is because the configuration can be saved either in the .env file or the organization collection variablesNotInWhitelist.removeAll(VARIABLE_WHITELIST); variablesNotInWhitelist.removeAll(organizationConfigWhitelist); - if (!variablesNotInWhitelist.isEmpty()) { - throw new AppsmithException(AppsmithError.GENERIC_BAD_REQUEST); + return Mono.error(new AppsmithException(AppsmithError.GENERIC_BAD_REQUEST)); } - if (changes.containsKey(APPSMITH_MAIL_HOST.name())) { - changes.put( - APPSMITH_MAIL_ENABLED.name(), - Boolean.toString(StringUtils.hasText(changes.get(APPSMITH_MAIL_HOST.name())))); - } + return organizationService.getCurrentUserOrganizationId().map(organizationId -> { + // Create a copy of the changes map to avoid modifying the original map and avoiding unsupported ops + // exception if the map is unmodifiable + Map updatedChanges = new HashMap<>(changes); + // Add a check to remove blacklisted envs for updates + Set blacklistedEnvVariable = + blacklistedEnvVariableHelper.getBlacklistedEnvVariableForAppsmithCloud(organizationId); + for (String key : updatedChanges.keySet()) { + if (blacklistedEnvVariable.contains(key)) { + updatedChanges.remove(key); + } + } - if (changes.containsKey(APPSMITH_MAIL_USERNAME.name())) { - changes.put( - APPSMITH_MAIL_SMTP_AUTH.name(), - Boolean.toString(StringUtils.hasText(changes.get(APPSMITH_MAIL_USERNAME.name())))); - } + if (updatedChanges.containsKey(APPSMITH_MAIL_HOST.name())) { + updatedChanges.put( + APPSMITH_MAIL_ENABLED.name(), + Boolean.toString(StringUtils.hasText(updatedChanges.get(APPSMITH_MAIL_HOST.name())))); + } - final Set remainingChangedNames = new HashSet<>(changes.keySet()); + if (updatedChanges.containsKey(APPSMITH_MAIL_USERNAME.name())) { + updatedChanges.put( + APPSMITH_MAIL_SMTP_AUTH.name(), + Boolean.toString(StringUtils.hasText(updatedChanges.get(APPSMITH_MAIL_USERNAME.name())))); + } - final List outLines = envContent - .lines() - .map(line -> { - final Matcher matcher = ENV_VARIABLE_PATTERN.matcher(line); - if (!matcher.matches()) { - return line; - } - final String name = matcher.group("name"); - return remainingChangedNames.remove(name) - ? String.format("%s=%s", name, escapeForShell(changes.get(name))) - : line; - }) - .collect(Collectors.toList()); + final Set remainingChangedNames = new HashSet<>(updatedChanges.keySet()); - for (final String name : remainingChangedNames) { - outLines.add(name + "=" + escapeForShell(changes.get(name))); - } + final List outLines = envContent + .lines() + .map(line -> { + final Matcher matcher = ENV_VARIABLE_PATTERN.matcher(line); + if (!matcher.matches()) { + return line; + } + final String name = matcher.group("name"); + return remainingChangedNames.remove(name) + ? String.format("%s=%s", name, escapeForShell(updatedChanges.get(name))) + : line; + }) + .collect(Collectors.toList()); + + for (final String name : remainingChangedNames) { + outLines.add(name + "=" + escapeForShell(updatedChanges.get(name))); + } - return outLines; + return outLines; + }); } private String escapeForShell(String input) { @@ -350,25 +370,31 @@ private Mono updateOrganizationConfiguration(String organizationId organizationService.updateOrganizationConfiguration(organizationId, organizationConfiguration)); } + // This flow is pertinent for any variables that need to change in the .env file or be saved in the organization + // configuration @Override public Mono applyChanges(Map changes, String originHeader) { - // This flow is pertinent for any variables that need to change in the .env file or be saved in the organization - // configuration + // Create a copy of the changes map to avoid modifying the original map and avoiding unsupported ops exception + // if the map is unmodifiable + Map envChanges = new HashMap<>(changes); return verifyCurrentUserIsSuper() - .flatMap(user -> validateChanges(user, changes).thenReturn(user)) - .flatMap(user -> applyChangesToEnvFileWithoutAclCheck(changes) + .flatMap(user -> validateChanges(user, envChanges).thenReturn(user)) + .flatMap(user -> applyChangesToEnvFileWithoutAclCheck(envChanges) + // Add the organization id to the context to be able to extract the feature flags + .contextWrite(Context.of(ORGANIZATION_ID, user.getOrganizationId())) // For configuration variables, save the variables to the config collection instead of .env file // We ideally want to migrate all variables from .env file to the config collection for better // scalability // Write the changes to the organization collection in configuration field - .flatMap(originalVariables -> updateOrganizationConfiguration(user.getOrganizationId(), changes) - .then(sendAnalyticsEvent(user, originalVariables, changes)) + .flatMap(originalVariables -> updateOrganizationConfiguration( + user.getOrganizationId(), envChanges) + .then(sendAnalyticsEvent(user, originalVariables, envChanges)) .thenReturn(originalVariables))) .flatMap(originalValues -> { Mono dependentTasks = Mono.empty(); // Try and update any at runtime, that can be. - final Map changesCopy = new HashMap<>(changes); + final Map changesCopy = new HashMap<>(envChanges); if (changesCopy.containsKey(APPSMITH_SIGNUP_ALLOWED_DOMAINS.name())) { commonConfig.setAllowedDomainsString( @@ -434,14 +460,11 @@ public Mono applyChanges(Map changes, String originHeader) /** * This method applies the changes to the env file and should be called internally within the server as the ACL * checks are skipped. For client side calls please use {@link EnvManagerCEImpl#applyChanges(Map, String)}. - * Please refer {@link FeatureFlagMigrationHelper} for the use case where ACL checks - * should be skipped. * * @param changes Map of changes to be applied to the env file * @return Map of original variables before the changes were applied */ - @Override - public Mono> applyChangesToEnvFileWithoutAclCheck(Map changes) { + private Mono> applyChangesToEnvFileWithoutAclCheck(Map changes) { final Path envFilePath = Path.of(commonConfig.getEnvFilePath()); String originalContent; try { @@ -459,15 +482,17 @@ public Mono> applyChangesToEnvFileWithoutAclCheck(Map changedContent = transformEnvContent(originalContent, envFileChanges); - - try { - Files.write(envFilePath, changedContent); - } catch (IOException e) { - log.error("Unable to write to env file " + envFilePath, e); - return Mono.error(e); - } - return Mono.just(originalVariables); + return transformEnvContent(originalContent, envFileChanges) + .publishOn(Schedulers.boundedElastic()) + .map(changedContent -> { + try { + Files.write(envFilePath, changedContent); + } catch (IOException e) { + log.error("Unable to write to env file " + envFilePath, e); + throw new AppsmithException(AppsmithError.IO_ERROR, "Unable to write to env file"); + } + return originalVariables; + }); } @Override @@ -700,15 +725,20 @@ public Mono> getAllWithoutAclCheck() { */ @Override public Mono> getAllNonEmpty() { - return getAll().flatMap(map -> { - Map nonEmptyValuesMap = new HashMap<>(); - for (Map.Entry entry : map.entrySet()) { - if (StringUtils.hasText(entry.getValue())) { - nonEmptyValuesMap.put(entry.getKey(), entry.getValue()); - } - } - return Mono.just(nonEmptyValuesMap); - }); + return getAll().zipWith(organizationService.getCurrentUserOrganizationId()) + .flatMap(tuple2 -> { + Map map = tuple2.getT1(); + String organizationId = tuple2.getT2(); + Map nonEmptyValuesMap = new HashMap<>(); + Set blacklistedEnvVariable = + blacklistedEnvVariableHelper.getBlacklistedEnvVariableForAppsmithCloud(organizationId); + for (Map.Entry entry : map.entrySet()) { + if (StringUtils.hasText(entry.getValue()) && !blacklistedEnvVariable.contains(entry.getKey())) { + nonEmptyValuesMap.put(entry.getKey(), entry.getValue()); + } + } + return Mono.just(nonEmptyValuesMap); + }); } @Override diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java index 5bcb12f37009..67b74551f2fe 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java @@ -6,6 +6,7 @@ import com.appsmith.server.domains.User; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.BlacklistedEnvVariableHelper; import com.appsmith.server.helpers.FileUtils; import com.appsmith.server.helpers.UserUtils; import com.appsmith.server.notifications.EmailSender; @@ -32,9 +33,10 @@ import java.util.HashMap; import java.util.Map; +import java.util.Set; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.eq; @ExtendWith(SpringExtension.class) @Slf4j @@ -87,14 +89,16 @@ public class EnvManagerTest { private ObjectMapper objectMapper; @MockBean - private AssetService assetService; + AssetService assetService; @MockBean + BlacklistedEnvVariableHelper blacklistedEnvVariableHelper; + private EmailService emailService; @BeforeEach public void setup() { - envManager = new EnvManagerImpl( + EnvManager realEnvManager = new EnvManagerImpl( sessionUserService, userService, analyticsService, @@ -110,7 +114,15 @@ public void setup() { userUtils, organizationService, objectMapper, - emailService); + emailService, + blacklistedEnvVariableHelper); + + // Create a spy of the real env manager + envManager = Mockito.spy(realEnvManager); + + Mockito.when(organizationService.getCurrentUserOrganizationId()).thenReturn(Mono.just("org-id")); + Mockito.when(blacklistedEnvVariableHelper.getBlacklistedEnvVariableForAppsmithCloud(eq("org-id"))) + .thenReturn(Set.of()); } @Test @@ -118,53 +130,81 @@ public void simpleSample() { final String content = "APPSMITH_DB_URL='first value'\nAPPSMITH_REDIS_URL='second value'\n\nAPPSMITH_INSTANCE_NAME='third value'"; - assertThat(envManager.transformEnvContent(content, Map.of("APPSMITH_DB_URL", "new first value"))) - .containsExactly( - "APPSMITH_DB_URL='new first value'", - "APPSMITH_REDIS_URL='second value'", - "", - "APPSMITH_INSTANCE_NAME='third value'"); - - assertThat(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", "new second value"))) - .containsExactly( - "APPSMITH_DB_URL='first value'", - "APPSMITH_REDIS_URL='new second value'", - "", - "APPSMITH_INSTANCE_NAME='third value'"); - - assertThat(envManager.transformEnvContent(content, Map.of("APPSMITH_INSTANCE_NAME", "new third value"))) - .containsExactly( - "APPSMITH_DB_URL='first value'", - "APPSMITH_REDIS_URL='second value'", - "", - "APPSMITH_INSTANCE_NAME='new third value'"); - - assertThat(envManager.transformEnvContent( + StepVerifier.create(envManager.transformEnvContent(content, Map.of("APPSMITH_DB_URL", "new first value"))) + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='new first value'", + "APPSMITH_REDIS_URL='second value'", + "", + "APPSMITH_INSTANCE_NAME='third value'"); + }) + .verifyComplete(); + + StepVerifier.create(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", "new second value"))) + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='first value'", + "APPSMITH_REDIS_URL='new second value'", + "", + "APPSMITH_INSTANCE_NAME='third value'"); + }) + .verifyComplete(); + + StepVerifier.create( + envManager.transformEnvContent(content, Map.of("APPSMITH_INSTANCE_NAME", "new third value"))) + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='first value'", + "APPSMITH_REDIS_URL='second value'", + "", + "APPSMITH_INSTANCE_NAME='new third value'"); + }) + .verifyComplete(); + + StepVerifier.create(envManager.transformEnvContent( content, Map.of( "APPSMITH_DB_URL", "new first value", "APPSMITH_INSTANCE_NAME", "new third value"))) - .containsExactly( - "APPSMITH_DB_URL='new first value'", - "APPSMITH_REDIS_URL='second value'", - "", - "APPSMITH_INSTANCE_NAME='new third value'"); + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='new first value'", + "APPSMITH_REDIS_URL='second value'", + "", + "APPSMITH_INSTANCE_NAME='new third value'"); + }) + .verifyComplete(); } @Test public void emptyValues() { final String content = "APPSMITH_DB_URL=first value\nAPPSMITH_REDIS_URL=\n\nAPPSMITH_INSTANCE_NAME=third value"; - assertThat(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", "new second value"))) - .containsExactly( - "APPSMITH_DB_URL=first value", - "APPSMITH_REDIS_URL='new second value'", - "", - "APPSMITH_INSTANCE_NAME=third value"); + StepVerifier.create(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", "new second value"))) + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL=first value", + "APPSMITH_REDIS_URL='new second value'", + "", + "APPSMITH_INSTANCE_NAME=third value"); + }) + .verifyComplete(); - assertThat(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", ""))) - .containsExactly( - "APPSMITH_DB_URL=first value", "APPSMITH_REDIS_URL=", "", "APPSMITH_INSTANCE_NAME=third value"); + StepVerifier.create(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", ""))) + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL=first value", + "APPSMITH_REDIS_URL=", + "", + "APPSMITH_INSTANCE_NAME=third value"); + }) + .verifyComplete(); } @Test @@ -172,34 +212,46 @@ public void quotedValues() { final String content = "APPSMITH_DB_URL='first value'\nAPPSMITH_REDIS_URL=\"quoted value\"\n\nAPPSMITH_INSTANCE_NAME='third value'"; - assertThat(envManager.transformEnvContent( + StepVerifier.create(envManager.transformEnvContent( content, Map.of( "APPSMITH_DB_URL", "new first value", "APPSMITH_REDIS_URL", "new second value"))) - .containsExactly( - "APPSMITH_DB_URL='new first value'", - "APPSMITH_REDIS_URL='new second value'", - "", - "APPSMITH_INSTANCE_NAME='third value'"); - - assertThat(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", ""))) - .containsExactly( - "APPSMITH_DB_URL='first value'", - "APPSMITH_REDIS_URL=", - "", - "APPSMITH_INSTANCE_NAME='third value'"); - - assertThat(envManager.transformEnvContent( + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='new first value'", + "APPSMITH_REDIS_URL='new second value'", + "", + "APPSMITH_INSTANCE_NAME='third value'"); + }) + .verifyComplete(); + + StepVerifier.create(envManager.transformEnvContent(content, Map.of("APPSMITH_REDIS_URL", ""))) + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='first value'", + "APPSMITH_REDIS_URL=", + "", + "APPSMITH_INSTANCE_NAME='third value'"); + }) + .verifyComplete(); + + StepVerifier.create(envManager.transformEnvContent( content, Map.of( "APPSMITH_INSTANCE_NAME", "Sponge-bob's Instance", "APPSMITH_REDIS_URL", "value with \" char in it"))) - .containsExactly( - "APPSMITH_DB_URL='first value'", - "APPSMITH_REDIS_URL='value with \" char in it'", - "", - "APPSMITH_INSTANCE_NAME='Sponge-bob'\"'\"'s Instance'"); + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='first value'", + "APPSMITH_REDIS_URL='value with \" char in it'", + "", + "APPSMITH_INSTANCE_NAME='Sponge-bob'\"'\"'s Instance'"); + }) + .verifyComplete(); } @Test @@ -242,13 +294,14 @@ public void disallowedVariable() { final String content = "APPSMITH_DB_URL=first value\nDISALLOWED_NASTY_STUFF=\"quoted value\"\n\nAPPSMITH_INSTANCE_NAME=third value"; - assertThatThrownBy(() -> envManager.transformEnvContent( + StepVerifier.create(envManager.transformEnvContent( content, Map.of( "APPSMITH_DB_URL", "new first value", "DISALLOWED_NASTY_STUFF", "new second value"))) - .matches(value -> value instanceof AppsmithException - && AppsmithError.GENERIC_BAD_REQUEST.equals(((AppsmithException) value).getError())); + .expectErrorMatches(throwable -> throwable instanceof AppsmithException + && ((AppsmithException) throwable).getError().equals(AppsmithError.GENERIC_BAD_REQUEST)) + .verify(); } @Test @@ -256,17 +309,21 @@ public void addNewVariable() { final String content = "APPSMITH_DB_URL='first value'\nAPPSMITH_REDIS_URL='quoted value'\n\nAPPSMITH_INSTANCE_NAME='third value'"; - assertThat(envManager.transformEnvContent( + StepVerifier.create(envManager.transformEnvContent( content, Map.of( "APPSMITH_DB_URL", "new first value", "APPSMITH_DISABLE_TELEMETRY", "false"))) - .containsExactly( - "APPSMITH_DB_URL='new first value'", - "APPSMITH_REDIS_URL='quoted value'", - "", - "APPSMITH_INSTANCE_NAME='third value'", - "APPSMITH_DISABLE_TELEMETRY=false"); + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL='new first value'", + "APPSMITH_REDIS_URL='quoted value'", + "", + "APPSMITH_INSTANCE_NAME='third value'", + "APPSMITH_DISABLE_TELEMETRY=false"); + }) + .verifyComplete(); } @Test @@ -274,17 +331,21 @@ public void setValueWithQuotes() { final String content = "APPSMITH_DB_URL='first value'\nAPPSMITH_REDIS_URL='quoted value'\n\nAPPSMITH_INSTANCE_NAME='third value'"; - assertThat(envManager.transformEnvContent( + StepVerifier.create(envManager.transformEnvContent( content, Map.of( "APPSMITH_DB_URL", "'just quotes'", "APPSMITH_DISABLE_TELEMETRY", "some quotes 'inside' it"))) - .containsExactly( - "APPSMITH_DB_URL=\"'\"'just quotes'\"'\"", - "APPSMITH_REDIS_URL='quoted value'", - "", - "APPSMITH_INSTANCE_NAME='third value'", - "APPSMITH_DISABLE_TELEMETRY='some quotes '\"'\"'inside'\"'\"' it'"); + .assertNext(value -> { + assertThat(value) + .containsExactly( + "APPSMITH_DB_URL=\"'\"'just quotes'\"'\"", + "APPSMITH_REDIS_URL='quoted value'", + "", + "APPSMITH_INSTANCE_NAME='third value'", + "APPSMITH_DISABLE_TELEMETRY='some quotes '\"'\"'inside'\"'\"' it'"); + }) + .verifyComplete(); } @Test @@ -302,21 +363,47 @@ public void sendTestEmail_WhenUserNotSuperUser_ThrowsException() { @Test public void setEnv_AndGetAll() { - EnvManager envManagerInner = Mockito.mock(EnvManagerImpl.class); - + // Create a test map of environment variables Map envs = new HashMap<>(); envs.put("APPSMITH_DB_URL", "mongo-url"); envs.put("APPSMITH_DISABLE_TELEMETRY", ""); - Mockito.when(envManagerInner.getAll()).thenReturn(Mono.just(envs)); - Mockito.when(envManagerInner.getAllNonEmpty()).thenCallRealMethod(); - - Mono> envMono = envManagerInner.getAllNonEmpty(); + // Mock the getAll method on the spy to return our test environment variables + Mockito.doReturn(Mono.just(envs)).when(envManager).getAll(); - StepVerifier.create(envMono) + // Test the getAllNonEmpty method (which filters out empty values) + StepVerifier.create(envManager.getAllNonEmpty()) .assertNext(map -> { assertThat(map).hasSize(1); - assertThat(map.containsKey("APPSMITH_DISABLE_TELEMETRY")).isFalse(); + assertThat(map).containsKey("APPSMITH_DB_URL"); + assertThat(map).containsValue("mongo-url"); + assertThat(map).doesNotContainKey("APPSMITH_DISABLE_TELEMETRY"); + }) + .verifyComplete(); + } + + @Test + public void getAllNonEmpty_WithMultipleVariables_FiltersEmptyOnes() { + // Create a test map with multiple environment variables + Map envs = new HashMap<>(); + envs.put("APPSMITH_DB_URL", "mongo-url"); + envs.put("APPSMITH_DISABLE_TELEMETRY", ""); + envs.put("APPSMITH_INSTANCE_NAME", "test-instance"); + envs.put("APPSMITH_ADMIN_EMAILS", ""); + envs.put("APPSMITH_MAIL_HOST", "smtp.example.com"); + + // Mock the getAll method on the spy to return our test environment variables + Mockito.doReturn(Mono.just(envs)).when(envManager).getAll(); + + // Test the getAllNonEmpty method (which filters out empty values) + StepVerifier.create(envManager.getAllNonEmpty()) + .assertNext(map -> { + assertThat(map).hasSize(3); + assertThat(map).containsEntry("APPSMITH_DB_URL", "mongo-url"); + assertThat(map).containsEntry("APPSMITH_INSTANCE_NAME", "test-instance"); + assertThat(map).containsEntry("APPSMITH_MAIL_HOST", "smtp.example.com"); + assertThat(map).doesNotContainKey("APPSMITH_DISABLE_TELEMETRY"); + assertThat(map).doesNotContainKey("APPSMITH_ADMIN_EMAILS"); }) .verifyComplete(); }