Browse Source

fix(clients): persist all editable fields for clients with no inbound (#6053)

ClientService.Update only wrote most editable fields to the clients table
inside the per-inbound loop (UpdateInboundClient -> SyncInbound), so a
client with no attached inbound — the external-links / remote-
subscription client — silently dropped subId, totalGB, expiryTime,
limitIp, tgId, comment, reset, flow, security and the credential fields
on edit. Update still returned success, so the panel showed a saved
toast while the row was untouched. email/enable/group_name/ad_tag/reverse
already had dedicated unconditional direct writes that covered the
no-inbound case; the rest did not.

This was a known failure shape: commit c4448f4e added the no-inbound
fallback for email only, and TestUpdate_PersistsRecordEnable_NoInbound
covered enable only — both stayed scoped to a single field.

Extract SyncInbound's per-row field merge into applyClientRecordMerge
(unconditional for scalar quota/lifecycle/subscription fields,
preserve-when-empty for credentials/identifiers, earliest CreatedAt)
and reuse it from Update. Update's new branch runs only when the client
has no inbound, so it fixes the gap without altering the inbound-attached
path (where flow is per-inbound gated via clientWithInboundFlow and
SyncInbound already persists every field). email/reverse/group/ad_tag/
enable keep their existing dedicated writes; the new write covers the
remaining columns.

Fixes #5981
H-TTTTT 17 hours ago
parent
commit
d623410cf4

+ 29 - 0
internal/web/service/client_crud.go

@@ -433,6 +433,35 @@ func (s *ClientService) Update(inboundSvc *InboundService, id int, updated model
 		}
 	}
 
