Advisor: Add checks for list validation for SSO Settings service (#104520)

* Add format validation for allowed_organizations and allowed_groups

* Refactor, introduce ListFormatValidation

* Add tests

* Update apps/advisor/pkg/app/checks/authchecks/list_format_validation.go

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

* Update apps/advisor/pkg/app/checks/authchecks/list_format_validation.go

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

* Use one step instead of multiple separate ones

---------

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
This commit is contained in:
Misi
2025-05-05 12:10:49 +02:00
committed by GitHub
co-authored by Andres Martinez Gotor
parent ddf33bcb66
commit 7d1eda2e5e
6 changed files with 506 additions and 10 deletions
@@ -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),
}
}
@@ -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
}
@@ -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)
})
}
@@ -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",
},
}
}
@@ -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",
},
}
}
+11 -9
View File
@@ -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"