diff --git a/backend/internal/handler/admin/setting_handler_partial_payload_test.go b/backend/internal/handler/admin/setting_handler_partial_payload_test.go new file mode 100644 index 0000000000..e39e664a3e --- /dev/null +++ b/backend/internal/handler/admin/setting_handler_partial_payload_test.go @@ -0,0 +1,67 @@ +//go:build unit + +package admin + +import ( + "net/http" + "testing" + + "github.com/Wei-Shaw/sub2api/internal/service" + + "github.com/stretchr/testify/require" +) + +// Saving settings is a whole-document PUT. A client that sends only the field it +// cares about must not reset everything else: a payload as small as +// `{"risk_control_enabled":true}` used to clear site_name, after which +// getStringOrDefault rendered the empty value as the built-in default and the +// login page silently changed name. + +func TestUpdateSettingsPartialPayloadKeepsUnsentKeys(t *testing.T) { + h, repo := newStepUpSwitchTestHandler(t, map[string]string{ + service.SettingKeySiteName: "Example Gateway", + service.SettingKeySiteSubtitle: "Example Gateway Platform", + service.SettingKeySMTPHost: "smtp.example.com", + service.SettingKeySMTPFrom: "noreply@example.com", + service.SettingKeyTurnstileEnabled: "true", + }) + + rec := doUpdateSettings(t, h, map[string]any{"risk_control_enabled": true}, nil) + require.Equal(t, http.StatusOK, rec.Code) + + require.Equal(t, "true", repo.values[service.SettingKeyRiskControlEnabled], + "the field the caller actually sent must be written") + + require.Equal(t, "Example Gateway", repo.values[service.SettingKeySiteName]) + require.Equal(t, "Example Gateway Platform", repo.values[service.SettingKeySiteSubtitle]) + require.Equal(t, "smtp.example.com", repo.values[service.SettingKeySMTPHost]) + require.Equal(t, "noreply@example.com", repo.values[service.SettingKeySMTPFrom]) + require.Equal(t, "true", repo.values[service.SettingKeyTurnstileEnabled]) +} + +// A full payload keeps whole-document semantics: fields explicitly set to their +// zero value are still cleared. +func TestUpdateSettingsFullPayloadStillClearsSentEmptyFields(t *testing.T) { + h, repo := newStepUpSwitchTestHandler(t, map[string]string{ + service.SettingKeySiteName: "Example Gateway", + }) + + rec := doUpdateSettings(t, h, map[string]any{"site_name": ""}, nil) + require.Equal(t, http.StatusOK, rec.Code) + + require.Equal(t, "", repo.values[service.SettingKeySiteName], + "an explicitly sent empty value is a deliberate clear, not an omission") +} + +// smtp_from_email is the one request field whose JSON name differs from its +// setting key; the alias keeps it from being treated as always-omitted. +func TestUpdateSettingsSMTPFromAliasIsWritable(t *testing.T) { + h, repo := newStepUpSwitchTestHandler(t, map[string]string{ + service.SettingKeySMTPFrom: "old@example.com", + }) + + rec := doUpdateSettings(t, h, map[string]any{"smtp_from_email": "new@example.com"}, nil) + require.Equal(t, http.StatusOK, rec.Code) + + require.Equal(t, "new@example.com", repo.values[service.SettingKeySMTPFrom]) +} diff --git a/backend/internal/handler/admin/setting_handler_update.go b/backend/internal/handler/admin/setting_handler_update.go index f852bb7e6f..ec910b5256 100644 --- a/backend/internal/handler/admin/setting_handler_update.go +++ b/backend/internal/handler/admin/setting_handler_update.go @@ -6,6 +6,7 @@ import ( "errors" "log/slog" "net/http" + "reflect" "strings" "github.com/Wei-Shaw/sub2api/internal/config" @@ -15,6 +16,7 @@ import ( "github.com/Wei-Shaw/sub2api/internal/service" "github.com/gin-gonic/gin" + "github.com/gin-gonic/gin/binding" ) // UpdateSettingsRequest 更新设置请求 @@ -375,12 +377,70 @@ func (h *SettingHandler) ensureActorTotpForStepUp(c *gin.Context) bool { return true } +// settingKeyJSONAliases covers the request fields whose JSON name differs from +// the setting key they persist to. Every other field of UpdateSettingsRequest +// is named after its setting key. +var settingKeyJSONAliases = map[string]string{ + "smtp_from_email": service.SettingKeySMTPFrom, +} + +// settingKeyByJSONName maps the value-typed top-level JSON fields of +// UpdateSettingsRequest to the setting key each one writes. Resolved once from +// the struct tags so new fields are covered without touching this file. +// +// Pointer-typed fields are deliberately excluded: they already carry their own +// "omitted = keep the stored value" merge in UpdateSettings, and some of them +// rely on being rewritten on every save to re-normalize fail-closed security +// state (see TestUpdateSettingsMalformedForwardedClientIPHeadersRemainFailClosedWhenOmitted). +// Only the value-typed fields are indistinguishable from a deliberate clear. +var settingKeyByJSONName = buildSettingKeyByJSONName() + +func buildSettingKeyByJSONName() map[string]string { + t := reflect.TypeOf(UpdateSettingsRequest{}) + out := make(map[string]string, t.NumField()) + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + if field.Type.Kind() == reflect.Ptr { + continue + } + name, _, _ := strings.Cut(field.Tag.Get("json"), ",") + if name == "" || name == "-" { + continue + } + if alias, ok := settingKeyJSONAliases[name]; ok { + out[name] = alias + continue + } + out[name] = name + } + return out +} + +// omittedSettingKeys reports the setting keys this payload never mentioned. +// Saving settings is a whole-document PUT, so without this a client that sends +// only the one field it cares about resets every other field to a zero value. +func omittedSettingKeys(sentFields map[string]json.RawMessage) service.OmittedSettingKeys { + omitted := make(service.OmittedSettingKeys, len(settingKeyByJSONName)) + for jsonName, settingKey := range settingKeyByJSONName { + if _, sent := sentFields[jsonName]; !sent { + omitted[settingKey] = struct{}{} + } + } + return omitted +} + func (h *SettingHandler) UpdateSettings(c *gin.Context) { - var req UpdateSettingsRequest - if err := c.ShouldBindJSON(&req); err != nil { + var sentFields map[string]json.RawMessage + if err := c.ShouldBindBodyWith(&sentFields, binding.JSON); err != nil { response.BadRequest(c, "Invalid request: "+err.Error()) return } + var req UpdateSettingsRequest + if err := c.ShouldBindBodyWith(&req, binding.JSON); err != nil { + response.BadRequest(c, "Invalid request: "+err.Error()) + return + } + omitted := omittedSettingKeys(sentFields) previousSettings, err := h.settingService.GetAllSettings(c.Request.Context()) if err != nil { @@ -1692,7 +1752,7 @@ func (h *SettingHandler) UpdateSettings(c *gin.Context) { }, ForceEmailOnThirdPartySignup: boolValueOrDefault(req.ForceEmailOnThirdPartySignup, previousAuthSourceDefaults.ForceEmailOnThirdPartySignup), } - if err := h.settingService.UpdateSettingsWithAuthSourceDefaults(c.Request.Context(), settings, authSourceDefaults); err != nil { + if err := h.settingService.UpdateSettingsWithAuthSourceDefaultsOmitting(c.Request.Context(), settings, authSourceDefaults, omitted); err != nil { response.ErrorFrom(c, err) return } diff --git a/backend/internal/service/setting_update.go b/backend/internal/service/setting_update.go index 7449a3de69..2364b89484 100644 --- a/backend/internal/service/setting_update.go +++ b/backend/internal/service/setting_update.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "math" "strconv" "strings" @@ -15,22 +16,50 @@ import ( infraerrors "github.com/Wei-Shaw/sub2api/internal/pkg/errors" ) +// OmittedSettingKeys marks setting keys the caller's payload never carried. +// SystemSettings is a plain struct, so a field the caller omitted arrives as a +// zero value and is indistinguishable from a deliberate clear. Listing the key +// here drops it from the write, leaving the stored value in place. +// +// A nil or empty set keeps whole-document semantics: every key is written. +type OmittedSettingKeys map[string]struct{} + +func (o OmittedSettingKeys) dropFrom(updates map[string]string) { + for key := range o { + delete(updates, key) + } +} + // UpdateSettings 更新系统设置 func (s *SettingService) UpdateSettings(ctx context.Context, settings *SystemSettings) error { + return s.UpdateSettingsOmitting(ctx, settings, nil) +} + +// UpdateSettingsOmitting persists system settings, leaving the keys in omitted +// at their stored value. +func (s *SettingService) UpdateSettingsOmitting(ctx context.Context, settings *SystemSettings, omitted OmittedSettingKeys) error { updates, err := s.buildSystemSettingsUpdates(ctx, settings) if err != nil { return err } + omitted.dropFrom(updates) - err = s.settingRepo.SetMultiple(ctx, updates) - if err == nil { - s.refreshCachedSettings(settings) + if err := s.settingRepo.SetMultiple(ctx, updates); err != nil { + return err } - return err + s.refreshCachedSettingsAfterWrite(ctx, settings, omitted) + return nil } // UpdateSettingsWithAuthSourceDefaults persists system settings and auth-source defaults in a single write. func (s *SettingService) UpdateSettingsWithAuthSourceDefaults(ctx context.Context, settings *SystemSettings, authDefaults *AuthSourceDefaultSettings) error { + return s.UpdateSettingsWithAuthSourceDefaultsOmitting(ctx, settings, authDefaults, nil) +} + +// UpdateSettingsWithAuthSourceDefaultsOmitting persists system settings and +// auth-source defaults in a single write, leaving the keys in omitted at their +// stored value. +func (s *SettingService) UpdateSettingsWithAuthSourceDefaultsOmitting(ctx context.Context, settings *SystemSettings, authDefaults *AuthSourceDefaultSettings, omitted OmittedSettingKeys) error { updates, err := s.buildSystemSettingsUpdates(ctx, settings) if err != nil { return err @@ -43,12 +72,30 @@ func (s *SettingService) UpdateSettingsWithAuthSourceDefaults(ctx context.Contex for key, value := range authSourceUpdates { updates[key] = value } + omitted.dropFrom(updates) - err = s.settingRepo.SetMultiple(ctx, updates) - if err == nil { - s.refreshCachedSettings(settings) + if err := s.settingRepo.SetMultiple(ctx, updates); err != nil { + return err } - return err + s.refreshCachedSettingsAfterWrite(ctx, settings, omitted) + return nil +} + +// refreshCachedSettingsAfterWrite keeps the in-process caches in step with the +// write that just landed. A partial payload carries zero values for the fields +// it omitted, so in that case the caches are rebuilt from storage rather than +// from the request struct. +func (s *SettingService) refreshCachedSettingsAfterWrite(ctx context.Context, settings *SystemSettings, omitted OmittedSettingKeys) { + if len(omitted) == 0 { + s.refreshCachedSettings(settings) + return + } + stored, err := s.GetAllSettings(ctx) + if err != nil { + slog.Warn("refresh cached settings after partial update failed", "error", err) + return + } + s.refreshCachedSettings(stored) } func (s *SettingService) buildSystemSettingsUpdates(ctx context.Context, settings *SystemSettings) (map[string]string, error) {