Manual refinements

cmd/user.go

@@ -378,11 +378,7 @@ func createUserManager(c *cli.Context) (*user.Manager, error) {
 		ProvisionEnabled:    false, // Hack: Do not re-provision users on manager initialization
 		BcryptCost:          user.DefaultUserPasswordBcryptCost,
 		QueueWriterInterval: user.DefaultUserStatsQueueWriterInterval,
-		// CLI subcommands never serve authorizeTopicAccess and are short-lived,
-		// so the cache (and its background poller) would be wasted work. Mutations
-		// hit the DB directly; the running server, if any, picks them up via its
-		// own poller when the cache is enabled there.
-		AccessCacheEnabled: false,
+		AccessCacheEnabled:  false, // Do not cache for CLI commands
 	}
 	if databaseURL != "" {
 		host, dbErr := pg.Open(databaseURL)

user/access_cache.go

@@ -24,21 +24,14 @@ type accessCache struct {
 	mu      sync.RWMutex // Protect exact and pattern
 }
 
-// aclEntry mirrors one user_access row in the in-memory cache.
-//
-// length is the length of the original stored value (topic for exact rows,
-// SQL LIKE pattern for wildcard rows). It is only used by better() to
-// implement the "longer pattern beats shorter" tie-break from the original
-// SQL ORDER BY. The string itself is intentionally not stored on the entry:
-// the exact map already keys on it, and surfacing it would invite misuse
-// (wildcard "topics" are actually SQL patterns like "up%").
-//
-// pattern is the pre-compiled regex equivalent of the stored LIKE pattern.
-// For exact-match entries (no % in the stored value) pattern is nil and the
-// entry is reachable only through accessCache.exact[username][topic].
+// aclEntry mirrors one user_access row. length feeds better()'s "longer
+// pattern wins" tie-break; the stored topic/pattern string itself is not kept
+// on the entry (the exact map already keys on it; surfacing wildcard "topics"
+// like "up%" alongside real ones would invite misuse). pattern is the
+// compiled regex form of the LIKE pattern; nil for exact entries.
 type aclEntry struct {
-	length  int            // len() of the original stored topic/pattern
-	pattern *regexp.Regexp // nil for exact entries
+	length  int
+	pattern *regexp.Regexp
 	read    bool
 	write   bool
 }
@@ -50,118 +43,6 @@ func newAccessCache() *accessCache {
 	}
 }
 
-// reload runs the bulk-load query against the primary and swaps in freshly-built
-// exact and pattern maps under the write lock. The primary is used (not
-// ReadOnly) so a reload immediately after an ACL mutation sees the freshly-
-// written rows without replica lag.
-func (c *accessCache) reload(d *db.DB, query string) error {
-	rows, err := d.Query(query)
-	if err != nil {
-		return err
-	}
-	defer rows.Close()
-	exacts := make(map[string]map[string]aclEntry)
-	patterns := make(map[string][]aclEntry)
-	for rows.Next() {
-		var username, topic string
-		var read, write bool
-		if err := rows.Scan(&username, &topic, &read, &write); err != nil {
-			return err
-		}
-		entry, isWildcard, err := newACLEntry(topic, read, write)
-		if err != nil {
-			return err
-		}
-		if isWildcard {
-			patterns[username] = append(patterns[username], entry)
-		} else {
-			if exacts[username] == nil {
-				exacts[username] = make(map[string]aclEntry)
-			}
-			exacts[username][topic] = entry
-		}
-	}
-	if err := rows.Err(); err != nil {
-		return err
-	}
-	c.mu.Lock()
-	c.exact = exacts
-	c.pattern = patterns
-	c.mu.Unlock()
-	return nil
-}
-
-// reloadUser refreshes just one user's rules from the database and swaps them
-// into the cache. Used as a cheaper, owner-cascade-safe alternative to the
-// full bulk reload after a mutation that affects a known set of users. The
-// caller is expected to invoke reloadUser for every user whose row set may
-// have changed -- typically the targeted user plus Everyone, since most
-// reservation flows touch both.
-//
-// The query must return (topic, read, write) for every row whose user_id
-// matches the given username. An empty result set is treated as "this user
-// has no rules": the user's entries are removed from both maps so the inner
-// maps don't grow unbounded under churn.
-func (c *accessCache) reloadUser(d *db.DB, query, username string) error {
-	rows, err := d.Query(query, username)
-	if err != nil {
-		return err
-	}
-	defer rows.Close()
-	exact := make(map[string]aclEntry)
-	var pattern []aclEntry
-	for rows.Next() {
-		var topic string
-		var read, write bool
-		if err := rows.Scan(&topic, &read, &write); err != nil {
-			return err
-		}
-		entry, isWildcard, err := newACLEntry(topic, read, write)
-		if err != nil {
-			return err
-		}
-		if isWildcard {
-			pattern = append(pattern, entry)
-		} else {
-			exact[topic] = entry
-		}
-	}
-	if err := rows.Err(); err != nil {
-		return err
-	}
-	c.mu.Lock()
-	if len(exact) == 0 {
-		delete(c.exact, username)
-	} else {
-		c.exact[username] = exact
-	}
-	if len(pattern) == 0 {
-		delete(c.pattern, username)
-	} else {
-		c.pattern[username] = pattern
-	}
-	c.mu.Unlock()
-	return nil
-}
-
-// newACLEntry builds an aclEntry from one user_access row's values. The
-// isWildcard return tells the caller which storage slot the entry belongs in:
-// the per-user wildcard slice if true, the per-user exact map if false.
-// Wildcards have their LIKE pattern pre-compiled into entry.pattern; exact
-// entries leave entry.pattern nil.
-func newACLEntry(topic string, read, write bool) (entry aclEntry, isWildcard bool, err error) {
-	entry = aclEntry{length: len(topic), read: read, write: write}
-	if !strings.Contains(topic, "%") {
-		return entry, false, nil
-	}
-	re, err := compileLikeToRegex(topic)
-	if err != nil {
-		return entry, true, err
-	}
-	entry.pattern = re
-	return entry, true, nil
-}
-
 // Lookup returns the effective (read, write, found) permission for the given
 // (username, topic), preserving the priority ordering of the original SQL query:
 //  1. specific user beats Everyone
