diff --git a/cmd/user.go b/cmd/user.go index d9d5b0fc..9e33e1da 100644 --- a/cmd/user.go +++ b/cmd/user.go @@ -350,7 +350,7 @@ func execUserResetPass(c *cli.Context) error { } } // The reset token is bound to the user, not an email -- so this works even with no SMTP - token, err := manager.CreateMagicLink(user.MagicLinkKindPasswordReset, u.ID, "", time.Hour) + token, err := manager.AddMagicLink(user.MagicLinkKindPasswordReset, u.ID, "", time.Hour) if err != nil { return err } @@ -398,7 +398,7 @@ func execUserHash(c *cli.Context) error { if err != nil { return err } - hash, err := user.HashPassword(password) + hash, err := user.HashPassword(password, user.DefaultUserPasswordBcryptCost) if err != nil { return fmt.Errorf("failed to hash password: %w", err) } diff --git a/server/server_account.go b/server/server_account.go index 7598a91f..807ad453 100644 --- a/server/server_account.go +++ b/server/server_account.go @@ -783,7 +783,7 @@ func (s *Server) enqueueEmailVerification(userID, email string) error { if s.config.BaseURL == "" { return errHTTPInternalErrorMissingBaseURL } - token, err := s.userManager.CreateMagicLink(user.MagicLinkKindEmailVerify, userID, email, emailVerificationTokenExpiry) + token, err := s.userManager.AddMagicLink(user.MagicLinkKindEmailVerify, userID, email, emailVerificationTokenExpiry) if err != nil { return err } @@ -808,7 +808,7 @@ func (s *Server) handleAccountPasswordResetRequest(w http.ResponseWriter, r *htt identifier := strings.TrimSpace(req.Identifier) if identifier != "" && s.config.BaseURL != "" { if userID, email, ok := s.resolveResetTarget(identifier); ok { - token, err := s.userManager.CreateMagicLink(user.MagicLinkKindPasswordReset, userID, "", passwordResetTokenExpiry) + token, err := s.userManager.AddMagicLink(user.MagicLinkKindPasswordReset, userID, "", passwordResetTokenExpiry) if err != nil { logvr(v, r).Tag(tagAccount).Err(err).Warn("Failed to create password reset token") } else { diff --git a/server/server_account_email_test.go b/server/server_account_email_test.go index 9bbcccd5..88ebf1cb 100644 --- a/server/server_account_email_test.go +++ b/server/server_account_email_test.go @@ -366,7 +366,7 @@ func TestAccount_Signup_WithoutEmail_NoSend(t *testing.T) { func TestAccount_Email_ProvisionedNoPrimary(t *testing.T) { forEachBackend(t, func(t *testing.T, databaseURL string) { - hash, err := user.HashPassword("provpass") + hash, err := user.HashPassword("provpass", user.DefaultUserPasswordBcryptCost) require.Nil(t, err) conf := newTestConfigWithAuthFile(t, databaseURL) conf.SMTPSenderAddr = "localhost:25" diff --git a/user/manager.go b/user/manager.go index e98adee0..8aea195a 100644 --- a/user/manager.go +++ b/user/manager.go @@ -73,6 +73,9 @@ func newManager(d *db.DB, queries queries, config *Config) (*Manager, error) { if config.AccessCacheReloadInterval <= 0 { config.AccessCacheReloadInterval = DefaultAccessCacheReloadInterval } + if config.ExpiredMagicLinkReapInterval <= 0 { + config.ExpiredMagicLinkReapInterval = DefaultExpiredMagicLinkReapInterval + } manager := &Manager{ config: config, db: d, @@ -92,7 +95,7 @@ func newManager(d *db.DB, queries queries, config *Config) (*Manager, error) { go manager.asyncAccessCacheReloadLoop(manager.config.AccessCacheReloadInterval) } go manager.asyncQueueWriteLoop(manager.config.QueueWriterInterval) - go manager.asyncExpiredMagicLinkReapLoop(DefaultExpiredMagicLinkReapInterval) + go manager.asyncExpiredMagicLinkReapLoop(manager.config.ExpiredMagicLinkReapInterval) return manager, nil } @@ -664,7 +667,7 @@ func (a *Manager) maybeHashPassword(password string, hashed bool) (string, error } return password, nil } - return hashPassword(password, a.config.BcryptCost) + return HashPassword(password, a.config.BcryptCost) } // Authorize returns nil if the given user has access to the given topic using the desired @@ -1569,11 +1572,15 @@ func (a *Manager) SetPrimaryEmail(userID, email string) error { }) } -// CreateMagicLink generates a fresh magic-link token of the given kind, stores it (hashed, -// replacing any existing link in the same scope), and returns the RAW token for use in the -// emailed link. Only the hash is persisted; the raw token is never stored. email is the -// address being verified for email_verify, and "" for password_reset. -func (a *Manager) CreateMagicLink(kind MagicLinkKind, userID, email string, ttl time.Duration) (string, error) { +// AddMagicLink generates a fresh magic-link token of the given kind, stores it (hashed, replacing +// any existing link in the same scope), and returns the RAW token for use in the emailed link. +// Only the hash is persisted; the raw token is never stored. email is the address being verified +// for email_verify, and "" for password_reset. +// +// The scope replaced is, for email_verify, the (user_id, email) pair (one pending verification per +// address); for password_reset, the user_id (one active reset per account). The replace-delete and +// the insert run in one transaction so a re-request atomically supersedes the old token. +func (a *Manager) AddMagicLink(kind MagicLinkKind, userID, email string, ttl time.Duration) (string, error) { token := generateLinkToken() now := time.Now() m := &MagicLink{ @@ -1584,35 +1591,14 @@ func (a *Manager) CreateMagicLink(kind MagicLinkKind, userID, email string, ttl Expires: now.Add(ttl).Unix(), Created: now.Unix(), } - if err := a.AddMagicLink(m); err != nil { - return "", err - } - return token, nil -} - -// MagicLinkByToken looks up a magic link by its raw token (hashing it first). See MagicLinkByHash. -func (a *Manager) MagicLinkByToken(rawToken string) (*MagicLink, error) { - return a.MagicLinkByHash(hashToken(rawToken)) -} - -// DeleteMagicLinkByToken deletes a magic link identified by its raw token (single-use consume). -func (a *Manager) DeleteMagicLinkByToken(rawToken string) error { - return a.DeleteMagicLink(hashToken(rawToken)) -} - -// AddMagicLink stores a pending magic link, replacing any existing link in the same scope: -// for email_verify that is the (user_id, email) pair (one pending verification per address); -// for password_reset that is the user_id (one active reset per account). The replace-delete and -// the insert run in one transaction so a re-request atomically supersedes the old token. -func (a *Manager) AddMagicLink(m *MagicLink) error { - return db.ExecTx(a.db, func(tx *sql.Tx) error { + err := db.ExecTx(a.db, func(tx *sql.Tx) error { switch m.Kind { case MagicLinkKindEmailVerify: - if _, err := tx.Exec(a.queries.deleteVerifyScope, string(MagicLinkKindEmailVerify), m.UserID, m.Email); err != nil { + if _, err := tx.Exec(a.queries.deleteMagicLinkEmailVerify, string(MagicLinkKindEmailVerify), m.UserID, m.Email); err != nil { return err } case MagicLinkKindPasswordReset: - if _, err := tx.Exec(a.queries.deleteResetScope, string(MagicLinkKindPasswordReset), m.UserID); err != nil { + if _, err := tx.Exec(a.queries.deleteMagicLinkResetPassword, string(MagicLinkKindPasswordReset), m.UserID); err != nil { return err } default: @@ -1623,6 +1609,15 @@ func (a *Manager) AddMagicLink(m *MagicLink) error { } return nil }) + if err != nil { + return "", err + } + return token, nil +} + +// MagicLinkByToken looks up a magic link by its raw token (hashing it first). See MagicLinkByHash. +func (a *Manager) MagicLinkByToken(rawToken string) (*MagicLink, error) { + return a.MagicLinkByHash(hashToken(rawToken)) } // MagicLinkByHash looks up a magic link by the hex SHA-256 of its raw token, returning @@ -1643,17 +1638,18 @@ func (a *Manager) MagicLinkByHash(tokenHash string) (*MagicLink, error) { return &m, nil } -// DeleteMagicLink deletes a magic link by its token hash. Used to enforce single use after a -// reset is performed (email verification deletes the row inside VerifyEmail's transaction). -func (a *Manager) DeleteMagicLink(tokenHash string) error { - _, err := a.db.Exec(a.queries.deleteMagicLinkByHash, tokenHash) +// DeleteMagicLinkByToken deletes a magic link identified by its raw token (single-use consume). +// Used to enforce single use after a reset is performed (email verification deletes the row +// inside VerifyEmail's transaction). +func (a *Manager) DeleteMagicLinkByToken(rawToken string) error { + _, err := a.db.Exec(a.queries.deleteMagicLinkByHash, hashToken(rawToken)) return err } // DeleteEmailVerification removes any pending email verification for (userID, email). Used when // an unverified (pending) address is cancelled/deleted from the account. func (a *Manager) DeleteEmailVerification(userID, email string) error { - _, err := a.db.Exec(a.queries.deleteVerifyScope, string(MagicLinkKindEmailVerify), userID, email) + _, err := a.db.Exec(a.queries.deleteMagicLinkEmailVerify, string(MagicLinkKindEmailVerify), userID, email) return err } @@ -1736,7 +1732,7 @@ func (a *Manager) ResetPassword(rawToken, password string) error { if u.Provisioned { return ErrProvisionedUserChange // Provisioned users get their password from the config file, not reset } - hash, err := a.maybeHashPassword(password, false) + hash, err := HashPassword(password, a.config.BcryptCost) if err != nil { return err } diff --git a/user/manager_postgres.go b/user/manager_postgres.go index 57fcf312..a9e12d50 100644 --- a/user/manager_postgres.go +++ b/user/manager_postgres.go @@ -329,8 +329,8 @@ var postgresQueries = queries{ insertMagicLink: postgresInsertMagicLinkQuery, selectMagicLinkByHash: postgresSelectMagicLinkByHashQuery, deleteMagicLinkByHash: postgresDeleteMagicLinkByHashQuery, - deleteVerifyScope: postgresDeleteVerifyScopeQuery, - deleteResetScope: postgresDeleteResetScopeQuery, + deleteMagicLinkEmailVerify: postgresDeleteVerifyScopeQuery, + deleteMagicLinkResetPassword: postgresDeleteResetScopeQuery, selectPendingEmails: postgresSelectPendingEmailsQuery, deleteExpiredMagicLinks: postgresDeleteExpiredMagicLinksQuery, updateBilling: postgresUpdateBillingQuery, diff --git a/user/manager_sqlite.go b/user/manager_sqlite.go index 9bcf13df..a2a46e18 100644 --- a/user/manager_sqlite.go +++ b/user/manager_sqlite.go @@ -325,8 +325,8 @@ var sqliteQueries = queries{ insertMagicLink: sqliteInsertMagicLinkQuery, selectMagicLinkByHash: sqliteSelectMagicLinkByHashQuery, deleteMagicLinkByHash: sqliteDeleteMagicLinkByHashQuery, - deleteVerifyScope: sqliteDeleteVerifyScopeQuery, - deleteResetScope: sqliteDeleteResetScopeQuery, + deleteMagicLinkEmailVerify: sqliteDeleteVerifyScopeQuery, + deleteMagicLinkResetPassword: sqliteDeleteResetScopeQuery, selectPendingEmails: sqliteSelectPendingEmailsQuery, deleteExpiredMagicLinks: sqliteDeleteExpiredMagicLinksQuery, updateBilling: sqliteUpdateBillingQuery, diff --git a/user/manager_test.go b/user/manager_test.go index 6bc0bccd..c6dd2467 100644 --- a/user/manager_test.go +++ b/user/manager_test.go @@ -2,6 +2,7 @@ package user import ( "database/sql" + "errors" "fmt" "net/netip" "path/filepath" @@ -1847,7 +1848,7 @@ func TestMigrationFrom7(t *testing.T) { require.Equal(t, "", primary) // The new magic-link machinery works post-migration - raw, err := a.CreateMagicLink(MagicLinkKindEmailVerify, "u_phil", "new@example.com", 24*time.Hour) + raw, err := a.AddMagicLink(MagicLinkKindEmailVerify, "u_phil", "new@example.com", 24*time.Hour) require.Nil(t, err) m, err := a.VerifyEmail(raw) require.Nil(t, err) @@ -2901,7 +2902,7 @@ func TestStoreOtherAccessCount(t *testing.T) { // addVerifyLink stores an email-verification magic link and returns the raw token so the test // can "click" it via VerifyEmail. func addVerifyLink(t *testing.T, a *Manager, userID, email string, ttl time.Duration) string { - raw, err := a.CreateMagicLink(MagicLinkKindEmailVerify, userID, email, ttl) + raw, err := a.AddMagicLink(MagicLinkKindEmailVerify, userID, email, ttl) require.Nil(t, err) return raw } @@ -3083,7 +3084,7 @@ func TestUser_MagicLink_PasswordReset_RoundTrip(t *testing.T) { phil, err := a.User("phil") require.Nil(t, err) - raw, err := a.CreateMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) + raw, err := a.AddMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) require.Nil(t, err) m, err := a.MagicLinkByToken(raw) @@ -3098,7 +3099,7 @@ func TestUser_MagicLink_PasswordReset_RoundTrip(t *testing.T) { require.Equal(t, 0, len(pending)) // New request replaces the old token - raw2, err := a.CreateMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) + raw2, err := a.AddMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) require.Nil(t, err) _, err = a.MagicLinkByToken(raw) require.ErrorIs(t, err, ErrMagicLinkNotFound) @@ -3130,6 +3131,37 @@ func TestUser_MagicLink_Reaper(t *testing.T) { }) } +// TestUser_MagicLink_ReaperLoop proves the background reap goroutine actually runs on its +// configured interval: an expired link inserted into a manager with a tiny reap interval is +// deleted without anyone calling deleteExpiredMagicLinks directly. Mirrors the loop-coverage +// pattern of TestAccessCacheReloadInterval_PicksUpExternalWrite. +func TestUser_MagicLink_ReaperLoop(t *testing.T) { + forEachBackend(t, func(t *testing.T, newManager newManagerFunc) { + a := newTestManagerFromConfig(t, newManager, &Config{ + DefaultAccess: PermissionDenyAll, + BcryptCost: bcrypt.MinCost, + ExpiredMagicLinkReapInterval: 25 * time.Millisecond, + }) + require.Nil(t, a.AddUser("phil", "phil", RoleUser, false)) + phil, err := a.User("phil") + require.Nil(t, err) + + expired := addVerifyLink(t, a, phil.ID, "expired@example.com", -time.Hour) + valid := addVerifyLink(t, a, phil.ID, "valid@example.com", time.Hour) + + // The background loop (not a direct call) must reap the expired link within a few intervals + require.Eventually(t, func() bool { + _, err := a.MagicLinkByToken(expired) + return errors.Is(err, ErrMagicLinkNotFound) + }, 2*time.Second, 10*time.Millisecond, "reaper loop never deleted the expired magic link") + + // The unexpired link must survive + m, err := a.MagicLinkByToken(valid) + require.Nil(t, err) + require.Equal(t, "valid@example.com", m.Email) + }) +} + func TestUser_MagicLink_ResetPassword(t *testing.T) { forEachBackend(t, func(t *testing.T, newManager newManagerFunc) { a := newTestManager(t, newManager, PermissionDenyAll) @@ -3137,7 +3169,7 @@ func TestUser_MagicLink_ResetPassword(t *testing.T) { phil, err := a.User("phil") require.Nil(t, err) - raw, err := a.CreateMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) + raw, err := a.AddMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) require.Nil(t, err) // Old password works before reset @@ -3169,7 +3201,7 @@ func TestUser_MagicLink_ResetPassword_WrongKindRejected(t *testing.T) { require.ErrorIs(t, a.ResetPassword(verifyToken, "newpass"), ErrMagicLinkNotFound) // ...and a reset token must not be usable for email verification - resetToken, err := a.CreateMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) + resetToken, err := a.AddMagicLink(MagicLinkKindPasswordReset, phil.ID, "", time.Hour) require.Nil(t, err) _, err = a.VerifyEmail(resetToken) require.ErrorIs(t, err, ErrMagicLinkNotFound) @@ -3221,7 +3253,7 @@ func TestUser_MagicLink_ResetPassword_ProvisionedRejected(t *testing.T) { // A reset token can be created, but consuming it must be rejected for a provisioned user // (their password comes from the config file, like change-pass). - raw, err := a.CreateMagicLink(MagicLinkKindPasswordReset, prov.ID, "", time.Hour) + raw, err := a.AddMagicLink(MagicLinkKindPasswordReset, prov.ID, "", time.Hour) require.Nil(t, err) require.ErrorIs(t, a.ResetPassword(raw, "newpass"), ErrProvisionedUserChange) }) @@ -3234,7 +3266,7 @@ func TestUser_MagicLink_ResetPassword_Expired(t *testing.T) { phil, err := a.User("phil") require.Nil(t, err) - raw, err := a.CreateMagicLink(MagicLinkKindPasswordReset, phil.ID, "", -time.Minute) + raw, err := a.AddMagicLink(MagicLinkKindPasswordReset, phil.ID, "", -time.Minute) require.Nil(t, err) require.ErrorIs(t, a.ResetPassword(raw, "newpass"), ErrMagicLinkNotFound) _, err = a.Authenticate("phil", "oldpass") diff --git a/user/types.go b/user/types.go index ce7efe36..e198b273 100644 --- a/user/types.go +++ b/user/types.go @@ -266,18 +266,19 @@ const ( // Config holds the configuration for the user Manager type Config struct { - Filename string // Database filename, e.g. "/var/lib/ntfy/user.db" (SQLite) - DatabaseURL string // Database connection string (PostgreSQL) - StartupQueries string // Queries to run on startup, e.g. to create initial users or tiers (SQLite only) - DefaultAccess Permission // Default permission if no ACL matches - ProvisionEnabled bool // Hack: Enable auto-provisioning of users and access grants, disabled for "ntfy user" commands - Users []*User // Predefined users to create on startup - Access map[string][]*Grant // Predefined access grants to create on startup (username -> []*Grant) - Tokens map[string][]*Token // Predefined users to create on startup (username -> []*Token) - QueueWriterInterval time.Duration // Interval for the async queue writer to flush stats and token updates to the database - BcryptCost int // Cost of generated passwords; lowering makes testing faster - AccessCacheEnabled bool // Enables the in-memory ACL cache (high volume servers only) - AccessCacheReloadInterval time.Duration // Reload interval for access cache, relevant for ACL writes from CLI + Filename string // Database filename, e.g. "/var/lib/ntfy/user.db" (SQLite) + DatabaseURL string // Database connection string (PostgreSQL) + StartupQueries string // Queries to run on startup, e.g. to create initial users or tiers (SQLite only) + DefaultAccess Permission // Default permission if no ACL matches + ProvisionEnabled bool // Hack: Enable auto-provisioning of users and access grants, disabled for "ntfy user" commands + Users []*User // Predefined users to create on startup + Access map[string][]*Grant // Predefined access grants to create on startup (username -> []*Grant) + Tokens map[string][]*Token // Predefined users to create on startup (username -> []*Token) + QueueWriterInterval time.Duration // Interval for the async queue writer to flush stats and token updates to the database + BcryptCost int // Cost of generated passwords; lowering makes testing faster + AccessCacheEnabled bool // Enables the in-memory ACL cache (high volume servers only) + AccessCacheReloadInterval time.Duration // Reload interval for access cache, relevant for ACL writes from CLI + ExpiredMagicLinkReapInterval time.Duration // Interval for sweeping expired email-verify/password-reset links } // Error constants used by the package @@ -383,13 +384,13 @@ type queries struct { updateEmailClearPrimary string // Magic link queries (email verification + password reset) - insertMagicLink string - selectMagicLinkByHash string - deleteMagicLinkByHash string - deleteVerifyScope string // Delete pending email_verify rows for (user_id, email) - deleteResetScope string // Delete the active password_reset row for user_id - selectPendingEmails string // Pending (unverified) email addresses for a user - deleteExpiredMagicLinks string + insertMagicLink string + selectMagicLinkByHash string + deleteMagicLinkByHash string + deleteMagicLinkEmailVerify string // Delete pending email_verify rows for (user_id, email) + deleteMagicLinkResetPassword string // Delete the active password_reset row for user_id + selectPendingEmails string // Pending (unverified) email addresses for a user + deleteExpiredMagicLinks string // Billing queries updateBilling string diff --git a/user/util.go b/user/util.go index 5157b95f..50d0c012 100644 --- a/user/util.go +++ b/user/util.go @@ -89,12 +89,8 @@ func hashToken(raw string) string { return hex.EncodeToString(sum[:]) } -// HashPassword hashes the given password using bcrypt with the configured cost -func HashPassword(password string) (string, error) { - return hashPassword(password, DefaultUserPasswordBcryptCost) -} - -func hashPassword(password string, cost int) (string, error) { +// HashPassword hashes the given password using bcrypt with the given cost +func HashPassword(password string, cost int) (string, error) { hash, err := bcrypt.GenerateFromPassword([]byte(password), cost) if err != nil { return "", err diff --git a/user/util_test.go b/user/util_test.go index 97c4bc4a..2fa542ab 100644 --- a/user/util_test.go +++ b/user/util_test.go @@ -176,7 +176,7 @@ func TestHashPassword(t *testing.T) { password := "test-password-123" // Hash the password - hash, err := HashPassword(password) + hash, err := HashPassword(password, DefaultUserPasswordBcryptCost) require.Nil(t, err) require.NotEmpty(t, hash) @@ -187,12 +187,12 @@ func TestHashPassword(t *testing.T) { require.True(t, strings.HasPrefix(hash, "$2a$")) // Hash the same password again - should produce different hash - hash2, err := HashPassword(password) + hash2, err := HashPassword(password, DefaultUserPasswordBcryptCost) require.Nil(t, err) require.NotEqual(t, hash, hash2, "Same password should produce different hashes (salt)") // Empty password should still work - emptyHash, err := HashPassword("") + emptyHash, err := HashPassword("", DefaultUserPasswordBcryptCost) require.Nil(t, err) require.NotEmpty(t, emptyHash) require.Nil(t, ValidPasswordHash(emptyHash, DefaultUserPasswordBcryptCost)) @@ -202,15 +202,15 @@ func TestHashPassword_WithCost(t *testing.T) { password := "test-password" // Test with different costs - hash4, err := hashPassword(password, 4) + hash4, err := HashPassword(password, 4) require.Nil(t, err) require.True(t, strings.HasPrefix(hash4, "$2a$04$")) - hash10, err := hashPassword(password, 10) + hash10, err := HashPassword(password, 10) require.Nil(t, err) require.True(t, strings.HasPrefix(hash10, "$2a$10$")) - hash12, err := hashPassword(password, 12) + hash12, err := HashPassword(password, 12) require.Nil(t, err) require.True(t, strings.HasPrefix(hash12, "$2a$12$"))