diff --git a/internal/database/client_email_lower_index_test.go b/internal/database/client_email_lower_index_test.go new file mode 100644 index 000000000..60f7bd8cd --- /dev/null +++ b/internal/database/client_email_lower_index_test.go @@ -0,0 +1,48 @@ +package database + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// The migration runs on every start, so its guard has to actually match the +// index it created — otherwise every boot re-issues the CREATE. +func TestMigrateClientEmailLowerIndexIsIdempotent(t *testing.T) { + if err := InitDB(filepath.Join(t.TempDir(), "x-ui.db")); err != nil { + t.Fatalf("InitDB: %v", err) + } + t.Cleanup(func() { _ = CloseDB() }) + + if !db.Migrator().HasIndex(&model.ClientRecord{}, "idx_clients_email_lower") { + t.Fatal("idx_clients_email_lower missing after InitDB") + } + if err := migrateClientEmailLowerIndex(); err != nil { + t.Fatalf("second run: %v", err) + } + if !db.Migrator().HasIndex(&model.ClientRecord{}, "idx_clients_email_lower") { + t.Fatal("idx_clients_email_lower vanished after a second run") + } + + if IsPostgres() { + return + } + // The identity lookups filter on LOWER(email); without the expression index + // they seq-scan, which is the cost this migration exists to remove. + var plan []struct{ Detail string } + if err := db.Raw("EXPLAIN QUERY PLAN SELECT email FROM clients WHERE LOWER(email) IN ('a')"). + Scan(&plan).Error; err != nil { + t.Fatalf("explain: %v", err) + } + used := false + for _, row := range plan { + if strings.Contains(row.Detail, "idx_clients_email_lower") { + used = true + } + } + if !used { + t.Errorf("LOWER(email) lookup does not use idx_clients_email_lower: %+v", plan) + } +} diff --git a/internal/database/db.go b/internal/database/db.go index 33b7f8c2e..ebeb678ac 100644 --- a/internal/database/db.go +++ b/internal/database/db.go @@ -167,6 +167,9 @@ func initModels() error { if err := migrateSyncOrphanColumns(); err != nil { return err } + if err := migrateClientEmailLowerIndex(); err != nil { + return err + } if IsPostgres() { if err := resyncPostgresSequences(db, models); err != nil { log.Printf("Error resyncing postgres sequences: %v", err) @@ -349,6 +352,15 @@ func migrateSyncOrphanColumns() error { return db.Exec("UPDATE clients SET sync_orphaned_at = 0 WHERE sync_orphaned_at IS NULL").Error } +// The client identity checks match emails case-insensitively; without an +// expression index (which no GORM struct tag can declare) they seq-scan. +func migrateClientEmailLowerIndex() error { + if db.Migrator().HasIndex(&model.ClientRecord{}, "idx_clients_email_lower") { + return nil + } + return db.Exec("CREATE INDEX IF NOT EXISTS idx_clients_email_lower ON clients (LOWER(email))").Error +} + func migrateHostVerifyPeerCertByNameColumn() error { if !db.Migrator().HasColumn(&model.Host{}, "verify_peer_cert_by_name") { return nil diff --git a/internal/web/service/client_bulk.go b/internal/web/service/client_bulk.go index e87447594..ba59d3c41 100644 --- a/internal/web/service/client_bulk.go +++ b/internal/web/service/client_bulk.go @@ -60,12 +60,6 @@ func (s *ClientService) BulkAttach(inboundSvc *InboundService, emails []string, records = append(records, rec) } - emailSubIDs, sidErr := inboundSvc.getAllEmailSubIDs() - if sidErr != nil { - emailSubIDs = nil - logger.Warningf("[BulkAttach] getAllEmailSubIDs: %v", sidErr) - } - needRestart := false for _, ibId := range inboundIds { inbound, err := inboundSvc.GetInbound(ibId) @@ -107,7 +101,7 @@ func (s *ClientService) BulkAttach(inboundSvc *InboundService, emails []string, recordErr("inbound %d: %v", ibId, err) continue } - nr, err := s.addInboundClient(inboundSvc, &model.Inbound{Id: ibId, Settings: string(payload)}, emailSubIDs) + nr, err := s.AddInboundClient(inboundSvc, &model.Inbound{Id: ibId, Settings: string(payload)}) if err != nil { recordErr("inbound %d: %v", ibId, err) continue @@ -1117,11 +1111,6 @@ func (s *ClientService) BulkCreate(inboundSvc *InboundService, payloads []Client result.Skipped = append(result.Skipped, BulkCreateReport{Email: email, Reason: reason}) } - emailSubIDs, err := inboundSvc.getAllEmailSubIDs() - if err != nil { - emailSubIDs = nil - } - type prepared struct { client model.Client inboundIds []int @@ -1304,7 +1293,7 @@ func (s *ClientService) BulkCreate(inboundSvc *InboundService, payloads []Client payload, e := json.Marshal(map[string][]model.Client{"clients": byInbound[ibId]}) if e == nil { var nr bool - nr, e = s.addInboundClient(inboundSvc, &model.Inbound{Id: ibId, Settings: string(payload)}, emailSubIDs) + nr, e = s.AddInboundClient(inboundSvc, &model.Inbound{Id: ibId, Settings: string(payload)}) if e == nil && nr { needRestart = true } diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index b8bc6b113..f75383276 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -193,11 +193,6 @@ func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreate } } - emailSubIDs, sidErr := inboundSvc.getAllEmailSubIDs() - if sidErr != nil { - return false, sidErr - } - needRestart := false for _, ibId := range payload.InboundIds { inbound, getErr := inboundSvc.GetInbound(ibId) @@ -211,10 +206,10 @@ func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreate if mErr != nil { return needRestart, mErr } - nr, addErr := s.addInboundClient(inboundSvc, &model.Inbound{ + nr, addErr := s.AddInboundClient(inboundSvc, &model.Inbound{ Id: ibId, Settings: string(settingsPayload), - }, emailSubIDs) + }) if addErr != nil { return needRestart, addErr } @@ -731,11 +726,6 @@ func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds [] clientWire.Flow = flow clientWire.UpdatedAt = time.Now().UnixMilli() - emailSubIDs, sidErr := inboundSvc.getAllEmailSubIDs() - if sidErr != nil { - return false, sidErr - } - needRestart := false for _, ibId := range inboundIds { if _, attached := have[ibId]; attached { @@ -753,10 +743,10 @@ func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds [] if mErr != nil { return needRestart, mErr } - nr, addErr := s.addInboundClient(inboundSvc, &model.Inbound{ + nr, addErr := s.AddInboundClient(inboundSvc, &model.Inbound{ Id: ibId, Settings: string(settingsPayload), - }, emailSubIDs) + }) if addErr != nil { return needRestart, addErr } diff --git a/internal/web/service/client_identity_normalized_test.go b/internal/web/service/client_identity_normalized_test.go new file mode 100644 index 000000000..ff5fd47fc --- /dev/null +++ b/internal/web/service/client_identity_normalized_test.go @@ -0,0 +1,245 @@ +package service + +import ( + "strings" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" + "github.com/mhsanaei/3x-ui/v3/internal/xray" +) + +// Identity now comes from the clients table, so settings JSON that drifted from +// it no longer decides who may claim an email (#6252). +func TestAddInboundClientIgnoresStaleSettingsSubIds(t *testing.T) { + t.Run("stale entry no longer blocks the email", func(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + target := mkInbound(t, 21101, model.VLESS, `{"clients": []}`) + // Never synced, so no clients row backs it: pure settings-JSON drift. + mkInbound(t, 21102, model.VLESS, `{"clients": [{"email": "bob@x", "subId": "s-old", "enable": true}]}`) + + add := []model.Client{{ID: "id-bob", Email: "bob@x", Enable: true, SubID: "s-new"}} + if _, err := cs.AddInboundClient(is, &model.Inbound{Id: target.Id, Settings: clientsSettings(t, add)}); err != nil { + t.Fatalf("AddInboundClient rejected by a stale settings entry: %v", err) + } + if got := recordSubID(t, "bob@x"); got != "s-new" { + t.Errorf("stored subId = %q, want %q", got, "s-new") + } + }) + + t.Run("two drifted subIds no longer lock out the matching one", func(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + seed := []model.Client{{ID: "id-bob", Email: "bob@x", Enable: true, SubID: "s1"}} + owner := mkInbound(t, 21103, model.VLESS, clientsSettings(t, seed)) + if err := cs.SyncInbound(nil, owner.Id, seed); err != nil { + t.Fatalf("seed SyncInbound: %v", err) + } + // A second inbound whose JSON disagrees about the subId. The old scan + // locked the email to "" and then rejected even the correct subId. + mkInbound(t, 21104, model.VLESS, `{"clients": [{"email": "bob@x", "subId": "s2", "enable": true}]}`) + target := mkInbound(t, 21105, model.VLESS, `{"clients": []}`) + + add := []model.Client{{ID: "id-bob", Email: "bob@x", Enable: true, SubID: "s1"}} + if _, err := cs.AddInboundClient(is, &model.Inbound{Id: target.Id, Settings: clientsSettings(t, add)}); err != nil { + t.Fatalf("AddInboundClient rejected the matching subId: %v", err) + } + }) +} + +// A mismatched subId must still be rejected: the check moved tables, it did not +// get weaker. +func TestAddInboundClientStillRejectsMismatchedSubId(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + seed := []model.Client{{ID: "id-bob", Email: "bob@x", Enable: true, SubID: "s1"}} + owner := mkInbound(t, 21111, model.VLESS, clientsSettings(t, seed)) + if err := cs.SyncInbound(nil, owner.Id, seed); err != nil { + t.Fatalf("seed SyncInbound: %v", err) + } + target := mkInbound(t, 21112, model.VLESS, `{"clients": []}`) + + add := []model.Client{{ID: "id-other", Email: "bob@x", Enable: true, SubID: "s-different"}} + _, err := cs.AddInboundClient(is, &model.Inbound{Id: target.Id, Settings: clientsSettings(t, add)}) + if err == nil { + t.Fatal("a different subId for a taken email was accepted") + } + if !strings.Contains(err.Error(), "Duplicate email") { + t.Errorf("error = %q, want it to mention Duplicate email", err) + } +} + +// emailsUsedByOtherInbounds keys on lower(email); the clients table stores the +// email as typed under a case-sensitive unique index, so a plain IN would miss. +func TestEmailsUsedByOtherInboundsMatchesCaseInsensitively(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + seed := []model.Client{{ID: "id-a", Email: "Alice@x", Enable: true, SubID: "s-a"}} + ibA := mkInbound(t, 21121, model.VLESS, clientsSettings(t, seed)) + ibB := mkInbound(t, 21122, model.VLESS, clientsSettings(t, seed)) + for _, ib := range []*model.Inbound{ibA, ibB} { + if err := cs.SyncInbound(nil, ib.Id, seed); err != nil { + t.Fatalf("seed SyncInbound: %v", err) + } + } + + shared, err := is.emailsUsedByOtherInbounds([]string{"alice@x"}, ibA.Id) + if err != nil { + t.Fatalf("emailsUsedByOtherInbounds: %v", err) + } + if !shared["alice@x"] { + t.Error("lower-cased lookup missed a stored mixed-case email") + } + used, err := is.emailUsedByOtherInbounds("alice@x", ibA.Id) + if err != nil { + t.Fatalf("emailUsedByOtherInbounds: %v", err) + } + if !used { + t.Error("emailUsedByOtherInbounds missed a stored mixed-case email") + } + + // The traffic row is shared, so removing the client from one inbound keeps it. + if err := database.GetDB().Create(&xray.ClientTraffic{ + InboundId: ibA.Id, Email: "Alice@x", Enable: true, + }).Error; err != nil { + t.Fatalf("seed traffic: %v", err) + } + if _, err := cs.DelInboundClientByEmail(is, ibA.Id, "Alice@x", false, false); err != nil { + t.Fatalf("DelInboundClientByEmail: %v", err) + } + var count int64 + if err := database.GetDB().Model(&xray.ClientTraffic{}).Where("email = ?", "Alice@x").Count(&count).Error; err != nil { + t.Fatalf("count traffic: %v", err) + } + if count == 0 { + t.Error("traffic row purged even though the email is still on another inbound") + } +} + +// Guard, not a reproducer: this passes before the delta too. It pins the one +// delta case that is not obviously safe — the rename the taken-email guard +// refuses, where the old record must still lose this inbound's link. +func TestUpdateInboundClientRenameToTakenEmailDetachesOldLink(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + const subID = "s-shared" + oldSeed := []model.Client{{ID: "id-old", Email: "old@x", Enable: true, SubID: subID}} + newSeed := []model.Client{{ID: "id-new", Email: "new@x", Enable: true, SubID: subID}} + ibX := mkInbound(t, 21131, model.VLESS, clientsSettings(t, oldSeed)) + ibY := mkInbound(t, 21132, model.VLESS, clientsSettings(t, newSeed)) + if err := cs.SyncInbound(nil, ibX.Id, oldSeed); err != nil { + t.Fatalf("seed X: %v", err) + } + if err := cs.SyncInbound(nil, ibY.Id, newSeed); err != nil { + t.Fatalf("seed Y: %v", err) + } + + renamed := []model.Client{{ID: "id-old", Email: "new@x", Enable: true, SubID: subID}} + if _, err := cs.UpdateInboundClient(is, + &model.Inbound{Id: ibX.Id, Settings: clientsSettings(t, renamed)}, "old@x"); err != nil { + t.Fatalf("UpdateInboundClient: %v", err) + } + + links := linksOf(t, ibX.Id) + if len(links) != 1 { + t.Fatalf("inbound X link count = %d, want 1: %v", len(links), links) + } + if _, ok := links[recordID(t, "new@x")]; !ok { + t.Error("inbound X is not linked to the new@x record") + } + // The refused rename leaves old@x behind; it must not still claim inbound X. + if _, ok := links[recordID(t, "old@x")]; ok { + t.Error("old@x kept its link to inbound X after the rename") + } +} + +func recordSubID(t *testing.T, email string) string { + t.Helper() + var rec model.ClientRecord + if err := database.GetDB().Where("email = ?", email).First(&rec).Error; err != nil { + t.Fatalf("record %q: %v", email, err) + } + return rec.SubID +} + +// The stored record must carry the subId the panel generated into the settings +// JSON. Building the membership delta from the pre-stamp request values instead +// of the stamped wire entries silently desyncs the two. +func TestAddInboundClientPersistsTheGeneratedSubId(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + ib := mkInbound(t, 21141, model.VLESS, `{"clients": []}`) + add := []model.Client{{ID: "id-nosub", Email: "nosub@x", Enable: true}} + if _, err := cs.AddInboundClient(is, &model.Inbound{Id: ib.Id, Settings: clientsSettings(t, add)}); err != nil { + t.Fatalf("AddInboundClient: %v", err) + } + + stored := recordSubID(t, "nosub@x") + if stored == "" { + t.Fatal("client record has no subId; the generated one was not persisted") + } + inSettings := settingsSubID(t, ib.Id, "nosub@x") + if stored != inSettings { + t.Errorf("record subId = %q but settings JSON says %q: the two representations desynced", + stored, inSettings) + } +} + +// clients.email is unique but case-sensitive, so an identity check that does not +// fold case lets a second record for the same address be created. +func TestAddInboundClientRejectsCaseVariantOfTakenEmail(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + seed := []model.Client{{ID: "id-mix", Email: "Bob@x", Enable: true, SubID: "s-mix"}} + owner := mkInbound(t, 21151, model.VLESS, clientsSettings(t, seed)) + if err := cs.SyncInbound(nil, owner.Id, seed); err != nil { + t.Fatalf("seed SyncInbound: %v", err) + } + target := mkInbound(t, 21152, model.VLESS, `{"clients": []}`) + + add := []model.Client{{ID: "id-other", Email: "bob@x", Enable: true, SubID: "s-other"}} + _, err := cs.AddInboundClient(is, &model.Inbound{Id: target.Id, Settings: clientsSettings(t, add)}) + if err == nil { + var count int64 + database.GetDB().Model(&model.ClientRecord{}). + Where("LOWER(email) = ?", "bob@x").Count(&count) + t.Fatalf("a case variant of a taken email was accepted; clients now holds %d rows for bob@x", count) + } + if !strings.Contains(err.Error(), "Duplicate email") { + t.Errorf("error = %q, want it to mention Duplicate email", err) + } +} + +func settingsSubID(t *testing.T, inboundId int, email string) string { + t.Helper() + var ib model.Inbound + if err := database.GetDB().First(&ib, inboundId).Error; err != nil { + t.Fatalf("load inbound: %v", err) + } + clients, err := ParseInboundSettingsClients(ib.Settings) + if err != nil { + t.Fatalf("parse settings: %v", err) + } + for _, c := range clients { + if c.Email == email { + return c.SubID + } + } + t.Fatalf("%q not found in settings", email) + return "" +} diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 58a46a92f..b41bc36a4 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -42,7 +42,7 @@ func advancePushedInbound(rt runtime.Runtime, prevSettings string, ib *model.Inb } // delInboundClients removes several clients from a single inbound in one pass: -// one settings rewrite, one runtime sweep, one Save and one SyncInbound for the +// one settings rewrite, one runtime sweep, one Save and one link delta for the // whole batch, instead of repeating the full per-client cycle. It mirrors the // semantics of DelInboundClientByEmail for each removed client. needRestart is // the OR across all removals. @@ -177,11 +177,13 @@ func (s *ClientService) delInboundClients(inboundSvc *InboundService, inboundId if e := tx.Save(oldInbound).Error; e != nil { return e } - finalClients, gcErr := inboundSvc.GetClients(oldInbound) - if gcErr != nil { - return gcErr + detached := make([]string, 0, len(targets)) + for _, t := range targets { + if t.email != "" { + detached = append(detached, t.email) + } } - if err := s.SyncInbound(tx, inboundId, finalClients); err != nil { + if err := s.ApplyInboundClientDelta(tx, inboundId, nil, detached); err != nil { return err } if oldInbound.NodeID != nil { @@ -239,13 +241,10 @@ func (s *ClientService) delInboundClients(inboundSvc *InboundService, inboundId return needRestart, nil } -func (s *ClientService) checkEmailsExistForClients(inboundSvc *InboundService, clients []model.Client, emailSubIDs map[string]string) (string, error) { - if emailSubIDs == nil { - var err error - emailSubIDs, err = inboundSvc.getAllEmailSubIDs() - if err != nil { - return "", err - } +func (s *ClientService) checkEmailsExistForClients(inboundSvc *InboundService, clients []model.Client) (string, error) { + emailSubIDs, err := inboundSvc.emailSubIDsForClients(clients) + if err != nil { + return "", err } seen := make(map[string]string, len(clients)) for _, client := range clients { @@ -270,14 +269,6 @@ func (s *ClientService) checkEmailsExistForClients(inboundSvc *InboundService, c } func (s *ClientService) AddInboundClient(inboundSvc *InboundService, data *model.Inbound) (bool, error) { - return s.addInboundClient(inboundSvc, data, nil) -} - -// addInboundClient is AddInboundClient with an optional precomputed email→subId -// map. Bulk callers pass a single snapshot so the global getAllEmailSubIDs scan -// runs once for the whole batch instead of once per target inbound; a nil map -// makes it compute its own (the single-add path). -func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model.Inbound, emailSubIDs map[string]string) (bool, error) { defer lockInbound(data.Id).Unlock() clients, err := inboundSvc.GetClients(data) @@ -306,7 +297,7 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model interfaceClients[i] = cm } } - existEmail, err := s.checkEmailsExistForClients(inboundSvc, clients, emailSubIDs) + existEmail, err := s.checkEmailsExistForClients(inboundSvc, clients) if err != nil { return false, err } @@ -422,6 +413,13 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model prevSettings := oldInbound.Settings oldInbound.Settings = string(newSettings) + // From the stamped wire entries, not from clients: created_at / updated_at / + // subId are written onto interfaceClients above, after clients was parsed. + addedClients, err := settingsEntriesToClients(interfaceClients) + if err != nil { + return false, err + } + needRestart := false rt, push, _, perr := inboundSvc.nodePushPlan(oldInbound) @@ -443,11 +441,7 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model if e := tx.Save(oldInbound).Error; e != nil { return e } - finalClients, gcErr := inboundSvc.GetClients(oldInbound) - if gcErr != nil { - return gcErr - } - if err := s.SyncInbound(tx, oldInbound.Id, finalClients); err != nil { + if err := s.ApplyInboundClientDelta(tx, oldInbound.Id, addedClients, nil); err != nil { return err } if oldInbound.NodeID != nil { @@ -587,7 +581,7 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo } if clients[0].Email != oldEmail { - existEmail, err := s.checkEmailsExistForClients(inboundSvc, clients, nil) + existEmail, err := s.checkEmailsExistForClients(inboundSvc, clients) if err != nil { return false, err } @@ -731,6 +725,17 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo prevSettings := oldInbound.Settings oldInbound.Settings = string(newSettings) + // From the stamped wire entry, not from clients[0]: created_at, the + // preserved subId and the WireGuard carry-forward land on interfaceClients. + changedClients, err := settingsEntriesToClients(interfaceClients[:1]) + if err != nil { + return false, err + } + var detachEmails []string + if len(oldEmail) > 0 && oldEmail != clients[0].Email { + detachEmails = []string{oldEmail} + } + needRestart := false // Resolve the push plan before the DB write so a node-state lookup failure @@ -820,11 +825,9 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo } } } - finalClients, gcErr := inboundSvc.GetClients(oldInbound) - if gcErr != nil { - return gcErr - } - if err := s.SyncInbound(tx, oldInbound.Id, finalClients); err != nil { + // detachEmails covers the rename the guard above refused: the old record + // keeps this inbound's link otherwise, which the full sync used to drop. + if err := s.ApplyInboundClientDelta(tx, oldInbound.Id, changedClients, detachEmails); err != nil { return err } if oldInbound.NodeID != nil { @@ -998,11 +1001,7 @@ func (s *ClientService) DelInboundClientByEmail(inboundSvc *InboundService, inbo if e := tx.Save(oldInbound).Error; e != nil { return e } - finalClients, gcErr := inboundSvc.GetClients(oldInbound) - if gcErr != nil { - return gcErr - } - if err := s.SyncInbound(tx, inboundId, finalClients); err != nil { + if err := s.ApplyInboundClientDelta(tx, inboundId, nil, []string{email}); err != nil { return err } if oldInbound.NodeID != nil { diff --git a/internal/web/service/client_link.go b/internal/web/service/client_link.go index 70eba4b7f..c3e24aea1 100644 --- a/internal/web/service/client_link.go +++ b/internal/web/service/client_link.go @@ -7,6 +7,7 @@ import ( "github.com/mhsanaei/3x-ui/v3/internal/database/model" "gorm.io/gorm" + "gorm.io/gorm/clause" ) // applyClientRecordMerge merges incoming client-record fields onto row using the @@ -78,15 +79,25 @@ func applyClientRecordMerge(row *model.ClientRecord, incoming *model.ClientRecor } } +// SyncInbound makes the inbound's client records and links match clients +// exactly: links for clients no longer in the set are removed. func (s *ClientService) SyncInbound(tx *gorm.DB, inboundId int, clients []model.Client) error { + return s.syncInboundClients(tx, inboundId, clients, nil, true) +} + +// ApplyInboundClientDelta persists only the clients an edit actually changed +// plus the emails it detached, leaving every other link on the inbound alone — +// the whole point being that a one-client edit must not rewrite the inbound's +// entire membership set (#6252). +func (s *ClientService) ApplyInboundClientDelta(tx *gorm.DB, inboundId int, changed []model.Client, detachEmails []string) error { + return s.syncInboundClients(tx, inboundId, changed, detachEmails, false) +} + +func (s *ClientService) syncInboundClients(tx *gorm.DB, inboundId int, clients []model.Client, detachEmails []string, prune bool) error { if tx == nil { tx = database.GetDB() } - if err := tx.Where("inbound_id = ?", inboundId).Delete(&model.ClientInbound{}).Error; err != nil { - return err - } - emails := make([]string, 0, len(clients)) seen := make(map[string]struct{}, len(clients)) for i := range clients { @@ -166,8 +177,8 @@ func (s *ClientService) SyncInbound(tx *gorm.DB, inboundId int, clients []model. } } - links := make([]model.ClientInbound, 0, len(clients)) - linked := make(map[int]struct{}, len(clients)) + wantedFlow := make(map[int]string, len(clients)) + wantedIds := make([]int, 0, len(clients)) for i := range clients { email := strings.TrimSpace(clients[i].Email) if email == "" { @@ -177,18 +188,100 @@ func (s *ClientService) SyncInbound(tx *gorm.DB, inboundId int, clients []model. if !ok { continue } - if _, dup := linked[id]; dup { + if _, dup := wantedFlow[id]; dup { continue } - linked[id] = struct{}{} - links = append(links, model.ClientInbound{ + wantedFlow[id] = clients[i].Flow + wantedIds = append(wantedIds, id) + } + + return s.reconcileInboundLinks(tx, inboundId, wantedFlow, wantedIds, detachEmails, prune) +} + +// reconcileInboundLinks writes only the client_inbounds rows that differ. prune +// also removes links absent from wantedFlow, which only a full sync may do. +func (s *ClientService) reconcileInboundLinks(tx *gorm.DB, inboundId int, wantedFlow map[int]string, wantedIds []int, detachEmails []string, prune bool) error { + var current []model.ClientInbound + if prune { + if err := tx.Where("inbound_id = ?", inboundId).Find(¤t).Error; err != nil { + return err + } + } else { + for _, batch := range chunkInts(wantedIds, sqlInChunk) { + var rows []model.ClientInbound + if err := tx.Where("inbound_id = ? AND client_id IN ?", inboundId, batch).Find(&rows).Error; err != nil { + return err + } + current = append(current, rows...) + } + } + + var toDelete []int + toUpdate := make(map[string][]int) + have := make(map[int]struct{}, len(current)) + for _, link := range current { + have[link.ClientId] = struct{}{} + flow, keep := wantedFlow[link.ClientId] + if !keep { + if prune { + toDelete = append(toDelete, link.ClientId) + } + continue + } + // Plain compare, not non-empty-wins: clearing a flow must persist "". + if flow != link.FlowOverride { + toUpdate[flow] = append(toUpdate[flow], link.ClientId) + } + } + + if len(detachEmails) > 0 { + for _, batch := range chunkStrings(detachEmails, sqlInChunk) { + var ids []int + if err := tx.Model(&model.ClientRecord{}).Where("email IN ?", batch).Pluck("id", &ids).Error; err != nil { + return err + } + for _, id := range ids { + if _, keep := wantedFlow[id]; !keep { + toDelete = append(toDelete, id) + } + } + } + } + + toInsert := make([]model.ClientInbound, 0, len(wantedIds)) + for _, id := range wantedIds { + if _, exists := have[id]; exists { + continue + } + toInsert = append(toInsert, model.ClientInbound{ ClientId: id, InboundId: inboundId, - FlowOverride: clients[i].Flow, + FlowOverride: wantedFlow[id], }) } - if len(links) > 0 { - if err := tx.CreateInBatches(links, 200).Error; err != nil { + + for _, batch := range chunkInts(toDelete, sqlInChunk) { + if err := tx.Where("inbound_id = ? AND client_id IN ?", inboundId, batch). + Delete(&model.ClientInbound{}).Error; err != nil { + return err + } + } + for flow, ids := range toUpdate { + for _, batch := range chunkInts(ids, sqlInChunk) { + if err := tx.Model(&model.ClientInbound{}). + Where("inbound_id = ? AND client_id IN ?", inboundId, batch). + Update("flow_override", flow).Error; err != nil { + return err + } + } + } + if len(toInsert) > 0 { + // The delete this replaced also serialized concurrent syncs of one + // inbound; without the clause a racing node poll aborts its whole tx. + if err := tx.Clauses(clause.OnConflict{ + Columns: []clause.Column{{Name: "client_id"}, {Name: "inbound_id"}}, + DoUpdates: clause.AssignmentColumns([]string{"flow_override"}), + }).CreateInBatches(toInsert, 200).Error; err != nil { return err } } diff --git a/internal/web/service/client_link_delta_test.go b/internal/web/service/client_link_delta_test.go new file mode 100644 index 000000000..4825551fc --- /dev/null +++ b/internal/web/service/client_link_delta_test.go @@ -0,0 +1,209 @@ +package service + +import ( + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// stampLinkCreatedAt marks every link of an inbound with a sentinel timestamp. +// A row that still carries it afterwards was not deleted and re-inserted. +func stampLinkCreatedAt(t *testing.T, inboundId int) { + t.Helper() + err := database.GetDB().Model(&model.ClientInbound{}). + Where("inbound_id = ?", inboundId). + UpdateColumn("created_at", 1).Error + if err != nil { + t.Fatalf("stamp created_at: %v", err) + } +} + +func linksOf(t *testing.T, inboundId int) map[int]model.ClientInbound { + t.Helper() + var rows []model.ClientInbound + if err := database.GetDB().Where("inbound_id = ?", inboundId).Find(&rows).Error; err != nil { + t.Fatalf("load links: %v", err) + } + out := make(map[int]model.ClientInbound, len(rows)) + for _, r := range rows { + out[r.ClientId] = r + } + return out +} + +func recordID(t *testing.T, email string) int { + t.Helper() + var rec model.ClientRecord + if err := database.GetDB().Where("email = ?", email).First(&rec).Error; err != nil { + t.Fatalf("record %q: %v", email, err) + } + return rec.Id +} + +// A re-sync that changes one client's flow must leave the other links in place +// and UPDATE the changed one, not rebuild the whole membership set (#6252). +func TestSyncInboundReusesUnchangedLinkRows(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + + seed := []model.Client{ + {ID: "id-a", Email: "a@x", Enable: true, SubID: "s-a"}, + {ID: "id-b", Email: "b@x", Enable: true, SubID: "s-b", Flow: "xtls-rprx-vision"}, + {ID: "id-c", Email: "c@x", Enable: true, SubID: "s-c"}, + } + ib := mkInbound(t, 21001, model.VLESS, clientsSettings(t, seed)) + if err := cs.SyncInbound(nil, ib.Id, seed); err != nil { + t.Fatalf("seed SyncInbound: %v", err) + } + stampLinkCreatedAt(t, ib.Id) + + changed := make([]model.Client, len(seed)) + copy(changed, seed) + changed[1].Flow = "" + if err := cs.SyncInbound(nil, ib.Id, changed); err != nil { + t.Fatalf("re-sync: %v", err) + } + + links := linksOf(t, ib.Id) + if len(links) != 3 { + t.Fatalf("link count = %d, want 3", len(links)) + } + for _, email := range []string{"a@x", "b@x", "c@x"} { + link, ok := links[recordID(t, email)] + if !ok { + t.Fatalf("%s lost its link", email) + } + if link.CreatedAt != 1 { + t.Errorf("%s link created_at = %d, want the 1 sentinel: the row was deleted and re-inserted", email, link.CreatedAt) + } + } + if got := links[recordID(t, "b@x")].FlowOverride; got != "" { + t.Errorf("b@x flow_override = %q, want \"\" (cleared in place)", got) + } + + // Dropping a client must still remove exactly that one link. + if err := cs.SyncInbound(nil, ib.Id, []model.Client{seed[0], seed[2]}); err != nil { + t.Fatalf("prune sync: %v", err) + } + links = linksOf(t, ib.Id) + if len(links) != 2 { + t.Fatalf("after prune link count = %d, want 2", len(links)) + } + if _, still := links[recordID(t, "b@x")]; still { + t.Error("b@x link survived a full sync that dropped it") + } + for _, email := range []string{"a@x", "c@x"} { + if links[recordID(t, email)].CreatedAt != 1 { + t.Errorf("%s link was rebuilt by the prune sync", email) + } + } +} + +// Adding a client must not re-merge its bystanders' records from the settings +// JSON; comment lives only in the clients table, so a full sync erases it. +func TestAddInboundClientLeavesBystanderRecordsUntouched(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + seed := []model.Client{ + {ID: "id-a", Email: "a@x", Enable: true, SubID: "s-a"}, + {ID: "id-b", Email: "b@x", Enable: true, SubID: "s-b"}, + } + ib := mkInbound(t, 21002, model.VLESS, clientsSettings(t, seed)) + if err := cs.SyncInbound(nil, ib.Id, seed); err != nil { + t.Fatalf("seed SyncInbound: %v", err) + } + db := database.GetDB() + if err := db.Model(&model.ClientRecord{}).Where("email = ?", "a@x"). + UpdateColumn("comment", "operator note").Error; err != nil { + t.Fatalf("set comment: %v", err) + } + stampLinkCreatedAt(t, ib.Id) + + add := []model.Client{{ID: "id-c", Email: "c@x", Enable: true, SubID: "s-c"}} + if _, err := cs.AddInboundClient(is, &model.Inbound{Id: ib.Id, Settings: clientsSettings(t, add)}); err != nil { + t.Fatalf("AddInboundClient: %v", err) + } + + var bystander model.ClientRecord + if err := db.Where("email = ?", "a@x").First(&bystander).Error; err != nil { + t.Fatalf("reload a@x: %v", err) + } + if bystander.Comment != "operator note" { + t.Errorf("bystander comment = %q, want %q: the add re-merged an unrelated record from settings JSON", + bystander.Comment, "operator note") + } + + links := linksOf(t, ib.Id) + if len(links) != 3 { + t.Fatalf("link count = %d, want 3", len(links)) + } + for _, email := range []string{"a@x", "b@x"} { + if links[recordID(t, email)].CreatedAt != 1 { + t.Errorf("%s link was rebuilt by an unrelated add", email) + } + } + newLink, ok := links[recordID(t, "c@x")] + if !ok { + t.Fatal("c@x got no link") + } + if newLink.CreatedAt == 1 { + t.Error("c@x link carries the sentinel; it should be freshly inserted") + } +} + +// Deleting one client detaches only that client; the others keep both their +// link rows and the record fields that live only in the clients table. +func TestDelInboundClientDetachesOnlyTheRemovedClient(t *testing.T) { + setupBulkDB(t) + cs := &ClientService{} + is := &InboundService{} + + seed := []model.Client{ + {ID: "id-a", Email: "a@x", Enable: true, SubID: "s-a"}, + {ID: "id-b", Email: "b@x", Enable: true, SubID: "s-b"}, + {ID: "id-c", Email: "c@x", Enable: true, SubID: "s-c"}, + } + ib := mkInbound(t, 21003, model.VLESS, clientsSettings(t, seed)) + if err := cs.SyncInbound(nil, ib.Id, seed); err != nil { + t.Fatalf("seed SyncInbound: %v", err) + } + db := database.GetDB() + if err := db.Model(&model.ClientRecord{}).Where("email = ?", "c@x"). + UpdateColumn("comment", "keep me").Error; err != nil { + t.Fatalf("set comment: %v", err) + } + removedID := recordID(t, "b@x") + stampLinkCreatedAt(t, ib.Id) + + if _, err := cs.DelInboundClientByEmail(is, ib.Id, "b@x", true, false); err != nil { + t.Fatalf("DelInboundClientByEmail: %v", err) + } + + links := linksOf(t, ib.Id) + if _, still := links[removedID]; still { + t.Error("b@x link survived the delete") + } + if len(links) != 2 { + t.Fatalf("link count = %d, want 2", len(links)) + } + for _, email := range []string{"a@x", "c@x"} { + if links[recordID(t, email)].CreatedAt != 1 { + t.Errorf("%s link was rebuilt by an unrelated delete", email) + } + } + // Detach must not delete the record itself. + var removed model.ClientRecord + if err := db.Where("email = ?", "b@x").First(&removed).Error; err != nil { + t.Fatalf("b@x record should survive a detach: %v", err) + } + var kept model.ClientRecord + if err := db.Where("email = ?", "c@x").First(&kept).Error; err != nil { + t.Fatalf("reload c@x: %v", err) + } + if kept.Comment != "keep me" { + t.Errorf("bystander comment = %q, want %q", kept.Comment, "keep me") + } +} diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index ec9ea653a..2b97d951a 100644 --- a/internal/web/service/inbound.go +++ b/internal/web/service/inbound.go @@ -474,37 +474,40 @@ func (s *InboundService) GetAllEmails() ([]string, error) { return emails, nil } -// getAllEmailSubIDs returns email→subId. An email seen with two different -// non-empty subIds is locked (mapped to "") so neither identity can claim it. -func (s *InboundService) getAllEmailSubIDs() (map[string]string, error) { +// emailSubIDsForClients returns lower(email)→subId for just the emails being +// checked. One clients row owns an email's identity, so the answer no longer +// needs a scan of every inbound's settings JSON (#6252). +func (s *InboundService) emailSubIDsForClients(clients []model.Client) (map[string]string, error) { + want := make(map[string]struct{}, len(clients)) + for i := range clients { + if email := strings.ToLower(strings.TrimSpace(clients[i].Email)); email != "" { + want[email] = struct{}{} + } + } + result := make(map[string]string, len(want)) + if len(want) == 0 { + return result, nil + } + lowered := make([]string, 0, len(want)) + for email := range want { + lowered = append(lowered, email) + } db := database.GetDB() - var rows []struct { - Email string - SubID string - } - query := fmt.Sprintf( - "SELECT %s AS email, %s AS sub_id %s", - database.JSONFieldText("client.value", "email"), - database.JSONFieldText("client.value", "subId"), - database.JSONClientsFromInbound(), - ) - if err := db.Raw(query).Scan(&rows).Error; err != nil { - return nil, err - } - result := make(map[string]string, len(rows)) - for _, r := range rows { - email := strings.ToLower(r.Email) - if email == "" { - continue + for _, batch := range chunkStrings(lowered, sqlInChunk) { + var rows []struct { + Email string + SubID string `gorm:"column:sub_id"` } - subID := r.SubID - if existing, ok := result[email]; ok { - if existing != subID { - result[email] = "" - } - continue + err := db.Model(&model.ClientRecord{}). + Select("email, sub_id"). + Where("LOWER(email) IN ?", batch). + Scan(&rows).Error + if err != nil { + return nil, err + } + for _, r := range rows { + result[strings.ToLower(r.Email)] = r.SubID } - result[email] = subID } return result, nil } @@ -940,7 +943,7 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo if err != nil { return inbound, false, err } - existEmail, err := s.clientService.checkEmailsExistForClients(s, clients, nil) + existEmail, err := s.clientService.checkEmailsExistForClients(s, clients) if err != nil { return inbound, false, err } diff --git a/internal/web/service/inbound_clients.go b/internal/web/service/inbound_clients.go index 8957510b0..bb98e3935 100644 --- a/internal/web/service/inbound_clients.go +++ b/internal/web/service/inbound_clients.go @@ -127,14 +127,13 @@ func (s *InboundService) emailUsedByOtherInbounds(email string, exceptInboundId if email == "" { return false, nil } - db := database.GetDB() var count int64 - query := fmt.Sprintf( - "SELECT COUNT(*) %s WHERE inbounds.id != ? AND LOWER(%s) = LOWER(?)", - database.JSONClientsFromInbound(), - database.JSONFieldText("client.value", "email"), - ) - if err := db.Raw(query, exceptInboundId, email).Scan(&count).Error; err != nil { + err := database.GetDB().Table("client_inbounds"). + Joins("JOIN clients ON clients.id = client_inbounds.client_id"). + Where("client_inbounds.inbound_id != ? AND LOWER(clients.email) = ?", + exceptInboundId, strings.ToLower(strings.TrimSpace(email))). + Count(&count).Error + if err != nil { return false, err } return count > 0, nil @@ -152,20 +151,23 @@ func (s *InboundService) emailsUsedByOtherInbounds(emails []string, exceptInboun if len(want) == 0 { return shared, nil } - db := database.GetDB() - var rows []string - query := fmt.Sprintf( - "SELECT DISTINCT LOWER(%s) %s WHERE inbounds.id != ?", - database.JSONFieldText("client.value", "email"), - database.JSONClientsFromInbound(), - ) - if err := db.Raw(query, exceptInboundId).Scan(&rows).Error; err != nil { - return nil, err + lowered := make([]string, 0, len(want)) + for e := range want { + lowered = append(lowered, e) } - for _, e := range rows { - e = strings.ToLower(strings.TrimSpace(e)) - if _, ok := want[e]; ok { - shared[e] = true + db := database.GetDB() + for _, batch := range chunkStrings(lowered, sqlInChunk) { + var rows []struct{ Email string } + err := db.Table("client_inbounds"). + Joins("JOIN clients ON clients.id = client_inbounds.client_id"). + Select("DISTINCT LOWER(clients.email) AS email"). + Where("client_inbounds.inbound_id != ? AND LOWER(clients.email) IN ?", exceptInboundId, batch). + Scan(&rows).Error + if err != nil { + return nil, err + } + for _, r := range rows { + shared[r.Email] = true } } return shared, nil diff --git a/internal/web/service/inbound_settings_clients.go b/internal/web/service/inbound_settings_clients.go index 1df211811..1bdf4a214 100644 --- a/internal/web/service/inbound_settings_clients.go +++ b/internal/web/service/inbound_settings_clients.go @@ -30,3 +30,21 @@ func ParseInboundSettingsClients(settings string) ([]model.Client, error) { } return clients, nil } + +// settingsEntriesToClients decodes the wire entries a caller has already +// stamped, so a delta carries the persisted created_at / updated_at / subId +// rather than the pre-stamp values the request was parsed into. +func settingsEntriesToClients(entries []any) ([]model.Client, error) { + if len(entries) == 0 { + return nil, nil + } + raw, err := json.Marshal(entries) + if err != nil { + return nil, err + } + var clients []model.Client + if err := json.Unmarshal(raw, &clients); err != nil { + return nil, err + } + return clients, nil +} diff --git a/internal/web/service/node_client_traffic_sum_test.go b/internal/web/service/node_client_traffic_sum_test.go index 9477f2e54..dbb09e35c 100644 --- a/internal/web/service/node_client_traffic_sum_test.go +++ b/internal/web/service/node_client_traffic_sum_test.go @@ -35,7 +35,7 @@ func createNodeInbound(t *testing.T, db *gorm.DB, nodeID int, tag string, port i } // createNodeInboundWithClient mirrors createNodeInbound but stores the client -// in the settings JSON so emailUsedByOtherInbounds can see the attachment. +// in the settings JSON, which the node sync turns into a client_inbounds link. func createNodeInboundWithClient(t *testing.T, db *gorm.DB, nodeID int, tag string, port int, email string) { t.Helper() nid := nodeID diff --git a/internal/web/service/node_sync_link_churn_test.go b/internal/web/service/node_sync_link_churn_test.go new file mode 100644 index 000000000..9ee6c6e84 --- /dev/null +++ b/internal/web/service/node_sync_link_churn_test.go @@ -0,0 +1,70 @@ +package service + +import ( + "fmt" + "strings" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database/model" + "github.com/mhsanaei/3x-ui/v3/internal/xray" +) + +// The node traffic poll runs every 5s and re-syncs every node inbound from its +// snapshot. A steady-state poll must not rewrite the inbound's whole membership +// set — that churn was the bulk of the write load in #6252. +func TestNodeSyncDoesNotChurnInboundLinks(t *testing.T) { + db := initTrafficTestDB(t) + svc := &InboundService{} + + emails := []string{"n1@x", "n2@x", "n3@x"} + entries := make([]string, 0, len(emails)) + stats := make([]xray.ClientTraffic, 0, len(emails)) + for i, e := range emails { + entries = append(entries, fmt.Sprintf(`{"email": %q, "enable": true}`, e)) + stats = append(stats, xray.ClientTraffic{Email: e, Up: int64(100 * (i + 1)), Down: 100, Enable: true}) + } + settings := `{"clients": [` + strings.Join(entries, ",") + `]}` + + createNodeInbound(t, db, 1, "n1-in", 41101) + syncNodeWithSettings(t, svc, 1, "n1-in", settings, stats...) + + var ib model.Inbound + if err := db.Where("tag = ?", "n1-in").First(&ib).Error; err != nil { + t.Fatalf("load inbound: %v", err) + } + before := linksOf(t, ib.Id) + if len(before) != len(emails) { + t.Fatalf("link count after first sync = %d, want %d", len(before), len(emails)) + } + stampLinkCreatedAt(t, ib.Id) + + // Second poll: identical client set, counters have grown. + for i := range stats { + stats[i].Up += 500 + stats[i].Down += 500 + } + syncNodeWithSettings(t, svc, 1, "n1-in", settings, stats...) + + after := linksOf(t, ib.Id) + if len(after) != len(before) { + t.Fatalf("link count after second sync = %d, want %d", len(after), len(before)) + } + for id, link := range after { + if link.CreatedAt != 1 { + t.Errorf("client %d: link created_at = %d, want the 1 sentinel: a steady-state node poll rebuilt the membership set", + id, link.CreatedAt) + } + } + + // A client removed on the node must still lose its link, or the soft-orphan + // sweep that reads this table would stop seeing remote deletions. + shrunk := `{"clients": [` + strings.Join(entries[:2], ",") + `]}` + syncNodeWithSettings(t, svc, 1, "n1-in", shrunk, stats[:2]...) + pruned := linksOf(t, ib.Id) + if len(pruned) != 2 { + t.Fatalf("link count after shrink = %d, want 2", len(pruned)) + } + if _, still := pruned[recordID(t, "n3@x")]; still { + t.Error("n3@x link survived a snapshot that dropped it") + } +}