+	if len(inboundIds) == 0 {
+		merged := *existing
+		applyClientRecordMerge(&merged, updated.ToRecord())
+		if err := database.GetDB().Model(&model.ClientRecord{}).
+			Where("id = ?", id).
+			Updates(map[string]any{
+				"sub_id":            merged.SubID,
+				"uuid":              merged.UUID,
+				"password":          merged.Password,
+				"auth":              merged.Auth,
+				"secret":            merged.Secret,
+				"flow":              merged.Flow,
+				"security":          merged.Security,
+				"wg_private_key":    merged.PrivateKey,
+				"wg_public_key":     merged.PublicKey,
+				"wg_allowed_ips":    merged.AllowedIPs,
+				"wg_pre_shared_key": merged.PreSharedKey,
+				"wg_keep_alive":     merged.KeepAlive,
+				"limit_ip":          merged.LimitIP,
+				"total_gb":          merged.TotalGB,
+				"expiry_time":       merged.ExpiryTime,
+				"tg_id":             merged.TgID,
+				"comment":           merged.Comment,
+				"reset":             merged.Reset,
+			}).Error; err != nil {
+			return needRestart, err
+		}
+	}
+
 	reverseStr := ""
 	if updated.Reverse != nil && strings.TrimSpace(updated.Reverse.Tag) != "" {
 		if b, mErr := json.Marshal(updated.Reverse); mErr == nil {

+ 60 - 47
internal/web/service/client_link.go

@@ -9,6 +9,65 @@ import (
 	"gorm.io/gorm"
 )
 
+// applyClientRecordMerge merges incoming client-record fields onto row using the
+// same rules everywhere a client record is persisted: scalar quota / lifecycle /
+// subscription fields are applied unconditionally (so clearing them takes
+// effect), while credentials and identifiers are only overwritten when the
+// incoming value is non-empty (so a partial update preserves the stored UUID /
+// password / keys). CreatedAt keeps the earliest known value. Email, UpdatedAt,
+// and the Id primary key are intentionally not touched here — callers handle
+// those separately. Shared by SyncInbound (per-inbound persistence) and Update
+// (the no-attached-inbound fallback) so the two paths cannot diverge.
+func applyClientRecordMerge(row *model.ClientRecord, incoming *model.ClientRecord) {
+	if incoming.UUID != "" {
+		row.UUID = incoming.UUID
+	}
+	if incoming.Password != "" {
+		row.Password = incoming.Password
+	}
+	if incoming.Auth != "" {
+		row.Auth = incoming.Auth
+	}
+	if incoming.Secret != "" {
+		row.Secret = incoming.Secret
+	}
+	if incoming.AdTag != "" {
+		row.AdTag = incoming.AdTag
+	}
+	row.Flow = incoming.Flow
+	if incoming.Security != "" {
+		row.Security = incoming.Security
+	}
+	if incoming.Reverse != "" {
+		row.Reverse = incoming.Reverse
+	}
+	if incoming.PrivateKey != "" {
+		row.PrivateKey = incoming.PrivateKey
+	}
+	if incoming.PublicKey != "" {
+		row.PublicKey = incoming.PublicKey
+	}
+	if incoming.AllowedIPs != "" {
+		row.AllowedIPs = incoming.AllowedIPs
+	}
+	row.PreSharedKey = incoming.PreSharedKey
+	row.KeepAlive = incoming.KeepAlive
+	row.SubID = incoming.SubID
+	row.LimitIP = incoming.LimitIP
+	row.TotalGB = incoming.TotalGB
+	row.ExpiryTime = incoming.ExpiryTime
+	row.Enable = incoming.Enable
+	row.TgID = incoming.TgID
+	if incoming.Group != "" {
+		row.Group = incoming.Group
+	}
+	row.Comment = incoming.Comment
+	row.Reset = incoming.Reset
+	if incoming.CreatedAt > 0 && (row.CreatedAt == 0 || incoming.CreatedAt < row.CreatedAt) {
+		row.CreatedAt = incoming.CreatedAt
+	}
+}
+
 func (s *ClientService) SyncInbound(tx *gorm.DB, inboundId int, clients []model.Client) error {
 	if tx == nil {
 		tx = database.GetDB()
@@ -66,53 +125,7 @@ func (s *ClientService) SyncInbound(tx *gorm.DB, inboundId int, clients []model.
 		}
 
 		before := *row
-		if incoming.UUID != "" {
-			row.UUID = incoming.UUID
-		}
-		if incoming.Password != "" {
-			row.Password = incoming.Password
-		}
-		if incoming.Auth != "" {
-			row.Auth = incoming.Auth
-		}
-		if incoming.Secret != "" {
-			row.Secret = incoming.Secret
-		}
-		if incoming.AdTag != "" {
-			row.AdTag = incoming.AdTag
-		}
-		row.Flow = incoming.Flow
-		if incoming.Security != "" {
-			row.Security = incoming.Security
-		}
-		if incoming.Reverse != "" {
-			row.Reverse = incoming.Reverse
-		}
-		if incoming.PrivateKey != "" {
-			row.PrivateKey = incoming.PrivateKey
-		}
-		if incoming.PublicKey != "" {
-			row.PublicKey = incoming.PublicKey
-		}
-		if incoming.AllowedIPs != "" {
-			row.AllowedIPs = incoming.AllowedIPs
-		}
-		row.PreSharedKey = incoming.PreSharedKey
-		row.KeepAlive = incoming.KeepAlive
-		row.SubID = incoming.SubID
-		row.LimitIP = incoming.LimitIP
-		row.TotalGB = incoming.TotalGB
-		row.ExpiryTime = incoming.ExpiryTime
-		row.Enable = incoming.Enable
-		row.TgID = incoming.TgID
-		if incoming.Group != "" {
-			row.Group = incoming.Group
-		}
-		row.Comment = incoming.Comment
-		row.Reset = incoming.Reset
-		if incoming.CreatedAt > 0 && (row.CreatedAt == 0 || incoming.CreatedAt < row.CreatedAt) {
-			row.CreatedAt = incoming.CreatedAt
-		}
+		applyClientRecordMerge(row, incoming)
 		preservedUpdatedAt := max(incoming.UpdatedAt, row.UpdatedAt)
 		row.UpdatedAt = preservedUpdatedAt
 

+ 205 - 0
internal/web/service/client_update_no_inbound_test.go

@@ -0,0 +1,205 @@
+package service
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/database"
+	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
+)
+
+func TestUpdate_PersistsFields_NoInbound(t *testing.T) {
+	cases := []struct {
+		name     string
+		mutate   func(c *model.Client)
+		readBack func(rec *model.ClientRecord) any
+		want     any
+	}{
+		{
+			name:     "subId",
+			mutate:   func(c *model.Client) { c.SubID = "new-sub-id" },
+			readBack: func(rec *model.ClientRecord) any { return rec.SubID },
+			want:     "new-sub-id",
+		},
+		{
+			name:     "totalGB cleared to zero",
+			mutate:   func(c *model.Client) { c.TotalGB = 0 },
+			readBack: func(rec *model.ClientRecord) any { return rec.TotalGB },
+			want:     int64(0),
+		},
+		{
+			name:     "expiryTime",
+			mutate:   func(c *model.Client) { c.ExpiryTime = 1700000000 },
+			readBack: func(rec *model.ClientRecord) any { return rec.ExpiryTime },
+			want:     int64(1700000000),
+		},
+		{
+			name:     "limitIp",
+			mutate:   func(c *model.Client) { c.LimitIP = 7 },
+			readBack: func(rec *model.ClientRecord) any { return rec.LimitIP },
+			want:     7,
+		},
+		{
+			name:     "tgId",
+			mutate:   func(c *model.Client) { c.TgID = 9876543210 },
+			readBack: func(rec *model.ClientRecord) any { return rec.TgID },
+			want:     int64(9876543210),
+		},
+		{
+			name:     "comment cleared to empty",
+			mutate:   func(c *model.Client) { c.Comment = "" },
+			readBack: func(rec *model.ClientRecord) any { return rec.Comment },
+			want:     "",
+		},
+		{
+			name:     "reset",
+			mutate:   func(c *model.Client) { c.Reset = 30 },
+			readBack: func(rec *model.ClientRecord) any { return rec.Reset },
+			want:     30,
+		},
+		{
+			name:     "flow",
+			mutate:   func(c *model.Client) { c.Flow = "xtls-rprx-vision" },
+			readBack: func(rec *model.ClientRecord) any { return rec.Flow },
+			want:     "xtls-rprx-vision",
+		},
+		{
+			name:     "security",
+			mutate:   func(c *model.Client) { c.Security = "aes-128-gcm" },
+			readBack: func(rec *model.ClientRecord) any { return rec.Security },
+			want:     "aes-128-gcm",
+		},
+		{
+			name:     "uuid rotated",
+			mutate:   func(c *model.Client) { c.ID = "22222222-2222-2222-2222-222222222222" },
+			readBack: func(rec *model.ClientRecord) any { return rec.UUID },
+			want:     "22222222-2222-2222-2222-222222222222",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			setupBulkDB(t)
+			svc := &ClientService{}
+			inboundSvc := &InboundService{}
+
+			email := "noib-" + strings.ReplaceAll(strings.ToLower(tc.name), " ", "-") + "@x"
+			rec := &model.ClientRecord{
+				Email:      email,
+				UUID:       "11111111-1111-1111-1111-111111111111",
+				SubID:      email,
+				TotalGB:    5,
+				ExpiryTime: 1000,
+				LimitIP:    1,
+				TgID:       1,
+				Comment:    "seeded",
+				Reset:      1,
+				Flow:       "seeded-flow",
+				Security:   "seeded-sec",
+			}
+			if err := database.GetDB().Create(rec).Error; err != nil {
+				t.Fatalf("create record: %v", err)
+			}
+
+			updated := rec.ToClient()
+			tc.mutate(updated)
+			if _, err := svc.Update(inboundSvc, rec.Id, *updated); err != nil {
+				t.Fatalf("Update: %v", err)
+			}
+
+			got, err := svc.GetByID(rec.Id)
+			if err != nil {
+				t.Fatalf("GetByID: %v", err)
+			}
+			if tc.readBack(got) != tc.want {
+				t.Fatalf("%s: not persisted for no-inbound client, got %v, want %v", tc.name, tc.readBack(got), tc.want)
+			}
+		})
+	}
+}
+
+func TestUpdate_NoInbound_PreservesCredentialsWhenOmitted(t *testing.T) {
+	setupBulkDB(t)
+	svc := &ClientService{}
+	inboundSvc := &InboundService{}
+
+	email := "noib-preserve@x"
+	rec := &model.ClientRecord{
+		Email:    email,
+		UUID:     "11111111-1111-1111-1111-111111111111",
+		SubID:    email,
+		Password: "seeded-pw",
+		Auth:     "seeded-auth",
+		Secret:   "seeded-secret",
+	}
+	if err := database.GetDB().Create(rec).Error; err != nil {
+		t.Fatalf("create record: %v", err)
+	}
+
+	updated := rec.ToClient()
+	updated.ID = ""
+	updated.Password = ""
+	updated.Auth = ""
+	updated.Secret = ""
+	updated.Comment = "only comment changed"
+	if _, err := svc.Update(inboundSvc, rec.Id, *updated); err != nil {
+		t.Fatalf("Update: %v", err)
+	}
+
+	got, err := svc.GetByID(rec.Id)
+	if err != nil {
+		t.Fatalf("GetByID: %v", err)
+	}
+	if got.UUID != "11111111-1111-1111-1111-111111111111" {
+		t.Fatalf("uuid wiped on partial update, got %q", got.UUID)
+	}
+	if got.Password != "seeded-pw" {
+		t.Fatalf("password wiped on partial update, got %q", got.Password)
+	}
+	if got.Auth != "seeded-auth" {
+		t.Fatalf("auth wiped on partial update, got %q", got.Auth)
+	}
+	if got.Secret != "seeded-secret" {
+		t.Fatalf("secret wiped on partial update, got %q", got.Secret)
+	}
+	if got.Comment != "only comment changed" {
+		t.Fatalf("comment not persisted, got %q", got.Comment)
+	}
+}
+
+func TestApplyClientRecordMerge_MirrorsSyncInboundRules(t *testing.T) {
+	row := &model.ClientRecord{
+		UUID:     "kept-uuid",
+		Password: "kept-pw",
+		Flow:     "kept-flow",
+		TotalGB:  9,
+		Group:    "kept-group",
+		Comment:  "kept-comment",
+	}
+	incoming := &model.ClientRecord{
+		Password: "new-pw",
+		TotalGB:  0,
+		Comment:  "new-comment",
+	}
+
+	applyClientRecordMerge(row, incoming)
+
+	if row.UUID != "kept-uuid" {
+		t.Fatalf("empty incoming UUID should preserve stored UUID, got %q", row.UUID)
+	}
+	if row.Password != "new-pw" {
+		t.Fatalf("non-empty incoming Password should overwrite, got %q", row.Password)
+	}
+	if row.Flow != "" {
+		t.Fatalf("incoming Flow is unconditional and should overwrite with empty, got %q", row.Flow)
+	}
+	if row.TotalGB != 0 {
+		t.Fatalf("incoming TotalGB is unconditional and should overwrite with zero, got %v", row.TotalGB)
+	}
+	if row.Group != "kept-group" {
+		t.Fatalf("empty incoming Group should preserve stored group, got %q", row.Group)
+	}
+	if row.Comment != "new-comment" {
+		t.Fatalf("incoming Comment is unconditional and should overwrite, got %q", row.Comment)
+	}
+}