diff options
author | Giteabot <teabot@gitea.io> | 2023-06-21 00:51:26 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-06-21 04:51:26 +0000 |
commit | 8302b95d6bfdbc40083a72e5d1b727fcb1e00940 (patch) | |
tree | 470eb05db1149b9ef171d26fb2ecd54c6524fe2c /modules/setting | |
parent | 6f1c95ec5b951347f549972a8699b05be5d3f634 (diff) | |
download | gitea-8302b95d6bfdbc40083a72e5d1b727fcb1e00940.tar.gz gitea-8302b95d6bfdbc40083a72e5d1b727fcb1e00940.zip |
Avoid polluting config file when "save" (#25395) (#25406)
Backport #25395 by @wxiaoguang
That's a longstanding INI package problem: the "MustXxx" calls change
the option values, and the following "Save" will save a lot of garbage
options into the user's config file.
Ideally we should refactor the INI package to a clear solution, but it's
a huge work.
A clear workaround is what this PR does: when "Save", load a clear INI
instance and save it.
Partially fix #25377, the "install" page needs more fine tunes.
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Diffstat (limited to 'modules/setting')
-rw-r--r-- | modules/setting/config_provider.go | 38 | ||||
-rw-r--r-- | modules/setting/config_provider_test.go | 30 | ||||
-rw-r--r-- | modules/setting/lfs.go | 13 | ||||
-rw-r--r-- | modules/setting/oauth2.go | 7 | ||||
-rw-r--r-- | modules/setting/security.go | 7 | ||||
-rw-r--r-- | modules/setting/setting.go | 3 |
6 files changed, 85 insertions, 13 deletions
diff --git a/modules/setting/config_provider.go b/modules/setting/config_provider.go index 526d69bbdc..deec5cc586 100644 --- a/modules/setting/config_provider.go +++ b/modules/setting/config_provider.go @@ -4,6 +4,7 @@ package setting import ( + "errors" "fmt" "os" "path/filepath" @@ -51,11 +52,17 @@ type ConfigProvider interface { GetSection(name string) (ConfigSection, error) Save() error SaveTo(filename string) error + + DisableSaving() + PrepareSaving() (ConfigProvider, error) } type iniConfigProvider struct { - opts *Options - ini *ini.File + opts *Options + ini *ini.File + + disableSaving bool + newFile bool // whether the file has not existed previously } @@ -191,7 +198,7 @@ type Options struct { // NewConfigProviderFromFile load configuration from file. // NOTE: do not print any log except error. func NewConfigProviderFromFile(opts *Options) (ConfigProvider, error) { - cfg := ini.Empty() + cfg := ini.Empty(ini.LoadOptions{KeyValueDelimiterOnWrite: " = "}) newFile := true if opts.CustomConf != "" { @@ -252,8 +259,13 @@ func (p *iniConfigProvider) GetSection(name string) (ConfigSection, error) { return &iniConfigSection{sec: sec}, nil } +var errDisableSaving = errors.New("this config can't be saved, developers should prepare a new config to save") + // Save saves the content into file func (p *iniConfigProvider) Save() error { + if p.disableSaving { + return errDisableSaving + } filename := p.opts.CustomConf if filename == "" { if !p.opts.AllowEmpty { @@ -285,9 +297,29 @@ func (p *iniConfigProvider) Save() error { } func (p *iniConfigProvider) SaveTo(filename string) error { + if p.disableSaving { + return errDisableSaving + } return p.ini.SaveTo(filename) } +// DisableSaving disables the saving function, use PrepareSaving to get clear config options. +func (p *iniConfigProvider) DisableSaving() { + p.disableSaving = true +} + +// PrepareSaving loads the ini from file again to get clear config options. +// Otherwise, the "MustXxx" calls would have polluted the current config provider, +// it makes the "Save" outputs a lot of garbage options +// After the INI package gets refactored, no "MustXxx" pollution, this workaround can be dropped. +func (p *iniConfigProvider) PrepareSaving() (ConfigProvider, error) { + cfgFile := p.opts.CustomConf + if cfgFile == "" { + return nil, errors.New("no config file to save") + } + return NewConfigProviderFromFile(p.opts) +} + func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting any) { if err := rootCfg.Section(sectionName).MapTo(setting); err != nil { log.Fatal("Failed to map %s settings: %v", sectionName, err) diff --git a/modules/setting/config_provider_test.go b/modules/setting/config_provider_test.go index 17650edea4..c5c5196e04 100644 --- a/modules/setting/config_provider_test.go +++ b/modules/setting/config_provider_test.go @@ -84,11 +84,11 @@ func TestNewConfigProviderFromFile(t *testing.T) { bs, err := os.ReadFile(testFile) assert.NoError(t, err) - assert.Equal(t, "[foo]\nk1=a\n", string(bs)) + assert.Equal(t, "[foo]\nk1 = a\n", string(bs)) bs, err = os.ReadFile(testFile1) assert.NoError(t, err) - assert.Equal(t, "[foo]\nk1=a\nk2=b\n", string(bs)) + assert.Equal(t, "[foo]\nk1 = a\nk2 = b\n", string(bs)) // load existing file and save cfg, err = NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true}) @@ -99,7 +99,7 @@ func TestNewConfigProviderFromFile(t *testing.T) { assert.NoError(t, cfg.Save()) bs, err = os.ReadFile(testFile) assert.NoError(t, err) - assert.Equal(t, "[foo]\nk1=a\n\n[bar]\nk1=b\n", string(bs)) + assert.Equal(t, "[foo]\nk1 = a\n\n[bar]\nk1 = b\n", string(bs)) } func TestNewConfigProviderForLocale(t *testing.T) { @@ -119,3 +119,27 @@ func TestNewConfigProviderForLocale(t *testing.T) { assert.Equal(t, "foo", cfg.Section("").Key("k1").String()) assert.Equal(t, "xxx", cfg.Section("").Key("k2").String()) } + +func TestDisableSaving(t *testing.T) { + testFile := t.TempDir() + "/test.ini" + _ = os.WriteFile(testFile, []byte("k1=a\nk2=b"), 0o644) + cfg, err := NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true}) + assert.NoError(t, err) + + cfg.DisableSaving() + err = cfg.Save() + assert.ErrorIs(t, err, errDisableSaving) + + saveCfg, err := cfg.PrepareSaving() + assert.NoError(t, err) + + saveCfg.Section("").Key("k1").MustString("x") + saveCfg.Section("").Key("k2").SetValue("y") + saveCfg.Section("").Key("k3").SetValue("z") + err = saveCfg.Save() + assert.NoError(t, err) + + bs, err := os.ReadFile(testFile) + assert.NoError(t, err) + assert.Equal(t, "k1 = a\nk2 = y\nk3 = z\n", string(bs)) +} diff --git a/modules/setting/lfs.go b/modules/setting/lfs.go index d68349be86..140a96f9ed 100644 --- a/modules/setting/lfs.go +++ b/modules/setting/lfs.go @@ -59,13 +59,18 @@ func loadLFSFrom(rootCfg ConfigProvider) error { if err != nil || n != 32 { LFS.JWTSecretBase64, err = generate.NewJwtSecretBase64() if err != nil { - return fmt.Errorf("Error generating JWT Secret for custom config: %v", err) + return fmt.Errorf("error generating JWT Secret for custom config: %v", err) } // Save secret - sec.Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64) - if err := rootCfg.Save(); err != nil { - return fmt.Errorf("Error saving JWT Secret for custom config: %v", err) + saveCfg, err := rootCfg.PrepareSaving() + if err != nil { + return fmt.Errorf("error saving JWT Secret for custom config: %v", err) + } + rootCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64) + saveCfg.Section("server").Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64) + if err := saveCfg.Save(); err != nil { + return fmt.Errorf("error saving JWT Secret for custom config: %v", err) } } diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index 836a2bb25f..83c607a416 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -130,8 +130,13 @@ func loadOAuth2From(rootCfg ConfigProvider) { } secretBase64 := base64.RawURLEncoding.EncodeToString(key) + saveCfg, err := rootCfg.PrepareSaving() + if err != nil { + log.Fatal("save oauth2.JWT_SECRET failed: %v", err) + } rootCfg.Section("oauth2").Key("JWT_SECRET").SetValue(secretBase64) - if err := rootCfg.Save(); err != nil { + saveCfg.Section("oauth2").Key("JWT_SECRET").SetValue(secretBase64) + if err := saveCfg.Save(); err != nil { log.Fatal("save oauth2.JWT_SECRET failed: %v", err) } } diff --git a/modules/setting/security.go b/modules/setting/security.go index ce2e7711f1..c39eb7f3eb 100644 --- a/modules/setting/security.go +++ b/modules/setting/security.go @@ -89,8 +89,13 @@ func generateSaveInternalToken(rootCfg ConfigProvider) { } InternalToken = token + saveCfg, err := rootCfg.PrepareSaving() + if err != nil { + log.Fatal("Error saving internal token: %v", err) + } rootCfg.Section("security").Key("INTERNAL_TOKEN").SetValue(token) - if err := rootCfg.Save(); err != nil { + saveCfg.Section("security").Key("INTERNAL_TOKEN").SetValue(token) + if err = saveCfg.Save(); err != nil { log.Fatal("Error saving internal token: %v", err) } } diff --git a/modules/setting/setting.go b/modules/setting/setting.go index 539eb4b197..6eaddbe2b5 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -202,6 +202,7 @@ func Init(opts *Options) { } var err error CfgProvider, err = NewConfigProviderFromFile(opts) + CfgProvider.DisableSaving() // do not allow saving the CfgProvider into file, it will be polluted by the "MustXxx" calls if err != nil { log.Fatal("newConfigProviderFromFile[%v]: %v", opts, err) } @@ -214,7 +215,7 @@ func Init(opts *Options) { // loadCommonSettingsFrom loads common configurations from a configuration provider. func loadCommonSettingsFrom(cfg ConfigProvider) error { - // WARNNING: don't change the sequence except you know what you are doing. + // WARNING: don't change the sequence except you know what you are doing. loadRunModeFrom(cfg) loadLogGlobalFrom(cfg) loadServerFrom(cfg) |