public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Auth: Add validation to Generic OAuth API and UI (#81345)

Browse Source 
* wip

* Update validation

* Chore: Remove InputControl usage

* Fixes, validation

* Remove empty option

* Validation changes

* Add tests, rename

* lint

---------

Co-authored-by: Clarity-89 <homes89@ukr.net>
This commit is contained in:
Misi
2024-01-29 12:04:22 +01:00
committed by GitHub
co-authored by Clarity-89
parent 7e96a2be56
commit bcc2409564
16 changed files with 529 additions and 368 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/login/social/connectors/azuread_oauth.go
+7 -1
View File
@@ -12,6 +12,7 @@ import (
jose "github.com/go-jose/go-jose/v3"
"github.com/go-jose/go-jose/v3/jwt"
"github.com/google/uuid"
"golang.org/x/oauth2"
"github.com/grafana/grafana/pkg/infra/remotecache"
@@ -195,7 +196,12 @@ func (s *SocialAzureAD) Validate(ctx context.Context, settings ssoModels.SSOSett
return err
}
// add specific validation rules for AzureAD
for _, groupId := range info.AllowedGroups {
_, err := uuid.Parse(groupId)
if err != nil {
return ssosettings.ErrInvalidOAuthConfig("One or more of the Allowed groups are not in the correct format. Allowed groups should be a list of Object Ids.")
}
}
return nil
}
pkg/login/social/connectors/azuread_oauth_test.go
+34 -12
View File
@@ -19,6 +19,7 @@ import (
"github.com/grafana/grafana/pkg/infra/remotecache"
"github.com/grafana/grafana/pkg/login/social"
"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/setting"
@@ -991,18 +992,18 @@ func TestSocialAzureAD_InitializeExtraFields(t *testing.T) {
func TestSocialAzureAD_Validate(t *testing.T) {
testCases := []struct {
name string
settings ssoModels.SSOSettings
expectError bool
name string
settings ssoModels.SSOSettings
wantErr error
}{
{
name: "SSOSettings is valid",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"client_id": "client-id",
"allowed_groups": "0bb9c9cc-4945-418f-9b6a-c1d3b81141b0, 6034d328-0e6a-4240-8d03-cb9f2c1f16e4",
},
},
expectError: false,
},
{
name: "fails if settings map contains an invalid field",
@@ -1012,7 +1013,7 @@ func TestSocialAzureAD_Validate(t *testing.T) {
"invalid_field": []int{1, 2, 3},
},
},
expectError: true,
wantErr: ssosettings.ErrInvalidSettings,
},
{
name: "fails if client id is empty",
@@ -1021,14 +1022,35 @@ func TestSocialAzureAD_Validate(t *testing.T) {
"client_id": "",
},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if client id does not exist",
settings: ssoModels.SSOSettings{
Settings: map[string]any{},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if allowed groups are not uuids",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"allowed_groups": "abc, def",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if both allow assign grafana admin and skip org role sync are enabled",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"allow_assign_grafana_admin": "true",
"skip_org_role_sync": "true",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
}
@@ -1037,11 +1059,11 @@ func TestSocialAzureAD_Validate(t *testing.T) {
s := NewAzureADProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures(), nil)
err := s.Validate(context.Background(), tc.settings)
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
if tc.wantErr != nil {
require.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, err)
})
}
}
pkg/login/social/connectors/generic_oauth.go
+7 -1
View File
@@ -78,7 +78,13 @@ func (s *SocialGenericOAuth) Validate(ctx context.Context, settings ssoModels.SS
return err
}
// add specific validation rules for Generic OAuth
if info.Extra[teamIdsKey] != "" && (info.TeamIdsAttributePath == "" || info.TeamsUrl == "") {
return ssosettings.ErrInvalidOAuthConfig("If Team Ids are configured then Team Ids attribute path and Teams URL must be configured.")
}
if info.AllowedGroups != nil && len(info.AllowedGroups) > 0 && info.GroupsAttributePath == "" {
return ssosettings.ErrInvalidOAuthConfig("If Allowed groups are configured then Groups attribute path must be configured.")
}
return nil
}
pkg/login/social/connectors/generic_oauth_test.go
+22 -11
View File
@@ -15,6 +15,7 @@ import (
"github.com/grafana/grafana/pkg/login/social"
"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/setting"
@@ -919,9 +920,9 @@ func TestSocialGenericOAuth_InitializeExtraFields(t *testing.T) {
func TestSocialGenericOAuth_Validate(t *testing.T) {
testCases := []struct {
name string
settings ssoModels.SSOSettings
expectError bool
name string
settings ssoModels.SSOSettings
wantErr error
}{
{
name: "SSOSettings is valid",
@@ -930,7 +931,6 @@ func TestSocialGenericOAuth_Validate(t *testing.T) {
"client_id": "client-id",
},
},
expectError: false,
},
{
name: "fails if settings map contains an invalid field",
@@ -940,7 +940,7 @@ func TestSocialGenericOAuth_Validate(t *testing.T) {
"invalid_field": []int{1, 2, 3},
},
},
expectError: true,
wantErr: ssosettings.ErrInvalidSettings,
},
{
name: "fails if client id is empty",
@@ -949,14 +949,25 @@ func TestSocialGenericOAuth_Validate(t *testing.T) {
"client_id": "",
},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if client id does not exist",
settings: ssoModels.SSOSettings{
Settings: map[string]any{},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if both allow assign grafana admin and skip org role sync are enabled",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"allow_assign_grafana_admin": "true",
"skip_org_role_sync": "true",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
}
@@ -965,11 +976,11 @@ func TestSocialGenericOAuth_Validate(t *testing.T) {
s := NewGenericOAuthProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures())
err := s.Validate(context.Background(), tc.settings)
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
if tc.wantErr != nil {
require.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, err)
})
}
}
pkg/login/social/connectors/github_oauth.go
+1 -2
View File
@@ -89,8 +89,7 @@ func (s *SocialGithub) Validate(ctx context.Context, settings ssoModels.SSOSetti
teamIds := mustInts(teamIdsSplitted)
if len(teamIdsSplitted) != len(teamIds) {
s.log.Warn("Failed to parse team ids. Team ids must be a list of numbers.", "teamIds", teamIdsSplitted)
return ssosettings.ErrInvalidSettings.Errorf("Failed to parse team ids. Team ids must be a list of numbers.")
return ssosettings.ErrInvalidOAuthConfig("Failed to parse Team Ids. Team Ids must be a list of numbers.")
}
return nil
pkg/login/social/connectors/github_oauth_test.go
+32 -11
View File
@@ -13,6 +13,7 @@ import (
"github.com/grafana/grafana/pkg/login/social"
"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/setting"
@@ -345,9 +346,9 @@ func TestSocialGitHub_InitializeExtraFields(t *testing.T) {
func TestSocialGitHub_Validate(t *testing.T) {
testCases := []struct {
name string
settings ssoModels.SSOSettings
expectError bool
name string
settings ssoModels.SSOSettings
wantErr error
}{
{
name: "SSOSettings is valid",
@@ -356,7 +357,6 @@ func TestSocialGitHub_Validate(t *testing.T) {
"client_id": "client-id",
},
},
expectError: false,
},
{
name: "fails if settings map contains an invalid field",
@@ -366,7 +366,7 @@ func TestSocialGitHub_Validate(t *testing.T) {
"invalid_field": []int{1, 2, 3},
},
},
expectError: true,
wantErr: ssosettings.ErrInvalidSettings,
},
{
name: "fails if client id is empty",
@@ -375,14 +375,35 @@ func TestSocialGitHub_Validate(t *testing.T) {
"client_id": "",
},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if client id does not exist",
settings: ssoModels.SSOSettings{
Settings: map[string]any{},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if team ids are not integers",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"team_ids": "abc1234,5678,def",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if both allow assign grafana admin and skip org role sync are enabled",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"allow_assign_grafana_admin": "true",
"skip_org_role_sync": "true",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
}
@@ -391,11 +412,11 @@ func TestSocialGitHub_Validate(t *testing.T) {
s := NewGitHubProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures())
err := s.Validate(context.Background(), tc.settings)
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
if tc.wantErr != nil {
require.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, err)
})
}
}
pkg/login/social/connectors/gitlab_oauth_test.go
+22 -11
View File
@@ -18,6 +18,7 @@ import (
"github.com/grafana/grafana/pkg/login/social"
"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/setting"
@@ -463,9 +464,9 @@ func TestSocialGitlab_GetGroupsNextPage(t *testing.T) {
func TestSocialGitlab_Validate(t *testing.T) {
testCases := []struct {
name string
settings ssoModels.SSOSettings
expectError bool
name string
settings ssoModels.SSOSettings
wantErr error
}{
{
name: "SSOSettings is valid",
@@ -474,7 +475,6 @@ func TestSocialGitlab_Validate(t *testing.T) {
"client_id": "client-id",
},
},
expectError: false,
},
{
name: "fails if settings map contains an invalid field",
@@ -484,7 +484,7 @@ func TestSocialGitlab_Validate(t *testing.T) {
"invalid_field": []int{1, 2, 3},
},
},
expectError: true,
wantErr: ssosettings.ErrInvalidSettings,
},
{
name: "fails if client id is empty",
@@ -493,14 +493,25 @@ func TestSocialGitlab_Validate(t *testing.T) {
"client_id": "",
},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if client id does not exist",
settings: ssoModels.SSOSettings{
Settings: map[string]any{},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if both allow assign grafana admin and skip org role sync are enabled",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"allow_assign_grafana_admin": "true",
"skip_org_role_sync": "true",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
}
@@ -509,11 +520,11 @@ func TestSocialGitlab_Validate(t *testing.T) {
s := NewGitLabProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures())
err := s.Validate(context.Background(), tc.settings)
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
if tc.wantErr != nil {
require.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, err)
})
}
}
pkg/login/social/connectors/google_oauth_test.go
+22 -11
View File
@@ -17,6 +17,7 @@ import (
"github.com/grafana/grafana/pkg/login/social"
"github.com/grafana/grafana/pkg/models/roletype"
"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/setting"
@@ -668,9 +669,9 @@ func TestSocialGoogle_UserInfo(t *testing.T) {
func TestSocialGoogle_Validate(t *testing.T) {
testCases := []struct {
name string
settings ssoModels.SSOSettings
expectError bool
name string
settings ssoModels.SSOSettings
wantErr error
}{
{
name: "SSOSettings is valid",
@@ -679,7 +680,6 @@ func TestSocialGoogle_Validate(t *testing.T) {
"client_id": "client-id",
},
},
expectError: false,
},
{
name: "fails if settings map contains an invalid field",
@@ -689,7 +689,7 @@ func TestSocialGoogle_Validate(t *testing.T) {
"invalid_field": []int{1, 2, 3},
},
},
expectError: true,
wantErr: ssosettings.ErrInvalidSettings,
},
{
name: "fails if client id is empty",
@@ -698,14 +698,25 @@ func TestSocialGoogle_Validate(t *testing.T) {
"client_id": "",
},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if client id does not exist",
settings: ssoModels.SSOSettings{
Settings: map[string]any{},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if both allow assign grafana admin and skip org role sync are enabled",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"allow_assign_grafana_admin": "true",
"skip_org_role_sync": "true",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
}
@@ -714,11 +725,11 @@ func TestSocialGoogle_Validate(t *testing.T) {
s := NewGoogleProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures())
err := s.Validate(context.Background(), tc.settings)
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
if tc.wantErr != nil {
require.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, err)
})
}
}
pkg/login/social/connectors/okta_oauth_test.go
+22 -11
View File
@@ -15,6 +15,7 @@ import (
"github.com/grafana/grafana/pkg/login/social"
"github.com/grafana/grafana/pkg/models/roletype"
"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/setting"
@@ -136,9 +137,9 @@ func TestSocialOkta_UserInfo(t *testing.T) {
func TestSocialOkta_Validate(t *testing.T) {
testCases := []struct {
name string
settings ssoModels.SSOSettings
expectError bool
name string
settings ssoModels.SSOSettings
wantErr error
}{
{
name: "SSOSettings is valid",
@@ -147,7 +148,6 @@ func TestSocialOkta_Validate(t *testing.T) {
"client_id": "client-id",
},
},
expectError: false,
},
{
name: "fails if settings map contains an invalid field",
@@ -157,7 +157,7 @@ func TestSocialOkta_Validate(t *testing.T) {
"invalid_field": []int{1, 2, 3},
},
},
expectError: true,
wantErr: ssosettings.ErrInvalidSettings,
},
{
name: "fails if client id is empty",
@@ -166,14 +166,25 @@ func TestSocialOkta_Validate(t *testing.T) {
"client_id": "",
},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if client id does not exist",
settings: ssoModels.SSOSettings{
Settings: map[string]any{},
},
expectError: true,
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
{
name: "fails if both allow assign grafana admin and skip org role sync are enabled",
settings: ssoModels.SSOSettings{
Settings: map[string]any{
"client_id": "client-id",
"allow_assign_grafana_admin": "true",
"skip_org_role_sync": "true",
},
},
wantErr: ssosettings.ErrBaseInvalidOAuthConfig,
},
}
@@ -182,11 +193,11 @@ func TestSocialOkta_Validate(t *testing.T) {
s := NewOktaProvider(&social.OAuthInfo{}, &setting.Cfg{}, &ssosettingstests.MockService{}, featuremgmt.WithFeatures())
err := s.Validate(context.Background(), tc.settings)
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
if tc.wantErr != nil {
require.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, err)
})
}
}
pkg/login/social/connectors/social_base.go
+5 -1
View File
@@ -222,7 +222,11 @@ func getRoleFromSearch(role string) (org.RoleType, bool) {
func validateInfo(info *social.OAuthInfo) error {
if info.ClientId == "" {
return ssosettings.ErrEmptyClientId.Errorf("clientId is empty")
return ssosettings.ErrInvalidOAuthConfig("ClientId is empty")
}
if info.AllowAssignGrafanaAdmin && info.SkipOrgRoleSync {
return ssosettings.ErrInvalidOAuthConfig("Allow assign Grafana Admin and Skip org role sync are both set thus Grafana Admin role will not be synced. Consider setting one or the other.")
}
return nil
pkg/services/ssosettings/errors.go
+8
View File
@@ -10,6 +10,14 @@ var (
ErrNotConfigurable = errNotFoundBase.Errorf("not configurable")
ErrBaseInvalidOAuthConfig = errutil.ValidationFailed("sso.invalidOauthConfig")
ErrInvalidOAuthConfig = func(msg string) error {
base := ErrBaseInvalidOAuthConfig.Errorf("OAuth settings are invalid")
base.PublicMessage = msg
return base
}
ErrInvalidProvider = errutil.ValidationFailed("sso.invalidProvider", errutil.WithPublicMessage("Provider is invalid"))
ErrInvalidSettings = errutil.ValidationFailed("sso.settings", errutil.WithPublicMessage("Settings field is invalid"))
ErrEmptyClientId = errutil.ValidationFailed("sso.emptyClientId", errutil.WithPublicMessage("ClientId cannot be empty"))