Merge 52f195a6c8 into 076cd214fe

pkg/migration/20260627101958.go

@@ -0,0 +1,125 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present Vikunja and contributors. All rights reserved.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package migration
+
+import (
+	"bytes"
+	"encoding/base64"
+	"encoding/json"
+	"strings"
+
+	"src.techknowlogick.com/xormigrate"
+	"xorm.io/xorm"
+	"xorm.io/xorm/schemas"
+)
+
+// usersFrontendSettings20260627101958 reads the raw frontend_settings JSON text.
+// The json tag makes xorm hand back the stored column text verbatim instead of
+// decoding it into a typed value.
+type usersFrontendSettings20260627101958 struct {
+	ID               int64  `xorm:"bigint autoincr not null unique pk"`
+	FrontendSettings string `xorm:"frontend_settings json null"`
+}
+
+func (usersFrontendSettings20260627101958) TableName() string {
+	return "users"
+}
+
+// legacyFrontendSettingsWhereClause20260627101958 selects rows whose
+// frontend_settings is a JSON string ('"…"'). Healthy values are JSON objects
+// ('{…}'); only the legacy double-encoded values are stored as JSON strings.
+func legacyFrontendSettingsWhereClause20260627101958(dbType schemas.DBType) string {
+	if dbType == schemas.POSTGRES {
+		return `frontend_settings IS NOT NULL AND frontend_settings::text LIKE '"%'`
+	}
+	return `frontend_settings IS NOT NULL AND frontend_settings LIKE '"%'`
+}
+
+// repairLegacyFrontendSettings20260627101958 reverses the historical double
+// encoding of frontend_settings. The pre-fix UpdateUser stored
+// json.Marshal(FrontendSettings) back into the interface field, so xorm
+// base64-encoded the resulting []byte: a nil value became the JSON string
+// "bnVsbA==" (base64 of "null") and a real settings object became the base64 of
+// its JSON.
+//
+// changed reports whether a repair applies; setNull reports the value should
+// become SQL NULL; value carries the repaired JSON object otherwise. Only values
+// that base64-decode to JSON null or a JSON object are touched, so a legitimate
+// string-valued setting is never rewritten.
+func repairLegacyFrontendSettings20260627101958(raw string) (value string, setNull, changed bool) {
+	trimmed := strings.TrimSpace(raw)
+	if len(trimmed) == 0 || trimmed[0] != '"' {
+		return "", false, false
+	}
+
+	var inner string
+	if err := json.Unmarshal([]byte(trimmed), &inner); err != nil {
+		return "", false, false
+	}
+
+	decoded, err := base64.StdEncoding.DecodeString(inner)
+	if err != nil {
+		return "", false, false
+	}
+
+	decoded = bytes.TrimSpace(decoded)
+	if bytes.Equal(decoded, []byte("null")) {
+		return "", true, true
+	}
+	if len(decoded) > 0 && decoded[0] == '{' && json.Valid(decoded) {
+		return string(decoded), false, true
+	}
+	return "", false, false
+}
+
+func init() {
+	migrations = append(migrations, &xormigrate.Migration{
+		ID:          "20260627101958",
+		Description: "repair double-encoded frontend_settings stored before UpdateUser stopped re-marshalling the column",
+		Migrate: func(tx *xorm.Engine) error {
+			users := []*usersFrontendSettings20260627101958{}
+			if err := tx.
+				Where(legacyFrontendSettingsWhereClause20260627101958(tx.Dialect().URI().DBType)).
+				Find(&users); err != nil {
+				return err
+			}
+
+			for _, u := range users {
+				value, setNull, changed := repairLegacyFrontendSettings20260627101958(u.FrontendSettings)
+				if !changed {
+					continue
+				}
+
+				if setNull {
+					if _, err := tx.Exec("UPDATE users SET frontend_settings = NULL WHERE id = ?", u.ID); err != nil {
+						return err
+					}
+					continue
+				}
+
+				if _, err := tx.Exec("UPDATE users SET frontend_settings = ? WHERE id = ?", value, u.ID); err != nil {
+					return err
+				}
+			}
+
+			return nil
+		},
+		Rollback: func(tx *xorm.Engine) error {
+			return nil
+		},
+	})
+}

pkg/migration/20260627101958_test.go

