From dac165a7fca3e9a5b532d0fc94c29b7924557943 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Mon, 31 Oct 2022 12:31:43 +0100 Subject: [PATCH] RBAC: Allow to list users for dashboard / folder admins (#57080) (#57252) * RBAC: Use query struct in tests * RBAC: If access control enforcement is disabled don't filter out users when fetching permissions (cherry picked from commit 9adaf1565c2408e3e13668af7d50cdcdd5d77242) Co-authored-by: Karl Persson --- .../resourcepermissions/models.go | 15 +-- .../resourcepermissions/service.go | 15 +-- .../resourcepermissions/store.go | 16 +-- .../resourcepermissions/store_test.go | 102 +++++++++++------- 4 files changed, 90 insertions(+), 58 deletions(-) diff --git a/pkg/services/accesscontrol/resourcepermissions/models.go b/pkg/services/accesscontrol/resourcepermissions/models.go index ac73ef5885f..f3b5425f0e9 100644 --- a/pkg/services/accesscontrol/resourcepermissions/models.go +++ b/pkg/services/accesscontrol/resourcepermissions/models.go @@ -22,11 +22,12 @@ type SetResourcePermissionsCommand struct { } type GetResourcePermissionsQuery struct { - Actions []string - Resource string - ResourceID string - ResourceAttribute string - OnlyManaged bool - InheritedScopes []string - User *user.SignedInUser + Actions []string + Resource string + ResourceID string + ResourceAttribute string + OnlyManaged bool + InheritedScopes []string + EnforceAccessControl bool + User *user.SignedInUser } diff --git a/pkg/services/accesscontrol/resourcepermissions/service.go b/pkg/services/accesscontrol/resourcepermissions/service.go index b96ed0bf192..0770354d2d5 100644 --- a/pkg/services/accesscontrol/resourcepermissions/service.go +++ b/pkg/services/accesscontrol/resourcepermissions/service.go @@ -123,13 +123,14 @@ func (s *Service) GetPermissions(ctx context.Context, user *user.SignedInUser, r } return s.store.GetResourcePermissions(ctx, user.OrgID, GetResourcePermissionsQuery{ - User: user, - Actions: s.actions, - Resource: s.options.Resource, - ResourceID: resourceID, - ResourceAttribute: s.options.ResourceAttribute, - InheritedScopes: inheritedScopes, - OnlyManaged: s.options.OnlyManaged, + User: user, + Actions: s.actions, + Resource: s.options.Resource, + ResourceID: resourceID, + ResourceAttribute: s.options.ResourceAttribute, + InheritedScopes: inheritedScopes, + OnlyManaged: s.options.OnlyManaged, + EnforceAccessControl: s.license.FeatureEnabled("accesscontrol.enforcement"), }) } diff --git a/pkg/services/accesscontrol/resourcepermissions/store.go b/pkg/services/accesscontrol/resourcepermissions/store.go index 12aa1246614..c1172909d1f 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store.go +++ b/pkg/services/accesscontrol/resourcepermissions/store.go @@ -350,13 +350,15 @@ func (s *store) getResourcePermissions(sess *sqlstore.DBSession, orgID int64, qu } initialLength := len(args) - - userFilter, err := accesscontrol.Filter(query.User, "u.id", "users:id:", accesscontrol.ActionOrgUsersRead) - if err != nil { - return nil, err + userQuery := userSelect + userFrom + where + if query.EnforceAccessControl { + userFilter, err := accesscontrol.Filter(query.User, "u.id", "users:id:", accesscontrol.ActionOrgUsersRead) + if err != nil { + return nil, err + } + userQuery += " AND " + userFilter.Where + args = append(args, userFilter.Args...) } - user := userSelect + userFrom + where + " AND " + userFilter.Where - args = append(args, userFilter.Args...) teamFilter, err := accesscontrol.Filter(query.User, "t.id", "teams:id:", accesscontrol.ActionTeamsRead) if err != nil { @@ -370,7 +372,7 @@ func (s *store) getResourcePermissions(sess *sqlstore.DBSession, orgID int64, qu builtin := builtinSelect + builtinFrom + where args = append(args, args[:initialLength]...) - sql := user + " UNION " + team + " UNION " + builtin + sql := userQuery + " UNION " + team + " UNION " + builtin queryResults := make([]flatResourcePermission, 0) if err := sess.SQL(sql, args...).Find(&queryResults); err != nil { return nil, err diff --git a/pkg/services/accesscontrol/resourcepermissions/store_test.go b/pkg/services/accesscontrol/resourcepermissions/store_test.go index 40b731821bd..8bf801f4a31 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/store_test.go @@ -335,14 +335,11 @@ func TestIntegrationStore_SetResourcePermissions(t *testing.T) { } type getResourcePermissionsTest struct { - desc string - user *user.SignedInUser - numUsers int - actions []string - resource string - resourceID string - resourceAttribute string - onlyManaged bool + desc string + user *user.SignedInUser + numUsers int + query GetResourcePermissionsQuery + expectedLen int } func TestIntegrationStore_GetResourcePermissions(t *testing.T) { @@ -354,11 +351,15 @@ func TestIntegrationStore_GetResourcePermissions(t *testing.T) { Permissions: map[int64]map[string][]string{ 1: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}, }}, - numUsers: 3, - actions: []string{"datasources:query"}, - resource: "datasources", - resourceID: "1", - resourceAttribute: "uid", + numUsers: 3, + query: GetResourcePermissionsQuery{ + Actions: []string{"datasources:query"}, + Resource: "datasources", + ResourceID: "1", + ResourceAttribute: "uid", + EnforceAccessControl: true, + }, + expectedLen: 4, }, { desc: "should return manage permissions for all resource ids", @@ -367,22 +368,60 @@ func TestIntegrationStore_GetResourcePermissions(t *testing.T) { Permissions: map[int64]map[string][]string{ 1: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}, }}, - numUsers: 3, - actions: []string{"datasources:query"}, - resource: "datasources", - resourceID: "1", - resourceAttribute: "uid", - onlyManaged: true, + numUsers: 3, + query: GetResourcePermissionsQuery{ + Actions: []string{"datasources:query"}, + Resource: "datasources", + ResourceID: "1", + ResourceAttribute: "uid", + OnlyManaged: true, + EnforceAccessControl: true, + }, + expectedLen: 3, + }, + { + desc: "should return users caller can read", + user: &user.SignedInUser{ + OrgID: 1, + Permissions: map[int64]map[string][]string{ + 1: {accesscontrol.ActionOrgUsersRead: {"users:id:1", "users:id:3"}}, + }}, + numUsers: 3, + query: GetResourcePermissionsQuery{ + Actions: []string{"datasources:query"}, + Resource: "datasources", + ResourceID: "1", + ResourceAttribute: "uid", + OnlyManaged: true, + EnforceAccessControl: true, + }, + expectedLen: 2, + }, + { + desc: "should return permissions for all users when access control is not enforces", + user: &user.SignedInUser{ + OrgID: 1, + Permissions: map[int64]map[string][]string{1: {}}}, + numUsers: 3, + query: GetResourcePermissionsQuery{ + Actions: []string{"datasources:query"}, + Resource: "datasources", + ResourceID: "1", + ResourceAttribute: "uid", + OnlyManaged: true, + EnforceAccessControl: false, + }, + expectedLen: 3, }, } - for _, test := range tests { - t.Run(test.desc, func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { store, sql := setupTestEnv(t) err := sql.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { role := &accesscontrol.Role{ - OrgID: test.user.OrgID, + OrgID: tt.user.OrgID, UID: "seeded", Name: "seeded", Updated: time.Now(), @@ -415,23 +454,12 @@ func TestIntegrationStore_GetResourcePermissions(t *testing.T) { }) require.NoError(t, err) - seedResourcePermissions(t, store, sql, test.actions, test.resource, test.resourceID, test.resourceAttribute, test.numUsers) + seedResourcePermissions(t, store, sql, tt.query.Actions, tt.query.Resource, tt.query.ResourceID, tt.query.ResourceAttribute, tt.numUsers) - permissions, err := store.GetResourcePermissions(context.Background(), test.user.OrgID, GetResourcePermissionsQuery{ - User: test.user, - Actions: test.actions, - Resource: test.resource, - ResourceID: test.resourceID, - ResourceAttribute: test.resourceAttribute, - OnlyManaged: test.onlyManaged, - }) + tt.query.User = tt.user + permissions, err := store.GetResourcePermissions(context.Background(), tt.user.OrgID, tt.query) require.NoError(t, err) - - expectedLen := test.numUsers - if !test.onlyManaged { - expectedLen += 1 - } - assert.Len(t, permissions, expectedLen) + assert.Len(t, permissions, tt.expectedLen) }) } }