From 27d8b68c9c3ca15d23957f74428df572799cb133 Mon Sep 17 00:00:00 2001 From: Mihai Doarna Date: Fri, 24 Nov 2023 12:02:05 +0200 Subject: [PATCH] Auth: Add more tests for the SSO settings upsert function (#78544) * add more tests for the SSO settings upsert function * fix client id to match provider * use time now in tests --- pkg/services/ssosettings/database/database.go | 35 +-- .../ssosettings/database/database_test.go | 199 ++++++++++++++---- 2 files changed, 175 insertions(+), 59 deletions(-) diff --git a/pkg/services/ssosettings/database/database.go b/pkg/services/ssosettings/database/database.go index f2c5735ffc4..2923d2900e0 100644 --- a/pkg/services/ssosettings/database/database.go +++ b/pkg/services/ssosettings/database/database.go @@ -17,6 +17,11 @@ type SSOSettingsStore struct { log log.Logger } +var ( + // timeNow makes it possible to test usage of time + timeNow = time.Now +) + func ProvideStore(sqlStore db.DB) *SSOSettingsStore { return &SSOSettingsStore{ sqlStore: sqlStore, @@ -72,33 +77,37 @@ func (s *SSOSettingsStore) List(ctx context.Context) ([]*models.SSOSetting, erro } func (s *SSOSettingsStore) Upsert(ctx context.Context, provider string, data map[string]interface{}) error { - err := s.sqlStore.WithDbSession(ctx, func(sess *db.Session) error { - var err error - found, err := sess.Where("provider = ? AND is_deleted = ?", provider, s.sqlStore.GetDialect().BooleanStr(false)).Exist(&models.SSOSetting{}) - + return s.sqlStore.WithDbSession(ctx, func(sess *db.Session) error { + existing := &models.SSOSetting{ + Provider: provider, + IsDeleted: false, + } + found, err := sess.UseBool("is_deleted").Exist(existing) if err != nil { return err } + now := timeNow().UTC() + if found { - _, err = sess.Where("provider = ? AND is_deleted = ?", provider, s.sqlStore.GetDialect().BooleanStr(false)).Update(&models.SSOSetting{ - Settings: data, - Updated: time.Now().UTC(), - }) + updated := &models.SSOSetting{ + Settings: data, + Updated: now, + IsDeleted: false, + } + _, err = sess.UseBool("is_deleted").Update(updated, existing) } else { _, err = sess.Insert(&models.SSOSetting{ ID: uuid.New().String(), Provider: provider, Settings: data, - Created: time.Now().UTC(), - Updated: time.Now().UTC(), + Created: now, + Updated: now, }) } return err }) - - return err } func (s *SSOSettingsStore) Patch(ctx context.Context, provider string, data map[string]interface{}) error { @@ -121,7 +130,7 @@ func (s *SSOSettingsStore) Delete(ctx context.Context, provider string) error { return ssosettings.ErrNotFound } - existing.Updated = time.Now().UTC() + existing.Updated = timeNow().UTC() existing.IsDeleted = true _, err = sess.ID(existing.ID).MustCols("updated", "is_deleted").Update(existing) diff --git a/pkg/services/ssosettings/database/database_test.go b/pkg/services/ssosettings/database/database_test.go index c2462241cee..eb5831b399e 100644 --- a/pkg/services/ssosettings/database/database_test.go +++ b/pkg/services/ssosettings/database/database_test.go @@ -80,45 +80,132 @@ func TestIntegrationUpsertSSOSettings(t *testing.T) { t.Run("insert a new SSO setting successfully", func(t *testing.T) { setup() - expected := &models.SSOSetting{ - Provider: "azuread", - Settings: map[string]interface{}{ - "enabled": true, - }, + mockTimeNow(time.Now()) + defer resetTimeNow() + + provider := "azuread" + settings := map[string]interface{}{ + "enabled": true, + "client_id": "azuread-client", } - err := ssoSettingsStore.Upsert(context.Background(), "azuread", map[string]interface{}{ - "enabled": true, - }) + err := ssoSettingsStore.Upsert(context.Background(), provider, settings) require.NoError(t, err) - actual, err := ssoSettingsStore.Get(context.Background(), "azuread") + actual, err := getSSOSettingsByProvider(sqlStore, provider, false) require.NoError(t, err) + require.Equal(t, settings, actual.Settings) + require.Equal(t, formatTime(timeNow().UTC()), formatTime(actual.Created)) + require.Equal(t, formatTime(timeNow().UTC()), formatTime(actual.Updated)) - require.True(t, maps.Equal(expected.Settings, actual.Settings)) + deleted, notDeleted, err := getSSOSettingsCountByDeleted(sqlStore) + require.NoError(t, err) + require.EqualValues(t, 0, deleted) + require.EqualValues(t, 1, notDeleted) }) t.Run("replaces an existing SSO setting for the specified provider", func(t *testing.T) { setup() - err := ssoSettingsStore.Upsert(context.Background(), "azuread", map[string]interface{}{ - "enabled": true, - }) + mockTimeNow(time.Now()) + defer resetTimeNow() + + provider := "github" + settings := map[string]interface{}{ + "enabled": true, + "client_id": "github-client", + "client_secret": "this-is-a-secret", + } + err := populateSSOSettings(sqlStore, settings, false, provider) require.NoError(t, err) - err = ssoSettingsStore.Upsert(context.Background(), "azuread", map[string]interface{}{ - "enabled": false, - }) + newSettings := map[string]interface{}{ + "enabled": true, + "client_id": "new-github-client", + "client_secret": "this-is-a-new-secret", + } + err = ssoSettingsStore.Upsert(context.Background(), provider, newSettings) require.NoError(t, err) - actual, err := ssoSettingsStore.Get(context.Background(), "azuread") + actual, err := getSSOSettingsByProvider(sqlStore, provider, false) + require.NoError(t, err) + require.Equal(t, newSettings, actual.Settings) + require.Equal(t, formatTime(timeNow().UTC()), formatTime(actual.Updated)) + + deleted, notDeleted, err := getSSOSettingsCountByDeleted(sqlStore) + require.NoError(t, err) + require.EqualValues(t, 0, deleted) + require.EqualValues(t, 1, notDeleted) + }) + + t.Run("trying to update a deleted SSO Settings will insert a new record", func(t *testing.T) { + setup() + + mockTimeNow(time.Now()) + defer resetTimeNow() + + provider := "azuread" + settings := map[string]interface{}{ + "enabled": true, + "client_id": "azuread-client", + "client_secret": "this-is-a-secret", + } + err := populateSSOSettings(sqlStore, settings, true, provider) require.NoError(t, err) - list, err := ssoSettingsStore.List(context.Background()) + newSettings := map[string]interface{}{ + "enabled": true, + "client_id": "new-azuread-client", + "client_secret": "this-is-a-new-secret", + } + + err = ssoSettingsStore.Upsert(context.Background(), provider, newSettings) require.NoError(t, err) - require.Equal(t, 1, len(list)) - require.Equal(t, false, actual.Settings["enabled"]) + actual, err := getSSOSettingsByProvider(sqlStore, provider, false) + require.NoError(t, err) + require.Equal(t, newSettings, actual.Settings) + require.Equal(t, formatTime(timeNow().UTC()), formatTime(actual.Created)) + require.Equal(t, formatTime(timeNow().UTC()), formatTime(actual.Updated)) + + old, err := getSSOSettingsByProvider(sqlStore, provider, true) + require.NoError(t, err) + require.Equal(t, settings, old.Settings) + }) + + t.Run("replaces the settings only for the specified provider leaving the other provider's settings unchanged", func(t *testing.T) { + setup() + + mockTimeNow(time.Now()) + defer resetTimeNow() + + providers := []string{"github", "gitlab", "google"} + settings := map[string]interface{}{ + "enabled": true, + "client_id": "my-client", + "client_secret": "this-is-a-secret", + } + err := populateSSOSettings(sqlStore, settings, false, providers...) + require.NoError(t, err) + + newSettings := map[string]interface{}{ + "enabled": true, + "client_id": "my-new-client", + "client_secret": "this-is-a-new-secret", + } + err = ssoSettingsStore.Upsert(context.Background(), providers[0], newSettings) + require.NoError(t, err) + + actual, err := getSSOSettingsByProvider(sqlStore, providers[0], false) + require.NoError(t, err) + require.Equal(t, newSettings, actual.Settings) + require.Equal(t, formatTime(timeNow().UTC()), formatTime(actual.Updated)) + + for index := 1; index < len(providers); index++ { + existing, err := getSSOSettingsByProvider(sqlStore, providers[index], false) + require.NoError(t, err) + require.Equal(t, settings, existing.Settings) + } }) } @@ -173,18 +260,15 @@ func TestIntegrationDeleteSSOSettings(t *testing.T) { providers := []string{"azuread", "github", "google"} - err := populateSSOSettings(sqlStore, false, providers...) + err := populateSSOSettings(sqlStore, nil, false, providers...) require.NoError(t, err) err = ssoSettingsStore.Delete(context.Background(), providers[0]) require.NoError(t, err) - deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) + deleted, notDeleted, err := getSSOSettingsCountByDeleted(sqlStore) require.NoError(t, err) require.EqualValues(t, 1, deleted) - - notDeleted, err := getSSOSettingsCountByDeleted(sqlStore, false) - require.NoError(t, err) require.EqualValues(t, len(providers)-1, notDeleted) }) @@ -194,19 +278,16 @@ func TestIntegrationDeleteSSOSettings(t *testing.T) { providers := []string{"github", "google", "okta"} invalidProvider := "azuread" - err := populateSSOSettings(sqlStore, false, providers...) + err := populateSSOSettings(sqlStore, nil, false, providers...) require.NoError(t, err) err = ssoSettingsStore.Delete(context.Background(), invalidProvider) require.Error(t, err) require.ErrorIs(t, err, ssosettings.ErrNotFound) - deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) + deleted, notDeleted, err := getSSOSettingsCountByDeleted(sqlStore) require.NoError(t, err) require.EqualValues(t, 0, deleted) - - notDeleted, err := getSSOSettingsCountByDeleted(sqlStore, false) - require.NoError(t, err) require.EqualValues(t, len(providers), notDeleted) }) @@ -215,16 +296,17 @@ func TestIntegrationDeleteSSOSettings(t *testing.T) { providers := []string{"azuread", "github", "google"} - err := populateSSOSettings(sqlStore, true, providers...) + err := populateSSOSettings(sqlStore, nil, true, providers...) require.NoError(t, err) err = ssoSettingsStore.Delete(context.Background(), providers[0]) require.Error(t, err) require.ErrorIs(t, err, ssosettings.ErrNotFound) - deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) + deleted, notDeleted, err := getSSOSettingsCountByDeleted(sqlStore) require.NoError(t, err) require.EqualValues(t, len(providers), deleted) + require.EqualValues(t, 0, notDeleted) }) t.Run("delete one record if more valid sso settings are available for a provider", func(t *testing.T) { @@ -233,20 +315,17 @@ func TestIntegrationDeleteSSOSettings(t *testing.T) { provider := "azuread" // insert sso for the same provider 2 times in the database - err := populateSSOSettings(sqlStore, false, provider) + err := populateSSOSettings(sqlStore, nil, false, provider) require.NoError(t, err) - err = populateSSOSettings(sqlStore, false, provider) + err = populateSSOSettings(sqlStore, nil, false, provider) require.NoError(t, err) err = ssoSettingsStore.Delete(context.Background(), provider) require.NoError(t, err) - deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) + deleted, notDeleted, err := getSSOSettingsCountByDeleted(sqlStore) require.NoError(t, err) require.EqualValues(t, 1, deleted) - - notDeleted, err := getSSOSettingsCountByDeleted(sqlStore, false) - require.NoError(t, err) require.EqualValues(t, 1, notDeleted) }) } @@ -260,13 +339,14 @@ func insertSSOSetting(ssoSettingsStore ssosettings.Store, provider string, setti return ssoSettingsStore.Upsert(context.Background(), provider, settings) } -func populateSSOSettings(sqlStore *sqlstore.SQLStore, deleted bool, providers ...string) error { +func populateSSOSettings(sqlStore *sqlstore.SQLStore, settings map[string]interface{}, deleted bool, providers ...string) error { return sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { for _, provider := range providers { _, err := sess.Insert(&models.SSOSetting{ ID: uuid.New().String(), Provider: provider, - Created: time.Now().UTC(), + Settings: settings, + Created: timeNow().UTC(), IsDeleted: deleted, }) if err != nil { @@ -277,14 +357,41 @@ func populateSSOSettings(sqlStore *sqlstore.SQLStore, deleted bool, providers .. }) } -func getSSOSettingsCountByDeleted(sqlStore *sqlstore.SQLStore, deleted bool) (int64, error) { - var count int64 - var err error - +func getSSOSettingsCountByDeleted(sqlStore *sqlstore.SQLStore) (deleted, notDeleted int64, err error) { err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { - count, err = sess.Table("sso_setting").Where("is_deleted = ?", sqlStore.GetDialect().BooleanStr(deleted)).Count() + deleted, err = sess.Table("sso_setting").Where("is_deleted = ?", sqlStore.GetDialect().BooleanStr(true)).Count() + if err != nil { + return err + } + notDeleted, err = sess.Table("sso_setting").Where("is_deleted = ?", sqlStore.GetDialect().BooleanStr(false)).Count() return err }) - return count, err + return +} + +func getSSOSettingsByProvider(sqlStore *sqlstore.SQLStore, provider string, deleted bool) (*models.SSOSetting, error) { + var model models.SSOSetting + var err error + + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + _, err = sess.Table("sso_setting").Where("provider = ? AND is_deleted = ?", provider, sqlStore.GetDialect().BooleanStr(deleted)).Get(&model) + return err + }) + + return &model, err +} + +func mockTimeNow(timeSeed time.Time) { + timeNow = func() time.Time { + return timeSeed + } +} + +func resetTimeNow() { + timeNow = time.Now +} + +func formatTime(timestamp time.Time) string { + return timestamp.Format(time.RFC3339) }