Skip to content

Commit

Permalink
OAuth: Make sub claim required for generic oauth behind feature toggle (
Browse files Browse the repository at this point in the history 
#85065)

* Add feature toggle for sub claims requirement

* OAuth: require valid auth id

* Fix feature toggle description
  • Loading branch information
@kalleep
kalleep committed Mar 25, 2024
1 parent e2f155f commit 2f3a01f
Show file tree
Hide file tree
Showing 8 changed files with 103 additions and 10 deletions.
1 change: 1 addition & 0 deletions packages/grafana-data/src/types/featureToggles.gen.ts
View file
Expand Up @@ -176,4 +176,5 @@ export interface FeatureToggles {
scopeFilters?: boolean;
ssoSettingsSAML?: boolean;
usePrometheusFrontendPackage?: boolean;
oauthRequireSubClaim?: boolean;
}
2 changes: 1 addition & 1 deletion pkg/services/authn/authnimpl/service.go
View file
Expand Up @@ -142,7 +142,7 @@ func ProvideService(

for name := range socialService.GetOAuthProviders() {
clientName := authn.ClientWithPrefix(name)
s.RegisterClient(clients.ProvideOAuth(clientName, cfg, oauthTokenService, socialService, settingsProviderService))
s.RegisterClient(clients.ProvideOAuth(clientName, cfg, oauthTokenService, socialService, settingsProviderService, features))
}

// FIXME (jguer): move to User package
Expand Down
17 changes: 11 additions & 6 deletions pkg/services/authn/clients/oauth.go
View file
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/grafana/grafana/pkg/login/social/connectors"
"github.com/grafana/grafana/pkg/services/auth/identity"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/oauthtoken"
"github.com/grafana/grafana/pkg/services/org"
Expand Down Expand Up @@ -65,12 +66,12 @@ var _ authn.RedirectClient = new(OAuth)

func ProvideOAuth(
name string, cfg *setting.Cfg, oauthService oauthtoken.OAuthTokenService,
socialService social.Service, settingsProviderService setting.Provider,
socialService social.Service, settingsProviderService setting.Provider, features featuremgmt.FeatureToggles,
) *OAuth {
providerName := strings.TrimPrefix(name, "auth.client.")
return &OAuth{
name, fmt.Sprintf("oauth_%s", providerName), providerName,
log.New(name), cfg, settingsProviderService, oauthService, socialService,
log.New(name), cfg, settingsProviderService, oauthService, socialService, features,
}
}

Expand All @@ -84,6 +85,7 @@ type OAuth struct {
settingsProviderSvc setting.Provider
oauthService oauthtoken.OAuthTokenService
socialService social.Service
features featuremgmt.FeatureToggles
}

func (c *OAuth) Name() string {
Expand Down Expand Up @@ -148,10 +150,13 @@ func (c *OAuth) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden
return nil, errOAuthUserInfo.Errorf("failed to get user info: %w", err)
}

// Implement in Grafana 11
// if userInfo.Id == "" {
// return nil, errors.New("idP did not return a user id")
// }
if userInfo.Id == "" {
if c.features.IsEnabledGlobally(featuremgmt.FlagOauthRequireSubClaim) {
return nil, errOAuthUserInfo.Errorf("missing required sub claims")
} else {
c.log.FromContext(ctx).Warn("Missing sub claim, oauth authentication without a sub claim is deprecated and will be rejected in future versions.")
}
}

if userInfo.Email == "" {
return nil, errOAuthMissingRequiredEmail.Errorf("required attribute email was not provided")
Expand Down
63 changes: 60 additions & 3 deletions pkg/services/authn/clients/oauth_test.go
View file
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/grafana/grafana/pkg/login/social/socialtest"
"github.com/grafana/grafana/pkg/services/auth/identity"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/oauthtoken/oauthtokentest"
"github.com/grafana/grafana/pkg/services/org"
Expand All @@ -37,6 +38,8 @@ func TestOAuth_Authenticate(t *testing.T) {
addPKCECookie bool
pkceCookieValue string

features []any

isEmailAllowed bool
userInfo *social.BasicUserInfo

Expand Down Expand Up @@ -120,6 +123,24 @@ func TestOAuth_Authenticate(t *testing.T) {
isEmailAllowed: false,
expectedErr: errOAuthEmailNotAllowed,
},
{
desc: "should return error when no auth id is set and feature toggle is enabled",
req: &authn.Request{
HTTPRequest: &http.Request{
Header: map[string][]string{},
URL: mustParseURL("http://grafana.com/?state=some-state"),
},
},
features: []any{featuremgmt.FlagOauthRequireSubClaim},
oauthCfg: &social.OAuthInfo{UsePKCE: true, Enabled: true},
addStateCookie: true,
stateCookieValue: "some-state",
addPKCECookie: true,
pkceCookieValue: "some-pkce-value",
userInfo: &social.BasicUserInfo{Email: "some@email.com"},
isEmailAllowed: false,
expectedErr: errOAuthUserInfo,
},
{
desc: "should return identity for valid request",
req: &authn.Request{HTTPRequest: &http.Request{
Expand Down Expand Up @@ -197,6 +218,42 @@ func TestOAuth_Authenticate(t *testing.T) {
},
},
},
{
desc: "should return identity when feature toggle is enabled and auth id is set",
req: &authn.Request{
HTTPRequest: &http.Request{
Header: map[string][]string{},
URL: mustParseURL("http://grafana.com/?state=some-state"),
},
},
oauthCfg: &social.OAuthInfo{Enabled: true},
addStateCookie: true,
stateCookieValue: "some-state",
isEmailAllowed: true,
features: []any{featuremgmt.FlagOauthRequireSubClaim},
userInfo: &social.BasicUserInfo{
Id: "123",
Name: "name",
Email: "some@email.com",
Role: "Admin",
},
expectedIdentity: &authn.Identity{
Email: "some@email.com",
AuthenticatedBy: login.AzureADAuthModule,
AuthID: "123",
Name: "name",
OAuthToken: &oauth2.Token{},
OrgRoles: map[int64]org.RoleType{1: org.RoleAdmin},
ClientParams: authn.ClientParams{
SyncUser: true,
SyncTeams: true,
AllowSignUp: true,
FetchSyncedUser: true,
SyncOrgRoles: true,
LookUpParams: login.UserLookupParams{},
},
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -231,7 +288,7 @@ func TestOAuth_Authenticate(t *testing.T) {
},
}

c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, settingsProvider)
c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, settingsProvider, featuremgmt.WithFeatures(tt.features...))

identity, err := c.Authenticate(context.Background(), tt.req)
assert.ErrorIs(t, err, tt.expectedErr)
Expand Down Expand Up @@ -314,7 +371,7 @@ func TestOAuth_RedirectURL(t *testing.T) {

cfg := setting.NewCfg()

c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, &setting.OSSImpl{Cfg: cfg})
c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, &setting.OSSImpl{Cfg: cfg}, featuremgmt.WithFeatures())

redirect, err := c.RedirectURL(context.Background(), nil)
assert.ErrorIs(t, err, tt.expectedErr)
Expand Down Expand Up @@ -427,7 +484,7 @@ func TestOAuth_Logout(t *testing.T) {
fakeSocialSvc := &socialtest.FakeSocialService{
ExpectedAuthInfoProvider: tt.oauthCfg,
}
c := ProvideOAuth(authn.ClientWithPrefix("azuread"), tt.cfg, mockService, fakeSocialSvc, &setting.OSSImpl{Cfg: tt.cfg})
c := ProvideOAuth(authn.ClientWithPrefix("azuread"), tt.cfg, mockService, fakeSocialSvc, &setting.OSSImpl{Cfg: tt.cfg}, featuremgmt.WithFeatures())

redirect, ok := c.Logout(context.Background(), &authn.Identity{}, &login.UserAuth{})

Expand Down
8 changes: 8 additions & 0 deletions pkg/services/featuremgmt/registry.go
View file
Expand Up @@ -1181,6 +1181,14 @@ var (
FrontendOnly: true,
Owner: grafanaObservabilityMetricsSquad,
},
{
Name: "oauthRequireSubClaim",
Description: "Require that sub claims is present in oauth tokens.",
Stage: FeatureStageExperimental,
Owner: identityAccessTeam,
HideFromDocs: true,
HideFromAdminPage: true,
},
}
)

Expand Down
1 change: 1 addition & 0 deletions pkg/services/featuremgmt/toggles_gen.csv
View file
Expand Up @@ -157,3 +157,4 @@ betterPageScrolling,GA,@grafana/grafana-frontend-platform,false,false,true
scopeFilters,experimental,@grafana/dashboards-squad,false,false,false
ssoSettingsSAML,experimental,@grafana/identity-access-team,false,false,false
usePrometheusFrontendPackage,experimental,@grafana/observability-metrics,false,false,true
oauthRequireSubClaim,experimental,@grafana/identity-access-team,false,false,false
4 changes: 4 additions & 0 deletions pkg/services/featuremgmt/toggles_gen.go
View file
Expand Up @@ -638,4 +638,8 @@ const (
// FlagUsePrometheusFrontendPackage
// Use the @grafana/prometheus frontend package in core Prometheus.
FlagUsePrometheusFrontendPackage = "usePrometheusFrontendPackage"

// FlagOauthRequireSubClaim
// Require that sub claims is present in oauth tokens.
FlagOauthRequireSubClaim = "oauthRequireSubClaim"
)
17 changes: 17 additions & 0 deletions pkg/services/featuremgmt/toggles_gen.json
View file
Expand Up @@ -2046,6 +2046,23 @@
"stage": "experimental",
"codeowner": "@grafana/alerting-squad"
}
},
{
"metadata": {
"name": "oauthRequireSubClaim",
"resourceVersion": "1711371458317",
"creationTimestamp": "2024-03-25T10:50:29Z",
"annotations": {
"grafana.app/updatedTimestamp": "2024-03-25 12:57:38.317427693 +0000 UTC"
}
},
"spec": {
"description": "Require that sub claims is present in oauth tokens.",
"stage": "experimental",
"codeowner": "@grafana/identity-access-team",
"hideFromAdminPage": true,
"hideFromDocs": true
}
}
]
}

0 comments on commit 2f3a01f

Please sign in to comment.