diff --git a/pkg/services/ssosettings/api/api.go b/pkg/services/ssosettings/api/api.go index 0b062d61fa3d..926802854daa 100644 --- a/pkg/services/ssosettings/api/api.go +++ b/pkg/services/ssosettings/api/api.go @@ -1,6 +1,7 @@ package api import ( + "errors" "net/http" "github.com/grafana/grafana/pkg/api/response" @@ -121,6 +122,9 @@ func (api *Api) removeProviderSettings(c *contextmodel.ReqContext) response.Resp err := api.SSOSettingsService.Delete(c.Req.Context(), key) if err != nil { + if errors.Is(err, ssosettings.ErrNotFound) { + return response.Error(http.StatusNotFound, "The provider was not found", err) + } return response.Error(http.StatusInternalServerError, "Failed to delete provider settings", err) } diff --git a/pkg/services/ssosettings/database/database.go b/pkg/services/ssosettings/database/database.go index fb239b795e5a..f2c5735ffc43 100644 --- a/pkg/services/ssosettings/database/database.go +++ b/pkg/services/ssosettings/database/database.go @@ -5,6 +5,7 @@ import ( "time" "github.com/google/uuid" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ssosettings" @@ -105,15 +106,19 @@ func (s *SSOSettingsStore) Patch(ctx context.Context, provider string, data map[ } func (s *SSOSettingsStore) Delete(ctx context.Context, provider string) error { - err := s.sqlStore.WithDbSession(ctx, func(sess *db.Session) error { - existing := new(models.SSOSetting) - found, err := sess.Where("provider = ? AND is_deleted = ?", provider, s.sqlStore.GetDialect().BooleanStr(false)).Get(existing) + return s.sqlStore.WithDbSession(ctx, func(sess *db.Session) error { + existing := &models.SSOSetting{ + Provider: provider, + IsDeleted: false, + } + + found, err := sess.UseBool("is_deleted").Get(existing) if err != nil { return err } if !found { - return nil // nothing to delete + return ssosettings.ErrNotFound } existing.Updated = time.Now().UTC() @@ -122,5 +127,4 @@ func (s *SSOSettingsStore) Delete(ctx context.Context, provider string) error { _, err = sess.ID(existing.ID).MustCols("updated", "is_deleted").Update(existing) return err }) - return err } diff --git a/pkg/services/ssosettings/database/database_test.go b/pkg/services/ssosettings/database/database_test.go index 6539e8351cb6..c2462241cee7 100644 --- a/pkg/services/ssosettings/database/database_test.go +++ b/pkg/services/ssosettings/database/database_test.go @@ -3,14 +3,16 @@ package database import ( "context" "testing" + "time" + "github.com/google/uuid" + "github.com/stretchr/testify/require" "golang.org/x/exp/maps" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/ssosettings" "github.com/grafana/grafana/pkg/services/ssosettings/models" - "github.com/stretchr/testify/require" ) func TestIntegrationGetSSOSettings(t *testing.T) { @@ -169,39 +171,83 @@ func TestIntegrationDeleteSSOSettings(t *testing.T) { t.Run("soft deletes the settings successfully", func(t *testing.T) { setup() - err := insertSSOSetting(ssoSettingsStore, "azuread", map[string]interface{}{ - "enabled": true, - }) + providers := []string{"azuread", "github", "google"} + + err := populateSSOSettings(sqlStore, false, providers...) require.NoError(t, err) - err = ssoSettingsStore.Delete(context.Background(), "azuread") - + err = ssoSettingsStore.Delete(context.Background(), providers[0]) require.NoError(t, err) - var count int64 - err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { - count, err = sess.Table("sso_setting").Where("is_deleted = ?", sqlStore.GetDialect().BooleanStr(true)).Count() - return err - }) + deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) require.NoError(t, err) + require.EqualValues(t, 1, deleted) - require.Equal(t, int64(1), count) + notDeleted, err := getSSOSettingsCountByDeleted(sqlStore, false) + require.NoError(t, err) + require.EqualValues(t, len(providers)-1, notDeleted) }) - t.Run("return without error if the integration was not found", func(t *testing.T) { + t.Run("return not found if the provider doesn't exist in db", func(t *testing.T) { setup() - err := ssoSettingsStore.Delete(context.Background(), "azuread") + providers := []string{"github", "google", "okta"} + invalidProvider := "azuread" + + err := populateSSOSettings(sqlStore, false, providers...) require.NoError(t, err) - var count int64 - err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { - count, err = sess.Table("sso_setting").Where("is_deleted = ?", sqlStore.GetDialect().BooleanStr(true)).Count() - return err - }) + err = ssoSettingsStore.Delete(context.Background(), invalidProvider) + require.Error(t, err) + require.ErrorIs(t, err, ssosettings.ErrNotFound) + + deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) + require.NoError(t, err) + require.EqualValues(t, 0, deleted) + + notDeleted, err := getSSOSettingsCountByDeleted(sqlStore, false) + require.NoError(t, err) + require.EqualValues(t, len(providers), notDeleted) + }) + + t.Run("return not found if the provider sso settings are already deleted", func(t *testing.T) { + setup() + + providers := []string{"azuread", "github", "google"} + + err := populateSSOSettings(sqlStore, true, providers...) require.NoError(t, err) - require.Equal(t, int64(0), count) + err = ssoSettingsStore.Delete(context.Background(), providers[0]) + require.Error(t, err) + require.ErrorIs(t, err, ssosettings.ErrNotFound) + + deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) + require.NoError(t, err) + require.EqualValues(t, len(providers), deleted) + }) + + t.Run("delete one record if more valid sso settings are available for a provider", func(t *testing.T) { + setup() + + provider := "azuread" + + // insert sso for the same provider 2 times in the database + err := populateSSOSettings(sqlStore, false, provider) + require.NoError(t, err) + err = populateSSOSettings(sqlStore, false, provider) + require.NoError(t, err) + + err = ssoSettingsStore.Delete(context.Background(), provider) + require.NoError(t, err) + + deleted, err := getSSOSettingsCountByDeleted(sqlStore, true) + require.NoError(t, err) + require.EqualValues(t, 1, deleted) + + notDeleted, err := getSSOSettingsCountByDeleted(sqlStore, false) + require.NoError(t, err) + require.EqualValues(t, 1, notDeleted) }) } @@ -213,3 +259,32 @@ 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 { + 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(), + IsDeleted: deleted, + }) + if err != nil { + return err + } + } + return nil + }) +} + +func getSSOSettingsCountByDeleted(sqlStore *sqlstore.SQLStore, deleted bool) (int64, error) { + var count int64 + var 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() + return err + }) + + return count, err +} diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index a5bd60fe5d18..a25cba5b3fe2 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -2,6 +2,7 @@ package ssosettingsimpl import ( "context" + "errors" "fmt" "testing" @@ -360,6 +361,40 @@ func TestSSOSettingsService_List(t *testing.T) { } } +func TestSSOSettingsService_Delete(t *testing.T) { + t.Run("successfully delete SSO settings", func(t *testing.T) { + env := setupTestEnv(t) + + provider := "azuread" + env.store.ExpectedError = nil + + err := env.service.Delete(context.Background(), provider) + require.NoError(t, err) + }) + + t.Run("SSO settings not found for the specified provider", func(t *testing.T) { + env := setupTestEnv(t) + + provider := "azuread" + env.store.ExpectedError = ssosettings.ErrNotFound + + err := env.service.Delete(context.Background(), provider) + require.Error(t, err) + require.ErrorIs(t, err, ssosettings.ErrNotFound) + }) + + t.Run("store fails to delete the SSO settings for the specified provider", func(t *testing.T) { + env := setupTestEnv(t) + + provider := "azuread" + env.store.ExpectedError = errors.New("delete sso settings failed") + + err := env.service.Delete(context.Background(), provider) + require.Error(t, err) + require.NotErrorIs(t, err, ssosettings.ErrNotFound) + }) +} + func setupTestEnv(t *testing.T) testEnv { store := ssosettingstests.NewFakeStore() fallbackStrategy := ssosettingstests.NewFakeFallbackStrategy()