Grafana PR #2 — Live Benchmark — FriendlyReviewer

Golden Comments (2/2 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	Asymmetric cache trust logic: cached permission grants are trusted and returned immediately, but cached denials from the same cache are ignored → fresh DB lookup. Risk of stale grants for revoked resources.	HIGH	◈ Partial	FriendlyReviewer found a key mismatch on the denial cache (RED service.go:117) that renders it ineffective for numeric user IDs, and a missing invalidation of the denial cache (YELLOW service.go:155). These are real bugs in the same functional area but address a different mechanism than the golden describes.
#2	Test comment says cached permissions “allow access” but the map stores `false` for `dashboards:uid:dash1` → `checkPermission` will still reject.	LOW	✓ Found	Match exact: FriendlyReviewer spotted that the test uses `false` values which never occur in production (`getScopeMap` only stores `true`).

Supplementary Findings (6 findings)

Finding	File	Severity	Legitimate?
Denial cache key mismatch: `checkReq.UserUID` (raw) vs `userIdentifiers.UID` (resolved) → cache broken for users identified by numeric ID	service.go:117	CRITICAL	✓ Legitimate — the denial cache is ineffective for certain users
Duplicate DB call on cache error: if `getCachedIdentityPermissions` fails (`GetUserIdentifiers` DB error), `Check` calls `getIdentityPermissions` → `GetUserIdentifiers` again	service.go:381	CRITICAL	✓ Legitimate — guaranteed redundant DB call on error
Metrics `permissionCacheUsage` label `"false"` conflates “cache miss” with “cache hit without the scope”	service.go:141	MEDIUM	✓ Legitimate — misleading metric accounting
Denial cache never invalidated → entries persist for 30s, false rejections if RBAC roles change	service.go:155	MEDIUM	✓ Legitimate — real freshness problem
List caching tests too limited: only happy path cache-hit — missing cache miss, DB fallback, outdated cache, anonymous permissions	service_test.go:1288	MEDIUM	✓ Legitimate — insufficient coverage
Test “Should deny on explicit cache deny entry” uses `false` values that never occur in production (same as golden #2)	service_test.go:967	LOW	✓ Legitimate — misleading test scenario

Honest Analysis

FriendlyReviewer detected 1 of 2 golden bugs fully and partially covered the other. Golden #2 (test with false values) was a precise hit — FriendlyReviewer correctly identified that getScopeMap only stores true in production, making the test scenario impossible. The diagnosis was exact.

Golden #1 (asymmetric cache trust logic) was partially covered. FriendlyReviewer did not find the exact asymmetry — the fact that cached permission grants are trusted immediately while cached denials prompt a fresh DB lookup from the same cache. Instead, it found two genuine bugs in the denial cache specifically: a key mismatch that makes the denial cache ineffective for users identified by numeric IDs, and a complete absence of invalidation causing false rejections for up to 30 seconds after role changes. These are real issues in the same functional area, but they address a different mechanism than the golden described. The core design flaw — asymmetric trust of cached grants versus denials — went undetected.

The six supplementary findings are all legitimate. Two stand out as genuinely critical: the duplicate database call on cache error (where GetUserIdentifiers is invoked twice in sequence when the cache errors, doubling latency for an already slow path), and the denial cache key mismatch mentioned above. The metrics labeling issue conflating “cache miss” with “cache hit without scope” is a medium-severity quality concern — it would mislead operators trying to evaluate cache effectiveness. The missing test coverage for List caching edge cases is another medium-severity gap that, while not urgent individually, represents the kind of systematic debt that accumulates over time.

One minor observation: the finding about “test with false values” (golden #2) appears as both a golden hit and a supplementary finding. FriendlyReviewer reported it as a low-severity test quality issue, while the golden dataset framed it as a correctness concern. Both perspectives are valid, but the dual classification highlights how the same observation can be interpreted differently depending on what the reviewer considers the “main” issue versus a side observation. This is acceptable — the finding is valid either way.

For a 4-file PR with subtle caching logic, FriendlyReviewer provided useful coverage of concrete denial cache bugs and the test quality issue, but missed the higher-level architectural concern about asymmetric cache trust.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
2 high, 3 medium, 1 low

The code review of the RBAC caching implementation identifies several correctness and efficiency issues that should be addressed before merging. Key concerns include a denial cache key mismatch causing cache ineffectiveness for numeric user IDs, duplicate database calls on cache-miss errors, misleading metrics accounting, missing test coverage for List caching edge cases, and absence of denial cache invalidation leading to temporary false rejections. Overall, the caching architecture is sound but requires these refinements to ensure reliability and performance.

These might need a close look

🔴 pkg/services/authz/rbac/service.go (L117)
Denial cache key uses raw checkReq.UserUID while permission cache keys use resolved userIdentifiers.UID, causing key mismatch and effectively breaking the denial cache for users identified by numeric IDs. Use the resolved UID for the denial cache key.
🔴 pkg/services/authz/rbac/service.go (L381)
If getCachedIdentityPermissions fails because GetUserIdentifiers returns a DB error, the error propagates to Check which then calls getIdentityPermissions → getUserPermissions → GetUserIdentifiers again, duplicating the DB call. Return cache.ErrNotFound from getCachedIdentityPermissions on any error to let the downstream path handle it once.

Worth checking

🟡 pkg/services/authz/rbac/service.go (L141)
Metrics permissionCacheUsage is incremented with label "false" when the cache is hit but the specific scope is not found, conflating “no cache entry” with “cache entry existed but didn’t contain the scope.” Fix to report a cache hit accurately.
🟡 pkg/services/authz/rbac/service.go (L155)
The denial cache (permDenialCache) is never explicitly invalidated; entries persist for 30s, causing false rejections when a user's RBAC roles change. Consider documenting the trade-off, supporting invalidation, or using a shorter TTL.
🟡 pkg/services/authz/rbac/service_test.go (L1288)
Test coverage for List caching is limited to the cache-hit happy path. Missing tests: cache miss → DB fallback, outdated cache, cached anonymous permissions, and renderer identity type. Add these to ensure robustness.

Small things (take or leave)

🔵 pkg/services/authz/rbac/service_test.go (L967)
Test “Should deny on explicit cache deny entry” sets permCache with false values, which never occur in production (getScopeMap only stores true). Update the test to use a missing scope instead, or add a clarifying comment.

AuthZService: improve authz caching