diff --git a/pkg/api/api.go b/pkg/api/api.go index 4ae8e13bb13..2f574fdd2b1 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -180,13 +180,13 @@ func (hs *HTTPServer) registerRoutes() { // team (admin permission required) apiRoute.Group("/teams", func(teamsRoute routing.RouteRegister) { - teamsRoute.Post("/", authorize(reqCanAccessTeams, ac.EvalPermission(ActionTeamsCreate)), routing.Wrap(hs.CreateTeam)) + 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/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)) + teamsRoute.Delete("/:teamId/members/:userId", authorize(reqCanAccessTeams, ac.EvalPermission(ac.ActionTeamsPermissionsWrite, ac.ScopeTeamsID)), routing.Wrap(hs.RemoveTeamMember)) teamsRoute.Get("/:teamId/preferences", reqCanAccessTeams, routing.Wrap(hs.GetTeamPreferences)) teamsRoute.Put("/:teamId/preferences", reqCanAccessTeams, routing.Wrap(hs.UpdateTeamPreferences)) }) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 9bccf66e97a..09117c2c183 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -23,9 +23,11 @@ import ( "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/database" acmiddleware "github.com/grafana/grafana/pkg/services/accesscontrol/middleware" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourceservices" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/contexthandler" "github.com/grafana/grafana/pkg/services/quota" @@ -319,13 +321,14 @@ func setupHTTPServerWithCfg(t *testing.T, useFakeAccessControl, enableAccessCont bus := bus.GetBus() + routeRegister := routing.NewRouteRegister() // Create minimal HTTP Server hs := &HTTPServer{ Cfg: cfg, Bus: bus, Live: newTestLive(t), QuotaService: "a.QuotaService{Cfg: cfg}, - RouteRegister: routing.NewRouteRegister(), + RouteRegister: routeRegister, SQLStore: db, searchUsersService: searchusers.ProvideUsersService(bus, filters.ProvideOSSSearchUserFilter()), } @@ -337,6 +340,9 @@ func setupHTTPServerWithCfg(t *testing.T, useFakeAccessControl, enableAccessCont acmock = acmock.WithDisabled() } hs.AccessControl = acmock + teamPermissionService, err := resourceservices.ProvideTeamPermissions(routeRegister, db, acmock, database.ProvideService(db)) + require.NoError(t, err) + hs.TeamPermissionsService = teamPermissionService } else { ac = ossaccesscontrol.ProvideService(cfg, &usagestats.UsageStatsMock{T: t}) hs.AccessControl = ac @@ -345,6 +351,9 @@ func setupHTTPServerWithCfg(t *testing.T, useFakeAccessControl, enableAccessCont require.NoError(t, err) err = ac.RegisterFixedRoles() require.NoError(t, err) + teamPermissionService, err := resourceservices.ProvideTeamPermissions(routeRegister, db, ac, database.ProvideService(db)) + require.NoError(t, err) + hs.TeamPermissionsService = teamPermissionService } // Instantiate a new Server diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index d7dd2162e1b..dc79080df01 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -13,10 +13,6 @@ import ( "strings" "sync" - "github.com/grafana/grafana/pkg/services/query" - "github.com/grafana/grafana/pkg/services/serviceaccounts" - "github.com/grafana/grafana/pkg/services/thumbs" - "github.com/grafana/grafana/pkg/api/routing" httpstatic "github.com/grafana/grafana/pkg/api/static" "github.com/grafana/grafana/pkg/bus" @@ -32,6 +28,8 @@ import ( "github.com/grafana/grafana/pkg/plugins/plugincontext" "github.com/grafana/grafana/pkg/services/accesscontrol" acmiddleware "github.com/grafana/grafana/pkg/services/accesscontrol/middleware" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourceservices" "github.com/grafana/grafana/pkg/services/alerting" "github.com/grafana/grafana/pkg/services/cleanup" "github.com/grafana/grafana/pkg/services/contexthandler" @@ -46,15 +44,18 @@ import ( "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/ngalert" "github.com/grafana/grafana/pkg/services/provisioning" + "github.com/grafana/grafana/pkg/services/query" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/services/schemaloader" "github.com/grafana/grafana/pkg/services/search" "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/services/secrets" + "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/shorturls" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/teamguardian" + "github.com/grafana/grafana/pkg/services/thumbs" "github.com/grafana/grafana/pkg/services/updatechecker" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util/errutil" @@ -118,6 +119,7 @@ type HTTPServer struct { teamGuardian teamguardian.TeamGuardian queryDataService *query.Service serviceAccountsService serviceaccounts.Service + TeamPermissionsService *resourcepermissions.Service } type ServerOptions struct { @@ -142,7 +144,8 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi quotaService *quota.QuotaService, socialService social.Service, tracer tracing.Tracer, encryptionService encryption.Internal, updateChecker *updatechecker.Service, searchUsersService searchusers.Service, dataSourcesService *datasources.Service, secretsService secrets.Service, queryDataService *query.Service, - teamGuardian teamguardian.TeamGuardian, serviceaccountsService serviceaccounts.Service) (*HTTPServer, error) { + teamGuardian teamguardian.TeamGuardian, serviceaccountsService serviceaccounts.Service, + resourcePermissionServices *resourceservices.ResourceServices) (*HTTPServer, error) { web.Env = cfg.Env m := web.New() @@ -196,6 +199,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi teamGuardian: teamGuardian, queryDataService: queryDataService, serviceAccountsService: serviceaccountsService, + TeamPermissionsService: resourcePermissionServices.GetTeamService(), } if hs.Listener != nil { hs.log.Debug("Using provided listener") diff --git a/pkg/api/roles.go b/pkg/api/roles.go index df14fae699c..d56d2ff7202 100644 --- a/pkg/api/roles.go +++ b/pkg/api/roles.go @@ -24,8 +24,6 @@ const ( ActionOrgsQuotasWrite = "orgs.quotas:write" ActionOrgsDelete = "orgs:delete" ActionOrgsCreate = "orgs:create" - - ActionTeamsCreate = "teams:create" ) // API related scopes @@ -188,29 +186,29 @@ func (hs *HTTPServer) declareFixedRoles() error { Grants: []string{string(accesscontrol.RoleGrafanaAdmin)}, } - teamWriterGrants := []string{string(models.ROLE_ADMIN)} + teamCreatorGrants := []string{string(models.ROLE_ADMIN)} if hs.Cfg.EditorsCanAdmin { - teamWriterGrants = append(teamWriterGrants, string(models.ROLE_EDITOR)) + teamCreatorGrants = append(teamCreatorGrants, string(models.ROLE_EDITOR)) } - teamsWriterRole := accesscontrol.RoleRegistration{ + teamsCreatorRole := accesscontrol.RoleRegistration{ Role: accesscontrol.RoleDTO{ - Name: "fixed:teams:writer", - DisplayName: "Team writer", + Name: "fixed:teams:creator", + DisplayName: "Team creator", Description: "Create teams.", Group: "Teams", Version: 1, Permissions: []accesscontrol.Permission{ { - Action: ActionTeamsCreate, + Action: accesscontrol.ActionTeamsCreate, }, }, }, - Grants: teamWriterGrants, + Grants: teamCreatorGrants, } return hs.AccessControl.DeclareFixedRoles( provisioningWriterRole, datasourcesReaderRole, datasourcesWriterRole, datasourcesIdReaderRole, - datasourcesCompatibilityReaderRole, orgReaderRole, orgWriterRole, orgMaintainerRole, teamsWriterRole, + datasourcesCompatibilityReaderRole, orgReaderRole, orgWriterRole, orgMaintainerRole, teamsCreatorRole, ) } diff --git a/pkg/api/team.go b/pkg/api/team.go index de0b4474864..2cc7488b479 100644 --- a/pkg/api/team.go +++ b/pkg/api/team.go @@ -37,8 +37,7 @@ func (hs *HTTPServer) CreateTeam(c *models.ReqContext) response.Response { // the SignedInUser is an empty struct therefore // an additional check whether it is an actual user is required if c.SignedInUser.IsRealUser() { - if err := addTeamMember(hs.SQLStore, c.SignedInUser.UserId, c.OrgId, team.Id, false, - models.PERMISSION_ADMIN); err != nil { + if err := addOrUpdateTeamMember(c.Req.Context(), hs.TeamPermissionsService, c.SignedInUser.UserId, c.OrgId, team.Id, models.PERMISSION_ADMIN.String()); err != nil { c.Logger.Error("Could not add creator to team", "error", err) } } else { diff --git a/pkg/api/team_members.go b/pkg/api/team_members.go index 53acdc8f1eb..2be1eb6ef23 100644 --- a/pkg/api/team_members.go +++ b/pkg/api/team_members.go @@ -1,14 +1,16 @@ package api import ( + "context" "errors" + "fmt" "net/http" "strconv" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) @@ -59,20 +61,22 @@ func (hs *HTTPServer) AddTeamMember(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.TeamId, c.SignedInUser); err != nil { - return response.Error(403, "Not allowed to add team member", err) + if !hs.Cfg.FeatureToggles["accesscontrol"] { + if err := hs.teamGuardian.CanAdmin(c.Req.Context(), cmd.OrgId, cmd.TeamId, c.SignedInUser); err != nil { + return response.Error(403, "Not allowed to add team member", err) + } } - err = addTeamMember(hs.SQLStore, cmd.UserId, cmd.OrgId, cmd.TeamId, cmd.External, cmd.Permission) + isTeamMember, err := hs.SQLStore.IsTeamMember(c.OrgId, cmd.TeamId, cmd.UserId) if err != nil { - if errors.Is(err, models.ErrTeamNotFound) { - return response.Error(404, "Team not found", nil) - } - - if errors.Is(err, models.ErrTeamMemberAlreadyAdded) { - return response.Error(400, "User is already added to this team", nil) - } + return response.Error(500, "Failed to add team member.", err) + } + if isTeamMember { + return response.Error(400, "User is already added to this team", nil) + } + err = addOrUpdateTeamMember(c.Req.Context(), hs.TeamPermissionsService, cmd.UserId, cmd.OrgId, cmd.TeamId, getPermissionName(cmd.Permission)) + if err != nil { return response.Error(500, "Failed to add Member to Team", err) } @@ -91,32 +95,43 @@ func (hs *HTTPServer) UpdateTeamMember(c *models.ReqContext) response.Response { if err != nil { return response.Error(http.StatusBadRequest, "teamId is invalid", err) } - orgId := c.OrgId - - if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, c.SignedInUser); err != nil { - return response.Error(403, "Not allowed to update team member", err) - } - - if c.OrgRole != models.ROLE_ADMIN { - cmd.ProtectLastAdmin = true - } - - cmd.TeamId = teamId - cmd.UserId, err = strconv.ParseInt(web.Params(c.Req)[":userId"], 10, 64) + userId, err := strconv.ParseInt(web.Params(c.Req)[":userId"], 10, 64) if err != nil { return response.Error(http.StatusBadRequest, "userId is invalid", err) } - cmd.OrgId = orgId + orgId := c.OrgId - if err := hs.SQLStore.UpdateTeamMember(c.Req.Context(), &cmd); err != nil { - if errors.Is(err, models.ErrTeamMemberNotFound) { - return response.Error(404, "Team member not found.", nil) + if !hs.Cfg.FeatureToggles["accesscontrol"] { + if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, c.SignedInUser); err != nil { + return response.Error(403, "Not allowed to update team member", err) } + } + + isTeamMember, err := hs.SQLStore.IsTeamMember(orgId, teamId, userId) + if err != nil { + return response.Error(500, "Failed to update team member.", err) + } + if !isTeamMember { + return response.Error(404, "Team member not found.", nil) + } + + err = addOrUpdateTeamMember(c.Req.Context(), hs.TeamPermissionsService, userId, orgId, teamId, getPermissionName(cmd.Permission)) + if err != nil { return response.Error(500, "Failed to update team member.", err) } return response.Success("Team member updated") } +func getPermissionName(permission models.PermissionType) string { + permissionName := permission.String() + // Team member permission is 0, which maps to an empty string. + // However, we want the team permission service to display "Member" for team members. This is a hack to make it work. + if permissionName == "" { + permissionName = "Member" + } + return permissionName +} + // DELETE /api/teams/:teamId/members/:userId func (hs *HTTPServer) RemoveTeamMember(c *models.ReqContext) response.Response { orgId := c.OrgId @@ -129,16 +144,14 @@ func (hs *HTTPServer) RemoveTeamMember(c *models.ReqContext) response.Response { return response.Error(http.StatusBadRequest, "userId is invalid", err) } - if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, c.SignedInUser); err != nil { - return response.Error(403, "Not allowed to remove team member", err) + if !hs.Cfg.FeatureToggles["accesscontrol"] { + if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, c.SignedInUser); err != nil { + return response.Error(403, "Not allowed to remove team member", err) + } } - protectLastAdmin := false - if c.OrgRole != models.ROLE_ADMIN { - protectLastAdmin = true - } - - if err := hs.SQLStore.RemoveTeamMember(c.Req.Context(), &models.RemoveTeamMemberCommand{OrgId: orgId, TeamId: teamId, UserId: userId, ProtectLastAdmin: protectLastAdmin}); err != nil { + teamIDString := strconv.FormatInt(teamId, 10) + if _, err := hs.TeamPermissionsService.SetUserPermission(c.Req.Context(), orgId, userId, teamIDString, ""); err != nil { if errors.Is(err, models.ErrTeamNotFound) { return response.Error(404, "Team not found", nil) } @@ -152,10 +165,13 @@ func (hs *HTTPServer) RemoveTeamMember(c *models.ReqContext) response.Response { return response.Success("Team Member removed") } -// addTeamMember adds a team member. +// addOrUpdateTeamMember adds or updates a team member. // // Stubbable by tests. -var addTeamMember = func(sqlStore *sqlstore.SQLStore, userID, orgID, teamID int64, isExternal bool, - permission models.PermissionType) error { - return sqlStore.AddTeamMember(userID, orgID, teamID, isExternal, permission) +var addOrUpdateTeamMember = func(ctx context.Context, resourcePermissionService *resourcepermissions.Service, userID, orgID, teamID int64, permission string) error { + teamIDString := strconv.FormatInt(teamID, 10) + if _, err := resourcePermissionService.SetUserPermission(ctx, orgID, userID, teamIDString, permission); err != nil { + return fmt.Errorf("failed setting permissions for user %d in team %d: %w", userID, teamID, err) + } + return nil } diff --git a/pkg/api/team_members_test.go b/pkg/api/team_members_test.go index 69ecb7fad0d..446fb9dd420 100644 --- a/pkg/api/team_members_test.go +++ b/pkg/api/team_members_test.go @@ -4,12 +4,17 @@ import ( "context" "encoding/json" "fmt" + "math/rand" "net/http" + "strings" "testing" "github.com/grafana/grafana/pkg/models" + "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/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" @@ -84,3 +89,274 @@ func TestTeamMembersAPIEndpoint_userLoggedIn(t *testing.T) { }) }) } + +func createUser(db *sqlstore.SQLStore, orgId int64, t *testing.T) int64 { + user, err := db.CreateUser(context.Background(), models.CreateUserCommand{ + Login: fmt.Sprintf("TestUser%d", rand.Int()), + OrgId: orgId, + Password: "password", + }) + require.NoError(t, err) + + return user.Id +} + +func setupTeamTestScenario(userCount int, db *sqlstore.SQLStore, t *testing.T) int64 { + user, err := db.CreateUser(context.Background(), models.CreateUserCommand{SkipOrgSetup: true, Login: testUserLogin}) + require.NoError(t, err) + testOrg, err := db.CreateOrgWithMember("TestOrg", user.Id) + require.NoError(t, err) + + team, err := db.CreateTeam("test", "test@test.com", testOrg.Id) + require.NoError(t, err) + + for i := 0; i < userCount; i++ { + userId := createUser(db, testOrg.Id, t) + require.NoError(t, err) + + err = db.AddTeamMember(userId, testOrg.Id, team.Id, false, 0) + require.NoError(t, err) + } + + return testOrg.Id +} + +var ( + teamMemberAddRoute = "/api/teams/%s/members" + createTeamMemberCmd = `{"userId": %d}` + teamMemberUpdateRoute = "/api/teams/%s/members/%s" + updateTeamMemberCmd = `{"permission": %d}` + teamMemberDeleteRoute = "/api/teams/%s/members/%s" +) + +func TestAddTeamMembersAPIEndpoint_LegacyAccessControl(t *testing.T) { + cfg := setting.NewCfg() + cfg.EditorsCanAdmin = true + sc := setupHTTPServerWithCfg(t, true, false, cfg) + guardian := manager.ProvideService(database.ProvideTeamGuardianStore(sc.db)) + sc.hs.teamGuardian = guardian + + teamMemberCount := 3 + testOrgId := setupTeamTestScenario(teamMemberCount, sc.db, t) + + setInitCtxSignedInOrgAdmin(sc.initCtx) + newUserId := createUser(sc.db, testOrgId, t) + input := strings.NewReader(fmt.Sprintf(createTeamMemberCmd, newUserId)) + t.Run("Organisation admins can add a team member", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t) + assert.Equal(t, http.StatusOK, response.Code) + }) + + setInitCtxSignedInEditor(sc.initCtx) + sc.initCtx.IsGrafanaAdmin = true + newUserId = createUser(sc.db, testOrgId, t) + input = strings.NewReader(fmt.Sprintf(createTeamMemberCmd, newUserId)) + t.Run("Editors cannot add team members", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) + + err := sc.db.AddTeamMember(sc.initCtx.UserId, 1, 1, false, 0) + require.NoError(t, err) + input = strings.NewReader(fmt.Sprintf(createTeamMemberCmd, newUserId)) + t.Run("Team members cannot add team members", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) + + err = sc.db.UpdateTeamMember(context.Background(), &models.UpdateTeamMemberCommand{ + UserId: sc.initCtx.UserId, + OrgId: 1, + TeamId: 1, + Permission: models.PERMISSION_ADMIN, + }) + require.NoError(t, err) + input = strings.NewReader(fmt.Sprintf(createTeamMemberCmd, newUserId)) + t.Run("Team admins can add a team member", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t) + assert.Equal(t, http.StatusOK, response.Code) + }) +} + +func TestAddTeamMembersAPIEndpoint_FGAC(t *testing.T) { + sc := setupHTTPServer(t, true, true) + sc.hs.License = &licensing.OSSLicensingService{} + + teamMemberCount := 3 + testOrgId := setupTeamTestScenario(teamMemberCount, sc.db, t) + + setInitCtxSignedInViewer(sc.initCtx) + 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) + response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t) + assert.Equal(t, http.StatusOK, response.Code) + }) + + setInitCtxSignedInOrgAdmin(sc.initCtx) + newUserId = createUser(sc.db, testOrgId, t) + input = strings.NewReader(fmt.Sprintf(createTeamCmd, 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) + 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) + response := callAPI(sc.server, http.MethodPost, fmt.Sprintf(teamMemberAddRoute, "1"), input, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) +} + +func TestUpdateTeamMembersAPIEndpoint_LegacyAccessControl(t *testing.T) { + cfg := setting.NewCfg() + cfg.EditorsCanAdmin = true + sc := setupHTTPServerWithCfg(t, true, false, cfg) + guardian := manager.ProvideService(database.ProvideTeamGuardianStore(sc.db)) + sc.hs.teamGuardian = guardian + + teamMemberCount := 3 + setupTeamTestScenario(teamMemberCount, sc.db, t) + + setInitCtxSignedInOrgAdmin(sc.initCtx) + input := strings.NewReader(fmt.Sprintf(updateTeamMemberCmd, models.PERMISSION_ADMIN)) + t.Run("Organisation admins can update a team member", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t) + assert.Equal(t, http.StatusOK, response.Code) + }) + + setInitCtxSignedInEditor(sc.initCtx) + sc.initCtx.IsGrafanaAdmin = true + input = strings.NewReader(fmt.Sprintf(updateTeamMemberCmd, 0)) + t.Run("Editors cannot update team members", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) + + err := sc.db.AddTeamMember(sc.initCtx.UserId, 1, 1, false, 0) + require.NoError(t, err) + input = strings.NewReader(fmt.Sprintf(updateTeamMemberCmd, 0)) + t.Run("Team members cannot update team members", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) + + err = sc.db.UpdateTeamMember(context.Background(), &models.UpdateTeamMemberCommand{ + UserId: sc.initCtx.UserId, + OrgId: 1, + TeamId: 1, + Permission: models.PERMISSION_ADMIN, + }) + require.NoError(t, err) + input = strings.NewReader(fmt.Sprintf(updateTeamMemberCmd, 0)) + t.Run("Team admins can update a team member", func(t *testing.T) { + response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t) + assert.Equal(t, http.StatusOK, response.Code) + }) +} + +func TestUpdateTeamMembersAPIEndpoint_FGAC(t *testing.T) { + sc := setupHTTPServer(t, true, true) + sc.hs.License = &licensing.OSSLicensingService{} + + teamMemberCount := 3 + setupTeamTestScenario(teamMemberCount, sc.db, 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) + response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t) + assert.Equal(t, http.StatusOK, response.Code) + }) + + 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) + 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) + response := callAPI(sc.server, http.MethodPut, fmt.Sprintf(teamMemberUpdateRoute, "1", "2"), input, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) +} + +func TestDeleteTeamMembersAPIEndpoint_LegacyAccessControl(t *testing.T) { + cfg := setting.NewCfg() + cfg.EditorsCanAdmin = true + sc := setupHTTPServerWithCfg(t, true, false, cfg) + guardian := manager.ProvideService(database.ProvideTeamGuardianStore(sc.db)) + sc.hs.teamGuardian = guardian + + teamMemberCount := 3 + setupTeamTestScenario(teamMemberCount, sc.db, t) + + setInitCtxSignedInOrgAdmin(sc.initCtx) + t.Run("Organisation admins can remove a team member", func(t *testing.T) { + response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "2"), nil, t) + assert.Equal(t, http.StatusOK, response.Code) + }) + + setInitCtxSignedInEditor(sc.initCtx) + sc.initCtx.IsGrafanaAdmin = true + t.Run("Editors cannot remove team members", func(t *testing.T) { + response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "3"), nil, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) + + err := sc.db.AddTeamMember(sc.initCtx.UserId, 1, 1, false, 0) + require.NoError(t, err) + t.Run("Team members cannot remove team members", func(t *testing.T) { + response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "3"), nil, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) + + err = sc.db.UpdateTeamMember(context.Background(), &models.UpdateTeamMemberCommand{ + UserId: sc.initCtx.UserId, + OrgId: 1, + TeamId: 1, + Permission: models.PERMISSION_ADMIN, + }) + require.NoError(t, err) + t.Run("Team admins can remove a team member", func(t *testing.T) { + response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "3"), nil, t) + assert.Equal(t, http.StatusOK, response.Code) + }) +} + +func TestDeleteTeamMembersAPIEndpoint_FGAC(t *testing.T) { + sc := setupHTTPServer(t, true, true) + sc.hs.License = &licensing.OSSLicensingService{} + + teamMemberCount := 3 + setupTeamTestScenario(teamMemberCount, sc.db, 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) + 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) + 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) + response := callAPI(sc.server, http.MethodDelete, fmt.Sprintf(teamMemberDeleteRoute, "1", "3"), nil, t) + assert.Equal(t, http.StatusForbidden, response.Code) + }) +} diff --git a/pkg/api/team_test.go b/pkg/api/team_test.go index 94b72fe79d2..97dd083e59d 100644 --- a/pkg/api/team_test.go +++ b/pkg/api/team_test.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/json" "fmt" "net/http" @@ -10,6 +11,7 @@ import ( "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/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" @@ -79,11 +81,11 @@ func TestTeamAPIEndpoint(t *testing.T) { teamName := "team foo" // TODO: Use a fake SQLStore when it's represented by an interface - origCreateTeam := createTeam - origAddTeamMember := addTeamMember + orgCreateTeam := createTeam + orgAddTeamMember := addOrUpdateTeamMember t.Cleanup(func() { - createTeam = origCreateTeam - addTeamMember = origAddTeamMember + createTeam = orgCreateTeam + addOrUpdateTeamMember = orgAddTeamMember }) createTeamCalled := 0 @@ -93,8 +95,8 @@ func TestTeamAPIEndpoint(t *testing.T) { } addTeamMemberCalled := 0 - addTeamMember = func(sqlStore *sqlstore.SQLStore, userID, orgID, teamID int64, isExternal bool, - permission models.PermissionType) error { + addOrUpdateTeamMember = func(ctx context.Context, resourcePermissionService *resourcepermissions.Service, userID, orgID, teamID int64, + permission string) error { addTeamMemberCalled++ return nil } @@ -179,7 +181,7 @@ func TestTeamAPIEndpoint_CreateTeam_FGAC(t *testing.T) { 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: ActionTeamsCreate}}, 1) + 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) }) diff --git a/pkg/models/team_member.go b/pkg/models/team_member.go index 1ad8dd97e84..8b7078c5168 100644 --- a/pkg/models/team_member.go +++ b/pkg/models/team_member.go @@ -35,18 +35,16 @@ type AddTeamMemberCommand struct { } type UpdateTeamMemberCommand struct { - UserId int64 `json:"-"` - OrgId int64 `json:"-"` - TeamId int64 `json:"-"` - Permission PermissionType `json:"permission"` - ProtectLastAdmin bool `json:"-"` + UserId int64 `json:"-"` + OrgId int64 `json:"-"` + TeamId int64 `json:"-"` + Permission PermissionType `json:"permission"` } type RemoveTeamMemberCommand struct { - OrgId int64 `json:"-"` - UserId int64 - TeamId int64 - ProtectLastAdmin bool `json:"-"` + OrgId int64 `json:"-"` + UserId int64 + TeamId int64 } // ---------------------- diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 986e78fb74b..9921a2ce787 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -28,6 +28,7 @@ import ( "github.com/grafana/grafana/pkg/plugins/manager/loader" "github.com/grafana/grafana/pkg/plugins/plugincontext" "github.com/grafana/grafana/pkg/plugins/plugindashboards" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourceservices" "github.com/grafana/grafana/pkg/services/alerting" "github.com/grafana/grafana/pkg/services/auth/jwt" "github.com/grafana/grafana/pkg/services/cleanup" @@ -182,6 +183,7 @@ var wireBasicSet = wire.NewSet( wire.Bind(new(teamguardian.Store), new(*teamguardianDatabase.TeamGuardianStoreImpl)), teamguardianManager.ProvideService, wire.Bind(new(teamguardian.TeamGuardian), new(*teamguardianManager.Service)), + resourceservices.ProvideResourceServices, ) var wireSet = wire.NewSet( diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index 862711f0bb6..b5f2148139e 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -312,8 +312,18 @@ const ( ActionLicensingDelete = "licensing:delete" ActionLicensingReportsRead = "licensing.reports:read" - // Team actions - ActionTeamsCreate = "teams:create" + // Team related actions + ActionTeamsCreate = "teams:create" + ActionTeamsDelete = "teams:delete" + ActionTeamsRead = "teams:read" + ActionTeamsWrite = "teams:write" + ActionTeamsPermissionsRead = "teams.permissions:read" + ActionTeamsPermissionsWrite = "teams.permissions:write" +) + +var ( + // Team scope + ScopeTeamsID = Scope("teams", "id", Parameter(":teamId")) ) const RoleGrafanaAdmin = "Grafana Admin" diff --git a/pkg/services/accesscontrol/resourceservices/resource_services.go b/pkg/services/accesscontrol/resourceservices/resource_services.go new file mode 100644 index 00000000000..077e6cd93a5 --- /dev/null +++ b/pkg/services/accesscontrol/resourceservices/resource_services.go @@ -0,0 +1,103 @@ +package resourceservices + +import ( + "context" + "fmt" + "strconv" + + "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" + "github.com/grafana/grafana/pkg/services/sqlstore" +) + +func ProvideResourceServices(router routing.RouteRegister, sql *sqlstore.SQLStore, ac accesscontrol.AccessControl, store resourcepermissions.Store) (*ResourceServices, error) { + teamPermissions, err := ProvideTeamPermissions(router, sql, ac, store) + if err != nil { + return nil, err + } + + return &ResourceServices{services: map[string]*resourcepermissions.Service{ + "teams": teamPermissions, + }}, nil +} + +type ResourceServices struct { + services map[string]*resourcepermissions.Service +} + +func (s *ResourceServices) GetTeamService() *resourcepermissions.Service { + return s.services["teams"] +} + +var ( + TeamMemberActions = []string{ + accesscontrol.ActionTeamsRead, + } + + TeamAdminActions = []string{ + accesscontrol.ActionTeamsRead, + accesscontrol.ActionTeamsDelete, + accesscontrol.ActionTeamsWrite, + accesscontrol.ActionTeamsPermissionsRead, + accesscontrol.ActionTeamsPermissionsWrite, + } +) + +func ProvideTeamPermissions(router routing.RouteRegister, sql *sqlstore.SQLStore, ac accesscontrol.AccessControl, store resourcepermissions.Store) (*resourcepermissions.Service, error) { + options := resourcepermissions.Options{ + Resource: "teams", + OnlyManaged: true, + ResourceValidator: func(ctx context.Context, orgID int64, resourceID string) error { + id, err := strconv.ParseInt(resourceID, 10, 64) + if err != nil { + return err + } + + err = sql.GetTeamById(context.Background(), &models.GetTeamByIdQuery{ + OrgId: orgID, + Id: id, + }) + if err != nil { + return err + } + + return nil + }, + Assignments: resourcepermissions.Assignments{ + Users: true, + Teams: false, + BuiltInRoles: false, + }, + PermissionsToActions: map[string][]string{ + "Member": TeamMemberActions, + "Admin": TeamAdminActions, + }, + ReaderRoleName: "Team permission reader", + WriterRoleName: "Team permission writer", + RoleGroup: "Teams", + OnSetUser: func(session *sqlstore.DBSession, orgID, userID int64, resourceID, permission string) error { + teamId, err := strconv.ParseInt(resourceID, 10, 64) + if err != nil { + return err + } + switch permission { + case "Member": + return sqlstore.AddOrUpdateTeamMemberHook(session, userID, orgID, teamId, false, 0) + case "Admin": + return sqlstore.AddOrUpdateTeamMemberHook(session, userID, orgID, teamId, false, models.PERMISSION_ADMIN) + case "": + return sqlstore.RemoveTeamMemberHook(session, &models.RemoveTeamMemberCommand{ + OrgId: orgID, + UserId: userID, + TeamId: teamId, + }) + default: + return fmt.Errorf("invalid team permission type %s", permission) + } + }, + } + + return resourcepermissions.New(options, router, ac, store, sql) +} diff --git a/pkg/services/sqlstore/team.go b/pkg/services/sqlstore/team.go index a1fd714d8fd..284b2012233 100644 --- a/pkg/services/sqlstore/team.go +++ b/pkg/services/sqlstore/team.go @@ -32,6 +32,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 + AddOrUpdateTeamMember(userID, orgID, teamID int64, isExternal bool, permission models.PermissionType) error } func getFilteredUsers(signedInUser *models.SignedInUser, hiddenUsers map[string]struct{}) []string { @@ -292,29 +293,13 @@ func (ss *SQLStore) GetTeamsByUser(ctx context.Context, query *models.GetTeamsBy // AddTeamMember adds a user to a team func (ss *SQLStore) AddTeamMember(userID, orgID, teamID int64, isExternal bool, permission models.PermissionType) error { return ss.WithTransactionalDbSession(context.Background(), func(sess *DBSession) error { - if res, err := sess.Query("SELECT 1 from team_member WHERE org_id=? and team_id=? and user_id=?", - orgID, teamID, userID); err != nil { + if isMember, err := isTeamMember(sess, orgID, teamID, userID); err != nil { return err - } else if len(res) == 1 { + } else if isMember { return models.ErrTeamMemberAlreadyAdded } - if _, err := teamExists(orgID, teamID, sess); err != nil { - return err - } - - entity := models.TeamMember{ - OrgId: orgID, - TeamId: teamID, - UserId: userID, - External: isExternal, - Created: time.Now(), - Updated: time.Now(), - Permission: permission, - } - - _, err := sess.Insert(&entity) - return err + return addTeamMember(sess, orgID, teamID, userID, isExternal, permission) }) } @@ -335,58 +320,126 @@ func getTeamMember(sess *DBSession, orgId int64, teamId int64, userId int64) (mo // UpdateTeamMember updates a team member func (ss *SQLStore) UpdateTeamMember(ctx context.Context, cmd *models.UpdateTeamMemberCommand) error { - return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - member, err := getTeamMember(sess, cmd.OrgId, cmd.TeamId, cmd.UserId) + return inTransaction(func(sess *DBSession) error { + return updateTeamMember(sess, cmd.OrgId, cmd.TeamId, cmd.UserId, cmd.Permission) + }) +} + +func (ss *SQLStore) IsTeamMember(orgId int64, teamId int64, userId int64) (bool, error) { + var isMember bool + + err := ss.WithTransactionalDbSession(context.Background(), func(sess *DBSession) error { + var err error + isMember, err = isTeamMember(sess, orgId, teamId, userId) + return err + }) + + return isMember, err +} + +func isTeamMember(sess *DBSession, orgId int64, teamId int64, userId int64) (bool, error) { + if res, err := sess.Query("SELECT 1 FROM team_member WHERE org_id=? and team_id=? and user_id=?", orgId, teamId, userId); err != nil { + return false, err + } else if len(res) != 1 { + return false, nil + } + + return true, nil +} + +// AddOrUpdateTeamMemberHook is called from team resource permission service +// it adds user to a team or updates user permissions in a team within the given transaction session +func AddOrUpdateTeamMemberHook(sess *DBSession, userID, orgID, teamID int64, isExternal bool, permission models.PermissionType) error { + isMember, err := isTeamMember(sess, orgID, teamID, userID) + if err != nil { + return err + } + + if isMember { + err = updateTeamMember(sess, orgID, teamID, userID, permission) + } else { + err = addTeamMember(sess, orgID, teamID, userID, isExternal, permission) + } + + return err +} + +func addTeamMember(sess *DBSession, orgID, teamID, userID int64, isExternal bool, permission models.PermissionType) error { + if _, err := teamExists(orgID, teamID, sess); err != nil { + return err + } + + entity := models.TeamMember{ + OrgId: orgID, + TeamId: teamID, + UserId: userID, + External: isExternal, + Created: time.Now(), + Updated: time.Now(), + Permission: permission, + } + + _, err := sess.Insert(&entity) + return err +} + +func updateTeamMember(sess *DBSession, orgID, teamID, userID int64, permission models.PermissionType) error { + member, err := getTeamMember(sess, orgID, teamID, userID) + if err != nil { + return err + } + + if permission != models.PERMISSION_ADMIN { + permission = 0 // make sure we don't get invalid permission levels in store + + // protect the last team admin + _, err := isLastAdmin(sess, orgID, teamID, userID) if err != nil { return err } + } - if cmd.ProtectLastAdmin { - _, err := isLastAdmin(sess, cmd.OrgId, cmd.TeamId, cmd.UserId) - if err != nil { - return err - } - } - - if cmd.Permission != models.PERMISSION_ADMIN { // make sure we don't get invalid permission levels in store - cmd.Permission = 0 - } - - member.Permission = cmd.Permission - _, err = sess.Cols("permission").Where("org_id=? and team_id=? and user_id=?", cmd.OrgId, cmd.TeamId, cmd.UserId).Update(member) - - return err - }) + member.Permission = permission + _, err = sess.Cols("permission").Where("org_id=? and team_id=? and user_id=?", orgID, teamID, userID).Update(member) + return err } // RemoveTeamMember removes a member from a team func (ss *SQLStore) RemoveTeamMember(ctx context.Context, cmd *models.RemoveTeamMemberCommand) error { - return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - if _, err := teamExists(cmd.OrgId, cmd.TeamId, sess); err != nil { - return err - } - - if cmd.ProtectLastAdmin { - _, err := isLastAdmin(sess, cmd.OrgId, cmd.TeamId, cmd.UserId) - if err != nil { - return err - } - } - - var rawSQL = "DELETE FROM team_member WHERE org_id=? and team_id=? and user_id=?" - res, err := sess.Exec(rawSQL, cmd.OrgId, cmd.TeamId, cmd.UserId) - if err != nil { - return err - } - rows, err := res.RowsAffected() - if rows == 0 { - return models.ErrTeamMemberNotFound - } - - return err + return inTransaction(func(sess *DBSession) error { + return removeTeamMember(sess, cmd) }) } +// RemoveTeamMemberHook is called from team resource permission service +// it removes a member from a team within the given transaction session +func RemoveTeamMemberHook(sess *DBSession, cmd *models.RemoveTeamMemberCommand) error { + return removeTeamMember(sess, cmd) +} + +func removeTeamMember(sess *DBSession, cmd *models.RemoveTeamMemberCommand) error { + if _, err := teamExists(cmd.OrgId, cmd.TeamId, sess); err != nil { + return err + } + + _, err := isLastAdmin(sess, cmd.OrgId, cmd.TeamId, cmd.UserId) + if err != nil { + return err + } + + var rawSQL = "DELETE FROM team_member WHERE org_id=? and team_id=? and user_id=?" + res, err := sess.Exec(rawSQL, cmd.OrgId, cmd.TeamId, cmd.UserId) + if err != nil { + return err + } + rows, err := res.RowsAffected() + if rows == 0 { + return models.ErrTeamMemberNotFound + } + + return err +} + func isLastAdmin(sess *DBSession, orgId int64, teamId int64, userId int64) (bool, error) { rawSQL := "SELECT user_id FROM team_member WHERE org_id=? and team_id=? and permission=?" userIds := []*int64{} diff --git a/pkg/services/sqlstore/team_test.go b/pkg/services/sqlstore/team_test.go index f555202c92e..2d8f0b6defc 100644 --- a/pkg/services/sqlstore/team_test.go +++ b/pkg/services/sqlstore/team_test.go @@ -229,24 +229,24 @@ func TestTeamCommandsAndQueries(t *testing.T) { require.Equal(t, len(q2.Result), 0) }) - t.Run("When ProtectLastAdmin is set to true", func(t *testing.T) { + t.Run("Should never remove the last admin of a team", func(t *testing.T) { err = sqlStore.AddTeamMember(userIds[0], testOrgID, team1.Id, false, models.PERMISSION_ADMIN) require.NoError(t, err) t.Run("A user should not be able to remove the last admin", func(t *testing.T) { - err = sqlStore.RemoveTeamMember(context.Background(), &models.RemoveTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[0], ProtectLastAdmin: true}) + err = sqlStore.RemoveTeamMember(context.Background(), &models.RemoveTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[0]}) require.Equal(t, err, models.ErrLastTeamAdmin) }) t.Run("A user should be able to remove an admin if there are other admins", func(t *testing.T) { err = sqlStore.AddTeamMember(userIds[1], testOrgID, team1.Id, false, models.PERMISSION_ADMIN) require.NoError(t, err) - err = sqlStore.RemoveTeamMember(context.Background(), &models.RemoveTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[0], ProtectLastAdmin: true}) + err = sqlStore.RemoveTeamMember(context.Background(), &models.RemoveTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[1]}) require.NoError(t, err) }) t.Run("A user should not be able to remove the admin permission for the last admin", func(t *testing.T) { - err = sqlStore.UpdateTeamMember(context.Background(), &models.UpdateTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[0], Permission: 0, ProtectLastAdmin: true}) + err = sqlStore.UpdateTeamMember(context.Background(), &models.UpdateTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[0], Permission: 0}) require.Error(t, err, models.ErrLastTeamAdmin) }) @@ -259,7 +259,7 @@ func TestTeamCommandsAndQueries(t *testing.T) { err = sqlStore.AddTeamMember(userIds[1], testOrgID, team1.Id, false, models.PERMISSION_ADMIN) require.NoError(t, err) - err = sqlStore.UpdateTeamMember(context.Background(), &models.UpdateTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[0], Permission: 0, ProtectLastAdmin: true}) + err = sqlStore.UpdateTeamMember(context.Background(), &models.UpdateTeamMemberCommand{OrgId: testOrgID, TeamId: team1.Id, UserId: userIds[0], Permission: 0}) require.NoError(t, err) }) })