From 0302b757213279c826ebd184a4d687e5e2f15919 Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Fri, 10 May 2024 11:06:52 +0200 Subject: [PATCH] Access control: Use search options for computing permissions cache key (#87589) --- pkg/services/accesscontrol/acimpl/service.go | 4 +- .../accesscontrol/acimpl/service_test.go | 63 -------- pkg/services/accesscontrol/cacheutils.go | 17 +++ pkg/services/accesscontrol/cacheutils_test.go | 144 ++++++++++++++++++ 4 files changed, 163 insertions(+), 65 deletions(-) create mode 100644 pkg/services/accesscontrol/cacheutils_test.go diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index 867d94873d5..92e75e3a704 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -553,7 +553,7 @@ func (s *Service) searchUserPermissions(ctx context.Context, orgID int64, search } permissions = append(permissions, dbPermissions[userID]...) - key := accesscontrol.GetPermissionCacheKey(&user.SignedInUser{UserID: userID, OrgID: orgID}) + key := accesscontrol.GetSearchPermissionCacheKey(&user.SignedInUser{UserID: userID, OrgID: orgID}, searchOptions) s.cache.Set(key, permissions, cacheTTL) return permissions, nil @@ -571,7 +571,7 @@ func (s *Service) searchUserPermissionsFromCache(orgID int64, searchOptions acce OrgID: orgID, } - key := accesscontrol.GetPermissionCacheKey(tempUser) + key := accesscontrol.GetSearchPermissionCacheKey(tempUser, searchOptions) permissions, ok := s.cache.Get((key)) if !ok { metrics.MAccessSearchUserPermissionsCacheUsage.WithLabelValues(accesscontrol.CacheMiss).Inc() diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index 27d2a918925..a021ea4d8dd 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -20,7 +20,6 @@ import ( "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/licensing" - "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/testsuite" @@ -748,68 +747,6 @@ func TestService_SearchUserPermissions(t *testing.T) { } } -func TestPermissionCacheKey(t *testing.T) { - testcases := []struct { - name string - signedInUser *user.SignedInUser - expected string - }{ - { - name: "should return correct key for user", - signedInUser: &user.SignedInUser{ - OrgID: 1, - UserID: 1, - NamespacedID: identity.MustParseNamespaceID("user:1"), - }, - expected: "rbac-permissions-1-user-1", - }, - { - name: "should return correct key for api key", - signedInUser: &user.SignedInUser{ - OrgID: 1, - ApiKeyID: 1, - IsServiceAccount: false, - NamespacedID: identity.MustParseNamespaceID("user:1"), - }, - expected: "rbac-permissions-1-api-key-1", - }, - { - name: "should return correct key for service account", - signedInUser: &user.SignedInUser{ - OrgID: 1, - UserID: 1, - IsServiceAccount: true, - NamespacedID: identity.MustParseNamespaceID("service-account:1"), - }, - expected: "rbac-permissions-1-service-account-1", - }, - { - name: "should return correct key for matching a service account with userId -1", - signedInUser: &user.SignedInUser{ - OrgID: 1, - UserID: -1, - IsServiceAccount: true, - NamespacedID: identity.MustParseNamespaceID("service-account:-1"), - }, - expected: "rbac-permissions-1-service-account--1", - }, - { - name: "should use org role if no unique id", - signedInUser: &user.SignedInUser{ - OrgID: 1, - OrgRole: org.RoleNone, - NamespacedID: identity.MustParseNamespaceID("user:1"), - }, - expected: "rbac-permissions-1-user-None", - }, - } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, accesscontrol.GetPermissionCacheKey(tc.signedInUser)) - }) - } -} - func TestService_SaveExternalServiceRole(t *testing.T) { type run struct { cmd accesscontrol.SaveExternalServiceRoleCommand diff --git a/pkg/services/accesscontrol/cacheutils.go b/pkg/services/accesscontrol/cacheutils.go index 45b191bc29e..40119929090 100644 --- a/pkg/services/accesscontrol/cacheutils.go +++ b/pkg/services/accesscontrol/cacheutils.go @@ -11,6 +11,23 @@ func GetPermissionCacheKey(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) + } + 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 +} + func GetUserDirectPermissionCacheKey(user identity.Requester) string { return fmt.Sprintf("rbac-permissions-direct-%s", user.GetCacheKey()) } diff --git a/pkg/services/accesscontrol/cacheutils_test.go b/pkg/services/accesscontrol/cacheutils_test.go new file mode 100644 index 00000000000..e97037da3fc --- /dev/null +++ b/pkg/services/accesscontrol/cacheutils_test.go @@ -0,0 +1,144 @@ +package accesscontrol + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/grafana/grafana/pkg/services/auth/identity" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/user" +) + +func TestPermissionCacheKey(t *testing.T) { + testcases := []struct { + name string + signedInUser *user.SignedInUser + expected string + }{ + { + name: "should return correct key for user", + signedInUser: &user.SignedInUser{ + OrgID: 1, + UserID: 1, + NamespacedID: identity.MustParseNamespaceID("user:1"), + }, + expected: "rbac-permissions-1-user-1", + }, + { + name: "should return correct key for api key", + signedInUser: &user.SignedInUser{ + OrgID: 1, + ApiKeyID: 1, + IsServiceAccount: false, + NamespacedID: identity.MustParseNamespaceID("user:1"), + }, + expected: "rbac-permissions-1-api-key-1", + }, + { + name: "should return correct key for service account", + signedInUser: &user.SignedInUser{ + OrgID: 1, + UserID: 1, + IsServiceAccount: true, + NamespacedID: identity.MustParseNamespaceID("service-account:1"), + }, + expected: "rbac-permissions-1-service-account-1", + }, + { + name: "should return correct key for matching a service account with userId -1", + signedInUser: &user.SignedInUser{ + OrgID: 1, + UserID: -1, + IsServiceAccount: true, + NamespacedID: identity.MustParseNamespaceID("service-account:-1"), + }, + expected: "rbac-permissions-1-service-account--1", + }, + { + name: "should use org role if no unique id", + signedInUser: &user.SignedInUser{ + OrgID: 1, + OrgRole: org.RoleNone, + NamespacedID: identity.MustParseNamespaceID("user:1"), + }, + expected: "rbac-permissions-1-user-None", + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, GetPermissionCacheKey(tc.signedInUser)) + }) + } +} + +func TestGetSearchPermissionCacheKey(t *testing.T) { + testcases := []struct { + name string + 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, + }, + 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, + }, + 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, + }, + searchOptions: SearchOptions{ + 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, + }, + 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)) + }) + } +}