AuthZService: improve authz caching

[hussam789]

User description

PR #2

---

PR Type

Enhancement

---

Description

• Add permission denial cache to avoid repeated database queries for denied permissions

• Implement two-tier caching strategy: check denial cache first, then permission cache

• Remove client-side caching from in-process RBAC client, use NoopCache instead

• Refactor permission retrieval to use new getCachedIdentityPermissions method

• Add comprehensive tests for cache hit/miss scenarios and denial cache behavior

---

Diagram Walkthrough

flowchart LR
  A["Check Request"] --> B["Check Denial Cache"]
  B -->|Hit| C["Return Denied"]
  B -->|Miss| D["Check Permission Cache"]
  D -->|Hit| E["Check Permission"]
  D -->|Miss| F["Query Database"]
  E -->|Allowed| G["Return Allowed"]
  E -->|Denied| H["Cache Denial"]
  F --> I["Cache Result"]
  I --> G
  I --> H

Loading

File Walkthrough

	Relevant files
Enhancement	rbac.go Refactor RBAC client initialization and caching strategy  pkg/services/authz/rbac.go Replace newRBACClient function with direct authzlib.NewClient calls Use NoopCache for in-process RBAC client instead of client-side caching Use LocalCache with 30-second expiry for remote RBAC client Add NoopCache implementation that always returns ErrNotFound +22/-6    cache.go Add permission denial cache key generation                              pkg/services/authz/rbac/cache.go Add userPermDenialCacheKey function to generate cache keys for denied permissions Key includes namespace, userUID, action, name, and parent folder for granular denial tracking +4/-0      service.go Implement two-tier caching with denial cache                          pkg/services/authz/rbac/service.go Add permDenialCache field to Service struct for caching denied permissions Add getCachedIdentityPermissions method to check cache before database queries Refactor Check method to check denial cache first, then permission cache Refactor List method to use getCachedIdentityPermissions for cache lookup Remove inline cache checks from getUserPermissions and getAnonymousPermissions Cache permission denials to avoid repeated database queries for same denied access +74/-19
Tests	service_test.go Add comprehensive cache behavior tests                                      pkg/services/authz/rbac/service_test.go Remove test case for cache hit in getUserPermissions (now handled by getCachedIdentityPermissions) Add TestService_CacheCheck with four test cases covering cache hits, misses, outdated cache, and denial cache Add TestService_CacheList to verify List operation uses cached permissions Update setupService to initialize permDenialCache +140/-8

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: The new denial-cache and permission-cache paths in Check/List add critical authorization decisions but do not add or modify audit logging to record who attempted what action and the outcome. Referred Code permDenialKey := userPermDenialCacheKey(checkReq.Namespace.Value, checkReq.UserUID, checkReq.Action, checkReq.Name, checkReq.ParentFolder) if _, ok := s.permDenialCache.Get(ctx, permDenialKey); ok { s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc() s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() return &authzv1.CheckResponse{Allowed: false}, nil } cachedPerms, err := s.getCachedIdentityPermissions(ctx, checkReq.Namespace, checkReq.IdentityType, checkReq.UserUID, checkReq.Action) if err == nil { allowed, err := s.checkPermission(ctx, cachedPerms, checkReq) if err != nil { ctxLogger.Error("could not check permission", "error", err) s.metrics.requestCount.WithLabelValues("true", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() return deny, err } if allowed { s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc() s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() return &authzv1.CheckResponse{Allowed: allowed}, nil } } ... (clipped 23 lines)

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid redundant database query on cache hit In the Check method, if a permission check on cached data results in a denial, return immediately instead of performing a redundant database query. Also, cache this specific denial. pkg/services/authz/rbac/service.go [123-139] cachedPerms, err := s.getCachedIdentityPermissions(ctx, checkReq.Namespace, checkReq.IdentityType, checkReq.UserUID, checkReq.Action) if err == nil { + s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc() allowed, err := s.checkPermission(ctx, cachedPerms, checkReq) if err != nil { ctxLogger.Error("could not check permission", "error", err) s.metrics.requestCount.WithLabelValues("true", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() return deny, err } - if allowed { - s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc() - s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() - return &authzv1.CheckResponse{Allowed: allowed}, nil + + if !allowed { + s.permDenialCache.Set(ctx, permDenialKey, true) } + + s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc() + return &authzv1.CheckResponse{Allowed: allowed}, nil } s.metrics.permissionCacheUsage.WithLabelValues("false", checkReq.Action).Inc() permissions, err := s.getIdentityPermissions(ctx, checkReq.Namespace, checkReq.IdentityType, checkReq.UserUID, checkReq.Action) Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a performance issue where a cache hit that results in a denial incorrectly falls back to a database query. Fixing this improves efficiency by making the cache authoritative, which is the intended purpose of this PR.	Medium
	Correctly set permission cache in test In the test "Should deny on explicit cache deny entry", correct the permission cache setup to grant access (true) instead of denying it (false) to properly test that the denial cache overrides a grant. pkg/services/authz/rbac/service_test.go [973-995] t.Run("Should deny on explicit cache deny entry", func(t *testing.T) { s := setupService() s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID) // Explicitly deny access to the dashboard s.permDenialCache.Set(ctx, userPermDenialCacheKey("org-12", "test-uid", "dashboards:read", "dash1", "fold1"), true) // 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}) resp, err := s.Check(ctx, &authzv1.CheckRequest{ Namespace: "org-12", Subject: "user:test-uid", Group: "dashboard.grafana.app", Resource: "dashboards", Verb: "get", Name: "dash1", Folder: "fold1", }) require.NoError(t, err) assert.False(t, resp.Allowed) }) Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out a flaw in the test logic where a permission grant was incorrectly simulated. Correcting the test ensures it accurately verifies that the denial cache overrides a positive permission grant, improving test correctness.	Medium
More