Auth: Case insensitive ids duplicate usagestats (#50724)

* WIP

* update for prometheus

* usagestats: tests pass for user duplicate entries

* metrics: added duplicate user entries

* usagestats: adds metrics gauge for duplicate users

* usagestats: skip test for mysql

* sql in oneplace

* only use prometheus register to not panic

* usagestats: RegisterMetricsFunc with loginstats

* fix: remove unused commited code

* refactor: move test to authinfoservice

* Update pkg/models/stats.go

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>

* removed prometheus metrics, due to sql cpu requirement

* Added: has_duplicate_user_entries and fix tests

* remove unused test

* fix: empty else statement removal

* missing argument to authinfoservice

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>
This commit is contained in:
Eric Leijonmarck 2022-06-22 14:16:28 +02:00 committed by GitHub
parent 5ad3d10016
commit 421f7a999a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 126 additions and 5 deletions

View File

@ -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{

View File

@ -0,0 +1,57 @@
package database
import (
"context"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/db"
)
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 := db.DB.GetDialect(s.sqlStore).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
}

View File

@ -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
}

View File

@ -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"])
}
})
})
}

View File

@ -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,

View File

@ -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)
}

View File

@ -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 {
@ -72,6 +73,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) HasEditPermissionInFolders(ctx context.Context, query *models.HasEditPermissionInFoldersQuery) error {
return m.ExpectedError
}

View File

@ -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
GetOrgByName(name string) (*models.Org, error)
CreateOrg(ctx context.Context, cmd *models.CreateOrgCommand) error