AccessControl: Add access control actions and scopes to team update and delete

* AccessControl: Add access control actions and scopes to team update and delete

* AccessControl: Add tests for AC guards in update/delete

* AccessControl: add fixed role for team writer

* AccessControl: ensure team related AC is deleted with team

* Update pkg/api/team_test.go

pkg/api/api.go

@@ -182,8 +182,8 @@ func (hs *HTTPServer) registerRoutes() {
 		// team (admin permission required)
 		apiRoute.Group("/teams", func(teamsRoute routing.RouteRegister) {
 			teamsRoute.Post("/", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsCreate)), routing.Wrap(hs.CreateTeam))
-			teamsRoute.Put("/:teamId", reqCanAccessTeams, routing.Wrap(hs.UpdateTeam))
+			teamsRoute.Put("/:teamId", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsWrite, ac.ScopeTeamsID)), routing.Wrap(hs.UpdateTeam))
-			teamsRoute.Delete("/:teamId", reqCanAccessTeams, routing.Wrap(hs.DeleteTeamByID))
+			teamsRoute.Delete("/:teamId", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsDelete, ac.ScopeTeamsID)), routing.Wrap(hs.DeleteTeamByID))
 			teamsRoute.Get("/:teamId/members", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsPermissionsRead, ac.ScopeTeamsID)), routing.Wrap(hs.GetTeamMembers))
 			teamsRoute.Post("/:teamId/members", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsPermissionsWrite, ac.ScopeTeamsID)), routing.Wrap(hs.AddTeamMember))
 			teamsRoute.Put("/:teamId/members/:userId", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsPermissionsWrite, ac.ScopeTeamsID)), routing.Wrap(hs.UpdateTeamMember))

pkg/api/roles.go

@@ -198,17 +198,35 @@ func (hs *HTTPServer) declareFixedRoles() error {
 			Group:       "Teams",
 			Version:     1,
 			Permissions: []accesscontrol.Permission{
-				{
+				{Action: accesscontrol.ActionTeamsCreate},
-					Action: accesscontrol.ActionTeamsCreate,
-				},
 			},
 		},
 		Grants: teamCreatorGrants,
 	}
 
+	teamsWriterRole := accesscontrol.RoleRegistration{
+		Role: accesscontrol.RoleDTO{
+			Name:        "fixed:teams:writer",
+			DisplayName: "Team writer",
+			Description: "Create, read, write, or delete a team as well as controlling team memberships.",
+			Group:       "Teams",
+			Version:     1,
+			Permissions: []accesscontrol.Permission{
+				{Action: accesscontrol.ActionTeamsCreate},
+				{Action: accesscontrol.ActionTeamsDelete, Scope: accesscontrol.ScopeTeamsAll},
+				{Action: accesscontrol.ActionTeamsPermissionsRead, Scope: accesscontrol.ScopeTeamsAll},
+				{Action: accesscontrol.ActionTeamsPermissionsWrite, Scope: accesscontrol.ScopeTeamsAll},
+				{Action: accesscontrol.ActionTeamsRead, Scope: accesscontrol.ScopeTeamsAll},
+				{Action: accesscontrol.ActionTeamsWrite, Scope: accesscontrol.ScopeTeamsAll},
+			},
+		},
+		Grants: []string{string(models.ROLE_ADMIN)},
+	}
+
 	return hs.AccessControl.DeclareFixedRoles(
 		provisioningWriterRole, datasourcesReaderRole, datasourcesWriterRole, datasourcesIdReaderRole,
-		datasourcesCompatibilityReaderRole, orgReaderRole, orgWriterRole, orgMaintainerRole, teamsCreatorRole,
+		datasourcesCompatibilityReaderRole, orgReaderRole, orgWriterRole,
+		orgMaintainerRole, teamsCreatorRole, teamsWriterRole,
 	)
 }
 

pkg/api/team.go

@@ -65,8 +65,10 @@ func (hs *HTTPServer) UpdateTeam(c *models.ReqContext) response.Response {
 		return response.Error(http.StatusBadRequest, "teamId is invalid", err)
 	}
 
-	if err := hs.teamGuardian.CanAdmin(c.Req.Context(), cmd.OrgId, cmd.Id, c.SignedInUser); err != nil {
+	if !hs.Features.IsEnabled(featuremgmt.FlagAccesscontrol) {
-		return response.Error(403, "Not allowed to update team", err)
+		if err := hs.teamGuardian.CanAdmin(c.Req.Context(), cmd.OrgId, cmd.Id, c.SignedInUser); err != nil {
+			return response.Error(403, "Not allowed to update team", err)
+		}
 	}
 
 	if err := hs.SQLStore.UpdateTeam(c.Req.Context(), &cmd); err != nil {
@@ -88,8 +90,10 @@ func (hs *HTTPServer) DeleteTeamByID(c *models.ReqContext) response.Response {
 	}
 	user := c.SignedInUser
 
-	if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, user); err != nil {
+	if !hs.Features.IsEnabled(featuremgmt.FlagAccesscontrol) {
-		return response.Error(403, "Not allowed to delete team", err)
+		if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, user); err != nil {
+			return response.Error(403, "Not allowed to delete team", err)
+		}
 	}
 
 	if err := hs.SQLStore.DeleteTeam(c.Req.Context(), &models.DeleteTeamCommand{OrgId: orgId, Id: teamId}); err != nil {

pkg/api/team_members_test.go

@@ -196,7 +196,7 @@ func TestAddTeamMembersAPIEndpoint_FGAC(t *testing.T) {
 
 	setInitCtxSignedInOrgAdmin(sc.initCtx)
 	newUserId = createUser(sc.db, testOrgId, t)
-	input = strings.NewReader(fmt.Sprintf(createTeamCmd, newUserId))
+	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)
 		response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t)

pkg/api/team_test.go

@@ -134,16 +134,17 @@ func TestTeamAPIEndpoint(t *testing.T) {
 	})
 }
 
