IAM: fix GetSearchPermissionCacheKey uniqueness (#95192)

* fix: Change users permissions search to use a consistent key without collisions

* Move HashString to cacheutils

* Change error handling logic for what to do with a cache key

* Add a test that confirms search cache key consistency

pkg/services/accesscontrol/acimpl/service.go

@@ -702,8 +702,12 @@ func (s *Service) searchUserPermissions(ctx context.Context, orgID int64, search
 		permissions = s.actionResolver.ExpandActionSetsWithFilter(permissions, GetActionFilter(searchOptions))
 	}
 
-	key := accesscontrol.GetSearchPermissionCacheKey(&user.SignedInUser{UserID: userID, OrgID: orgID}, searchOptions)
-	s.cache.Set(key, permissions, cacheTTL)
+	key, err := accesscontrol.GetSearchPermissionCacheKey(s.log, &user.SignedInUser{UserID: userID, OrgID: orgID}, searchOptions)
+	if err != nil {
+		s.log.Warn("failed to create search permission cache key", "err", err)
+	} else {
+		s.cache.Set(key, permissions, cacheTTL)
+	}
 
 	return permissions, nil
 }
@@ -723,7 +727,11 @@ func (s *Service) searchUserPermissionsFromCache(ctx context.Context, orgID int6
 		OrgID:  orgID,
 	}
 
