From 1c270b1dc2d8960ba1002c69f1f3255749d1e912 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Mon, 27 Nov 2023 13:37:31 +0100 Subject: [PATCH] RBAC: Adjust filter for acl list to check for permissions on service accounts (#78681) Adjust filter to check for permissions on service accounts --- .../resourcepermissions/store.go | 14 +++- .../resourcepermissions/store_test.go | 71 ++++++++++++++----- 2 files changed, 65 insertions(+), 20 deletions(-) diff --git a/pkg/services/accesscontrol/resourcepermissions/store.go b/pkg/services/accesscontrol/resourcepermissions/store.go index 3d62b0d6ad9..21c706d92d2 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store.go +++ b/pkg/services/accesscontrol/resourcepermissions/store.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/team" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/util" @@ -391,8 +392,19 @@ func (s *store) getResourcePermissions(sess *db.Session, orgID int64, query GetR if err != nil { return nil, err } - userQuery += " AND " + userFilter.Where + + filter := "(" + userFilter.Where + " AND NOT u.is_service_account)" + + saFilter, err := accesscontrol.Filter(query.User, "u.id", "serviceaccounts:id:", serviceaccounts.ActionRead) + if err != nil { + return nil, err + } + + filter += " OR (" + saFilter.Where + " AND u.is_service_account)" + + userQuery += " AND " + filter args = append(args, userFilter.Args...) + args = append(args, saFilter.Args...) } teamFilter, err := accesscontrol.Filter(query.User, "t.id", "teams:id:", accesscontrol.ActionTeamsRead) diff --git a/pkg/services/accesscontrol/resourcepermissions/store_test.go b/pkg/services/accesscontrol/resourcepermissions/store_test.go index 50ed2015c6d..d83d41a4fad 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/store_test.go @@ -16,6 +16,7 @@ import ( "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/org/orgimpl" "github.com/grafana/grafana/pkg/services/quota/quotatest" + "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" "github.com/grafana/grafana/pkg/services/user" @@ -355,11 +356,12 @@ func TestIntegrationStore_SetResourcePermissions(t *testing.T) { } type getResourcePermissionsTest struct { - desc string - user *user.SignedInUser - numUsers int - query GetResourcePermissionsQuery - expectedLen int + desc string + user *user.SignedInUser + numUsers int + numServiceAccounts int + query GetResourcePermissionsQuery + expectedLen int } func TestIntegrationStore_GetResourcePermissions(t *testing.T) { @@ -420,6 +422,28 @@ func TestIntegrationStore_GetResourcePermissions(t *testing.T) { }, expectedLen: 2, }, + { + desc: "should return users and service accounts caller can read", + user: &user.SignedInUser{ + OrgID: 1, + Permissions: map[int64]map[string][]string{ + 1: { + accesscontrol.ActionOrgUsersRead: {"users:id:1", "users:id:2", "users:id:3"}, + serviceaccounts.ActionRead: {"serviceaccounts:id:5"}, + }, + }}, + numUsers: 3, + numServiceAccounts: 3, + query: GetResourcePermissionsQuery{ + Actions: []string{"datasources:query"}, + Resource: "datasources", + ResourceID: "1", + ResourceAttribute: "uid", + OnlyManaged: true, + EnforceAccessControl: true, + }, + expectedLen: 4, + }, { desc: "should return permissions for all users when access control is not enforces", user: &user.SignedInUser{ @@ -479,7 +503,7 @@ func TestIntegrationStore_GetResourcePermissions(t *testing.T) { }) require.NoError(t, err) - seedResourcePermissions(t, store, sql, orgService, tt.query.Actions, tt.query.Resource, tt.query.ResourceID, tt.query.ResourceAttribute, tt.numUsers) + seedResourcePermissions(t, store, sql, orgService, tt.query.Actions, tt.query.Resource, tt.query.ResourceID, tt.query.ResourceAttribute, tt.numUsers, tt.numServiceAccounts) tt.query.User = tt.user permissions, err := store.GetResourcePermissions(context.Background(), tt.user.OrgID, tt.query) @@ -489,27 +513,28 @@ func TestIntegrationStore_GetResourcePermissions(t *testing.T) { } } -func seedResourcePermissions(t *testing.T, store *store, sql *sqlstore.SQLStore, orgService org.Service, actions []string, resource, resourceID, resourceAttribute string, numUsers int) { +func seedResourcePermissions( + t *testing.T, store *store, sql *sqlstore.SQLStore, orgService org.Service, + actions []string, resource, resourceID, resourceAttribute string, numUsers, numServiceAccounts int, +) { t.Helper() - var orgModel *org.Org + + orgID, err := orgService.GetOrCreate(context.Background(), "test") + require.NoError(t, err) + usrSvc, err := userimpl.ProvideService(sql, orgService, sql.Cfg, nil, nil, quotatest.New(false, nil), supportbundlestest.NewFakeBundleService()) require.NoError(t, err) - for i := 0; i < numUsers; i++ { - if orgModel == nil { - cmd := &org.CreateOrgCommand{Name: "test", UserID: int64(i)} - addedOrg, err := orgService.CreateWithMember(context.Background(), cmd) - require.NoError(t, err) - orgModel = addedOrg - } - + create := func(login string, isServiceAccount bool) { u, err := usrSvc.Create(context.Background(), &user.CreateUserCommand{ - Login: fmt.Sprintf("user:%s%d", resourceID, i), - OrgID: orgModel.ID, + Login: login, + IsServiceAccount: isServiceAccount, + OrgID: orgID, }) + require.NoError(t, err) - _, err = store.SetUserResourcePermission(context.Background(), 1, accesscontrol.User{ID: u.ID}, SetResourcePermissionCommand{ + _, err = store.SetUserResourcePermission(context.Background(), orgID, accesscontrol.User{ID: u.ID}, SetResourcePermissionCommand{ Actions: actions, Resource: resource, ResourceID: resourceID, @@ -517,6 +542,14 @@ func seedResourcePermissions(t *testing.T, store *store, sql *sqlstore.SQLStore, }, nil) require.NoError(t, err) } + + for i := 0; i < numUsers; i++ { + create(fmt.Sprintf("user:%s:%d", resourceID, i), false) + } + + for i := 0; i < numServiceAccounts; i++ { + create(fmt.Sprintf("sa:%s:%d", resourceID, i), true) + } } func setupTestEnv(t testing.TB) (*store, *sqlstore.SQLStore) {