IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-02-25 18:55:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

AccessControl: Implement SQL filters for team members filtering (#44898)

Browse Source 
* AccessControl: Filter team members

* Modify GetTeamMembersByUser comment

* Fix postgres failing test due to quoting

* Rename GetTeamMembersByUser to GetUserTeamMemberships

* Update TeamStore interface
This commit is contained in:
Gabriel MABILLE 2022-02-09 17:46:37 +01:00 committed by GitHub
parent 3fbe4801b7
commit 78fc0258b1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 252 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
pkg/api/team_members.go
View File

@ -24,9 +24,14 @@ func (hs *HTTPServer) GetTeamMembers(c *models.ReqContext) response.Response {
return response.Error(http.StatusBadRequest, "teamId is invalid", err)
}
query := models.GetTeamMembersQuery{OrgId: c.OrgId, TeamId: teamId}
if err := hs.teamGuardian.CanAdmin(c.Req.Context(), query.OrgId, query.TeamId, c.SignedInUser); err != nil {
return response.Error(403, "Not allowed to list team members", err)
query := models.GetTeamMembersQuery{OrgId: c.OrgId, TeamId: teamId, SignedInUser: c.SignedInUser}
// With accesscontrol the permission check has been done at middleware layer
// and the membership filtering will be done at DB layer based on user permissions
if !hs.Features.IsEnabled(featuremgmt.FlagAccesscontrol) {
if err := hs.teamGuardian.CanAdmin(c.Req.Context(), query.OrgId, query.TeamId, c.SignedInUser); err != nil {
return response.Error(403, "Not allowed to list team members", err)
}
}
if err := hs.SQLStore.GetTeamMembers(c.Req.Context(), &query); err != nil {

94
pkg/api/team_members_test.go
View File

@ -9,16 +9,17 @@ import (
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
ac "github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/licensing"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/services/teamguardian/database"
"github.com/grafana/grafana/pkg/services/teamguardian/manager"
"github.com/grafana/grafana/pkg/setting"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
type TeamGuardianMock struct {
@ -49,14 +50,14 @@ func setUpGetTeamMembersHandler(t *testing.T, sqlStore *sqlstore.SQLStore) {
}
func TestTeamMembersAPIEndpoint_userLoggedIn(t *testing.T) {
settings := setting.NewCfg()
hs := setupSimpleHTTPServer(nil)
settings := hs.Cfg
sqlStore := sqlstore.InitTestDB(t)
hs := &HTTPServer{
Cfg: settings,
License: &licensing.OSSLicensingService{},
SQLStore: sqlStore,
teamGuardian: &TeamGuardianMock{},
}
sqlStore.Cfg = settings
hs.SQLStore = sqlStore
hs.License = &licensing.OSSLicensingService{}
hs.teamGuardian = &TeamGuardianMock{}
mock := mockstore.NewSQLStoreMock()
loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "api/teams/1/members",
@ -133,6 +134,7 @@ func setupTeamTestScenario(userCount int, db *sqlstore.SQLStore, t *testing.T) i
}
var (
teamMemberGetRoute = "/api/teams/%s/members"
teamMemberAddRoute = "/api/teams/%s/members"
createTeamMemberCmd = `{"userId": %d}`
teamMemberUpdateRoute = "/api/teams/%s/members/%s"
@ -189,6 +191,60 @@ func TestAddTeamMembersAPIEndpoint_LegacyAccessControl(t *testing.T) {
})
}
func TestGetTeamMembersAPIEndpoint_FGAC(t *testing.T) {
sc := setupHTTPServer(t, true, true)
sc.hs.License = &licensing.OSSLicensingService{}
teamMemberCount := 3
// setupTeamTestScenario sets up 3 user (id: 2,3,4) in the team (id: 1)
testOrgId := setupTeamTestScenario(teamMemberCount, sc.db, t)
setInitCtxSignedInViewer(sc.initCtx)
t.Run("Access control allows getting a team members with the right permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock,
[]*ac.Permission{
{Action: ac.ActionTeamsPermissionsRead, Scope: ac.Scope("teams", "id", "1")},
{Action: ac.ActionOrgUsersRead, Scope: ac.ScopeUsersAll},
},
testOrgId,
)
response := callAPI(sc.server, http.MethodGet, fmt.Sprintf(teamMemberGetRoute, "1"), nil, t)
require.Equal(t, http.StatusOK, response.Code)
res := []*models.TeamMemberDTO{}
err := json.Unmarshal(response.Body.Bytes(), &res)
require.NoError(t, err)
require.Len(t, res, 3, "expected all team members to have been returned")
})
setInitCtxSignedInOrgAdmin(sc.initCtx)
t.Run("Access control filters team members based on user permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock,
[]*ac.Permission{
{Action: ac.ActionTeamsPermissionsRead, Scope: ac.Scope("teams", "id", "1")},
{Action: ac.ActionOrgUsersRead, Scope: ac.Scope("users", "id", "2")},
{Action: ac.ActionOrgUsersRead, Scope: ac.Scope("users", "id", "3")},
},
testOrgId)
response := callAPI(sc.server, http.MethodGet, fmt.Sprintf(teamMemberGetRoute, "1"), nil, t)
require.Equal(t, http.StatusOK, response.Code)
res := []*models.TeamMemberDTO{}
err := json.Unmarshal(response.Body.Bytes(), &res)
require.NoError(t, err)
require.Len(t, res, 2, "expected a subset team members to have been returned")
})
setInitCtxSignedInViewer(sc.initCtx)
t.Run("Access control prevents getting a team member with incorrect scope", func(t *testing.T) {
setAccessControlPermissions(sc.acmock,
[]*ac.Permission{{Action: ac.ActionTeamsPermissionsRead, Scope: ac.Scope("teams", "id", "2")}},
testOrgId)
response := callAPI(sc.server, http.MethodGet, fmt.Sprintf(teamMemberGetRoute, "1"), nil, t)
require.Equal(t, http.StatusForbidden, response.Code)
})
}
func TestAddTeamMembersAPIEndpoint_FGAC(t *testing.T) {
sc := setupHTTPServer(t, true, true)
sc.hs.License = &licensing.OSSLicensingService{}
@ -200,7 +256,7 @@ func TestAddTeamMembersAPIEndpoint_FGAC(t *testing.T) {
newUserId := createUser(sc.db, testOrgId, t)
input := strings.NewReader(fmt.Sprintf(createTeamMemberCmd, newUserId))
t.Run("Access control allows adding a team member with the right permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsWrite, Scope: "teams:id:1"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsWrite, Scope: "teams:id:1"}}, 1)
response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t)
assert.Equal(t, http.StatusOK, response.Code)
})
@ -209,14 +265,14 @@ func TestAddTeamMembersAPIEndpoint_FGAC(t *testing.T) {
newUserId = createUser(sc.db, testOrgId, t)
input = strings.NewReader(fmt.Sprintf(teamCmd, newUserId))
t.Run("Access control prevents from adding a team member with the wrong permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsRead, Scope: "teams:id:1"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsRead, Scope: "teams:id:1"}}, 1)
response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t)
assert.Equal(t, http.StatusForbidden, response.Code)
})
setInitCtxSignedInViewer(sc.initCtx)
t.Run("Access control prevents adding a team member with incorrect scope", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsWrite, Scope: "teams:id:2"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsWrite, Scope: "teams:id:2"}}, 1)
response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t)
assert.Equal(t, http.StatusForbidden, response.Code)
})
@ -279,7 +335,7 @@ func TestUpdateTeamMembersAPIEndpoint_FGAC(t *testing.T) {
setInitCtxSignedInViewer(sc.initCtx)
input := strings.NewReader(fmt.Sprintf(updateTeamMemberCmd, models.PERMISSION_ADMIN))
t.Run("Access control allows updating a team member with the right permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsWrite, Scope: "teams:id:1"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsWrite, Scope: "teams:id:1"}}, 1)
response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t)
assert.Equal(t, http.StatusOK, response.Code)
})
@ -287,14 +343,14 @@ func TestUpdateTeamMembersAPIEndpoint_FGAC(t *testing.T) {
setInitCtxSignedInOrgAdmin(sc.initCtx)
input = strings.NewReader(fmt.Sprintf(updateTeamMemberCmd, models.PERMISSION_ADMIN))
t.Run("Access control prevents updating a team member with the wrong permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsRead, Scope: "teams:id:1"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsRead, Scope: "teams:id:1"}}, 1)
response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t)
assert.Equal(t, http.StatusForbidden, response.Code)
})
setInitCtxSignedInViewer(sc.initCtx)
t.Run("Access control prevents updating a team member with incorrect scope", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsWrite, Scope: "teams:id:2"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsWrite, Scope: "teams:id:2"}}, 1)
response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t)
assert.Equal(t, http.StatusForbidden, response.Code)
})
@ -352,21 +408,21 @@ func TestDeleteTeamMembersAPIEndpoint_FGAC(t *testing.T) {
setInitCtxSignedInViewer(sc.initCtx)
t.Run("Access control allows removing a team member with the right permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsWrite, Scope: "teams:id:1"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsWrite, Scope: "teams:id:1"}}, 1)
response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "2"), nil, t)
assert.Equal(t, http.StatusOK, response.Code)
})
setInitCtxSignedInOrgAdmin(sc.initCtx)
t.Run("Access control prevents removing a team member with the wrong permissions", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsRead, Scope: "teams:id:1"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsRead, Scope: "teams:id:1"}}, 1)
response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "3"), nil, t)
assert.Equal(t, http.StatusForbidden, response.Code)
})
setInitCtxSignedInViewer(sc.initCtx)
t.Run("Access control prevents removing a team member with incorrect scope", func(t *testing.T) {
setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsPermissionsWrite, Scope: "teams:id:2"}}, 1)
setAccessControlPermissions(sc.acmock, []*ac.Permission{{Action: ac.ActionTeamsPermissionsWrite, Scope: "teams:id:2"}}, 1)
response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "3"), nil, t)
assert.Equal(t, http.StatusForbidden, response.Code)
})

