MM-11720: disable loading plugin specific config from the environment (#9334)

There are numerous issues here, including some non-determinism in the viper library (fixable) and some annoying behaviour regarding periods in keys, often used by plugin ids (fix unknown). Let's defer the handling of same until we can get our config loading library to do what we need it to do vs. having to hack around viper all the time.

2 files changed, 133 insertions, 22 deletions

diff --git a/utils/config.go b/utils/config.go
index 3ce2215a3..7e5a42faa 100644
--- a/utils/config.go
+++ b/utils/config.go
@@ -247,6 +247,16 @@ func ReadConfig(r io.Reader, allowEnvironmentOverrides bool) (*model.Config, map
 
 	var config model.Config
 	unmarshalErr := v.Unmarshal(&config)
+	// https://github.com/spf13/viper/issues/324
+	// https://github.com/spf13/viper/issues/348
+	if unmarshalErr == nil {
+		config.PluginSettings.Plugins = make(map[string]map[string]interface{})
+		unmarshalErr = v.UnmarshalKey("pluginsettings.plugins", &config.PluginSettings.Plugins)
+	}
+	if unmarshalErr == nil {
+		config.PluginSettings.PluginStates = make(map[string]*model.PluginState)
+		unmarshalErr = v.UnmarshalKey("pluginsettings.pluginstates", &config.PluginSettings.PluginStates)
+	}
 
 	envConfig := v.EnvSettings()
 
@@ -274,6 +284,10 @@ func newViper(allowEnvironmentOverrides bool) *viper.Viper {
 	defaults := getDefaultsFromStruct(model.Config{})
 
 	for key, value := range defaults {
+		if key == "PluginSettings.Plugins" || key == "PluginSettings.PluginStates" {
+			continue
+		}
+
 		v.SetDefault(key, value)
 	}
 
diff --git a/utils/config_test.go b/utils/config_test.go
index a08b73632..77705192d 100644
--- a/utils/config_test.go
+++ b/utils/config_test.go
@@ -52,32 +52,64 @@ func TestReadConfig_PluginSettings(t *testing.T) {
 						"abc": 123,
 						"def": "456"
 					}
-				}
+				},
+				"jira": {
+					"number": 2,
+					"string": "123",
+					"boolean": true,
+					"abc.def.ghi": {
+						"abc": 456,
+						"def": "123"
+					}
+ 				}
 			},
 			"PluginStates": {
 				"com.example.plugin": {
 					"enable": true
-				}
+				},
+				"jira": {
+					"enable": false
+ 				}
 			}
 		}
 	}`)), false)
 	require.Nil(t, err)
 
 	assert.Equal(t, "/temp/mattermost-plugins", *config.PluginSettings.Directory)
-	assert.Contains(t, config.PluginSettings.Plugins, "com.example.plugin")
-	assert.Equal(t, map[string]interface{}{
-		"number":  float64(1),
-		"string":  "abc",
-		"boolean": false,
-		"abc.def.ghi": map[string]interface{}{
-			"abc": float64(123),
-			"def": "456",
-		},
-	}, config.PluginSettings.Plugins["com.example.plugin"])
-	assert.Contains(t, config.PluginSettings.PluginStates, "com.example.plugin")
-	assert.Equal(t, model.PluginState{
-		Enable: true,
-	}, *config.PluginSettings.PluginStates["com.example.plugin"])
+
+	if assert.Contains(t, config.PluginSettings.Plugins, "com.example.plugin") {
+		assert.Equal(t, map[string]interface{}{
+			"number":  float64(1),
+			"string":  "abc",
+			"boolean": false,
+			"abc.def.ghi": map[string]interface{}{
+				"abc": float64(123),
+				"def": "456",
+			},
+		}, config.PluginSettings.Plugins["com.example.plugin"])
+	}
+	if assert.Contains(t, config.PluginSettings.PluginStates, "com.example.plugin") {
+		assert.Equal(t, model.PluginState{
+			Enable: true,
+		}, *config.PluginSettings.PluginStates["com.example.plugin"])
+	}
+
+	if assert.Contains(t, config.PluginSettings.Plugins, "jira") {
+		assert.Equal(t, map[string]interface{}{
+			"number":  float64(2),
+			"string":  "123",
+			"boolean": true,
+			"abc.def.ghi": map[string]interface{}{
+				"abc": float64(456),
+				"def": "123",
+			},
+		}, config.PluginSettings.Plugins["jira"])
+	}
+	if assert.Contains(t, config.PluginSettings.PluginStates, "jira") {
+		assert.Equal(t, model.PluginState{
+			Enable: false,
+		}, *config.PluginSettings.PluginStates["jira"])
+	}
 }
 
 func TestTimezoneConfig(t *testing.T) {
@@ -381,6 +413,20 @@ func TestConfigFromEnviroVars(t *testing.T) {
 		},
 		"SupportSettings": {
 			"TermsOfServiceLink": "https://about.mattermost.com/default-terms/"
+		},
+		"PluginSettings": {
+			"Enable": true,
+			"Plugins": {
+				"jira": {
+					"enabled": "true",
+					"secret": "config-secret"
+				}
+			},
+			"PluginStates": {
+				"jira": {
+					"Enable": true
+				}
+			}
 		}
 	}`
 
