AuthZService: improve authz caching#3
Conversation
* remove the use of client side cache for in-proc authz client Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com> * add a permission denial cache, fetch perms if not in either of the caches Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com> * Clean up tests Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Cache tests Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Add test to list + cache Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Add outdated cache test Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Re-organize metrics Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> --------- Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pull request implements permission-denial caching for RBAC authorization checks. It introduces a Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service
participant PermDenialCache
participant CachedPermCache
participant Database
Client->>Service: Check(ctx, identity, action, resource)
Service->>PermDenialCache: Get(key)
alt Denial Cached
PermDenialCache-->>Service: true (NotFound error)
Service-->>Client: false (denied, metrics updated)
else Not Cached
Service->>CachedPermCache: getCachedIdentityPermissions(...)
alt Cache Hit
CachedPermCache-->>Service: map[string]bool
Service->>Service: checkPermission(map)
Service-->>Client: bool result
else Cache Miss
CachedPermCache->>Database: Query permissions
Database-->>CachedPermCache: Permissions
CachedPermCache-->>Service: Permissions
Service->>Service: Evaluate permissions
alt Denied
Service->>PermDenialCache: Set(key, true)
end
Service-->>Client: bool result
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/services/authz/rbac.go (1)
239-251: Consider adding a brief doc comment for the exported type.The
NoopCacheimplementation is correct. Since it's an exported type, a short doc comment explaining its purpose (disabling client-side caching for in-process scenarios) would improve discoverability.📝 Suggested documentation
+// NoopCache is a cache implementation that never stores data. +// It is used for in-process clients where server-side caching is preferred. type NoopCache struct{}pkg/services/authz/rbac/service_test.go (2)
973-995: Test correctly validates denial cache precedence.This test properly verifies that the denial cache takes precedence over other cached permissions. Note that line 982 sets the permission value to
false, which wouldn't grant access anyway due to howscopeMap[key]is evaluated incheckPermission. If the intent is to prove that permCache is entirely bypassed when denial cache hits, consider usingtrue:📝 Minor clarity improvement
// Allow access to the dashboard to prove this is not checked - s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": false}) + s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": true})
1291-1323: Consider adding cache miss test for List operation.The test covers the cache hit scenario well. For parity with
TestService_CacheCheck, consider adding a test case that verifies List correctly falls back to the database when permissions are not cached.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/services/authz/rbac.gopkg/services/authz/rbac/cache.gopkg/services/authz/rbac/service.gopkg/services/authz/rbac/service_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/services/authz/rbac/service_test.go (2)
public/app/api/clients/advisor/endpoints.gen.ts (1)
Check(382-390)packages/grafana-data/src/index.ts (1)
Scope(573-573)
pkg/services/authz/rbac.go (1)
pkg/services/authz/zanzana/client.go (1)
NewClient(23-25)
🔇 Additional comments (8)
pkg/services/authz/rbac.go (1)
101-105: LGTM - Appropriate use of NoopCache for in-process client.Using
NoopCachefor the in-process client makes sense since the server-sideServicealready maintains its own caching layer (viapermCache,permDenialCache, etc.). This avoids redundant caching at the client level when both client and server run in the same process.pkg/services/authz/rbac/cache.go (1)
30-32: LGTM - Cache key follows established patterns.The key structure mirrors existing cache key functions. The inclusion of
nameandparentin the denial cache key correctly scopes denials to specific resource instances.pkg/services/authz/rbac/service_test.go (1)
1336-1336: LGTM - Test setup correctly initializes the new denial cache.The
permDenialCacheis properly initialized insetupService, enabling the new cache tests to function correctly.pkg/services/authz/rbac/service.go (5)
56-61: LGTM - Struct fields properly organized with new denial cache.The addition of
permDenialCachefollows the established pattern for cache fields in the service.
116-137: Verify the caching strategy handles stale caches correctly.The fast-path logic correctly prioritizes the denial cache. The fallthrough to DB when cached permissions don't grant access (lines 131-136) handles the "outdated cache" scenario mentioned in tests.
One consideration: if a user genuinely lacks permission for a resource, every first request will traverse: denial cache miss → permission cache hit (but no match) → DB query → denial cached. The
permissionCacheUsagemetric on line 137 will record a "miss" even though the permission cache was consulted. This is technically accurate (no cache-based decision was made), but worth noting in observability documentation.
153-155: LGTM - Denial caching after authoritative DB check.Caching denials only after the full DB permission check ensures we don't cache false negatives from stale/incomplete permission caches. This asymmetric approach (explicit denial cache + permission cache for grants) is sound.
342-368: LGTM - Cached permission lookup correctly handles all identity types.The implementation properly:
- Returns
cache.ErrNotFoundfor render service (since those permissions are hardcoded, not cached)- Leverages
GetUserIdentifierswhich has its own caching layer for user/service-account lookups- Uses consistent cache keys matching what
getUserPermissions/getAnonymousPermissionspopulate
180-192: LGTM - List uses straightforward cache-or-DB pattern.Unlike
Checkwhich needs the denial cache for specific resource+folder combinations,Listcorrectly uses a simple cache-hit-or-fetch pattern since it returns the complete permission set for an action.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
2 participants
Test 2nn
Summary by CodeRabbit
Release Notes
New Features
Performance
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/grafana-coderabbit#2](https://github.com/ai-code-review-evaluation/grafana-coderabbit/pull/2)*