From 4069fe1c39ced0d0703f23eebb77ad34da873865 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Mon, 15 Aug 2022 09:41:20 +0200 Subject: [PATCH] RBAC: Update permission query to not join on team table (#53677) * RBAC: Add teamIDs to get permission query * RBAC: Remove join on team table and use team ids * RBAC: Pass team ids --- .../accesscontrol/database/database.go | 70 ++++++++++++------- .../accesscontrol/database/database_test.go | 13 +--- pkg/services/accesscontrol/models.go | 1 + .../ossaccesscontrol/ossaccesscontrol.go | 7 +- 4 files changed, 53 insertions(+), 38 deletions(-) diff --git a/pkg/services/accesscontrol/database/database.go b/pkg/services/accesscontrol/database/database.go index eac6ba968f8..6822d4c2d58 100644 --- a/pkg/services/accesscontrol/database/database.go +++ b/pkg/services/accesscontrol/database/database.go @@ -24,17 +24,23 @@ type AccessControlStore struct { func (s *AccessControlStore) GetUserPermissions(ctx context.Context, query accesscontrol.GetUserPermissionsQuery) ([]accesscontrol.Permission, error) { result := make([]accesscontrol.Permission, 0) err := s.sql.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - filter, params := userRolesFilter(query.OrgID, query.UserID, query.Roles) + if query.UserID == 0 && len(query.TeamIDs) == 0 && len(query.Roles) == 0 { + // no permission to fetch + return nil + } - q := `SELECT + filter, params := userRolesFilter(query.OrgID, query.UserID, query.TeamIDs, query.Roles) + + q := ` + SELECT permission.action, permission.scope FROM permission INNER JOIN role ON role.id = permission.role_id ` + filter - if query.Actions != nil { - q += " AND permission.action IN(" + if len(query.Actions) > 0 { + q += " WHERE permission.action IN(" if len(query.Actions) > 0 { q += "?" + strings.Repeat(",?", len(query.Actions)-1) } @@ -54,45 +60,55 @@ func (s *AccessControlStore) GetUserPermissions(ctx context.Context, query acces return result, err } -func userRolesFilter(orgID, userID int64, roles []string) (string, []interface{}) { - params := []interface{}{} - q := `INNER JOIN (` +func userRolesFilter(orgID, userID int64, teamIDs []int64, roles []string) (string, []interface{}) { + var params []interface{} + builder := strings.Builder{} // This is an additional security. We should never have permissions granted to userID 0. // Only allow real users to get user/team permissions (anonymous/apikeys) if userID > 0 { - q += ` - SELECT ur.role_id - FROM user_role AS ur - WHERE ur.user_id = ? - AND (ur.org_id = ? OR ur.org_id = ?) - UNION - SELECT tr.role_id FROM team_role as tr - INNER JOIN team_member as tm ON tm.team_id = tr.team_id - WHERE tm.user_id = ? AND tr.org_id = ?` - params = []interface{}{userID, orgID, globalOrgID, userID, orgID} + builder.WriteString(` + SELECT ur.role_id + FROM user_role AS ur + WHERE ur.user_id = ? + AND (ur.org_id = ? OR ur.org_id = ?) + `) + params = []interface{}{userID, orgID, globalOrgID} + } + + if len(teamIDs) > 0 { + if builder.Len() > 0 { + builder.WriteString("UNION") + } + builder.WriteString(` + SELECT tr.role_id FROM team_role as tr + WHERE tr.team_id IN(?` + strings.Repeat(", ?", len(teamIDs)-1) + `) + AND tr.org_id = ? + `) + for _, id := range teamIDs { + params = append(params, id) + } + params = append(params, orgID) } if len(roles) != 0 { - if userID > 0 { - q += ` - UNION` + if builder.Len() > 0 { + builder.WriteString("UNION") } - q += ` + + builder.WriteString(` SELECT br.role_id FROM builtin_role AS br - WHERE br.role IN (? ` + strings.Repeat(", ?", len(roles)-1) + `) - ` + WHERE br.role IN (?` + strings.Repeat(", ?", len(roles)-1) + `) + AND (br.org_id = ? OR br.org_id = ?) + `) for _, role := range roles { params = append(params, role) } - q += `AND (br.org_id = ? OR br.org_id = ?)` params = append(params, orgID, globalOrgID) } - q += `) as all_role ON role.id = all_role.role_id` - - return q, params + return "INNER JOIN (" + builder.String() + ") as all_role ON role.id = all_role.role_id", params } func deletePermissions(sess *sqlstore.DBSession, ids []int64) error { diff --git a/pkg/services/accesscontrol/database/database_test.go b/pkg/services/accesscontrol/database/database_test.go index 0840c31e0d0..2d90177ecaf 100644 --- a/pkg/services/accesscontrol/database/database_test.go +++ b/pkg/services/accesscontrol/database/database_test.go @@ -66,16 +66,6 @@ func TestAccessControlStore_GetUserPermissions(t *testing.T) { expected: 3, actions: []string{"dashboards:write"}, }, - { - desc: "Should return no permission when passing empty slice of actions", - orgID: 1, - role: "Viewer", - userPermissions: []string{"1", "2", "10"}, - teamPermissions: []string{"100", "2"}, - builtinPermissions: []string{"5", "6"}, - expected: 0, - actions: []string{}, - }, { desc: "should only get br permissions for anonymous user", anonymousUser: true, @@ -131,14 +121,17 @@ func TestAccessControlStore_GetUserPermissions(t *testing.T) { } userID := user.ID + teamIDs := []int64{team.Id} if tt.anonymousUser { userID = 0 + teamIDs = []int64{} } permissions, err := store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{ OrgID: tt.orgID, UserID: userID, Roles: roles, Actions: tt.actions, + TeamIDs: teamIDs, }) require.NoError(t, err) diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index 2227f0e598f..5b99055d3f4 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -211,6 +211,7 @@ type GetUserPermissionsQuery struct { UserID int64 `json:"userId"` Roles []string Actions []string + TeamIDs []int64 } // ScopeParams holds the parameters used to fill in scope templates diff --git a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go index 0262987197e..39d80206378 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go @@ -102,6 +102,10 @@ func (ac *OSSAccessControlService) Evaluate(ctx context.Context, user *user.Sign return resolvedEvaluator.Evaluate(user.Permissions[user.OrgID]), nil } +var actionsToFetch = append( + TeamAdminActions, append(DashboardAdminActions, FolderAdminActions...)..., +) + // GetUserPermissions returns user permissions based on built-in roles func (ac *OSSAccessControlService) GetUserPermissions(ctx context.Context, user *user.SignedInUser, _ accesscontrol.Options) ([]accesscontrol.Permission, error) { timer := prometheus.NewTimer(metrics.MAccessPermissionsSummary) @@ -113,7 +117,8 @@ func (ac *OSSAccessControlService) GetUserPermissions(ctx context.Context, user OrgID: user.OrgID, UserID: user.UserID, Roles: accesscontrol.GetOrgRoles(ac.cfg, user), - Actions: append(TeamAdminActions, append(DashboardAdminActions, FolderAdminActions...)...), + TeamIDs: user.Teams, + Actions: actionsToFetch, }) if err != nil { return nil, err