public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Chore: Remove legacy AC checks from team (#68715)

Browse Source 
* removing legacy AC checks from team API handlers

* Chore: remove `UserIDFilter` from team queries (#68820)

* remove userIDfilter from team queries in favour of RBAC SQL filtering

* fix typo

* remove redundant tests

* remove another unused function

* fix failing test
This commit is contained in:
Ieva
2023-05-22 18:41:53 +02:00
committed by GitHub
parent e42c3ee55e
commit d54fa569ec
21 changed files with 74 additions and 929 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/team/model.go
-2
View File
@@ -58,7 +58,6 @@ type GetTeamByIDQuery struct {
ID int64
SignedInUser *user.SignedInUser
HiddenUsers map[string]struct{}
UserIdFilter int64
}
// FilterIgnoreUser is used in a get / search teams query when the caller does not want to filter teams by user ID / membership
@@ -76,7 +75,6 @@ type SearchTeamsQuery struct {
Limit int
Page int
OrgID int64 `xorm:"org_id"`
UserIDFilter int64 `xorm:"user_id_filter"`
SignedInUser *user.SignedInUser
HiddenUsers map[string]struct{}
}
pkg/services/team/team.go
+1
View File
@@ -17,6 +17,7 @@ type Service interface {
UpdateTeamMember(ctx context.Context, cmd *UpdateTeamMemberCommand) error
IsTeamMember(orgId int64, teamId int64, userId int64) (bool, error)
RemoveTeamMember(ctx context.Context, cmd *RemoveTeamMemberCommand) error
RemoveUsersMemberships(tx context.Context, userID int64) error
GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*TeamMemberDTO, error)
GetTeamMembers(ctx context.Context, query *GetTeamMembersQuery) ([]*TeamMemberDTO, error)
IsAdminOfTeams(ctx context.Context, query *IsAdminOfTeamsQuery) (bool, error)
pkg/services/team/teamimpl/store.go
+28 -65
View File
@@ -23,6 +23,7 @@ type store interface {
Search(ctx context.Context, query *team.SearchTeamsQuery) (team.SearchTeamQueryResult, error)
GetByID(ctx context.Context, query *team.GetTeamByIDQuery) (*team.TeamDTO, error)
GetByUser(ctx context.Context, query *team.GetTeamsByUserQuery) ([]*team.TeamDTO, error)
RemoveUsersMemberships(ctx context.Context, userID int64) error
AddMember(userID, orgID, teamID int64, isExternal bool, permission dashboards.PermissionType) error
UpdateMember(ctx context.Context, cmd *team.UpdateTeamMemberCommand) error
IsMember(orgId int64, teamId int64, userId int64) (bool, error)
@@ -76,19 +77,6 @@ func getTeamSelectSQLBase(db db.DB, filteredUsers []string) string {
` FROM team as team `
}
func getTeamSelectWithPermissionsSQLBase(db db.DB, filteredUsers []string) string {
return `SELECT
team.id AS id,
team.uid,
team.org_id,
team.name AS name,
team.email AS email,
team_member.permission, ` +
getTeamMemberCount(db, filteredUsers) +
` FROM team AS team
INNER JOIN team_member ON team.id = team_member.team_id AND team_member.user_id = ? `
}
func (ss *xormStore) Create(name, email string, orgID int64) (team.Team, error) {
t := team.Team{
UID: util.GenerateShortUID(),
@@ -207,13 +195,7 @@ func (ss *xormStore) Search(ctx context.Context, query *team.SearchTeamsQuery) (
params = append(params, user)
}
if query.UserIDFilter == team.FilterIgnoreUser {
sql.WriteString(getTeamSelectSQLBase(ss.db, filteredUsers))
} else {
sql.WriteString(getTeamSelectWithPermissionsSQLBase(ss.db, filteredUsers))
params = append(params, query.UserIDFilter)
}
sql.WriteString(getTeamSelectSQLBase(ss.db, filteredUsers))
sql.WriteString(` WHERE team.org_id = ?`)
params = append(params, query.OrgID)
@@ -227,18 +209,12 @@ func (ss *xormStore) Search(ctx context.Context, query *team.SearchTeamsQuery) (
params = append(params, query.Name)
}
var (
acFilter ac.SQLFilter
err error
)
if !ac.IsDisabled(ss.cfg) {
acFilter, err = ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead)
if err != nil {
return err
}
sql.WriteString(` and` + acFilter.Where)
params = append(params, acFilter.Args...)
acFilter, err := ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead)
if err != nil {
return err
}
sql.WriteString(` and` + acFilter.Where)
params = append(params, acFilter.Args...)
sql.WriteString(` order by team.name asc`)
@@ -263,22 +239,8 @@ func (ss *xormStore) Search(ctx context.Context, query *team.SearchTeamsQuery) (
countSess.Where("name=?", query.Name)
}
// If we're not retrieving all results, then only search for teams that this user has access to
if query.UserIDFilter != team.FilterIgnoreUser {
countSess.
Where(`
team.id IN (
SELECT
team_id
FROM team_member
WHERE team_member.user_id = ?
)`, query.UserIDFilter)
}
// Only count teams user can see
if !ac.IsDisabled(ss.cfg) {
countSess.Where(acFilter.Where, acFilter.Args...)
}
countSess.Where(acFilter.Where, acFilter.Args...)
count, err := countSess.Count(&t)
queryResult.TotalCount = count
@@ -303,11 +265,6 @@ func (ss *xormStore) GetByID(ctx context.Context, query *team.GetTeamByIDQuery)
params = append(params, user)
}
if query.UserIdFilter != team.FilterIgnoreUser {
sql.WriteString(` INNER JOIN team_member ON team.id = team_member.team_id AND team_member.user_id = ?`)
params = append(params, query.UserIdFilter)
}
sql.WriteString(` WHERE team.org_id = ? and team.id = ?`)
params = append(params, query.OrgID, query.ID)
@@ -343,16 +300,14 @@ func (ss *xormStore) GetByUser(ctx context.Context, query *team.GetTeamsByUserQu
sql.WriteString(` INNER JOIN team_member on team.id = team_member.team_id`)
sql.WriteString(` WHERE team.org_id = ? and team_member.user_id = ?`)
if !ac.IsDisabled(ss.cfg) {
acFilter, err := ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead)
if err != nil {
return err
}
sql.WriteString(` and` + acFilter.Where)
params = append(params, acFilter.Args...)
acFilter, err := ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead)
if err != nil {
return err
}
sql.WriteString(` and` + acFilter.Where)
params = append(params, acFilter.Args...)
err := sess.SQL(sql.String(), params...).Find(&queryResult)
err = sess.SQL(sql.String(), params...).Find(&queryResult)
return err
})
if err != nil {
@@ -500,6 +455,16 @@ func removeTeamMember(sess *db.Session, cmd *team.RemoveTeamMemberCommand) error
return err
}
// RemoveUsersMemberships removes all the team membership entries for the given user.
// Only used when removing a user from a Grafana instance.
func (ss *xormStore) RemoveUsersMemberships(ctx context.Context, userID int64) error {
return ss.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error {
var rawSQL = "DELETE FROM team_member WHERE user_id = ?"
_, err := sess.Exec(rawSQL, userID)
return err
})
}
// GetUserTeamMemberships return a list of memberships to teams granted to a user
// If external is specified, only memberships provided by an external auth provider will be listed
// This function doesn't perform any accesscontrol filtering.
@@ -521,12 +486,10 @@ func (ss *xormStore) GetMembers(ctx context.Context, query *team.GetTeamMembersQ
// With accesscontrol we filter out users based on the SignedInUser's permissions
// Note we assume that checking SignedInUser is allowed to see team members for this team has already been performed
// If the signed in user is not set no member will be returned
if !ac.IsDisabled(ss.cfg) {
sqlID := fmt.Sprintf("%s.%s", ss.db.GetDialect().Quote("user"), ss.db.GetDialect().Quote("id"))
*acFilter, err = ac.Filter(query.SignedInUser, sqlID, "users:id:", ac.ActionOrgUsersRead)
if err != nil {
return nil, err
}
sqlID := fmt.Sprintf("%s.%s", ss.db.GetDialect().Quote("user"), ss.db.GetDialect().Quote("id"))
*acFilter, err = ac.Filter(query.SignedInUser, sqlID, "users:id:", ac.ActionOrgUsersRead)
if err != nil {
return nil, err
}
return ss.getTeamMembers(ctx, query, acFilter)
pkg/services/team/teamimpl/store_test.go
+8 -15
View File
@@ -369,13 +369,6 @@ func TestIntegrationTeamCommandsAndQueries(t *testing.T) {
team1 := searchQueryResult.Teams[0]
require.EqualValues(t, team1.MemberCount, 2)
searchQueryFilteredByUser := &team.SearchTeamsQuery{OrgID: testOrgID, Page: 1, Limit: 10, UserIDFilter: userIds[0], SignedInUser: signedInUser, HiddenUsers: hiddenUsers}
searchQueryFilteredByUserResult, err := teamSvc.SearchTeams(context.Background(), searchQueryFilteredByUser)
require.NoError(t, err)
require.Equal(t, len(searchQueryFilteredByUserResult.Teams), 1)
team1 = searchQueryResult.Teams[0]
require.EqualValues(t, team1.MemberCount, 2)
getTeamQuery := &team.GetTeamByIDQuery{OrgID: testOrgID, ID: teamId, SignedInUser: signedInUser, HiddenUsers: hiddenUsers}
getTeamQueryResult, err := teamSvc.GetTeamByID(context.Background(), getTeamQuery)
require.NoError(t, err)
@@ -427,9 +420,9 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) {
t.Skip("skipping integration test")
}
type searchTeamsTestCase struct {
desc string
query *team.SearchTeamsQuery
expectedNumUsers int
desc string
query *team.SearchTeamsQuery
expectedTeamCount int
}
tests := []searchTeamsTestCase{
@@ -442,7 +435,7 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) {
Permissions: map[int64]map[string][]string{1: {ac.ActionTeamsRead: {ac.ScopeTeamsAll}}},
},
},
expectedNumUsers: 10,
expectedTeamCount: 10,
},
{
desc: "should return no teams",
@@ -453,7 +446,7 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) {
Permissions: map[int64]map[string][]string{1: {ac.ActionTeamsRead: {""}}},
},
},
expectedNumUsers: 0,
expectedTeamCount: 0,
},
{
desc: "should return some teams",
@@ -468,7 +461,7 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) {
}}},
},
},
expectedNumUsers: 3,
expectedTeamCount: 3,
},
}
@@ -485,8 +478,8 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) {
t.Run(tt.desc, func(t *testing.T) {
queryResult, err := teamSvc.SearchTeams(context.Background(), tt.query)
require.NoError(t, err)
assert.Len(t, queryResult.Teams, tt.expectedNumUsers)
assert.Equal(t, queryResult.TotalCount, int64(tt.expectedNumUsers))
assert.Len(t, queryResult.Teams, tt.expectedTeamCount)
assert.Equal(t, queryResult.TotalCount, int64(tt.expectedTeamCount))
if !hasWildcardScope(tt.query.SignedInUser, ac.ActionTeamsRead) {
for _, team := range queryResult.Teams {
pkg/services/team/teamimpl/team.go
+4
View File
@@ -57,6 +57,10 @@ func (s *Service) RemoveTeamMember(ctx context.Context, cmd *team.RemoveTeamMemb
return s.store.RemoveMember(ctx, cmd)
}
func (s *Service) RemoveUsersMemberships(ctx context.Context, userID int64) error {
return s.store.RemoveUsersMemberships(ctx, userID)
}
func (s *Service) GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*team.TeamMemberDTO, error) {
return s.store.GetMemberships(ctx, orgID, userID, external)
}
pkg/services/team/teamtest/team.go
+4
View File
@@ -61,6 +61,10 @@ func (s *FakeService) RemoveTeamMember(ctx context.Context, cmd *team.RemoveTeam
return s.ExpectedError
}
func (s *FakeService) RemoveUsersMemberships(ctx context.Context, userID int64) error {
return s.ExpectedError
}
func (s *FakeService) GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*team.TeamMemberDTO, error) {
return s.ExpectedMembers, s.ExpectedError
}
pkg/services/teamguardian/database/database.go
-34
View File
@@ -1,34 +0,0 @@
package database
import (
"context"
"github.com/grafana/grafana/pkg/infra/db"
"github.com/grafana/grafana/pkg/services/team"
)
type TeamGuardianStoreImpl struct {
sqlStore db.DB
teamService team.Service
}
func ProvideTeamGuardianStore(sqlStore db.DB, teamService team.Service) *TeamGuardianStoreImpl {
return &TeamGuardianStoreImpl{sqlStore: sqlStore, teamService: teamService}
}
func (t *TeamGuardianStoreImpl) GetTeamMembers(ctx context.Context, query team.GetTeamMembersQuery) ([]*team.TeamMemberDTO, error) {
queryResult, err := t.teamService.GetTeamMembers(ctx, &query)
if err != nil {
return nil, err
}
return queryResult, nil
}
func (t *TeamGuardianStoreImpl) DeleteByUser(ctx context.Context, userID int64) error {
return t.sqlStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error {
var rawSQL = "DELETE FROM team_member WHERE user_id = ?"
_, err := sess.Exec(rawSQL, userID)
return err
})
}
pkg/services/teamguardian/database/database_mock.go
-23
View File
@@ -1,23 +0,0 @@
package database
import (
"context"
"github.com/stretchr/testify/mock"
"github.com/grafana/grafana/pkg/services/team"
)
type TeamGuardianStoreMock struct {
mock.Mock
}
func (t *TeamGuardianStoreMock) GetTeamMembers(ctx context.Context, query team.GetTeamMembersQuery) ([]*team.TeamMemberDTO, error) {
args := t.Called(ctx, query)
return args.Get(0).([]*team.TeamMemberDTO), args.Error(1)
}
func (t *TeamGuardianStoreMock) DeleteByUser(ctx context.Context, userID int64) error {
args := t.Called(ctx, userID)
return args.Get(0).(error)
}
pkg/services/teamguardian/manager/service.go
-53
View File
@@ -1,53 +0,0 @@
package manager
import (
"context"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/team"
"github.com/grafana/grafana/pkg/services/teamguardian"
"github.com/grafana/grafana/pkg/services/user"
)
type Service struct {
store teamguardian.Store
}
func ProvideService(store teamguardian.Store) teamguardian.TeamGuardian {
return &Service{store: store}
}
func (s *Service) CanAdmin(ctx context.Context, orgId int64, teamId int64, user *user.SignedInUser) error {
if user.OrgRole == org.RoleAdmin {
return nil
}
if user.OrgID != orgId {
return team.ErrNotAllowedToUpdateTeamInDifferentOrg
}
cmd := team.GetTeamMembersQuery{
OrgID: orgId,
TeamID: teamId,
UserID: user.UserID,
SignedInUser: user,
}
results, err := s.store.GetTeamMembers(ctx, cmd)
if err != nil {
return err
}
for _, member := range results {
if member.UserID == user.UserID && member.Permission == dashboards.PERMISSION_ADMIN {
return nil
}
}
return team.ErrNotAllowedToUpdateTeam
}
func (s *Service) DeleteByUser(ctx context.Context, userID int64) error {
return s.store.DeleteByUser(ctx, userID)
}
pkg/services/teamguardian/manager/service_mock.go
-27
View File
@@ -1,27 +0,0 @@
package manager
import (
"context"
"github.com/stretchr/testify/mock"
"github.com/grafana/grafana/pkg/services/user"
)
type TeamGuardianMock struct {
mock.Mock
ExpectedError error
}
func NewTeamGuardianMock() *TeamGuardianMock {
return &TeamGuardianMock{}
}
func (t *TeamGuardianMock) CanAdmin(ctx context.Context, orgId int64, teamId int64, user *user.SignedInUser) error {
args := t.Called(ctx, orgId, teamId, user)
return args.Error(0)
}
func (t *TeamGuardianMock) DeleteByUser(ctx context.Context, userID int64) error {
return t.ExpectedError
}
pkg/services/teamguardian/manager/service_test.go
-92
View File
@@ -1,92 +0,0 @@
package manager
import (
"context"
"testing"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/team"
"github.com/grafana/grafana/pkg/services/teamguardian/database"
"github.com/grafana/grafana/pkg/services/user"
)
func TestUpdateTeam(t *testing.T) {
store := new(database.TeamGuardianStoreMock)
teamGuardianService := ProvideService(store)
t.Run("Updating a team", func(t *testing.T) {
admin := user.SignedInUser{
UserID: 1,
OrgID: 1,
OrgRole: org.RoleAdmin,
}
editor := user.SignedInUser{
UserID: 2,
OrgID: 1,
OrgRole: org.RoleEditor,
}
testTeam := team.Team{
ID: 1,
OrgID: 1,
}
t.Run("Given an editor and a team he isn't a member of", func(t *testing.T) {
t.Run("Should not be able to update the team", func(t *testing.T) {
ctx := context.Background()
store.On("GetTeamMembers", ctx, mock.Anything).Return([]*team.TeamMemberDTO{}, nil).Once()
err := teamGuardianService.CanAdmin(ctx, testTeam.OrgID, testTeam.ID, &editor)
require.Equal(t, team.ErrNotAllowedToUpdateTeam, err)
})
})
t.Run("Given an editor and a team he is an admin in", func(t *testing.T) {
t.Run("Should be able to update the team", func(t *testing.T) {
ctx := context.Background()
result := []*team.TeamMemberDTO{{
OrgID: testTeam.OrgID,
TeamID: testTeam.ID,
UserID: editor.UserID,
Permission: dashboards.PERMISSION_ADMIN,
}}
store.On("GetTeamMembers", ctx, mock.Anything).Return(result, nil).Once()
err := teamGuardianService.CanAdmin(ctx, testTeam.OrgID, testTeam.ID, &editor)
require.NoError(t, err)
})
})
t.Run("Given an editor and a team in another org", func(t *testing.T) {
ctx := context.Background()
testTeamOtherOrg := team.Team{
ID: 1,
OrgID: 2,
}
t.Run("Shouldn't be able to update the team", func(t *testing.T) {
result := []*team.TeamMemberDTO{{
OrgID: testTeamOtherOrg.OrgID,
TeamID: testTeamOtherOrg.ID,
UserID: editor.UserID,
Permission: dashboards.PERMISSION_ADMIN,
}}
store.On("GetTeamMembers", ctx, mock.Anything).Return(result, nil).Once()
err := teamGuardianService.CanAdmin(ctx, testTeamOtherOrg.OrgID, testTeamOtherOrg.ID, &editor)
require.Equal(t, team.ErrNotAllowedToUpdateTeamInDifferentOrg, err)
})
})
t.Run("Given an org admin and a team", func(t *testing.T) {
t.Run("Should be able to update the team", func(t *testing.T) {
err := teamGuardianService.CanAdmin(context.Background(), testTeam.OrgID, testTeam.ID, &admin)
require.NoError(t, err)
})
})
})
}
pkg/services/teamguardian/team.go
-18
View File
@@ -1,18 +0,0 @@
package teamguardian
import (
"context"
"github.com/grafana/grafana/pkg/services/team"
"github.com/grafana/grafana/pkg/services/user"
)
type TeamGuardian interface {
CanAdmin(context.Context, int64, int64, *user.SignedInUser) error
DeleteByUser(context.Context, int64) error
}
type Store interface {
GetTeamMembers(context.Context, team.GetTeamMembersQuery) ([]*team.TeamMemberDTO, error)
DeleteByUser(context.Context, int64) error
}