diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 9c56e0f9a0fba..dbd0414194774 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -216,8 +216,8 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final return urlStr } - disableGravatar := setting.Config().Picture.DisableGravatar.Value(ctx) - if !disableGravatar { + enableGravatar := setting.Config().Picture.EnableGravatar.Value(ctx) + if enableGravatar { // copy GravatarSourceURL, because we will modify its Path. avatarURLCopy := *avatarSetting.gravatarSourceURL avatarURLCopy.Path = path.Join(avatarURLCopy.Path, HashEmail(email)) diff --git a/models/avatars/avatar_test.go b/models/avatars/avatar_test.go index 43a062cc2a117..f1f74f768000f 100644 --- a/models/avatars/avatar_test.go +++ b/models/avatars/avatar_test.go @@ -19,12 +19,14 @@ const gravatarSource = "https://secure.gravatar.com/avatar/" func disableGravatar(t *testing.T) { err := system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.EnableFederatedAvatar.DynKey(): "false"}) assert.NoError(t, err) - err = system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.DisableGravatar.DynKey(): "true"}) + // EnableGravatar.SelectFrom == picture.disable_gravatar for backwards compatibility; .Value will flip correctly but the true value here is counterintuitive + err = system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.EnableGravatar.SelectFromKey(): "true"}) assert.NoError(t, err) } func enableGravatar(t *testing.T) { - err := system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.DisableGravatar.DynKey(): "false"}) + // EnableGravatar.SelectFrom == picture.disable_gravatar for backwards compatibility; .Value will flip correctly but the false value here is counterintuitive + err := system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.EnableGravatar.SelectFromKey(): "false"}) assert.NoError(t, err) setting.GravatarSource = gravatarSource } diff --git a/models/system/setting.go b/models/system/setting.go index 4472b4c22862b..cf6b2dea1acae 100644 --- a/models/system/setting.go +++ b/models/system/setting.go @@ -150,3 +150,34 @@ func (d *dbConfigCachedGetter) InvalidateCache() { func NewDatabaseDynKeyGetter() config.DynKeyGetter { return &dbConfigCachedGetter{} } + +type dbConfigSetter struct { + mu sync.RWMutex +} + +var _ config.DynKeySetter = (*dbConfigSetter)(nil) + +func (d *dbConfigSetter) SetValue(ctx context.Context, dynKey, value string) error { + d.mu.RLock() + defer d.mu.RUnlock() + _ = GetRevision(ctx) // prepare the "revision" key ahead + return db.WithTx(ctx, func(ctx context.Context) error { + e := db.GetEngine(ctx) + res, err := e.Exec("UPDATE system_setting SET version=version+1, setting_value=? WHERE setting_key=?", value, dynKey) + if err != nil { + return err + } + rows, _ := res.RowsAffected() + if rows == 0 { // if no existing row, insert a new row + if _, err = e.Insert(&Setting{SettingKey: dynKey, SettingValue: value}); err != nil { + return err + } + } + + return nil + }) +} + +func NewDatabaseDynKeySetter() config.DynKeySetter { + return &dbConfigSetter{} +} diff --git a/models/user/avatar.go b/models/user/avatar.go index 542bd93b982ce..5fe67aafa7291 100644 --- a/models/user/avatar.go +++ b/models/user/avatar.go @@ -69,12 +69,12 @@ func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string { useLocalAvatar := false autoGenerateAvatar := false - disableGravatar := setting.Config().Picture.DisableGravatar.Value(ctx) + enableGravatar := setting.Config().Picture.EnableGravatar.Value(ctx) switch { - case u.UseCustomAvatar: + case u.UseCustomAvatar, enableGravatar: useLocalAvatar = true - case disableGravatar, setting.OfflineMode: + case setting.OfflineMode: useLocalAvatar = true autoGenerateAvatar = true } diff --git a/modules/setting/config.go b/modules/setting/config.go index 4c5d2df7d8a01..d34f671557777 100644 --- a/modules/setting/config.go +++ b/modules/setting/config.go @@ -11,7 +11,7 @@ import ( ) type PictureStruct struct { - DisableGravatar *config.Value[bool] + EnableGravatar *config.Value[bool] EnableFederatedAvatar *config.Value[bool] } @@ -66,7 +66,7 @@ func initDefaultConfig() { config.SetCfgSecKeyGetter(&cfgSecKeyGetter{}) defaultConfig = &ConfigStruct{ Picture: &PictureStruct{ - DisableGravatar: config.ValueJSON[bool]("picture.disable_gravatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "DISABLE_GRAVATAR"}), + EnableGravatar: config.ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "DISABLE_GRAVATAR"}).Invert(), EnableFederatedAvatar: config.ValueJSON[bool]("picture.enable_federated_avatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "ENABLE_FEDERATED_AVATAR"}), }, Repository: &RepositoryStruct{ diff --git a/modules/setting/config/setter.go b/modules/setting/config/setter.go new file mode 100644 index 0000000000000..f8e63971c241b --- /dev/null +++ b/modules/setting/config/setter.go @@ -0,0 +1,29 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package config + +import ( + "context" + "sync" +) + +var setterMu sync.RWMutex + +type DynKeySetter interface { + SetValue(ctx context.Context, dynKey, value string) error +} + +var dynKeySetterInternal DynKeySetter + +func SetDynSetter(p DynKeySetter) { + setterMu.Lock() + dynKeySetterInternal = p + setterMu.Unlock() +} + +func GetDynSetter() DynKeySetter { + getterMu.RLock() + defer getterMu.RUnlock() + return dynKeySetterInternal +} diff --git a/modules/setting/config/value.go b/modules/setting/config/value.go index 301c60f5e8250..f00a29850e35e 100644 --- a/modules/setting/config/value.go +++ b/modules/setting/config/value.go @@ -19,11 +19,12 @@ type CfgSecKey struct { type Value[T any] struct { mu sync.RWMutex - cfgSecKey CfgSecKey - dynKey string + cfgSecKey CfgSecKey + dynKey, selectFromKey string - def, value T - revision int + def, value T + revision int + flipBoolean bool } func (value *Value[T]) parse(key, valStr string) (v T) { @@ -33,9 +34,41 @@ func (value *Value[T]) parse(key, valStr string) (v T) { log.Error("Unable to unmarshal json config for key %q, err: %v", key, err) } } + + return value.invert(v) +} + +func (value *Value[T]) invertBoolStr(val string) (inverted string) { + if val == "true" { + return "false" + } + + return "true" +} + +func (value *Value[T]) invert(val T) (v T) { + v = val + if value.flipBoolean { + // if value is of type bool + if _, ok := any(val).(bool); ok { + // invert the boolean value upon retrieval + v = any(!any(val).(bool)).(T) + } else { + log.Warn("Ignoring attempt to invert key '%q' for non boolean type", value.selectFromKey) + } + } + return v } +func (value *Value[T]) getKey() string { + if value.selectFromKey != "" { + return value.selectFromKey + } + + return value.dynKey +} + func (value *Value[T]) Value(ctx context.Context) (v T) { dg := GetDynGetter() if dg == nil { @@ -57,7 +90,7 @@ func (value *Value[T]) Value(ctx context.Context) (v T) { // try to parse the config and cache it var valStr *string - if dynVal, has := dg.GetValue(ctx, value.dynKey); has { + if dynVal, has := dg.GetValue(ctx, value.getKey()); has { valStr = &dynVal } else if cfgVal, has := GetCfgSecKeyGetter().GetValue(value.cfgSecKey.Sec, value.cfgSecKey.Key); has { valStr = &cfgVal @@ -79,6 +112,10 @@ func (value *Value[T]) DynKey() string { return value.dynKey } +func (value *Value[T]) SelectFromKey() string { + return value.selectFromKey +} + func (value *Value[T]) WithDefault(def T) *Value[T] { value.def = def return value @@ -93,6 +130,32 @@ func (value *Value[T]) WithFileConfig(cfgSecKey CfgSecKey) *Value[T] { return value } +func (value *Value[bool]) Invert() *Value[bool] { + value.flipBoolean = true + return value +} + +func (value *Value[any]) SelectFrom(sectionName string) *Value[any] { + value.selectFromKey = sectionName + return value +} + +func (value *Value[any]) SetValue(val string) error { + ctx := context.Background() + ds := GetDynSetter() + if ds == nil { + // this is an edge case: the database is not initialized but the system setting is going to be used + // it should panic to avoid inconsistent config values (from config / system setting) and fix the code + panic("no config dyn value getter") + } + + if value.flipBoolean { + return ds.SetValue(ctx, value.getKey(), value.invertBoolStr(val)) + } + + return ds.SetValue(ctx, value.getKey(), val) +} + func ValueJSON[T any](dynKey string) *Value[T] { return &Value[T]{dynKey: dynKey} } diff --git a/modules/setting/config/value_test.go b/modules/setting/config/value_test.go new file mode 100644 index 0000000000000..b23d967eebb6e --- /dev/null +++ b/modules/setting/config/value_test.go @@ -0,0 +1,152 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package config + +import ( + "testing" +) + +func TestValue_parse(t *testing.T) { + tests := []struct { + name string // description of this test case + // Named input parameters for target function. + key string + valStr string + want bool + }{ + { + name: "Parse Invert Retrieval", + key: "picture.disable_gravatar", + valStr: "false", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + value := ValueJSON[bool]("picture.disable_gravatar").Invert() + got := value.parse(tt.key, tt.valStr) + + if got != tt.want { + t.Errorf("parse() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestValue_getKey(t *testing.T) { + tests := []struct { + name string // description of this test case + valueClass *Value[bool] + want string + }{ + { + name: "Custom dynKey name", + valueClass: ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar"), + want: "picture.disable_gravatar", + }, + { + name: "Normal dynKey name", + valueClass: ValueJSON[bool]("picture.disable_gravatar"), + want: "picture.disable_gravatar", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.valueClass.getKey() + + if got != tt.want { + t.Errorf("getKey() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestValue_invert(t *testing.T) { + tests := []struct { + name string // description of this test case + // Named input parameters for target function. + valueClass *Value[bool] + want bool + }{ + { + name: "Invert typed true", + valueClass: ValueJSON[bool]("picture.enable_gravatar").WithDefault(true).Invert(), + want: false, + }, + { + name: "Invert typed false", + valueClass: ValueJSON[bool]("picture.enable_gravatar").WithDefault(false).Invert(), + want: true, + }, + { + name: "Invert typed Does not invert", + valueClass: ValueJSON[bool]("picture.enable_gravatar").WithDefault(false), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.valueClass.invert(tt.valueClass.def) + + if got != tt.want { + t.Errorf("invert() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestValue_invertBoolStr(t *testing.T) { + tests := []struct { + name string // description of this test case + // Named input parameters for target function. + valueClass *Value[bool] + val string + want string + }{ + { + name: "Invert boolean string true", + valueClass: ValueJSON[bool]("picture.enable_gravatar"), + val: "true", + want: "false", + }, + { + name: "Invert boolean string false", + valueClass: ValueJSON[bool]("picture.enable_gravatar"), + val: "false", + want: "true", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.valueClass.invertBoolStr(tt.val) + if got != tt.want { + t.Errorf("invertBoolStr() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestValue_SelectFromKey(t *testing.T) { + tests := []struct { + name string // description of this test case + // Named input parameters for target function. + valueClass *Value[bool] + want string + }{ + { + name: "SelectFrom set and get", + valueClass: ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar"), + want: "picture.disable_gravatar", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.valueClass.SelectFromKey() + + if got != tt.want { + t.Errorf("SelectFromKey() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/modules/setting/picture.go b/modules/setting/picture.go index fafae45babafe..6e4ed4f125436 100644 --- a/modules/setting/picture.go +++ b/modules/setting/picture.go @@ -23,7 +23,7 @@ var ( } GravatarSource string - DisableGravatar bool // Depreciated: migrated to database + EnableGravatar bool // Depreciated: migrated to database EnableFederatedAvatar bool // Depreciated: migrated to database RepoAvatar = struct { @@ -65,9 +65,9 @@ func loadAvatarsFrom(rootCfg ConfigProvider) error { GravatarSource = source } - DisableGravatar = sec.Key("DISABLE_GRAVATAR").MustBool(GetDefaultDisableGravatar()) + EnableGravatar = !sec.Key("DISABLE_GRAVATAR").MustBool(GetDefaultDisableGravatar()) deprecatedSettingDB(rootCfg, "", "DISABLE_GRAVATAR") - EnableFederatedAvatar = sec.Key("ENABLE_FEDERATED_AVATAR").MustBool(GetDefaultEnableFederatedAvatar(DisableGravatar)) + EnableFederatedAvatar = sec.Key("ENABLE_FEDERATED_AVATAR").MustBool(GetDefaultEnableFederatedAvatar(EnableGravatar)) deprecatedSettingDB(rootCfg, "", "ENABLE_FEDERATED_AVATAR") return nil @@ -77,14 +77,12 @@ func GetDefaultDisableGravatar() bool { return OfflineMode } -func GetDefaultEnableFederatedAvatar(disableGravatar bool) bool { +func GetDefaultEnableFederatedAvatar(enableGravatar bool) bool { v := !InstallLock - if OfflineMode { - v = false - } - if disableGravatar { + if OfflineMode || !enableGravatar { v = false } + return v } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 03017ce6746df..ac212dd767ea7 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -316,8 +316,8 @@ mail_notify = Enable Email Notifications server_service_title = Server and Third-Party Service Settings offline_mode = Enable Local Mode offline_mode_popup = Disable third-party content delivery networks and serve all resources locally. -disable_gravatar = Disable Gravatar -disable_gravatar_popup = Disable Gravatar and third-party avatar sources. A default avatar will be used unless a user locally uploads an avatar. +enable_gravatar = Enable Gravatar +enable_gravatar_popup = Enable Gravatar and third-party avatar sources. A default avatar will be used unless a user locally uploads an avatar. federated_avatar_lookup = Enable Federated Avatars federated_avatar_lookup_popup = Enable federated avatar lookup using Libravatar. disable_registration = Disable Self-Registration @@ -3433,7 +3433,7 @@ config.cookie_life_time = Cookie Life Time config.picture_config = Picture and Avatar Configuration config.picture_service = Picture Service -config.disable_gravatar = Disable Gravatar +config.enable_gravatar = Enable Gravatar config.enable_federated_avatar = Enable Federated Avatars config.open_with_editor_app_help = The "Open with" editors for the clone menu. If left empty, the default will be used. Expand to see the default. config.git_guide_remote_name = Repository remote name for git commands in the guide diff --git a/routers/common/db.go b/routers/common/db.go index 01c026142790d..ccf812f228ba0 100644 --- a/routers/common/db.go +++ b/routers/common/db.go @@ -38,6 +38,7 @@ func InitDBEngine(ctx context.Context) (err error) { log.Info("Backing off for %d seconds", int64(setting.Database.DBConnectBackoff/time.Second)) time.Sleep(setting.Database.DBConnectBackoff) } + config.SetDynSetter(system_model.NewDatabaseDynKeySetter()) config.SetDynGetter(system_model.NewDatabaseDynKeyGetter()) return nil } diff --git a/routers/install/install.go b/routers/install/install.go index 4a9dabac6fe87..77ebaee9d1293 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -136,7 +136,7 @@ func Install(ctx *context.Context) { // Server and other services settings form.OfflineMode = setting.OfflineMode - form.DisableGravatar = setting.DisableGravatar // when installing, there is no database connection so that given a default value + form.EnableGravatar = setting.EnableGravatar // when installing, there is no database connection so that given a default value form.EnableFederatedAvatar = setting.EnableFederatedAvatar // when installing, there is no database connection so that given a default value form.EnableOpenIDSignIn = setting.Service.EnableOpenIDSignIn @@ -427,7 +427,8 @@ func SubmitInstall(ctx *context.Context) { cfg.Section("server").Key("OFFLINE_MODE").SetValue(strconv.FormatBool(form.OfflineMode)) if err := system_model.SetSettings(ctx, map[string]string{ - setting.Config().Picture.DisableGravatar.DynKey(): strconv.FormatBool(form.DisableGravatar), + // Form is submitted on install and should use the SelectFrom key for backwards compatibility; getting the value will properly invert the boolean + setting.Config().Picture.EnableGravatar.SelectFromKey(): strconv.FormatBool(!form.EnableGravatar), setting.Config().Picture.EnableFederatedAvatar.DynKey(): strconv.FormatBool(form.EnableFederatedAvatar), }); err != nil { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) diff --git a/routers/web/admin/config.go b/routers/web/admin/config.go index 774b31ab9842a..1e3276e289cf2 100644 --- a/routers/web/admin/config.go +++ b/routers/web/admin/config.go @@ -198,6 +198,10 @@ func ConfigSettings(ctx *context.Context) { func ChangeConfig(ctx *context.Context) { cfg := setting.Config() + subValueSet := map[string]func(string) error{ + cfg.Picture.EnableGravatar.DynKey(): cfg.Picture.EnableGravatar.SetValue, + } + marshalBool := func(v string) ([]byte, error) { b, _ := strconv.ParseBool(v) return json.Marshal(b) @@ -230,8 +234,9 @@ func ChangeConfig(ctx *context.Context) { } return json.Marshal(openWithEditorApps) } + marshallers := map[string]func(string) ([]byte, error){ - cfg.Picture.DisableGravatar.DynKey(): marshalBool, + cfg.Picture.EnableGravatar.DynKey(): marshalBool, cfg.Picture.EnableFederatedAvatar.DynKey(): marshalBool, cfg.Repository.OpenWithEditorApps.DynKey(): marshalOpenWithApps, cfg.Repository.GitGuideRemoteName.DynKey(): marshalString(cfg.Repository.GitGuideRemoteName.DefaultValue()), @@ -260,7 +265,15 @@ loop: ctx.JSONError(ctx.Tr("admin.config.set_setting_failed", key)) break loop } - configSettings[key] = string(marshaledValue) + + if setter, ok := subValueSet[key]; ok { + if err := setter(string(marshaledValue)); err != nil { + ctx.JSONError(ctx.Tr("admin.config.set_setting_failed", key)) + break loop + } + } else { + configSettings[key] = string(marshaledValue) + } } if ctx.Written() { return diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 27577cd35ba1c..54414ba707d30 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -316,7 +316,7 @@ func editUserCommon(ctx *context.Context) { ctx.Data["DisableGitHooks"] = setting.DisableGitHooks ctx.Data["DisableImportLocal"] = !setting.ImportLocalPaths ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() - ctx.Data["DisableGravatar"] = setting.Config().Picture.DisableGravatar.Value(ctx) + ctx.Data["EnableGravatar"] = setting.Config().Picture.EnableGravatar.Value(ctx) } // EditUser show editing user page diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index 98995cd69c435..2a5729ff33580 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -47,7 +47,7 @@ func Profile(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("settings.profile") ctx.Data["PageIsSettingsProfile"] = true ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() - ctx.Data["DisableGravatar"] = setting.Config().Picture.DisableGravatar.Value(ctx) + ctx.Data["EnableGravatar"] = setting.Config().Picture.EnableGravatar.Value(ctx) ctx.Data["UserDisabledFeatures"] = user_model.DisabledFeaturesWithLoginType(ctx.Doer) @@ -59,7 +59,7 @@ func ProfilePost(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("settings") ctx.Data["PageIsSettingsProfile"] = true ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() - ctx.Data["DisableGravatar"] = setting.Config().Picture.DisableGravatar.Value(ctx) + ctx.Data["EnableGravatar"] = setting.Config().Picture.EnableGravatar.Value(ctx) ctx.Data["UserDisabledFeatures"] = user_model.DisabledFeaturesWithLoginType(ctx.Doer) if ctx.HasError() { diff --git a/services/forms/user_form.go b/services/forms/user_form.go index 618294d43412f..77665fd8f9de8 100644 --- a/services/forms/user_form.go +++ b/services/forms/user_form.go @@ -46,7 +46,7 @@ type InstallForm struct { MailNotify bool OfflineMode bool - DisableGravatar bool + EnableGravatar bool EnableFederatedAvatar bool EnableOpenIDSignIn bool EnableOpenIDSignUp bool diff --git a/templates/admin/config_settings/avatars.tmpl b/templates/admin/config_settings/avatars.tmpl index 1fc761034d402..5ff972bea8cfa 100644 --- a/templates/admin/config_settings/avatars.tmpl +++ b/templates/admin/config_settings/avatars.tmpl @@ -3,10 +3,10 @@