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>
This commit is contained in:
Giteabot 2023-06-21 00:51:26 -04:00 committed by GitHub
parent 6f1c95ec5b
commit 8302b95d6b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 94 additions and 16 deletions

View file

@ -217,9 +217,15 @@ func setPort(port string) error {
defaultLocalURL += ":" + setting.HTTPPort + "/" defaultLocalURL += ":" + setting.HTTPPort + "/"
// Save LOCAL_ROOT_URL if port changed // Save LOCAL_ROOT_URL if port changed
setting.CfgProvider.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL) rootCfg := setting.CfgProvider
if err := setting.CfgProvider.Save(); err != nil { saveCfg, err := rootCfg.PrepareSaving()
return fmt.Errorf("Failed to save config file: %v", err) if err != nil {
return fmt.Errorf("failed to save config file: %v", err)
}
rootCfg.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
saveCfg.Section("server").Key("LOCAL_ROOT_URL").SetValue(defaultLocalURL)
if err = saveCfg.Save(); err != nil {
return fmt.Errorf("failed to save config file: %v", err)
} }
} }
return nil return nil

View file

@ -4,6 +4,7 @@
package setting package setting
import ( import (
"errors"
"fmt" "fmt"
"os" "os"
"path/filepath" "path/filepath"
@ -51,11 +52,17 @@ type ConfigProvider interface {
GetSection(name string) (ConfigSection, error) GetSection(name string) (ConfigSection, error)
Save() error Save() error
SaveTo(filename string) error SaveTo(filename string) error
DisableSaving()
PrepareSaving() (ConfigProvider, error)
} }
type iniConfigProvider struct { type iniConfigProvider struct {
opts *Options opts *Options
ini *ini.File ini *ini.File
disableSaving bool
newFile bool // whether the file has not existed previously newFile bool // whether the file has not existed previously
} }
@ -191,7 +198,7 @@ type Options struct {
// NewConfigProviderFromFile load configuration from file. // NewConfigProviderFromFile load configuration from file.
// NOTE: do not print any log except error. // NOTE: do not print any log except error.
func NewConfigProviderFromFile(opts *Options) (ConfigProvider, error) { func NewConfigProviderFromFile(opts *Options) (ConfigProvider, error) {
cfg := ini.Empty() cfg := ini.Empty(ini.LoadOptions{KeyValueDelimiterOnWrite: " = "})
newFile := true newFile := true
if opts.CustomConf != "" { if opts.CustomConf != "" {
@ -252,8 +259,13 @@ func (p *iniConfigProvider) GetSection(name string) (ConfigSection, error) {
return &iniConfigSection{sec: sec}, nil 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 // Save saves the content into file
func (p *iniConfigProvider) Save() error { func (p *iniConfigProvider) Save() error {
if p.disableSaving {
return errDisableSaving
}
filename := p.opts.CustomConf filename := p.opts.CustomConf
if filename == "" { if filename == "" {
if !p.opts.AllowEmpty { if !p.opts.AllowEmpty {
@ -285,9 +297,29 @@ func (p *iniConfigProvider) Save() error {
} }
func (p *iniConfigProvider) SaveTo(filename string) error { func (p *iniConfigProvider) SaveTo(filename string) error {
if p.disableSaving {
return errDisableSaving
}
return p.ini.SaveTo(filename) 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) { func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting any) {
if err := rootCfg.Section(sectionName).MapTo(setting); err != nil { if err := rootCfg.Section(sectionName).MapTo(setting); err != nil {
log.Fatal("Failed to map %s settings: %v", sectionName, err) log.Fatal("Failed to map %s settings: %v", sectionName, err)

View file

@ -84,11 +84,11 @@ func TestNewConfigProviderFromFile(t *testing.T) {
bs, err := os.ReadFile(testFile) bs, err := os.ReadFile(testFile)
assert.NoError(t, err) 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) bs, err = os.ReadFile(testFile1)
assert.NoError(t, err) 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 // load existing file and save
cfg, err = NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true}) cfg, err = NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true})
@ -99,7 +99,7 @@ func TestNewConfigProviderFromFile(t *testing.T) {
assert.NoError(t, cfg.Save()) assert.NoError(t, cfg.Save())
bs, err = os.ReadFile(testFile) bs, err = os.ReadFile(testFile)
assert.NoError(t, err) 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) { 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, "foo", cfg.Section("").Key("k1").String())
assert.Equal(t, "xxx", cfg.Section("").Key("k2").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))
}

View file

@ -59,13 +59,18 @@ func loadLFSFrom(rootCfg ConfigProvider) error {
if err != nil || n != 32 { if err != nil || n != 32 {
LFS.JWTSecretBase64, err = generate.NewJwtSecretBase64() LFS.JWTSecretBase64, err = generate.NewJwtSecretBase64()
if err != nil { 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 // Save secret
sec.Key("LFS_JWT_SECRET").SetValue(LFS.JWTSecretBase64) saveCfg, err := rootCfg.PrepareSaving()
if err := rootCfg.Save(); err != nil { if err != nil {
return fmt.Errorf("Error saving JWT Secret for custom config: %v", err) 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)
} }
} }

View file

@ -130,8 +130,13 @@ func loadOAuth2From(rootCfg ConfigProvider) {
} }
secretBase64 := base64.RawURLEncoding.EncodeToString(key) 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) 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) log.Fatal("save oauth2.JWT_SECRET failed: %v", err)
} }
} }

View file

@ -89,8 +89,13 @@ func generateSaveInternalToken(rootCfg ConfigProvider) {
} }
InternalToken = token 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) 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) log.Fatal("Error saving internal token: %v", err)
} }
} }

View file

@ -202,6 +202,7 @@ func Init(opts *Options) {
} }
var err error var err error
CfgProvider, err = NewConfigProviderFromFile(opts) 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 { if err != nil {
log.Fatal("newConfigProviderFromFile[%v]: %v", opts, err) log.Fatal("newConfigProviderFromFile[%v]: %v", opts, err)
} }
@ -214,7 +215,7 @@ func Init(opts *Options) {
// loadCommonSettingsFrom loads common configurations from a configuration provider. // loadCommonSettingsFrom loads common configurations from a configuration provider.
func loadCommonSettingsFrom(cfg ConfigProvider) error { 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) loadRunModeFrom(cfg)
loadLogGlobalFrom(cfg) loadLogGlobalFrom(cfg)
loadServerFrom(cfg) loadServerFrom(cfg)