From b1dc505a2b8706a062496397992adb6c02a8c190 Mon Sep 17 00:00:00 2001 From: Misi Date: Fri, 9 Feb 2024 13:10:23 +0100 Subject: [PATCH] Auth: Validate admin assignment in SSO Settings (#82233) * Add validation for allowAssignGrafanaAdmin * Update default values * Do not render hidden fields * Change error message * Improve tests --------- Co-authored-by: Clarity-89 --- pkg/login/social/connectors/azuread_oauth.go | 5 +-- .../social/connectors/azuread_oauth_test.go | 27 ++++++++++++--- pkg/login/social/connectors/generic_oauth.go | 5 +-- .../social/connectors/generic_oauth_test.go | 32 +++++++++++++++--- pkg/login/social/connectors/github_oauth.go | 5 +-- .../social/connectors/github_oauth_test.go | 33 ++++++++++++++++--- pkg/login/social/connectors/gitlab_oauth.go | 5 +-- .../social/connectors/gitlab_oauth_test.go | 33 ++++++++++++++++--- pkg/login/social/connectors/google_oauth.go | 5 +-- .../social/connectors/google_oauth_test.go | 33 ++++++++++++++++--- .../social/connectors/grafana_com_oauth.go | 5 +-- .../connectors/grafana_com_oauth_test.go | 27 +++++++++++++-- pkg/login/social/connectors/okta_oauth.go | 5 +-- .../social/connectors/okta_oauth_test.go | 32 +++++++++++++++--- pkg/login/social/connectors/social_base.go | 9 +++-- pkg/services/ssosettings/api/api.go | 2 +- pkg/services/ssosettings/api/api_test.go | 14 ++++---- pkg/services/ssosettings/ssosettings.go | 5 +-- .../ssosettings/ssosettingsimpl/service.go | 5 +-- .../ssosettingsimpl/service_test.go | 31 ++++++++--------- .../ssosettingstests/reloadable_mock.go | 14 ++++---- .../ssosettingstests/service_mock.go | 14 ++++---- .../features/auth-config/FieldRenderer.tsx | 6 +++- public/app/features/auth-config/fields.tsx | 4 ++- public/app/features/auth-config/types.ts | 3 +- 25 files changed, 271 insertions(+), 88 deletions(-) diff --git a/pkg/login/social/connectors/azuread_oauth.go b/pkg/login/social/connectors/azuread_oauth.go index 712d0f145ba..435f1c209ff 100644 --- a/pkg/login/social/connectors/azuread_oauth.go +++ b/pkg/login/social/connectors/azuread_oauth.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/ssosettings" @@ -185,13 +186,13 @@ func (s *SocialAzureAD) Reload(ctx context.Context, settings ssoModels.SSOSettin return nil } -func (s *SocialAzureAD) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { +func (s *SocialAzureAD) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - err = validateInfo(info) + err = validateInfo(info, requester) if err != nil { return err } diff --git a/pkg/login/social/connectors/azuread_oauth_test.go b/pkg/login/social/connectors/azuread_oauth_test.go index e59803135b2..39da9b7f76e 100644 --- a/pkg/login/social/connectors/azuread_oauth_test.go +++ b/pkg/login/social/connectors/azuread_oauth_test.go @@ -18,10 +18,12 @@ import ( "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -992,9 +994,10 @@ func TestSocialAzureAD_InitializeExtraFields(t *testing.T) { func TestSocialAzureAD_Validate(t *testing.T) { testCases := []struct { - name string - settings ssoModels.SSOSettings - wantErr error + name string + settings ssoModels.SSOSettings + requester identity.Requester + wantErr error }{ { name: "SSOSettings is valid", @@ -1052,13 +1055,29 @@ func TestSocialAzureAD_Validate(t *testing.T) { }, wantErr: ssosettings.ErrBaseInvalidOAuthConfig, }, + { + name: "fails if the user is not allowed to update allow assign grafana admin", + requester: &user.SignedInUser{ + IsGrafanaAdmin: false, + }, + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "allow_assign_grafana_admin": "true", + }, + }, + wantErr: ssosettings.ErrBaseInvalidOAuthConfig, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := NewAzureADProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures(), nil) - err := s.Validate(context.Background(), tc.settings) + if tc.requester == nil { + tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} + } + err := s.Validate(context.Background(), tc.settings, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/generic_oauth.go b/pkg/login/social/connectors/generic_oauth.go index 3e1c3cecb38..a001aef73d4 100644 --- a/pkg/login/social/connectors/generic_oauth.go +++ b/pkg/login/social/connectors/generic_oauth.go @@ -13,6 +13,7 @@ import ( "golang.org/x/oauth2" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" @@ -67,13 +68,13 @@ func NewGenericOAuthProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettin return provider } -func (s *SocialGenericOAuth) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { +func (s *SocialGenericOAuth) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - err = validateInfo(info) + err = validateInfo(info, requester) if err != nil { return err } diff --git a/pkg/login/social/connectors/generic_oauth_test.go b/pkg/login/social/connectors/generic_oauth_test.go index edcc5eb76e3..bf991f18712 100644 --- a/pkg/login/social/connectors/generic_oauth_test.go +++ b/pkg/login/social/connectors/generic_oauth_test.go @@ -13,11 +13,13 @@ import ( "golang.org/x/oauth2" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -920,17 +922,20 @@ func TestSocialGenericOAuth_InitializeExtraFields(t *testing.T) { func TestSocialGenericOAuth_Validate(t *testing.T) { testCases := []struct { - name string - settings ssoModels.SSOSettings - wantErr error + name string + settings ssoModels.SSOSettings + requester identity.Requester + wantErr error }{ { name: "SSOSettings is valid", settings: ssoModels.SSOSettings{ Settings: map[string]any{ - "client_id": "client-id", + "client_id": "client-id", + "allow_assign_grafana_admin": "true", }, }, + requester: &user.SignedInUser{IsGrafanaAdmin: true}, }, { name: "fails if settings map contains an invalid field", @@ -969,13 +974,30 @@ func TestSocialGenericOAuth_Validate(t *testing.T) { }, wantErr: ssosettings.ErrBaseInvalidOAuthConfig, }, + { + name: "fails if the user is not allowed to update allow assign grafana admin", + requester: &user.SignedInUser{ + IsGrafanaAdmin: false, + }, + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "allow_assign_grafana_admin": "true", + "skip_org_role_sync": "true", + }, + }, + wantErr: ssosettings.ErrBaseInvalidOAuthConfig, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := NewGenericOAuthProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) - err := s.Validate(context.Background(), tc.settings) + if tc.requester == nil { + tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} + } + err := s.Validate(context.Background(), tc.settings, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/github_oauth.go b/pkg/login/social/connectors/github_oauth.go index de7fc6def39..8e9bddfc316 100644 --- a/pkg/login/social/connectors/github_oauth.go +++ b/pkg/login/social/connectors/github_oauth.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" @@ -74,13 +75,13 @@ func NewGitHubProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings sso return provider } -func (s *SocialGithub) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { +func (s *SocialGithub) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - err = validateInfo(info) + err = validateInfo(info, requester) if err != nil { return err } diff --git a/pkg/login/social/connectors/github_oauth_test.go b/pkg/login/social/connectors/github_oauth_test.go index f0e09a85e07..9b27703da7d 100644 --- a/pkg/login/social/connectors/github_oauth_test.go +++ b/pkg/login/social/connectors/github_oauth_test.go @@ -12,10 +12,12 @@ import ( "golang.org/x/oauth2" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -346,17 +348,20 @@ func TestSocialGitHub_InitializeExtraFields(t *testing.T) { func TestSocialGitHub_Validate(t *testing.T) { testCases := []struct { - name string - settings ssoModels.SSOSettings - wantErr error + name string + settings ssoModels.SSOSettings + requester identity.Requester + wantErr error }{ { name: "SSOSettings is valid", settings: ssoModels.SSOSettings{ Settings: map[string]any{ - "client_id": "client-id", + "client_id": "client-id", + "allow_assign_grafana_admin": "true", }, }, + requester: &user.SignedInUser{IsGrafanaAdmin: true}, }, { name: "fails if settings map contains an invalid field", @@ -405,13 +410,31 @@ func TestSocialGitHub_Validate(t *testing.T) { }, wantErr: ssosettings.ErrBaseInvalidOAuthConfig, }, + { + name: "fails if the user is not allowed to update allow assign grafana admin", + requester: &user.SignedInUser{ + IsGrafanaAdmin: false, + }, + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "allow_assign_grafana_admin": "true", + "skip_org_role_sync": "true", + }, + }, + wantErr: ssosettings.ErrBaseInvalidOAuthConfig, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := NewGitHubProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) - err := s.Validate(context.Background(), tc.settings) + if tc.requester == nil { + tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} + } + + err := s.Validate(context.Background(), tc.settings, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/gitlab_oauth.go b/pkg/login/social/connectors/gitlab_oauth.go index 4025c26d1bf..329b4d58c11 100644 --- a/pkg/login/social/connectors/gitlab_oauth.go +++ b/pkg/login/social/connectors/gitlab_oauth.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" @@ -64,13 +65,13 @@ func NewGitLabProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings sso return provider } -func (s *SocialGitlab) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { +func (s *SocialGitlab) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - err = validateInfo(info) + err = validateInfo(info, requester) if err != nil { return err } diff --git a/pkg/login/social/connectors/gitlab_oauth_test.go b/pkg/login/social/connectors/gitlab_oauth_test.go index f2bf015da03..926bccaae9f 100644 --- a/pkg/login/social/connectors/gitlab_oauth_test.go +++ b/pkg/login/social/connectors/gitlab_oauth_test.go @@ -16,11 +16,13 @@ import ( "golang.org/x/oauth2" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -464,17 +466,20 @@ func TestSocialGitlab_GetGroupsNextPage(t *testing.T) { func TestSocialGitlab_Validate(t *testing.T) { testCases := []struct { - name string - settings ssoModels.SSOSettings - wantErr error + name string + settings ssoModels.SSOSettings + requester identity.Requester + wantErr error }{ { name: "SSOSettings is valid", settings: ssoModels.SSOSettings{ Settings: map[string]any{ - "client_id": "client-id", + "client_id": "client-id", + "allow_assign_grafana_admin": "true", }, }, + requester: &user.SignedInUser{IsGrafanaAdmin: true}, }, { name: "fails if settings map contains an invalid field", @@ -513,13 +518,31 @@ func TestSocialGitlab_Validate(t *testing.T) { }, wantErr: ssosettings.ErrBaseInvalidOAuthConfig, }, + { + name: "fails if the user is not allowed to update allow assign grafana admin", + requester: &user.SignedInUser{ + IsGrafanaAdmin: false, + }, + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "allow_assign_grafana_admin": "true", + "skip_org_role_sync": "true", + }, + }, + wantErr: ssosettings.ErrBaseInvalidOAuthConfig, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := NewGitLabProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) - err := s.Validate(context.Background(), tc.settings) + if tc.requester == nil { + tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} + } + + err := s.Validate(context.Background(), tc.settings, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/google_oauth.go b/pkg/login/social/connectors/google_oauth.go index 1398f045397..6964a21dd6c 100644 --- a/pkg/login/social/connectors/google_oauth.go +++ b/pkg/login/social/connectors/google_oauth.go @@ -11,6 +11,7 @@ import ( "golang.org/x/oauth2" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" @@ -54,13 +55,13 @@ func NewGoogleProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings sso return provider } -func (s *SocialGoogle) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { +func (s *SocialGoogle) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - err = validateInfo(info) + err = validateInfo(info, requester) if err != nil { return err } diff --git a/pkg/login/social/connectors/google_oauth_test.go b/pkg/login/social/connectors/google_oauth_test.go index df66caa0d7c..055f416952f 100644 --- a/pkg/login/social/connectors/google_oauth_test.go +++ b/pkg/login/social/connectors/google_oauth_test.go @@ -16,10 +16,12 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -669,17 +671,20 @@ func TestSocialGoogle_UserInfo(t *testing.T) { func TestSocialGoogle_Validate(t *testing.T) { testCases := []struct { - name string - settings ssoModels.SSOSettings - wantErr error + name string + settings ssoModels.SSOSettings + requester identity.Requester + wantErr error }{ { name: "SSOSettings is valid", settings: ssoModels.SSOSettings{ Settings: map[string]any{ - "client_id": "client-id", + "client_id": "client-id", + "allow_assign_grafana_admin": "true", }, }, + requester: &user.SignedInUser{IsGrafanaAdmin: true}, }, { name: "fails if settings map contains an invalid field", @@ -718,13 +723,31 @@ func TestSocialGoogle_Validate(t *testing.T) { }, wantErr: ssosettings.ErrBaseInvalidOAuthConfig, }, + { + name: "fails if the user is not allowed to update allow assign grafana admin", + requester: &user.SignedInUser{ + IsGrafanaAdmin: false, + }, + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "allow_assign_grafana_admin": "true", + "skip_org_role_sync": "true", + }, + }, + wantErr: ssosettings.ErrBaseInvalidOAuthConfig, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := NewGoogleProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) - err := s.Validate(context.Background(), tc.settings) + if tc.requester == nil { + tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} + } + + err := s.Validate(context.Background(), tc.settings, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/grafana_com_oauth.go b/pkg/login/social/connectors/grafana_com_oauth.go index 60c3d23e51e..1db16e06c29 100644 --- a/pkg/login/social/connectors/grafana_com_oauth.go +++ b/pkg/login/social/connectors/grafana_com_oauth.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/ssosettings" @@ -52,13 +53,13 @@ func NewGrafanaComProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings return provider } -func (s *SocialGrafanaCom) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { +func (s *SocialGrafanaCom) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - err = validateInfo(info) + err = validateInfo(info, requester) if err != nil { return err } diff --git a/pkg/login/social/connectors/grafana_com_oauth_test.go b/pkg/login/social/connectors/grafana_com_oauth_test.go index bdcf15e081b..dc800775f89 100644 --- a/pkg/login/social/connectors/grafana_com_oauth_test.go +++ b/pkg/login/social/connectors/grafana_com_oauth_test.go @@ -10,9 +10,11 @@ import ( "golang.org/x/oauth2" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -139,15 +141,18 @@ func TestSocialGrafanaCom_Validate(t *testing.T) { testCases := []struct { name string settings ssoModels.SSOSettings + requester identity.Requester expectError bool }{ { name: "SSOSettings is valid", settings: ssoModels.SSOSettings{ Settings: map[string]any{ - "client_id": "client-id", + "client_id": "client-id", + "allow_assign_grafana_admin": "true", }, }, + requester: &user.SignedInUser{IsGrafanaAdmin: true}, expectError: false, }, { @@ -176,13 +181,31 @@ func TestSocialGrafanaCom_Validate(t *testing.T) { }, expectError: true, }, + { + name: "fails if the user is not allowed to update allow assign grafana admin", + requester: &user.SignedInUser{ + IsGrafanaAdmin: false, + }, + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "allow_assign_grafana_admin": "true", + "skip_org_role_sync": "true", + }, + }, + expectError: true, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := NewGrafanaComProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) - err := s.Validate(context.Background(), tc.settings) + if tc.requester == nil { + tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} + } + + err := s.Validate(context.Background(), tc.settings, tc.requester) if tc.expectError { require.Error(t, err) } else { diff --git a/pkg/login/social/connectors/okta_oauth.go b/pkg/login/social/connectors/okta_oauth.go index 39eb41a1bc6..8354ab1a20f 100644 --- a/pkg/login/social/connectors/okta_oauth.go +++ b/pkg/login/social/connectors/okta_oauth.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" @@ -60,13 +61,13 @@ func NewOktaProvider(info *social.OAuthInfo, cfg *setting.Cfg, ssoSettings ssose return provider } -func (s *SocialOkta) Validate(ctx context.Context, settings ssoModels.SSOSettings) error { +func (s *SocialOkta) Validate(ctx context.Context, settings ssoModels.SSOSettings, requester identity.Requester) error { info, err := CreateOAuthInfoFromKeyValues(settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - err = validateInfo(info) + err = validateInfo(info, requester) if err != nil { return err } diff --git a/pkg/login/social/connectors/okta_oauth_test.go b/pkg/login/social/connectors/okta_oauth_test.go index 2bddad48984..e390680a522 100644 --- a/pkg/login/social/connectors/okta_oauth_test.go +++ b/pkg/login/social/connectors/okta_oauth_test.go @@ -14,10 +14,12 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/models/roletype" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -137,17 +139,20 @@ func TestSocialOkta_UserInfo(t *testing.T) { func TestSocialOkta_Validate(t *testing.T) { testCases := []struct { - name string - settings ssoModels.SSOSettings - wantErr error + name string + settings ssoModels.SSOSettings + requester identity.Requester + wantErr error }{ { name: "SSOSettings is valid", settings: ssoModels.SSOSettings{ Settings: map[string]any{ - "client_id": "client-id", + "client_id": "client-id", + "allow_assign_grafana_admin": "true", }, }, + requester: &user.SignedInUser{IsGrafanaAdmin: true}, }, { name: "fails if settings map contains an invalid field", @@ -186,13 +191,30 @@ func TestSocialOkta_Validate(t *testing.T) { }, wantErr: ssosettings.ErrBaseInvalidOAuthConfig, }, + { + name: "fails if the user is not allowed to update allow assign grafana admin", + requester: &user.SignedInUser{ + IsGrafanaAdmin: false, + }, + settings: ssoModels.SSOSettings{ + Settings: map[string]any{ + "client_id": "client-id", + "allow_assign_grafana_admin": "true", + "skip_org_role_sync": "true", + }, + }, + wantErr: ssosettings.ErrBaseInvalidOAuthConfig, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := NewOktaProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures()) - err := s.Validate(context.Background(), tc.settings) + if tc.requester == nil { + tc.requester = &user.SignedInUser{IsGrafanaAdmin: false} + } + err := s.Validate(context.Background(), tc.settings, tc.requester) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return diff --git a/pkg/login/social/connectors/social_base.go b/pkg/login/social/connectors/social_base.go index 31f8163cbba..b57d3c0dd5e 100644 --- a/pkg/login/social/connectors/social_base.go +++ b/pkg/login/social/connectors/social_base.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/ssosettings" @@ -220,9 +221,13 @@ func getRoleFromSearch(role string) (org.RoleType, bool) { return org.RoleType(cases.Title(language.Und).String(role)), false } -func validateInfo(info *social.OAuthInfo) error { +func validateInfo(info *social.OAuthInfo, requester identity.Requester) error { if info.ClientId == "" { - return ssosettings.ErrInvalidOAuthConfig("ClientId is empty") + return ssosettings.ErrInvalidOAuthConfig("Client Id is empty.") + } + + if info.AllowAssignGrafanaAdmin && !requester.GetIsGrafanaAdmin() { + return ssosettings.ErrInvalidOAuthConfig("Allow assign Grafana Admin can only be updated by Grafana Server Admins.") } if info.AllowAssignGrafanaAdmin && info.SkipOrgRoleSync { diff --git a/pkg/services/ssosettings/api/api.go b/pkg/services/ssosettings/api/api.go index 4ebc206a137..d45c2d0db57 100644 --- a/pkg/services/ssosettings/api/api.go +++ b/pkg/services/ssosettings/api/api.go @@ -178,7 +178,7 @@ func (api *Api) updateProviderSettings(c *contextmodel.ReqContext) response.Resp settings.Provider = key - err := api.SSOSettingsService.Upsert(c.Req.Context(), &settings) + err := api.SSOSettingsService.Upsert(c.Req.Context(), &settings, c.SignedInUser) if err != nil { return response.ErrOrFallback(http.StatusInternalServerError, "Failed to update provider settings", err) } diff --git a/pkg/services/ssosettings/api/api_test.go b/pkg/services/ssosettings/api/api_test.go index 3421c4e5dcb..2530f3cf0aa 100644 --- a/pkg/services/ssosettings/api/api_test.go +++ b/pkg/services/ssosettings/api/api_test.go @@ -132,19 +132,21 @@ func TestSSOSettingsAPI_Update(t *testing.T) { Settings: input.Settings, } + signedInUser := &user.SignedInUser{ + OrgRole: org.RoleAdmin, + OrgID: 1, + Permissions: getPermissionsForActionAndScope(tt.action, tt.scope), + } + service := ssosettingstests.NewMockService(t) if tt.expectedServiceCall { - service.On("Upsert", mock.Anything, &settings).Return(tt.expectedError).Once() + service.On("Upsert", mock.Anything, &settings, signedInUser).Return(tt.expectedError).Once() } server := setupTests(t, service) path := fmt.Sprintf("/api/v1/sso-settings/%s", tt.key) req := server.NewRequest(http.MethodPut, path, bytes.NewBufferString(tt.body)) - webtest.RequestWithSignedInUser(req, &user.SignedInUser{ - OrgRole: org.RoleEditor, - OrgID: 1, - Permissions: getPermissionsForActionAndScope(tt.action, tt.scope), - }) + webtest.RequestWithSignedInUser(req, signedInUser) res, err := server.SendJSON(req) require.NoError(t, err) diff --git a/pkg/services/ssosettings/ssosettings.go b/pkg/services/ssosettings/ssosettings.go index f31d30b4fde..be0bdaac718 100644 --- a/pkg/services/ssosettings/ssosettings.go +++ b/pkg/services/ssosettings/ssosettings.go @@ -4,6 +4,7 @@ import ( "context" "github.com/grafana/grafana/pkg/login/social" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/ssosettings/models" ) @@ -28,7 +29,7 @@ type Service interface { // GetForProviderWithRedactedSecrets returns the SSO settings for a given provider (DB or config file) with secret values redacted GetForProviderWithRedactedSecrets(ctx context.Context, provider string) (*models.SSOSettings, error) // Upsert creates or updates the SSO settings for a given provider - Upsert(ctx context.Context, settings *models.SSOSettings) error + Upsert(ctx context.Context, settings *models.SSOSettings, requester identity.Requester) error // Delete deletes the SSO settings for a given provider (soft delete) Delete(ctx context.Context, provider string) error // Patch updates the specified SSO settings (key-value pairs) for a given provider @@ -44,7 +45,7 @@ type Service interface { //go:generate mockery --name Reloadable --structname MockReloadable --outpkg ssosettingstests --filename reloadable_mock.go --output ./ssosettingstests/ type Reloadable interface { Reload(ctx context.Context, settings models.SSOSettings) error - Validate(ctx context.Context, settings models.SSOSettings) error + Validate(ctx context.Context, settings models.SSOSettings, requester identity.Requester) error } // FallbackStrategy is an interface that can be implemented to allow a provider to load settings from a different source diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 02b1319dd79..9466848d7ce 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/usagestats" ac "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/secrets" "github.com/grafana/grafana/pkg/services/ssosettings" @@ -164,7 +165,7 @@ func (s *Service) ListWithRedactedSecrets(ctx context.Context) ([]*models.SSOSet return configurableSettings, nil } -func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings) error { +func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings, requester identity.Requester) error { if !s.isProviderConfigurable(settings.Provider) { return ssosettings.ErrNotConfigurable } @@ -174,7 +175,7 @@ func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings) erro return ssosettings.ErrInvalidProvider.Errorf("provider %s not found in reloadables", settings.Provider) } - err := social.Validate(ctx, *settings) + err := social.Validate(ctx, *settings, requester) if err != nil { return err } diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index 333efa33a04..6f736c5479e 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -22,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/services/ssosettings" "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/ssosettingstests" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" ) @@ -796,7 +797,7 @@ func TestService_Upsert(t *testing.T) { wg.Add(1) reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.MatchedBy(func(settings models.SSOSettings) bool { defer wg.Done() return settings.Provider == provider && @@ -830,7 +831,7 @@ func TestService_Upsert(t *testing.T) { }, }, nil } - err := env.service.Upsert(context.Background(), &settings) + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) require.NoError(t, err) // Wait for the goroutine first to assert the Reload call @@ -859,7 +860,7 @@ func TestService_Upsert(t *testing.T) { reloadable := ssosettingstests.NewMockReloadable(t) env.reloadables[provider] = reloadable - err := env.service.Upsert(context.Background(), settings) + err := env.service.Upsert(context.Background(), settings, &user.SignedInUser{}) require.Error(t, err) }) @@ -883,7 +884,7 @@ func TestService_Upsert(t *testing.T) { // the reloadable is available for other provider env.reloadables["github"] = reloadable - err := env.service.Upsert(context.Background(), settings) + err := env.service.Upsert(context.Background(), settings, &user.SignedInUser{}) require.Error(t, err) }) @@ -904,10 +905,10 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings).Return(errors.New("validation failed")) + reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(errors.New("validation failed")) env.reloadables[provider] = reloadable - err := env.service.Upsert(context.Background(), &settings) + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) require.Error(t, err) }) @@ -928,7 +929,7 @@ func TestService_Upsert(t *testing.T) { env.fallbackStrategy.ExpectedIsMatch = false - err := env.service.Upsert(context.Background(), settings) + err := env.service.Upsert(context.Background(), settings, &user.SignedInUser{}) require.Error(t, err) }) @@ -949,11 +950,11 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) env.reloadables[provider] = reloadable env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return(nil, errors.New("encryption failed")).Once() - err := env.service.Upsert(context.Background(), &settings) + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) require.Error(t, err) }) @@ -981,13 +982,13 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.Anything).Return(nil).Maybe() env.reloadables[provider] = reloadable env.secrets.On("Decrypt", mock.Anything, []byte("current-client-secret"), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once() env.secrets.On("Encrypt", mock.Anything, []byte("encrypted-client-secret"), mock.Anything).Return([]byte("current-client-secret"), nil).Once() - err := env.service.Upsert(context.Background(), &settings) + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) require.NoError(t, err) settings.Settings["client_secret"] = base64.RawStdEncoding.EncodeToString([]byte("current-client-secret")) @@ -1011,7 +1012,7 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) env.reloadables[provider] = reloadable env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once() env.store.GetFn = func(ctx context.Context, provider string) (*models.SSOSettings, error) { @@ -1022,7 +1023,7 @@ func TestService_Upsert(t *testing.T) { return errors.New("failed to upsert settings") } - err := env.service.Upsert(context.Background(), &settings) + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) require.Error(t, err) }) @@ -1043,12 +1044,12 @@ func TestService_Upsert(t *testing.T) { } reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings).Return(nil) + reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.Anything).Return(errors.New("failed reloading new settings")).Maybe() env.reloadables[provider] = reloadable env.secrets.On("Encrypt", mock.Anything, []byte(settings.Settings["client_secret"].(string)), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once() - err := env.service.Upsert(context.Background(), &settings) + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) require.NoError(t, err) settings.Settings["client_secret"] = base64.RawStdEncoding.EncodeToString([]byte("encrypted-client-secret")) diff --git a/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go b/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go index a6c0526cfa4..599fe1c5c19 100644 --- a/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go +++ b/pkg/services/ssosettings/ssosettingstests/reloadable_mock.go @@ -5,8 +5,10 @@ package ssosettingstests import ( context "context" - models "github.com/grafana/grafana/pkg/services/ssosettings/models" + identity "github.com/grafana/grafana/pkg/services/auth/identity" mock "github.com/stretchr/testify/mock" + + models "github.com/grafana/grafana/pkg/services/ssosettings/models" ) // MockReloadable is an autogenerated mock type for the Reloadable type @@ -32,17 +34,17 @@ func (_m *MockReloadable) Reload(ctx context.Context, settings models.SSOSetting return r0 } -// Validate provides a mock function with given fields: ctx, settings -func (_m *MockReloadable) Validate(ctx context.Context, settings models.SSOSettings) error { - ret := _m.Called(ctx, settings) +// Validate provides a mock function with given fields: ctx, settings, requester +func (_m *MockReloadable) Validate(ctx context.Context, settings models.SSOSettings, requester identity.Requester) error { + ret := _m.Called(ctx, settings, requester) if len(ret) == 0 { panic("no return value specified for Validate") } var r0 error - if rf, ok := ret.Get(0).(func(context.Context, models.SSOSettings) error); ok { - r0 = rf(ctx, settings) + if rf, ok := ret.Get(0).(func(context.Context, models.SSOSettings, identity.Requester) error); ok { + r0 = rf(ctx, settings, requester) } else { r0 = ret.Error(0) } diff --git a/pkg/services/ssosettings/ssosettingstests/service_mock.go b/pkg/services/ssosettings/ssosettingstests/service_mock.go index 42ef1091fb9..02837917856 100644 --- a/pkg/services/ssosettings/ssosettingstests/service_mock.go +++ b/pkg/services/ssosettings/ssosettingstests/service_mock.go @@ -5,9 +5,11 @@ package ssosettingstests import ( context "context" - models "github.com/grafana/grafana/pkg/services/ssosettings/models" + identity "github.com/grafana/grafana/pkg/services/auth/identity" mock "github.com/stretchr/testify/mock" + models "github.com/grafana/grafana/pkg/services/ssosettings/models" + ssosettings "github.com/grafana/grafana/pkg/services/ssosettings" ) @@ -182,17 +184,17 @@ func (_m *MockService) Reload(ctx context.Context, provider string) { _m.Called(ctx, provider) } -// Upsert provides a mock function with given fields: ctx, settings -func (_m *MockService) Upsert(ctx context.Context, settings *models.SSOSettings) error { - ret := _m.Called(ctx, settings) +// Upsert provides a mock function with given fields: ctx, settings, requester +func (_m *MockService) Upsert(ctx context.Context, settings *models.SSOSettings, requester identity.Requester) error { + ret := _m.Called(ctx, settings, requester) if len(ret) == 0 { panic("no return value specified for Upsert") } var r0 error - if rf, ok := ret.Get(0).(func(context.Context, *models.SSOSettings) error); ok { - r0 = rf(ctx, settings) + if rf, ok := ret.Get(0).(func(context.Context, *models.SSOSettings, identity.Requester) error); ok { + r0 = rf(ctx, settings, requester) } else { r0 = ret.Error(0) } diff --git a/public/app/features/auth-config/FieldRenderer.tsx b/public/app/features/auth-config/FieldRenderer.tsx index 4c74ff5c0c8..856ac56b252 100644 --- a/public/app/features/auth-config/FieldRenderer.tsx +++ b/public/app/features/auth-config/FieldRenderer.tsx @@ -47,6 +47,10 @@ export const FieldRenderer = ({ return null; } + if (!!fieldData.hidden) { + return null; + } + // Dependant field means the field depends on another field's value and shouldn't be rendered if the parent field is false if (isDependantField) { const parentValue = watch(field.dependsOn); @@ -61,7 +65,7 @@ export const FieldRenderer = ({ error: fieldData.validation?.message, key: name, description: fieldData.description, - defaultValue: fieldData.defaultValue, + defaultValue: fieldData.defaultValue?.value, }; switch (fieldData.type) { diff --git a/public/app/features/auth-config/fields.tsx b/public/app/features/auth-config/fields.tsx index 813523ffd6b..1d174f59ed0 100644 --- a/public/app/features/auth-config/fields.tsx +++ b/public/app/features/auth-config/fields.tsx @@ -2,6 +2,7 @@ import React from 'react'; import { validate as uuidValidate } from 'uuid'; import { TextLink } from '@grafana/ui'; +import { contextSrv } from 'app/core/core'; import { FieldData, SSOProvider, SSOSettingsField } from './types'; import { isSelectableValue } from './utils/guards'; @@ -151,7 +152,7 @@ export function fieldMap(provider: string): Record { { value: 'InParams', label: 'InParams' }, { value: 'InHeader', label: 'InHeader' }, ], - defaultValue: 'AutoDetect', + defaultValue: { value: 'AutoDetect', label: 'AutoDetect' }, }, tokenUrl: { label: 'Token URL', @@ -280,6 +281,7 @@ export function fieldMap(provider: string): Record { label: 'Allow assign Grafana admin', description: 'If enabled, it will automatically sync the Grafana server administrator role.', type: 'switch', + hidden: !contextSrv.isGrafanaAdmin, }, skipOrgRoleSync: { label: 'Skip organization role sync', diff --git a/public/app/features/auth-config/types.ts b/public/app/features/auth-config/types.ts index eb385fa7183..015101b2882 100644 --- a/public/app/features/auth-config/types.ts +++ b/public/app/features/auth-config/types.ts @@ -113,7 +113,8 @@ export type FieldData = { allowCustomValue?: boolean; options?: Array>; placeholder?: string; - defaultValue?: string; + defaultValue?: SelectableValue; + hidden?: boolean; }; export type SSOSettingsField =