From 4b87a2732633e3a6e7285e0378b4bc1719eb2c06 Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Sun, 31 May 2026 11:22:30 -0400 Subject: [PATCH] Docblock, more tests --- user/access_cache.go | 17 +++++++--- user/access_cache_test.go | 70 ++++++++++++++++++++++++++++++++++----- 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/user/access_cache.go b/user/access_cache.go index 5c5359fd..a49327d2 100644 --- a/user/access_cache.go +++ b/user/access_cache.go @@ -113,9 +113,17 @@ func (c *aclCache) Lookup(usernameOrEveryone, topic string) (read, write, found return false, false, false } -// pickBestNoLock 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) -// pattern list. Caller must hold c.mu (RLock is sufficient). +// 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: +// +// 1. longer stored pattern beats shorter (a more specific rule wins over a +// more general one) +// 2. at equal length, write beats read (a stronger permission wins the tie) +// +// Exact and wildcard rules are ranked together under the same criteria, so +// an exact "foo" (length 3) beats a wildcard "f%" (length 2), but a wildcard +// "foo%" (length 4) beats an exact "foo" (length 3). func (c *aclCache) pickBestNoLock(username, topic, escapedTopic string) (*aclEntry, bool) { var best aclEntry var found bool @@ -139,8 +147,7 @@ func (c *aclCache) pickBestNoLock(username, topic, escapedTopic string) (*aclEnt func better(a, b aclEntry) bool { if a.length != b.length { return a.length > b.length - } - if a.write != b.write { + } else if a.write != b.write { return a.write } return false diff --git a/user/access_cache_test.go b/user/access_cache_test.go index 54d8d071..90839872 100644 --- a/user/access_cache_test.go +++ b/user/access_cache_test.go @@ -64,14 +64,6 @@ func TestCompileLikeToRegex_RegexMetaCharsInTopic(t *testing.T) { require.False(t, r.MatchString("foo.bar")) // would match if '-' leaked into a character class } -func TestACLCache_LookupOnNilReceiverSafe(t *testing.T) { - var c *aclCache - read, write, found := c.Lookup("phil", "mytopic") - require.False(t, found) - require.False(t, read) - require.False(t, write) -} - func TestACLCache_LookupBeforeReload(t *testing.T) { // A freshly-constructed cache has empty exact and wildcards maps. The // cache treats this as "no rule found", which the caller resolves via @@ -142,6 +134,36 @@ func TestACLCache_SpecificUserBeatsEveryone(t *testing.T) { require.False(t, write) } +func TestACLCache_SpecificUserBeatsEveryoneEvenWhenShorter(t *testing.T) { + // The SQL's "user_name DESC" sort key takes precedence over LENGTH(topic). + // Concretely: a specific user with a shorter matching rule still wins over + // Everyone with a longer matching rule. + c := newAccessCache() + loadCache(t, c, []rawACLRow{ + {user: Everyone, topic: "foo", read: true, write: true}, // exact, length 3 + {user: "phil", topic: "f%", read: false, write: false}, // wildcard, length 2, deny-all + }) + read, write, found := c.Lookup("phil", "foo") + require.True(t, found) + require.False(t, read) + require.False(t, write) +} + +func TestACLCache_SpecificUserBeatsEveryoneRegardlessOfWrite(t *testing.T) { + // Same-length rules but conflicting permissions across user boundary: the + // specific user always wins, even if its permission set is weaker (or + // stronger, in either direction). + c := newAccessCache() + loadCache(t, c, []rawACLRow{ + {user: Everyone, topic: "mytopic", read: true, write: true}, // wide-open + {user: "phil", topic: "mytopic", read: true, write: false}, // read-only for phil + }) + read, write, found := c.Lookup("phil", "mytopic") + require.True(t, found) + require.True(t, read) + require.False(t, write) +} + func TestACLCache_AnonymousReadsEveryone(t *testing.T) { c := newAccessCache() loadCache(t, c, []rawACLRow{ @@ -167,6 +189,38 @@ func TestACLCache_LongerPatternWinsForSameUser(t *testing.T) { require.True(t, write) } +func TestACLCache_ExactBeatsShorterWildcardSameUser(t *testing.T) { + // Same user, two matching rules: exact "foo" (length 3) and wildcard "f%" + // (length 2). The longer one wins, which is the exact rule -- mirroring + // the SQL's "LENGTH(topic) DESC" tie-break. Crucially, the cache must seed + // "best" from the exact map probe before walking wildcards, otherwise a + // shorter wildcard could overwrite a longer exact. + c := newAccessCache() + loadCache(t, c, []rawACLRow{ + {user: "phil", topic: "foo", read: true, write: true}, // exact, length 3 + {user: "phil", topic: "f%", read: false, write: false}, // wildcard, length 2, deny-all + }) + read, write, found := c.Lookup("phil", "foo") + require.True(t, found) + require.True(t, read) + require.True(t, write) +} + +func TestACLCache_LongerWildcardBeatsExactSameUser(t *testing.T) { + // Same user, two matching rules: exact "foo" (length 3) and wildcard "foo%" + // (length 4). The wildcard wins on length DESC. Exercises the "swap best + // to wildcard when better() returns true" path. + c := newAccessCache() + loadCache(t, c, []rawACLRow{ + {user: "phil", topic: "foo", read: false, write: false}, // exact, length 3, deny-all + {user: "phil", topic: "foo%", read: true, write: true}, // wildcard, length 4 + }) + read, write, found := c.Lookup("phil", "foo") + require.True(t, found) + require.True(t, read) + require.True(t, write) +} + func TestACLCache_WriteBeatsReadAtEqualLength(t *testing.T) { // Two wildcard rules of identical length for the same user. The write rule // should win the tie-break. The two-rows-with-same-topic shape is