diff --git a/cmd/createuser/main.go b/cmd/createuser/main.go index 579a4e0c..d3557418 100644 --- a/cmd/createuser/main.go +++ b/cmd/createuser/main.go @@ -27,11 +27,14 @@ import ( "context" "crypto/rand" "encoding/binary" + "errors" "flag" "fmt" "os" "time" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgxpool" "telesrv/internal/config" @@ -124,7 +127,7 @@ func main() { var createdID int64 if err := row.Scan(&createdID); err != nil { - fmt.Fprintf(os.Stderr, "createuser: id %d already exists (or insert failed): %v\n", *id, err) + fmt.Fprintln(os.Stderr, describeInsertFailure(*id, *username, displayPhone, signupEmail, err)) os.Exit(1) } @@ -132,6 +135,31 @@ func main() { createdID, accessHash, *firstName, *lastName, *username, displayPhone, signupEmail) } +// describeInsertFailure turns the INSERT's failure into a message naming the +// actual thing that collided, instead of "id already exists" for every case: +// ON CONFLICT (id) DO NOTHING only covers the id itself, so a duplicate +// username/phone/signup_email surfaces here as a distinct unique-violation +// error (pgx.ErrNoRows only means the id itself was the conflict). +func describeInsertFailure(id int64, username, phone, signupEmail string, err error) string { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == "23505" { + switch pgErr.ConstraintName { + case "users_username_lower_unique_idx": + return fmt.Sprintf("createuser: username %q is already taken", username) + case "users_phone_unique_idx": + return fmt.Sprintf("createuser: phone %q is already in use", phone) + case "users_signup_email_lower_unique_idx": + return fmt.Sprintf("createuser: email %q is already in use by another account", signupEmail) + default: + return fmt.Sprintf("createuser: unique constraint %q violated: %v", pgErr.ConstraintName, err) + } + } + if errors.Is(err, pgx.ErrNoRows) { + return fmt.Sprintf("createuser: id %d already exists", id) + } + return fmt.Sprintf("createuser: insert failed: %v", err) +} + // assignEmailSignupDisplayPhone mirrors internal/app/auth/service.go's method // of the same name: pick a random "888"-prefixed display phone and re-roll on // the astronomically unlikely collision with an existing account's phone. diff --git a/internal/app/users/service.go b/internal/app/users/service.go index 2fc78932..f745c175 100644 --- a/internal/app/users/service.go +++ b/internal/app/users/service.go @@ -769,7 +769,16 @@ func (s *Service) putCachedUsers(ctx context.Context, users ...domain.User) { // Collectible ownership may change inside the star-gift aggregate. Keep // these uncommon users on the authoritative store path so the database // lifecycle trigger can never be masked by a stale base-user cache entry. - if user.ID != 0 && user.EmojiStatusCollectible.Empty() { + // + // redisstore.userBaseValue has no field for Deleted/DeletedAt/Status: caching + // a deleted user silently resets Deleted to false (and Status to the zero + // UserStatusUnknown) on every round trip, which never self-heals -- each + // subsequent miss reloads the correctly tombstoned DB row and immediately + // re-corrupts it on write. That regressed a deleted account back to looking + // live (blank name, but "last seen recently" instead of "Deleted Account"). + // Keep deleted users off the cache so lookups always hit the authoritative + // store, which encodes the tombstone correctly. + if user.ID != 0 && user.EmojiStatusCollectible.Empty() && !user.Deleted { cacheable = append(cacheable, user) } } diff --git a/internal/app/users/service_test.go b/internal/app/users/service_test.go index a3029039..553d2293 100644 --- a/internal/app/users/service_test.go +++ b/internal/app/users/service_test.go @@ -397,6 +397,69 @@ func TestServiceUsesBaseCacheWithoutCachingViewerOverlay(t *testing.T) { } } +// deletedOverrideUserStore reports one user id as an already-tombstoned +// domain.User regardless of what the underlying memory store holds, so the +// test doesn't need a memory.UserStore deletion helper to exercise the +// base-user cache's handling of deleted users. +type deletedOverrideUserStore struct { + *memory.UserStore + deletedID int64 + byIDsCalls int +} + +func (s *deletedOverrideUserStore) ByIDs(ctx context.Context, ids []int64) ([]domain.User, error) { + s.byIDsCalls++ + users, err := s.UserStore.ByIDs(ctx, ids) + if err != nil { + return nil, err + } + for i, u := range users { + if u.ID == s.deletedID { + users[i] = domain.User{ID: u.ID, Deleted: true, Status: domain.UserStatus{Kind: domain.UserStatusEmpty}} + } + } + return users, nil +} + +// TestServiceNeverCachesDeletedUser guards the redisstore.UserCache cache +// schema gap: userBaseValue carries no Deleted/DeletedAt/Status field, so +// caching a deleted user silently resets Deleted to false (and Status to the +// zero UserStatusUnknown) on every round trip -- a bug that never self-heals, +// since each subsequent cache miss reloads the correctly tombstoned row and +// immediately re-corrupts it on write. It regressed a deleted account back to +// looking live: blank name (still blank, that part survives), but "last seen +// recently" instead of "Deleted Account". The service must keep deleted users +// off the base cache entirely so every lookup hits the authoritative store. +func TestServiceNeverCachesDeletedUser(t *testing.T) { + ctx := context.Background() + base := memory.NewUserStore() + owner, err := base.Create(ctx, domain.User{AccessHash: 1, Phone: "15550000031", FirstName: "Owner"}) + if err != nil { + t.Fatalf("create owner: %v", err) + } + target, err := base.Create(ctx, domain.User{AccessHash: 2, Phone: "15550000032", FirstName: "Target"}) + if err != nil { + t.Fatalf("create target: %v", err) + } + store := &deletedOverrideUserStore{UserStore: base, deletedID: target.ID} + cache := newMemoryBaseUserCache() + svc := NewService(store, WithBaseUserCache(cache)) + + got, found, err := svc.ByID(ctx, owner.ID, target.ID) + if err != nil || !found || !got.Deleted { + t.Fatalf("ByID = %+v found=%v err=%v, want a deleted user", got, found, err) + } + if _, cached := cache.users[target.ID]; cached { + t.Fatalf("cache holds deleted user %d, want it kept off the cache entirely", target.ID) + } + if _, found, err := svc.ByID(ctx, owner.ID, target.ID); err != nil || !found { + t.Fatalf("second ByID found=%v err=%v", found, err) + } + if store.byIDsCalls != 2 { + t.Fatalf("store ByIDs calls = %d, want 2 (deleted user must never be served from cache)", store.byIDsCalls) + } +} + func TestServiceRefreshesBaseCacheAfterProfileUpdate(t *testing.T) { ctx := context.Background() base := memory.NewUserStore() diff --git a/internal/domain/account.go b/internal/domain/account.go index 77369673..7ca459e2 100644 --- a/internal/domain/account.go +++ b/internal/domain/account.go @@ -7,6 +7,7 @@ import ( var ( ErrPasswordHashInvalid = errors.New("password hash invalid") + ErrPasswordMissing = errors.New("password missing") ErrSRPIDInvalid = errors.New("srp id invalid") ErrSRPPasswordChanged = errors.New("srp password changed") ErrNewSettingsInvalid = errors.New("new password settings invalid") diff --git a/internal/rpc/channels_leave_rpc_test.go b/internal/rpc/channels_leave_rpc_test.go index 22c09360..a2f3acee 100644 --- a/internal/rpc/channels_leave_rpc_test.go +++ b/internal/rpc/channels_leave_rpc_test.go @@ -9,6 +9,7 @@ import ( "github.com/iamxvbaba/td/tgerr" "go.uber.org/zap/zaptest" + appaccount "telesrv/internal/app/account" appchannels "telesrv/internal/app/channels" appusers "telesrv/internal/app/users" "telesrv/internal/domain" @@ -26,6 +27,10 @@ func (acceptPasswordAccountService) CheckPassword(_ context.Context, _ int64, ch return nil } +func (acceptPasswordAccountService) GetPassword(_ context.Context, _ int64) (domain.PasswordSettings, error) { + return domain.PasswordSettings{HasPassword: true}, nil +} + func TestMessagesGetFutureChatCreatorAfterLeaveAndCreatorLeaveTransfers(t *testing.T) { ctx := context.Background() userStore := memory.NewUserStore() @@ -242,6 +247,58 @@ func TestMessagesEditChatCreatorTransfersWithoutChannelPts(t *testing.T) { } } +// TestMessagesEditChatCreatorRequiresPasswordSetup covers an owner who has never +// enabled two-step verification: the client's transfer-ownership probe +// (inputUserEmpty + inputCheckPasswordEmpty) must get PASSWORD_MISSING, not +// PASSWORD_HASH_INVALID -- the desktop client only recognizes PASSWORD_MISSING +// to show its "enable 2FA first" box, and otherwise falls through into a real +// password-entry flow it can't satisfy. +func TestMessagesEditChatCreatorRequiresPasswordSetup(t *testing.T) { + ctx := context.Background() + userStore := memory.NewUserStore() + owner, err := userStore.Create(ctx, domain.User{AccessHash: 9221, Phone: "15550009221", FirstName: "Owner"}) + if err != nil { + t.Fatalf("create owner: %v", err) + } + member, err := userStore.Create(ctx, domain.User{AccessHash: 9222, Phone: "15550009222", FirstName: "Member"}) + if err != nil { + t.Fatalf("create member: %v", err) + } + channelStore := memory.NewChannelStore() + channelService := appchannels.NewService(channelStore) + r := New(Config{}, Deps{ + Account: appaccount.NewService(memory.NewPasswordStore()), + Users: appusers.NewService(userStore), + Channels: channelService, + }, zaptest.NewLogger(t), fixedClock{now: time.Unix(1700009130, 0)}) + created, err := channelService.CreateChannel(ctx, owner.ID, domain.CreateChannelRequest{ + CreatorUserID: owner.ID, + Title: "no password owner", + Megagroup: true, + MemberUserIDs: []int64{member.ID}, + Date: 1700009130, + }) + if err != nil { + t.Fatalf("create channel: %v", err) + } + ownerCtx := WithUserID(ctx, owner.ID) + peer := &tg.InputPeerChannel{ChannelID: created.Channel.ID, AccessHash: created.Channel.AccessHash} + if _, err := r.onMessagesEditChatCreator(ownerCtx, &tg.MessagesEditChatCreatorRequest{ + Peer: peer, + UserID: &tg.InputUserEmpty{}, + Password: &tg.InputCheckPasswordEmpty{}, + }); err == nil || !tgerr.Is(err, "PASSWORD_MISSING") { + t.Fatalf("editChatCreator probe err = %v, want PASSWORD_MISSING", err) + } + if _, err := r.onMessagesEditChatCreator(ownerCtx, &tg.MessagesEditChatCreatorRequest{ + Peer: peer, + UserID: &tg.InputUser{UserID: member.ID, AccessHash: member.AccessHash}, + Password: &tg.InputCheckPasswordSRP{SRPID: 1, A: []byte{1}, M1: []byte{2}}, + }); err == nil || !tgerr.Is(err, "PASSWORD_MISSING") { + t.Fatalf("editChatCreator transfer err = %v, want PASSWORD_MISSING", err) + } +} + func TestMessagesGetFutureChatCreatorAfterLeaveNoCandidate(t *testing.T) { ctx := context.Background() userStore := memory.NewUserStore() diff --git a/internal/rpc/channels_legacy_chat.go b/internal/rpc/channels_legacy_chat.go index b3d653ff..999a3dbc 100644 --- a/internal/rpc/channels_legacy_chat.go +++ b/internal/rpc/channels_legacy_chat.go @@ -339,6 +339,20 @@ func (r *Router) onMessagesEditChatCreator(ctx context.Context, req *tg.Messages if req.Password == nil { return nil, passwordHashInvalidErr() } + if r.deps.Account == nil { + return nil, passwordHashInvalidErr() + } + // 转让所有权无条件要求已开启两步验证：先探测账号是否设有密码， + // 让 messages.editChatCreator 的探测请求（inputUserEmpty + inputCheckPasswordEmpty） + // 拿到 PASSWORD_MISSING 而非 PASSWORD_HASH_INVALID —— 客户端只识别前者来展示 + // “请先开启两步验证”提示，否则会误入真实密码校验流程并在没有密码可核对时崩溃。 + passwordSettings, err := r.deps.Account.GetPassword(ctx, userID) + if err != nil { + return nil, internalErr() + } + if !passwordSettings.HasPassword { + return nil, passwordMissingErr() + } if _, ok := req.UserID.(*tg.InputUserEmpty); ok { return nil, passwordHashInvalidErr() } @@ -355,9 +369,6 @@ func (r *Router) onMessagesEditChatCreator(ctx context.Context, req *tg.Messages if target.Bot { return nil, userIDInvalidErr() } - if r.deps.Account == nil { - return nil, passwordHashInvalidErr() - } if err := r.deps.Account.CheckPassword(ctx, userID, domainPasswordCheck(req.Password)); err != nil { return nil, passwordErr(err) } diff --git a/internal/rpc/errors.go b/internal/rpc/errors.go index 564bb558..197ca0d0 100644 --- a/internal/rpc/errors.go +++ b/internal/rpc/errors.go @@ -291,6 +291,7 @@ func scoreInvalidErr() error { return tgerr.New(400, "SCORE_INVALID") } func sessionPasswordNeededErr() error { return tgerr.New(401, "SESSION_PASSWORD_NEEDED") } func passwordHashInvalidErr() error { return tgerr.New(400, "PASSWORD_HASH_INVALID") } +func passwordMissingErr() error { return tgerr.New(400, "PASSWORD_MISSING") } func srpIDInvalidErr() error { return tgerr.New(400, "SRP_ID_INVALID") } func srpPasswordChangedErr() error { return tgerr.New(400, "SRP_PASSWORD_CHANGED") } func newSettingsInvalidErr() error { return tgerr.New(400, "NEW_SETTINGS_INVALID") } @@ -468,6 +469,8 @@ func passwordErr(err error) error { switch { case errors.Is(err, domain.ErrPasswordHashInvalid): return passwordHashInvalidErr() + case errors.Is(err, domain.ErrPasswordMissing): + return passwordMissingErr() case errors.Is(err, domain.ErrSRPIDInvalid): return srpIDInvalidErr() case errors.Is(err, domain.ErrSRPPasswordChanged): diff --git a/internal/rpc/presence.go b/internal/rpc/presence.go index 92fb5a8d..659e51f3 100644 --- a/internal/rpc/presence.go +++ b/internal/rpc/presence.go @@ -531,6 +531,13 @@ func (r *Router) withUserPresence(u domain.User) domain.User { if u.Bot { return u } + // 已注销账号同理不参与 presence：tgUser/tgSelfUser 对 Deleted 用户直接短路输出 + // 精简 tombstone，从不读取 Status，这里覆盖与否本应无观测差异——但只靠那一层 + // 短路里应外合：任何上游把 Deleted 弄丢的 bug（例如缓存往返丢字段）都会让这里 + // 覆盖出的 Status 变成可见的“最近上线”，掩盖真正的 bug 而不是让它更早炸出来。 + if u.Deleted { + return u + } u.Status = r.userPresenceStatusForUser(u) return u }