From 9adaf1565c2408e3e13668af7d50cdcdd5d77242 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 19 Oct 2022 11:53:59 +0200 Subject: [PATCH] RBAC: Allow to list users for dashboard / folder admins (#57080) * RBAC: Use query struct in tests * RBAC: If access control enforcement is disabled don't filter out users when fetching permissions --- .../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 542eb969942..a89c39c120b 100644 --- a/pkg/services/accesscontrol/resourcepermissions/service.go +++ b/pkg/services/accesscontrol/resourcepermissions/service.go @@ -125,13 +125,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 e13d0d1767c..2f2a11ec271 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store.go +++ b/pkg/services/accesscontrol/resourcepermissions/store.go @@ -351,13 +351,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 { @@ -371,7 +373,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 2943815e5ad..8eff2896285 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/store_test.go @@ -336,14 +336,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) { @@ -355,11 +352,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", @@ -368,22 +369,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(), @@ -416,23 +455,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) }) } }