AccessControl: SQL filters for team search (#44557)

* AccessControl: SQL filters for team search Set test config * Remove userIdFilter when FGAC is on
3 years ago · 01b88adb3a
parent f016e19d1a
commit 01b88adb3a
6 changed files with 190 additions and 6 deletions
--- a/pkg/api/api.go
+++ b/pkg/api/api.go
@ -198,8 +198,8 @@ func (hs *HTTPServer) registerRoutes() {

 		// team without requirement of user to be org admin
 		apiRoute.Group("/teams", func(teamsRoute routing.RouteRegister) {
-			teamsRoute.Get("/:teamId", routing.Wrap(hs.GetTeamByID))
-			teamsRoute.Get("/search", routing.Wrap(hs.SearchTeams))
+			teamsRoute.Get("/:teamId", authorize(reqSignedIn, ac.EvalPermission(ac.ActionTeamsRead, ac.ScopeTeamsID)), routing.Wrap(hs.GetTeamByID))
+			teamsRoute.Get("/search", authorize(reqSignedIn, ac.EvalPermission(ac.ActionTeamsRead)), routing.Wrap(hs.SearchTeams))
 		})

 		// org information available to all users.
--- a/pkg/api/team.go
+++ b/pkg/api/team.go
@ -130,11 +130,17 @@ func (hs *HTTPServer) SearchTeams(c *models.ReqContext) response.Response {
 		page = 1
 	}

+	// Using accesscontrol the filtering is done based on user permissions
+	userIdFilter := models.FilterIgnoreUser
+	if !hs.Features.IsEnabled(featuremgmt.FlagAccesscontrol) {
+		userIdFilter = userFilter(hs.Cfg.EditorsCanAdmin, c)
+	}
+
 	query := models.SearchTeamsQuery{
 		OrgId:        c.OrgId,
 		Query:        c.Query("query"),
 		Name:         c.Query("name"),
-		UserIdFilter: userFilter(hs.Cfg.EditorsCanAdmin, c),
+		UserIdFilter: userIdFilter,
 		Page:         page,
 		Limit:        perPage,
 		SignedInUser: c.SignedInUser,
@ -201,12 +207,18 @@ func (hs *HTTPServer) GetTeamByID(c *models.ReqContext) response.Response {
 		return response.Error(http.StatusBadRequest, "teamId is invalid", err)
 	}

+	// Using accesscontrol the filtering has already been performed at middleware layer
+	userIdFilter := models.FilterIgnoreUser
+	if !hs.Features.IsEnabled(featuremgmt.FlagAccesscontrol) {
+		userIdFilter = userFilter(hs.Cfg.EditorsCanAdmin, c)
+	}
+
 	query := models.GetTeamByIdQuery{
 		OrgId:        c.OrgId,
 		Id:           teamId,
 		SignedInUser: c.SignedInUser,
 		HiddenUsers:  hs.Cfg.HiddenUsers,
-		UserIdFilter: userFilter(hs.Cfg.EditorsCanAdmin, c),
+		UserIdFilter: userIdFilter,
 	}

 	if err := hs.SQLStore.GetTeamById(c.Req.Context(), &query); err != nil {
--- a/pkg/api/team_test.go
+++ b/pkg/api/team_test.go
@ -34,9 +34,12 @@ func (stub *testLogger) Warn(testMessage string, ctx ...interface{}) {
 func TestTeamAPIEndpoint(t *testing.T) {
 	t.Run("Given two teams", func(t *testing.T) {
 		hs := setupSimpleHTTPServer(nil)
-		hs.SQLStore = sqlstore.InitTestDB(t)
-		mock := &mockstore.SQLStoreMock{}
 		hs.Cfg.EditorsCanAdmin = true
+		store := sqlstore.InitTestDB(t)
+		store.Cfg = hs.Cfg
+		hs.SQLStore = store
+		mock := &mockstore.SQLStoreMock{}
+
 		loggedInUserScenario(t, "When calling GET on", "/api/teams/search", "/api/teams/search", func(sc *scenarioContext) {
 			_, err := hs.SQLStore.CreateTeam("team1", "", 1)
 			require.NoError(t, err)
@ -123,6 +126,7 @@ func TestTeamAPIEndpoint(t *testing.T) {
 }

 const (
+	searchTeamsURL          = "/api/teams/search"
 	createTeamURL           = "/api/teams/"
 	detailTeamURL           = "/api/teams/%d"
 	detailTeamPreferenceURL = "/api/teams/%d/preferences"
@ -182,6 +186,79 @@ func TestTeamAPIEndpoint_CreateTeam_FGAC(t *testing.T) {
 	})
 }

+func TestTeamAPIEndpoint_SearchTeams_FGAC(t *testing.T) {
+	sc := setupHTTPServer(t, true, true)
+	sc.db = sqlstore.InitTestDB(t)
+
+	// Seed three teams
+	for i := 1; i <= 3; i++ {
+		_, err := sc.db.CreateTeam(fmt.Sprintf("team%d", i), fmt.Sprintf("team%d@example.org", i), 1)
+		require.NoError(t, err)
+	}
+
+	setInitCtxSignedInViewer(sc.initCtx)
+
+	t.Run("Access control prevents searching for teams with the incorrect permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsDelete, Scope: "teams:id:*"}}, 1)
+		response := callAPI(sc.server, http.MethodGet, searchTeamsURL, http.NoBody, t)
+		assert.Equal(t, http.StatusForbidden, response.Code)
+	})
+
+	t.Run("Access control allows searching for teams with the correct permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsRead, Scope: "teams:id:*"}}, 1)
+		response := callAPI(sc.server, http.MethodGet, searchTeamsURL, http.NoBody, t)
+		assert.Equal(t, http.StatusOK, response.Code)
+
+		res := &models.SearchTeamQueryResult{}
+		err := json.Unmarshal(response.Body.Bytes(), res)
+		require.NoError(t, err)
+		require.Len(t, res.Teams, 3, "expected all teams to have been returned")
+		require.Equal(t, res.TotalCount, int64(3), "expected count to match teams length")
+	})
+
+	t.Run("Access control filters teams based on user permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsRead, Scope: "teams:id:1"}, {Action: accesscontrol.ActionTeamsRead, Scope: "teams:id:3"}}, 1)
+		response := callAPI(sc.server, http.MethodGet, searchTeamsURL, http.NoBody, t)
+		assert.Equal(t, http.StatusOK, response.Code)
+
+		res := &models.SearchTeamQueryResult{}
+		err := json.Unmarshal(response.Body.Bytes(), res)
+		require.NoError(t, err)
+		require.Len(t, res.Teams, 2, "expected a subset of teams to have been returned")
+		require.Equal(t, res.TotalCount, int64(2), "expected count to match teams length")
+		for _, team := range res.Teams {
+			require.NotEqual(t, team.Name, "team2", "expected team2 to have been filtered")
+		}
+	})
+}
+
+func TestTeamAPIEndpoint_GetTeamByID_FGAC(t *testing.T) {
+	sc := setupHTTPServer(t, true, true)
+	sc.db = sqlstore.InitTestDB(t)
+
+	_, err := sc.db.CreateTeam("team1", "team1@example.org", 1)
+	require.NoError(t, err)
+
+	setInitCtxSignedInViewer(sc.initCtx)
+
+	t.Run("Access control prevents getting a team with the incorrect permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsRead, Scope: "teams:id:2"}}, 1)
+		response := callAPI(sc.server, http.MethodGet, fmt.Sprintf(detailTeamURL, 1), http.NoBody, t)
+		assert.Equal(t, http.StatusForbidden, response.Code)
+	})
+
+	t.Run("Access control allows getting a team with the correct permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsRead, Scope: "teams:id:1"}}, 1)
+		response := callAPI(sc.server, http.MethodGet, fmt.Sprintf(detailTeamURL, 1), http.NoBody, t)
+		assert.Equal(t, http.StatusOK, response.Code)
+
+		res := &models.TeamDTO{}
+		err := json.Unmarshal(response.Body.Bytes(), res)
+		require.NoError(t, err)
+		assert.Equal(t, "team1", res.Name)
+	})
+}
+
 // Given a team with a user, when the user is granted X permission,
 // Then the endpoint should return 200 if the user has accesscontrol.ActionTeamsWrite with teams:id:1 scope
 // else return 403
--- a/pkg/services/accesscontrol/filter.go
+++ b/pkg/services/accesscontrol/filter.go
@ -12,6 +12,7 @@ import (
 var sqlIDAcceptList = map[string]struct{}{
 	"org_user.user_id": {},
 	"role.id":          {},
+	"team.id":          {},
 }

 var (
--- a/pkg/services/sqlstore/team.go
+++ b/pkg/services/sqlstore/team.go
@ -10,6 +10,7 @@ import (
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/models"
 	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
 )

 func (ss *SQLStore) addTeamQueryAndCommandHandlers() {
@ -210,6 +211,19 @@ func (ss *SQLStore) SearchTeams(ctx context.Context, query *models.SearchTeamsQu
 		params = append(params, query.Name)
 	}

+	var (
+		acFilter ac.SQLFilter
+		err      error
+	)
+	if ss.Cfg.IsFeatureToggleEnabled(featuremgmt.FlagAccesscontrol) {
+		acFilter, err = ac.Filter(ctx, "team.id", "teams", ac.ActionTeamsRead, query.SignedInUser)
+		if err != nil {
+			return err
+		}
+		sql.WriteString(` and` + acFilter.Where)
+		params = append(params, acFilter.Args...)
+	}
+
 	sql.WriteString(` order by team.name asc`)

 	if query.Limit != 0 {
@ -245,6 +259,11 @@ func (ss *SQLStore) SearchTeams(ctx context.Context, query *models.SearchTeamsQu
 			)`, query.UserIdFilter)
 	}

+	// Only count teams user can see
+	if ss.Cfg.IsFeatureToggleEnabled(featuremgmt.FlagAccesscontrol) {
+		countSess.Where(acFilter.Where, acFilter.Args...)
+	}
+
 	count, err := countSess.Count(&team)
 	query.Result.TotalCount = count

--- a/pkg/services/sqlstore/team_test.go
+++ b/pkg/services/sqlstore/team_test.go
@ -9,6 +9,9 @@ import (
 	"testing"

 	"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"
 )

@ -344,3 +347,75 @@ func TestTeamCommandsAndQueries(t *testing.T) {
 		})
 	})
 }
+
+func TestSQLStore_SearchTeams(t *testing.T) {
+	type searchTeamsTestCase struct {
+		desc             string
+		query            *models.SearchTeamsQuery
+		expectedNumUsers int
+	}
+
+	tests := []searchTeamsTestCase{
+		{
+			desc: "should return all teams",
+			query: &models.SearchTeamsQuery{
+				OrgId: 1,
+				SignedInUser: &models.SignedInUser{
+					OrgId:       1,
+					Permissions: map[int64]map[string][]string{1: {ac.ActionTeamsRead: {ac.ScopeTeamsAll}}},
+				},
+			},
+			expectedNumUsers: 10,
+		},
+		{
+			desc: "should return no teams",
+			query: &models.SearchTeamsQuery{
+				OrgId: 1,
+				SignedInUser: &models.SignedInUser{
+					OrgId:       1,
+					Permissions: map[int64]map[string][]string{1: {ac.ActionTeamsRead: {""}}},
+				},
+			},
+			expectedNumUsers: 0,
+		},
+		{
+			desc: "should return some teams",
+			query: &models.SearchTeamsQuery{
+				OrgId: 1,
+				SignedInUser: &models.SignedInUser{
+					OrgId: 1,
+					Permissions: map[int64]map[string][]string{1: {ac.ActionTeamsRead: {
+						"teams:id:1",
+						"teams:id:5",
+						"teams:id:9",
+					}}},
+				},
+			},
+			expectedNumUsers: 3,
+		},
+	}
+
+	store := InitTestDB(t)
+	store.Cfg.IsFeatureToggleEnabled = featuremgmt.WithFeatures(featuremgmt.FlagAccesscontrol).IsEnabled
+
+	// Seed 10 teams
+	for i := 1; i <= 10; i++ {
+		_, err := store.CreateTeam(fmt.Sprintf("team-%d", i), fmt.Sprintf("team-%d@example.org", i), 1)
+		require.NoError(t, err)
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			err := store.SearchTeams(context.Background(), tt.query)
+			require.NoError(t, err)
+			assert.Len(t, tt.query.Result.Teams, tt.expectedNumUsers)
+			assert.Equal(t, tt.query.Result.TotalCount, int64(tt.expectedNumUsers))
+
+			if !hasWildcardScope(tt.query.SignedInUser, ac.ActionTeamsRead) {
+				for _, team := range tt.query.Result.Teams {
+					assert.Contains(t, tt.query.SignedInUser.Permissions[tt.query.SignedInUser.OrgId][ac.ActionTeamsRead], fmt.Sprintf("teams:id:%d", team.Id))
+				}
+			}
+		})
+	}
+}