[MM-66789] Restrict ImportSettings.Directory changes via API and add validation (#34653) (#34987)

Automatic Merge

server/channels/api4/config.go

@@ -159,6 +159,9 @@ func updateConfig(c *Context, w http.ResponseWriter, r *http.Request) {
 	// modifications to the slice.
 	cfg.PluginSettings.SignaturePublicKeyFiles = appCfg.PluginSettings.SignaturePublicKeyFiles
 
+	// Do not allow import directory to be changed through the API
+	*cfg.ImportSettings.Directory = *appCfg.ImportSettings.Directory
+
 	// Do not allow marketplace URL to be toggled through the API if EnableUploads are disabled.
 	if cfg.PluginSettings.EnableUploads != nil && !*appCfg.PluginSettings.EnableUploads {
 		*cfg.PluginSettings.MarketplaceURL = *appCfg.PluginSettings.MarketplaceURL
@@ -317,6 +320,12 @@ func patchConfig(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Do not allow import directory to be changed through the API
+	if cfg.ImportSettings.Directory != nil && *cfg.ImportSettings.Directory != *appCfg.ImportSettings.Directory {
+		c.Err = model.NewAppError("patchConfig", "api.config.update_config.not_allowed_security.app_error", map[string]any{"Name": "ImportSettings.Directory"}, "", http.StatusForbidden)
+		return
+	}
+
 	// Do not allow marketplace URL to be toggled if plugin uploads are disabled.
 	if cfg.PluginSettings.MarketplaceURL != nil && cfg.PluginSettings.EnableUploads != nil {
 		// Breaking it down to 2 conditions to make it simple.

server/channels/api4/config_test.go

@@ -310,6 +310,43 @@ func TestUpdateConfig(t *testing.T) {
 		CheckForbiddenStatus(t, resp)
 	})
 
+	t.Run("Should not be able to modify ImportSettings.Directory", func(t *testing.T) {
+		t.Run("sysadmin", func(t *testing.T) {
+			oldDirectory := *th.App.Config().ImportSettings.Directory
+			cfg2 := th.App.Config().Clone()
+			*cfg2.ImportSettings.Directory = "./new-import-dir"
+
+			cfg2, _, err = th.SystemAdminClient.UpdateConfig(context.Background(), cfg2)
+			require.NoError(t, err)
+			assert.Equal(t, oldDirectory, *cfg2.ImportSettings.Directory)
+			assert.Equal(t, oldDirectory, *th.App.Config().ImportSettings.Directory)
+
+			cfg2.ImportSettings.Directory = nil
+			cfg2, _, err = th.SystemAdminClient.UpdateConfig(context.Background(), cfg2)
+			require.NoError(t, err)
+			assert.Equal(t, oldDirectory, *cfg2.ImportSettings.Directory)
+			assert.Equal(t, oldDirectory, *th.App.Config().ImportSettings.Directory)
+		})
+
+		t.Run("local mode", func(t *testing.T) {
+			oldDirectory := *th.App.Config().ImportSettings.Directory
+			cfg2 := th.App.Config().Clone()
+			newDirectory := "./new-import-dir"
+			*cfg2.ImportSettings.Directory = newDirectory
+
+			cfg2, _, err = th.LocalClient.UpdateConfig(context.Background(), cfg2)
+			require.NoError(t, err)
+			assert.Equal(t, newDirectory, *cfg2.ImportSettings.Directory)
+			assert.Equal(t, newDirectory, *th.App.Config().ImportSettings.Directory)
+
+			cfg2.ImportSettings.Directory = nil
+			cfg2, _, err = th.LocalClient.UpdateConfig(context.Background(), cfg2)
+			require.NoError(t, err)
+			assert.Equal(t, oldDirectory, *cfg2.ImportSettings.Directory)
+			assert.Equal(t, oldDirectory, *th.App.Config().ImportSettings.Directory)
+		})
+	})
+
 	t.Run("System Admin should not be able to clear Site URL", func(t *testing.T) {
 		siteURL := cfg.ServiceSettings.SiteURL
 		defer th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.SiteURL = siteURL })
@@ -822,6 +859,30 @@ func TestPatchConfig(t *testing.T) {
 				CheckForbiddenStatus(t, resp)
 			}
 		})
+
+		t.Run("not allowing to change import directory via api, unless local mode", func(t *testing.T) {
+			oldDirectory := *th.App.Config().ImportSettings.Directory
+			config := model.Config{ImportSettings: model.ImportSettings{
+				Directory: model.NewPointer("./new-import-dir"),
+			}}
+
+			updatedConfig, resp, err := client.PatchConfig(context.Background(), &config)
+			if client == th.LocalClient {
+				require.NoError(t, err)
+				CheckOKStatus(t, resp)
+				assert.Equal(t, "./new-import-dir", *updatedConfig.ImportSettings.Directory)
+			} else {
+				require.Error(t, err)
+				CheckForbiddenStatus(t, resp)
+			}
+
+			// Reset for local mode
+			if client == th.LocalClient {
+				th.App.UpdateConfig(func(cfg *model.Config) {
+					*cfg.ImportSettings.Directory = oldDirectory
+				})
+			}
+		})
 	})
 
 	t.Run("Should not be able to modify PluginSettings.MarketplaceURL if EnableUploads is disabled", func(t *testing.T) {

server/cmd/mmctl/commands/config_e2e_test.go

@@ -163,6 +163,34 @@ func (s *MmctlE2ETestSuite) TestConfigSetCmd() {
 		s.Require().Len(printer.GetLines(), 0)
 		s.Require().Len(printer.GetErrorLines(), 0)
 	})
+
+	s.Run("ImportSettings.Directory cannot be set via API but can via local mode", func() {
+		printer.Clean()
+		originalDir := *s.th.App.Config().ImportSettings.Directory
+
+		args := []string{"ImportSettings.Directory", "./api-blocked-import"}
+		err := configSetCmdF(s.th.SystemAdminClient, &cobra.Command{}, args)
+		s.Require().NotNil(err)
+		s.Require().Contains(err.Error(), "not allowed due to security reasons")
+		s.Require().Len(printer.GetLines(), 0)
+
+		// Verify value didn't change
+		s.Require().Equal(originalDir, *s.th.App.Config().ImportSettings.Directory)
+
+		printer.Clean()
+		args = []string{"ImportSettings.Directory", "./local-allowed-import"}
+		err = configSetCmdF(s.th.LocalClient, &cobra.Command{}, args)
+		s.Require().Nil(err)
+		s.Require().Len(printer.GetLines(), 1)
+		config, ok := printer.GetLines()[0].(*model.Config)
+		s.Require().True(ok)
+		s.Require().Equal("./local-allowed-import", *config.ImportSettings.Directory)
+
+		// Reset to original
+		s.th.App.UpdateConfig(func(cfg *model.Config) {
+			cfg.ImportSettings.Directory = &originalDir
+		})
+	})
 }
 
 func (s *MmctlE2ETestSuite) TestConfigEditCmd() {