[GitHub] [ignite-3] valepakh commented on a diff in pull request #1942: IGNITE-19193 Store sensitive CLI config separately

["valepakh (via GitHub)" <gi...@apache.org>]

valepakh commented on code in PR #1942:
URL: https://github.com/apache/ignite-3/pull/1942#discussion_r1166476420


##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/CliCommandTestNotInitializedIntegrationBase.java:
##########
@@ -86,7 +86,10 @@ public class CliCommandTestNotInitializedIntegrationBase extends CliIntegrationT
     @BeforeEach
     public void setUp(TestInfo testInfo) throws Exception {
         super.setUp(testInfo);
-        configManagerProvider.configManager = new IniConfigManager(TestConfigManagerHelper.createIntegrationTests());
+        configManagerProvider.configManager = new IniConfigManager(
+                TestConfigManagerHelper.createIntegrationTests(),
+                TestConfigManagerHelper.createEmptySecretConfig()
+        );

Review Comment:
   Maybe we could create a helper method in the TestConfigManagerProvider which will assign a configManager with the IniConfigManager with the provided config file and an empty secret config?



##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/config/ini/IniProfile.java:
##########
@@ -17,30 +17,93 @@
 
 package org.apache.ignite.internal.cli.config.ini;
 
-import org.apache.ignite.internal.cli.config.Config;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.stream.Collectors;
+import org.apache.ignite.internal.cli.config.CliConfigKeys;
 import org.apache.ignite.internal.cli.config.Profile;
 
 /**
  * Implementation of {@link Profile} based on {@link IniSection}.
  */
 public class IniProfile implements Profile {
-    private final IniSection section;
+    private final String name;
     private final IniConfig config;
+    private final IniConfig secretConfig;
 
-    public IniProfile(IniSection section, Runnable saveAction) {
-        this.section = section;
-        this.config = new IniConfig(section, saveAction);
+    /**
+     * Constructor.
+     *
+     * @param name Profile name.
+     * @param config Ini config.
+     * @param secretConfig Ini secret config.
+     */
+    public IniProfile(String name, IniConfig config, IniConfig secretConfig) {
+        this.name = name;
+        this.config = config;
+        this.secretConfig = secretConfig;
     }
 
     /** {@inheritDoc} */
     @Override
     public String getName() {
-        return section.getName();
+        return name;
     }
 
     /** {@inheritDoc} */
     @Override
-    public Config getConfig() {
-        return config;
+    public Map<String, String> getAll() {
+        Map<String, String> all = new HashMap<>(config.getAll());
+        all.putAll(secretConfig.getAll());
+        return all;
+    }
+
+    /** {@inheritDoc} */
+    @Override
+    public String getProperty(String key) {
+        if (CliConfigKeys.secretConfigKeys().contains(key)) {
+            return secretConfig.getProperty(key);
+        } else {
+            return config.getProperty(key);
+        }
+    }
+
+    /** {@inheritDoc} */
+    @Override
+    public String getProperty(String key, String defaultValue) {
+        if (CliConfigKeys.secretConfigKeys().contains(key)) {
+            return secretConfig.getProperty(key, defaultValue);
+        } else {
+            return config.getProperty(key, defaultValue);
+        }
+    }
+
+    /** {@inheritDoc} */
+    @Override
+    public void setProperty(String key, String value) {
+        if (CliConfigKeys.secretConfigKeys().contains(key)) {
+            secretConfig.setProperty(key, value);
+        } else {
+            config.setProperty(key, value);
+        }
+    }
+
+    /** {@inheritDoc} */
+    @Override
+    public void setProperties(Map<String, String> values) {
+        Map<Boolean, Map<String, String>> secretToValues = values.entrySet().stream()
+                .collect(Collectors.groupingBy(it -> CliConfigKeys.secretConfigKeys().contains(it.getKey()),
+                        Collectors.toMap(Entry::getKey, Entry::getValue)));
+
+        Map<String, String> configValues = secretToValues.get(false);
+        if (configValues != null) {
+            config.setProperties(configValues);
+        }
+
+        Map<String, String> secretConfigValues = secretToValues.get(true);
+        if (secretConfigValues != null) {
+            secretConfig.setProperties(secretToValues.get(true));

Review Comment:
   ```suggestion
               secretConfig.setProperties(secretConfigValues);
   ```



##########
modules/cli/src/test/java/org/apache/ignite/internal/cli/commands/cliconfig/CliConfigShowCommandTest.java:
##########
@@ -33,14 +33,16 @@ protected Class<?> getCommandClass() {
     void noKey() {
         execute();
 
-        String expectedResult = "[database]" + System.lineSeparator()
-                + "server=127.0.0.1" + System.lineSeparator()
-                + "port=8080" + System.lineSeparator()
-                + "file=\"apache.ignite\"" + System.lineSeparator();
+        String[] expectedResult = {
+                "[database]",

Review Comment:
   Where did the lineSeparator go and what's the point of changing this to array?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org