From d54fa569ec0460eec319930c94e56eb4ce401049 Mon Sep 17 00:00:00 2001 From: Ieva Date: Mon, 22 May 2023 17:41:53 +0100 Subject: [PATCH] Chore: Remove legacy AC checks from team (#68715) * removing legacy AC checks from team API handlers * Chore: remove `UserIDFilter` from team queries (#68820) * remove userIDfilter from team queries in favour of RBAC SQL filtering * fix typo * remove redundant tests * remove another unused function * fix failing test --- .github/CODEOWNERS | 1 - pkg/api/admin_users.go | 2 +- pkg/api/http_server.go | 5 +- pkg/api/team.go | 85 +---- pkg/api/team_members.go | 26 -- pkg/api/team_members_test.go | 302 +----------------- pkg/api/team_test.go | 196 +----------- pkg/api/user_test.go | 6 +- pkg/server/wire.go | 6 - pkg/services/team/model.go | 2 - pkg/services/team/team.go | 1 + pkg/services/team/teamimpl/store.go | 93 ++---- pkg/services/team/teamimpl/store_test.go | 23 +- pkg/services/team/teamimpl/team.go | 4 + pkg/services/team/teamtest/team.go | 4 + .../teamguardian/database/database.go | 34 -- .../teamguardian/database/database_mock.go | 23 -- pkg/services/teamguardian/manager/service.go | 53 --- .../teamguardian/manager/service_mock.go | 27 -- .../teamguardian/manager/service_test.go | 92 ------ pkg/services/teamguardian/team.go | 18 -- 21 files changed, 74 insertions(+), 929 deletions(-) delete mode 100644 pkg/services/teamguardian/database/database.go delete mode 100644 pkg/services/teamguardian/database/database_mock.go delete mode 100644 pkg/services/teamguardian/manager/service.go delete mode 100644 pkg/services/teamguardian/manager/service_mock.go delete mode 100644 pkg/services/teamguardian/manager/service_test.go delete mode 100644 pkg/services/teamguardian/team.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index a4e8f8ef6c0..45d7522e50e 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -541,7 +541,6 @@ lerna.json @grafana/frontend-ops /pkg/services/loginattempt/ @grafana/grafana-authnz-team /pkg/services/oauthtoken/ @grafana/grafana-authnz-team /pkg/services/serviceaccounts/ @grafana/grafana-authnz-team -/pkg/services/teamguardian/ @grafana/grafana-authnz-team # Support bundles /public/app/features/support-bundles/ @grafana/grafana-authnz-team diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 81f7faee645..bfd6e96cda0 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -235,7 +235,7 @@ func (hs *HTTPServer) AdminDeleteUser(c *contextmodel.ReqContext) response.Respo return nil }) g.Go(func() error { - if err := hs.teamGuardian.DeleteByUser(ctx, cmd.UserID); err != nil { + if err := hs.teamService.RemoveUsersMemberships(ctx, cmd.UserID); err != nil { return err } return nil diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 78a459389a8..4f8e75eef86 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -93,7 +93,6 @@ import ( "github.com/grafana/grafana/pkg/services/store/entity/httpentitystore" "github.com/grafana/grafana/pkg/services/tag" "github.com/grafana/grafana/pkg/services/team" - "github.com/grafana/grafana/pkg/services/teamguardian" tempUser "github.com/grafana/grafana/pkg/services/temp_user" "github.com/grafana/grafana/pkg/services/updatechecker" "github.com/grafana/grafana/pkg/services/user" @@ -168,7 +167,6 @@ type HTTPServer struct { grafanaUpdateChecker *updatechecker.GrafanaService pluginsUpdateChecker *updatechecker.PluginsService searchUsersService searchusers.Service - teamGuardian teamguardian.TeamGuardian queryDataService query.Service serviceAccountsService serviceaccounts.Service authInfoService login.AuthInfoService @@ -231,7 +229,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi encryptionService encryption.Internal, grafanaUpdateChecker *updatechecker.GrafanaService, pluginsUpdateChecker *updatechecker.PluginsService, searchUsersService searchusers.Service, dataSourcesService datasources.DataSourceService, queryDataService query.Service, pluginFileStore plugins.FileStore, - teamGuardian teamguardian.TeamGuardian, serviceaccountsService serviceaccounts.Service, + serviceaccountsService serviceaccounts.Service, authInfoService login.AuthInfoService, storageService store.StorageService, httpEntityStore httpentitystore.HTTPEntityStore, notificationService *notifications.NotificationService, dashboardService dashboards.DashboardService, dashboardProvisioningService dashboards.DashboardProvisioningService, folderService folder.Service, @@ -314,7 +312,6 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi httpEntityStore: httpEntityStore, DataSourcesService: dataSourcesService, searchUsersService: searchUsersService, - teamGuardian: teamGuardian, queryDataService: queryDataService, serviceAccountsService: serviceaccountsService, authInfoService: authInfoService, diff --git a/pkg/api/team.go b/pkg/api/team.go index 5d5d2b6ed22..18944eca554 100644 --- a/pkg/api/team.go +++ b/pkg/api/team.go @@ -9,7 +9,6 @@ import ( "github.com/grafana/grafana/pkg/api/response" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/team" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" @@ -30,10 +29,6 @@ func (hs *HTTPServer) CreateTeam(c *contextmodel.ReqContext) response.Response { if err := web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } - accessControlEnabled := !hs.AccessControl.IsDisabled() - if !accessControlEnabled && c.OrgRole == org.RoleViewer { - return response.Error(403, "Not allowed to create team.", nil) - } t, err := hs.teamService.CreateTeam(cmd.Name, cmd.Email, c.OrgID) if err != nil { @@ -45,21 +40,17 @@ func (hs *HTTPServer) CreateTeam(c *contextmodel.ReqContext) response.Response { // Clear permission cache for the user who's created the team, so that new permissions are fetched for their next call // Required for cases when caller wants to immediately interact with the newly created object - if !hs.AccessControl.IsDisabled() { - hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser) - } + hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser) - if accessControlEnabled || (c.OrgRole == org.RoleEditor && 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 - if c.SignedInUser.IsRealUser() { - if err := addOrUpdateTeamMember(c.Req.Context(), hs.teamPermissionsService, c.SignedInUser.UserID, c.OrgID, t.ID, dashboards.PERMISSION_ADMIN.String()); err != nil { - c.Logger.Error("Could not add creator to team", "error", err) - } - } else { - c.Logger.Warn("Could not add creator to team because is not a real user") + // 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 + if c.SignedInUser.IsRealUser() { + if err := addOrUpdateTeamMember(c.Req.Context(), hs.teamPermissionsService, c.SignedInUser.UserID, c.OrgID, t.ID, dashboards.PERMISSION_ADMIN.String()); err != nil { + c.Logger.Error("Could not add creator to team", "error", err) } + } else { + c.Logger.Warn("Could not add creator to team because is not a real user") } return response.JSON(http.StatusOK, &util.DynMap{ "teamId": t.ID, @@ -90,12 +81,6 @@ func (hs *HTTPServer) UpdateTeam(c *contextmodel.ReqContext) response.Response { return response.Error(http.StatusBadRequest, "teamId is invalid", err) } - if hs.AccessControl.IsDisabled() { - 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.teamService.UpdateTeam(c.Req.Context(), &cmd); err != nil { if errors.Is(err, team.ErrTeamNameTaken) { return response.Error(400, "Team name taken", err) @@ -122,13 +107,6 @@ func (hs *HTTPServer) DeleteTeamByID(c *contextmodel.ReqContext) response.Respon if err != nil { return response.Error(http.StatusBadRequest, "teamId is invalid", err) } - user := c.SignedInUser - - if hs.AccessControl.IsDisabled() { - 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.teamService.DeleteTeam(c.Req.Context(), &team.DeleteTeamCommand{OrgID: orgID, ID: teamID}); err != nil { if errors.Is(err, team.ErrTeamNotFound) { @@ -158,17 +136,10 @@ func (hs *HTTPServer) SearchTeams(c *contextmodel.ReqContext) response.Response page = 1 } - // Using accesscontrol the filtering is done based on user permissions - userIDFilter := team.FilterIgnoreUser - if hs.AccessControl.IsDisabled() { - userIDFilter = userFilter(c) - } - query := team.SearchTeamsQuery{ OrgID: c.OrgID, Query: c.Query("query"), Name: c.Query("name"), - UserIDFilter: userIDFilter, Page: page, Limit: perPage, SignedInUser: c.SignedInUser, @@ -199,17 +170,6 @@ func (hs *HTTPServer) SearchTeams(c *contextmodel.ReqContext) response.Response return response.JSON(http.StatusOK, queryResult) } -// UserFilter returns the user ID used in a filter when querying a team -// 1. If the user is a viewer or editor, this will return the user's ID. -// 2. If the user is an admin, this will return models.FilterIgnoreUser (0) -func userFilter(c *contextmodel.ReqContext) int64 { - userIdFilter := c.SignedInUser.UserID - if c.OrgRole == org.RoleAdmin { - userIdFilter = team.FilterIgnoreUser - } - return userIdFilter -} - // swagger:route GET /teams/{team_id} teams getTeamByID // // Get Team By ID. @@ -226,18 +186,11 @@ func (hs *HTTPServer) GetTeamByID(c *contextmodel.ReqContext) response.Response return response.Error(http.StatusBadRequest, "teamId is invalid", err) } - // Using accesscontrol the filtering has already been performed at middleware layer - userIdFilter := team.FilterIgnoreUser - if hs.AccessControl.IsDisabled() { - userIdFilter = userFilter(c) - } - query := team.GetTeamByIDQuery{ OrgID: c.OrgID, ID: teamId, SignedInUser: c.SignedInUser, HiddenUsers: hs.Cfg.HiddenUsers, - UserIdFilter: userIdFilter, } queryResult, err := hs.teamService.GetTeamByID(c.Req.Context(), &query) @@ -270,15 +223,7 @@ func (hs *HTTPServer) GetTeamPreferences(c *contextmodel.ReqContext) response.Re return response.Error(http.StatusBadRequest, "teamId is invalid", err) } - orgId := c.OrgID - - if hs.AccessControl.IsDisabled() { - if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, c.SignedInUser); err != nil { - return response.Error(403, "Not allowed to view team preferences.", err) - } - } - - return hs.getPreferencesFor(c.Req.Context(), orgId, 0, teamId) + return hs.getPreferencesFor(c.Req.Context(), c.OrgID, 0, teamId) } // swagger:route PUT /teams/{team_id}/preferences teams updateTeamPreferences @@ -301,15 +246,7 @@ func (hs *HTTPServer) UpdateTeamPreferences(c *contextmodel.ReqContext) response return response.Error(http.StatusBadRequest, "teamId is invalid", err) } - orgId := c.OrgID - - if hs.AccessControl.IsDisabled() { - if err := hs.teamGuardian.CanAdmin(c.Req.Context(), orgId, teamId, c.SignedInUser); err != nil { - return response.Error(403, "Not allowed to update team preferences.", err) - } - } - - return hs.updatePreferencesFor(c.Req.Context(), orgId, 0, teamId, &dtoCmd) + return hs.updatePreferencesFor(c.Req.Context(), c.OrgID, 0, teamId, &dtoCmd) } // swagger:parameters updateTeamPreferences diff --git a/pkg/api/team_members.go b/pkg/api/team_members.go index 3da7f5eda3b..0a069a0eb08 100644 --- a/pkg/api/team_members.go +++ b/pkg/api/team_members.go @@ -36,14 +36,6 @@ func (hs *HTTPServer) GetTeamMembers(c *contextmodel.ReqContext) response.Respon query := team.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.AccessControl.IsDisabled() { - 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) - } - } - queryResult, err := hs.teamService.GetTeamMembers(c.Req.Context(), &query) if err != nil { return response.Error(500, "Failed to get Team Members", err) @@ -91,12 +83,6 @@ func (hs *HTTPServer) AddTeamMember(c *contextmodel.ReqContext) response.Respons return response.Error(http.StatusBadRequest, "teamId is invalid", err) } - if hs.AccessControl.IsDisabled() { - 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) - } - } - isTeamMember, err := hs.teamService.IsTeamMember(c.OrgID, cmd.TeamID, cmd.UserID) if err != nil { return response.Error(500, "Failed to add team member.", err) @@ -140,12 +126,6 @@ func (hs *HTTPServer) UpdateTeamMember(c *contextmodel.ReqContext) response.Resp } orgId := c.OrgID - if hs.AccessControl.IsDisabled() { - 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.teamService.IsTeamMember(orgId, teamId, userId) if err != nil { return response.Error(500, "Failed to update team member.", err) @@ -192,12 +172,6 @@ func (hs *HTTPServer) RemoveTeamMember(c *contextmodel.ReqContext) response.Resp return response.Error(http.StatusBadRequest, "userId is invalid", err) } - if hs.AccessControl.IsDisabled() { - 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) - } - } - teamIDString := strconv.FormatInt(teamId, 10) if _, err := hs.teamPermissionsService.SetUserPermission(c.Req.Context(), orgId, accesscontrol.User{ID: userId}, teamIDString, ""); err != nil { if errors.Is(err, team.ErrTeamNotFound) { diff --git a/pkg/api/team_members_test.go b/pkg/api/team_members_test.go index 8c54bb68805..85858bc2902 100644 --- a/pkg/api/team_members_test.go +++ b/pkg/api/team_members_test.go @@ -1,193 +1,21 @@ package api import ( - "context" - "encoding/json" - "fmt" "net/http" "strings" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" - "github.com/grafana/grafana/pkg/infra/db" - "github.com/grafana/grafana/pkg/infra/db/dbtest" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" - "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/licensing" - "github.com/grafana/grafana/pkg/services/org" - "github.com/grafana/grafana/pkg/services/org/orgimpl" - "github.com/grafana/grafana/pkg/services/quota/quotaimpl" - "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" - "github.com/grafana/grafana/pkg/services/team" - "github.com/grafana/grafana/pkg/services/team/teamimpl" "github.com/grafana/grafana/pkg/services/team/teamtest" - "github.com/grafana/grafana/pkg/services/teamguardian/database" - "github.com/grafana/grafana/pkg/services/teamguardian/manager" - "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/services/user/userimpl" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web/webtest" ) -type TeamGuardianMock struct { - result error -} - -func (t *TeamGuardianMock) CanAdmin(ctx context.Context, orgId int64, teamId int64, user *user.SignedInUser) error { - return t.result -} - -func (t *TeamGuardianMock) DeleteByUser(ctx context.Context, userID int64) error { - return t.result -} - -func setUpGetTeamMembersHandler(t *testing.T, sqlStore *sqlstore.SQLStore) { - const testOrgID int64 = 1 - var userCmd user.CreateUserCommand - teamSvc := teamimpl.ProvideService(sqlStore, setting.NewCfg()) - team, err := teamSvc.CreateTeam("group1 name", "test1@test.com", testOrgID) - require.NoError(t, err) - quotaService := quotaimpl.ProvideService(sqlStore, sqlStore.Cfg) - orgService, err := orgimpl.ProvideService(sqlStore, sqlStore.Cfg, quotaService) - require.NoError(t, err) - usrSvc, err := userimpl.ProvideService(sqlStore, orgService, sqlStore.Cfg, nil, nil, quotaService, supportbundlestest.NewFakeBundleService()) - require.NoError(t, err) - - for i := 0; i < 3; i++ { - userCmd = user.CreateUserCommand{ - Email: fmt.Sprint("user", i, "@test.com"), - Name: fmt.Sprint("user", i), - Login: fmt.Sprint("loginuser", i), - } - // user - user, err := usrSvc.Create(context.Background(), &userCmd) - require.NoError(t, err) - err = teamSvc.AddTeamMember(user.ID, testOrgID, team.ID, false, 1) - require.NoError(t, err) - } -} - -func TestTeamMembersAPIEndpoint_userLoggedIn(t *testing.T) { - hs := setupSimpleHTTPServer(nil) - settings := hs.Cfg - sqlStore := db.InitTestDB(t) - sqlStore.Cfg = settings - - hs.SQLStore = sqlStore - hs.teamService = teamimpl.ProvideService(sqlStore, settings) - hs.License = &licensing.OSSLicensingService{} - hs.teamGuardian = &TeamGuardianMock{} - mock := dbtest.NewFakeDB() - - loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "api/teams/1/members", - "api/teams/:teamId/members", org.RoleAdmin, func(sc *scenarioContext) { - setUpGetTeamMembersHandler(t, sqlStore) - - sc.handlerFunc = hs.GetTeamMembers - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - require.Equal(t, http.StatusOK, sc.resp.Code) - - var resp []team.TeamMemberDTO - err := json.Unmarshal(sc.resp.Body.Bytes(), &resp) - require.NoError(t, err) - assert.Len(t, resp, 3) - }, mock) - - t.Run("Given there is two hidden users", func(t *testing.T) { - settings.HiddenUsers = map[string]struct{}{ - "user1": {}, - testUserLogin: {}, - } - t.Cleanup(func() { settings.HiddenUsers = make(map[string]struct{}) }) - - loggedInUserScenarioWithRole(t, "When calling GET on", "GET", "api/teams/1/members", - "api/teams/:teamId/members", org.RoleAdmin, func(sc *scenarioContext) { - setUpGetTeamMembersHandler(t, sqlStore) - - sc.handlerFunc = hs.GetTeamMembers - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - - require.Equal(t, http.StatusOK, sc.resp.Code) - - var resp []team.TeamMemberDTO - err := json.Unmarshal(sc.resp.Body.Bytes(), &resp) - require.NoError(t, err) - assert.Len(t, resp, 3) - assert.Equal(t, "loginuser0", resp[0].Login) - assert.Equal(t, "loginuser1", resp[1].Login) - assert.Equal(t, "loginuser2", resp[2].Login) - }, mock) - }) -} - -func TestAddTeamMembersAPIEndpoint_LegacyAccessControl(t *testing.T) { - server := SetupAPITestServer(t, func(hs *HTTPServer) { - cfg := setting.NewCfg() - cfg.RBACEnabled = false - cfg.EditorsCanAdmin = true - hs.Cfg = cfg - hs.teamService = teamtest.NewFakeService() - store := &database.TeamGuardianStoreMock{} - store.On("GetTeamMembers", mock.Anything, mock.Anything).Return([]*team.TeamMemberDTO{ - {UserID: 2, Permission: dashboards.PERMISSION_ADMIN}, - {UserID: 3, Permission: dashboards.PERMISSION_VIEW}, - }, nil).Maybe() - hs.teamGuardian = manager.ProvideService(store) - hs.teamPermissionsService = &actest.FakePermissionsService{} - }) - - t.Run("Admin can add team member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPost, "/api/teams/1/members", strings.NewReader("{\"userId\": 1}")), - &user.SignedInUser{OrgID: 1, OrgRole: org.RoleAdmin}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("Editor cannot add team member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPost, "/api/teams/1/members", strings.NewReader("{\"userId\": 1}")), - &user.SignedInUser{OrgID: 1, OrgRole: org.RoleEditor}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusForbidden, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("team member cannot add members", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPost, "/api/teams/1/members", strings.NewReader("{\"userId\": 1}")), - &user.SignedInUser{UserID: 3, OrgID: 1, OrgRole: org.RoleViewer}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusForbidden, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("team admin can add members", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPost, "/api/teams/1/members", strings.NewReader("{\"userId\": 1}")), - &user.SignedInUser{UserID: 2, OrgID: 1, OrgRole: org.RoleEditor}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) -} - -func TestAddTeamMembersAPIEndpoint_RBAC(t *testing.T) { +func TestAddTeamMembersAPIEndpoint(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = teamtest.NewFakeService() @@ -217,7 +45,7 @@ func TestAddTeamMembersAPIEndpoint_RBAC(t *testing.T) { }) } -func TestGetTeamMembersAPIEndpoint_RBAC(t *testing.T) { +func TestGetTeamMembersAPIEndpoint(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = teamtest.NewFakeService() @@ -246,68 +74,7 @@ func TestGetTeamMembersAPIEndpoint_RBAC(t *testing.T) { }) } -func TestUpdateTeamMembersAPIEndpoint_LegacyAccessControl(t *testing.T) { - server := SetupAPITestServer(t, func(hs *HTTPServer) { - cfg := setting.NewCfg() - cfg.RBACEnabled = false - cfg.EditorsCanAdmin = true - hs.Cfg = cfg - hs.teamService = &teamtest.FakeService{ExpectedIsMember: true} - store := &database.TeamGuardianStoreMock{} - store.On("GetTeamMembers", mock.Anything, mock.Anything).Return([]*team.TeamMemberDTO{ - {UserID: 2, Permission: dashboards.PERMISSION_ADMIN}, - {UserID: 3, Permission: dashboards.PERMISSION_VIEW}, - }, nil).Maybe() - hs.teamGuardian = manager.ProvideService(store) - hs.teamPermissionsService = &actest.FakePermissionsService{} - }) - - t.Run("Admin can update team member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPut, "/api/teams/1/members/1", strings.NewReader("{\"permission\": 4}")), - &user.SignedInUser{OrgID: 1, OrgRole: org.RoleAdmin}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("Editor cannot update team member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPut, "/api/teams/1/members/1", strings.NewReader("{\"permission\": 4}")), - &user.SignedInUser{OrgID: 1, OrgRole: org.RoleEditor}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusForbidden, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("team member cannot update member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPut, "/api/teams/1/members/1", strings.NewReader("{\"permission\": 4}")), - &user.SignedInUser{UserID: 3, OrgID: 1, OrgRole: org.RoleViewer}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusForbidden, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("team admin can add members", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodPut, "/api/teams/1/members/1", strings.NewReader("{\"permission\": 4}")), - &user.SignedInUser{UserID: 2, OrgID: 1, OrgRole: org.RoleEditor}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) -} - -func TestUpdateTeamMembersAPIEndpoint_RBAC(t *testing.T) { +func TestUpdateTeamMembersAPIEndpoint(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = &teamtest.FakeService{ExpectedIsMember: true} @@ -336,68 +103,7 @@ func TestUpdateTeamMembersAPIEndpoint_RBAC(t *testing.T) { }) } -func TestDeleteTeamMembersAPIEndpoint_LegacyAccessControl(t *testing.T) { - server := SetupAPITestServer(t, func(hs *HTTPServer) { - cfg := setting.NewCfg() - cfg.RBACEnabled = false - cfg.EditorsCanAdmin = true - hs.Cfg = cfg - hs.teamService = &teamtest.FakeService{ExpectedIsMember: true} - store := &database.TeamGuardianStoreMock{} - store.On("GetTeamMembers", mock.Anything, mock.Anything).Return([]*team.TeamMemberDTO{ - {UserID: 2, Permission: dashboards.PERMISSION_ADMIN}, - {UserID: 3, Permission: dashboards.PERMISSION_VIEW}, - }, nil).Maybe() - hs.teamGuardian = manager.ProvideService(store) - hs.teamPermissionsService = &actest.FakePermissionsService{} - }) - - t.Run("Admin can delete team member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodDelete, "/api/teams/1/members/1", nil), - &user.SignedInUser{OrgID: 1, OrgRole: org.RoleAdmin}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("Editor cannot delete team member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodDelete, "/api/teams/1/members/1", nil), - &user.SignedInUser{OrgID: 1, OrgRole: org.RoleEditor}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusForbidden, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("team member cannot delete member", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodDelete, "/api/teams/1/members/1", nil), - &user.SignedInUser{UserID: 3, OrgID: 1, OrgRole: org.RoleViewer}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusForbidden, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - t.Run("team admin can delete members", func(t *testing.T) { - req := webtest.RequestWithSignedInUser( - server.NewRequest(http.MethodDelete, "/api/teams/1/members/1", nil), - &user.SignedInUser{UserID: 2, OrgID: 1, OrgRole: org.RoleEditor}, - ) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) -} - -func TestDeleteTeamMembersAPIEndpoint_RBAC(t *testing.T) { +func TestDeleteTeamMembersAPIEndpoint(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = &teamtest.FakeService{ExpectedIsMember: true} diff --git a/pkg/api/team_test.go b/pkg/api/team_test.go index 1423a2ea7c4..5c6646124a7 100644 --- a/pkg/api/team_test.go +++ b/pkg/api/team_test.go @@ -1,8 +1,6 @@ package api import ( - "context" - "encoding/json" "fmt" "net/http" "strings" @@ -11,151 +9,18 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/grafana/grafana/pkg/infra/db" - "github.com/grafana/grafana/pkg/infra/db/dbtest" - "github.com/grafana/grafana/pkg/infra/log/logtest" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" - contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" - "github.com/grafana/grafana/pkg/services/org" pref "github.com/grafana/grafana/pkg/services/preference" "github.com/grafana/grafana/pkg/services/preference/preftest" "github.com/grafana/grafana/pkg/services/team" - "github.com/grafana/grafana/pkg/services/team/teamimpl" "github.com/grafana/grafana/pkg/services/team/teamtest" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/web" "github.com/grafana/grafana/pkg/web/webtest" ) -func TestTeamAPIEndpoint(t *testing.T) { - t.Run("Given two teams", func(t *testing.T) { - hs := setupSimpleHTTPServer(nil) - hs.Cfg.EditorsCanAdmin = true - store := db.InitTestDB(t) - store.Cfg = hs.Cfg - hs.teamService = teamimpl.ProvideService(store, hs.Cfg) - hs.SQLStore = store - mock := dbtest.NewFakeDB() - - loggedInUserScenarioWithRole(t, "When admin is calling GET on", "GET", "/api/teams/search", "/api/teams/search", - org.RoleAdmin, func(sc *scenarioContext) { - _, err := hs.teamService.CreateTeam("team1", "", 1) - require.NoError(t, err) - _, err = hs.teamService.CreateTeam("team2", "", 1) - require.NoError(t, err) - - sc.handlerFunc = hs.SearchTeams - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - require.Equal(t, http.StatusOK, sc.resp.Code) - var resp team.SearchTeamQueryResult - err = json.Unmarshal(sc.resp.Body.Bytes(), &resp) - require.NoError(t, err) - - assert.EqualValues(t, 2, resp.TotalCount) - assert.Equal(t, 2, len(resp.Teams)) - }, mock) - - loggedInUserScenario(t, "When editor (with editors_can_admin) is calling GET on", "/api/teams/search", - "/api/teams/search", func(sc *scenarioContext) { - team1, err := hs.teamService.CreateTeam("team1", "", 1) - require.NoError(t, err) - _, err = hs.teamService.CreateTeam("team2", "", 1) - require.NoError(t, err) - - // Adding the test user to the teams in order for him to list them - err = hs.teamService.AddTeamMember(testUserID, testOrgID, team1.ID, false, 0) - require.NoError(t, err) - - sc.handlerFunc = hs.SearchTeams - sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() - require.Equal(t, http.StatusOK, sc.resp.Code) - var resp team.SearchTeamQueryResult - err = json.Unmarshal(sc.resp.Body.Bytes(), &resp) - require.NoError(t, err) - - assert.EqualValues(t, 1, resp.TotalCount) - assert.Equal(t, 1, len(resp.Teams)) - }, mock) - - loggedInUserScenario(t, "When editor (with editors_can_admin) calling GET with pagination on", - "/api/teams/search", "/api/teams/search", func(sc *scenarioContext) { - team1, err := hs.teamService.CreateTeam("team1", "", 1) - require.NoError(t, err) - team2, err := hs.teamService.CreateTeam("team2", "", 1) - require.NoError(t, err) - - // Adding the test user to the teams in order for him to list them - err = hs.teamService.AddTeamMember(testUserID, testOrgID, team1.ID, false, 0) - require.NoError(t, err) - err = hs.teamService.AddTeamMember(testUserID, testOrgID, team2.ID, false, 0) - require.NoError(t, err) - - sc.handlerFunc = hs.SearchTeams - sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec() - require.Equal(t, http.StatusOK, sc.resp.Code) - var resp team.SearchTeamQueryResult - err = json.Unmarshal(sc.resp.Body.Bytes(), &resp) - require.NoError(t, err) - - assert.EqualValues(t, 2, resp.TotalCount) - assert.Equal(t, 0, len(resp.Teams)) - }, mock) - }) - - t.Run("When creating team with API key", func(t *testing.T) { - hs := setupSimpleHTTPServer(nil) - hs.Cfg.EditorsCanAdmin = true - hs.SQLStore = dbtest.NewFakeDB() - hs.teamService = &teamtest.FakeService{} - teamName := "team foo" - - addTeamMemberCalled := 0 - addOrUpdateTeamMember = func(ctx context.Context, resourcePermissionService accesscontrol.TeamPermissionsService, userID, orgID, teamID int64, - permission string) error { - addTeamMemberCalled++ - return nil - } - - req, err := http.NewRequest("POST", "/api/teams", nil) - require.NoError(t, err) - - t.Run("with no real signed in user", func(t *testing.T) { - logger := &logtest.Fake{} - c := &contextmodel.ReqContext{ - Context: &web.Context{Req: req}, - SignedInUser: &user.SignedInUser{}, - Logger: logger, - } - c.OrgRole = org.RoleEditor - c.Req.Body = mockRequestBody(team.CreateTeamCommand{Name: teamName}) - c.Req.Header.Add("Content-Type", "application/json") - r := hs.CreateTeam(c) - - assert.Equal(t, 200, r.Status()) - assert.NotZero(t, logger.WarnLogs.Calls) - assert.Equal(t, "Could not add creator to team because is not a real user", logger.WarnLogs.Message) - }) - - t.Run("with real signed in user", func(t *testing.T) { - logger := &logtest.Fake{} - c := &contextmodel.ReqContext{ - Context: &web.Context{Req: req}, - SignedInUser: &user.SignedInUser{UserID: 42}, - Logger: logger, - } - c.OrgRole = org.RoleEditor - c.Req.Body = mockRequestBody(team.CreateTeamCommand{Name: teamName}) - c.Req.Header.Add("Content-Type", "application/json") - r := hs.CreateTeam(c) - assert.Equal(t, 200, r.Status()) - assert.Zero(t, logger.WarnLogs.Calls) - }) - }) -} - const ( searchTeamsURL = "/api/teams/search" createTeamURL = "/api/teams/" @@ -163,56 +28,9 @@ const ( detailTeamPreferenceURL = "/api/teams/%d/preferences" teamCmd = `{"name": "MyTestTeam%d"}` teamPreferenceCmd = `{"theme": "dark"}` - teamPreferenceCmdLight = `{"theme": "light"}` ) -func TestTeamAPIEndpoint_CreateTeam_LegacyAccessControl(t *testing.T) { - server := SetupAPITestServer(t, func(hs *HTTPServer) { - hs.teamService = teamtest.NewFakeService() - }) - - input := strings.NewReader(fmt.Sprintf(teamCmd, 1)) - t.Run("Organisation admin can create a team", func(t *testing.T) { - req := server.NewPostRequest(createTeamURL, input) - req = webtest.RequestWithSignedInUser(req, &user.SignedInUser{OrgRole: org.RoleAdmin}) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) - - input = strings.NewReader(fmt.Sprintf(teamCmd, 2)) - t.Run("Org editor and server admin cannot create a team", func(t *testing.T) { - req := server.NewPostRequest(createTeamURL, input) - req = webtest.RequestWithSignedInUser(req, &user.SignedInUser{OrgRole: org.RoleEditor, IsGrafanaAdmin: true}) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusForbidden, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) -} - -func TestTeamAPIEndpoint_CreateTeam_LegacyAccessControl_EditorsCanAdmin(t *testing.T) { - server := SetupAPITestServer(t, func(hs *HTTPServer) { - cfg := setting.NewCfg() - cfg.RBACEnabled = false - cfg.EditorsCanAdmin = true - hs.Cfg = cfg - hs.teamService = teamtest.NewFakeService() - }) - - t.Run("Editors can create a team if editorsCanAdmin is set to true", func(t *testing.T) { - input := strings.NewReader(fmt.Sprintf(teamCmd, 1)) - req := server.NewPostRequest(createTeamURL, input) - req = webtest.RequestWithSignedInUser(req, &user.SignedInUser{OrgRole: org.RoleAdmin}) - res, err := server.SendJSON(req) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - require.NoError(t, res.Body.Close()) - }) -} - -func TestTeamAPIEndpoint_CreateTeam_RBAC(t *testing.T) { +func TestTeamAPIEndpoint_CreateTeam(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = teamtest.NewFakeService() @@ -241,7 +59,7 @@ func TestTeamAPIEndpoint_CreateTeam_RBAC(t *testing.T) { }) } -func TestTeamAPIEndpoint_SearchTeams_RBAC(t *testing.T) { +func TestTeamAPIEndpoint_SearchTeams(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = teamtest.NewFakeService() @@ -268,7 +86,7 @@ func TestTeamAPIEndpoint_SearchTeams_RBAC(t *testing.T) { }) } -func TestTeamAPIEndpoint_GetTeamByID_RBAC(t *testing.T) { +func TestTeamAPIEndpoint_GetTeamByID(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = &teamtest.FakeService{ExpectedTeamDTO: &team.TeamDTO{}} @@ -311,7 +129,7 @@ func TestTeamAPIEndpoint_GetTeamByID_RBAC(t *testing.T) { // 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_RBAC(t *testing.T) { +func TestTeamAPIEndpoint_UpdateTeam(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = &teamtest.FakeService{ExpectedTeamDTO: &team.TeamDTO{}} @@ -354,7 +172,7 @@ func TestTeamAPIEndpoint_UpdateTeam_RBAC(t *testing.T) { // 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_RBAC(t *testing.T) { +func TestTeamAPIEndpoint_DeleteTeam(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.teamService = &teamtest.FakeService{ExpectedTeamDTO: &team.TeamDTO{}} @@ -388,7 +206,7 @@ func TestTeamAPIEndpoint_DeleteTeam_RBAC(t *testing.T) { // Given a team with a user, when the user is granted X permission, // Then the endpoint should return 200 if the user has accesscontrol.ActionTeamsRead with teams:id:1 scope // else return 403 -func TestTeamAPIEndpoint_GetTeamPreferences_RBAC(t *testing.T) { +func TestTeamAPIEndpoint_GetTeamPreferences(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.preferenceService = &preftest.FakePreferenceService{ExpectedPreference: &pref.Preference{}} @@ -422,7 +240,7 @@ func TestTeamAPIEndpoint_GetTeamPreferences_RBAC(t *testing.T) { // 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_UpdateTeamPreferences_RBAC(t *testing.T) { +func TestTeamAPIEndpoint_UpdateTeamPreferences(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.preferenceService = &preftest.FakePreferenceService{ExpectedPreference: &pref.Preference{}} diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 2124a3ef9c0..f0a396d7111 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -19,6 +19,7 @@ import ( "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/db/dbtest" "github.com/grafana/grafana/pkg/infra/usagestats" + "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/login" @@ -41,10 +42,11 @@ import ( func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { settings := setting.NewCfg() sqlStore := db.InitTestDB(t) + sqlStore.Cfg = settings hs := &HTTPServer{ Cfg: settings, SQLStore: sqlStore, - AccessControl: acmock.New(), + AccessControl: acimpl.ProvideAccessControl(settings), } mockResult := user.SearchUserQueryResult{ @@ -56,6 +58,7 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { } mock := dbtest.NewFakeDB() userMock := usertest.NewUserServiceFake() + loggedInUserScenario(t, "When calling GET on", "api/users/1", "api/users/:id", func(sc *scenarioContext) { fakeNow := time.Date(2019, 2, 11, 17, 30, 40, 0, time.UTC) secretsService := secretsManager.SetupTestService(t, database.ProvideSecretsStore(sqlStore)) @@ -68,6 +71,7 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { hs.authInfoService = srv orgSvc, err := orgimpl.ProvideService(sqlStore, sqlStore.Cfg, quotatest.New(false, nil)) require.NoError(t, err) + require.NoError(t, err) userSvc, err := userimpl.ProvideService(sqlStore, orgSvc, sc.cfg, nil, nil, quotatest.New(false, nil), supportbundlestest.NewFakeBundleService()) require.NoError(t, err) hs.userService = userSvc diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 780e5ed21ef..9733f2d7ed0 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -133,9 +133,6 @@ import ( "github.com/grafana/grafana/pkg/services/tag" "github.com/grafana/grafana/pkg/services/tag/tagimpl" "github.com/grafana/grafana/pkg/services/team/teamimpl" - "github.com/grafana/grafana/pkg/services/teamguardian" - teamguardianDatabase "github.com/grafana/grafana/pkg/services/teamguardian/database" - teamguardianManager "github.com/grafana/grafana/pkg/services/teamguardian/manager" tempuser "github.com/grafana/grafana/pkg/services/temp_user" "github.com/grafana/grafana/pkg/services/temp_user/tempuserimpl" "github.com/grafana/grafana/pkg/services/updatechecker" @@ -282,9 +279,6 @@ var wireBasicSet = wire.NewSet( serviceaccountsmanager.ProvideServiceAccountsService, wire.Bind(new(serviceaccounts.Service), new(*serviceaccountsmanager.ServiceAccountsService)), expr.ProvideService, - teamguardianDatabase.ProvideTeamGuardianStore, - wire.Bind(new(teamguardian.Store), new(*teamguardianDatabase.TeamGuardianStoreImpl)), - teamguardianManager.ProvideService, featuremgmt.ProvideManagerService, featuremgmt.ProvideToggles, dashboardservice.ProvideDashboardServiceImpl, diff --git a/pkg/services/team/model.go b/pkg/services/team/model.go index bff43aed86b..a0a8a7afd9c 100644 --- a/pkg/services/team/model.go +++ b/pkg/services/team/model.go @@ -58,7 +58,6 @@ type GetTeamByIDQuery struct { ID int64 SignedInUser *user.SignedInUser HiddenUsers map[string]struct{} - UserIdFilter int64 } // FilterIgnoreUser is used in a get / search teams query when the caller does not want to filter teams by user ID / membership @@ -76,7 +75,6 @@ type SearchTeamsQuery struct { Limit int Page int OrgID int64 `xorm:"org_id"` - UserIDFilter int64 `xorm:"user_id_filter"` SignedInUser *user.SignedInUser HiddenUsers map[string]struct{} } diff --git a/pkg/services/team/team.go b/pkg/services/team/team.go index 9b9643894ff..86650074ae0 100644 --- a/pkg/services/team/team.go +++ b/pkg/services/team/team.go @@ -17,6 +17,7 @@ type Service interface { UpdateTeamMember(ctx context.Context, cmd *UpdateTeamMemberCommand) error IsTeamMember(orgId int64, teamId int64, userId int64) (bool, error) RemoveTeamMember(ctx context.Context, cmd *RemoveTeamMemberCommand) error + RemoveUsersMemberships(tx context.Context, userID int64) error GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*TeamMemberDTO, error) GetTeamMembers(ctx context.Context, query *GetTeamMembersQuery) ([]*TeamMemberDTO, error) IsAdminOfTeams(ctx context.Context, query *IsAdminOfTeamsQuery) (bool, error) diff --git a/pkg/services/team/teamimpl/store.go b/pkg/services/team/teamimpl/store.go index 8f2bc5178a4..ca18a846a33 100644 --- a/pkg/services/team/teamimpl/store.go +++ b/pkg/services/team/teamimpl/store.go @@ -23,6 +23,7 @@ type store interface { Search(ctx context.Context, query *team.SearchTeamsQuery) (team.SearchTeamQueryResult, error) GetByID(ctx context.Context, query *team.GetTeamByIDQuery) (*team.TeamDTO, error) GetByUser(ctx context.Context, query *team.GetTeamsByUserQuery) ([]*team.TeamDTO, error) + RemoveUsersMemberships(ctx context.Context, userID int64) error AddMember(userID, orgID, teamID int64, isExternal bool, permission dashboards.PermissionType) error UpdateMember(ctx context.Context, cmd *team.UpdateTeamMemberCommand) error IsMember(orgId int64, teamId int64, userId int64) (bool, error) @@ -76,19 +77,6 @@ func getTeamSelectSQLBase(db db.DB, filteredUsers []string) string { ` FROM team as team ` } -func getTeamSelectWithPermissionsSQLBase(db db.DB, filteredUsers []string) string { - return `SELECT - team.id AS id, - team.uid, - team.org_id, - team.name AS name, - team.email AS email, - team_member.permission, ` + - getTeamMemberCount(db, filteredUsers) + - ` FROM team AS team - INNER JOIN team_member ON team.id = team_member.team_id AND team_member.user_id = ? ` -} - func (ss *xormStore) Create(name, email string, orgID int64) (team.Team, error) { t := team.Team{ UID: util.GenerateShortUID(), @@ -207,13 +195,7 @@ func (ss *xormStore) Search(ctx context.Context, query *team.SearchTeamsQuery) ( params = append(params, user) } - if query.UserIDFilter == team.FilterIgnoreUser { - sql.WriteString(getTeamSelectSQLBase(ss.db, filteredUsers)) - } else { - sql.WriteString(getTeamSelectWithPermissionsSQLBase(ss.db, filteredUsers)) - params = append(params, query.UserIDFilter) - } - + sql.WriteString(getTeamSelectSQLBase(ss.db, filteredUsers)) sql.WriteString(` WHERE team.org_id = ?`) params = append(params, query.OrgID) @@ -227,18 +209,12 @@ func (ss *xormStore) Search(ctx context.Context, query *team.SearchTeamsQuery) ( params = append(params, query.Name) } - var ( - acFilter ac.SQLFilter - err error - ) - if !ac.IsDisabled(ss.cfg) { - acFilter, err = ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead) - if err != nil { - return err - } - sql.WriteString(` and` + acFilter.Where) - params = append(params, acFilter.Args...) + acFilter, err := ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead) + if err != nil { + return err } + sql.WriteString(` and` + acFilter.Where) + params = append(params, acFilter.Args...) sql.WriteString(` order by team.name asc`) @@ -263,22 +239,8 @@ func (ss *xormStore) Search(ctx context.Context, query *team.SearchTeamsQuery) ( countSess.Where("name=?", query.Name) } - // If we're not retrieving all results, then only search for teams that this user has access to - if query.UserIDFilter != team.FilterIgnoreUser { - countSess. - Where(` - team.id IN ( - SELECT - team_id - FROM team_member - WHERE team_member.user_id = ? - )`, query.UserIDFilter) - } - // Only count teams user can see - if !ac.IsDisabled(ss.cfg) { - countSess.Where(acFilter.Where, acFilter.Args...) - } + countSess.Where(acFilter.Where, acFilter.Args...) count, err := countSess.Count(&t) queryResult.TotalCount = count @@ -303,11 +265,6 @@ func (ss *xormStore) GetByID(ctx context.Context, query *team.GetTeamByIDQuery) params = append(params, user) } - if query.UserIdFilter != team.FilterIgnoreUser { - sql.WriteString(` INNER JOIN team_member ON team.id = team_member.team_id AND team_member.user_id = ?`) - params = append(params, query.UserIdFilter) - } - sql.WriteString(` WHERE team.org_id = ? and team.id = ?`) params = append(params, query.OrgID, query.ID) @@ -343,16 +300,14 @@ func (ss *xormStore) GetByUser(ctx context.Context, query *team.GetTeamsByUserQu sql.WriteString(` INNER JOIN team_member on team.id = team_member.team_id`) sql.WriteString(` WHERE team.org_id = ? and team_member.user_id = ?`) - if !ac.IsDisabled(ss.cfg) { - acFilter, err := ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead) - if err != nil { - return err - } - sql.WriteString(` and` + acFilter.Where) - params = append(params, acFilter.Args...) + acFilter, err := ac.Filter(query.SignedInUser, "team.id", "teams:id:", ac.ActionTeamsRead) + if err != nil { + return err } + sql.WriteString(` and` + acFilter.Where) + params = append(params, acFilter.Args...) - err := sess.SQL(sql.String(), params...).Find(&queryResult) + err = sess.SQL(sql.String(), params...).Find(&queryResult) return err }) if err != nil { @@ -500,6 +455,16 @@ func removeTeamMember(sess *db.Session, cmd *team.RemoveTeamMemberCommand) error return err } +// RemoveUsersMemberships removes all the team membership entries for the given user. +// Only used when removing a user from a Grafana instance. +func (ss *xormStore) RemoveUsersMemberships(ctx context.Context, userID int64) error { + return ss.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + var rawSQL = "DELETE FROM team_member WHERE user_id = ?" + _, err := sess.Exec(rawSQL, userID) + return err + }) +} + // 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. @@ -521,12 +486,10 @@ func (ss *xormStore) GetMembers(ctx context.Context, query *team.GetTeamMembersQ // 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 !ac.IsDisabled(ss.cfg) { - sqlID := fmt.Sprintf("%s.%s", ss.db.GetDialect().Quote("user"), ss.db.GetDialect().Quote("id")) - *acFilter, err = ac.Filter(query.SignedInUser, sqlID, "users:id:", ac.ActionOrgUsersRead) - if err != nil { - return nil, err - } + sqlID := fmt.Sprintf("%s.%s", ss.db.GetDialect().Quote("user"), ss.db.GetDialect().Quote("id")) + *acFilter, err = ac.Filter(query.SignedInUser, sqlID, "users:id:", ac.ActionOrgUsersRead) + if err != nil { + return nil, err } return ss.getTeamMembers(ctx, query, acFilter) diff --git a/pkg/services/team/teamimpl/store_test.go b/pkg/services/team/teamimpl/store_test.go index c185a371f72..f9d0930aeb5 100644 --- a/pkg/services/team/teamimpl/store_test.go +++ b/pkg/services/team/teamimpl/store_test.go @@ -369,13 +369,6 @@ func TestIntegrationTeamCommandsAndQueries(t *testing.T) { team1 := searchQueryResult.Teams[0] require.EqualValues(t, team1.MemberCount, 2) - searchQueryFilteredByUser := &team.SearchTeamsQuery{OrgID: testOrgID, Page: 1, Limit: 10, UserIDFilter: userIds[0], SignedInUser: signedInUser, HiddenUsers: hiddenUsers} - searchQueryFilteredByUserResult, err := teamSvc.SearchTeams(context.Background(), searchQueryFilteredByUser) - require.NoError(t, err) - require.Equal(t, len(searchQueryFilteredByUserResult.Teams), 1) - team1 = searchQueryResult.Teams[0] - require.EqualValues(t, team1.MemberCount, 2) - getTeamQuery := &team.GetTeamByIDQuery{OrgID: testOrgID, ID: teamId, SignedInUser: signedInUser, HiddenUsers: hiddenUsers} getTeamQueryResult, err := teamSvc.GetTeamByID(context.Background(), getTeamQuery) require.NoError(t, err) @@ -427,9 +420,9 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) { t.Skip("skipping integration test") } type searchTeamsTestCase struct { - desc string - query *team.SearchTeamsQuery - expectedNumUsers int + desc string + query *team.SearchTeamsQuery + expectedTeamCount int } tests := []searchTeamsTestCase{ @@ -442,7 +435,7 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) { Permissions: map[int64]map[string][]string{1: {ac.ActionTeamsRead: {ac.ScopeTeamsAll}}}, }, }, - expectedNumUsers: 10, + expectedTeamCount: 10, }, { desc: "should return no teams", @@ -453,7 +446,7 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) { Permissions: map[int64]map[string][]string{1: {ac.ActionTeamsRead: {""}}}, }, }, - expectedNumUsers: 0, + expectedTeamCount: 0, }, { desc: "should return some teams", @@ -468,7 +461,7 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) { }}}, }, }, - expectedNumUsers: 3, + expectedTeamCount: 3, }, } @@ -485,8 +478,8 @@ func TestIntegrationSQLStore_SearchTeams(t *testing.T) { t.Run(tt.desc, func(t *testing.T) { queryResult, err := teamSvc.SearchTeams(context.Background(), tt.query) require.NoError(t, err) - assert.Len(t, queryResult.Teams, tt.expectedNumUsers) - assert.Equal(t, queryResult.TotalCount, int64(tt.expectedNumUsers)) + assert.Len(t, queryResult.Teams, tt.expectedTeamCount) + assert.Equal(t, queryResult.TotalCount, int64(tt.expectedTeamCount)) if !hasWildcardScope(tt.query.SignedInUser, ac.ActionTeamsRead) { for _, team := range queryResult.Teams { diff --git a/pkg/services/team/teamimpl/team.go b/pkg/services/team/teamimpl/team.go index 31ce4295cf5..ee6eb1285d0 100644 --- a/pkg/services/team/teamimpl/team.go +++ b/pkg/services/team/teamimpl/team.go @@ -57,6 +57,10 @@ func (s *Service) RemoveTeamMember(ctx context.Context, cmd *team.RemoveTeamMemb return s.store.RemoveMember(ctx, cmd) } +func (s *Service) RemoveUsersMemberships(ctx context.Context, userID int64) error { + return s.store.RemoveUsersMemberships(ctx, userID) +} + func (s *Service) GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*team.TeamMemberDTO, error) { return s.store.GetMemberships(ctx, orgID, userID, external) } diff --git a/pkg/services/team/teamtest/team.go b/pkg/services/team/teamtest/team.go index dc10a7150e1..35eeadea001 100644 --- a/pkg/services/team/teamtest/team.go +++ b/pkg/services/team/teamtest/team.go @@ -61,6 +61,10 @@ func (s *FakeService) RemoveTeamMember(ctx context.Context, cmd *team.RemoveTeam return s.ExpectedError } +func (s *FakeService) RemoveUsersMemberships(ctx context.Context, userID int64) error { + return s.ExpectedError +} + func (s *FakeService) GetUserTeamMemberships(ctx context.Context, orgID, userID int64, external bool) ([]*team.TeamMemberDTO, error) { return s.ExpectedMembers, s.ExpectedError } diff --git a/pkg/services/teamguardian/database/database.go b/pkg/services/teamguardian/database/database.go deleted file mode 100644 index 4ddc50a4cc2..00000000000 --- a/pkg/services/teamguardian/database/database.go +++ /dev/null @@ -1,34 +0,0 @@ -package database - -import ( - "context" - - "github.com/grafana/grafana/pkg/infra/db" - "github.com/grafana/grafana/pkg/services/team" -) - -type TeamGuardianStoreImpl struct { - sqlStore db.DB - teamService team.Service -} - -func ProvideTeamGuardianStore(sqlStore db.DB, teamService team.Service) *TeamGuardianStoreImpl { - return &TeamGuardianStoreImpl{sqlStore: sqlStore, teamService: teamService} -} - -func (t *TeamGuardianStoreImpl) GetTeamMembers(ctx context.Context, query team.GetTeamMembersQuery) ([]*team.TeamMemberDTO, error) { - queryResult, err := t.teamService.GetTeamMembers(ctx, &query) - if err != nil { - return nil, err - } - - return queryResult, nil -} - -func (t *TeamGuardianStoreImpl) DeleteByUser(ctx context.Context, userID int64) error { - return t.sqlStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error { - var rawSQL = "DELETE FROM team_member WHERE user_id = ?" - _, err := sess.Exec(rawSQL, userID) - return err - }) -} diff --git a/pkg/services/teamguardian/database/database_mock.go b/pkg/services/teamguardian/database/database_mock.go deleted file mode 100644 index 0a7dd142e56..00000000000 --- a/pkg/services/teamguardian/database/database_mock.go +++ /dev/null @@ -1,23 +0,0 @@ -package database - -import ( - "context" - - "github.com/stretchr/testify/mock" - - "github.com/grafana/grafana/pkg/services/team" -) - -type TeamGuardianStoreMock struct { - mock.Mock -} - -func (t *TeamGuardianStoreMock) GetTeamMembers(ctx context.Context, query team.GetTeamMembersQuery) ([]*team.TeamMemberDTO, error) { - args := t.Called(ctx, query) - return args.Get(0).([]*team.TeamMemberDTO), args.Error(1) -} - -func (t *TeamGuardianStoreMock) DeleteByUser(ctx context.Context, userID int64) error { - args := t.Called(ctx, userID) - return args.Get(0).(error) -} diff --git a/pkg/services/teamguardian/manager/service.go b/pkg/services/teamguardian/manager/service.go deleted file mode 100644 index a21018c8302..00000000000 --- a/pkg/services/teamguardian/manager/service.go +++ /dev/null @@ -1,53 +0,0 @@ -package manager - -import ( - "context" - - "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/org" - "github.com/grafana/grafana/pkg/services/team" - "github.com/grafana/grafana/pkg/services/teamguardian" - "github.com/grafana/grafana/pkg/services/user" -) - -type Service struct { - store teamguardian.Store -} - -func ProvideService(store teamguardian.Store) teamguardian.TeamGuardian { - return &Service{store: store} -} - -func (s *Service) CanAdmin(ctx context.Context, orgId int64, teamId int64, user *user.SignedInUser) error { - if user.OrgRole == org.RoleAdmin { - return nil - } - - if user.OrgID != orgId { - return team.ErrNotAllowedToUpdateTeamInDifferentOrg - } - - cmd := team.GetTeamMembersQuery{ - OrgID: orgId, - TeamID: teamId, - UserID: user.UserID, - SignedInUser: user, - } - - results, err := s.store.GetTeamMembers(ctx, cmd) - if err != nil { - return err - } - - for _, member := range results { - if member.UserID == user.UserID && member.Permission == dashboards.PERMISSION_ADMIN { - return nil - } - } - - return team.ErrNotAllowedToUpdateTeam -} - -func (s *Service) DeleteByUser(ctx context.Context, userID int64) error { - return s.store.DeleteByUser(ctx, userID) -} diff --git a/pkg/services/teamguardian/manager/service_mock.go b/pkg/services/teamguardian/manager/service_mock.go deleted file mode 100644 index b561b926d69..00000000000 --- a/pkg/services/teamguardian/manager/service_mock.go +++ /dev/null @@ -1,27 +0,0 @@ -package manager - -import ( - "context" - - "github.com/stretchr/testify/mock" - - "github.com/grafana/grafana/pkg/services/user" -) - -type TeamGuardianMock struct { - mock.Mock - ExpectedError error -} - -func NewTeamGuardianMock() *TeamGuardianMock { - return &TeamGuardianMock{} -} - -func (t *TeamGuardianMock) CanAdmin(ctx context.Context, orgId int64, teamId int64, user *user.SignedInUser) error { - args := t.Called(ctx, orgId, teamId, user) - return args.Error(0) -} - -func (t *TeamGuardianMock) DeleteByUser(ctx context.Context, userID int64) error { - return t.ExpectedError -} diff --git a/pkg/services/teamguardian/manager/service_test.go b/pkg/services/teamguardian/manager/service_test.go deleted file mode 100644 index d4da1f1c5f5..00000000000 --- a/pkg/services/teamguardian/manager/service_test.go +++ /dev/null @@ -1,92 +0,0 @@ -package manager - -import ( - "context" - "testing" - - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" - - "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/org" - "github.com/grafana/grafana/pkg/services/team" - "github.com/grafana/grafana/pkg/services/teamguardian/database" - "github.com/grafana/grafana/pkg/services/user" -) - -func TestUpdateTeam(t *testing.T) { - store := new(database.TeamGuardianStoreMock) - teamGuardianService := ProvideService(store) - - t.Run("Updating a team", func(t *testing.T) { - admin := user.SignedInUser{ - UserID: 1, - OrgID: 1, - OrgRole: org.RoleAdmin, - } - editor := user.SignedInUser{ - UserID: 2, - OrgID: 1, - OrgRole: org.RoleEditor, - } - testTeam := team.Team{ - ID: 1, - OrgID: 1, - } - - t.Run("Given an editor and a team he isn't a member of", func(t *testing.T) { - t.Run("Should not be able to update the team", func(t *testing.T) { - ctx := context.Background() - store.On("GetTeamMembers", ctx, mock.Anything).Return([]*team.TeamMemberDTO{}, nil).Once() - err := teamGuardianService.CanAdmin(ctx, testTeam.OrgID, testTeam.ID, &editor) - require.Equal(t, team.ErrNotAllowedToUpdateTeam, err) - }) - }) - - t.Run("Given an editor and a team he is an admin in", func(t *testing.T) { - t.Run("Should be able to update the team", func(t *testing.T) { - ctx := context.Background() - - result := []*team.TeamMemberDTO{{ - OrgID: testTeam.OrgID, - TeamID: testTeam.ID, - UserID: editor.UserID, - Permission: dashboards.PERMISSION_ADMIN, - }} - - store.On("GetTeamMembers", ctx, mock.Anything).Return(result, nil).Once() - err := teamGuardianService.CanAdmin(ctx, testTeam.OrgID, testTeam.ID, &editor) - require.NoError(t, err) - }) - }) - - t.Run("Given an editor and a team in another org", func(t *testing.T) { - ctx := context.Background() - - testTeamOtherOrg := team.Team{ - ID: 1, - OrgID: 2, - } - - t.Run("Shouldn't be able to update the team", func(t *testing.T) { - result := []*team.TeamMemberDTO{{ - OrgID: testTeamOtherOrg.OrgID, - TeamID: testTeamOtherOrg.ID, - UserID: editor.UserID, - Permission: dashboards.PERMISSION_ADMIN, - }} - - store.On("GetTeamMembers", ctx, mock.Anything).Return(result, nil).Once() - err := teamGuardianService.CanAdmin(ctx, testTeamOtherOrg.OrgID, testTeamOtherOrg.ID, &editor) - require.Equal(t, team.ErrNotAllowedToUpdateTeamInDifferentOrg, err) - }) - }) - - t.Run("Given an org admin and a team", func(t *testing.T) { - t.Run("Should be able to update the team", func(t *testing.T) { - err := teamGuardianService.CanAdmin(context.Background(), testTeam.OrgID, testTeam.ID, &admin) - require.NoError(t, err) - }) - }) - }) -} diff --git a/pkg/services/teamguardian/team.go b/pkg/services/teamguardian/team.go deleted file mode 100644 index 7c930deb58b..00000000000 --- a/pkg/services/teamguardian/team.go +++ /dev/null @@ -1,18 +0,0 @@ -package teamguardian - -import ( - "context" - - "github.com/grafana/grafana/pkg/services/team" - "github.com/grafana/grafana/pkg/services/user" -) - -type TeamGuardian interface { - CanAdmin(context.Context, int64, int64, *user.SignedInUser) error - DeleteByUser(context.Context, int64) error -} - -type Store interface { - GetTeamMembers(context.Context, team.GetTeamMembersQuery) ([]*team.TeamMemberDTO, error) - DeleteByUser(context.Context, int64) error -}