Chore: Split delete user method (#52216)

* Remove user from preferences, stars, orguser, team member

* Fix lint

* Add Delete user from org and dashboard acl

* Delete user from user auth

* Add DeleteUser to quota

* Add test files and adjust user auth store

* Rename package in wire for user auth

* Import Quota Service interface in other services

* do the same in tests

* fix lint tests

* Fix tests

* Add some tests

* Rename InsertUser and DeleteUser to InsertOrgUser and DeleteOrgUser

* Rename DeleteUser to DeleteByUser in quota

* changing a method name in few additional places

* Fix in other places

* Fix lint

* Fix tests

* Chore: Split Delete User method

* Add fakes for userauth

* Add mock for access control Delete User permossion, use interface

* Use interface for ream guardian

* Add simple fake for dashboard acl

* Add go routines, clean up, use interfaces

* fix lint

* Update pkg/services/user/userimpl/user_test.go

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>

* Update pkg/services/user/userimpl/user_test.go

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>

* Update pkg/services/user/userimpl/user_test.go

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>

* Add wrapper for not service account error

* fix indentation

* Use fmt for error wrapper

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>
This commit is contained in:
idafurjes 2022-07-19 16:01:05 +02:00 committed by GitHub
parent 39025bb4cd
commit c061b66d5f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
20 changed files with 315 additions and 35 deletions

View File

@ -208,8 +208,8 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
avatarCacheServer *avatar.AvatarCacheServer, preferenceService pref.Service, entityEventsService store.EntityEventsService,
teamsPermissionsService accesscontrol.TeamPermissionsService, folderPermissionsService accesscontrol.FolderPermissionsService,
dashboardPermissionsService accesscontrol.DashboardPermissionsService, dashboardVersionService dashver.Service,
starService star.Service, playlistService playlist.Service, csrfService csrf.Service, coremodelRegistry *registry.Generic, coremodelStaticRegistry *registry.Static,
kvStore kvstore.KVStore, secretsMigrator secrets.Migrator, remoteSecretsCheck secretsKV.UseRemoteSecretsPluginCheck,
starService star.Service, csrfService csrf.Service, coremodelRegistry *registry.Generic, coremodelStaticRegistry *registry.Static,
playlistService playlist.Service, kvStore kvstore.KVStore, secretsMigrator secrets.Migrator, remoteSecretsCheck secretsKV.UseRemoteSecretsPluginCheck,
publicDashboardsApi *publicdashboardsApi.Api, userService user.Service) (*HTTPServer, error) {
web.Env = cfg.Env
m := web.New()

View File

@ -110,6 +110,7 @@ import (
"github.com/grafana/grafana/pkg/services/thumbs"
"github.com/grafana/grafana/pkg/services/updatechecker"
"github.com/grafana/grafana/pkg/services/user/userimpl"
"github.com/grafana/grafana/pkg/services/userauth/userauthimpl"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/tsdb/azuremonitor"
"github.com/grafana/grafana/pkg/tsdb/cloudmonitoring"
@ -251,7 +252,6 @@ var wireBasicSet = wire.NewSet(
teamguardianDatabase.ProvideTeamGuardianStore,
wire.Bind(new(teamguardian.Store), new(*teamguardianDatabase.TeamGuardianStoreImpl)),
teamguardianManager.ProvideService,
wire.Bind(new(teamguardian.TeamGuardian), new(*teamguardianManager.Service)),
featuremgmt.ProvideManagerService,
featuremgmt.ProvideToggles,
dashboardservice.ProvideDashboardService,
@ -299,6 +299,7 @@ var wireBasicSet = wire.NewSet(
datasourceservice.ProvideDataSourceMigrationService,
secretsMigrations.ProvideSecretMigrationService,
wire.Bind(new(secretsMigrations.SecretMigrationService), new(*secretsMigrations.SecretMigrationServiceImpl)),
userauthimpl.ProvideService,
)
var wireSet = wire.NewSet(

View File

@ -33,6 +33,8 @@ type AccessControl interface {
// RegisterScopeAttributeResolver allows the caller to register a scope resolver for a
// specific scope prefix (ex: datasources:name:)
RegisterScopeAttributeResolver(scopePrefix string, resolver ScopeAttributeResolver)
DeleteUserPermissions(ctx context.Context, userID int64) error
}
type RoleRegistry interface {
@ -43,6 +45,7 @@ type RoleRegistry interface {
type PermissionsStore interface {
// GetUserPermissions returns user permissions with only action and scope fields set.
GetUserPermissions(ctx context.Context, query GetUserPermissionsQuery) ([]Permission, error)
DeleteUserPermissions(ctx context.Context, userID int64) error
}
type TeamPermissionsService interface {

View File

@ -21,6 +21,7 @@ type Calls struct {
GetUserBuiltInRoles []interface{}
RegisterFixedRoles []interface{}
RegisterAttributeScopeResolver []interface{}
DeleteUserPermissions []interface{}
}
type Mock struct {
@ -42,6 +43,7 @@ type Mock struct {
GetUserBuiltInRolesFunc func(user *models.SignedInUser) []string
RegisterFixedRolesFunc func() error
RegisterScopeAttributeResolverFunc func(string, accesscontrol.ScopeAttributeResolver)
DeleteUserPermissionsFunc func(context.Context, int64) error
scopeResolvers accesscontrol.ScopeResolvers
}
@ -180,3 +182,12 @@ func (m *Mock) RegisterScopeAttributeResolver(scopePrefix string, resolver acces
m.RegisterScopeAttributeResolverFunc(scopePrefix, resolver)
}
}
func (m *Mock) DeleteUserPermissions(ctx context.Context, userID int64) error {
m.Calls.DeleteUserPermissions = append(m.Calls.DeleteUserPermissions, []interface{}{ctx, userID})
// Use override if provided
if m.DeleteUserPermissionsFunc != nil {
return m.DeleteUserPermissionsFunc(ctx, userID)
}
return nil
}

View File

@ -198,3 +198,7 @@ func (ac *OSSAccessControlService) DeclareFixedRoles(registrations ...accesscont
func (ac *OSSAccessControlService) RegisterScopeAttributeResolver(scopePrefix string, resolver accesscontrol.ScopeAttributeResolver) {
ac.scopeResolvers.AddScopeAttributeResolver(scopePrefix, resolver)
}
func (ac *OSSAccessControlService) DeleteUserPermissions(ctx context.Context, userID int64) error {
return ac.store.DeleteUserPermissions(ctx, userID)
}

View File

@ -24,6 +24,7 @@ type DashboardService interface {
SaveDashboard(ctx context.Context, dto *SaveDashboardDTO, allowUiUpdate bool) (*models.Dashboard, error)
SearchDashboards(ctx context.Context, query *models.FindPersistedDashboardsQuery) error
UpdateDashboardACL(ctx context.Context, uid int64, items []*models.DashboardACL) error
DeleteACLByUser(ctx context.Context, userID int64) error
}
// PluginService is a service for operating on plugin dashboards.

View File

@ -14,6 +14,7 @@ import (
// FakeDashboardService is an autogenerated mock type for the DashboardService type
type FakeDashboardService struct {
mock.Mock
ExpectedError error
}
// BuildSaveDashboardCommand provides a mock function with given fields: ctx, dto, shouldValidateAlerts, validateProvisionedDashboard
@ -262,6 +263,10 @@ func (_m *FakeDashboardService) UpdateDashboardACL(ctx context.Context, uid int6
return r0
}
func (_m *FakeDashboardService) DeleteACLByUser(ctx context.Context, userID int64) error {
return _m.ExpectedError
}
// NewFakeDashboardService creates a new instance of FakeDashboardService. It also registers the testing.TB interface on the mock and a cleanup function to assert the mocks expectations.
func NewFakeDashboardService(t testing.TB) *FakeDashboardService {
mock := &FakeDashboardService{}

View File

@ -223,4 +223,18 @@ func TestDashboardService(t *testing.T) {
// })
})
})
t.Run("Delete user by acl", func(t *testing.T) {
fakeStore := dashboards.FakeDashboardStore{}
defer fakeStore.AssertExpectations(t)
service := &DashboardServiceImpl{
cfg: setting.NewCfg(),
log: log.New("test.logger"),
dashboardStore: &fakeStore,
dashAlertExtractor: &dummyDashAlertExtractor{},
}
err := service.DeleteACLByUser(context.Background(), 1)
require.NoError(t, err)
})
}

View File

@ -14,6 +14,7 @@ import (
// FakeDashboardStore is an autogenerated mock type for the Store type
type FakeDashboardStore struct {
mock.Mock
ExpectedError error
}
// DeleteDashboard provides a mock function with given fields: ctx, cmd
@ -436,16 +437,7 @@ func (_m *FakeDashboardStore) ValidateDashboardBeforeSave(dashboard *models.Dash
}
func (_m *FakeDashboardStore) DeleteACLByUser(ctx context.Context, userID int64) error {
ret := _m.Called(ctx, userID)
var r0 error
if rf, ok := ret.Get(0).(func(context.Context, int64) error); ok {
r0 = rf(ctx, userID)
} else {
r0 = ret.Error(0)
}
return r0
return _m.ExpectedError
}
// NewFakeDashboardStore creates a new instance of FakeDashboardStore. It also registers the testing.TB interface on the mock and a cleanup function to assert the mocks expectations.

View File

@ -48,6 +48,11 @@ func TestIntegrationOrgDataAccess(t *testing.T) {
_, err = orgStore.Get(context.Background(), orgID)
require.NoError(t, err)
})
t.Run("delete by user", func(t *testing.T) {
err := orgStore.DeleteUserFromAll(context.Background(), 1)
require.NoError(t, err)
})
}
func TestIntegrationOrgUserDataAccess(t *testing.T) {

View File

@ -11,7 +11,7 @@ type Service struct {
store teamguardian.Store
}
func ProvideService(store teamguardian.Store) *Service {
func ProvideService(store teamguardian.Store) teamguardian.TeamGuardian {
return &Service{store: store}
}

View File

@ -9,6 +9,11 @@ import (
type TeamGuardianMock struct {
mock.Mock
ExpectedError error
}
func NewTeamGuardianMock() *TeamGuardianMock {
return &TeamGuardianMock{}
}
func (t *TeamGuardianMock) CanAdmin(ctx context.Context, orgId int64, teamId int64, user *models.SignedInUser) error {
@ -16,7 +21,6 @@ func (t *TeamGuardianMock) CanAdmin(ctx context.Context, orgId int64, teamId int
return args.Error(0)
}
func (t *TeamGuardianMock) DeleteByUser(context.Context, int64) error {
args := t.Called(context.Background(), 0)
return args.Error(0)
func (t *TeamGuardianMock) DeleteByUser(ctx context.Context, userID int64) error {
return t.ExpectedError
}

View File

@ -1,6 +1,8 @@
package user
import "time"
import (
"time"
)
type HelpFlags1 uint64
@ -53,3 +55,7 @@ func (u *User) NameOrFallback() string {
}
return u.Email
}
type DeleteUserCommand struct {
UserID int64
}

View File

@ -6,4 +6,5 @@ import (
type Service interface {
Create(context.Context, *CreateUserCommand) (*User, error)
Delete(context.Context, *DeleteUserCommand) error
}

View File

@ -2,21 +2,26 @@ package userimpl
import (
"context"
"fmt"
"github.com/grafana/grafana/pkg/events"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/db"
"github.com/grafana/grafana/pkg/services/sqlstore/migrator"
"github.com/grafana/grafana/pkg/services/user"
)
type store interface {
Insert(context.Context, *user.User) (int64, error)
Get(context.Context, *user.User) (*user.User, error)
GetNotServiceAccount(context.Context, int64) (*user.User, error)
Delete(context.Context, int64) error
}
type sqlStore struct {
db db.DB
db db.DB
dialect migrator.Dialect
}
func (ss *sqlStore) Insert(ctx context.Context, cmd *user.User) (int64, error) {
@ -43,10 +48,9 @@ func (ss *sqlStore) Insert(ctx context.Context, cmd *user.User) (int64, error) {
return userID, nil
}
func (ss *sqlStore) Get(ctx context.Context, cmd *user.User) (*user.User, error) {
var usr *user.User
func (ss *sqlStore) Get(ctx context.Context, usr *user.User) (*user.User, error) {
err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
exists, err := sess.Where("email=? OR login=?", cmd.Email, cmd.Login).Get(&user.User{})
exists, err := sess.Where("email=? OR login=?", usr.Email, usr.Login).Get(usr)
if !exists {
return models.ErrUserNotFound
}
@ -60,3 +64,36 @@ func (ss *sqlStore) Get(ctx context.Context, cmd *user.User) (*user.User, error)
}
return usr, nil
}
func (ss *sqlStore) Delete(ctx context.Context, userID int64) error {
err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
var rawSQL = "DELETE FROM " + ss.dialect.Quote("user") + " WHERE id = ?"
_, err := sess.Exec(rawSQL, userID)
return err
})
if err != nil {
return err
}
return nil
}
func (ss *sqlStore) GetNotServiceAccount(ctx context.Context, userID int64) (*user.User, error) {
user := user.User{ID: userID}
err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
has, err := sess.Where(ss.notServiceAccountFilter()).Get(&user)
if err != nil {
return err
}
if !has {
return models.ErrUserNotFound
}
return nil
})
return &user, err
}
func (ss *sqlStore) notServiceAccountFilter() string {
return fmt.Sprintf("%s.is_service_account = %s",
ss.dialect.Quote("user"),
ss.dialect.BooleanStr(false))
}

View File

@ -3,27 +3,62 @@ package userimpl
import (
"context"
"errors"
"fmt"
"time"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/org"
pref "github.com/grafana/grafana/pkg/services/preference"
"github.com/grafana/grafana/pkg/services/quota"
"github.com/grafana/grafana/pkg/services/sqlstore/db"
"github.com/grafana/grafana/pkg/services/star"
"github.com/grafana/grafana/pkg/services/teamguardian"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/userauth"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/util"
"golang.org/x/sync/errgroup"
)
type Service struct {
store store
orgService org.Service
store store
orgService org.Service
starService star.Service
dashboardService dashboards.DashboardService
preferenceService pref.Service
teamMemberService teamguardian.TeamGuardian
userAuthService userauth.Service
quotaService quota.Service
accessControlStore accesscontrol.AccessControl
}
func ProvideService(db db.DB, orgService org.Service) user.Service {
func ProvideService(
db db.DB,
orgService org.Service,
starService star.Service,
dashboardService dashboards.DashboardService,
preferenceService pref.Service,
teamMemberService teamguardian.TeamGuardian,
userAuthService userauth.Service,
quotaService quota.Service,
accessControlStore accesscontrol.AccessControl,
) user.Service {
return &Service{
store: &sqlStore{
db: db,
db: db,
dialect: db.GetDialect(),
},
orgService: orgService,
orgService: orgService,
starService: starService,
dashboardService: dashboardService,
preferenceService: preferenceService,
teamMemberService: teamMemberService,
userAuthService: userAuthService,
quotaService: quotaService,
accessControlStore: accessControlStore,
}
}
@ -88,7 +123,7 @@ func (s *Service) Create(ctx context.Context, cmd *user.CreateUserCommand) (*use
usr.Password = encodedPassword
}
_, err = s.store.Insert(ctx, usr)
userID, err := s.store.Insert(ctx, usr)
if err != nil {
return nil, err
}
@ -112,10 +147,82 @@ func (s *Service) Create(ctx context.Context, cmd *user.CreateUserCommand) (*use
}
_, err = s.orgService.InsertOrgUser(ctx, &orgUser)
if err != nil {
// HERE ADD DELETE USER
err := s.store.Delete(ctx, userID)
return usr, err
}
}
return usr, nil
}
func (s *Service) Delete(ctx context.Context, cmd *user.DeleteUserCommand) error {
_, err := s.store.GetNotServiceAccount(ctx, cmd.UserID)
if err != nil {
return fmt.Errorf("failed to get user with not service account: %w", err)
}
// delete from all the stores
if err := s.store.Delete(ctx, cmd.UserID); err != nil {
return err
}
g, ctx := errgroup.WithContext(ctx)
g.Go(func() error {
if err := s.starService.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.orgService.DeleteUserFromAll(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.dashboardService.DeleteACLByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.preferenceService.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.teamMemberService.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.userAuthService.Delete(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.userAuthService.DeleteToken(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.quotaService.DeleteByUser(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
g.Go(func() error {
if err := s.accessControlStore.DeleteUserPermissions(ctx, cmd.UserID); err != nil {
return err
}
return nil
})
if err := g.Wait(); err != nil {
return err
}
return nil
}

View File

@ -2,30 +2,88 @@ package userimpl
import (
"context"
"errors"
"testing"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/org/orgtest"
"github.com/grafana/grafana/pkg/services/preference/preftest"
"github.com/grafana/grafana/pkg/services/quota/quotatest"
"github.com/grafana/grafana/pkg/services/star/startest"
"github.com/grafana/grafana/pkg/services/teamguardian/manager"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/userauth/userauthtest"
"github.com/stretchr/testify/require"
)
func TestUserService(t *testing.T) {
userStore := newUserStoreFake()
orgService := orgtest.NewOrgServiceFake()
starService := startest.NewStarServiceFake()
dashboardService := dashboards.NewFakeDashboardService(t)
preferenceService := preftest.NewPreferenceServiceFake()
teamMemberService := manager.NewTeamGuardianMock()
userAuthService := userauthtest.NewFakeUserAuthService()
quotaService := quotatest.NewQuotaServiceFake()
accessControlStore := mock.New()
userService := Service{
store: userStore,
orgService: orgService,
store: userStore,
orgService: orgService,
starService: starService,
dashboardService: dashboardService,
preferenceService: preferenceService,
teamMemberService: teamMemberService,
userAuthService: userAuthService,
quotaService: quotaService,
accessControlStore: accessControlStore,
}
t.Run("create user", func(t *testing.T) {
_, err := userService.Create(context.Background(), &user.CreateUserCommand{})
require.NoError(t, err)
})
t.Run("delete user store returns error", func(t *testing.T) {
userStore.ExpectedDeleteUserError = models.ErrUserNotFound
t.Cleanup(func() {
userStore.ExpectedDeleteUserError = nil
})
err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
require.Error(t, err, models.ErrUserNotFound)
})
t.Run("delete user returns from team", func(t *testing.T) {
teamMemberService.ExpectedError = errors.New("some error")
t.Cleanup(func() {
teamMemberService.ExpectedError = nil
})
err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
require.Error(t, err)
})
t.Run("delete user returns from team and pref", func(t *testing.T) {
teamMemberService.ExpectedError = errors.New("some error")
preferenceService.ExpectedError = errors.New("some error 2")
t.Cleanup(func() {
teamMemberService.ExpectedError = nil
preferenceService.ExpectedError = nil
})
err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
require.Error(t, err)
})
t.Run("delete user successfully", func(t *testing.T) {
err := userService.Delete(context.Background(), &user.DeleteUserCommand{UserID: 1})
require.NoError(t, err)
})
}
type FakeUserStore struct {
ExpectedUser *user.User
ExpectedError error
ExpectedUser *user.User
ExpectedError error
ExpectedDeleteUserError error
}
func newUserStoreFake() *FakeUserStore {
@ -39,3 +97,11 @@ func (f *FakeUserStore) Get(ctx context.Context, query *user.User) (*user.User,
func (f *FakeUserStore) Insert(ctx context.Context, query *user.User) (int64, error) {
return 0, f.ExpectedError
}
func (f *FakeUserStore) Delete(ctx context.Context, userID int64) error {
return f.ExpectedDeleteUserError
}
func (f *FakeUserStore) GetNotServiceAccount(ctx context.Context, userID int64) (*user.User, error) {
return f.ExpectedUser, f.ExpectedError
}

View File

@ -18,3 +18,7 @@ func NewUserServiceFake() *FakeUserService {
func (f *FakeUserService) Create(ctx context.Context, cmd *user.CreateUserCommand) (*user.User, error) {
return f.ExpectedUser, f.ExpectedError
}
func (f *FakeUserService) Delete(ctx context.Context, cmd *user.DeleteUserCommand) error {
return f.ExpectedError
}

View File

@ -3,6 +3,6 @@ package userauth
import "context"
type Service interface {
Delete(ctx context.Context, userID int64) error
DeleteToken(ctx context.Context, userID int64) error
Delete(context.Context, int64) error
DeleteToken(context.Context, int64) error
}

View File

@ -0,0 +1,19 @@
package userauthtest
import "context"
type FakeUserAuthService struct {
ExpectedError error
}
func NewFakeUserAuthService() *FakeUserAuthService {
return &FakeUserAuthService{}
}
func (f *FakeUserAuthService) Delete(ctx context.Context, userID int64) error {
return f.ExpectedError
}
func (f *FakeUserAuthService) DeleteToken(ctx context.Context, userID int64) error {
return f.ExpectedError
}