-var (
+const (
 	createTeamURL = "/api/teams/"
-	createTeamCmd = `{"name": "MyTestTeam%d"}`
+	detailTeamURL = "/api/teams/%d"
+	teamCmd       = `{"name": "MyTestTeam%d"}`
 )
 
 func TestTeamAPIEndpoint_CreateTeam_LegacyAccessControl(t *testing.T) {
 	sc := setupHTTPServer(t, true, false)
 	setInitCtxSignedInOrgAdmin(sc.initCtx)
 
-	input := strings.NewReader(fmt.Sprintf(createTeamCmd, 1))
+	input := strings.NewReader(fmt.Sprintf(teamCmd, 1))
 	t.Run("Organisation admin can create a team", func(t *testing.T) {
 		response := callAPI(sc.server, http.MethodPost, createTeamURL, input, t)
 		assert.Equal(t, http.StatusOK, response.Code)
@@ -151,9 +152,9 @@ func TestTeamAPIEndpoint_CreateTeam_LegacyAccessControl(t *testing.T) {
 
 	setInitCtxSignedInEditor(sc.initCtx)
 	sc.initCtx.IsGrafanaAdmin = true
-	input = strings.NewReader(fmt.Sprintf(createTeamCmd, 2))
+	input = strings.NewReader(fmt.Sprintf(teamCmd, 2))
 	t.Run("Org editor and server admin cannot create a team", func(t *testing.T) {
-		response := callAPI(sc.server, http.MethodPost, createTeamURL, strings.NewReader(createTeamCmd), t)
+		response := callAPI(sc.server, http.MethodPost, createTeamURL, strings.NewReader(teamCmd), t)
 		assert.Equal(t, http.StatusForbidden, response.Code)
 	})
 }
@@ -164,7 +165,7 @@ func TestTeamAPIEndpoint_CreateTeam_LegacyAccessControl_EditorsCanAdmin(t *testi
 	sc := setupHTTPServerWithCfg(t, true, false, cfg)
 
 	setInitCtxSignedInEditor(sc.initCtx)
-	input := strings.NewReader(fmt.Sprintf(createTeamCmd, 1))
+	input := strings.NewReader(fmt.Sprintf(teamCmd, 1))
 	t.Run("Editors can create a team if editorsCanAdmin is set to true", func(t *testing.T) {
 		response := callAPI(sc.server, http.MethodPost, createTeamURL, input, t)
 		assert.Equal(t, http.StatusOK, response.Code)
@@ -175,17 +176,98 @@ func TestTeamAPIEndpoint_CreateTeam_FGAC(t *testing.T) {
 	sc := setupHTTPServer(t, true, true)
 
 	setInitCtxSignedInViewer(sc.initCtx)
-	input := strings.NewReader(fmt.Sprintf(createTeamCmd, 1))
+	input := strings.NewReader(fmt.Sprintf(teamCmd, 1))
 	t.Run("Access control allows creating teams with the correct permissions", func(t *testing.T) {
 		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsCreate}}, 1)
 		response := callAPI(sc.server, http.MethodPost, createTeamURL, input, t)
 		assert.Equal(t, http.StatusOK, response.Code)
 	})
 
-	input = strings.NewReader(fmt.Sprintf(createTeamCmd, 2))
+	input = strings.NewReader(fmt.Sprintf(teamCmd, 2))
 	t.Run("Access control prevents creating teams with the incorrect permissions", func(t *testing.T) {
 		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: "teams:invalid"}}, accesscontrol.GlobalOrgID)
 		response := callAPI(sc.server, http.MethodPost, createTeamURL, input, t)
 		assert.Equal(t, http.StatusForbidden, response.Code)
 	})
 }
+
+// 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
+func TestTeamAPIEndpoint_UpdateTeam_FGAC(t *testing.T) {
+	sc := setupHTTPServer(t, true, true)
+	sc.db = sqlstore.InitTestDB(t)
+	_, err := sc.db.CreateTeam("team1", "", 1)
+
+	require.NoError(t, err)
+
+	setInitCtxSignedInViewer(sc.initCtx)
+
+	input := strings.NewReader(fmt.Sprintf(teamCmd, 1))
+	t.Run("Access control allows updating teams with the correct permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsWrite, Scope: "teams:id:1"}}, 1)
+		response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(detailTeamURL, 1), input, t)
+		assert.Equal(t, http.StatusOK, response.Code)
+
+		teamQuery := &models.GetTeamByIdQuery{OrgId: 1, SignedInUser: sc.initCtx.SignedInUser, Id: 1, Result: &models.TeamDTO{}}
+		err := sc.db.GetTeamById(context.Background(), teamQuery)
+		require.NoError(t, err)
+		assert.Equal(t, "MyTestTeam1", teamQuery.Result.Name)
+	})
+
+	input = strings.NewReader(fmt.Sprintf(teamCmd, 2))
+	t.Run("Access control allows updating teams with the correct global permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsWrite, Scope: "teams:id:*"}}, 1)
+		response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(detailTeamURL, 1), input, t)
+		assert.Equal(t, http.StatusOK, response.Code)
+
+		teamQuery := &models.GetTeamByIdQuery{OrgId: 1, SignedInUser: sc.initCtx.SignedInUser, Id: 1, Result: &models.TeamDTO{}}
+		err := sc.db.GetTeamById(context.Background(), teamQuery)
+		require.NoError(t, err)
+		assert.Equal(t, "MyTestTeam2", teamQuery.Result.Name)
+	})
+
+	input = strings.NewReader(fmt.Sprintf(teamCmd, 3))
+	t.Run("Access control prevents updating teams with the incorrect permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsWrite, Scope: "teams:id:2"}}, 1)
+		response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(detailTeamURL, 1), input, t)
+		assert.Equal(t, http.StatusForbidden, response.Code)
+
+		teamQuery := &models.GetTeamByIdQuery{OrgId: 1, SignedInUser: sc.initCtx.SignedInUser, Id: 1, Result: &models.TeamDTO{}}
+		err := sc.db.GetTeamById(context.Background(), teamQuery)
+		assert.NoError(t, err)
+		assert.Equal(t, "MyTestTeam2", teamQuery.Result.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.ActionTeamsDelete with teams:id:1 scope
+// else return 403
+func TestTeamAPIEndpoint_DeleteTeam_FGAC(t *testing.T) {
+	sc := setupHTTPServer(t, true, true)
+	sc.db = sqlstore.InitTestDB(t)
+	_, err := sc.db.CreateTeam("team1", "", 1)
+	require.NoError(t, err)
+
+	setInitCtxSignedInViewer(sc.initCtx)
+
+	t.Run("Access control prevents deleting teams with the incorrect permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsDelete, Scope: "teams:id:7"}}, 1)
+		response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(detailTeamURL, 1), http.NoBody, t)
+		assert.Equal(t, http.StatusForbidden, response.Code)
+
+		teamQuery := &models.GetTeamByIdQuery{OrgId: 1, SignedInUser: sc.initCtx.SignedInUser, Id: 1, Result: &models.TeamDTO{}}
+		err := sc.db.GetTeamById(context.Background(), teamQuery)
+		require.NoError(t, err)
+	})
+
+	t.Run("Access control allows deleting teams with the correct permissions", func(t *testing.T) {
+		setAccessControlPermissions(sc.acmock, []*accesscontrol.Permission{{Action: accesscontrol.ActionTeamsDelete, Scope: "teams:id:1"}}, 1)
+		response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(detailTeamURL, 1), http.NoBody, t)
+		assert.Equal(t, http.StatusOK, response.Code)
+
+		teamQuery := &models.GetTeamByIdQuery{OrgId: 1, SignedInUser: sc.initCtx.SignedInUser, Id: 1, Result: &models.TeamDTO{}}
+		err := sc.db.GetTeamById(context.Background(), teamQuery)
+		require.ErrorIs(t, err, models.ErrTeamNotFound)
+	})
+}

