diff --git a/apps/advisor/pkg/app/checkregistry/checkregistry.go b/apps/advisor/pkg/app/checkregistry/checkregistry.go index 5b07b827cdd..224ec552f65 100644 --- a/apps/advisor/pkg/app/checkregistry/checkregistry.go +++ b/apps/advisor/pkg/app/checkregistry/checkregistry.go @@ -2,6 +2,7 @@ package checkregistry import ( "github.com/grafana/grafana/apps/advisor/pkg/app/checks" + "github.com/grafana/grafana/apps/advisor/pkg/app/checks/authchecks" "github.com/grafana/grafana/apps/advisor/pkg/app/checks/datasourcecheck" "github.com/grafana/grafana/apps/advisor/pkg/app/checks/plugincheck" "github.com/grafana/grafana/pkg/plugins" @@ -12,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/services/pluginsintegration/plugininstaller" "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" "github.com/grafana/grafana/pkg/services/pluginsintegration/provisionedplugins" + "github.com/grafana/grafana/pkg/services/ssosettings" ) type CheckService interface { @@ -27,12 +29,13 @@ type Service struct { pluginPreinstall plugininstaller.Preinstall managedPlugins managedplugins.Manager provisionedPlugins provisionedplugins.Manager + ssoSettingsSvc ssosettings.Service } func ProvideService(datasourceSvc datasources.DataSourceService, pluginStore pluginstore.Store, pluginContextProvider *plugincontext.Provider, pluginClient plugins.Client, pluginRepo repo.Service, pluginPreinstall plugininstaller.Preinstall, managedPlugins managedplugins.Manager, - provisionedPlugins provisionedplugins.Manager, + provisionedPlugins provisionedplugins.Manager, ssoSettingsSvc ssosettings.Service, ) *Service { return &Service{ datasourceSvc: datasourceSvc, @@ -43,6 +46,7 @@ func ProvideService(datasourceSvc datasources.DataSourceService, pluginStore plu pluginPreinstall: pluginPreinstall, managedPlugins: managedPlugins, provisionedPlugins: provisionedPlugins, + ssoSettingsSvc: ssoSettingsSvc, } } @@ -62,6 +66,7 @@ func (s *Service) Checks() []checks.Check { s.managedPlugins, s.provisionedPlugins, ), + authchecks.New(s.ssoSettingsSvc), } } diff --git a/apps/advisor/pkg/app/checks/authchecks/check.go b/apps/advisor/pkg/app/checks/authchecks/check.go new file mode 100644 index 00000000000..9cbb2801c02 --- /dev/null +++ b/apps/advisor/pkg/app/checks/authchecks/check.go @@ -0,0 +1,58 @@ +package authchecks + +import ( + "context" + "fmt" + + "github.com/grafana/grafana/apps/advisor/pkg/app/checks" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/ssosettings" +) + +const ( + CheckID = "ssosetting" +) + +var _ checks.Check = (*check)(nil) + +type check struct { + ssoSettingsService ssosettings.Service + log log.Logger +} + +func New(ssoSettingsService ssosettings.Service) checks.Check { + return &check{ + ssoSettingsService: ssoSettingsService, + log: log.New("advisor.ssosettingcheck"), + } +} + +func (c *check) ID() string { + return CheckID +} + +func (c *check) Steps() []checks.Step { + return []checks.Step{ + &listFormatValidation{}, + } +} + +func (c *check) Items(ctx context.Context) ([]any, error) { + ssoSettings, err := c.ssoSettingsService.ListWithRedactedSecrets(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list SSO settings: %w", err) + } + res := make([]any, len(ssoSettings)) + for i, ds := range ssoSettings { + res[i] = ds + } + return res, nil +} + +func (c *check) Item(ctx context.Context, id string) (any, error) { + ssoSetting, err := c.ssoSettingsService.GetForProviderWithRedactedSecrets(ctx, id) + if err != nil { + return nil, err + } + return ssoSetting, nil +} diff --git a/apps/advisor/pkg/app/checks/authchecks/check_test.go b/apps/advisor/pkg/app/checks/authchecks/check_test.go new file mode 100644 index 00000000000..8b117c10da2 --- /dev/null +++ b/apps/advisor/pkg/app/checks/authchecks/check_test.go @@ -0,0 +1,94 @@ +package authchecks + +import ( + "context" + "errors" + "testing" + + "github.com/grafana/grafana/pkg/services/ssosettings/models" + "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" // Correct import path for the mock + "github.com/stretchr/testify/require" +) + +func TestCheck_ID(t *testing.T) { + mockService := ssosettingstests.NewMockService(t) + c := New(mockService) + require.Equal(t, CheckID, c.ID()) +} + +func TestCheck_Items(t *testing.T) { + ctx := context.Background() + + t.Run("Success", func(t *testing.T) { + mockService := ssosettingstests.NewMockService(t) + expectedSettings := []*models.SSOSettings{ + {Provider: "google", Settings: map[string]any{"client_id": "id1"}}, + {Provider: "github", Settings: map[string]any{"client_id": "id2"}}, + } + mockService.On("ListWithRedactedSecrets", ctx).Return(expectedSettings, nil) + + c := New(mockService) + items, err := c.Items(ctx) + + require.NoError(t, err) + require.Len(t, items, len(expectedSettings)) + + actualSettings := make([]*models.SSOSettings, len(items)) + for i, item := range items { + setting, ok := item.(*models.SSOSettings) + require.True(t, ok, "Item should be of type *models.SSOSettings") + actualSettings[i] = setting + } + require.Equal(t, expectedSettings, actualSettings) + }) + + t.Run("Error from service", func(t *testing.T) { + mockService := ssosettingstests.NewMockService(t) + expectedErr := errors.New("database error") + mockService.On("ListWithRedactedSecrets", ctx).Return(nil, expectedErr) + + c := New(mockService) + items, err := c.Items(ctx) + + require.Error(t, err) + require.Nil(t, items) + require.ErrorContains(t, err, "failed to list SSO settings") + require.ErrorIs(t, err, expectedErr) + }) +} + +func TestCheck_Item(t *testing.T) { + ctx := context.Background() + providerID := "google" + + t.Run("Success", func(t *testing.T) { + mockService := ssosettingstests.NewMockService(t) + expectedSetting := &models.SSOSettings{ + Provider: providerID, + Settings: map[string]any{"client_id": "id1"}, + } + mockService.On("GetForProviderWithRedactedSecrets", ctx, providerID).Return(expectedSetting, nil) + + c := New(mockService) + item, err := c.Item(ctx, providerID) + + require.NoError(t, err) + require.NotNil(t, item) + actualSetting, ok := item.(*models.SSOSettings) + require.True(t, ok, "Item should be of type *models.SSOSettings") + require.Equal(t, expectedSetting, actualSetting) + }) + + t.Run("Error from service", func(t *testing.T) { + mockService := ssosettingstests.NewMockService(t) + expectedErr := errors.New("not found") + mockService.On("GetForProviderWithRedactedSecrets", ctx, providerID).Return(nil, expectedErr) + + c := New(mockService) + item, err := c.Item(ctx, providerID) + + require.Error(t, err) + require.Nil(t, item) + require.ErrorIs(t, err, expectedErr) + }) +} diff --git a/apps/advisor/pkg/app/checks/authchecks/list_format_validation.go b/apps/advisor/pkg/app/checks/authchecks/list_format_validation.go new file mode 100644 index 00000000000..304d478a6a7 --- /dev/null +++ b/apps/advisor/pkg/app/checks/authchecks/list_format_validation.go @@ -0,0 +1,104 @@ +package authchecks + +import ( + "context" + "fmt" + "strings" + + advisor "github.com/grafana/grafana/apps/advisor/pkg/apis/advisor/v0alpha1" + "github.com/grafana/grafana/apps/advisor/pkg/app/checks" + "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/ssosettings/models" + "github.com/grafana/grafana/pkg/util" +) + +const ListFormatValidationStepID = "sso-list-format-validation" + +// listSettingKeys defines the SSO setting keys that expect a list format (space-separated, comma-separated or JSON array). +var listSettingKeys = []string{ + "allowed_domains", + "allowed_groups", + "allowed_organizations", + "role_values_none", + "role_values_grafana_admin", + "role_values_admin", + "role_values_editor", + "role_values_viewer", +} + +var _ checks.Step = (*listFormatValidation)(nil) + +// listFormatValidation checks if the specified list parameters in SSO settings are in a valid format. +type listFormatValidation struct{} + +func (s *listFormatValidation) ID() string { + return ListFormatValidationStepID +} + +func (s *listFormatValidation) Title() string { + return "SSO List Setting Format Validation" +} + +func (s *listFormatValidation) Description() string { + return "Checks if list configs in SSO settings are in a valid list format (space-separated, comma-separated or JSON array)." +} + +func (s *listFormatValidation) Resolution() string { + return "Configure the relevant SSO setting using a valid format, like space-separated (\"opt1 opt2\"), comma-separated values (\"opt1, opt2\") or JSON array format ([\"opt1\", \"opt2\"])." +} + +func (s *listFormatValidation) Run(ctx context.Context, _ *advisor.CheckSpec, objToCheck any) (*advisor.CheckReportFailure, error) { + setting, ok := objToCheck.(*models.SSOSettings) + if !ok { + return nil, fmt.Errorf("invalid item type %T", objToCheck) + } + + for _, settingKey := range listSettingKeys { + currentSettingValue, exists := setting.Settings[settingKey] + if !exists || currentSettingValue == nil { + // If the setting is not present or nil, its format is considered valid (or non-applicable). + continue + } + + currentSettingStr, ok := currentSettingValue.(string) + if !ok { + return checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + s.ID(), + fmt.Sprintf("%s - Invalid type for '%s': expected string, got %T", login.GetAuthProviderLabel(setting.Provider), settingKey, currentSettingValue), + setting.Provider, + s.generateLinks(setting.Provider), + ), nil + } + + if currentSettingStr == "" { + continue + } + + _, err := util.SplitStringWithError(currentSettingStr) + if err != nil { + return checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + s.ID(), + fmt.Sprintf("%s - Invalid format for '%s': %s", login.GetAuthProviderLabel(setting.Provider), settingKey, currentSettingStr), + setting.Provider, + s.generateLinks(setting.Provider), + ), nil + } + } + + return nil, nil +} + +func (s *listFormatValidation) generateLinks(provider string) []advisor.CheckErrorLink { + return []advisor.CheckErrorLink{ + { + Url: fmt.Sprintf("https://grafana.com/docs/grafana/latest/setup-grafana/configure-security/configure-authentication/%s", strings.ReplaceAll(provider, "_", "-")), + Message: "Check the documentation", + }, + { + Url: fmt.Sprintf("/admin/authentication/%s", provider), + Message: "Configure provider", + }, + } +} diff --git a/apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go b/apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go new file mode 100644 index 00000000000..62460fde219 --- /dev/null +++ b/apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go @@ -0,0 +1,233 @@ +package authchecks + +import ( + "context" + "fmt" + "strings" + "testing" + + advisor "github.com/grafana/grafana/apps/advisor/pkg/apis/advisor/v0alpha1" + "github.com/grafana/grafana/apps/advisor/pkg/app/checks" + "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/ssosettings/models" + "github.com/stretchr/testify/require" +) + +func TestListFormatValidation_Methods(t *testing.T) { + validator := &listFormatValidation{} + + require.Equal(t, ListFormatValidationStepID, validator.ID()) + require.Equal(t, "SSO List Setting Format Validation", validator.Title()) + require.Equal(t, "Checks if list configs in SSO settings are in a valid list format (space-separated, comma-separated or JSON array).", validator.Description()) + require.Equal(t, "Configure the relevant SSO setting using a valid format, like space-separated (\"opt1 opt2\"), comma-separated values (\"opt1, opt2\") or JSON array format ([\"opt1\", \"opt2\"]).", validator.Resolution()) +} + +func TestListFormatValidation_Run(t *testing.T) { + validator := &listFormatValidation{} + ctx := context.Background() + spec := &advisor.CheckSpec{} + provider := "generic_oauth" + providerLabel := login.GetAuthProviderLabel(provider) + + tests := []struct { + name string + objToCheck any + expectedError string + expectedFailure *advisor.CheckReportFailure + }{ + { + name: "invalid object type", + objToCheck: struct{}{}, + expectedError: "invalid item type struct {}", + }, + { + name: "no relevant settings exist", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{"other_setting": "value"}, + }, + expectedFailure: nil, + }, + { + name: "one setting exists and is nil", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_groups": nil, + }, + }, + expectedFailure: nil, + }, + { + name: "one setting exists and is empty string", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_groups": "", + }, + }, + expectedFailure: nil, + }, + { + name: "one setting exists and is empty JSON array", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_groups": "[]", + }, + }, + expectedFailure: nil, + }, + { + name: "one setting exists and is valid (comma-separated)", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_groups": "group1, group2", + }, + }, + expectedFailure: nil, + }, + { + name: "one setting exists and is valid (JSON array)", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_domains": `["domain1.com", "domain2.com"]`, + }, + }, + expectedFailure: nil, + }, + { + name: "one setting exists and is valid (space-separated)", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_groups": "group1 group2", + }, + }, + expectedFailure: nil, + }, + { + name: "one setting exists and is not a string", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_groups": 123, + }, + }, + expectedFailure: checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + ListFormatValidationStepID, + fmt.Sprintf("%s - Invalid type for '%s': expected string, got %T", providerLabel, "allowed_groups", 123), + provider, + generateExpectedLinks(provider), + ), + }, + { + name: "one setting exists and has invalid format (bad JSON)", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_groups": `["group1", "group2"`, + }, + }, + expectedFailure: checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + ListFormatValidationStepID, + fmt.Sprintf("%s - Invalid format for '%s': %s", providerLabel, "allowed_groups", `["group1", "group2"`), + provider, + generateExpectedLinks(provider), + ), + }, + { + name: "multiple settings exist, first one is invalid (type)", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_domains": 123, + "allowed_groups": "group1, group2", + }, + }, + expectedFailure: checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + ListFormatValidationStepID, + fmt.Sprintf("%s - Invalid type for '%s': expected string, got %T", providerLabel, "allowed_domains", 123), + provider, + generateExpectedLinks(provider), + ), + }, + { + name: "multiple settings exist, second one is invalid (format)", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_domains": "domain1.com", + "allowed_groups": `["group1",`, + }, + }, + expectedFailure: checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + ListFormatValidationStepID, + fmt.Sprintf("%s - Invalid format for '%s': %s", providerLabel, "allowed_groups", `["group1",`), + provider, + generateExpectedLinks(provider), + ), + }, + { + name: "all settings exist and are valid", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_domains": "d1.com, d2.com", + "allowed_groups": `["g1", "g2"]`, + "allowed_organizations": "org1", + "role_values_none": "None", + "role_values_grafana_admin": "GrafanaAdmin", + "role_values_admin": "Admin", + "role_values_editor": "Editor", + "role_values_viewer": "Viewer", + }, + }, + expectedFailure: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + failure, err := validator.Run(ctx, spec, tt.objToCheck) + + if tt.expectedError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tt.expectedError) + require.Nil(t, failure) + return + } + + require.NoError(t, err) + if tt.expectedFailure != nil { + require.NotNil(t, failure, "Expected a failure report, but got nil") + require.Equal(t, tt.expectedFailure.Severity, failure.Severity) + require.Equal(t, tt.expectedFailure.StepID, failure.StepID) + require.Equal(t, tt.expectedFailure.Item, failure.Item) + require.Equal(t, tt.expectedFailure.ItemID, failure.ItemID) + require.ElementsMatch(t, tt.expectedFailure.Links, failure.Links) + } else { + require.Nil(t, failure, "Expected no failure report, but got one: %+v", failure) + } + }) + } +} + +func generateExpectedLinks(provider string) []advisor.CheckErrorLink { + return []advisor.CheckErrorLink{ + { + Url: fmt.Sprintf("https://grafana.com/docs/grafana/latest/setup-grafana/configure-security/configure-authentication/%s", strings.ReplaceAll(provider, "_", "-")), + Message: "Check the documentation", + }, + { + Url: fmt.Sprintf("/admin/authentication/%s", provider), + Message: "Configure provider", + }, + } +} diff --git a/pkg/services/login/authinfo.go b/pkg/services/login/authinfo.go index 32c66beef59..3e9751d0ea2 100644 --- a/pkg/services/login/authinfo.go +++ b/pkg/services/login/authinfo.go @@ -2,6 +2,7 @@ package login import ( "context" + "strings" ) type AuthInfoService interface { @@ -56,22 +57,23 @@ const ( OktaLabel = "Okta" ) -// used for frontend to display a more user friendly label +// GetAuthProviderLabel returns the label for the given auth module. +// Used for frontend to display a more user friendly label. func GetAuthProviderLabel(authModule string) string { switch authModule { - case GithubAuthModule: + case GithubAuthModule, strings.TrimPrefix(GithubAuthModule, "oauth_"): return GithubLabel - case GoogleAuthModule: + case GoogleAuthModule, strings.TrimPrefix(GoogleAuthModule, "oauth_"): return GoogleLabel - case AzureADAuthModule: + case AzureADAuthModule, strings.TrimPrefix(AzureADAuthModule, "oauth_"): return AzureADLabel - case GitLabAuthModule: + case GitLabAuthModule, strings.TrimPrefix(GitLabAuthModule, "oauth_"): return GitLabLabel - case OktaAuthModule: + case OktaAuthModule, strings.TrimPrefix(OktaAuthModule, "oauth_"): return OktaLabel - case GrafanaComAuthModule, GrafanaNetAuthModule: + case GrafanaComAuthModule, GrafanaNetAuthModule, strings.TrimPrefix(GrafanaComAuthModule, "oauth_"), strings.TrimPrefix(GrafanaNetAuthModule, "oauth_"): return GrafanaComLabel - case SAMLAuthModule: + case SAMLAuthModule, strings.TrimPrefix(SAMLAuthModule, "auth."): return SAMLLabel case LDAPAuthModule, "": // FIXME: verify this situation doesn't exist anymore return LDAPLabel @@ -79,7 +81,7 @@ func GetAuthProviderLabel(authModule string) string { return JWTLabel case AuthProxyAuthModule: return AuthProxyLabel - case GenericOAuthModule: + case GenericOAuthModule, strings.TrimPrefix(GenericOAuthModule, "oauth_"): return GenericOAuthLabel default: return "Unknown"