@@ -182,6 +63,68 @@ func (c *accessCache) Lookup(usernameOrEveryone, topic string) (read, write, fou
 	return false, false, false
 }
 
+// reload scans (user_name, topic, read, write) rows and merges them into the
+// cache. With no usernames the cache is replaced wholesale; otherwise the
+// query is invoked with those usernames as positional args and only the
+// listed users' slices are touched (a username absent from the result drops
+// them from both maps). Runs against the primary so a reload after a
+// mutation sees the just-written rows.
+func (c *accessCache) reload(d *db.DB, query string, usernames ...string) error {
+	args := make([]any, len(usernames))
+	for i, u := range usernames {
+		args[i] = u
+	}
+	rows, err := d.Query(query, args...)
+	if err != nil {
+		return err
+	}
+	defer rows.Close()
+	exacts := make(map[string]map[string]aclEntry)
+	patterns := make(map[string][]aclEntry)
+	for rows.Next() {
+		var u, topic string
+		var read, write bool
+		if err := rows.Scan(&u, &topic, &read, &write); err != nil {
+			return err
+		}
+		entry, isPattern, err := toACLEntry(topic, read, write)
+		if err != nil {
+			return err
+		}
+		if isPattern {
+			patterns[u] = append(patterns[u], entry)
+		} else {
+			if exacts[u] == nil {
+				exacts[u] = make(map[string]aclEntry)
+			}
+			exacts[u][topic] = entry
+		}
+	}
+	if err := rows.Err(); err != nil {
+		return err
+	}
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	if len(usernames) == 0 {
+		c.exact = exacts
+		c.pattern = patterns
+		return nil
+	}
+	for _, u := range usernames {
+		if e, ok := exacts[u]; ok {
+			c.exact[u] = e
+		} else {
+			delete(c.exact, u)
+		}
+		if p, ok := patterns[u]; ok {
+			c.pattern[u] = p
+		} else {
+			delete(c.pattern, u)
+		}
+	}
+	return nil
+}
+
 // pickBestNoLock returns the highest-priority entry for a single user. When
 // more than one of that user's rules matches the requested topic, the winner
 // is chosen by:
@@ -211,6 +154,24 @@ func (c *accessCache) pickBestNoLock(username, topic, escapedTopic string) (*acl
 	return &best, found
 }
 
