From 41b709d08d14457e1977526020d589d57f53b641 Mon Sep 17 00:00:00 2001 From: Ieva Date: Mon, 10 Jan 2022 19:05:53 +0200 Subject: [PATCH] Access control: permissions for team creation (#43506) * FGAC for team creation * tests * fix snapshot for UI tests * linting * update snapshots * Remove unecessary class and update tests Co-authored-by: ievaVasiljeva * Make the condition slightly easier Co-authored-by: ievaVasiljeva Co-authored-by: gamab --- pkg/api/api.go | 20 +++--- pkg/api/common_test.go | 19 ++++-- pkg/api/team.go | 5 +- pkg/api/team_test.go | 65 +++++++++++++++++-- pkg/services/accesscontrol/models.go | 3 + pkg/services/accesscontrol/roles.go | 16 +++++ public/app/features/teams/TeamList.test.tsx | 10 ++- public/app/features/teams/TeamList.tsx | 12 ++-- .../__snapshots__/TeamList.test.tsx.snap | 8 +-- public/app/types/accessControl.ts | 2 + 10 files changed, 128 insertions(+), 32 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 8dc71cfa07b..546acbb3ab8 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -178,16 +178,16 @@ func (hs *HTTPServer) registerRoutes() { // team (admin permission required) apiRoute.Group("/teams", func(teamsRoute routing.RouteRegister) { - teamsRoute.Post("/", routing.Wrap(hs.CreateTeam)) - teamsRoute.Put("/:teamId", routing.Wrap(hs.UpdateTeam)) - teamsRoute.Delete("/:teamId", routing.Wrap(hs.DeleteTeamByID)) - teamsRoute.Get("/:teamId/members", routing.Wrap(hs.GetTeamMembers)) - teamsRoute.Post("/:teamId/members", routing.Wrap(hs.AddTeamMember)) - teamsRoute.Put("/:teamId/members/:userId", routing.Wrap(hs.UpdateTeamMember)) - teamsRoute.Delete("/:teamId/members/:userId", routing.Wrap(hs.RemoveTeamMember)) - teamsRoute.Get("/:teamId/preferences", routing.Wrap(hs.GetTeamPreferences)) - teamsRoute.Put("/:teamId/preferences", routing.Wrap(hs.UpdateTeamPreferences)) - }, reqCanAccessTeams) + teamsRoute.Post("/", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsCreate)), routing.Wrap(hs.CreateTeam)) + teamsRoute.Put("/:teamId", reqCanAccessTeams, routing.Wrap(hs.UpdateTeam)) + teamsRoute.Delete("/:teamId", reqCanAccessTeams, routing.Wrap(hs.DeleteTeamByID)) + teamsRoute.Get("/:teamId/members", reqCanAccessTeams, routing.Wrap(hs.GetTeamMembers)) + teamsRoute.Post("/:teamId/members", reqCanAccessTeams, routing.Wrap(hs.AddTeamMember)) + teamsRoute.Put("/:teamId/members/:userId", reqCanAccessTeams, routing.Wrap(hs.UpdateTeamMember)) + teamsRoute.Delete("/:teamId/members/:userId", reqCanAccessTeams, routing.Wrap(hs.RemoveTeamMember)) + teamsRoute.Get("/:teamId/preferences", reqCanAccessTeams, routing.Wrap(hs.GetTeamPreferences)) + teamsRoute.Put("/:teamId/preferences", reqCanAccessTeams, routing.Wrap(hs.UpdateTeamPreferences)) + }) // team without requirement of user to be org admin apiRoute.Group("/teams", func(teamsRoute routing.RouteRegister) { diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 65dff2d974c..eb6f4d6b16b 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -283,17 +283,17 @@ func setInitCtxSignedInViewer(initCtx *models.ReqContext) { initCtx.SignedInUser = &models.SignedInUser{UserId: testUserID, OrgId: 1, OrgRole: models.ROLE_VIEWER, Login: testUserLogin} } +func setInitCtxSignedInEditor(initCtx *models.ReqContext) { + initCtx.IsSignedIn = true + initCtx.SignedInUser = &models.SignedInUser{UserId: testUserID, OrgId: 1, OrgRole: models.ROLE_EDITOR, Login: testUserLogin} +} + func setInitCtxSignedInOrgAdmin(initCtx *models.ReqContext) { initCtx.IsSignedIn = true initCtx.SignedInUser = &models.SignedInUser{UserId: testUserID, OrgId: 1, OrgRole: models.ROLE_ADMIN, Login: testUserLogin} } func setupHTTPServer(t *testing.T, useFakeAccessControl bool, enableAccessControl bool) accessControlScenarioContext { - t.Helper() - - var acmock *accesscontrolmock.Mock - var ac *ossaccesscontrol.OSSAccessControlService - // Use a new conf cfg := setting.NewCfg() cfg.FeatureToggles = make(map[string]bool) @@ -301,6 +301,15 @@ func setupHTTPServer(t *testing.T, useFakeAccessControl bool, enableAccessContro cfg.FeatureToggles["accesscontrol"] = enableAccessControl } + return setupHTTPServerWithCfg(t, useFakeAccessControl, enableAccessControl, cfg) +} + +func setupHTTPServerWithCfg(t *testing.T, useFakeAccessControl, enableAccessControl bool, cfg *setting.Cfg) accessControlScenarioContext { + t.Helper() + + var acmock *accesscontrolmock.Mock + var ac *ossaccesscontrol.OSSAccessControlService + // Use a test DB db := sqlstore.InitTestDB(t) db.Cfg = cfg diff --git a/pkg/api/team.go b/pkg/api/team.go index 373e19c4a4c..907a05c9c1a 100644 --- a/pkg/api/team.go +++ b/pkg/api/team.go @@ -19,7 +19,8 @@ func (hs *HTTPServer) CreateTeam(c *models.ReqContext) response.Response { if err := web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } - if c.OrgRole == models.ROLE_VIEWER { + accessControlEnabled := hs.Cfg.FeatureToggles["accesscontrol"] + if !accessControlEnabled && c.OrgRole == models.ROLE_VIEWER { return response.Error(403, "Not allowed to create team.", nil) } @@ -31,7 +32,7 @@ func (hs *HTTPServer) CreateTeam(c *models.ReqContext) response.Response { return response.Error(500, "Failed to create Team", err) } - if c.OrgRole == models.ROLE_EDITOR && hs.Cfg.EditorsCanAdmin { + if accessControlEnabled || (c.OrgRole == models.ROLE_EDITOR && hs.Cfg.EditorsCanAdmin) { // if the request is authenticated using API tokens // the SignedInUser is an empty struct therefore // an additional check whether it is an actual user is required diff --git a/pkg/api/team_test.go b/pkg/api/team_test.go index 211b01a19ab..1f46eb170ac 100644 --- a/pkg/api/team_test.go +++ b/pkg/api/team_test.go @@ -2,18 +2,19 @@ package api import ( "context" + "fmt" + "net/http" + "strings" "testing" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" - - "net/http" - - "github.com/grafana/grafana/pkg/infra/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -156,3 +157,59 @@ func TestTeamAPIEndpoint(t *testing.T) { }) }) } + +var ( + createTeamURL = "/api/teams/" + createTeamCmd = `{"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)) + 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) + }) + + setInitCtxSignedInEditor(sc.initCtx) + sc.initCtx.IsGrafanaAdmin = true + input = strings.NewReader(fmt.Sprintf(createTeamCmd, 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) + assert.Equal(t, http.StatusForbidden, response.Code) + }) +} + +func TestTeamAPIEndpoint_CreateTeam_LegacyAccessControl_EditorsCanAdmin(t *testing.T) { + cfg := setting.NewCfg() + cfg.EditorsCanAdmin = true + sc := setupHTTPServerWithCfg(t, true, false, cfg) + + setInitCtxSignedInEditor(sc.initCtx) + input := strings.NewReader(fmt.Sprintf(createTeamCmd, 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) + }) +} + +func TestTeamAPIEndpoint_CreateTeam_FGAC(t *testing.T) { + sc := setupHTTPServer(t, true, true) + + setInitCtxSignedInViewer(sc.initCtx) + input := strings.NewReader(fmt.Sprintf(createTeamCmd, 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)) + 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) + }) +} diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index 0ed854b80cf..5c54ffa9db4 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -310,6 +310,9 @@ const ( ActionLicensingUpdate = "licensing:update" ActionLicensingDelete = "licensing:delete" ActionLicensingReportsRead = "licensing.reports:read" + + // Team actions + ActionTeamsCreate = "teams:create" ) const RoleGrafanaAdmin = "Grafana Admin" diff --git a/pkg/services/accesscontrol/roles.go b/pkg/services/accesscontrol/roles.go index 6e1107faf3d..88c214dccaa 100644 --- a/pkg/services/accesscontrol/roles.go +++ b/pkg/services/accesscontrol/roles.go @@ -197,6 +197,19 @@ var ( }, }), } + + teamsWriterRole = RoleDTO{ + Name: teamsWriter, + DisplayName: "Teams writer", + Description: "Create teams.", + Group: "Teams", + Version: 1, + Permissions: []Permission{ + { + Action: ActionTeamsCreate, + }, + }, + } ) // Role names definitions @@ -210,6 +223,7 @@ const ( statsReader = "fixed:stats:reader" usersReader = "fixed:users:reader" usersWriter = "fixed:users:writer" + teamsWriter = "fixed:teams:writer" ) var ( @@ -229,6 +243,7 @@ var ( statsReader: statsReaderRole, usersReader: usersReaderRole, usersWriter: usersWriterRole, + teamsWriter: teamsWriterRole, } // FixedRoleGrants specifies which built-in roles are assigned @@ -247,6 +262,7 @@ var ( string(models.ROLE_ADMIN): { orgUsersReader, orgUsersWriter, + teamsWriter, }, string(models.ROLE_EDITOR): { datasourcesExplorer, diff --git a/public/app/features/teams/TeamList.test.tsx b/public/app/features/teams/TeamList.test.tsx index 3d96a8916f9..4eca8c0d192 100644 --- a/public/app/features/teams/TeamList.test.tsx +++ b/public/app/features/teams/TeamList.test.tsx @@ -3,11 +3,17 @@ import { shallow } from 'enzyme'; import { Props, TeamList } from './TeamList'; import { OrgRole, Team } from '../../types'; import { getMockTeam, getMultipleMockTeams } from './__mocks__/teamMocks'; -import { User } from 'app/core/services/context_srv'; +import { contextSrv, User } from 'app/core/services/context_srv'; import { NavModel } from '@grafana/data'; import { mockToolkitActionCreator } from 'test/core/redux/mocks'; import { setSearchQuery } from './state/reducers'; +jest.mock('app/core/config', () => { + return { + featureToggles: { accesscontrol: false }, + }; +}); + const setup = (propOverrides?: object) => { const props: Props = { navModel: { @@ -34,6 +40,8 @@ const setup = (propOverrides?: object) => { Object.assign(props, propOverrides); + contextSrv.user = props.signedInUser; + const wrapper = shallow(); const instance = wrapper.instance() as TeamList; diff --git a/public/app/features/teams/TeamList.tsx b/public/app/features/teams/TeamList.tsx index 074579a56ed..81bb4442a89 100644 --- a/public/app/features/teams/TeamList.tsx +++ b/public/app/features/teams/TeamList.tsx @@ -3,7 +3,7 @@ import Page from 'app/core/components/Page/Page'; import { DeleteButton, LinkButton, FilterInput } from '@grafana/ui'; import { NavModel } from '@grafana/data'; import EmptyListCTA from 'app/core/components/EmptyListCTA/EmptyListCTA'; -import { OrgRole, StoreState, Team } from 'app/types'; +import { AccessControlAction, StoreState, Team } from 'app/types'; import { deleteTeam, loadTeams } from './state/actions'; import { getSearchQuery, getTeams, getTeamsCount, isPermissionTeamAdmin } from './state/selectors'; import { getNavModel } from 'app/core/selectors/navModel'; @@ -94,10 +94,10 @@ export class TeamList extends PureComponent { } renderTeamList() { - const { teams, searchQuery, editorsCanAdmin, signedInUser } = this.props; - const isCanAdminAndViewer = editorsCanAdmin && signedInUser.orgRole === OrgRole.Viewer; - const disabledClass = isCanAdminAndViewer ? ' disabled' : ''; - const newTeamHref = isCanAdminAndViewer ? '#' : 'org/teams/new'; + const { teams, searchQuery, editorsCanAdmin } = this.props; + const teamAdmin = contextSrv.hasRole('Admin') || (editorsCanAdmin && contextSrv.hasRole('Editor')); + const canCreate = contextSrv.hasAccess(AccessControlAction.ActionTeamsCreate, teamAdmin); + const newTeamHref = canCreate ? 'org/teams/new' : '#'; return ( <> @@ -106,7 +106,7 @@ export class TeamList extends PureComponent { - + New Team diff --git a/public/app/features/teams/__snapshots__/TeamList.test.tsx.snap b/public/app/features/teams/__snapshots__/TeamList.test.tsx.snap index 63d6e2c90dc..f476372b9e6 100644 --- a/public/app/features/teams/__snapshots__/TeamList.test.tsx.snap +++ b/public/app/features/teams/__snapshots__/TeamList.test.tsx.snap @@ -48,8 +48,8 @@ exports[`Render should render teams table 1`] = ` /> New Team @@ -388,7 +388,7 @@ exports[`Render when feature toggle editorsCanAdmin is turned on and signedin us /> New Team @@ -512,7 +512,7 @@ exports[`Render when feature toggle editorsCanAdmin is turned on and signedin us /> New Team diff --git a/public/app/types/accessControl.ts b/public/app/types/accessControl.ts index 04da2f7956f..4aead88c760 100644 --- a/public/app/types/accessControl.ts +++ b/public/app/types/accessControl.ts @@ -48,6 +48,8 @@ export enum AccessControlAction { DataSourcesPermissionsRead = 'datasources.permissions:read', ActionServerStatsRead = 'server.stats:read', + + ActionTeamsCreate = 'teams:create', } export interface Role {