From 2f4afbdae5690b16f4c539be820212dc4d138281 Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Sun, 31 May 2026 15:26:28 -0400 Subject: [PATCH] Manual refinements --- cmd/user.go | 6 +- user/access_cache.go | 213 ++++++++++++++++----------------------- user/manager.go | 57 +++++------ user/manager_postgres.go | 30 ++++-- user/manager_sqlite.go | 23 +++-- user/types.go | 6 +- 6 files changed, 149 insertions(+), 186 deletions(-) diff --git a/cmd/user.go b/cmd/user.go index 8eca5ce5..2e5af5f4 100644 --- a/cmd/user.go +++ b/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) diff --git a/user/access_cache.go b/user/access_cache.go index a1518058..1318b01d 100644 --- a/user/access_cache.go +++ b/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 { diff --git a/user/manager.go b/user/manager.go index 292d59b2..587ba8f2 100644 --- a/user/manager.go +++ b/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 diff --git a/user/manager_postgres.go b/user/manager_postgres.go index 8f0079b6..49d294b3 100644 --- a/user/manager_postgres.go +++ b/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, diff --git a/user/manager_sqlite.go b/user/manager_sqlite.go index a1ca4700..7c6507f2 100644 --- a/user/manager_sqlite.go +++ b/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, diff --git a/user/types.go b/user/types.go index 65032aa3..d2746c77 100644 --- a/user/types.go +++ b/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