diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index 6f2f9b7f072..4ec1badfd69 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -143,11 +143,7 @@ func (s *Service) getUserPermissions(ctx context.Context, user identity.Requeste } func (s *Service) getCachedUserPermissions(ctx context.Context, user identity.Requester, options accesscontrol.Options) ([]accesscontrol.Permission, error) { - key, err := permissionCacheKey(user) - if err != nil { - return nil, err - } - + key := permissionCacheKey(user) if !options.ReloadCache { permissions, ok := s.cache.Get(key) if ok { @@ -169,11 +165,7 @@ func (s *Service) getCachedUserPermissions(ctx context.Context, user identity.Re } func (s *Service) ClearUserPermissionCache(user identity.Requester) { - key, err := permissionCacheKey(user) - if err != nil { - return - } - s.cache.Delete(key) + s.cache.Delete(permissionCacheKey(user)) } func (s *Service) DeleteUserPermissions(ctx context.Context, orgID int64, userID int64) error { @@ -215,12 +207,8 @@ func (s *Service) RegisterFixedRoles(ctx context.Context) error { return nil } -func permissionCacheKey(user identity.Requester) (string, error) { - key, err := user.GetCacheKey() - if err != nil { - return "", err - } - return fmt.Sprintf("rbac-permissions-%s", key), nil +func permissionCacheKey(user identity.Requester) string { + return fmt.Sprintf("rbac-permissions-%s", user.GetCacheKey()) } // DeclarePluginRoles allow the caller to declare, to the service, plugin roles and their assignments @@ -380,13 +368,9 @@ func (s *Service) searchUserPermissionsFromCache(orgID int64, searchOptions acce UserID: searchOptions.UserID, OrgID: orgID, } - key, err := permissionCacheKey(tempUser) - if err != nil { - s.log.Debug("Could not obtain cache key to search user permissions", "error", err.Error()) - return nil, false - } - permissions, ok := s.cache.Get(key) + key := permissionCacheKey(tempUser) + permissions, ok := s.cache.Get((key)) if !ok { return nil, false } diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index 598af455117..808db85efb2 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol/database" "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" ) @@ -695,7 +696,6 @@ func TestPermissionCacheKey(t *testing.T) { name string signedInUser *user.SignedInUser expected string - expectedErr error }{ { name: "should return correct key for user", @@ -703,8 +703,7 @@ func TestPermissionCacheKey(t *testing.T) { OrgID: 1, UserID: 1, }, - expected: "rbac-permissions-1-user-1", - expectedErr: nil, + expected: "rbac-permissions-1-user-1", }, { name: "should return correct key for api key", @@ -713,8 +712,7 @@ func TestPermissionCacheKey(t *testing.T) { ApiKeyID: 1, IsServiceAccount: false, }, - expected: "rbac-permissions-1-apikey-1", - expectedErr: nil, + expected: "rbac-permissions-1-api-key-1", }, { name: "should return correct key for service account", @@ -723,8 +721,7 @@ func TestPermissionCacheKey(t *testing.T) { UserID: 1, IsServiceAccount: true, }, - expected: "rbac-permissions-1-service-1", - expectedErr: nil, + expected: "rbac-permissions-1-service-account-1", }, { name: "should return correct key for matching a service account with userId -1", @@ -733,24 +730,20 @@ func TestPermissionCacheKey(t *testing.T) { UserID: -1, IsServiceAccount: true, }, - expected: "rbac-permissions-1-service--1", - expectedErr: nil, + expected: "rbac-permissions-1-service-account--1", }, { - name: "should return error if not matching any", + name: "should use org role if no unique id", signedInUser: &user.SignedInUser{ - OrgID: 1, - UserID: -1, + OrgID: 1, + OrgRole: org.RoleNone, }, - expected: "", - expectedErr: user.ErrNoUniqueID, + expected: "rbac-permissions-1-user-None", }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - str, err := permissionCacheKey(tc.signedInUser) - require.Equal(t, tc.expectedErr, err) - assert.Equal(t, tc.expected, str) + assert.Equal(t, tc.expected, permissionCacheKey(tc.signedInUser)) }) } } diff --git a/pkg/services/auth/identity/requester.go b/pkg/services/auth/identity/requester.go index bd7cc9ab0c1..01b72e679ab 100644 --- a/pkg/services/auth/identity/requester.go +++ b/pkg/services/auth/identity/requester.go @@ -57,7 +57,7 @@ type Requester interface { HasRole(role roletype.RoleType) bool // GetCacheKey returns a unique key for the entity. // Add an extra prefix to avoid collisions with other caches - GetCacheKey() (string, error) + GetCacheKey() string // HasUniqueId returns true if the entity has a unique id HasUniqueId() bool // AuthenticatedBy returns the authentication method used to authenticate the entity. diff --git a/pkg/services/authn/authn.go b/pkg/services/authn/authn.go index 4ed22226942..7641ec51856 100644 --- a/pkg/services/authn/authn.go +++ b/pkg/services/authn/authn.go @@ -180,6 +180,8 @@ const ( NamespaceUser = identity.NamespaceUser NamespaceAPIKey = identity.NamespaceAPIKey NamespaceServiceAccount = identity.NamespaceServiceAccount + NamespaceAnonymous = identity.NamespaceAnonymous + NamespaceRenderService = identity.NamespaceRenderService ) type Identity struct { diff --git a/pkg/services/user/identity.go b/pkg/services/user/identity.go index f4e8b28bba1..f58df59f4e6 100644 --- a/pkg/services/user/identity.go +++ b/pkg/services/user/identity.go @@ -105,17 +105,15 @@ func (u *SignedInUser) HasUniqueId() bool { // GetCacheKey returns a unique key for the entity. // Add an extra prefix to avoid collisions with other caches -func (u *SignedInUser) GetCacheKey() (string, error) { - if u.IsRealUser() { - return fmt.Sprintf("%d-user-%d", u.OrgID, u.UserID), nil +func (u *SignedInUser) GetCacheKey() string { + namespace, id := u.GetNamespacedID() + if !u.HasUniqueId() { + // Hack use the org role as id for identities that do not have a unique id + // e.g. anonymous and render key. + id = string(u.GetOrgRole()) } - if u.IsApiKeyUser() { - return fmt.Sprintf("%d-apikey-%d", u.OrgID, u.ApiKeyID), nil - } - if u.IsServiceAccountUser() { // not considered a real user - return fmt.Sprintf("%d-service-%d", u.OrgID, u.UserID), nil - } - return "", ErrNoUniqueID + + return fmt.Sprintf("%d-%s-%s", u.GetOrgID(), namespace, id) } // GetIsGrafanaAdmin returns true if the user is a server admin @@ -161,6 +159,9 @@ func (u *SignedInUser) GetTeams() []int64 { // GetOrgRole returns the role of the active entity in the active organization func (u *SignedInUser) GetOrgRole() roletype.RoleType { + if u.OrgRole == "" { + return roletype.RoleNone + } return u.OrgRole } @@ -178,7 +179,7 @@ func (u *SignedInUser) GetNamespacedID() (string, string) { return identity.NamespaceAnonymous, "" case u.AuthenticatedBy == "render": //import cycle render if u.UserID == 0 { - return identity.NamespaceRenderService, fmt.Sprintf("%d", u.UserID) + return identity.NamespaceRenderService, "0" } else { // this should never happen as u.UserID > 0 already catches this return identity.NamespaceUser, fmt.Sprintf("%d", u.UserID) }