From 5735f6c88b5457d53d50c4df424960ec0a54bf34 Mon Sep 17 00:00:00 2001 From: Eric Leijonmarck Date: Tue, 28 Jun 2022 11:03:53 +0200 Subject: [PATCH] [v9.0.x] Auth: Case insensitive ids duplicate usagestats (#51240) * merged and backport to 9.0 * implement the missing method GetDialect for sqlStore * Add back mockstore missing methods --- pkg/api/user_test.go | 7 ++- .../authinfoservice/database/usagestats.go | 56 +++++++++++++++++++ pkg/services/login/authinfoservice/service.go | 5 +- .../login/authinfoservice/user_auth_test.go | 48 +++++++++++++++- .../login/loginservice/loginservice.go | 6 +- pkg/services/login/userprotection.go | 1 + pkg/services/sqlstore/mockstore/mockstore.go | 5 ++ pkg/services/sqlstore/sqlstore.go | 5 ++ pkg/services/sqlstore/store.go | 2 + 9 files changed, 130 insertions(+), 5 deletions(-) create mode 100644 pkg/services/login/authinfoservice/database/usagestats.go diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 4e59ac05d8c..0879f79dd54 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/models" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/login/authinfoservice" @@ -48,7 +49,11 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { fakeNow := time.Date(2019, 2, 11, 17, 30, 40, 0, time.UTC) secretsService := secretsManager.SetupTestService(t, database.ProvideSecretsStore(sqlStore)) authInfoStore := authinfostore.ProvideAuthInfoStore(sqlStore, secretsService) - srv := authinfoservice.ProvideAuthInfoService(&authinfoservice.OSSUserProtectionImpl{}, authInfoStore) + srv := authinfoservice.ProvideAuthInfoService( + &authinfoservice.OSSUserProtectionImpl{}, + authInfoStore, + &usagestats.UsageStatsMock{}, + ) hs.authInfoService = srv createUserCmd := models.CreateUserCommand{ diff --git a/pkg/services/login/authinfoservice/database/usagestats.go b/pkg/services/login/authinfoservice/database/usagestats.go new file mode 100644 index 00000000000..6ab134a1b2a --- /dev/null +++ b/pkg/services/login/authinfoservice/database/usagestats.go @@ -0,0 +1,56 @@ +package database + +import ( + "context" + + "github.com/grafana/grafana/pkg/services/sqlstore" +) + +type loginStats struct { + DuplicateUserEntries int `xorm:"duplicate_user_entries"` +} + +func (s *AuthInfoStore) GetLoginStats(ctx context.Context) (loginStats, error) { + var stats loginStats + outerErr := s.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { + rawSQL := `SELECT COUNT(*) as duplicate_user_entries FROM (` + s.duplicateUserEntriesSQL(ctx) + `)` + _, err := dbSession.SQL(rawSQL).Get(&stats) + return err + }) + if outerErr != nil { + return stats, outerErr + } + return stats, nil +} + +func (s *AuthInfoStore) CollectLoginStats(ctx context.Context) (map[string]interface{}, error) { + m := map[string]interface{}{} + + loginStats, err := s.GetLoginStats(ctx) + if err != nil { + s.logger.Error("Failed to get login stats", "error", err) + return nil, err + } + + m["stats.users.duplicate_user_entries"] = loginStats.DuplicateUserEntries + if loginStats.DuplicateUserEntries > 0 { + m["stats.users.has_duplicate_user_entries"] = 1 + } else { + m["stats.users.has_duplicate_user_entries"] = 0 + } + return m, nil +} + +func (s *AuthInfoStore) duplicateUserEntriesSQL(ctx context.Context) string { + userDialect := s.sqlStore.GetDialect().Quote("user") + // this query counts how many users have the same login or email. + // which might be confusing, but gives a good indication + // we want this query to not require too much cpu + sqlQuery := `SELECT + (SELECT login from ` + userDialect + ` WHERE (LOWER(login) = LOWER(u.login)) AND (login != u.login)) AS dup_login, + (SELECT email from ` + userDialect + ` WHERE (LOWER(email) = LOWER(u.email)) AND (email != u.email)) AS dup_email + FROM ` + userDialect + ` AS u + WHERE (dup_login IS NOT NULL OR dup_email IS NOT NULL) + ` + return sqlQuery +} diff --git a/pkg/services/login/authinfoservice/service.go b/pkg/services/login/authinfoservice/service.go index 8fc26ab7b10..b14b9dfd965 100644 --- a/pkg/services/login/authinfoservice/service.go +++ b/pkg/services/login/authinfoservice/service.go @@ -5,6 +5,7 @@ import ( "errors" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/login" ) @@ -17,13 +18,13 @@ type Implementation struct { logger log.Logger } -func ProvideAuthInfoService(userProtectionService login.UserProtectionService, authInfoStore login.Store) *Implementation { +func ProvideAuthInfoService(userProtectionService login.UserProtectionService, authInfoStore login.Store, usageStats usagestats.Service) *Implementation { s := &Implementation{ UserProtectionService: userProtectionService, authInfoStore: authInfoStore, logger: log.New("login.authinfo"), } - + usageStats.RegisterMetricsFunc(authInfoStore.CollectLoginStats) return s } diff --git a/pkg/services/login/authinfoservice/user_auth_test.go b/pkg/services/login/authinfoservice/user_auth_test.go index e03640be847..5189a7d415e 100644 --- a/pkg/services/login/authinfoservice/user_auth_test.go +++ b/pkg/services/login/authinfoservice/user_auth_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/login/authinfoservice/database" secretstore "github.com/grafana/grafana/pkg/services/secrets/database" @@ -20,7 +21,11 @@ func TestUserAuth(t *testing.T) { sqlStore := sqlstore.InitTestDB(t) secretsService := secretsManager.SetupTestService(t, secretstore.ProvideSecretsStore(sqlStore)) authInfoStore := database.ProvideAuthInfoStore(sqlStore, secretsService) - srv := ProvideAuthInfoService(&OSSUserProtectionImpl{}, authInfoStore) + srv := ProvideAuthInfoService( + &OSSUserProtectionImpl{}, + authInfoStore, + &usagestats.UsageStatsMock{}, + ) t.Run("Given 5 users", func(t *testing.T) { for i := 0; i < 5; i++ { @@ -343,5 +348,46 @@ func TestUserAuth(t *testing.T) { require.NotNil(t, err) require.Nil(t, user) }) + + t.Run("calculate metrics on duplicate userstats", func(t *testing.T) { + // Restore after destructive operation + sqlStore = sqlstore.InitTestDB(t) + + for i := 0; i < 5; i++ { + cmd := models.CreateUserCommand{ + Email: fmt.Sprint("user", i, "@test.com"), + Name: fmt.Sprint("user", i), + Login: fmt.Sprint("loginuser", i), + OrgId: 1, + } + _, err := sqlStore.CreateUser(context.Background(), cmd) + require.Nil(t, err) + } + + // "Skipping duplicate users test for mysql as it does make unique constraint case insensitive by default + if sqlStore.GetDialect().DriverName() != "mysql" { + dupUserEmailcmd := models.CreateUserCommand{ + Email: "USERDUPLICATETEST1@TEST.COM", + Name: "user name 1", + Login: "USER_DUPLICATE_TEST_1_LOGIN", + } + _, err := sqlStore.CreateUser(context.Background(), dupUserEmailcmd) + require.NoError(t, err) + + // add additional user with duplicate login where DOMAIN is upper case + dupUserLogincmd := models.CreateUserCommand{ + Email: "userduplicatetest1@test.com", + Name: "user name 1", + Login: "user_duplicate_test_1_login", + } + _, err = sqlStore.CreateUser(context.Background(), dupUserLogincmd) + require.NoError(t, err) + // require metrics and statistics to be 2 + m, err := srv.authInfoStore.CollectLoginStats(context.Background()) + require.NoError(t, err) + require.Equal(t, 2, m["stats.users.duplicate_user_entries"]) + require.Equal(t, 1, m["stats.users.has_duplicate_user_entries"]) + } + }) }) } diff --git a/pkg/services/login/loginservice/loginservice.go b/pkg/services/login/loginservice/loginservice.go index ea9338602b7..3879b50c257 100644 --- a/pkg/services/login/loginservice/loginservice.go +++ b/pkg/services/login/loginservice/loginservice.go @@ -15,7 +15,11 @@ var ( logger = log.New("login.ext_user") ) -func ProvideService(sqlStore sqlstore.Store, quotaService *quota.QuotaService, authInfoService login.AuthInfoService) *Implementation { +func ProvideService( + sqlStore sqlstore.Store, + quotaService *quota.QuotaService, + authInfoService login.AuthInfoService, +) *Implementation { s := &Implementation{ SQLStore: sqlStore, QuotaService: quotaService, diff --git a/pkg/services/login/userprotection.go b/pkg/services/login/userprotection.go index bdd8c23e255..0a36fd3b900 100644 --- a/pkg/services/login/userprotection.go +++ b/pkg/services/login/userprotection.go @@ -20,4 +20,5 @@ type Store interface { GetUserById(ctx context.Context, id int64) (*models.User, error) GetUserByLogin(ctx context.Context, login string) (*models.User, error) GetUserByEmail(ctx context.Context, email string) (*models.User, error) + CollectLoginStats(ctx context.Context) (map[string]interface{}, error) } diff --git a/pkg/services/sqlstore/mockstore/mockstore.go b/pkg/services/sqlstore/mockstore/mockstore.go index 1545a21c6e8..61e90060688 100644 --- a/pkg/services/sqlstore/mockstore/mockstore.go +++ b/pkg/services/sqlstore/mockstore/mockstore.go @@ -5,6 +5,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/sqlstore" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" ) type OrgListResponse []struct { @@ -75,6 +76,10 @@ func (m *SQLStoreMock) GetSystemStats(ctx context.Context, query *models.GetSyst return m.ExpectedError } +func (m *SQLStoreMock) GetDialect() migrator.Dialect { + return nil +} + func (m *SQLStoreMock) DeleteExpiredSnapshots(ctx context.Context, cmd *models.DeleteExpiredSnapshotsCommand) error { return m.ExpectedError } diff --git a/pkg/services/sqlstore/sqlstore.go b/pkg/services/sqlstore/sqlstore.go index 8e727615f18..240637389cd 100644 --- a/pkg/services/sqlstore/sqlstore.go +++ b/pkg/services/sqlstore/sqlstore.go @@ -157,6 +157,11 @@ func (ss *SQLStore) Quote(value string) string { return ss.engine.Quote(value) } +// GetDialect retrieves the dialect of the current SQL engine +func (ss *SQLStore) GetDialect() migrator.Dialect { + return ss.Dialect +} + func (ss *SQLStore) ensureMainOrgAndAdminUser() error { ctx := context.Background() err := ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index 90c9bf56271..fa6246237d5 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -4,6 +4,7 @@ import ( "context" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" ) type Store interface { @@ -11,6 +12,7 @@ type Store interface { GetAlertNotifiersUsageStats(ctx context.Context, query *models.GetAlertNotifierUsageStatsQuery) error GetDataSourceStats(ctx context.Context, query *models.GetDataSourceStatsQuery) error GetDataSourceAccessStats(ctx context.Context, query *models.GetDataSourceAccessStatsQuery) error + GetDialect() migrator.Dialect GetSystemStats(ctx context.Context, query *models.GetSystemStatsQuery) error DeleteExpiredSnapshots(ctx context.Context, cmd *models.DeleteExpiredSnapshotsCommand) error CreateDashboardSnapshot(ctx context.Context, cmd *models.CreateDashboardSnapshotCommand) error