-	key := accesscontrol.GetSearchPermissionCacheKey(tempUser, searchOptions)
+	key, err := accesscontrol.GetSearchPermissionCacheKey(s.log, tempUser, searchOptions)
+	if err != nil {
+		s.log.Warn("failed to create search permission cache key", "err", err)
+		return nil, false
+	}
 	permissions, ok := s.cache.Get((key))
 	if !ok {
 		metrics.MAccessSearchUserPermissionsCacheUsage.WithLabelValues(accesscontrol.CacheMiss).Inc()

pkg/services/accesscontrol/cacheutils.go

@@ -1,31 +1,46 @@
 package accesscontrol
 
 import (
+	"bytes"
+	"encoding/base64"
+	"encoding/gob"
 	"fmt"
+	"hash/fnv"
 	"strings"
 
+	"github.com/grafana/grafana/pkg/infra/log"
+
 	"github.com/grafana/grafana/pkg/apimachinery/identity"
 )
 
+func (s *SearchOptions) HashString() (string, error) {
+	if s == nil {
+		return "", nil
+	}
+	var buf bytes.Buffer
+	encoder := gob.NewEncoder(&buf)
+	if err := encoder.Encode(s); err != nil {
+		return "", err
+	}
+	h := fnv.New64a()
+	_, err := h.Write(buf.Bytes())
+	if err != nil {
+		return "", err
+	}
+	return base64.StdEncoding.EncodeToString(h.Sum(nil)), nil
+}
+
 func GetUserPermissionCacheKey(user identity.Requester) string {
 	return fmt.Sprintf("rbac-permissions-%s", user.GetCacheKey())
 }
 
-func GetSearchPermissionCacheKey(user identity.Requester, searchOptions SearchOptions) string {
-	key := fmt.Sprintf("rbac-permissions-%s", user.GetCacheKey())
-	if searchOptions.Action != "" {
-		key += fmt.Sprintf("-%s", searchOptions.Action)
+func GetSearchPermissionCacheKey(log log.Logger, user identity.Requester, searchOptions SearchOptions) (string, error) {
+	searchHash, err := searchOptions.HashString()
+	if err != nil {
+		return "", err
 	}
-	if searchOptions.Scope != "" {
-		key += fmt.Sprintf("-%s", searchOptions.Scope)
-	}
-	if len(searchOptions.RolePrefixes) > 0 {
-		key += "-" + strings.Join(searchOptions.RolePrefixes, "-")
-	}
-	if searchOptions.ActionPrefix != "" {
-		key += fmt.Sprintf("-%s", searchOptions.ActionPrefix)
-	}
-	return key
+	key := fmt.Sprintf("rbac-permissions-%s-%s", user.GetCacheKey(), searchHash)
+	return key, nil
 }
 
 func GetUserDirectPermissionCacheKey(user identity.Requester) string {

pkg/services/accesscontrol/cacheutils_test.go

@@ -4,13 +4,17 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/authlib/claims"
 
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/user"
 )
 
+var testLogger = log.New("test")
+
 func TestPermissionCacheKey(t *testing.T) {
 	testcases := []struct {
 		name         string
@@ -75,23 +79,18 @@ func TestPermissionCacheKey(t *testing.T) {
 }
 
 func TestGetSearchPermissionCacheKey(t *testing.T) {
-	testcases := []struct {
-		name          string
+	keyInputs := []struct {
 		signedInUser  *user.SignedInUser
 		searchOptions SearchOptions
-		expected      string
 	}{
 		{
-			name: "should return correct key for user with no options",
 			signedInUser: &user.SignedInUser{
 				OrgID:  1,
 				UserID: 1,
 			},
 			searchOptions: SearchOptions{},
-			expected:      "rbac-permissions-1-user-1",
 		},
 		{
-			name: "should return correct key for user with action",
 			signedInUser: &user.SignedInUser{
 				OrgID:  1,
 				UserID: 1,
@@ -99,10 +98,8 @@ func TestGetSearchPermissionCacheKey(t *testing.T) {
 			searchOptions: SearchOptions{
 				Action: "datasources:read",
 			},
-			expected: "rbac-permissions-1-user-1-datasources:read",
 		},
 		{
-			name: "should return correct key for user with scope",
 			signedInUser: &user.SignedInUser{
 				OrgID:  1,
 				UserID: 1,
@@ -110,10 +107,8 @@ func TestGetSearchPermissionCacheKey(t *testing.T) {
 			searchOptions: SearchOptions{
 				Scope: "datasources:*",
 			},
-			expected: "rbac-permissions-1-user-1-datasources:*",
 		},
 		{
-			name: "should return correct key for user with action and scope",
 			signedInUser: &user.SignedInUser{
 				OrgID:  1,
 				UserID: 1,
@@ -122,10 +117,8 @@ func TestGetSearchPermissionCacheKey(t *testing.T) {
 				Action: "datasources:read",
 				Scope:  "datasources:*",
 			},
-			expected: "rbac-permissions-1-user-1-datasources:read-datasources:*",
 		},
 		{
-			name: "should return correct key for user with role prefixes",
 			signedInUser: &user.SignedInUser{
 				OrgID:  1,
 				UserID: 1,
@@ -133,13 +126,40 @@ func TestGetSearchPermissionCacheKey(t *testing.T) {
 			searchOptions: SearchOptions{
 				RolePrefixes: []string{"foo", "bar"},
 			},
-			expected: "rbac-permissions-1-user-1-foo-bar",
 		},
 	}
 
-	for _, tc := range testcases {
-		t.Run(tc.name, func(t *testing.T) {
-			assert.Equal(t, tc.expected, GetSearchPermissionCacheKey(tc.signedInUser, tc.searchOptions))
-		})
+	cacheKeys := make([]string, 0, len(keyInputs))
+
+	for _, i := range keyInputs {
+		key, err := GetSearchPermissionCacheKey(testLogger, i.signedInUser, i.searchOptions)
+		require.NoError(t, err)
+		cacheKeys = append(cacheKeys, key)
 	}
+
+	uniqueCheck := make(map[string]bool)
+	for _, str := range cacheKeys {
+		require.False(t, uniqueCheck[str], "Found duplicate string: %s", str)
+		uniqueCheck[str] = true
+	}
+
+	assert.Equal(t, len(cacheKeys), len(uniqueCheck), "The slice contains duplicate strings")
+
+	t.Run("the cache key is consistent", func(t *testing.T) {
+		user := &user.SignedInUser{
+			OrgID:  1,
+			UserID: 1,
+		}
+		key1, err := GetSearchPermissionCacheKey(testLogger, user, SearchOptions{
+			ActionPrefix: "foobar",
+			RolePrefixes: []string{"foo", "bar"},
+		})
+		require.NoError(t, err)
+		key2, err := GetSearchPermissionCacheKey(testLogger, user, SearchOptions{
+			ActionPrefix: "foobar",
+			RolePrefixes: []string{"foo", "bar"},
+		})
+		require.NoError(t, err)
+		assert.Equal(t, key1, key2, "expected search cache keys to be consistent")
+	})
 }