@@ -0,0 +1,103 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present Vikunja and contributors. All rights reserved.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package migration
+
+import (
+	"encoding/base64"
+	"testing"
+
+	"xorm.io/xorm/schemas"
+)
+
+func legacyFrontendSettingsRaw20260627101958(jsonValue string) string {
+	return `"` + base64.StdEncoding.EncodeToString([]byte(jsonValue)) + `"`
+}
+
+func TestRepairLegacyFrontendSettings20260627101958(t *testing.T) {
+	tests := []struct {
+		name       string
+		raw        string
+		wantValue  string
+		wantNull   bool
+		wantChange bool
+	}{
+		{
+			name:       "legacy null heals to NULL",
+			raw:        legacyFrontendSettingsRaw20260627101958("null"),
+			wantNull:   true,
+			wantChange: true,
+		},
+		{
+			name:       "legacy object is decoded back to the object",
+			raw:        legacyFrontendSettingsRaw20260627101958(`{"color_schema":"dark"}`),
+			wantValue:  `{"color_schema":"dark"}`,
+			wantChange: true,
+		},
+		{
+			name:       "healthy object is left untouched",
+			raw:        `{"color_schema":"dark"}`,
+			wantChange: false,
+		},
+		{
+			name:       "non-base64 string is left untouched",
+			raw:        `"hello world"`,
+			wantChange: false,
+		},
+		{
+			name:       "base64 of a scalar is left untouched",
+			raw:        legacyFrontendSettingsRaw20260627101958("123"),
+			wantChange: false,
+		},
+		{
+			name:       "base64 of an array is left untouched",
+			raw:        legacyFrontendSettingsRaw20260627101958("[]"),
+			wantChange: false,
+		},
+		{
+			name:       "empty value is left untouched",
+			raw:        "",
+			wantChange: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			value, setNull, changed := repairLegacyFrontendSettings20260627101958(tt.raw)
+			if changed != tt.wantChange {
+				t.Fatalf("changed = %v, want %v", changed, tt.wantChange)
+			}
+			if setNull != tt.wantNull {
+				t.Fatalf("setNull = %v, want %v", setNull, tt.wantNull)
+			}
+			if value != tt.wantValue {
+				t.Fatalf("value = %q, want %q", value, tt.wantValue)
+			}
+		})
+	}
+}
+
+func TestLegacyFrontendSettingsWhereClause20260627101958(t *testing.T) {
+	postgres := legacyFrontendSettingsWhereClause20260627101958(schemas.POSTGRES)
+	if want := `frontend_settings IS NOT NULL AND frontend_settings::text LIKE '"%'`; postgres != want {
+		t.Fatalf("postgres clause\nwant: %s\ngot:  %s", want, postgres)
+	}
+
+	other := legacyFrontendSettingsWhereClause20260627101958(schemas.SQLITE)
+	if want := `frontend_settings IS NOT NULL AND frontend_settings LIKE '"%'`; other != want {
+		t.Fatalf("default clause\nwant: %s\ngot:  %s", want, other)
+	}
+}

pkg/user/user.go

@@ -633,34 +633,15 @@ func UpdateUser(s *xorm.Session, user *User, forceOverride bool) (updatedUser *U
 		return nil, &ErrInvalidTimezone{Name: user.Timezone, LoadError: err}
 	}
 
-	frontendSettingsJSON, err := json.Marshal(user.FrontendSettings)
+	cols, err := userUpdateColumns(user, forceOverride)
 	if err != nil {
 		return nil, err
 	}
-	user.FrontendSettings = frontendSettingsJSON
 
 	// Update it
 	_, err = s.
 		ID(user.ID).
