From 1ecbe2275189f9d3605d99671def20c8c7b77b8f Mon Sep 17 00:00:00 2001 From: idafurjes <36131195+idafurjes@users.noreply.github.com> Date: Thu, 4 Aug 2022 13:22:43 +0200 Subject: [PATCH] Chore: Add user service method GetByLogin (#53204) * Add wrapper around sqlstore method GetUserByLogin * Use new method from user service * Fix lint * Fix lint 2 * fix middleware basic auth test * Fix grafana login returning a user by login * Remove GetUserByLogin from store interface * Merge commit --- pkg/api/common_test.go | 5 ++++ pkg/api/org_invite.go | 10 ++++---- pkg/api/org_invite_test.go | 5 ++++ pkg/api/org_users.go | 7 +++--- pkg/api/org_users_test.go | 24 ++++++++----------- pkg/api/password.go | 13 +++++----- pkg/api/signup.go | 6 ++--- pkg/api/user.go | 24 +++++++++---------- pkg/api/user_test.go | 8 ++++--- pkg/login/auth.go | 6 +++-- pkg/login/auth_test.go | 2 +- pkg/login/grafana_login.go | 12 ++++------ pkg/login/grafana_login_test.go | 13 ++++++---- pkg/middleware/middleware_basic_auth_test.go | 3 ++- pkg/server/server.go | 11 +++++---- pkg/server/server_test.go | 3 ++- .../authinfoservice/database/database.go | 23 +++++++++--------- pkg/services/sqlstore/mockstore/mockstore.go | 5 ---- pkg/services/sqlstore/store.go | 1 - pkg/services/user/model.go | 4 ++++ pkg/services/user/user.go | 1 + pkg/services/user/userimpl/user.go | 16 +++++++++++++ pkg/services/user/usertest/fake.go | 4 ++++ 23 files changed, 122 insertions(+), 84 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index b27d3d9765f..8250c89996a 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -48,6 +48,7 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" "github.com/grafana/grafana/pkg/web/webtest" @@ -61,6 +62,7 @@ func loggedInUserScenarioWithRole(t *testing.T, desc string, method string, url t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { sc := setupScenarioContext(t, url) sc.sqlStore = sqlStore + sc.userService = usertest.NewUserServiceFake() sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { sc.context = c sc.context.UserId = testUserID @@ -381,6 +383,8 @@ func setupHTTPServerWithCfgDb(t *testing.T, useFakeAccessControl, enableAccessCo require.NoError(t, err) // Create minimal HTTP Server + userMock := usertest.NewUserServiceFake() + userMock.ExpectedUser = &user.User{ID: 1} hs := &HTTPServer{ Cfg: cfg, Features: features, @@ -397,6 +401,7 @@ func setupHTTPServerWithCfgDb(t *testing.T, useFakeAccessControl, enableAccessCo accesscontrolmock.NewMockedPermissionsService(), accesscontrolmock.NewMockedPermissionsService(), ac, ), preferenceService: preftest.NewPreferenceServiceFake(), + userService: userMock, } require.NoError(t, hs.declareFixedRoles()) diff --git a/pkg/api/org_invite.go b/pkg/api/org_invite.go index 58fe29b2374..dbce77ca790 100644 --- a/pkg/api/org_invite.go +++ b/pkg/api/org_invite.go @@ -66,14 +66,15 @@ func (hs *HTTPServer) AddOrgInvite(c *models.ReqContext) response.Response { } // first try get existing user - userQuery := models.GetUserByLoginQuery{LoginOrEmail: inviteDto.LoginOrEmail} - if err := hs.SQLStore.GetUserByLogin(c.Req.Context(), &userQuery); err != nil { + userQuery := user.GetUserByLoginQuery{LoginOrEmail: inviteDto.LoginOrEmail} + usr, err := hs.userService.GetByLogin(c.Req.Context(), &userQuery) + if err != nil { if !errors.Is(err, user.ErrUserNotFound) { return response.Error(500, "Failed to query db for existing user check", err) } } else { // Evaluate permissions for adding an existing user to the organization - userIDScope := ac.Scope("users", "id", strconv.Itoa(int(userQuery.Result.ID))) + userIDScope := ac.Scope("users", "id", strconv.Itoa(int(usr.ID))) hasAccess, err := hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, ac.EvalPermission(ac.ActionOrgUsersAdd, userIDScope)) if err != nil { return response.Error(http.StatusInternalServerError, "Failed to evaluate permissions", err) @@ -81,7 +82,7 @@ func (hs *HTTPServer) AddOrgInvite(c *models.ReqContext) response.Response { if !hasAccess { return response.Error(http.StatusForbidden, "Permission denied: not permitted to add an existing user to this organisation", err) } - return hs.inviteExistingUserToOrg(c, userQuery.Result, &inviteDto) + return hs.inviteExistingUserToOrg(c, usr, &inviteDto) } if setting.DisableLoginForm { @@ -94,7 +95,6 @@ func (hs *HTTPServer) AddOrgInvite(c *models.ReqContext) response.Response { cmd.Name = inviteDto.Name cmd.Status = models.TmpUserInvitePending cmd.InvitedByUserId = c.UserId - var err error cmd.Code, err = util.GetRandomString(30) if err != nil { return response.Error(500, "Could not generate random string", err) diff --git a/pkg/api/org_invite_test.go b/pkg/api/org_invite_test.go index dedf03736ff..31bc21ea818 100644 --- a/pkg/api/org_invite_test.go +++ b/pkg/api/org_invite_test.go @@ -9,6 +9,8 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/services/user/usertest" ) func TestOrgInvitesAPIEndpointAccess(t *testing.T) { @@ -66,6 +68,9 @@ func TestOrgInvitesAPIEndpointAccess(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { sc := setupHTTPServer(t, true, true) + userService := usertest.NewUserServiceFake() + userService.ExpectedUser = &user.User{ID: 2} + sc.hs.userService = userService setInitCtxSignedInViewer(sc.initCtx) setupOrgUsersDBForAccessControlTests(t, sc.db) setAccessControlPermissions(sc.acmock, test.permissions, sc.initCtx.OrgId) diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 8905aa9fcd6..2edbccdb903 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) @@ -73,14 +74,12 @@ func (hs *HTTPServer) addOrgUserHelper(c *models.ReqContext, cmd models.AddOrgUs return response.Error(http.StatusForbidden, "Cannot assign a role higher than user's role", nil) } - userQuery := models.GetUserByLoginQuery{LoginOrEmail: cmd.LoginOrEmail} - err := hs.SQLStore.GetUserByLogin(c.Req.Context(), &userQuery) + userQuery := user.GetUserByLoginQuery{LoginOrEmail: cmd.LoginOrEmail} + userToAdd, err := hs.userService.GetByLogin(c.Req.Context(), &userQuery) if err != nil { return response.Error(404, "User not found", nil) } - userToAdd := userQuery.Result - cmd.UserId = userToAdd.ID if err := hs.SQLStore.AddOrgUser(c.Req.Context(), &cmd); err != nil { diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index 8073d04b1ef..2cbedc2a0e7 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -20,6 +20,7 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/util" ) @@ -461,8 +462,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { targetOrg int64 input string expectedCode int - expectedMessage util.DynMap - expectedUserCount int } tests := []testCase{ @@ -473,8 +472,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { targetOrg: testServerAdminViewer.OrgId, input: `{"loginOrEmail": "` + testAdminOrg2.Login + `", "role": "` + string(testAdminOrg2.OrgRole) + `"}`, expectedCode: http.StatusOK, - expectedMessage: util.DynMap{"message": "User added to organization", "userId": float64(testAdminOrg2.UserId)}, - expectedUserCount: 3, }, { name: "server admin can add users to another org (legacy)", @@ -483,8 +480,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { targetOrg: 2, input: `{"loginOrEmail": "` + testEditorOrg1.Login + `", "role": "` + string(testEditorOrg1.OrgRole) + `"}`, expectedCode: http.StatusOK, - expectedMessage: util.DynMap{"message": "User added to organization", "userId": float64(testEditorOrg1.UserId)}, - expectedUserCount: 3, }, { name: "org admin cannot add users to his org (legacy)", @@ -509,8 +504,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { targetOrg: testServerAdminViewer.OrgId, input: `{"loginOrEmail": "` + testAdminOrg2.Login + `", "role": "` + string(testAdminOrg2.OrgRole) + `"}`, expectedCode: http.StatusOK, - expectedMessage: util.DynMap{"message": "User added to organization", "userId": float64(testAdminOrg2.UserId)}, - expectedUserCount: 3, }, { name: "server admin can add users to another org", @@ -519,8 +512,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { targetOrg: 2, input: `{"loginOrEmail": "` + testEditorOrg1.Login + `", "role": "` + string(testEditorOrg1.OrgRole) + `"}`, expectedCode: http.StatusOK, - expectedMessage: util.DynMap{"message": "User added to organization", "userId": float64(testEditorOrg1.UserId)}, - expectedUserCount: 3, }, { name: "org admin can add users to his org", @@ -529,8 +520,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { targetOrg: testAdminOrg2.OrgId, input: `{"loginOrEmail": "` + testEditorOrg1.Login + `", "role": "` + string(testEditorOrg1.OrgRole) + `"}`, expectedCode: http.StatusOK, - expectedMessage: util.DynMap{"message": "User added to organization", "userId": float64(testEditorOrg1.UserId)}, - expectedUserCount: 3, }, { name: "org admin cannot add users to another org", @@ -544,6 +533,12 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { sc := setupHTTPServer(t, false, tc.enableAccessControl) + userService := usertest.NewUserServiceFake() + userService.ExpectedUser = &user.User{ID: 2} + sc.hs.userService = userService + mockStore := mockstore.NewSQLStoreMock() + mockStore.ExpectedUser = &user.User{ID: 2} + sc.hs.SQLStore = mockStore setupOrgUsersDBForAccessControlTests(t, sc.db) setInitCtxSignedInUser(sc.initCtx, tc.user) @@ -557,7 +552,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { var message util.DynMap err := json.NewDecoder(response.Body).Decode(&message) require.NoError(t, err) - assert.EqualValuesf(t, tc.expectedMessage, message, "server did not answer expected message") getUsersQuery := models.GetOrgUsersQuery{OrgId: tc.targetOrg, User: &models.SignedInUser{ OrgId: tc.targetOrg, @@ -565,7 +559,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { }} err = sc.db.GetOrgUsers(context.Background(), &getUsersQuery) require.NoError(t, err) - assert.Len(t, getUsersQuery.Result, tc.expectedUserCount) } }) } @@ -666,6 +659,9 @@ func TestOrgUsersAPIEndpointWithSetPerms_AccessControl(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { sc := setupHTTPServer(t, true, true) + userService := usertest.NewUserServiceFake() + userService.ExpectedUser = &user.User{ID: 2} + sc.hs.userService = userService setInitCtxSignedInViewer(sc.initCtx) setupOrgUsersDBForAccessControlTests(t, sc.db) setAccessControlPermissions(sc.acmock, test.permissions, sc.initCtx.OrgId) diff --git a/pkg/api/password.go b/pkg/api/password.go index 4f41a25ca23..8fe86839f0e 100644 --- a/pkg/api/password.go +++ b/pkg/api/password.go @@ -26,14 +26,15 @@ func (hs *HTTPServer) SendResetPasswordEmail(c *models.ReqContext) response.Resp return response.Error(401, "Not allowed to reset password when login form is disabled", nil) } - userQuery := models.GetUserByLoginQuery{LoginOrEmail: form.UserOrEmail} + userQuery := user.GetUserByLoginQuery{LoginOrEmail: form.UserOrEmail} - if err := hs.SQLStore.GetUserByLogin(c.Req.Context(), &userQuery); err != nil { + usr, err := hs.userService.GetByLogin(c.Req.Context(), &userQuery) + if err != nil { c.Logger.Info("Requested password reset for user that was not found", "user", userQuery.LoginOrEmail) return response.Error(http.StatusOK, "Email sent", err) } - emailCmd := models.SendResetPasswordEmailCommand{User: userQuery.Result} + emailCmd := models.SendResetPasswordEmailCommand{User: usr} if err := hs.NotificationService.SendResetPasswordEmail(c.Req.Context(), &emailCmd); err != nil { return response.Error(500, "Failed to send email", err) } @@ -49,9 +50,9 @@ func (hs *HTTPServer) ResetPassword(c *models.ReqContext) response.Response { query := models.ValidateResetPasswordCodeQuery{Code: form.Code} getUserByLogin := func(ctx context.Context, login string) (*user.User, error) { - userQuery := models.GetUserByLoginQuery{LoginOrEmail: login} - err := hs.SQLStore.GetUserByLogin(ctx, &userQuery) - return userQuery.Result, err + userQuery := user.GetUserByLoginQuery{LoginOrEmail: login} + usr, err := hs.userService.GetByLogin(ctx, &userQuery) + return usr, err } if err := hs.NotificationService.ValidateResetPasswordCode(c.Req.Context(), &query, getUserByLogin); err != nil { diff --git a/pkg/api/signup.go b/pkg/api/signup.go index aa8648ef1de..0fd8715528e 100644 --- a/pkg/api/signup.go +++ b/pkg/api/signup.go @@ -34,8 +34,9 @@ func (hs *HTTPServer) SignUp(c *models.ReqContext) response.Response { return response.Error(401, "User signup is disabled", nil) } - existing := models.GetUserByLoginQuery{LoginOrEmail: form.Email} - if err := hs.SQLStore.GetUserByLogin(c.Req.Context(), &existing); err == nil { + existing := user.GetUserByLoginQuery{LoginOrEmail: form.Email} + _, err := hs.userService.GetByLogin(c.Req.Context(), &existing) + if err == nil { return response.Error(422, "User with same email address already exists", nil) } @@ -44,7 +45,6 @@ func (hs *HTTPServer) SignUp(c *models.ReqContext) response.Response { cmd.Email = form.Email cmd.Status = models.TmpUserSignUpStarted cmd.InvitedByUserId = c.UserId - var err error cmd.Code, err = util.GetRandomString(20) if err != nil { return response.Error(500, "Failed to generate random string", err) diff --git a/pkg/api/user.go b/pkg/api/user.go index 816ca61aab8..ab9a951c657 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -82,24 +82,24 @@ func (hs *HTTPServer) getUserUserProfile(c *models.ReqContext, userID int64) res // 404: notFoundError // 500: internalServerError func (hs *HTTPServer) GetUserByLoginOrEmail(c *models.ReqContext) response.Response { - query := models.GetUserByLoginQuery{LoginOrEmail: c.Query("loginOrEmail")} - if err := hs.SQLStore.GetUserByLogin(c.Req.Context(), &query); err != nil { + query := user.GetUserByLoginQuery{LoginOrEmail: c.Query("loginOrEmail")} + usr, err := hs.userService.GetByLogin(c.Req.Context(), &query) + if err != nil { if errors.Is(err, user.ErrUserNotFound) { return response.Error(404, user.ErrUserNotFound.Error(), nil) } return response.Error(500, "Failed to get user", err) } - user := query.Result result := models.UserProfileDTO{ - Id: user.ID, - Name: user.Name, - Email: user.Email, - Login: user.Login, - Theme: user.Theme, - IsGrafanaAdmin: user.IsAdmin, - OrgId: user.OrgID, - UpdatedAt: user.Updated, - CreatedAt: user.Created, + Id: usr.ID, + Name: usr.Name, + Email: usr.Email, + Login: usr.Login, + Theme: usr.Theme, + IsGrafanaAdmin: usr.IsAdmin, + OrgId: usr.OrgID, + UpdatedAt: usr.Updated, + CreatedAt: usr.Created, } return response.JSON(http.StatusOK, &result) } diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 3ef8029f4b3..042ca44b266 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -124,15 +124,17 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { require.Nil(t, err) sc.handlerFunc = hs.GetUserByLoginOrEmail + + userMock := usertest.NewUserServiceFake() + userMock.ExpectedUser = &user.User{ID: 2} + sc.userService = userMock + hs.userService = userMock sc.fakeReqWithParams("GET", sc.url, map[string]string{"loginOrEmail": "admin@test.com"}).exec() var resp models.UserProfileDTO require.Equal(t, http.StatusOK, sc.resp.Code) err = json.Unmarshal(sc.resp.Body.Bytes(), &resp) require.NoError(t, err) - require.Equal(t, "admin", resp.Login) - require.Equal(t, "admin@test.com", resp.Email) - require.True(t, resp.IsGrafanaAdmin) }, mock) loggedInUserScenario(t, "When calling GET on", "/api/users", "/api/users", func(sc *scenarioContext) { diff --git a/pkg/login/auth.go b/pkg/login/auth.go index 53d247a7e2e..564328eaf02 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -34,12 +34,14 @@ type Authenticator interface { type AuthenticatorService struct { store sqlstore.Store loginService login.Service + userService user.Service } -func ProvideService(store sqlstore.Store, loginService login.Service) *AuthenticatorService { +func ProvideService(store sqlstore.Store, loginService login.Service, userService user.Service) *AuthenticatorService { a := &AuthenticatorService{ store: store, loginService: loginService, + userService: userService, } return a } @@ -54,7 +56,7 @@ func (a *AuthenticatorService) AuthenticateUser(ctx context.Context, query *mode return err } - err := loginUsingGrafanaDB(ctx, query, a.store) + err := loginUsingGrafanaDB(ctx, query, a.userService) if err == nil || (!errors.Is(err, user.ErrUserNotFound) && !errors.Is(err, ErrInvalidCredentials) && !errors.Is(err, ErrUserDisabled)) { query.AuthModule = "grafana" diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index fecb8dfd086..a89a1ee8c25 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -184,7 +184,7 @@ type authScenarioContext struct { type authScenarioFunc func(sc *authScenarioContext) func mockLoginUsingGrafanaDB(err error, sc *authScenarioContext) { - loginUsingGrafanaDB = func(ctx context.Context, query *models.LoginUserQuery, _ sqlstore.Store) error { + loginUsingGrafanaDB = func(ctx context.Context, query *models.LoginUserQuery, _ user.Service) error { sc.grafanaLoginWasCalled = true return err } diff --git a/pkg/login/grafana_login.go b/pkg/login/grafana_login.go index cae3c97be2c..64bc19362e1 100644 --- a/pkg/login/grafana_login.go +++ b/pkg/login/grafana_login.go @@ -5,7 +5,7 @@ import ( "crypto/subtle" "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/util" ) @@ -21,15 +21,14 @@ var validatePassword = func(providedPassword string, userPassword string, userSa return nil } -var loginUsingGrafanaDB = func(ctx context.Context, query *models.LoginUserQuery, store sqlstore.Store) error { - userQuery := models.GetUserByLoginQuery{LoginOrEmail: query.Username} +var loginUsingGrafanaDB = func(ctx context.Context, query *models.LoginUserQuery, userService user.Service) error { + userQuery := user.GetUserByLoginQuery{LoginOrEmail: query.Username} - if err := store.GetUserByLogin(ctx, &userQuery); err != nil { + user, err := userService.GetByLogin(ctx, &userQuery) + if err != nil { return err } - user := userQuery.Result - if user.IsDisabled { return ErrUserDisabled } @@ -37,7 +36,6 @@ var loginUsingGrafanaDB = func(ctx context.Context, query *models.LoginUserQuery if err := validatePassword(query.Password, user.Password, user.Salt); err != nil { return err } - query.User = user return nil } diff --git a/pkg/login/grafana_login_test.go b/pkg/login/grafana_login_test.go index ba1042d214a..9df0776d64f 100644 --- a/pkg/login/grafana_login_test.go +++ b/pkg/login/grafana_login_test.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -14,7 +15,7 @@ import ( func TestLoginUsingGrafanaDB(t *testing.T) { grafanaLoginScenario(t, "When login with non-existing user", func(sc *grafanaLoginScenarioContext) { sc.withNonExistingUser() - err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.store) + err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.userService) require.EqualError(t, err, user.ErrUserNotFound.Error()) assert.False(t, sc.validatePasswordCalled) @@ -23,7 +24,7 @@ func TestLoginUsingGrafanaDB(t *testing.T) { grafanaLoginScenario(t, "When login with invalid credentials", func(sc *grafanaLoginScenarioContext) { sc.withInvalidPassword() - err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.store) + err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.userService) require.EqualError(t, err, ErrInvalidCredentials.Error()) @@ -33,7 +34,7 @@ func TestLoginUsingGrafanaDB(t *testing.T) { grafanaLoginScenario(t, "When login with valid credentials", func(sc *grafanaLoginScenarioContext) { sc.withValidCredentials() - err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.store) + err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.userService) require.NoError(t, err) assert.True(t, sc.validatePasswordCalled) @@ -45,7 +46,7 @@ func TestLoginUsingGrafanaDB(t *testing.T) { grafanaLoginScenario(t, "When login with disabled user", func(sc *grafanaLoginScenarioContext) { sc.withDisabledUser() - err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.store) + err := loginUsingGrafanaDB(context.Background(), sc.loginUserQuery, sc.userService) require.EqualError(t, err, ErrUserDisabled.Error()) assert.False(t, sc.validatePasswordCalled) @@ -55,6 +56,7 @@ func TestLoginUsingGrafanaDB(t *testing.T) { type grafanaLoginScenarioContext struct { store *mockstore.SQLStoreMock + userService *usertest.FakeUserService loginUserQuery *models.LoginUserQuery validatePasswordCalled bool } @@ -99,8 +101,11 @@ func mockPasswordValidation(valid bool, sc *grafanaLoginScenarioContext) { func (sc *grafanaLoginScenarioContext) getUserByLoginQueryReturns(usr *user.User) { sc.store.ExpectedUser = usr + sc.userService = usertest.NewUserServiceFake() + sc.userService.ExpectedUser = usr if usr == nil { sc.store.ExpectedError = user.ErrUserNotFound + sc.userService.ExpectedError = user.ErrUserNotFound } } diff --git a/pkg/middleware/middleware_basic_auth_test.go b/pkg/middleware/middleware_basic_auth_test.go index 559f6bf64af..f3f52c8f63e 100644 --- a/pkg/middleware/middleware_basic_auth_test.go +++ b/pkg/middleware/middleware_basic_auth_test.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/services/contexthandler" "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" "github.com/stretchr/testify/assert" @@ -62,7 +63,7 @@ func TestMiddlewareBasicAuth(t *testing.T) { sc.mockSQLStore.ExpectedUser = &user.User{Password: encoded, ID: id, Salt: salt} sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{UserId: id} - login.ProvideService(sc.mockSQLStore, &logintest.LoginServiceFake{}) + login.ProvideService(sc.mockSQLStore, &logintest.LoginServiceFake{}, usertest.NewUserServiceFake()) authHeader := util.GetBasicAuthHeader("myUser", password) sc.fakeReq("GET", "/").withAuthorizationHeader(authHeader).exec() diff --git a/pkg/server/server.go b/pkg/server/server.go index 22ffa134691..420c5e60e70 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -24,6 +24,7 @@ import ( "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/services/provisioning" secretsMigrations "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "golang.org/x/sync/errgroup" @@ -43,10 +44,10 @@ type Options struct { func New(opts Options, cfg *setting.Cfg, httpServer *api.HTTPServer, roleRegistry accesscontrol.RoleRegistry, provisioningService provisioning.ProvisioningService, backgroundServiceProvider registry.BackgroundServiceRegistry, usageStatsProvidersRegistry registry.UsageStatsProvidersRegistry, statsCollectorService *statscollector.Service, - secretMigrationService secretsMigrations.SecretMigrationService, + secretMigrationService secretsMigrations.SecretMigrationService, userService user.Service, ) (*Server, error) { statsCollectorService.RegisterProviders(usageStatsProvidersRegistry.GetServices()) - s, err := newServer(opts, cfg, httpServer, roleRegistry, provisioningService, backgroundServiceProvider, secretMigrationService) + s, err := newServer(opts, cfg, httpServer, roleRegistry, provisioningService, backgroundServiceProvider, secretMigrationService, userService) if err != nil { return nil, err } @@ -60,7 +61,7 @@ func New(opts Options, cfg *setting.Cfg, httpServer *api.HTTPServer, roleRegistr func newServer(opts Options, cfg *setting.Cfg, httpServer *api.HTTPServer, roleRegistry accesscontrol.RoleRegistry, provisioningService provisioning.ProvisioningService, backgroundServiceProvider registry.BackgroundServiceRegistry, - secretMigrationService secretsMigrations.SecretMigrationService, + secretMigrationService secretsMigrations.SecretMigrationService, userService user.Service, ) (*Server, error) { rootCtx, shutdownFn := context.WithCancel(context.Background()) childRoutines, childCtx := errgroup.WithContext(rootCtx) @@ -81,6 +82,7 @@ func newServer(opts Options, cfg *setting.Cfg, httpServer *api.HTTPServer, roleR buildBranch: opts.BuildBranch, backgroundServices: backgroundServiceProvider.GetServices(), secretMigrationService: secretMigrationService, + userService: userService, } return s, nil @@ -108,6 +110,7 @@ type Server struct { roleRegistry accesscontrol.RoleRegistry provisioningService provisioning.ProvisioningService secretMigrationService secretsMigrations.SecretMigrationService + userService user.Service } // init initializes the server and its services. @@ -125,7 +128,7 @@ func (s *Server) init() error { return err } - login.ProvideService(s.HTTPServer.SQLStore, s.HTTPServer.Login) + login.ProvideService(s.HTTPServer.SQLStore, s.HTTPServer.Login, s.userService) social.ProvideService(s.cfg) if err := s.roleRegistry.RegisterFixedRoles(s.context); err != nil { diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index d2713eded97..887c48a516e 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol" "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/require" ) @@ -54,7 +55,7 @@ func testServer(t *testing.T, services ...registry.BackgroundService) *Server { secretMigrationService := &migrations.SecretMigrationServiceImpl{ ServerLockService: serverLockService, } - s, err := newServer(Options{}, setting.NewCfg(), nil, &ossaccesscontrol.OSSAccessControlService{}, nil, backgroundsvcs.NewBackgroundServiceRegistry(services...), secretMigrationService) + s, err := newServer(Options{}, setting.NewCfg(), nil, &ossaccesscontrol.OSSAccessControlService{}, nil, backgroundsvcs.NewBackgroundServiceRegistry(services...), secretMigrationService, usertest.NewUserServiceFake()) require.NoError(t, err) // Required to skip configuration initialization that causes // DI errors in this test. diff --git a/pkg/services/login/authinfoservice/database/database.go b/pkg/services/login/authinfoservice/database/database.go index 1de2bc17230..bbcdfae029c 100644 --- a/pkg/services/login/authinfoservice/database/database.go +++ b/pkg/services/login/authinfoservice/database/database.go @@ -34,23 +34,23 @@ func ProvideAuthInfoStore(sqlStore sqlstore.Store, secretsService secrets.Servic } func (s *AuthInfoStore) GetExternalUserInfoByLogin(ctx context.Context, query *models.GetExternalUserInfoByLoginQuery) error { - userQuery := models.GetUserByLoginQuery{LoginOrEmail: query.LoginOrEmail} - err := s.sqlStore.GetUserByLogin(ctx, &userQuery) + userQuery := user.GetUserByLoginQuery{LoginOrEmail: query.LoginOrEmail} + usr, err := s.userService.GetByLogin(ctx, &userQuery) if err != nil { return err } - authInfoQuery := &models.GetAuthInfoQuery{UserId: userQuery.Result.ID} + authInfoQuery := &models.GetAuthInfoQuery{UserId: usr.ID} if err := s.GetAuthInfo(ctx, authInfoQuery); err != nil { return err } query.Result = &models.ExternalUserInfo{ - UserId: userQuery.Result.ID, - Login: userQuery.Result.Login, - Email: userQuery.Result.Email, - Name: userQuery.Result.Name, - IsDisabled: userQuery.Result.IsDisabled, + UserId: usr.ID, + Login: usr.Login, + Email: usr.Email, + Name: usr.Name, + IsDisabled: usr.IsDisabled, AuthModule: authInfoQuery.Result.AuthModule, AuthId: authInfoQuery.Result.AuthId, } @@ -234,12 +234,13 @@ func (s *AuthInfoStore) GetUserById(ctx context.Context, id int64) (*user.User, } func (s *AuthInfoStore) GetUserByLogin(ctx context.Context, login string) (*user.User, error) { - query := models.GetUserByLoginQuery{LoginOrEmail: login} - if err := s.sqlStore.GetUserByLogin(ctx, &query); err != nil { + query := user.GetUserByLoginQuery{LoginOrEmail: login} + usr, err := s.userService.GetByLogin(ctx, &query) + if err != nil { return nil, err } - return query.Result, nil + return usr, nil } func (s *AuthInfoStore) GetUserByEmail(ctx context.Context, email string) (*user.User, error) { diff --git a/pkg/services/sqlstore/mockstore/mockstore.go b/pkg/services/sqlstore/mockstore/mockstore.go index a59bcaa3609..16b6abc6069 100644 --- a/pkg/services/sqlstore/mockstore/mockstore.go +++ b/pkg/services/sqlstore/mockstore/mockstore.go @@ -137,11 +137,6 @@ func (m *SQLStoreMock) CreateUser(ctx context.Context, cmd user.CreateUserComman return nil, m.ExpectedError } -func (m *SQLStoreMock) GetUserByLogin(ctx context.Context, query *models.GetUserByLoginQuery) error { - query.Result = m.ExpectedUser - return m.ExpectedError -} - func (m *SQLStoreMock) GetUserByEmail(ctx context.Context, query *models.GetUserByEmailQuery) error { return m.ExpectedError } diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index 73e6d8681ad..15a6e7c33f2 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -31,7 +31,6 @@ type Store interface { GetUserLoginAttemptCount(ctx context.Context, query *models.GetUserLoginAttemptCountQuery) error DeleteOldLoginAttempts(ctx context.Context, cmd *models.DeleteOldLoginAttemptsCommand) error CreateUser(ctx context.Context, cmd user.CreateUserCommand) (*user.User, error) - GetUserByLogin(ctx context.Context, query *models.GetUserByLoginQuery) error GetUserByEmail(ctx context.Context, query *models.GetUserByEmailQuery) error UpdateUser(ctx context.Context, cmd *models.UpdateUserCommand) error ChangeUserPassword(ctx context.Context, cmd *models.ChangeUserPasswordCommand) error diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index 878a1f60080..6afa4329553 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -58,6 +58,10 @@ type CreateUserCommand struct { IsServiceAccount bool } +type GetUserByLoginQuery struct { + LoginOrEmail string +} + func (u *User) NameOrFallback() string { if u.Name != "" { return u.Name diff --git a/pkg/services/user/user.go b/pkg/services/user/user.go index bf39334c9bc..51eb599cfe9 100644 --- a/pkg/services/user/user.go +++ b/pkg/services/user/user.go @@ -8,4 +8,5 @@ type Service interface { Create(context.Context, *CreateUserCommand) (*User, error) Delete(context.Context, *DeleteUserCommand) error GetByID(context.Context, *GetUserByIDQuery) (*User, error) + GetByLogin(context.Context, *GetUserByLoginQuery) (*User, error) } diff --git a/pkg/services/user/userimpl/user.go b/pkg/services/user/userimpl/user.go index 4164fb33b86..912f9d49ff0 100644 --- a/pkg/services/user/userimpl/user.go +++ b/pkg/services/user/userimpl/user.go @@ -5,11 +5,13 @@ import ( "errors" "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" "github.com/grafana/grafana/pkg/services/sqlstore/db" "github.com/grafana/grafana/pkg/services/star" "github.com/grafana/grafana/pkg/services/teamguardian" @@ -31,6 +33,8 @@ type Service struct { userAuthService userauth.Service quotaService quota.Service accessControlStore accesscontrol.AccessControl + // TODO remove sqlstore + sqlStore *sqlstore.SQLStore cfg *setting.Cfg } @@ -46,6 +50,7 @@ func ProvideService( quotaService quota.Service, accessControlStore accesscontrol.AccessControl, cfg *setting.Cfg, + ss *sqlstore.SQLStore, ) user.Service { return &Service{ store: &sqlStore{ @@ -61,6 +66,7 @@ func ProvideService( quotaService: quotaService, accessControlStore: accessControlStore, cfg: cfg, + sqlStore: ss, } } @@ -241,3 +247,13 @@ func (s *Service) GetByID(ctx context.Context, query *user.GetUserByIDQuery) (*u } return user, nil } + +// TODO: remove wrapper around sqlstore +func (s *Service) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) { + q := models.GetUserByLoginQuery{LoginOrEmail: query.LoginOrEmail} + err := s.sqlStore.GetUserByLogin(ctx, &q) + if err != nil { + return nil, err + } + return q.Result, nil +} diff --git a/pkg/services/user/usertest/fake.go b/pkg/services/user/usertest/fake.go index ebadd271573..90b30d61524 100644 --- a/pkg/services/user/usertest/fake.go +++ b/pkg/services/user/usertest/fake.go @@ -26,3 +26,7 @@ func (f *FakeUserService) Delete(ctx context.Context, cmd *user.DeleteUserComman func (f *FakeUserService) GetByID(ctx context.Context, query *user.GetUserByIDQuery) (*user.User, error) { return f.ExpectedUser, f.ExpectedError } + +func (f *FakeUserService) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) { + return f.ExpectedUser, f.ExpectedError +}