From 33006436cc4afb2c0c2fb398f137b3068a7985af Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Mon, 4 Apr 2022 20:36:15 +0200 Subject: [PATCH] Chore: Remove bus.Dispatch from some login packages (#47248) * Chore: Remove bus.Dispatch from some login packages * remove debug log Co-authored-by: Emil Tullstedt * remove login.Init() * remove unused reset function * remove AuthenticateUserFunc global * swap conditional branches Co-authored-by: Emil Tullstedt * fix formatting Co-authored-by: Emil Tullstedt --- pkg/api/admin_users_test.go | 29 ++--------- pkg/api/common_test.go | 3 +- pkg/api/http_server.go | 5 +- pkg/api/ldap_debug.go | 3 +- pkg/api/ldap_debug_test.go | 22 +++----- pkg/api/login.go | 2 +- pkg/api/login_test.go | 23 ++++----- pkg/login/auth.go | 24 +++++++-- pkg/login/auth_test.go | 32 ++++++++---- pkg/login/ldap_login.go | 44 ++-------------- pkg/login/ldap_login_test.go | 7 ++- pkg/middleware/middleware_basic_auth_test.go | 3 +- pkg/server/server.go | 2 +- pkg/server/wire.go | 3 ++ pkg/services/login/authinfo.go | 1 + pkg/services/login/authinfoservice/service.go | 4 ++ pkg/services/login/login.go | 1 + .../login/loginservice/loginservice.go | 40 +++++++++++++++ .../login/loginservice/loginservice_mock.go | 4 ++ .../login/loginservice/loginservice_test.go | 30 ++--------- pkg/services/login/logintest/logintest.go | 51 +++++++++++++++++++ 21 files changed, 190 insertions(+), 143 deletions(-) create mode 100644 pkg/services/login/logintest/logintest.go diff --git a/pkg/api/admin_users_test.go b/pkg/api/admin_users_test.go index 3778c8db92c..b2d331eb213 100644 --- a/pkg/api/admin_users_test.go +++ b/pkg/api/admin_users_test.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/login/loginservice" + "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/setting" @@ -27,28 +28,6 @@ const ( existingTestLogin = "existing@example.com" ) -type mockAuthInfoService struct { - LatestUserID int64 - ExpectedError error -} - -func (m *mockAuthInfoService) LookupAndUpdate(ctx context.Context, query *models.GetUserByAuthInfoQuery) (*models.User, error) { - m.LatestUserID = query.UserId - return nil, m.ExpectedError -} -func (m *mockAuthInfoService) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error { - m.LatestUserID = query.UserId - return m.ExpectedError -} - -func (m *mockAuthInfoService) SetAuthInfo(ctx context.Context, query *models.SetAuthInfoCommand) error { - return m.ExpectedError -} - -func (m *mockAuthInfoService) UpdateAuthInfo(ctx context.Context, query *models.UpdateAuthInfoCommand) error { - return m.ExpectedError -} - func TestAdminAPIEndpoint(t *testing.T) { const role = models.ROLE_ADMIN @@ -282,7 +261,7 @@ func putAdminScenario(t *testing.T, desc string, url string, routePattern string hs := &HTTPServer{ Cfg: setting.NewCfg(), SQLStore: sqlStore, - authInfoService: &mockAuthInfoService{}, + authInfoService: &logintest.AuthInfoServiceFake{}, } sc := setupScenarioContext(t, url) @@ -397,7 +376,7 @@ func adminDisableUserScenario(t *testing.T, desc string, action string, url stri fakeAuthTokenService := auth.NewFakeUserAuthTokenService() - authInfoService := &mockAuthInfoService{} + authInfoService := &logintest.AuthInfoServiceFake{} hs := HTTPServer{ Bus: bus.GetBus(), @@ -435,7 +414,7 @@ func adminDeleteUserScenario(t *testing.T, desc string, url string, routePattern sc := setupScenarioContext(t, url) sc.sqlStore = hs.SQLStore - sc.authInfoService = &mockAuthInfoService{} + sc.authInfoService = &logintest.AuthInfoServiceFake{} sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { sc.context = c sc.context.UserId = testUserID diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 915fe604200..ca284d8ae38 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -34,6 +34,7 @@ import ( "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/login/loginservice" + "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/services/searchusers" @@ -168,7 +169,7 @@ type scenarioContext struct { url string userAuthTokenService *auth.FakeUserAuthTokenService sqlStore sqlstore.Store - authInfoService *mockAuthInfoService + authInfoService *logintest.AuthInfoServiceFake } func (sc *scenarioContext) exec() { diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index e508bcd15b9..efc19fd966a 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -21,6 +21,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/infra/tracing" + loginpkg "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" @@ -134,6 +135,7 @@ type HTTPServer struct { queryDataService *query.Service serviceAccountsService serviceaccounts.Service authInfoService login.AuthInfoService + authenticator loginpkg.Authenticator teamPermissionsService accesscontrol.PermissionsService permissionServices accesscontrol.PermissionsServices NotificationService *notifications.NotificationService @@ -160,7 +162,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi dataSourceCache datasources.CacheService, userTokenService models.UserTokenService, cleanUpService *cleanup.CleanUpService, shortURLService shorturls.Service, queryHistoryService queryhistory.Service, thumbService thumbs.Service, remoteCache *remotecache.RemoteCache, provisioningService provisioning.ProvisioningService, - loginService login.Service, accessControl accesscontrol.AccessControl, + loginService login.Service, authenticator loginpkg.Authenticator, accessControl accesscontrol.AccessControl, dataSourceProxy *datasourceproxy.DataSourceProxyService, searchService *search.SearchService, live *live.GrafanaLive, livePushGateway *pushhttp.Gateway, plugCtxProvider *plugincontext.Provider, contextHandler *contexthandler.ContextHandler, features *featuremgmt.FeatureManager, @@ -236,6 +238,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi queryDataService: queryDataService, serviceAccountsService: serviceaccountsService, authInfoService: authInfoService, + authenticator: authenticator, NotificationService: notificationService, dashboardService: dashboardService, dashboardProvisioningService: dashboardProvisioningService, diff --git a/pkg/api/ldap_debug.go b/pkg/api/ldap_debug.go index f59277c0d7a..a477bceedd6 100644 --- a/pkg/api/ldap_debug.go +++ b/pkg/api/ldap_debug.go @@ -9,7 +9,6 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/multildap" @@ -199,7 +198,7 @@ func (hs *HTTPServer) PostSyncUserWithLDAP(c *models.ReqContext) response.Respon } // Since the user was not in the LDAP server. Let's disable it. - err := login.DisableExternalUser(c.Req.Context(), query.Result.Login) + err := hs.Login.DisableExternalUser(c.Req.Context(), query.Result.Login) if err != nil { return response.Error(http.StatusInternalServerError, "Failed to disable the user", err) } diff --git a/pkg/api/ldap_debug_test.go b/pkg/api/ldap_debug_test.go index 15c66cc13a4..f9d796018bc 100644 --- a/pkg/api/ldap_debug_test.go +++ b/pkg/api/ldap_debug_test.go @@ -1,7 +1,6 @@ package api import ( - "context" "errors" "net/http" "net/http/httptest" @@ -10,12 +9,12 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/login/loginservice" + "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/ldap" @@ -368,6 +367,7 @@ func postSyncUserWithLDAPContext(t *testing.T, requestURL string, preHook func(* t.Helper() sc := setupScenarioContext(t, requestURL) + sc.authInfoService = &logintest.AuthInfoServiceFake{} ldap := setting.LDAPEnabled t.Cleanup(func() { @@ -380,7 +380,7 @@ func postSyncUserWithLDAPContext(t *testing.T, requestURL string, preHook func(* AuthTokenService: auth.NewFakeUserAuthTokenService(), SQLStore: sqlstoremock, Login: loginservice.LoginServiceMock{}, - authInfoService: &mockAuthInfoService{}, + authInfoService: sc.authInfoService, } sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { @@ -483,6 +483,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenGrafanaAdmin(t *testing.T) { func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotInLDAP(t *testing.T) { sqlstoremock := mockstore.SQLStoreMock{ExpectedUser: &models.User{Login: "ldap-daniel", Id: 34}} sc := postSyncUserWithLDAPContext(t, "/api/admin/ldap/sync/34", func(t *testing.T, sc *scenarioContext) { + sc.authInfoService.ExpectedExternalUser = &models.ExternalUserInfo{IsDisabled: true, UserId: 34} getLDAPConfig = func(*setting.Cfg) (*ldap.Config, error) { return &ldap.Config{}, nil } @@ -492,18 +493,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotInLDAP(t *testing.T) { } userSearchResult = nil - - bus.AddHandler("test", func(ctx context.Context, q *models.GetExternalUserInfoByLoginQuery) error { - assert.Equal(t, "ldap-daniel", q.LoginOrEmail) - q.Result = &models.ExternalUserInfo{IsDisabled: true, UserId: 34} - - return nil - }) - - bus.AddHandler("test", func(ctx context.Context, cmd *models.DisableUserCommand) error { - assert.Equal(t, 34, cmd.UserId) - return nil - }) + userSearchError = multildap.ErrDidNotFindUser }, &sqlstoremock) assert.Equal(t, http.StatusBadRequest, sc.resp.Code) @@ -616,7 +606,7 @@ func TestLDAP_AccessControl(t *testing.T) { cfg.LDAPEnabled = true sc, hs := setupAccessControlScenarioContext(t, cfg, test.url, test.permissions) hs.SQLStore = &mockstore.SQLStoreMock{ExpectedUser: &models.User{}} - hs.authInfoService = &mockAuthInfoService{} + hs.authInfoService = &logintest.AuthInfoServiceFake{} hs.Login = &loginservice.LoginServiceMock{} sc.resp = httptest.NewRecorder() sc.req, err = http.NewRequest(test.method, test.url, nil) diff --git a/pkg/api/login.go b/pkg/api/login.go index bad639e6d1d..49a33fa5744 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -209,7 +209,7 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext) response.Response { Cfg: hs.Cfg, } - err := login.AuthenticateUserFunc(c.Req.Context(), authQuery) + err := hs.authenticator.AuthenticateUser(c.Req.Context(), authQuery) authModule = authQuery.AuthModule if err != nil { resp = response.Error(401, "Invalid username or password", err) diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index 1c126f8ad79..4a1807caa2a 100644 --- a/pkg/api/login_test.go +++ b/pkg/api/login_test.go @@ -351,8 +351,7 @@ func TestLoginPostRedirect(t *testing.T) { Email: "", } - mockAuthenticateUserFunc(user, "", nil) - t.Cleanup(resetAuthenticateUserFunc) + hs.authenticator = &fakeAuthenticator{user, "", nil} redirectCases := []redirectCase{ { @@ -684,8 +683,7 @@ func TestLoginPostRunLokingHook(t *testing.T) { for _, c := range testCases { t.Run(c.desc, func(t *testing.T) { - mockAuthenticateUserFunc(c.authUser, c.authModule, c.authErr) - t.Cleanup(resetAuthenticateUserFunc) + hs.authenticator = &fakeAuthenticator{c.authUser, c.authModule, c.authErr} sc.m.Post(sc.url, sc.defaultHandler) sc.fakeReqNoAssertions("POST", sc.url).exec() @@ -732,13 +730,14 @@ func (m *mockSocialService) GetConnector(string) (social.SocialConnector, error) return m.socialConnector, m.err } -func mockAuthenticateUserFunc(user *models.User, authmodule string, err error) { - login.AuthenticateUserFunc = func(ctx context.Context, query *models.LoginUserQuery) error { - query.User = user - query.AuthModule = authmodule - return err - } +type fakeAuthenticator struct { + ExpectedUser *models.User + ExpectedAuthModule string + ExpectedError error } -func resetAuthenticateUserFunc() { - login.AuthenticateUserFunc = login.AuthenticateUser + +func (fa *fakeAuthenticator) AuthenticateUser(c context.Context, query *models.LoginUserQuery) error { + query.User = fa.ExpectedUser + query.AuthModule = fa.ExpectedAuthModule + return fa.ExpectedError } diff --git a/pkg/login/auth.go b/pkg/login/auth.go index 97eb4acd62d..ac73a8faf63 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -8,6 +8,8 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/ldap" + "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/sqlstore" ) var ( @@ -25,14 +27,26 @@ var ( var loginLogger = log.New("login") -var AuthenticateUserFunc = AuthenticateUser +type Authenticator interface { + AuthenticateUser(context.Context, *models.LoginUserQuery) error +} -func Init() { - bus.AddHandler("auth", AuthenticateUser) +type AuthenticatorService struct { + store sqlstore.Store + loginService login.Service +} + +func ProvideService(store sqlstore.Store, loginService login.Service) *AuthenticatorService { + a := &AuthenticatorService{ + store: store, + loginService: loginService, + } + bus.AddHandler("auth", a.AuthenticateUser) + return a } // AuthenticateUser authenticates the user via username & password -func AuthenticateUser(ctx context.Context, query *models.LoginUserQuery) error { +func (a *AuthenticatorService) AuthenticateUser(ctx context.Context, query *models.LoginUserQuery) error { if err := validateLoginAttempts(ctx, query); err != nil { return err } @@ -48,7 +62,7 @@ func AuthenticateUser(ctx context.Context, query *models.LoginUserQuery) error { return err } - ldapEnabled, ldapErr := loginUsingLDAP(ctx, query) + ldapEnabled, ldapErr := loginUsingLDAP(ctx, query, a.loginService) if ldapEnabled { query.AuthModule = models.AuthModuleLDAP if ldapErr == nil || !errors.Is(ldapErr, ldap.ErrInvalidCredentials) { diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index c8f1d7ca488..dfbc344a149 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -7,6 +7,9 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/ldap" + "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/login/logintest" + "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -21,7 +24,8 @@ func TestAuthenticateUser(t *testing.T) { Username: "user", Password: "", } - err := AuthenticateUserFunc(context.Background(), &loginQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), &loginQuery) require.EqualError(t, err, ErrPasswordEmpty.Error()) assert.False(t, sc.grafanaLoginWasCalled) @@ -35,7 +39,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(true, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.EqualError(t, err, ErrTooManyLoginAttempts.Error()) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -51,7 +56,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.NoError(t, err) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -68,7 +74,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.EqualError(t, err, customErr.Error()) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -84,7 +91,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(false, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.EqualError(t, err, models.ErrUserNotFound.Error()) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -100,7 +108,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(true, ldap.ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.EqualError(t, err, ErrInvalidCredentials.Error()) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -116,7 +125,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(true, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.NoError(t, err) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -133,7 +143,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(true, customErr, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.EqualError(t, err, customErr.Error()) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -149,7 +160,8 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLDAP(true, ldap.ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUserFunc(context.Background(), sc.loginUserQuery) + a := AuthenticatorService{store: mockstore.NewSQLStoreMock(), loginService: &logintest.LoginServiceFake{}} + err := a.AuthenticateUser(context.Background(), sc.loginUserQuery) require.EqualError(t, err, ErrInvalidCredentials.Error()) assert.True(t, sc.loginAttemptValidationWasCalled) @@ -177,7 +189,7 @@ func mockLoginUsingGrafanaDB(err error, sc *authScenarioContext) { } func mockLoginUsingLDAP(enabled bool, err error, sc *authScenarioContext) { - loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery) (bool, error) { + loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery, _ login.Service) (bool, error) { sc.ldapLoginWasCalled = true return enabled, err } diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index 2658de7fd9d..629b0c8d14a 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/ldap" + "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/multildap" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util/errutil" @@ -27,7 +28,7 @@ var ldapLogger = log.New("login.ldap") // loginUsingLDAP logs in user using LDAP. It returns whether LDAP is enabled and optional error and query arg will be // populated with the logged in user if successful. -var loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery) (bool, error) { +var loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery, loginService login.Service) (bool, error) { enabled := isLDAPEnabled() if !enabled { @@ -43,7 +44,7 @@ var loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery) (bo if err != nil { if errors.Is(err, ldap.ErrCouldNotFindUser) { // Ignore the error since user might not be present anyway - if err := DisableExternalUser(ctx, query.Username); err != nil { + if err := loginService.DisableExternalUser(ctx, query.Username); err != nil { ldapLogger.Debug("Failed to disable external user", "err", err) } @@ -66,42 +67,3 @@ var loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery) (bo return true, nil } - -// DisableExternalUser marks external user as disabled in Grafana db -func DisableExternalUser(ctx context.Context, username string) error { - // Check if external user exist in Grafana - userQuery := &models.GetExternalUserInfoByLoginQuery{ - LoginOrEmail: username, - } - - if err := bus.Dispatch(ctx, userQuery); err != nil { - return err - } - - userInfo := userQuery.Result - if !userInfo.IsDisabled { - ldapLogger.Debug( - "Disabling external user", - "user", - userQuery.Result.Login, - ) - - // Mark user as disabled in grafana db - disableUserCmd := &models.DisableUserCommand{ - UserId: userQuery.Result.UserId, - IsDisabled: true, - } - - if err := bus.Dispatch(ctx, disableUserCmd); err != nil { - ldapLogger.Debug( - "Error disabling external user", - "user", - userQuery.Result.Login, - "message", - err.Error(), - ) - return err - } - } - return nil -} diff --git a/pkg/login/ldap_login_test.go b/pkg/login/ldap_login_test.go index 25c53bada07..4503b272e92 100644 --- a/pkg/login/ldap_login_test.go +++ b/pkg/login/ldap_login_test.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/ldap" + "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/multildap" "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/assert" @@ -28,7 +29,8 @@ func TestLoginUsingLDAP(t *testing.T) { return config, nil } - enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery) + loginService := &logintest.LoginServiceFake{} + enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService) require.EqualError(t, err, errTest.Error()) assert.True(t, enabled) @@ -39,7 +41,8 @@ func TestLoginUsingLDAP(t *testing.T) { setting.LDAPEnabled = false sc.withLoginResult(false) - enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery) + loginService := &logintest.LoginServiceFake{} + enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService) require.NoError(t, err) assert.False(t, enabled) diff --git a/pkg/middleware/middleware_basic_auth_test.go b/pkg/middleware/middleware_basic_auth_test.go index 38847fd4891..e9a8a7a4ebd 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/login" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/contexthandler" + "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" "github.com/stretchr/testify/assert" @@ -78,7 +79,7 @@ func TestMiddlewareBasicAuth(t *testing.T) { const password = "MyPass" const salt = "Salt" - login.Init() + login.ProvideService(sc.sqlStore, &logintest.LoginServiceFake{}) bus.AddHandler("user-query", func(ctx context.Context, query *models.GetUserByLoginQuery) error { encoded, err := util.EncodePassword(password, salt) diff --git a/pkg/server/server.go b/pkg/server/server.go index 92a2839db8b..b1e8df32f94 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -118,7 +118,7 @@ func (s *Server) init() error { return err } - login.Init() + login.ProvideService(s.HTTPServer.SQLStore, s.HTTPServer.Login) social.ProvideService(s.cfg) if err := s.roleRegistry.RegisterFixedRoles(); err != nil { diff --git a/pkg/server/wire.go b/pkg/server/wire.go index dc59444d16d..7efd4d7c1af 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -20,6 +20,7 @@ import ( "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" uss "github.com/grafana/grafana/pkg/infra/usagestats/service" + loginpkg "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" @@ -159,6 +160,8 @@ var wireBasicSet = wire.NewSet( wire.Bind(new(login.AuthInfoService), new(*authinfoservice.Implementation)), authinfodatabase.ProvideAuthInfoStore, wire.Bind(new(login.Store), new(*authinfodatabase.AuthInfoStore)), + loginpkg.ProvideService, + wire.Bind(new(loginpkg.Authenticator), new(*loginpkg.AuthenticatorService)), datasourceproxy.ProvideService, search.ProvideService, searchV2.ProvideService, diff --git a/pkg/services/login/authinfo.go b/pkg/services/login/authinfo.go index 7ada13996e7..4db77aa6046 100644 --- a/pkg/services/login/authinfo.go +++ b/pkg/services/login/authinfo.go @@ -9,6 +9,7 @@ import ( type AuthInfoService interface { LookupAndUpdate(ctx context.Context, query *models.GetUserByAuthInfoQuery) (*models.User, error) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error + GetExternalUserInfoByLogin(ctx context.Context, query *models.GetExternalUserInfoByLoginQuery) error SetAuthInfo(ctx context.Context, cmd *models.SetAuthInfoCommand) error UpdateAuthInfo(ctx context.Context, cmd *models.UpdateAuthInfoCommand) error } diff --git a/pkg/services/login/authinfoservice/service.go b/pkg/services/login/authinfoservice/service.go index 5dbd218c7e8..bf61cbcf510 100644 --- a/pkg/services/login/authinfoservice/service.go +++ b/pkg/services/login/authinfoservice/service.go @@ -183,3 +183,7 @@ func (s *Implementation) UpdateAuthInfo(ctx context.Context, cmd *models.UpdateA func (s *Implementation) SetAuthInfo(ctx context.Context, cmd *models.SetAuthInfoCommand) error { return s.authInfoStore.SetAuthInfo(ctx, cmd) } + +func (s *Implementation) GetExternalUserInfoByLogin(ctx context.Context, query *models.GetExternalUserInfoByLoginQuery) error { + return s.authInfoStore.GetExternalUserInfoByLogin(ctx, query) +} diff --git a/pkg/services/login/login.go b/pkg/services/login/login.go index e8a995197c0..c698ee55dbe 100644 --- a/pkg/services/login/login.go +++ b/pkg/services/login/login.go @@ -18,5 +18,6 @@ type TeamSyncFunc func(user *models.User, externalUser *models.ExternalUserInfo) type Service interface { CreateUser(cmd models.CreateUserCommand) (*models.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 d3f9ffe01b1..db5ccf002cb 100644 --- a/pkg/services/login/loginservice/loginservice.go +++ b/pkg/services/login/loginservice/loginservice.go @@ -130,6 +130,46 @@ func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUser return nil } +func (ls *Implementation) DisableExternalUser(ctx context.Context, username string) error { + // Check if external user exist in Grafana + userQuery := &models.GetExternalUserInfoByLoginQuery{ + LoginOrEmail: username, + } + + if err := ls.AuthInfoService.GetExternalUserInfoByLogin(ctx, userQuery); err != nil { + return err + } + + userInfo := userQuery.Result + if userInfo.IsDisabled { + return nil + } + + logger.Debug( + "Disabling external user", + "user", + userQuery.Result.Login, + ) + + // Mark user as disabled in grafana db + disableUserCmd := &models.DisableUserCommand{ + UserId: userQuery.Result.UserId, + IsDisabled: true, + } + + if err := ls.SQLStore.DisableUser(ctx, disableUserCmd); err != nil { + logger.Debug( + "Error disabling external user", + "user", + userQuery.Result.Login, + "message", + err.Error(), + ) + return err + } + return nil +} + // SetTeamSyncFunc sets the function received through args as the team sync function. func (ls *Implementation) SetTeamSyncFunc(teamSyncFunc login.TeamSyncFunc) { ls.TeamSync = teamSyncFunc diff --git a/pkg/services/login/loginservice/loginservice_mock.go b/pkg/services/login/loginservice/loginservice_mock.go index abd77f9f2bd..0090653bdc2 100644 --- a/pkg/services/login/loginservice/loginservice_mock.go +++ b/pkg/services/login/loginservice/loginservice_mock.go @@ -45,3 +45,7 @@ func (s LoginServiceMock) UpsertUser(ctx context.Context, cmd *models.UpsertUser cmd.Result = s.ExpectedUser return s.ExpectedError } + +func (s LoginServiceMock) DisableExternalUser(ctx context.Context, username string) error { + return nil +} diff --git a/pkg/services/login/loginservice/loginservice_test.go b/pkg/services/login/loginservice/loginservice_test.go index faceae8d15f..4bace3556f9 100644 --- a/pkg/services/login/loginservice/loginservice_test.go +++ b/pkg/services/login/loginservice/loginservice_test.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/log/level" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/stretchr/testify/assert" @@ -19,7 +20,7 @@ import ( func Test_syncOrgRoles_doesNotBreakWhenTryingToRemoveLastOrgAdmin(t *testing.T) { user := createSimpleUser() externalUser := createSimpleExternalUser() - authInfoMock := &authInfoServiceMock{} + authInfoMock := &logintest.AuthInfoServiceFake{} store := &mockstore.SQLStoreMock{ ExpectedUserOrgList: createUserOrgDTO(), @@ -44,7 +45,7 @@ func Test_syncOrgRoles_whenTryingToRemoveLastOrgLogsError(t *testing.T) { user := createSimpleUser() externalUser := createSimpleExternalUser() - authInfoMock := &authInfoServiceMock{} + authInfoMock := &logintest.AuthInfoServiceFake{} store := &mockstore.SQLStoreMock{ ExpectedUserOrgList: createUserOrgDTO(), @@ -63,29 +64,8 @@ func Test_syncOrgRoles_whenTryingToRemoveLastOrgLogsError(t *testing.T) { assert.Contains(t, buf.String(), models.ErrLastOrgAdmin.Error()) } -type authInfoServiceMock struct { - user *models.User - err error -} - -func (a *authInfoServiceMock) LookupAndUpdate(ctx context.Context, query *models.GetUserByAuthInfoQuery) (*models.User, error) { - return a.user, a.err -} - -func (a *authInfoServiceMock) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error { - return nil -} - -func (a *authInfoServiceMock) SetAuthInfo(ctx context.Context, cmd *models.SetAuthInfoCommand) error { - return nil -} - -func (a *authInfoServiceMock) UpdateAuthInfo(ctx context.Context, cmd *models.UpdateAuthInfoCommand) error { - return nil -} - func Test_teamSync(t *testing.T) { - authInfoMock := &authInfoServiceMock{} + authInfoMock := &logintest.AuthInfoServiceFake{} login := Implementation{ Bus: bus.New(), QuotaService: "a.QuotaService{}, @@ -99,7 +79,7 @@ func Test_teamSync(t *testing.T) { Name: "test_user", Login: "test_user", } - authInfoMock.user = expectedUser + authInfoMock.ExpectedUser = expectedUser bus.ClearBusHandlers() t.Cleanup(func() { bus.ClearBusHandlers() }) diff --git a/pkg/services/login/logintest/logintest.go b/pkg/services/login/logintest/logintest.go new file mode 100644 index 00000000000..6197e8d5ce9 --- /dev/null +++ b/pkg/services/login/logintest/logintest.go @@ -0,0 +1,51 @@ +package logintest + +import ( + "context" + + "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/login" +) + +type LoginServiceFake struct{} + +func (l *LoginServiceFake) CreateUser(cmd models.CreateUserCommand) (*models.User, error) { + return nil, nil +} +func (l *LoginServiceFake) UpsertUser(ctx context.Context, cmd *models.UpsertUserCommand) error { + return nil +} +func (l *LoginServiceFake) DisableExternalUser(ctx context.Context, username string) error { + return nil +} +func (l *LoginServiceFake) SetTeamSyncFunc(login.TeamSyncFunc) {} + +type AuthInfoServiceFake struct { + LatestUserID int64 + ExpectedUser *models.User + ExpectedExternalUser *models.ExternalUserInfo + ExpectedError error +} + +func (a *AuthInfoServiceFake) LookupAndUpdate(ctx context.Context, query *models.GetUserByAuthInfoQuery) (*models.User, error) { + a.LatestUserID = query.UserId + return a.ExpectedUser, a.ExpectedError +} + +func (a *AuthInfoServiceFake) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error { + a.LatestUserID = query.UserId + return a.ExpectedError +} + +func (a *AuthInfoServiceFake) SetAuthInfo(ctx context.Context, cmd *models.SetAuthInfoCommand) error { + return a.ExpectedError +} + +func (a *AuthInfoServiceFake) UpdateAuthInfo(ctx context.Context, cmd *models.UpdateAuthInfoCommand) error { + return a.ExpectedError +} + +func (a *AuthInfoServiceFake) GetExternalUserInfoByLogin(ctx context.Context, query *models.GetExternalUserInfoByLoginQuery) error { + query.Result = a.ExpectedExternalUser + return a.ExpectedError +}