11
pkg/models/team_member.go
View File

@ -51,11 +51,12 @@ type RemoveTeamMemberCommand struct {
// QUERIES
type GetTeamMembersQuery struct {
OrgId int64
TeamId int64
UserId int64
External bool
Result []*TeamMemberDTO
OrgId int64
TeamId int64
UserId int64
External bool
SignedInUser *SignedInUser
Result []*TeamMemberDTO
}
// ----------------------

2
pkg/services/accesscontrol/filter.go
View File

@ -13,6 +13,8 @@ var sqlIDAcceptList = map[string]struct{}{
"org_user.user_id": {},
"role.id": {},
"team.id": {},
"\"user\".\"id\"": {}, // For Postgres
"`user`.`id`": {}, // For MySQL and SQLite
}
var (

6
pkg/services/sqlstore/mockstore/mockstore.go
View File

@ -239,7 +239,11 @@ func (m *SQLStoreMock) RemoveTeamMember(ctx context.Context, cmd *models.RemoveT
return m.ExpectedError
}
func (m *SQLStoreMock) GetTeamMembers(ctx context.Context, query *models.GetTeamMembersQuery) error {
func (m SQLStoreMock) GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*models.TeamMemberDTO, error) {
return nil, m.ExpectedError
}
func (m SQLStoreMock) GetTeamMembers(ctx context.Context, query *models.GetTeamMembersQuery) error {
return m.ExpectedError
}

1
pkg/services/sqlstore/store.go
View File

@ -56,6 +56,7 @@ type Store interface {
UpdateTeamMember(ctx context.Context, cmd *models.UpdateTeamMemberCommand) error
IsTeamMember(orgId int64, teamId int64, userId int64) (bool, error)
RemoveTeamMember(ctx context.Context, cmd *models.RemoveTeamMemberCommand) error
GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*models.TeamMemberDTO, error)
GetTeamMembers(ctx context.Context, query *models.GetTeamMembersQuery) error
NewSession(ctx context.Context) *DBSession
WithDbSession(ctx context.Context, callback DBTransactionFunc) error

46
pkg/services/sqlstore/team.go
View File

@ -34,6 +34,7 @@ type TeamStore interface {
UpdateTeamMember(ctx context.Context, cmd *models.UpdateTeamMemberCommand) error
RemoveTeamMember(ctx context.Context, cmd *models.RemoveTeamMemberCommand) error
GetTeamMembers(ctx context.Context, cmd *models.GetTeamMembersQuery) error
GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*models.TeamMemberDTO, error)
AddOrUpdateTeamMember(userID, orgID, teamID int64, isExternal bool, permission models.PermissionType) error
}
@ -492,11 +493,52 @@ func isLastAdmin(sess *DBSession, orgId int64, teamId int64, userId int64) (bool
return false, err
}
// GetTeamMembers return a list of members for the specified team
// 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.
func (ss *SQLStore) GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*models.TeamMemberDTO, error) {
query := &models.GetTeamMembersQuery{
OrgId: orgID,
UserId: userID,
External: external,
Result: []*models.TeamMemberDTO{},
}
err := ss.getTeamMembers(ctx, query, nil)
return query.Result, err
}
// GetTeamMembers return a list of members for the specified team filtered based on the user's permissions
func (ss *SQLStore) GetTeamMembers(ctx context.Context, query *models.GetTeamMembersQuery) error {
acFilter := &ac.SQLFilter{}
var err error
// 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 ss.Cfg.IsFeatureToggleEnabled(featuremgmt.FlagAccesscontrol) {
*acFilter, err = ac.Filter(ctx,
fmt.Sprintf("%s.%s", x.Dialect().Quote("user"), x.Dialect().Quote("id")),
"users", ac.ActionOrgUsersRead, query.SignedInUser,
)
if err != nil {
return err
}
}
return ss.getTeamMembers(ctx, query, acFilter)
}
// getTeamMembers return a list of members for the specified team
func (ss *SQLStore) getTeamMembers(ctx context.Context, query *models.GetTeamMembersQuery, acUserFilter *ac.SQLFilter) error {
query.Result = make([]*models.TeamMemberDTO, 0)
sess := x.Table("team_member")
sess.Join("INNER", x.Dialect().Quote("user"), fmt.Sprintf("team_member.user_id=%s.id", x.Dialect().Quote("user")))
sess.Join("INNER", x.Dialect().Quote("user"),
fmt.Sprintf("team_member.user_id=%s.%s", x.Dialect().Quote("user"), x.Dialect().Quote("id")),
)
if acUserFilter != nil {
sess.Where(acUserFilter.Where, acUserFilter.Args...)
}
// Join with only most recent auth module
authJoinCondition := `(

109
pkg/services/sqlstore/team_test.go
View File

@ -8,11 +8,12 @@ import (
"fmt"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/models"
ac "github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestTeamCommandsAndQueries(t *testing.T) {
@ -419,3 +420,107 @@ func TestSQLStore_SearchTeams(t *testing.T) {
})
}
}
// TestSQLStore_GetTeamMembers_ACFilter tests the accesscontrol filtering of
// team members based on the signed in user permissions
func TestSQLStore_GetTeamMembers_ACFilter(t *testing.T) {
testOrgID := int64(2)
userIds := make([]int64, 4)
// Seed 2 teams with 2 members
setup := func(store *SQLStore) {
team1, errCreateTeam := store.CreateTeam("group1 name", "test1@example.org", testOrgID)
require.NoError(t, errCreateTeam)
team2, errCreateTeam := store.CreateTeam("group2 name", "test2@example.org", testOrgID)
require.NoError(t, errCreateTeam)
for i := 0; i < 4; i++ {
userCmd := models.CreateUserCommand{
Email: fmt.Sprint("user", i, "@example.org"),
Name: fmt.Sprint("user", i),
Login: fmt.Sprint("loginuser", i),
}
user, errCreateUser := store.CreateUser(context.Background(), userCmd)
require.NoError(t, errCreateUser)
userIds[i] = user.Id
}
errAddMember := store.AddTeamMember(userIds[0], testOrgID, team1.Id, false, 0)
require.NoError(t, errAddMember)
errAddMember = store.AddTeamMember(userIds[1], testOrgID, team1.Id, false, 0)
require.NoError(t, errAddMember)
errAddMember = store.AddTeamMember(userIds[2], testOrgID, team2.Id, false, 0)
require.NoError(t, errAddMember)
errAddMember = store.AddTeamMember(userIds[3], testOrgID, team2.Id, false, 0)
require.NoError(t, errAddMember)
}
store := InitTestDB(t)
store.Cfg.IsFeatureToggleEnabled = featuremgmt.WithFeatures(featuremgmt.FlagAccesscontrol).IsEnabled
setup(store)
type getTeamMembersTestCase struct {
desc string
query *models.GetTeamMembersQuery
expectedNumUsers int
}
tests := []getTeamMembersTestCase{
{
desc: "should return all team members",
query: &models.GetTeamMembersQuery{
OrgId: testOrgID,
SignedInUser: &models.SignedInUser{
OrgId: testOrgID,
Permissions: map[int64]map[string][]string{testOrgID: {ac.ActionOrgUsersRead: {ac.ScopeUsersAll}}},
},
},
expectedNumUsers: 4,
},
{
desc: "should return no team members",
query: &models.GetTeamMembersQuery{
OrgId: testOrgID,
SignedInUser: &models.SignedInUser{
OrgId: testOrgID,
Permissions: map[int64]map[string][]string{testOrgID: {ac.ActionOrgUsersRead: {""}}},
},
},
expectedNumUsers: 0,
},
{
desc: "should return some team members",
query: &models.GetTeamMembersQuery{
OrgId: testOrgID,
SignedInUser: &models.SignedInUser{
OrgId: testOrgID,
Permissions: map[int64]map[string][]string{testOrgID: {ac.ActionOrgUsersRead: {
ac.Scope("users", "id", fmt.Sprintf("%d", userIds[0])),
ac.Scope("users", "id", fmt.Sprintf("%d", userIds[2])),
ac.Scope("users", "id", fmt.Sprintf("%d", userIds[3])),
}}},
},
},
expectedNumUsers: 3,
},
}
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
err := store.GetTeamMembers(context.Background(), tt.query)
require.NoError(t, err)
assert.Len(t, tt.query.Result, tt.expectedNumUsers)
if !hasWildcardScope(tt.query.SignedInUser, ac.ActionOrgUsersRead) {
for _, member := range tt.query.Result {
assert.Contains(t,
tt.query.SignedInUser.Permissions[tt.query.SignedInUser.OrgId][ac.ActionOrgUsersRead],
ac.Scope("users", "id", fmt.Sprintf("%d", member.UserId)),
)
}
}
})
}
}

7
pkg/services/teamguardian/manager/service.go
View File

@ -25,9 +25,10 @@ func (s *Service) CanAdmin(ctx context.Context, orgId int64, teamId int64, user
}
cmd := models.GetTeamMembersQuery{
OrgId: orgId,
TeamId: teamId,
UserId: user.UserId,
OrgId: orgId,
TeamId: teamId,
UserId: user.UserId,
SignedInUser: user,
}
results, err := s.store.GetTeamMembers(ctx, cmd)