pkg/services/accesscontrol/models.go

@@ -319,6 +319,9 @@ const (
 	ActionTeamsWrite            = "teams:write"
 	ActionTeamsPermissionsRead  = "teams.permissions:read"
 	ActionTeamsPermissionsWrite = "teams.permissions:write"
+
+	// Team related scopes
+	ScopeTeamsAll = "teams:*"
 )
 
 var (

pkg/services/sqlstore/team.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/models"
+	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
 )
 
 func (ss *SQLStore) addTeamQueryAndCommandHandlers() {
@@ -147,6 +148,7 @@ func (ss *SQLStore) DeleteTeam(ctx context.Context, cmd *models.DeleteTeamComman
 			"DELETE FROM team_member WHERE org_id=? and team_id = ?",
 			"DELETE FROM team WHERE org_id=? and id = ?",
 			"DELETE FROM dashboard_acl WHERE org_id=? and team_id = ?",
+			"DELETE FROM team_role WHERE org_id=? and team_id = ?",
 		}
 
 		for _, sql := range deletes {
@@ -155,7 +157,10 @@ func (ss *SQLStore) DeleteTeam(ctx context.Context, cmd *models.DeleteTeamComman
 				return err
 			}
 		}
-		return nil
+
+		_, err := sess.Exec("DELETE FROM permission WHERE scope=?", ac.Scope("teams", "id", fmt.Sprint(cmd.Id)))
+
+		return err
 	})
 }