+// toACLEntry builds an aclEntry from one user_access row's values. The
+// isWildcard return tells the caller which storage slot the entry belongs in:
+// the per-user wildcard slice if true, the per-user exact map if false.
+// Wildcards have their LIKE pattern pre-compiled into entry.pattern; exact
+// entries leave entry.pattern nil.
+func toACLEntry(topic string, read, write bool) (entry aclEntry, isWildcard bool, err error) {
+	entry = aclEntry{length: len(topic), read: read, write: write}
+	if !strings.Contains(topic, "%") {
+		return entry, false, nil
+	}
+	pattern, err := compileLikeToRegex(topic)
+	if err != nil {
+		return entry, true, err
+	}
+	entry.pattern = pattern
+	return entry, true, nil
+}
+
 // better implements the (length DESC, write DESC) tie-break used by the original
 // query's ORDER BY for entries owned by the same user.
 func better(a, b aclEntry) bool {

user/manager.go

@@ -93,39 +93,28 @@ func newManager(d *db.DB, queries queries, config *Config) (*Manager, error) {
 	if err := manager.maybeProvisionUsersAccessAndTokens(); err != nil {
 		return nil, err
 	}
-	// Populate the cache after provisioning so the initial snapshot includes
-	// any provisioned access rules. No-op when the cache is disabled.
-	if err := manager.reloadAccessCache(); err != nil {
-		return nil, err
-	}
-	go manager.asyncQueueWriter(manager.config.QueueWriterInterval)
-	if manager.accessCache != nil && manager.config.AccessCacheReloadInterval > 0 {
+	if manager.accessCache != nil {
+		if err := manager.maybeReloadAccessCache(); err != nil {
+			return nil, err
+		}
 		go manager.asyncAccessCacheReloader(manager.config.AccessCacheReloadInterval)
 	}
+	go manager.asyncQueueWriter(manager.config.QueueWriterInterval)
 	return manager, nil
 }
 
-// reloadAccessCache rebuilds the in-memory access cache from the primary
-// database. No-op when the cache is disabled.
-func (a *Manager) reloadAccessCache() error {
+// maybeReloadAccessCache refreshes the in-memory access cache from the
+// primary database. No-op when the cache is disabled. With no usernames it
+// does a full bulk reload; with one or more it refreshes only those users'
+// slices in a single DB round-trip via an IN clause.
+func (a *Manager) maybeReloadAccessCache(usernames ...string) error {
 	if a.accessCache == nil {
 		return nil
 	}
-	return a.accessCache.reload(a.db, a.queries.selectAllAccessForCache)
-}
-
-// reloadAccessCacheUsers refreshes the cache slices for the given usernames.
-// No-op when the cache is disabled.
-func (a *Manager) reloadAccessCacheUsers(usernames ...string) error {
-	if a.accessCache == nil {
-		return nil
+	if len(usernames) == 0 {
+		return a.accessCache.reload(a.db, a.queries.selectAccessCacheAll)
 	}
-	for _, username := range usernames {
-		if err := a.accessCache.reloadUser(a.db, a.queries.selectAccessForCacheByUser, username); err != nil {
-			return err
-		}
-	}
-	return nil
+	return a.accessCache.reload(a.db, a.queries.selectAccessCacheUsersFn(len(usernames)), usernames...)
 }
 
 // asyncAccessCacheReloader periodically refreshes the access cache
@@ -137,7 +126,7 @@ func (a *Manager) asyncAccessCacheReloader(interval time.Duration) {
 		case <-a.quit:
 			return
 		case <-ticker.C:
-			if err := a.reloadAccessCache(); err != nil {
+			if err := a.maybeReloadAccessCache(); err != nil {
 				log.Tag(tag).Err(err).Warn("Reloading ACL cache failed")
 			}
 		}
@@ -225,7 +214,7 @@ func (a *Manager) RemoveUser(username string) error {
 	// user_access rows are cascade-deleted along with the user (both by user_id
 	// and by owner_user_id). Refresh this user's own slice (now empty) and
 	// Everyone's slice, since reservations owned by this user landed there too.
-	return a.reloadAccessCacheUsers(username, Everyone)
+	return a.maybeReloadAccessCache(username, Everyone)
 }
 
 // removeUserTx deletes the user with the given username
@@ -265,7 +254,7 @@ func (a *Manager) MarkUserRemoved(user *User) error {
 	// resetUserAccessTx deleted this user's rows AND any row owned by this user
 	// (typically the matching Everyone rows from their reservations). Refresh
 	// both slices to mirror the DB exactly.
-	return a.reloadAccessCacheUsers(user.Name, Everyone)
+	return a.maybeReloadAccessCache(user.Name, Everyone)
 }
 
 // RemoveDeletedUsers deletes all users that have been marked deleted
@@ -274,7 +263,7 @@ func (a *Manager) RemoveDeletedUsers() error {
 		return err
 	}
 	// user_access rows are cascade-deleted with the users; refresh the snapshot.
-	return a.reloadAccessCache()
+	return a.maybeReloadAccessCache()
 }
 
 // ChangePassword changes a user's password
@@ -315,7 +304,7 @@ func (a *Manager) ChangeRole(username string, role Role) error {
 	// by this user (Everyone rows from their reservations). Other role changes
 	// are no-ops for the cache but reloading the two affected slices is cheap
 	// and keeps the code path uniform.
-	return a.reloadAccessCacheUsers(username, Everyone)
+	return a.maybeReloadAccessCache(username, Everyone)
 }
 
 // changeRoleTx changes a user's role
@@ -683,7 +672,7 @@ func (a *Manager) AllowAccess(username string, topicPattern string, permission P
 		return err
 	}
 	// Only this user's row set changed; refresh their slice only.
-	return a.reloadAccessCacheUsers(username)
+	return a.maybeReloadAccessCache(username)
 }
 
 func (a *Manager) allowAccessTx(tx *sql.Tx, username string, topicPattern string, permission Permission, provisioned bool) error {
@@ -710,9 +699,9 @@ func (a *Manager) ResetAccess(username string, topicPattern string) error {
 	// and deleteTopicAccess both touch rows owned by the user (typically
 	// Everyone rows from their reservations).
 	if username == "" {
-		return a.reloadAccessCache()
+		return a.maybeReloadAccessCache()
 	}
-	return a.reloadAccessCacheUsers(username, Everyone)
+	return a.maybeReloadAccessCache(username, Everyone)
 }
 
 func (a *Manager) resetAccessTx(tx *sql.Tx, username string, topicPattern string) error {
@@ -873,7 +862,7 @@ func (a *Manager) AddReservation(username string, topic string, everyone Permiss
 		return err
 	}
 	// Both user's and Everyone's rows changed.
-	return a.reloadAccessCacheUsers(username, Everyone)
+	return a.maybeReloadAccessCache(username, Everyone)
 }
 
 // RemoveReservations deletes the access control entries associated with the given username/topic,
@@ -901,7 +890,7 @@ func (a *Manager) RemoveReservations(username string, topics ...string) error {
 	}
 	// Mirror the DB: rows for this user and any Everyone rows owned by this
 	// user are gone. Refresh both slices.
-	return a.reloadAccessCacheUsers(username, Everyone)
+	return a.maybeReloadAccessCache(username, Everyone)
 }
 
 // Reservations returns all user-owned topics, and the associated everyone-access

user/manager_postgres.go

@@ -1,6 +1,9 @@
 package user
 
 import (
+	"fmt"
+	"strings"
+
 	"heckel.io/ntfy/v2/db"
 )
 
@@ -77,17 +80,11 @@ const (
 		WHERE (u.user_name = $1 OR u.user_name = $2) AND $3 LIKE a.topic ESCAPE '\'
 		ORDER BY u.user_name DESC, LENGTH(a.topic) DESC, CASE WHEN a.write THEN 1 ELSE 0 END DESC
 	`
-	postgresSelectAllAccessForCacheQuery = `
+	postgresSelectAccessCacheAllQuery = `
 		SELECT u.user_name, a.topic, a.read, a.write
 		FROM user_access a
 		JOIN "user" u ON u.id = a.user_id
 	`
-	postgresSelectAccessForCacheByUserQuery = `
-		SELECT a.topic, a.read, a.write
-		FROM user_access a
-		JOIN "user" u ON u.id = a.user_id
-		WHERE u.user_name = $1
-	`
 	postgresSelectUserAllAccessQuery = `
 		SELECT user_id, topic, read, write, provisioned
 		FROM user_access
@@ -232,6 +229,21 @@ const (
 	`
 )
 
+// postgresSelectAccessCacheUsersQuery builds the per-users cache-load query
+// with a "$1, $2, ..." IN clause sized for n usernames.
+func postgresSelectAccessCacheUsersQuery(n int) string {
+	var sb strings.Builder
+	sb.WriteString(`SELECT u.user_name, a.topic, a.read, a.write FROM user_access a JOIN "user" u ON u.id = a.user_id WHERE u.user_name IN (`)
+	for i := 0; i < n; i++ {
+		if i > 0 {
+			sb.WriteString(",")
+		}
+		fmt.Fprintf(&sb, "$%d", i+1)
+	}
+	sb.WriteString(")")
+	return sb.String()
+}
+
 // NewPostgresManager creates a new Manager backed by a PostgreSQL database using an existing connection pool.
 var postgresQueries = queries{
 	selectUserByID:               postgresSelectUserByIDQuery,
@@ -256,8 +268,8 @@ var postgresQueries = queries{
 	deleteUsersMarked:            postgresDeleteUsersMarkedQuery,
 	deleteUsersProvisioned:       postgresDeleteUsersProvisionedQuery,
 	selectTopicPerms:             postgresSelectTopicPermsQuery,
-	selectAllAccessForCache:      postgresSelectAllAccessForCacheQuery,
-	selectAccessForCacheByUser:   postgresSelectAccessForCacheByUserQuery,
+	selectAccessCacheAll:         postgresSelectAccessCacheAllQuery,
+	selectAccessCacheUsersFn:     postgresSelectAccessCacheUsersQuery,
 	selectUserAllAccess:          postgresSelectUserAllAccessQuery,
 	selectUserAccess:             postgresSelectUserAccessQuery,
 	selectUserReservations:       postgresSelectUserReservationsQuery,

user/manager_sqlite.go

@@ -4,6 +4,7 @@ import (
 	"database/sql"
 	"fmt"
 	"path/filepath"
+	"strings"
 
 	_ "github.com/mattn/go-sqlite3" // SQLite driver
 
@@ -83,17 +84,11 @@ const (
 		WHERE (u.user = ? OR u.user = ?) AND ? LIKE a.topic ESCAPE '\'
 		ORDER BY u.user DESC, LENGTH(a.topic) DESC, a.write DESC
 	`
-	sqliteSelectAllAccessForCacheQuery = `
+	sqliteSelectAccessCacheAllQuery = `
 		SELECT u.user, a.topic, a.read, a.write
 		FROM user_access a
 		JOIN user u ON u.id = a.user_id
 	`
-	sqliteSelectAccessForCacheByUserQuery = `
-		SELECT a.topic, a.read, a.write
-		FROM user_access a
-		JOIN user u ON u.id = a.user_id
-		WHERE u.user = ?
-	`
 	sqliteSelectUserAllAccessQuery = `
 		SELECT user_id, topic, read, write, provisioned
 		FROM user_access
@@ -231,6 +226,16 @@ const (
 	`
 )
 
+// sqliteSelectAccessCacheUsersQuery builds the per-users cache-load query
+// with a "?, ?, ..." IN clause sized for n usernames.
+func sqliteSelectAccessCacheUsersQuery(n int) string {
+	placeholders := strings.Repeat(",?", n)
+	if n > 0 {
+		placeholders = placeholders[1:] // drop the leading comma
+	}
+	return `SELECT u.user, a.topic, a.read, a.write FROM user_access a JOIN user u ON u.id = a.user_id WHERE u.user IN (` + placeholders + `)`
+}
+
 var sqliteQueries = queries{
 	selectUserByID:               sqliteSelectUserByIDQuery,
 	selectUserByName:             sqliteSelectUserByNameQuery,
@@ -254,8 +259,8 @@ var sqliteQueries = queries{
 	deleteUsersMarked:            sqliteDeleteUsersMarkedQuery,
 	deleteUsersProvisioned:       sqliteDeleteUsersProvisionedQuery,
 	selectTopicPerms:             sqliteSelectTopicPermsQuery,
-	selectAllAccessForCache:      sqliteSelectAllAccessForCacheQuery,
-	selectAccessForCacheByUser:   sqliteSelectAccessForCacheByUserQuery,
+	selectAccessCacheAll:         sqliteSelectAccessCacheAllQuery,
+	selectAccessCacheUsersFn:     sqliteSelectAccessCacheUsersQuery,
 	selectUserAllAccess:          sqliteSelectUserAllAccessQuery,
 	selectUserAccess:             sqliteSelectUserAccessQuery,
 	selectUserReservations:       sqliteSelectUserReservationsQuery,

user/types.go

@@ -318,9 +318,9 @@ type queries struct {
 	deleteUsersProvisioned       string
 
 	// Access queries
-	selectTopicPerms            string // Direct-DB authorizeTopicAccess query; used when the in-memory cache is disabled
-	selectAllAccessForCache     string // Bulk load: (user_name, topic, read, write) for the in-memory ACL cache
-	selectAccessForCacheByUser  string // Per-user load: (topic, read, write) for one username; used to refresh just one user's slice of the cache after mutation
+	selectTopicPerms            string             // Direct-DB authorizeTopicAccess query; used when the in-memory cache is disabled
+	selectAccessCacheAll        string             // Bulk load: (user_name, topic, read, write) for the in-memory ACL cache
+	selectAccessCacheUsersFn    func(n int) string // Returns a per-users load query whose IN clause is sized for n usernames
 	selectUserAllAccess         string
 	selectUserAccess            string
 	selectUserReservations      string