diff --git a/db/db.go b/db/db.go index 6586e763..d2ec60ba 100644 --- a/db/db.go +++ b/db/db.go @@ -90,6 +90,15 @@ func (d *DB) ReadOnly() *sql.DB { return d.primary.DB } +// MarkReplicasHealthyForTest immediately marks all configured replicas as healthy, bypassing the +// async health-check loop's initial delay. It exists so tests can deterministically route +// ReadOnly() to a replica without waiting; it is not used in production code. +func (d *DB) MarkReplicasHealthyForTest() { + for _, r := range d.replicas { + r.healthy.Store(true) + } +} + // Close closes the primary database and all replicas, and stops the health-check goroutine. func (d *DB) Close() error { d.cancel() diff --git a/docs/releases.md b/docs/releases.md index 407e70b7..26e0f744 100644 --- a/docs/releases.md +++ b/docs/releases.md @@ -1983,6 +1983,13 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release ## Not released yet +### ntfy server v2.26.x (UNRELEASED) + +**Bug fixes + maintenance:** + +* `GET /account` now reads from the primary database instead of a read replica, so the account view no longer shows stale data right after a change when replicas lag behind +* The web app no longer issues a redundant second `GET /account` after email actions (e.g. setting a primary email); it relies on the sync-topic event for the refresh + ### ntfy Android v1.25.x (UNRELEASED) This release makes the "connection lost" alert configurable and turns it off by default. Folks did not like it and many reached out diff --git a/server/server_account.go b/server/server_account.go index e28df08d..34d93d6b 100644 --- a/server/server_account.go +++ b/server/server_account.go @@ -180,10 +180,6 @@ func (s *Server) handleAccountGet(w http.ResponseWriter, r *http.Request, v *vis if err != nil { return err } - primaryEmail, err := s.userManager.PrimaryEmail(u.ID) - if err != nil { - return err - } pendingEmails, err := s.userManager.PendingEmails(u.ID) if err != nil { return err @@ -191,7 +187,7 @@ func (s *Server) handleAccountGet(w http.ResponseWriter, r *http.Request, v *vis // Combine verified (with primary flag) and pending (unverified) into one list emailInfos := make([]*apiAccountEmailInfo, 0, len(emails)+len(pendingEmails)) for _, email := range emails { - emailInfos = append(emailInfos, &apiAccountEmailInfo{Address: email, Primary: email == primaryEmail}) + emailInfos = append(emailInfos, &apiAccountEmailInfo{Address: email.Address, Primary: email.Primary}) } for _, email := range pendingEmails { emailInfos = append(emailInfos, &apiAccountEmailInfo{Address: email, Pending: true}) @@ -667,7 +663,7 @@ func (s *Server) handleAccountEmailAdd(w http.ResponseWriter, r *http.Request, v emails, err := s.userManager.Emails(u.ID) if err != nil { return err - } else if util.Contains(emails, req.Email) { + } else if emails.Contains(req.Email) { return errHTTPConflictEmailExists } // Rate limit (counts against the user's email quota) @@ -888,7 +884,7 @@ func (s *Server) convertEmailAddress(u *user.User, email string) (string, *errHT if err != nil { return "", errHTTPInternalError } else if len(emails) > 0 { - return emails[0], nil + return emails[0].Address, nil } return "", errHTTPBadRequestEmailAddressNotVerified } @@ -903,7 +899,7 @@ func (s *Server) convertEmailAddress(u *user.User, email string) (string, *errHT emails, err := s.userManager.Emails(u.ID) if err != nil { return "", errHTTPInternalError - } else if util.Contains(emails, email) { + } else if emails.Contains(email) { return email, nil } return "", errHTTPBadRequestEmailAddressNotVerified diff --git a/server/server_payments_email_test.go b/server/server_payments_email_test.go index 42a7aa43..2550d88e 100644 --- a/server/server_payments_email_test.go +++ b/server/server_payments_email_test.go @@ -72,7 +72,7 @@ func TestPayments_Checkout_SendsBillingEmailVerification(t *testing.T) { emails, err := s.userManager.Emails(u.ID) require.Nil(t, err) - require.Equal(t, []string{"billing@example.com"}, emails) + require.Equal(t, []string{"billing@example.com"}, emails.Strings()) primary, err := s.userManager.PrimaryEmail(u.ID) require.Nil(t, err) require.Equal(t, "billing@example.com", primary) diff --git a/user/manager.go b/user/manager.go index ba2c77d3..023243e7 100644 --- a/user/manager.go +++ b/user/manager.go @@ -941,7 +941,9 @@ func (a *Manager) RemoveReservations(username string, topics ...string) error { // Reservations returns all user-owned topics, and the associated everyone-access func (a *Manager) Reservations(username string) ([]Reservation, error) { - return a.reservationsTx(a.db.ReadOnly(), username) + // Read from the primary, not a replica: this backs GET /account, which the web app refetches + // immediately after a sync event. Replication lag would otherwise show stale data. + return a.reservationsTx(a.db, username) } func (a *Manager) reservationsTx(tx db.Querier, username string) ([]Reservation, error) { @@ -1204,7 +1206,8 @@ func (a *Manager) Token(userID, token string) (*Token, error) { // Tokens returns all existing tokens for the user with the given user ID func (a *Manager) Tokens(userID string) ([]*Token, error) { - rows, err := a.db.ReadOnly().Query(a.queries.selectTokens, userID) + // Primary read: backs GET /account (read-your-writes after a sync event). + rows, err := a.db.Query(a.queries.selectTokens, userID) if err != nil { return nil, err } @@ -1417,7 +1420,8 @@ func (a *Manager) readTier(rows *sql.Rows) (*Tier, error) { // PhoneNumbers returns all phone numbers for the user with the given user ID func (a *Manager) PhoneNumbers(userID string) ([]string, error) { - rows, err := a.db.ReadOnly().Query(a.queries.selectPhoneNumbers, userID) + // Primary read: backs GET /account (read-your-writes after a sync event). + rows, err := a.db.Query(a.queries.selectPhoneNumbers, userID) if err != nil { return nil, err } @@ -1465,14 +1469,17 @@ func (a *Manager) readPhoneNumber(rows *sql.Rows) (string, error) { return phoneNumber, nil } -// Emails returns all verified email addresses for the user with the given user ID -func (a *Manager) Emails(userID string) ([]string, error) { - rows, err := a.db.ReadOnly().Query(a.queries.selectEmails, userID) +// Emails returns all verified email addresses for the user with the given user ID, each carrying +// whether it is the primary (recovery) address. Because the primary flag is included, callers that +// need it (e.g. the account view) do not need a separate PrimaryEmail call. +func (a *Manager) Emails(userID string) (Emails, error) { + // Primary read: backs GET /account (read-your-writes after a sync event). + rows, err := a.db.Query(a.queries.selectEmails, userID) if err != nil { return nil, err } defer rows.Close() - emails := make([]string, 0) + emails := make(Emails, 0) for { email, err := a.readEmail(rows) if errors.Is(err, ErrEmailNotFound) { @@ -1508,7 +1515,8 @@ func (a *Manager) RemoveEmail(userID, email string) error { // the user has not designated one. func (a *Manager) PrimaryEmail(userID string) (string, error) { var email sql.NullString - err := a.db.ReadOnly().QueryRow(a.queries.selectPrimaryEmail, userID).Scan(&email) + // Primary read: backs GET /account (read-your-writes after a sync event). + err := a.db.QueryRow(a.queries.selectPrimaryEmail, userID).Scan(&email) if errors.Is(err, sql.ErrNoRows) { return "", nil } else if err != nil { @@ -1534,7 +1542,8 @@ func (a *Manager) UserIDByPrimaryEmail(email string) (string, error) { // PendingEmails returns the user's unverified (pending) email addresses, i.e. addresses with // an outstanding email-verification magic link. func (a *Manager) PendingEmails(userID string) ([]string, error) { - rows, err := a.db.ReadOnly().Query(a.queries.selectPendingEmails, string(MagicLinkKindEmailVerify), userID) + // Primary read: backs GET /account (read-your-writes after a sync event). + rows, err := a.db.Query(a.queries.selectPendingEmails, string(MagicLinkKindEmailVerify), userID) if err != nil { return nil, err } @@ -1756,17 +1765,18 @@ func (a *Manager) deleteExpiredMagicLinks() error { return err } -func (a *Manager) readEmail(rows *sql.Rows) (string, error) { - var email string +func (a *Manager) readEmail(rows *sql.Rows) (*Email, error) { + var address string + var primary bool if !rows.Next() { - return "", ErrEmailNotFound + return nil, ErrEmailNotFound } - if err := rows.Scan(&email); err != nil { - return "", err + if err := rows.Scan(&address, &primary); err != nil { + return nil, err } else if err := rows.Err(); err != nil { - return "", err + return nil, err } - return email, nil + return &Email{Address: address, Primary: primary}, nil } // ChangeBilling updates a user's billing fields diff --git a/user/manager_postgres.go b/user/manager_postgres.go index a9e12d50..ad6d63b8 100644 --- a/user/manager_postgres.go +++ b/user/manager_postgres.go @@ -217,7 +217,7 @@ const ( postgresDeletePhoneNumberQuery = `DELETE FROM user_phone WHERE user_id = $1 AND phone_number = $2` // Email queries - postgresSelectEmailsQuery = `SELECT email FROM user_email WHERE user_id = $1 ORDER BY email` + postgresSelectEmailsQuery = `SELECT email, is_primary FROM user_email WHERE user_id = $1 ORDER BY email` postgresInsertEmailQuery = `INSERT INTO user_email (user_id, email) VALUES ($1, $2)` postgresInsertEmailIgnoreQuery = `INSERT INTO user_email (user_id, email) VALUES ($1, $2) ON CONFLICT (user_id, email) DO NOTHING` postgresDeleteEmailQuery = `DELETE FROM user_email WHERE user_id = $1 AND email = $2` diff --git a/user/manager_sqlite.go b/user/manager_sqlite.go index a2a46e18..1d6f2fa1 100644 --- a/user/manager_sqlite.go +++ b/user/manager_sqlite.go @@ -214,7 +214,7 @@ const ( sqliteDeletePhoneNumberQuery = `DELETE FROM user_phone WHERE user_id = ? AND phone_number = ?` // Email queries - sqliteSelectEmailsQuery = `SELECT email FROM user_email WHERE user_id = ? ORDER BY email` + sqliteSelectEmailsQuery = `SELECT email, is_primary FROM user_email WHERE user_id = ? ORDER BY email` sqliteInsertEmailQuery = `INSERT INTO user_email (user_id, email) VALUES (?, ?)` sqliteInsertEmailIgnoreQuery = `INSERT INTO user_email (user_id, email) VALUES (?, ?) ON CONFLICT (user_id, email) DO NOTHING` sqliteDeleteEmailQuery = `DELETE FROM user_email WHERE user_id = ? AND email = ?` diff --git a/user/manager_test.go b/user/manager_test.go index 13497a3b..a7b205b0 100644 --- a/user/manager_test.go +++ b/user/manager_test.go @@ -1157,7 +1157,7 @@ func TestUser_EmailAddListRemove(t *testing.T) { emails, err := a.Emails(phil.ID) require.Nil(t, err) require.Equal(t, 1, len(emails)) - require.Equal(t, "phil@example.com", emails[0]) + require.Equal(t, "phil@example.com", emails[0].Address) require.Nil(t, a.RemoveEmail(phil.ID, "phil@example.com")) emails, err = a.Emails(phil.ID) @@ -2701,7 +2701,7 @@ func TestStoreEmails(t *testing.T) { emails, err = manager.Emails(u.ID) require.Nil(t, err) require.Len(t, emails, 1) - require.Equal(t, "phil2@example.com", emails[0]) + require.Equal(t, "phil2@example.com", emails[0].Address) }) } @@ -2871,7 +2871,7 @@ func TestUser_MagicLink_VerifyEmail_SetsPrimary(t *testing.T) { emails, err = a.Emails(phil.ID) require.Nil(t, err) - require.Equal(t, []string{"phil@example.com"}, emails) + require.Equal(t, []string{"phil@example.com"}, emails.Strings()) primary, err = a.PrimaryEmail(phil.ID) require.Nil(t, err) require.Equal(t, "phil@example.com", primary) @@ -2904,7 +2904,7 @@ func TestUser_MagicLink_VerifyEmail_SecondStaysSecondary(t *testing.T) { // Both verified, but primary is still the first emails, err := a.Emails(phil.ID) require.Nil(t, err) - require.Equal(t, []string{"first@example.com", "second@example.com"}, emails) + require.Equal(t, []string{"first@example.com", "second@example.com"}, emails.Strings()) primary, err := a.PrimaryEmail(phil.ID) require.Nil(t, err) require.Equal(t, "first@example.com", primary) @@ -2933,7 +2933,7 @@ func TestUser_MagicLink_PrimaryGlobalUniqueness(t *testing.T) { require.Nil(t, err) emails, err := a.Emails(ben.ID) require.Nil(t, err) - require.Equal(t, []string{"shared@example.com"}, emails) + require.Equal(t, []string{"shared@example.com"}, emails.Strings()) primary, err = a.PrimaryEmail(ben.ID) require.Nil(t, err) require.Equal(t, "", primary) @@ -3168,7 +3168,7 @@ func TestUser_MagicLink_VerifyEmail_ProvisionedGetsPrimary(t *testing.T) { emails, err := a.Emails(prov.ID) require.Nil(t, err) - require.Equal(t, []string{"prov@example.com"}, emails) + require.Equal(t, []string{"prov@example.com"}, emails.Strings()) primary, err := a.PrimaryEmail(prov.ID) require.Nil(t, err) require.Equal(t, "prov@example.com", primary) @@ -3219,3 +3219,97 @@ func TestUser_MagicLink_UserIDByPrimaryEmail_NotFound(t *testing.T) { require.ErrorIs(t, err, ErrUserNotFound) }) } + +func TestManager_Emails_PrimaryFlagAndHelpers(t *testing.T) { + forEachBackend(t, func(t *testing.T, newManager newManagerFunc) { + a := newTestManager(t, newManager, PermissionDenyAll) + require.Nil(t, a.AddUser("phil", "phil", RoleUser, false)) + u, err := a.User("phil") + require.Nil(t, err) + require.Nil(t, a.AddEmail(u.ID, "a@example.com")) + require.Nil(t, a.AddEmail(u.ID, "b@example.com")) + require.Nil(t, a.SetPrimaryEmail(u.ID, "b@example.com")) + + // Emails() carries the primary flag, so a separate PrimaryEmail() call is unnecessary. + emails, err := a.Emails(u.ID) + require.Nil(t, err) + require.Len(t, emails, 2) + require.Equal(t, "a@example.com", emails[0].Address) // ORDER BY email + require.False(t, emails[0].Primary) + require.Equal(t, "b@example.com", emails[1].Address) + require.True(t, emails[1].Primary) + + // Helper methods for the address-only callers + require.Equal(t, []string{"a@example.com", "b@example.com"}, emails.Strings()) + require.True(t, emails.Contains("a@example.com")) + require.False(t, emails.Contains("c@example.com")) + }) +} + +// openReplicaTestSQLite opens a fresh SQLite database file with the user schema applied. +func openReplicaTestSQLite(t *testing.T, filename string) *sql.DB { + d, err := sql.Open("sqlite3", filename+"?_case_sensitive_like=on") + require.Nil(t, err) + require.Nil(t, setupSQLite(d)) + return d +} + +// TestManager_AccountReadsUsePrimary verifies that the per-user reads backing the GET /account +// endpoint read from the primary, not from a read replica. The sync event ("something changed") +// is published immediately after a write, so a replica that lags would make the account view +// stale right after the user changes it. These reads must therefore be read-your-writes consistent. +// +// The test wires up a primary and a deliberately-empty replica (simulating replication lag), +// forces the replica healthy so ReadOnly() would route to it, writes everything to the primary, +// and asserts the reads still observe the fresh primary data. +func TestManager_AccountReadsUsePrimary(t *testing.T) { + dir := t.TempDir() + primaryDB := openReplicaTestSQLite(t, filepath.Join(dir, "primary.db")) + replicaDB := openReplicaTestSQLite(t, filepath.Join(dir, "replica.db")) // intentionally left empty + + pool := db.New(&db.Host{DB: primaryDB}, []*db.Host{{DB: replicaDB}}) + pool.MarkReplicasHealthyForTest() // force ReadOnly() to route to the stale replica + a, err := newManager(pool, sqliteQueries, &Config{BcryptCost: bcrypt.MinCost}) + require.Nil(t, err) + t.Cleanup(func() { a.Close() }) + + // All writes below go to the primary; the replica stays empty. + require.Nil(t, a.AddUser("phil", "phil", RoleUser, false)) + u, err := a.User("phil") + require.Nil(t, err) + _, err = a.CreateToken(u.ID, "test token", time.Now().Add(time.Hour), netip.IPv4Unspecified(), false) + require.Nil(t, err) + require.Nil(t, a.AddReservation("phil", "mytopic", PermissionDenyAll, 10)) + require.Nil(t, a.AddPhoneNumber(u.ID, "+12223334444")) + require.Nil(t, a.AddEmail(u.ID, "phil@example.com")) + require.Nil(t, a.SetPrimaryEmail(u.ID, "phil@example.com")) + _, err = a.AddMagicLink(MagicLinkKindEmailVerify, u.ID, "pending@example.com", time.Hour) + require.Nil(t, err) + + // Each read must observe the just-written primary data, NOT the empty replica. + tokens, err := a.Tokens(u.ID) + require.Nil(t, err) + require.Len(t, tokens, 1) + + reservations, err := a.Reservations("phil") + require.Nil(t, err) + require.Len(t, reservations, 1) + + phoneNumbers, err := a.PhoneNumbers(u.ID) + require.Nil(t, err) + require.Len(t, phoneNumbers, 1) + + emails, err := a.Emails(u.ID) + require.Nil(t, err) + require.Len(t, emails, 1) + require.Equal(t, "phil@example.com", emails[0].Address) + require.True(t, emails[0].Primary) + + primaryEmail, err := a.PrimaryEmail(u.ID) + require.Nil(t, err) + require.Equal(t, "phil@example.com", primaryEmail) + + pendingEmails, err := a.PendingEmails(u.ID) + require.Nil(t, err) + require.Len(t, pendingEmails, 1) +} diff --git a/user/types.go b/user/types.go index e198b273..344bfa5d 100644 --- a/user/types.go +++ b/user/types.go @@ -182,6 +182,36 @@ type Reservation struct { Everyone Permission } +// Email is a verified email address on a user account, along with whether it is the user's +// designated primary (recovery) address. +type Email struct { + Address string + Primary bool +} + +// Emails is a list of verified email addresses for a user. +type Emails []*Email + +// Strings returns just the address strings, in the same order. It is a convenience for callers +// that only care about the addresses and not the primary flag. +func (e Emails) Strings() []string { + addresses := make([]string, len(e)) + for i, email := range e { + addresses[i] = email.Address + } + return addresses +} + +// Contains reports whether the given address is in the list. +func (e Emails) Contains(address string) bool { + for _, email := range e { + if email.Address == address { + return true + } + } + return false +} + // Permission represents a read or write permission to a topic type Permission uint8 diff --git a/web/src/components/Account.jsx b/web/src/components/Account.jsx index ffbe32a9..3369e68e 100644 --- a/web/src/components/Account.jsx +++ b/web/src/components/Account.jsx @@ -402,13 +402,12 @@ const Emails = () => { setSnack(t("account_basics_emails_copied_to_clipboard")); }; - // runEmailAction wraps an account API call with the shared error handling (redirect on - // unauthorized, surface a message otherwise). On success it refetches the account so the email - // list reflects the change immediately, rather than waiting for the async sync event. + // runEmailAction wraps an email account action with the shared error handling used by the + // delete/set-primary/resend handlers (redirect on unauthorized, surface a message otherwise). + // The account view refreshes via the server's sync event, so there's no explicit refetch here. const runEmailAction = async (fn, errorMessage) => { try { await fn(); - await accountApi.sync(); } catch (e) { console.log(`[Account] Email action failed`, e); if (e instanceof UnauthorizedError) { @@ -569,7 +568,6 @@ const AddEmailDialog = (props) => { setSending(true); setError(""); // Clear any error from a previous attempt await accountApi.startEmailVerification(email); - await accountApi.sync(); // Refresh so the new "(unverified)" address shows up immediately setSent(true); } catch (e) { console.log(`[Account] Error starting email verification`, e);