@@ -517,20 +563,19 @@ func TestConfigFromEnviroVars(t *testing.T) {
 	})
 
 	t.Run("plugin directory settings", func(t *testing.T) {
+		os.Setenv("MM_PLUGINSETTINGS_ENABLE", "false")
 		os.Setenv("MM_PLUGINSETTINGS_DIRECTORY", "/temp/plugins")
 		os.Setenv("MM_PLUGINSETTINGS_CLIENTDIRECTORY", "/temp/clientplugins")
+		defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLE")
 		defer os.Unsetenv("MM_PLUGINSETTINGS_DIRECTORY")
 		defer os.Unsetenv("MM_PLUGINSETTINGS_CLIENTDIRECTORY")
 
 		cfg, envCfg, err := ReadConfig(strings.NewReader(config), true)
 		require.Nil(t, err)
 
-		if *cfg.PluginSettings.Directory != "/temp/plugins" {
-			t.Fatal("Couldn't read Directory from environment var")
-		}
-		if *cfg.PluginSettings.ClientDirectory != "/temp/clientplugins" {
-			t.Fatal("Couldn't read ClientDirectory from environment var")
-		}
+		assert.Equal(t, false, *cfg.PluginSettings.Enable)
+		assert.Equal(t, "/temp/plugins", *cfg.PluginSettings.Directory)
+		assert.Equal(t, "/temp/clientplugins", *cfg.PluginSettings.ClientDirectory)
 
 		if pluginSettings, ok := envCfg["PluginSettings"]; !ok {
 			t.Fatal("PluginSettings is missing from envConfig")
@@ -545,6 +590,58 @@ func TestConfigFromEnviroVars(t *testing.T) {
 			}
 		}
 	})
+
+	t.Run("plugin specific settings cannot be overridden via environment", func(t *testing.T) {
+		os.Setenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_ENABLED", "false")
+		os.Setenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_SECRET", "env-secret")
+		os.Setenv("MM_PLUGINSETTINGS_PLUGINSTATES_JIRA_ENABLE", "false")
+		defer os.Unsetenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_ENABLED")
+		defer os.Unsetenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_SECRET")
+		defer os.Unsetenv("MM_PLUGINSETTINGS_PLUGINSTATES_JIRA_ENABLE")
+
+		cfg, envCfg, err := ReadConfig(strings.NewReader(config), true)
+		require.Nil(t, err)
+
+		if pluginsJira, ok := cfg.PluginSettings.Plugins["jira"]; !ok {
+			t.Fatal("PluginSettings.Plugins.jira is missing from config")
+		} else {
+			if enabled, ok := pluginsJira["enabled"]; !ok {
+				t.Fatal("PluginSettings.Plugins.jira.enabled is missing from config")
+			} else {
+				assert.Equal(t, "true", enabled)
+			}
+
+			if secret, ok := pluginsJira["secret"]; !ok {
+				t.Fatal("PluginSettings.Plugins.jira.secret is missing from config")
+			} else {
+				assert.Equal(t, "config-secret", secret)
+			}
+		}
+
+		if pluginStatesJira, ok := cfg.PluginSettings.PluginStates["jira"]; !ok {
+			t.Fatal("PluginSettings.PluginStates.jira is missing from config")
+		} else {
+			require.Equal(t, true, pluginStatesJira.Enable)
+		}
+
+		if pluginSettings, ok := envCfg["PluginSettings"]; !ok {
+			t.Fatal("PluginSettings is missing from envConfig")
+		} else if pluginSettingsAsMap, ok := pluginSettings.(map[string]interface{}); !ok {
+			t.Fatal("PluginSettings is not a map in envConfig")
+		} else {
+			if plugins, ok := pluginSettingsAsMap["Plugins"].(map[string]interface{}); !ok {
+				t.Fatal("PluginSettings.Plugins is not a map in envConfig")
+			} else if _, ok := plugins["jira"].(map[string]interface{}); ok {
+				t.Fatal("PluginSettings.Plugins.jira should not be a map in envConfig")
+			}
+
+			if pluginStates, ok := pluginSettingsAsMap["PluginStates"].(map[string]interface{}); !ok {
+				t.Fatal("PluginSettings.PluginStates is missing from envConfig")
+			} else if _, ok := pluginStates["jira"].(map[string]interface{}); ok {
+				t.Fatal("PluginSettings.PluginStates.jira should not be a map in envConfig")
+			}
+		}
+	})
 }
 
 func TestValidateLocales(t *testing.T) {