Review

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
 	}

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,

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,

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")

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

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

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$"))