diff --git a/user/access_cache.go b/user/access_cache.go index 98942d05..d73f48a1 100644 --- a/user/access_cache.go +++ b/user/access_cache.go @@ -3,105 +3,101 @@ package user import ( "regexp" "strings" - "sync/atomic" + "sync" "heckel.io/ntfy/v2/db" ) -// aclEntry mirrors one user_access row in the in-memory snapshot. +// aclCache is an in-memory index over the entire user_access table. +// +// exact[username][escapedTopic] returns the matching entry in O(1) for the common +// case where the requested topic appears verbatim in some rule. The key is the +// stored form of the topic (i.e. with \_ escapes), so Lookup escapes incoming +// topics through escapeUnderscore before probing. +// +// wildcard[username] is the linear-scan list of %-bearing rules for that user. +// Walked per request; trivially small in practice. Wildcards are NOT u_everyone- +// only -- any user can create them. +type aclCache struct { + exact map[string]map[string]aclEntry + wildcard map[string][]aclEntry + mu sync.RWMutex // Protect exact and wildcard +} + +// aclEntry mirrors one user_access row in the in-memory cache. // // topic is the raw stored value: it may contain \_ escapes (for literal underscores) // and % wildcards (translated from user-supplied *). For exact-match entries (no %) -// matcher is nil and the entry is keyed by topic in aclSnapshot.exact. For wildcard +// matcher is nil and the entry is keyed by topic in aclCache.exact. For wildcard // entries (with %) matcher is the pre-compiled regex equivalent of the LIKE pattern. type aclEntry struct { topic string read bool write bool - matcher *regexp.Regexp -} - -// aclSnapshot is an immutable indexed form of the entire user_access table. -// -// exact[userName][escapedTopic] returns the matching entry in O(1) for the common -// case where the requested topic appears verbatim in some rule. The key is the -// stored form of the topic (i.e. with \_ escapes), so callers must pass topics -// through escapeUnderscore before probing. -// -// wildcards[userName] is the linear scan list of %-bearing rules for that user. -// Walked per request; trivially small in practice. Wildcards are NOT u_everyone- -// only -- any user can create them. -type aclSnapshot struct { - exact map[string]map[string]aclEntry - wildcards map[string][]aclEntry -} - -// aclCache holds the current snapshot behind an atomic pointer so that the hot -// path (Lookup) is lock-free. reload builds a fresh snapshot off the request -// path and atomically swaps the pointer; the old snapshot is GC'd once in-flight -// Lookups release their references. -// -// A nil receiver behaves as if no snapshot were loaded -- Lookup returns -// found=false, which the caller then resolves via DefaultAccess. This keeps -// tests and edge cases (e.g. early-startup) safe. -type aclCache struct { - snap atomic.Pointer[aclSnapshot] + matcher *regexp.Regexp // Nil for exact entries } func newAccessCache() *aclCache { - return &aclCache{} + return &aclCache{ + exact: make(map[string]map[string]aclEntry), + wildcard: make(map[string][]aclEntry), + } } -// reload runs the bulk-load query against the primary and atomically swaps in -// a fresh snapshot. The primary is used (not ReadOnly) so a reload immediately -// after an ACL mutation sees the freshly-written rows without replica lag. +// reload runs the bulk-load query against the primary and swaps in freshly-built +// exact and wildcard 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 *aclCache) reload(d *db.DB, query string) error { rows, err := d.Query(query) if err != nil { return err } defer rows.Close() - snap := &aclSnapshot{ - exact: make(map[string]map[string]aclEntry), - wildcards: make(map[string][]aclEntry), - } + exact := make(map[string]map[string]aclEntry) + wildcards := make(map[string][]aclEntry) for rows.Next() { - var userName string + var username string var entry aclEntry - if err := rows.Scan(&userName, &entry.topic, &entry.read, &entry.write); err != nil { + if err := rows.Scan(&username, &entry.topic, &entry.read, &entry.write); err != nil { return err } if strings.Contains(entry.topic, "%") { - entry.matcher = compileLikeToRegex(entry.topic) - snap.wildcards[userName] = append(snap.wildcards[userName], entry) - } else { - if snap.exact[userName] == nil { - snap.exact[userName] = make(map[string]aclEntry) + re, err := compileLikeToRegex(entry.topic) + if err != nil { + return err } - snap.exact[userName][entry.topic] = entry + entry.matcher = re + wildcards[username] = append(wildcards[username], entry) + } else { + if exact[username] == nil { + exact[username] = make(map[string]aclEntry) + } + exact[username][entry.topic] = entry } } if err := rows.Err(); err != nil { return err } - c.snap.Store(snap) + c.mu.Lock() + c.exact = exact + c.wildcard = wildcards + c.mu.Unlock() return 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 -// 2. longer pattern beats shorter (more specific wins) -// 3. write beats read at equal length (write is "stronger") +// 1. specific user beats Everyone +// 2. longer pattern beats shorter (more specific wins) +// 3. write beats read at equal length (write is "stronger") func (c *aclCache) Lookup(usernameOrEveryone, topic string) (read, write, found bool) { if c == nil { return false, false, false } - snap := c.snap.Load() - if snap == nil { - return false, false, false - } - // Pre-compute the escaped form once: exact-match keys in the snapshot are + c.mu.RLock() + defer c.mu.RUnlock() + // Pre-compute the escaped form once: exact-match keys in the cache are // stored as toSQLWildcard would emit them (literal _ -> \_), so the // incoming topic must be escaped the same way before map lookup. escaped := escapeUnderscore(topic) @@ -109,28 +105,28 @@ func (c *aclCache) Lookup(usernameOrEveryone, topic string) (read, write, found // Specific user takes priority over Everyone. Skip the first lookup when // the request is already anonymous to avoid scanning the same map twice. if usernameOrEveryone != Everyone { - if e, ok := pickBest(snap, usernameOrEveryone, topic, escaped); ok { + if e, ok := c.pickBestLocked(usernameOrEveryone, topic, escaped); ok { return e.read, e.write, true } } - if e, ok := pickBest(snap, Everyone, topic, escaped); ok { + if e, ok := c.pickBestLocked(Everyone, topic, escaped); ok { return e.read, e.write, true } return false, false, false } -// pickBest returns the highest-priority entry for a single user, combining the -// exact-match O(1) probe with a linear scan over the (usually empty or tiny) -// wildcard list. Priority within a user: longer pattern wins; write wins ties. -func pickBest(snap *aclSnapshot, userName, topic, escaped string) (aclEntry, bool) { +// pickBestLocked returns the highest-priority entry for a single user, combining +// the exact-match O(1) probe with a linear scan over the (usually empty or tiny) +// wildcard list. Caller must hold c.mu (RLock is sufficient). +func (c *aclCache) pickBestLocked(username, topic, escaped string) (aclEntry, bool) { var best aclEntry var found bool - if m, ok := snap.exact[userName]; ok { + if m, ok := c.exact[username]; ok { if e, ok := m[escaped]; ok { best, found = e, true } } - for _, w := range snap.wildcards[userName] { + for _, w := range c.wildcard[username] { if !w.matcher.MatchString(topic) { continue } @@ -158,7 +154,7 @@ func better(a, b aclEntry) bool { // \_ is a literal underscore; no other backslashes occur. Topics themselves are // restricted to [A-Za-z0-9_-] (see AllowedTopic), so neither % nor stray // backslashes appear in user-supplied input. -func compileLikeToRegex(pattern string) *regexp.Regexp { +func compileLikeToRegex(pattern string) (*regexp.Regexp, error) { var sb strings.Builder sb.WriteString("^") i := 0 @@ -176,5 +172,5 @@ func compileLikeToRegex(pattern string) *regexp.Regexp { } } sb.WriteString("$") - return regexp.MustCompile(sb.String()) + return regexp.Compile(sb.String()) } diff --git a/user/access_cache_test.go b/user/access_cache_test.go index d8f11d71..6c617a0c 100644 --- a/user/access_cache_test.go +++ b/user/access_cache_test.go @@ -1,6 +1,7 @@ package user import ( + "regexp" "sync" "sync/atomic" "testing" @@ -14,21 +15,21 @@ import ( // forEachStoreBackend. func TestCompileLikeToRegex_Exact(t *testing.T) { - r := compileLikeToRegex("foo") + r := mustCompileLikeToRegex(t, "foo") require.True(t, r.MatchString("foo")) require.False(t, r.MatchString("foox")) require.False(t, r.MatchString("xfoo")) } func TestCompileLikeToRegex_TrailingPercent(t *testing.T) { - r := compileLikeToRegex("up%") + r := mustCompileLikeToRegex(t, "up%") require.True(t, r.MatchString("up")) require.True(t, r.MatchString("up123")) require.False(t, r.MatchString("xup")) } func TestCompileLikeToRegex_LeadingAndEmbeddedPercent(t *testing.T) { - r := compileLikeToRegex("%test%") + r := mustCompileLikeToRegex(t, "%test%") require.True(t, r.MatchString("test")) require.True(t, r.MatchString("mytest")) require.True(t, r.MatchString("testxxx")) @@ -39,7 +40,7 @@ func TestCompileLikeToRegex_LeadingAndEmbeddedPercent(t *testing.T) { func TestCompileLikeToRegex_EscapedUnderscore(t *testing.T) { // "my\_topic" is the stored form of a literal "my_topic" -- the underscore // must match itself, NOT act as a SQL one-character wildcard. - r := compileLikeToRegex(`my\_topic`) + r := mustCompileLikeToRegex(t, `my\_topic`) require.True(t, r.MatchString("my_topic")) require.False(t, r.MatchString("myXtopic")) require.False(t, r.MatchString("mytopic")) @@ -48,7 +49,7 @@ func TestCompileLikeToRegex_EscapedUnderscore(t *testing.T) { func TestCompileLikeToRegex_EscapedUnderscoreAdjacentToPercent(t *testing.T) { // "nz\_vip\_%" is the stored form of "nz_vip_*" -- literal "nz_vip_" prefix // followed by any suffix. - r := compileLikeToRegex(`nz\_vip\_%`) + r := mustCompileLikeToRegex(t, `nz\_vip\_%`) require.True(t, r.MatchString("nz_vip_")) require.True(t, r.MatchString("nz_vip_alpha")) require.False(t, r.MatchString("nz_vipX")) @@ -58,7 +59,7 @@ func TestCompileLikeToRegex_EscapedUnderscoreAdjacentToPercent(t *testing.T) { func TestCompileLikeToRegex_RegexMetaCharsInTopic(t *testing.T) { // Topics in ntfy can include '-', which is benign, but make sure // regex metacharacters in the pattern are escaped properly anyway. - r := compileLikeToRegex("foo-bar") + r := mustCompileLikeToRegex(t, "foo-bar") require.True(t, r.MatchString("foo-bar")) require.False(t, r.MatchString("foo.bar")) // would match if '-' leaked into a character class } @@ -72,8 +73,9 @@ func TestACLCache_LookupOnNilReceiverSafe(t *testing.T) { } func TestACLCache_LookupBeforeReload(t *testing.T) { - // Before reload the snapshot pointer is nil. The cache treats this as - // "no rule found", which the caller resolves via DefaultAccess. + // A freshly-constructed cache has empty exact and wildcards maps. The + // cache treats this as "no rule found", which the caller resolves via + // DefaultAccess. c := newAccessCache() read, write, found := c.Lookup("phil", "mytopic") require.False(t, found) @@ -83,9 +85,9 @@ func TestACLCache_LookupBeforeReload(t *testing.T) { func TestACLCache_ExactMatchHit(t *testing.T) { c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: "phil", topic: "mytopic", read: true, write: true}, - })) + }) read, write, found := c.Lookup("phil", "mytopic") require.True(t, found) require.True(t, read) @@ -94,9 +96,9 @@ func TestACLCache_ExactMatchHit(t *testing.T) { func TestACLCache_ExactMatchMiss(t *testing.T) { c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: "phil", topic: "mytopic", read: true, write: true}, - })) + }) _, _, found := c.Lookup("phil", "othertopic") require.False(t, found) } @@ -105,9 +107,9 @@ func TestACLCache_LiteralUnderscoreExactMatch(t *testing.T) { // Stored as "my\_topic" (toSQLWildcard of "my_topic"). A literal underscore // in the requested topic must match, while any other single char must not. c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: "phil", topic: `my\_topic`, read: true, write: false}, - })) + }) read, write, found := c.Lookup("phil", "my_topic") require.True(t, found) require.True(t, read) @@ -119,9 +121,9 @@ func TestACLCache_LiteralUnderscoreExactMatch(t *testing.T) { func TestACLCache_WildcardMatch(t *testing.T) { c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: Everyone, topic: "up%", read: false, write: true}, - })) + }) read, write, found := c.Lookup("phil", "up42") require.True(t, found) require.False(t, read) @@ -130,10 +132,10 @@ func TestACLCache_WildcardMatch(t *testing.T) { func TestACLCache_SpecificUserBeatsEveryone(t *testing.T) { c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: Everyone, topic: "mytopic", read: true, write: false}, {user: "phil", topic: "mytopic", read: false, write: false}, // deny-all for phil - })) + }) read, write, found := c.Lookup("phil", "mytopic") require.True(t, found) require.False(t, read) @@ -142,9 +144,9 @@ func TestACLCache_SpecificUserBeatsEveryone(t *testing.T) { func TestACLCache_AnonymousReadsEveryone(t *testing.T) { c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: Everyone, topic: "announcements", read: true, write: false}, - })) + }) read, write, found := c.Lookup(Everyone, "announcements") require.True(t, found) require.True(t, read) @@ -155,10 +157,10 @@ func TestACLCache_LongerPatternWinsForSameUser(t *testing.T) { // Both rules belong to the same user (Everyone). The more specific (longer) // "mytopic%" should beat the catch-all "%". c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: Everyone, topic: "%", read: true, write: false}, {user: Everyone, topic: "mytopic%", read: true, write: true}, - })) + }) read, write, found := c.Lookup(Everyone, "mytopicX") require.True(t, found) require.True(t, read) @@ -167,36 +169,31 @@ func TestACLCache_LongerPatternWinsForSameUser(t *testing.T) { func TestACLCache_WriteBeatsReadAtEqualLength(t *testing.T) { // Two wildcard rules of identical length for the same user. The write rule - // should win the tie-break. + // should win the tie-break. The two-rows-with-same-topic shape is + // impossible via real upsert (pkey would conflict), so we inject the entries + // directly into the cache's wildcard slice. c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ - {user: Everyone, topic: "ab%", read: true, write: false}, - {user: Everyone, topic: "ab%", read: false, write: true}, // synthesized; impossible via real upsert but exercises the tie-break - })) - // One of the two will be the surviving exact-key entry (map collision keeps last); - // but the wildcard slice is what we want to exercise. Inject two wildcard entries - // directly to force the tie-break path. - c.snap.Store(&aclSnapshot{ - exact: map[string]map[string]aclEntry{}, - wildcards: map[string][]aclEntry{ - Everyone: { - {topic: "ab%", read: true, write: false, matcher: compileLikeToRegex("ab%")}, - {topic: "ab%", read: false, write: true, matcher: compileLikeToRegex("ab%")}, - }, + c.mu.Lock() + c.exact = map[string]map[string]aclEntry{} + c.wildcard = map[string][]aclEntry{ + Everyone: { + {topic: "ab%", read: true, write: false, matcher: mustCompileLikeToRegex(t, "ab%")}, + {topic: "ab%", read: false, write: true, matcher: mustCompileLikeToRegex(t, "ab%")}, }, - }) + } + c.mu.Unlock() _, write, found := c.Lookup(Everyone, "abc") require.True(t, found) require.True(t, write) } func TestACLCache_ConcurrentLookupAndReload(t *testing.T) { - // Atomic-pointer swap must be safe under concurrent reads. The race detector + // Lock-based swap must be safe under concurrent reads. The race detector // catches any unsafe shared mutation. c := newAccessCache() - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: Everyone, topic: "mytopic", read: true, write: true}, - })) + }) var stop atomic.Bool var wg sync.WaitGroup @@ -210,17 +207,17 @@ func TestACLCache_ConcurrentLookupAndReload(t *testing.T) { go func() { defer wg.Done() for i := 0; i < 100; i++ { - c.snap.Store(buildSnapshot(t, []rawACLRow{ + loadCache(t, c, []rawACLRow{ {user: Everyone, topic: "mytopic", read: i%2 == 0, write: i%2 == 1}, - })) + }) } stop.Store(true) }() wg.Wait() } -// rawACLRow + buildSnapshot mirror the rows that reload would Scan from the DB -// but avoid actually opening a DB for these unit tests. +// rawACLRow models the rows that reload would Scan from the DB but avoids +// actually opening a DB for these unit tests. type rawACLRow struct { user string topic string @@ -228,25 +225,35 @@ type rawACLRow struct { write bool } -func buildSnapshot(t *testing.T, rows []rawACLRow) *aclSnapshot { +// loadCache writes the given rows into the cache under its write lock, +// preserving the same exact/wildcard partitioning that reload would produce. +func loadCache(t *testing.T, c *aclCache, rows []rawACLRow) { t.Helper() - snap := &aclSnapshot{ - exact: make(map[string]map[string]aclEntry), - wildcards: make(map[string][]aclEntry), - } + exact := make(map[string]map[string]aclEntry) + wildcards := make(map[string][]aclEntry) for _, r := range rows { e := aclEntry{topic: r.topic, read: r.read, write: r.write} if containsPercent(r.topic) { - e.matcher = compileLikeToRegex(r.topic) - snap.wildcards[r.user] = append(snap.wildcards[r.user], e) + e.matcher = mustCompileLikeToRegex(t, r.topic) + wildcards[r.user] = append(wildcards[r.user], e) } else { - if snap.exact[r.user] == nil { - snap.exact[r.user] = make(map[string]aclEntry) + if exact[r.user] == nil { + exact[r.user] = make(map[string]aclEntry) } - snap.exact[r.user][r.topic] = e + exact[r.user][r.topic] = e } } - return snap + c.mu.Lock() + c.exact = exact + c.wildcard = wildcards + c.mu.Unlock() +} + +func mustCompileLikeToRegex(t *testing.T, pattern string) *regexp.Regexp { + t.Helper() + r, err := compileLikeToRegex(pattern) + require.NoError(t, err) + return r } func containsPercent(s string) bool {