From a4a53077228368241cb3c3b27653b57dd03d13b1 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Tue, 29 Nov 2022 10:20:44 +0100 Subject: [PATCH] Login: Remove CreateUser from LoginService (#59464) * LoginService: remove create user and use user.Service instead * Fix tests --- pkg/api/admin_users.go | 2 +- pkg/api/admin_users_test.go | 26 +++++++---------- pkg/api/org_invite.go | 2 +- pkg/api/signup.go | 2 +- pkg/services/login/login.go | 1 - .../login/loginservice/loginservice.go | 22 ++++---------- .../login/loginservice/loginservice_mock.go | 29 ++----------------- pkg/services/login/logintest/logintest.go | 3 -- 8 files changed, 22 insertions(+), 65 deletions(-) diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 54855e7d281..9510e1e575c 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -65,7 +65,7 @@ func (hs *HTTPServer) AdminCreateUser(c *models.ReqContext) response.Response { return response.Error(400, "Password is missing or too short", nil) } - usr, err := hs.Login.CreateUser(cmd) + usr, err := hs.userService.Create(c.Req.Context(), &cmd) if err != nil { if errors.Is(err, models.ErrOrgNotFound) { return response.Error(400, err.Error(), nil) diff --git a/pkg/api/admin_users_test.go b/pkg/api/admin_users_test.go index 3de90077df9..5f26ffd63f7 100644 --- a/pkg/api/admin_users_test.go +++ b/pkg/api/admin_users_test.go @@ -14,7 +14,6 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/auth/authtest" - "github.com/grafana/grafana/pkg/services/login/loginservice" "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -168,8 +167,8 @@ func TestAdminAPIEndpoint(t *testing.T) { Login: testLogin, Password: testPassword, } - - adminCreateUserScenario(t, "Should create the user", "/api/admin/users", "/api/admin/users", createCmd, func(sc *scenarioContext) { + usrSvc := &usertest.FakeUserService{ExpectedUser: &user.User{ID: testUserID}} + adminCreateUserScenario(t, "Should create the user", "/api/admin/users", "/api/admin/users", createCmd, usrSvc, func(sc *scenarioContext) { sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) @@ -186,8 +185,8 @@ func TestAdminAPIEndpoint(t *testing.T) { Password: testPassword, OrgId: testOrgID, } - - adminCreateUserScenario(t, "Should create the user", "/api/admin/users", "/api/admin/users", createCmd, func(sc *scenarioContext) { + usrSvc := &usertest.FakeUserService{ExpectedUser: &user.User{ID: testUserID}} + adminCreateUserScenario(t, "Should create the user", "/api/admin/users", "/api/admin/users", createCmd, usrSvc, func(sc *scenarioContext) { sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) @@ -204,8 +203,8 @@ func TestAdminAPIEndpoint(t *testing.T) { Password: testPassword, OrgId: nonExistingOrgID, } - - adminCreateUserScenario(t, "Should create the user", "/api/admin/users", "/api/admin/users", createCmd, func(sc *scenarioContext) { + usrSvc := &usertest.FakeUserService{ExpectedError: models.ErrOrgNotFound} + adminCreateUserScenario(t, "Should create the user", "/api/admin/users", "/api/admin/users", createCmd, usrSvc, func(sc *scenarioContext) { sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 400, sc.resp.Code) @@ -221,8 +220,8 @@ func TestAdminAPIEndpoint(t *testing.T) { Login: existingTestLogin, Password: testPassword, } - - adminCreateUserScenario(t, "Should return an error", "/api/admin/users", "/api/admin/users", createCmd, func(sc *scenarioContext) { + usrSvc := &usertest.FakeUserService{ExpectedError: user.ErrUserAlreadyExists} + adminCreateUserScenario(t, "Should return an error", "/api/admin/users", "/api/admin/users", createCmd, usrSvc, func(sc *scenarioContext) { sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 412, sc.resp.Code) @@ -397,15 +396,10 @@ func adminDeleteUserScenario(t *testing.T, desc string, url string, routePattern }) } -func adminCreateUserScenario(t *testing.T, desc string, url string, routePattern string, cmd dtos.AdminCreateUserForm, fn scenarioFunc) { +func adminCreateUserScenario(t *testing.T, desc string, url string, routePattern string, cmd dtos.AdminCreateUserForm, svc *usertest.FakeUserService, fn scenarioFunc) { t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { hs := HTTPServer{ - Login: loginservice.LoginServiceMock{ - ExpectedUserForm: cmd, - NoExistingOrgId: nonExistingOrgID, - AlreadyExitingLogin: existingTestLogin, - GeneratedUserId: testUserID, - }, + userService: svc, } sc := setupScenarioContext(t, url) diff --git a/pkg/api/org_invite.go b/pkg/api/org_invite.go index 0f4eecc4522..9a1b7610dce 100644 --- a/pkg/api/org_invite.go +++ b/pkg/api/org_invite.go @@ -259,7 +259,7 @@ func (hs *HTTPServer) CompleteInvite(c *models.ReqContext) response.Response { SkipOrgSetup: true, } - usr, err := hs.Login.CreateUser(cmd) + usr, err := hs.userService.Create(c.Req.Context(), &cmd) if err != nil { if errors.Is(err, user.ErrUserAlreadyExists) { return response.Error(412, fmt.Sprintf("User with email '%s' or username '%s' already exists", completeInvite.Email, completeInvite.Username), err) diff --git a/pkg/api/signup.go b/pkg/api/signup.go index c1cd5b5bd57..a654b8e5e22 100644 --- a/pkg/api/signup.go +++ b/pkg/api/signup.go @@ -102,7 +102,7 @@ func (hs *HTTPServer) SignUpStep2(c *models.ReqContext) response.Response { createUserCmd.EmailVerified = true } - usr, err := hs.Login.CreateUser(createUserCmd) + usr, err := hs.userService.Create(c.Req.Context(), &createUserCmd) if err != nil { if errors.Is(err, user.ErrUserAlreadyExists) { return response.Error(401, "User with same email address already exists", nil) diff --git a/pkg/services/login/login.go b/pkg/services/login/login.go index c2ee63df7c3..bb1ff508e4a 100644 --- a/pkg/services/login/login.go +++ b/pkg/services/login/login.go @@ -18,7 +18,6 @@ var ( type TeamSyncFunc func(user *user.User, externalUser *models.ExternalUserInfo) error type Service interface { - CreateUser(cmd user.CreateUserCommand) (*user.User, error) UpsertUser(ctx context.Context, cmd *models.UpsertUserCommand) error DisableExternalUser(ctx context.Context, username string) error SetTeamSyncFunc(TeamSyncFunc) diff --git a/pkg/services/login/loginservice/loginservice.go b/pkg/services/login/loginservice/loginservice.go index 44323ead976..b32129b0578 100644 --- a/pkg/services/login/loginservice/loginservice.go +++ b/pkg/services/login/loginservice/loginservice.go @@ -43,11 +43,6 @@ type Implementation struct { orgService org.Service } -// CreateUser creates inserts a new one. -func (ls *Implementation) CreateUser(cmd user.CreateUserCommand) (*user.User, error) { - return ls.userService.Create(context.Background(), &cmd) -} - // UpsertUser updates an existing user, or if it doesn't exist, inserts a new one. func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUserCommand) error { extUser := cmd.ExternalUser @@ -80,7 +75,12 @@ func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUser } } - result, errCreateUser := ls.createUser(extUser) + result, errCreateUser := ls.userService.Create(ctx, &user.CreateUserCommand{ + Login: extUser.Login, + Email: extUser.Email, + Name: extUser.Name, + SkipOrgSetup: len(extUser.OrgRoles) > 0, + }) if errCreateUser != nil { return errCreateUser } @@ -207,16 +207,6 @@ func (ls *Implementation) SetTeamSyncFunc(teamSyncFunc login.TeamSyncFunc) { ls.TeamSync = teamSyncFunc } -func (ls *Implementation) createUser(extUser *models.ExternalUserInfo) (*user.User, error) { - cmd := user.CreateUserCommand{ - Login: extUser.Login, - Email: extUser.Email, - Name: extUser.Name, - SkipOrgSetup: len(extUser.OrgRoles) > 0, - } - return ls.CreateUser(cmd) -} - func (ls *Implementation) updateUser(ctx context.Context, usr *user.User, extUser *models.ExternalUserInfo) error { // sync user info updateCmd := &user.UpdateUserCommand{ diff --git a/pkg/services/login/loginservice/loginservice_mock.go b/pkg/services/login/loginservice/loginservice_mock.go index 99348babc41..b77ac1b4caa 100644 --- a/pkg/services/login/loginservice/loginservice_mock.go +++ b/pkg/services/login/loginservice/loginservice_mock.go @@ -2,9 +2,7 @@ package loginservice import ( "context" - "errors" - "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/user" @@ -12,30 +10,9 @@ import ( type LoginServiceMock struct { login.Service - ExpectedUserForm dtos.AdminCreateUserForm - NoExistingOrgId int64 - AlreadyExitingLogin string - GeneratedUserId int64 - ExpectedUser *user.User - ExpectedUserFunc func(cmd *models.UpsertUserCommand) *user.User - ExpectedError error -} - -func (s LoginServiceMock) CreateUser(cmd user.CreateUserCommand) (*user.User, error) { - if cmd.OrgID == s.NoExistingOrgId { - return nil, models.ErrOrgNotFound - } - - if cmd.Login == s.AlreadyExitingLogin { - return nil, user.ErrUserAlreadyExists - } - - if s.ExpectedUserForm.Login == cmd.Login && s.ExpectedUserForm.Email == cmd.Email && - s.ExpectedUserForm.Password == cmd.Password && s.ExpectedUserForm.Name == cmd.Name && s.ExpectedUserForm.OrgId == cmd.OrgID { - return &user.User{ID: s.GeneratedUserId}, nil - } - - return nil, errors.New("unexpected cmd") + ExpectedUser *user.User + ExpectedUserFunc func(cmd *models.UpsertUserCommand) *user.User + ExpectedError error } func (s LoginServiceMock) UpsertUser(ctx context.Context, cmd *models.UpsertUserCommand) error { diff --git a/pkg/services/login/logintest/logintest.go b/pkg/services/login/logintest/logintest.go index 023ae063f13..6eda3dd1d14 100644 --- a/pkg/services/login/logintest/logintest.go +++ b/pkg/services/login/logintest/logintest.go @@ -10,9 +10,6 @@ import ( type LoginServiceFake struct{} -func (l *LoginServiceFake) CreateUser(cmd user.CreateUserCommand) (*user.User, error) { - return nil, nil -} func (l *LoginServiceFake) UpsertUser(ctx context.Context, cmd *models.UpsertUserCommand) error { return nil }