-		Cols(
-			"username",
-			"email",
-			"avatar_provider",
-			"avatar_file_id",
-			"status",
-			"name",
-			"email_reminders_enabled",
-			"discoverable_by_name",
-			"discoverable_by_email",
-			"overdue_tasks_reminders_enabled",
-			"default_project_id",
-			"week_start",
-			"language",
-			"timezone",
-			"overdue_tasks_reminders_time",
-			"frontend_settings",
-			"extra_settings_links",
-		).
+		Cols(cols...).
 		Update(user)
 	if err != nil {
 		return &User{}, err
@@ -675,6 +656,52 @@ func UpdateUser(s *xorm.Session, user *User, forceOverride bool) (updatedUser *U
 	return updatedUser, err
 }
 
+// userUpdateColumns lists the columns UpdateUser persists and, for settings
+// updates, normalises frontend_settings into the JSON-string form xorm stores
+// correctly.
+//
+// frontend_settings is written only when forceOverride is set, which the
+// user-settings endpoints pass and which no other caller does. Skipping the
+// column elsewhere is deliberate: it is set on no other path, and listing a nil
+// interface in Cols makes xorm write NULL, wiping the user's settings on every
+// OIDC login. We marshal the value to a string ourselves because xorm passes a
+// Go map in an interface{} json column straight to the driver (an error) and
+// base64-encodes a []byte (the historical double-encoding bug).
+func userUpdateColumns(user *User, forceOverride bool) ([]string, error) {
+	cols := []string{
+		"username",
+		"email",
+		"avatar_provider",
+		"avatar_file_id",
+		"status",
+		"name",
+		"email_reminders_enabled",
+		"discoverable_by_name",
+		"discoverable_by_email",
+		"overdue_tasks_reminders_enabled",
+		"default_project_id",
+		"week_start",
+		"language",
+		"timezone",
+		"overdue_tasks_reminders_time",
+		"extra_settings_links",
+	}
+
+	if !forceOverride {
+		return cols, nil
+	}
+
+	if user.FrontendSettings != nil {
+		frontendSettingsJSON, err := json.Marshal(user.FrontendSettings)
+		if err != nil {
+			return nil, fmt.Errorf("marshal frontend settings: %w", err)
+		}
+		user.FrontendSettings = string(frontendSettingsJSON)
+	}
+
+	return append(cols, "frontend_settings"), nil
+}
+
 func SetUserStatus(s *xorm.Session, user *User, status Status) (err error) {
 	_, err = s.Where("id = ?", user.ID).
 		Cols("status").

pkg/user/user_test.go

@@ -18,6 +18,8 @@ package user
 
 import (
 	"context"
+	"database/sql"
+	"encoding/json"
 	"testing"
 
 	"code.vikunja.io/api/pkg/db"
@@ -475,6 +477,103 @@ func TestUpdateUser(t *testing.T) {
 		require.Error(t, err)
 		assert.True(t, IsErrUserDoesNotExist(err))
 	})
+	t.Run("frontend settings survive profile-only update", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		originalSettings := map[string]any{
+			"color_schema": "dark",
+		}
+		settingsJSON, err := json.Marshal(originalSettings)
+		require.NoError(t, err)
+		_, err = s.Table("users").
+			Where("id = ?", 1).
+			Cols("frontend_settings").
+			Update(&struct {
+				FrontendSettings string `xorm:"frontend_settings"`
+			}{
+				FrontendSettings: string(settingsJSON),
+			})
+		require.NoError(t, err)
+
+		updated, err := UpdateUser(s, &User{
+			ID:    1,
+			Email: "testing@example.com",
+		}, false)
+		require.NoError(t, err)
+		require.Equal(t, map[string]interface{}(originalSettings), updated.FrontendSettings)
+
+		var stored sql.NullString
+		has, err := s.Table("users").
+			Where("id = ?", 1).
+			Cols("frontend_settings").
+			Get(&stored)
+		require.NoError(t, err)
+		require.True(t, has)
+		require.True(t, stored.Valid)
+		assert.JSONEq(t, string(settingsJSON), stored.String)
+	})
+	t.Run("frontend settings can be saved from request map", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		frontendSettings := map[string]any{
+			"color_schema": "dark",
+			"nested": map[string]any{
+				"a": float64(1),
+			},
+		}
+
+		updated, err := UpdateUser(s, &User{
+			ID:               1,
+			FrontendSettings: frontendSettings,
+		}, true)
+		require.NoError(t, err)
+		require.Equal(t, frontendSettings, updated.FrontendSettings)
+
+		var stored sql.NullString
+		has, err := s.Table("users").
+			Where("id = ?", 1).
+			Cols("frontend_settings").
+			Get(&stored)
+		require.NoError(t, err)
+		require.True(t, has)
+		require.True(t, stored.Valid)
+		assert.JSONEq(t, `{"color_schema":"dark","nested":{"a":1}}`, stored.String)
+	})
+	t.Run("frontend settings can be cleared", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		_, err := s.Table("users").
+			Where("id = ?", 1).
+			Cols("frontend_settings").
+			Update(&struct {
+				FrontendSettings string `xorm:"frontend_settings"`
+			}{
+				FrontendSettings: `{"color_schema":"dark"}`,
+			})
+		require.NoError(t, err)
+
+		updated, err := UpdateUser(s, &User{
+			ID:               1,
+			FrontendSettings: nil,
+		}, true)
+		require.NoError(t, err)
+		require.Nil(t, updated.FrontendSettings)
+
+		var stored sql.NullString
+		has, err := s.Table("users").
+			Where("id = ?", 1).
+			Cols("frontend_settings").
+			Get(&stored)
+		require.NoError(t, err)
+		require.True(t, has)
+		assert.False(t, stored.Valid)
+	})
 }
 
 func TestUpdateUserPassword(t *testing.T) {