From bf9e0e8bc8a2b56cdbf333c68df5b72fc0c5b6ee Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Tue, 5 Apr 2022 13:07:27 +0200 Subject: [PATCH] Chore: Remove some bus from login package (login attempts) (#47310) --- pkg/login/auth.go | 4 +- pkg/login/auth_test.go | 5 ++- pkg/login/brute_force_login_protection.go | 10 ++--- .../brute_force_login_protection_test.go | 45 +++++-------------- pkg/services/sqlstore/mockstore/mockstore.go | 15 +++++-- pkg/services/sqlstore/store.go | 1 + 6 files changed, 35 insertions(+), 45 deletions(-) diff --git a/pkg/login/auth.go b/pkg/login/auth.go index ac73a8faf63..429317ec4c7 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -47,7 +47,7 @@ func ProvideService(store sqlstore.Store, loginService login.Service) *Authentic // AuthenticateUser authenticates the user via username & password func (a *AuthenticatorService) AuthenticateUser(ctx context.Context, query *models.LoginUserQuery) error { - if err := validateLoginAttempts(ctx, query); err != nil { + if err := validateLoginAttempts(ctx, query, a.store); err != nil { return err } @@ -75,7 +75,7 @@ func (a *AuthenticatorService) AuthenticateUser(ctx context.Context, query *mode } if errors.Is(err, ErrInvalidCredentials) || errors.Is(err, ldap.ErrInvalidCredentials) { - if err := saveInvalidLoginAttempt(ctx, query); err != nil { + if err := saveInvalidLoginAttempt(ctx, query, a.store); err != nil { loginLogger.Error("Failed to save invalid login attempt", "err", err) } diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index dfbc344a149..5ebd4098936 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -9,6 +9,7 @@ import ( "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" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -196,14 +197,14 @@ func mockLoginUsingLDAP(enabled bool, err error, sc *authScenarioContext) { } func mockLoginAttemptValidation(err error, sc *authScenarioContext) { - validateLoginAttempts = func(context.Context, *models.LoginUserQuery) error { + validateLoginAttempts = func(context.Context, *models.LoginUserQuery, sqlstore.Store) error { sc.loginAttemptValidationWasCalled = true return err } } func mockSaveInvalidLoginAttempt(sc *authScenarioContext) { - saveInvalidLoginAttempt = func(ctx context.Context, query *models.LoginUserQuery) error { + saveInvalidLoginAttempt = func(ctx context.Context, query *models.LoginUserQuery, _ sqlstore.Store) error { sc.saveInvalidLoginAttemptWasCalled = true return nil } diff --git a/pkg/login/brute_force_login_protection.go b/pkg/login/brute_force_login_protection.go index 6e4084152d0..2fb30d97da7 100644 --- a/pkg/login/brute_force_login_protection.go +++ b/pkg/login/brute_force_login_protection.go @@ -4,8 +4,8 @@ import ( "context" "time" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore" ) var ( @@ -13,7 +13,7 @@ var ( loginAttemptsWindow = time.Minute * 5 ) -var validateLoginAttempts = func(ctx context.Context, query *models.LoginUserQuery) error { +var validateLoginAttempts = func(ctx context.Context, query *models.LoginUserQuery, store sqlstore.Store) error { if query.Cfg.DisableBruteForceLoginProtection { return nil } @@ -23,7 +23,7 @@ var validateLoginAttempts = func(ctx context.Context, query *models.LoginUserQue Since: time.Now().Add(-loginAttemptsWindow), } - if err := bus.Dispatch(ctx, &loginAttemptCountQuery); err != nil { + if err := store.GetUserLoginAttemptCount(ctx, &loginAttemptCountQuery); err != nil { return err } @@ -34,7 +34,7 @@ var validateLoginAttempts = func(ctx context.Context, query *models.LoginUserQue return nil } -var saveInvalidLoginAttempt = func(ctx context.Context, query *models.LoginUserQuery) error { +var saveInvalidLoginAttempt = func(ctx context.Context, query *models.LoginUserQuery, store sqlstore.Store) error { if query.Cfg.DisableBruteForceLoginProtection { return nil } @@ -44,5 +44,5 @@ var saveInvalidLoginAttempt = func(ctx context.Context, query *models.LoginUserQ IpAddress: query.IpAddress, } - return bus.Dispatch(ctx, &loginAttemptCommand) + return store.CreateLoginAttempt(ctx, &loginAttemptCommand) } diff --git a/pkg/login/brute_force_login_protection_test.go b/pkg/login/brute_force_login_protection_test.go index a0fd5be034e..adda89990b1 100644 --- a/pkg/login/brute_force_login_protection_test.go +++ b/pkg/login/brute_force_login_protection_test.go @@ -4,8 +4,8 @@ import ( "context" "testing" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -59,11 +59,12 @@ func TestValidateLoginAttempts(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - withLoginAttempts(t, tc.loginAttempts) + store := mockstore.NewSQLStoreMock() + store.ExpectedLoginAttempts = tc.loginAttempts query := &models.LoginUserQuery{Username: "user", Cfg: tc.cfg} - err := validateLoginAttempts(context.Background(), query) + err := validateLoginAttempts(context.Background(), query, store) require.Equal(t, tc.expected, err) }) } @@ -71,45 +72,31 @@ func TestValidateLoginAttempts(t *testing.T) { func TestSaveInvalidLoginAttempt(t *testing.T) { t.Run("When brute force protection enabled", func(t *testing.T) { - t.Cleanup(func() { bus.ClearBusHandlers() }) - - createLoginAttemptCmd := &models.CreateLoginAttemptCommand{} - bus.AddHandler("test", func(ctx context.Context, cmd *models.CreateLoginAttemptCommand) error { - createLoginAttemptCmd = cmd - return nil - }) - + store := mockstore.NewSQLStoreMock() err := saveInvalidLoginAttempt(context.Background(), &models.LoginUserQuery{ Username: "user", Password: "pwd", IpAddress: "192.168.1.1:56433", Cfg: cfgWithBruteForceLoginProtectionEnabled(t), - }) + }, store) require.NoError(t, err) - require.NotNil(t, createLoginAttemptCmd) - assert.Equal(t, "user", createLoginAttemptCmd.Username) - assert.Equal(t, "192.168.1.1:56433", createLoginAttemptCmd.IpAddress) + require.NotNil(t, store.LastLoginAttemptCommand) + assert.Equal(t, "user", store.LastLoginAttemptCommand.Username) + assert.Equal(t, "192.168.1.1:56433", store.LastLoginAttemptCommand.IpAddress) }) t.Run("When brute force protection disabled", func(t *testing.T) { - t.Cleanup(func() { bus.ClearBusHandlers() }) - - var createLoginAttemptCmd *models.CreateLoginAttemptCommand - bus.AddHandler("test", func(ctx context.Context, cmd *models.CreateLoginAttemptCommand) error { - createLoginAttemptCmd = cmd - return nil - }) - + store := mockstore.NewSQLStoreMock() err := saveInvalidLoginAttempt(context.Background(), &models.LoginUserQuery{ Username: "user", Password: "pwd", IpAddress: "192.168.1.1:56433", Cfg: cfgWithBruteForceLoginProtectionDisabled(t), - }) + }, store) require.NoError(t, err) - require.Nil(t, createLoginAttemptCmd) + require.Nil(t, store.LastLoginAttemptCommand) }) } @@ -126,11 +113,3 @@ func cfgWithBruteForceLoginProtectionEnabled(t *testing.T) *setting.Cfg { require.False(t, cfg.DisableBruteForceLoginProtection) return cfg } - -func withLoginAttempts(t *testing.T, loginAttempts int64) { - t.Helper() - bus.AddHandler("test", func(ctx context.Context, query *models.GetUserLoginAttemptCountQuery) error { - query.Result = loginAttempts - return nil - }) -} diff --git a/pkg/services/sqlstore/mockstore/mockstore.go b/pkg/services/sqlstore/mockstore/mockstore.go index a95e5435172..4a87e0c0fcb 100644 --- a/pkg/services/sqlstore/mockstore/mockstore.go +++ b/pkg/services/sqlstore/mockstore/mockstore.go @@ -12,8 +12,9 @@ type OrgListResponse []struct { Response error } type SQLStoreMock struct { - LastGetAlertsQuery *models.GetAlertsQuery - LatestUserId int64 + LastGetAlertsQuery *models.GetAlertsQuery + LastLoginAttemptCommand *models.CreateLoginAttemptCommand + LatestUserId int64 ExpectedUser *models.User ExpectedDatasource *models.DataSource @@ -40,7 +41,9 @@ type SQLStoreMock struct { ExpectedPersistedDashboards models.HitList ExpectedSignedInUser *models.SignedInUser ExpectedUserStars map[int64]bool - ExpectedError error + ExpectedLoginAttempts int64 + + ExpectedError error } func NewSQLStoreMock() *SQLStoreMock { @@ -130,6 +133,12 @@ func (m SQLStoreMock) DeleteOrphanedProvisionedDashboards(ctx context.Context, c } func (m *SQLStoreMock) CreateLoginAttempt(ctx context.Context, cmd *models.CreateLoginAttemptCommand) error { + m.LastLoginAttemptCommand = cmd + return m.ExpectedError +} + +func (m *SQLStoreMock) GetUserLoginAttemptCount(ctx context.Context, query *models.GetUserLoginAttemptCountQuery) error { + query.Result = m.ExpectedLoginAttempts return m.ExpectedError } diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index 05e4a8aafd2..35e66d58350 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -26,6 +26,7 @@ type Store interface { GetOrgById(context.Context, *models.GetOrgByIdQuery) error GetOrgByNameHandler(ctx context.Context, query *models.GetOrgByNameQuery) error CreateLoginAttempt(ctx context.Context, cmd *models.CreateLoginAttemptCommand) error + GetUserLoginAttemptCount(ctx context.Context, query *models.GetUserLoginAttemptCountQuery) error DeleteOldLoginAttempts(ctx context.Context, cmd *models.DeleteOldLoginAttemptsCommand) error CreateUser(ctx context.Context, cmd models.CreateUserCommand) (*models.User, error) GetUserById(ctx context.Context, query *models.GetUserByIdQuery) error