From 4d0c2e16f4b89a65a9ca1f7ee543a56d3a905a95 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 15 Jun 2022 08:00:17 -0700 Subject: [PATCH 1/6] require groups scope to get groups back from supervisor Signed-off-by: Margo Crawford --- internal/oidc/auth/auth_handler.go | 6 +- internal/oidc/auth/auth_handler_test.go | 10 +-- internal/oidc/callback/callback_handler.go | 5 +- .../oidc/callback/callback_handler_test.go | 75 +++++++++++++++++-- .../oidc/clientregistry/clientregistry.go | 3 +- .../clientregistry/clientregistry_test.go | 7 +- .../downstreamsession/downstream_session.go | 17 +++-- internal/oidc/login/post_login_handler.go | 8 +- .../oidc/login/post_login_handler_test.go | 33 +++++++- internal/oidc/oidc.go | 8 ++ .../testutil/oidctestutil/oidctestutil.go | 16 +++- test/integration/e2e_test.go | 28 +++++-- test/integration/supervisor_login_test.go | 4 +- 13 files changed, 172 insertions(+), 48 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 698ea7f3..67b1581b 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -146,7 +146,7 @@ func handleAuthRequestForLDAPUpstreamCLIFlow( username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() customSessionData := downstreamsession.MakeDownstreamLDAPOrADCustomSessionData(ldapUpstream, idpType, authenticateResponse) - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, true) return nil @@ -243,7 +243,7 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( return nil } - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, true) @@ -334,7 +334,7 @@ func newAuthorizeRequest(r *http.Request, w http.ResponseWriter, oauthHelper fos // Grant the openid scope (for now) if they asked for it so that `NewAuthorizeResponse` will perform its OIDC validations. // There don't seem to be any validations inside `NewAuthorizeResponse` related to the offline_access scope // at this time, however we will temporarily grant the scope just in case that changes in a future release of fosite. - downstreamsession.GrantScopesIfRequested(authorizeRequester) + downstreamsession.GrantScopesIfRequested(authorizeRequester, []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, oidc.RequestAudienceScope, oidc.DownstreamGroupsScope}) return authorizeRequester, true } diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 11431a0b..8847d8c4 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -375,8 +375,8 @@ func TestAuthorizationEndpoint(t *testing.T) { return urlToReturn } - happyDownstreamScopesRequested := []string{"openid", "profile", "email"} - happyDownstreamScopesGranted := []string{"openid"} + happyDownstreamScopesRequested := []string{"openid", "profile", "email", "groups"} + happyDownstreamScopesGranted := []string{"openid", "groups"} happyGetRequestQueryMap := map[string]string{ "response_type": "code", @@ -495,7 +495,7 @@ func TestAuthorizationEndpoint(t *testing.T) { } // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it - happyAuthcodeDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid&state=` + happyState + happyAuthcodeDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+groups&state=` + happyState incomingCookieCSRFValue := "csrf-value-from-cookie" encodedIncomingCookieCSRFValue, err := happyCookieEncoder.Encode("csrf", incomingCookieCSRFValue) @@ -957,7 +957,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation, wantStatus: http.StatusFound, wantContentType: htmlContentType, - wantRedirectLocationRegexp: downstreamRedirectURIWithDifferentPort + `\?code=([^&]+)&scope=openid&state=` + happyState, + wantRedirectLocationRegexp: downstreamRedirectURIWithDifferentPort + `\?code=([^&]+)&scope=openid\+groups&state=` + happyState, wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, wantDownstreamIDTokenUsername: oidcUpstreamUsername, wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, @@ -980,7 +980,7 @@ func TestAuthorizationEndpoint(t *testing.T) { customPasswordHeader: pointer.StringPtr(happyLDAPPassword), wantStatus: http.StatusFound, wantContentType: htmlContentType, - wantRedirectLocationRegexp: downstreamRedirectURIWithDifferentPort + `\?code=([^&]+)&scope=openid&state=` + happyState, + wantRedirectLocationRegexp: downstreamRedirectURIWithDifferentPort + `\?code=([^&]+)&scope=openid\+groups&state=` + happyState, wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, wantDownstreamIDTokenGroups: happyLDAPGroups, diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index bcb8bf1b..683de017 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -8,6 +8,7 @@ import ( "net/http" "net/url" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/ory/fosite" "go.pinniped.dev/internal/httputil/httperr" @@ -52,7 +53,7 @@ func NewHandler( } // Automatically grant the openid, offline_access, and pinniped:request-audience scopes, but only if they were requested. - downstreamsession.GrantScopesIfRequested(authorizeRequester) + downstreamsession.GrantScopesIfRequested(authorizeRequester, []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, oidc.RequestAudienceScope, oidc.DownstreamGroupsScope}) token, err := upstreamIDPConfig.ExchangeAuthcodeAndValidateTokens( r.Context(), @@ -76,7 +77,7 @@ func NewHandler( return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index d8f08822..8230e4c8 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -62,8 +62,8 @@ const ( var ( oidcUpstreamGroupMembership = []string{"test-pinniped-group-0", "test-pinniped-group-1"} - happyDownstreamScopesRequested = []string{"openid"} - happyDownstreamScopesGranted = []string{"openid"} + happyDownstreamScopesRequested = []string{"openid", "groups"} + happyDownstreamScopesGranted = []string{"openid", "groups"} happyDownstreamRequestParamsQuery = url.Values{ "response_type": []string{"code"}, @@ -133,7 +133,7 @@ func TestCallbackEndpoint(t *testing.T) { } // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it - happyDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid&state=` + happyDownstreamState + happyDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+groups&state=` + happyDownstreamState tests := []struct { name string @@ -236,6 +236,38 @@ func TestCallbackEndpoint(t *testing.T) { args: happyExchangeAndValidateTokensArgs, }, }, + { + name: "form_post happy path with no groups scope requested", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), + method: http.MethodGet, + path: newRequestPath().WithState( + happyUpstreamStateParam().WithAuthorizeRequestParams( + shallowCopyAndModifyQuery( + happyDownstreamRequestParamsQuery, + map[string]string{ + "response_mode": "form_post", + "scope": "openid", + }, + ).Encode(), + ).Build(t, happyStateCodec), + ).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusOK, + wantContentType: "text/html;charset=UTF-8", + wantBodyFormResponseRegexp: `(.+)`, + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamRequestedScopes: []string{"openid"}, + wantDownstreamGrantedScopes: []string{"openid"}, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, { name: "GET with authcode exchange that returns an access token but no refresh token but has a short token lifetime which is stored as a warning in the session", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken, metav1.NewTime(time.Now().Add(1*time.Hour))).WithUserInfoURL().Build()), @@ -683,6 +715,33 @@ func TestCallbackEndpoint(t *testing.T) { name: "state's downstream auth params does not contain openid scope", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), method: http.MethodGet, + path: newRequestPath(). + WithState( + happyUpstreamStateParam(). + WithAuthorizeRequestParams(shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, map[string]string{"scope": "profile email groups"}).Encode()). + Build(t, happyStateCodec), + ).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusSeeOther, + wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=groups&state=` + happyDownstreamState, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamRequestedScopes: []string{"profile", "email", "groups"}, + wantDownstreamGrantedScopes: []string{"groups"}, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, + { + name: "state's downstream auth params does not contain openid or groups scope", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), + method: http.MethodGet, path: newRequestPath(). WithState( happyUpstreamStateParam(). @@ -695,7 +754,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenUsername: oidcUpstreamUsername, wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, wantDownstreamRequestedScopes: []string{"profile", "email"}, - wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamGrantedScopes: []string{}, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, @@ -712,16 +771,16 @@ func TestCallbackEndpoint(t *testing.T) { path: newRequestPath(). WithState( happyUpstreamStateParam(). - WithAuthorizeRequestParams(shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, map[string]string{"scope": "openid offline_access"}).Encode()). + WithAuthorizeRequestParams(shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, map[string]string{"scope": "openid offline_access groups"}).Encode()). Build(t, happyStateCodec), ).String(), csrfCookie: happyCSRFCookie, wantStatus: http.StatusSeeOther, - wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+offline_access&state=` + happyDownstreamState, + wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+offline_access\+groups&state=` + happyDownstreamState, wantDownstreamIDTokenUsername: oidcUpstreamUsername, wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, - wantDownstreamRequestedScopes: []string{"openid", "offline_access"}, - wantDownstreamGrantedScopes: []string{"openid", "offline_access"}, + wantDownstreamRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantDownstreamGrantedScopes: []string{"openid", "offline_access", "groups"}, wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, wantDownstreamNonce: downstreamNonce, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, diff --git a/internal/oidc/clientregistry/clientregistry.go b/internal/oidc/clientregistry/clientregistry.go index c01caa7d..123a3d3a 100644 --- a/internal/oidc/clientregistry/clientregistry.go +++ b/internal/oidc/clientregistry/clientregistry.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package clientregistry defines Pinniped's OAuth2/OIDC clients. @@ -85,6 +85,7 @@ func PinnipedCLI() *Client { "profile", "email", "pinniped:request-audience", + "groups", }, Audience: nil, Public: true, diff --git a/internal/oidc/clientregistry/clientregistry_test.go b/internal/oidc/clientregistry/clientregistry_test.go index 5062f629..ac70aa65 100644 --- a/internal/oidc/clientregistry/clientregistry_test.go +++ b/internal/oidc/clientregistry/clientregistry_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package clientregistry @@ -50,7 +50,7 @@ func TestPinnipedCLI(t *testing.T) { require.Equal(t, []string{"http://127.0.0.1/callback"}, c.GetRedirectURIs()) require.Equal(t, fosite.Arguments{"authorization_code", "refresh_token", "urn:ietf:params:oauth:grant-type:token-exchange"}, c.GetGrantTypes()) require.Equal(t, fosite.Arguments{"code"}, c.GetResponseTypes()) - require.Equal(t, fosite.Arguments{oidc.ScopeOpenID, oidc.ScopeOfflineAccess, "profile", "email", "pinniped:request-audience"}, c.GetScopes()) + require.Equal(t, fosite.Arguments{oidc.ScopeOpenID, oidc.ScopeOfflineAccess, "profile", "email", "pinniped:request-audience", "groups"}, c.GetScopes()) require.True(t, c.IsPublic()) require.Nil(t, c.GetAudience()) require.Nil(t, c.GetRequestURIs()) @@ -82,7 +82,8 @@ func TestPinnipedCLI(t *testing.T) { "offline_access", "profile", "email", - "pinniped:request-audience" + "pinniped:request-audience", + "groups" ], "audience": null, "public": true, diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 2343c833..d5783e5e 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -10,7 +10,8 @@ import ( "net/url" "time" - coreosoidc "github.com/coreos/go-oidc/v3/oidc" + "k8s.io/utils/strings/slices" + "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" @@ -40,7 +41,7 @@ const ( ) // MakeDownstreamSession creates a downstream OIDC session. -func MakeDownstreamSession(subject string, username string, groups []string, custom *psession.CustomSessionData) *psession.PinnipedSession { +func MakeDownstreamSession(subject string, username string, groups []string, grantedScopes []string, custom *psession.CustomSessionData) *psession.PinnipedSession { now := time.Now().UTC() openIDSession := &psession.PinnipedSession{ Fosite: &openid.DefaultSession{ @@ -57,7 +58,9 @@ func MakeDownstreamSession(subject string, username string, groups []string, cus } openIDSession.IDTokenClaims().Extra = map[string]interface{}{ oidc.DownstreamUsernameClaim: username, - oidc.DownstreamGroupsClaim: groups, + } + if slices.Contains(grantedScopes, oidc.DownstreamGroupsScope) { + openIDSession.IDTokenClaims().Extra[oidc.DownstreamGroupsClaim] = groups } return openIDSession } @@ -147,10 +150,10 @@ func MakeDownstreamOIDCCustomSessionData(oidcUpstream provider.UpstreamOIDCIdent } // GrantScopesIfRequested auto-grants the scopes for which we do not require end-user approval, if they were requested. -func GrantScopesIfRequested(authorizeRequester fosite.AuthorizeRequester) { - oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) - oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) - oidc.GrantScopeIfRequested(authorizeRequester, "pinniped:request-audience") +func GrantScopesIfRequested(authorizeRequester fosite.AuthorizeRequester, scopes []string) { + for _, scope := range scopes { + oidc.GrantScopeIfRequested(authorizeRequester, scope) + } } // GetDownstreamIdentityFromUpstreamIDToken returns the mapped subject, username, and group names, in that order. diff --git a/internal/oidc/login/post_login_handler.go b/internal/oidc/login/post_login_handler.go index 5eb3a2e0..fdc480c0 100644 --- a/internal/oidc/login/post_login_handler.go +++ b/internal/oidc/login/post_login_handler.go @@ -7,6 +7,8 @@ import ( "net/http" "net/url" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" + "github.com/ory/fosite" "go.pinniped.dev/internal/httputil/httperr" @@ -44,8 +46,8 @@ func NewPostHandler(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvider return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } - // Automatically grant the openid, offline_access, and pinniped:request-audience scopes, but only if they were requested. - downstreamsession.GrantScopesIfRequested(authorizeRequester) + // Automatically grant the openid, offline_access, pinniped:request-audience and groups scopes, but only if they were requested. + downstreamsession.GrantScopesIfRequested(authorizeRequester, []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, oidc.RequestAudienceScope, oidc.DownstreamGroupsScope}) // Get the username and password form params from the POST body. username := r.PostFormValue(usernameParamName) @@ -80,7 +82,7 @@ func NewPostHandler(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvider username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() customSessionData := downstreamsession.MakeDownstreamLDAPOrADCustomSessionData(ldapUpstream, idpType, authenticateResponse) - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, false) return nil diff --git a/internal/oidc/login/post_login_handler_test.go b/internal/oidc/login/post_login_handler_test.go index 267c5e08..e6f44cc2 100644 --- a/internal/oidc/login/post_login_handler_test.go +++ b/internal/oidc/login/post_login_handler_test.go @@ -82,8 +82,8 @@ func TestPostLoginEndpoint(t *testing.T) { } ) - happyDownstreamScopesRequested := []string{"openid"} - happyDownstreamScopesGranted := []string{"openid"} + happyDownstreamScopesRequested := []string{"openid", "groups"} + happyDownstreamScopesGranted := []string{"openid", "groups"} happyDownstreamRequestParamsQuery := url.Values{ "response_type": []string{"code"}, @@ -211,7 +211,7 @@ func TestPostLoginEndpoint(t *testing.T) { } // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it - happyAuthcodeDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid&state=` + happyDownstreamState + happyAuthcodeDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+groups&state=` + happyDownstreamState happyUsernamePasswordFormParams := url.Values{userParam: []string{happyLDAPUsername}, passParam: []string{happyLDAPPassword}} @@ -348,7 +348,7 @@ func TestPostLoginEndpoint(t *testing.T) { wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantBodyString: "", - wantRedirectLocationRegexp: "http://127.0.0.1:4242/callback" + `\?code=([^&]+)&scope=openid&state=` + happyDownstreamState, + wantRedirectLocationRegexp: "http://127.0.0.1:4242/callback" + `\?code=([^&]+)&scope=openid\+groups&state=` + happyDownstreamState, wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, wantDownstreamIDTokenGroups: happyLDAPGroups, @@ -410,6 +410,31 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, }, + { + name: "happy LDAP login when groups scope is not requested", + idps: oidctestutil.NewUpstreamIDPListerBuilder(). + WithLDAP(&upstreamLDAPIdentityProvider). // should pick this one + WithActiveDirectory(&erroringUpstreamLDAPIdentityProvider), + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + query := copyOfHappyDownstreamRequestParamsQuery() + query["scope"] = []string{"openid"} + data.AuthParams = query.Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyString: "", + wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid&state=` + happyDownstreamState, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, + wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, + wantDownstreamRequestedScopes: []string{"openid"}, + wantDownstreamRedirectURI: downstreamRedirectURI, + wantDownstreamGrantedScopes: []string{"openid"}, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, + }, { name: "bad username LDAP login", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 79380df7..0b8df785 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -76,6 +76,14 @@ const ( // information. DownstreamGroupsClaim = "groups" + // DownstreamGroupsScope is a custom scope that determines whether the + // groups claim will be returned in ID tokens. + DownstreamGroupsScope = "groups" + + // RequestAudienceScope is a custom scope that determines whether a RFC8693 token + // exchange is allowed to request a different audience. + RequestAudienceScope = "pinniped:request-audience" + // CSRFCookieLifespan is the length of time that the CSRF cookie is valid. After this time, the // Supervisor's authorization endpoint should give the browser a new CSRF cookie. We set it to // a week so that it is unlikely to expire during a login. diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index c408ada9..f5598edc 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -14,6 +14,8 @@ import ( "testing" "time" + "k8s.io/utils/strings/slices" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/gorilla/securecookie" "github.com/ory/fosite" @@ -1063,10 +1065,16 @@ func validateAuthcodeStorage( // Check the user's identity, which are put into the downstream ID token's subject, username and groups claims. require.Equal(t, wantDownstreamIDTokenSubject, actualClaims.Subject) require.Equal(t, wantDownstreamIDTokenUsername, actualClaims.Extra["username"]) - require.Len(t, actualClaims.Extra, 2) - actualDownstreamIDTokenGroups := actualClaims.Extra["groups"] - require.NotNil(t, actualDownstreamIDTokenGroups) - require.ElementsMatch(t, wantDownstreamIDTokenGroups, actualDownstreamIDTokenGroups) + if slices.Contains(wantDownstreamGrantedScopes, "groups") { + require.Len(t, actualClaims.Extra, 2) + actualDownstreamIDTokenGroups := actualClaims.Extra["groups"] + require.NotNil(t, actualDownstreamIDTokenGroups) + require.ElementsMatch(t, wantDownstreamIDTokenGroups, actualDownstreamIDTokenGroups) + } else { + require.Len(t, actualClaims.Extra, 1) + actualDownstreamIDTokenGroups := actualClaims.Extra["groups"] + require.Nil(t, actualDownstreamIDTokenGroups) + } // Check the rest of the downstream ID token's claims. Fosite wants us to set these (in UTC time). testutil.RequireTimeInDelta(t, time.Now().UTC(), actualClaims.RequestedAt, timeComparisonFudgeFactor) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 9bafffde..b2305da8 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -170,6 +170,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--oidc-skip-browser", "--oidc-ca-bundle", testCABundlePath, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a browser login via the plugin. @@ -256,6 +257,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--oidc-skip-listen", "--oidc-ca-bundle", testCABundlePath, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a browser login via the plugin. @@ -381,6 +383,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--oidc-skip-listen", "--oidc-ca-bundle", testCABundlePath, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a browser login via the plugin. @@ -514,6 +517,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--upstream-identity-provider-flow", "cli_password", // create a kubeconfig configured to use the cli_password flow "--oidc-ca-bundle", testCABundlePath, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a browser-less CLI prompt login via the plugin. @@ -594,6 +598,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--upstream-identity-provider-flow", "cli_password", "--oidc-ca-bundle", testCABundlePath, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get --raw /healthz" which should trigger a browser-less CLI prompt login via the plugin. @@ -655,6 +660,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--concierge-authenticator-type", "jwt", "--concierge-authenticator-name", authenticator.Name, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger an LDAP-style login CLI prompt via the plugin. @@ -715,6 +721,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--concierge-authenticator-type", "jwt", "--concierge-authenticator-name", authenticator.Name, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Set up the username and password env vars to avoid the interactive prompts. @@ -787,6 +794,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--concierge-authenticator-type", "jwt", "--concierge-authenticator-name", authenticator.Name, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger an LDAP-style login CLI prompt via the plugin. @@ -847,6 +855,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--concierge-authenticator-type", "jwt", "--concierge-authenticator-name", authenticator.Name, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Set up the username and password env vars to avoid the interactive prompts. @@ -924,6 +933,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--oidc-ca-bundle", testCABundlePath, "--upstream-identity-provider-flow", "browser_authcode", "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a browser login via the plugin. @@ -980,6 +990,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--oidc-ca-bundle", testCABundlePath, "--upstream-identity-provider-flow", "browser_authcode", "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a browser login via the plugin. @@ -1036,6 +1047,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { "--oidc-ca-bundle", testCABundlePath, "--upstream-identity-provider-flow", "cli_password", // put cli_password in the kubeconfig, so we can override it with the env var "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Override the --upstream-identity-provider-flow flag from the kubeconfig using the env var. @@ -1311,7 +1323,7 @@ func requireUserCanUseKubectlWithoutAuthenticatingAgain( require.NoError(t, err) })) - downstreamScopes := []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience"} + downstreamScopes := []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"} sort.Strings(downstreamScopes) token := cache.GetToken(oidcclient.SessionCacheKey{ Issuer: downstream.Spec.Issuer, @@ -1326,12 +1338,16 @@ func requireUserCanUseKubectlWithoutAuthenticatingAgain( idTokenClaims := token.IDToken.Claims require.Equal(t, expectedUsername, idTokenClaims[oidc.DownstreamUsernameClaim]) - // The groups claim in the file ends up as an []interface{}, so adjust our expectation to match. - expectedGroupsAsEmptyInterfaces := make([]interface{}, 0, len(expectedGroups)) - for _, g := range expectedGroups { - expectedGroupsAsEmptyInterfaces = append(expectedGroupsAsEmptyInterfaces, g) + if expectedGroups == nil { + require.Nil(t, idTokenClaims[oidc.DownstreamGroupsClaim]) + } else { + // The groups claim in the file ends up as an []interface{}, so adjust our expectation to match. + expectedGroupsAsEmptyInterfaces := make([]interface{}, 0, len(expectedGroups)) + for _, g := range expectedGroups { + expectedGroupsAsEmptyInterfaces = append(expectedGroupsAsEmptyInterfaces, g) + } + require.ElementsMatch(t, expectedGroupsAsEmptyInterfaces, idTokenClaims[oidc.DownstreamGroupsClaim]) } - require.ElementsMatch(t, expectedGroupsAsEmptyInterfaces, idTokenClaims[oidc.DownstreamGroupsClaim]) expectedGroupsPlusAuthenticated := append([]string{}, expectedGroups...) expectedGroupsPlusAuthenticated = append(expectedGroupsPlusAuthenticated, "system:authenticated") diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 47587db7..2fa1679b 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -1381,7 +1381,7 @@ func testSupervisorLogin( ClientID: "pinniped-cli", Endpoint: discovery.Endpoint(), RedirectURL: localCallbackServer.URL, - Scopes: []string{"openid", "pinniped:request-audience", "offline_access"}, + Scopes: []string{"openid", "pinniped:request-audience", "offline_access", "groups"}, } // Build a valid downstream authorize URL for the supervisor. @@ -1416,7 +1416,7 @@ func testSupervisorLogin( t.Logf("got callback request: %s", testlib.MaskTokens(callback.URL.String())) if wantErrorType == "" { require.Equal(t, stateParam.String(), callback.URL.Query().Get("state")) - require.ElementsMatch(t, []string{"openid", "pinniped:request-audience", "offline_access"}, strings.Split(callback.URL.Query().Get("scope"), " ")) + require.ElementsMatch(t, []string{"openid", "pinniped:request-audience", "offline_access", "groups"}, strings.Split(callback.URL.Query().Get("scope"), " ")) authcode := callback.URL.Query().Get("code") require.NotEmpty(t, authcode) From 64cd8b0b9fe32e826548b2692e64e876f2b41700 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 15 Jun 2022 13:41:22 -0700 Subject: [PATCH 2/6] Add e2e test for groups scope Signed-off-by: Margo Crawford --- .../testutil/oidctestutil/oidctestutil.go | 3 +- test/integration/e2e_test.go | 177 +++++++++--------- 2 files changed, 90 insertions(+), 90 deletions(-) diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index f5598edc..508e4bf0 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -14,8 +14,6 @@ import ( "testing" "time" - "k8s.io/utils/strings/slices" - coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/gorilla/securecookie" "github.com/ory/fosite" @@ -27,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/utils/strings/slices" "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/crud" diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index b2305da8..9bbf1589 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -193,14 +193,85 @@ func TestE2EFullIntegration_Browser(t *testing.T) { requireKubectlGetNamespaceOutput(t, env, waitForKubectlOutput(t, kubectlOutputChan)) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) + }) + + // If scopes aren't specified, we don't request the groups scope, which means we won't get any groups back in our token. + t.Run("with Supervisor OIDC upstream IDP and browser flow, scopes not specified", func(t *testing.T) { + testCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + t.Cleanup(cancel) + + tempDir := testutil.TempDir(t) // per-test tmp dir to avoid sharing files between tests + + // Start a fresh browser driver because we don't want to share cookies between the various tests in this file. + page := browsertest.Open(t) + + expectedUsername := env.SupervisorUpstreamOIDC.Username + + // Create a ClusterRoleBinding to give our test user from the upstream read-only access to the cluster. + testlib.CreateTestClusterRoleBinding(t, + rbacv1.Subject{Kind: rbacv1.UserKind, APIGroup: rbacv1.GroupName, Name: expectedUsername}, + rbacv1.RoleRef{Kind: "ClusterRole", APIGroup: rbacv1.GroupName, Name: "view"}, ) + testlib.WaitForUserToHaveAccess(t, expectedUsername, []string{}, &authorizationv1.ResourceAttributes{ + Verb: "get", + Group: "", + Version: "v1", + Resource: "namespaces", + }) + + // Create upstream OIDC provider and wait for it to become ready. + testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ + Issuer: env.SupervisorUpstreamOIDC.Issuer, + TLS: &idpv1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)), + }, + AuthorizationConfig: idpv1alpha1.OIDCAuthorizationConfig{ + AdditionalScopes: env.SupervisorUpstreamOIDC.AdditionalScopes, + }, + Claims: idpv1alpha1.OIDCClaims{ + Username: env.SupervisorUpstreamOIDC.UsernameClaim, + Groups: env.SupervisorUpstreamOIDC.GroupsClaim, + }, + Client: idpv1alpha1.OIDCClient{ + SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name, + }, + }, idpv1alpha1.PhaseReady) + + // Use a specific session cache for this test. + sessionCachePath := tempDir + "/test-sessions.yaml" + + kubeconfigPath := runPinnipedGetKubeconfig(t, env, pinnipedExe, tempDir, []string{ + "get", "kubeconfig", + "--concierge-api-group-suffix", env.APIGroupSuffix, + "--concierge-authenticator-type", "jwt", + "--concierge-authenticator-name", authenticator.Name, + "--oidc-skip-browser", + "--oidc-ca-bundle", testCABundlePath, + "--oidc-session-cache", sessionCachePath, + }) + + // Run "kubectl get namespaces" which should trigger a browser login via the plugin. + kubectlCmd := exec.CommandContext(testCtx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath, "-v", "6") + kubectlCmd.Env = append(os.Environ(), env.ProxyEnv()...) + + // Run the kubectl command, wait for the Pinniped CLI to print the authorization URL, and open it in the browser. + kubectlOutputChan := startKubectlAndOpenAuthorizationURLInBrowser(testCtx, t, kubectlCmd, page) + + // Confirm that we got to the upstream IDP's login page, fill out the form, and submit the form. + browsertest.LoginToUpstreamOIDC(t, page, env.SupervisorUpstreamOIDC) + + // Expect to be redirected to the downstream callback which is serving the form_post HTML. + t.Logf("waiting for response page %s", downstream.Spec.Issuer) + browsertest.WaitForURL(t, page, regexp.MustCompile(regexp.QuoteMeta(downstream.Spec.Issuer))) + + // The response page should have done the background fetch() and POST'ed to the CLI's callback. + // It should now be in the "success" state. + formpostExpectSuccessState(t, page) + + requireKubectlGetNamespaceOutput(t, env, waitForKubectlOutput(t, kubectlOutputChan)) + + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, []string{}, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience"}) }) t.Run("with Supervisor OIDC upstream IDP and manual authcode copy-paste from browser flow", func(t *testing.T) { @@ -311,14 +382,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { t.Logf("first kubectl command took %s", time.Since(start).String()) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) t.Run("access token based refresh with Supervisor OIDC upstream IDP and manual authcode copy-paste from browser flow", func(t *testing.T) { @@ -454,14 +518,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { t.Logf("first kubectl command took %s", time.Since(start).String()) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) t.Run("with Supervisor OIDC upstream IDP and CLI password flow without web browser", func(t *testing.T) { @@ -544,14 +601,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { t.Logf("first kubectl command took %s", time.Since(start).String()) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) t.Run("with Supervisor OIDC upstream IDP and CLI password flow when OIDCIdentityProvider disallows it", func(t *testing.T) { @@ -687,14 +737,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { t.Logf("first kubectl command took %s", time.Since(start).String()) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) // Add an LDAP upstream IDP and try using it to authenticate during kubectl commands @@ -760,14 +803,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { require.NoError(t, os.Unsetenv(usernameEnvVar)) require.NoError(t, os.Unsetenv(passwordEnvVar)) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) // Add an Active Directory upstream IDP and try using it to authenticate during kubectl commands @@ -821,14 +857,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { t.Logf("first kubectl command took %s", time.Since(start).String()) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) // Add an ActiveDirectory upstream IDP and try using it to authenticate during kubectl commands @@ -894,14 +923,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { require.NoError(t, os.Unsetenv(usernameEnvVar)) require.NoError(t, os.Unsetenv(passwordEnvVar)) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) // Add an LDAP upstream IDP and try using it to authenticate during kubectl commands, using the browser flow. @@ -951,14 +973,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { requireKubectlGetNamespaceOutput(t, env, waitForKubectlOutput(t, kubectlOutputChan)) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) // Add an Active Directory upstream IDP and try using it to authenticate during kubectl commands, using the browser flow. @@ -1008,14 +1023,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { requireKubectlGetNamespaceOutput(t, env, waitForKubectlOutput(t, kubectlOutputChan)) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) // Add an LDAP upstream IDP and try using it to authenticate during kubectl commands, using the env var to choose the browser flow. @@ -1071,14 +1079,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { requireKubectlGetNamespaceOutput(t, env, waitForKubectlOutput(t, kubectlOutputChan)) - requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, - downstream, - kubeconfigPath, - sessionCachePath, - pinnipedExe, - expectedUsername, - expectedGroups, - ) + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, downstream, kubeconfigPath, sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"}) }) } @@ -1308,6 +1309,7 @@ func requireUserCanUseKubectlWithoutAuthenticatingAgain( pinnipedExe string, expectedUsername string, expectedGroups []string, + downstreamScopes []string, ) { // Run kubectl, which should work without any prompting for authentication. kubectlCmd := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) @@ -1323,7 +1325,6 @@ func requireUserCanUseKubectlWithoutAuthenticatingAgain( require.NoError(t, err) })) - downstreamScopes := []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"} sort.Strings(downstreamScopes) token := cache.GetToken(oidcclient.SessionCacheKey{ Issuer: downstream.Spec.Issuer, From 9903c5f79e9c78e1317623bb3d8918baad2e8ae6 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 22 Jun 2022 08:21:16 -0700 Subject: [PATCH 3/6] Handle refresh requests without groups scope Signed-off-by: Margo Crawford --- .../downstreamsession/downstream_session.go | 3 +- internal/oidc/login/post_login_handler.go | 1 - .../provider/dynamic_upstream_idp_provider.go | 2 + internal/oidc/token/token_handler.go | 69 ++-- internal/oidc/token/token_handler_test.go | 356 +++++++++++++----- test/integration/supervisor_login_test.go | 73 +++- test/integration/supervisor_warnings_test.go | 3 +- 7 files changed, 369 insertions(+), 138 deletions(-) diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index d5783e5e..fbb0ca52 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -10,12 +10,11 @@ import ( "net/url" "time" - "k8s.io/utils/strings/slices" - "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/strings/slices" "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/constable" diff --git a/internal/oidc/login/post_login_handler.go b/internal/oidc/login/post_login_handler.go index fdc480c0..dafe6e06 100644 --- a/internal/oidc/login/post_login_handler.go +++ b/internal/oidc/login/post_login_handler.go @@ -8,7 +8,6 @@ import ( "net/url" coreosoidc "github.com/coreos/go-oidc/v3/oidc" - "github.com/ory/fosite" "go.pinniped.dev/internal/httputil/httperr" diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 79771943..befcddb8 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -111,6 +111,8 @@ type UpstreamLDAPIdentityProviderI interface { PerformRefresh(ctx context.Context, storedRefreshAttributes StoredRefreshAttributes) (groups []string, err error) } +// StoredRefreshAttributes contains information about the user from the original login request +// and previous refreshes. type StoredRefreshAttributes struct { Username string Subject string diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 15f50ec1..6c3bf575 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -15,6 +15,7 @@ import ( "golang.org/x/oauth2" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/warning" + "k8s.io/utils/strings/slices" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/oidc" @@ -106,19 +107,21 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, return errorsx.WithStack(errMissingUpstreamSessionInternalError()) } + grantedScopes := accessRequest.GetGrantedScopes() + switch customSessionData.ProviderType { case psession.ProviderTypeOIDC: - return upstreamOIDCRefresh(ctx, session, providerCache) + return upstreamOIDCRefresh(ctx, session, providerCache, grantedScopes) case psession.ProviderTypeLDAP: - return upstreamLDAPRefresh(ctx, providerCache, session) + return upstreamLDAPRefresh(ctx, providerCache, session, grantedScopes) case psession.ProviderTypeActiveDirectory: - return upstreamLDAPRefresh(ctx, providerCache, session) + return upstreamLDAPRefresh(ctx, providerCache, session, grantedScopes) default: return errorsx.WithStack(errMissingUpstreamSessionInternalError()) } } -func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, providerCache oidc.UpstreamIdentityProvidersLister) error { +func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, providerCache oidc.UpstreamIdentityProvidersLister, grantedScopes []string) error { s := session.Custom if s.OIDC == nil { return errorsx.WithStack(errMissingUpstreamSessionInternalError()) @@ -177,30 +180,33 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, return err } - // If possible, update the user's group memberships. The configured groups claim name (if there is one) may or - // may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the - // claim name. It could also be missing because the claim was originally found in the ID token during login, but - // now we might not have a refreshed ID token. - // If the claim is found, then use it to update the user's group membership in the session. - // If the claim is not found, then we have no new information about groups, so skip updating the group membership - // and let any old groups memberships in the session remain. - refreshedGroups, err := downstreamsession.GetGroupsFromUpstreamIDToken(p, mergedClaims) - if err != nil { - return errUpstreamRefreshError().WithHintf( - "Upstream refresh error while extracting groups claim.").WithTrace(err). - WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType) - } - if refreshedGroups != nil { - oldGroups, err := getDownstreamGroupsFromPinnipedSession(session) + groupsScope := slices.Contains(grantedScopes, oidc.DownstreamGroupsScope) + if groupsScope { + // If possible, update the user's group memberships. The configured groups claim name (if there is one) may or + // may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the + // claim name. It could also be missing because the claim was originally found in the ID token during login, but + // now we might not have a refreshed ID token. + // If the claim is found, then use it to update the user's group membership in the session. + // If the claim is not found, then we have no new information about groups, so skip updating the group membership + // and let any old groups memberships in the session remain. + refreshedGroups, err := downstreamsession.GetGroupsFromUpstreamIDToken(p, mergedClaims) if err != nil { - return err + return errUpstreamRefreshError().WithHintf( + "Upstream refresh error while extracting groups claim.").WithTrace(err). + WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType) } - username, err := getDownstreamUsernameFromPinnipedSession(session) - if err != nil { - return err + if refreshedGroups != nil { + oldGroups, err := getDownstreamGroupsFromPinnipedSession(session) + if err != nil { + return err + } + username, err := getDownstreamUsernameFromPinnipedSession(session) + if err != nil { + return err + } + warnIfGroupsChanged(ctx, oldGroups, refreshedGroups, username) + session.Fosite.Claims.Extra[oidc.DownstreamGroupsClaim] = refreshedGroups } - warnIfGroupsChanged(ctx, oldGroups, refreshedGroups, username) - session.Fosite.Claims.Extra[oidc.DownstreamGroupsClaim] = refreshedGroups } // Upstream refresh may or may not return a new refresh token. If we got a new refresh token, then update it in @@ -291,7 +297,7 @@ func findOIDCProviderByNameAndValidateUID( WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } -func upstreamLDAPRefresh(ctx context.Context, providerCache oidc.UpstreamIdentityProvidersLister, session *psession.PinnipedSession) error { +func upstreamLDAPRefresh(ctx context.Context, providerCache oidc.UpstreamIdentityProvidersLister, session *psession.PinnipedSession, grantedScopes []string) error { username, err := getDownstreamUsernameFromPinnipedSession(session) if err != nil { return err @@ -339,10 +345,13 @@ func upstreamLDAPRefresh(ctx context.Context, providerCache oidc.UpstreamIdentit "Upstream refresh failed.").WithTrace(err). WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType) } - // Replace the old value with the new value. - session.Fosite.Claims.Extra[oidc.DownstreamGroupsClaim] = groups + groupsScope := slices.Contains(grantedScopes, oidc.DownstreamGroupsScope) + if groupsScope { + // Replace the old value with the new value. + session.Fosite.Claims.Extra[oidc.DownstreamGroupsClaim] = groups - warnIfGroupsChanged(ctx, oldGroups, groups, username) + warnIfGroupsChanged(ctx, oldGroups, groups, username) + } return nil } @@ -400,7 +409,7 @@ func getDownstreamGroupsFromPinnipedSession(session *psession.PinnipedSession) ( } downstreamGroupsInterface := extra[oidc.DownstreamGroupsClaim] if downstreamGroupsInterface == nil { - return nil, errorsx.WithStack(errMissingUpstreamSessionInternalError()) + return nil, nil } downstreamGroupsInterfaceList, ok := downstreamGroupsInterface.([]interface{}) if !ok { diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index ea0d9290..423a8c55 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -200,7 +200,7 @@ var ( happyAuthRequest = &http.Request{ Form: url.Values{ "response_type": {"code"}, - "scope": {"openid profile email"}, + "scope": {"openid profile email groups"}, "client_id": {goodClient}, "state": {"some-state-value-with-enough-bytes-to-exceed-min-allowed"}, "nonce": {goodNonce}, @@ -268,11 +268,12 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { { name: "request is valid and tokens are issued", authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid profile email groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token - wantRequestedScopes: []string{"openid", "profile", "email"}, - wantGrantedScopes: []string{"openid"}, + wantRequestedScopes: []string{"openid", "profile", "email", "groups"}, + wantGrantedScopes: []string{"openid", "groups"}, wantGroups: goodGroups, }, }, @@ -299,7 +300,7 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in", "refresh_token"}, // all possible tokens wantRequestedScopes: []string{"openid", "offline_access"}, wantGrantedScopes: []string{"openid", "offline_access"}, - wantGroups: goodGroups, + wantGroups: nil, }, }, }, @@ -316,6 +317,19 @@ func TestTokenEndpointAuthcodeExchange(t *testing.T) { }, }, }, + { + name: "groups scope is requested", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid profile email groups") }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token + wantRequestedScopes: []string{"openid", "profile", "email", "groups"}, + wantGrantedScopes: []string{"openid", "groups"}, + wantGroups: goodGroups, + }, + }, + }, // sad path { @@ -566,12 +580,12 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) { { name: "authcode exchange succeeds once and then fails when the same authcode is used again", authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access profile email") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access profile email groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access", "profile", "email"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "profile", "email", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: goodGroups, }, }, @@ -630,14 +644,14 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn successfulAuthCodeExchange := tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "pinniped:request-audience"}, - wantGrantedScopes: []string{"openid", "pinniped:request-audience"}, + wantRequestedScopes: []string{"openid", "pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"openid", "pinniped:request-audience", "groups"}, wantGroups: goodGroups, } doValidAuthCodeExchange := authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { - authRequest.Form.Set("scope", "openid pinniped:request-audience") + authRequest.Form.Set("scope", "openid pinniped:request-audience groups") }, want: successfulAuthCodeExchange, } @@ -732,13 +746,13 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn name: "access token missing pinniped:request-audience scope", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { - authRequest.Form.Set("scope", "openid") + authRequest.Form.Set("scope", "openid groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid"}, - wantGrantedScopes: []string{"openid"}, + wantRequestedScopes: []string{"openid", "groups"}, + wantGrantedScopes: []string{"openid", "groups"}, wantGroups: goodGroups, }, }, @@ -750,13 +764,13 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn name: "access token missing openid scope", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { - authRequest.Form.Set("scope", "pinniped:request-audience") + authRequest.Form.Set("scope", "pinniped:request-audience groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"pinniped:request-audience"}, - wantGrantedScopes: []string{"pinniped:request-audience"}, + wantRequestedScopes: []string{"pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"pinniped:request-audience", "groups"}, wantGroups: goodGroups, }, }, @@ -765,11 +779,28 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn wantResponseBodyContains: `missing the 'openid' scope`, }, { - name: "token minting failure", + name: "access token missing groups scope", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { authRequest.Form.Set("scope", "openid pinniped:request-audience") }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"access_token", "token_type", "expires_in", "scope", "id_token"}, + wantRequestedScopes: []string{"openid", "pinniped:request-audience"}, + wantGrantedScopes: []string{"openid", "pinniped:request-audience"}, + wantGroups: nil, + }, + }, + requestedAudience: "some-workload-cluster", + wantStatus: http.StatusOK, + }, + { + name: "token minting failure", + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "openid pinniped:request-audience groups") + }, // Fail to fetch a JWK signing key after the authcode exchange has happened. makeOathHelper: makeOauthHelperWithJWTKeyThatWorksOnlyOnce, want: successfulAuthCodeExchange, @@ -845,7 +876,10 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn require.NoError(t, json.Unmarshal(parsedJWT.UnsafePayloadWithoutVerification(), &tokenClaims)) // Make sure that these are the only fields in the token. - idTokenFields := []string{"sub", "aud", "iss", "jti", "auth_time", "exp", "iat", "rat", "groups", "username"} + idTokenFields := []string{"sub", "aud", "iss", "jti", "auth_time", "exp", "iat", "rat", "username"} + if test.authcodeExchange.want.wantGroups != nil { + idTokenFields = append(idTokenFields, "groups") + } require.ElementsMatch(t, idTokenFields, getMapKeys(tokenClaims)) // Assert that the returned token has expected claims values. @@ -859,7 +893,11 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn require.Equal(t, goodSubject, tokenClaims["sub"]) require.Equal(t, goodIssuer, tokenClaims["iss"]) require.Equal(t, goodUsername, tokenClaims["username"]) - require.Equal(t, toSliceOfInterface(test.authcodeExchange.want.wantGroups), tokenClaims["groups"]) + if test.authcodeExchange.want.wantGroups != nil { + require.Equal(t, toSliceOfInterface(test.authcodeExchange.want.wantGroups), tokenClaims["groups"]) + } else { + require.Nil(t, tokenClaims["groups"]) + } // Also assert that some are the same as the original downstream ID token. requireClaimsAreEqual(t, "iss", claimsOfFirstIDToken, tokenClaims) // issuer @@ -1003,8 +1041,8 @@ func TestRefreshGrant(t *testing.T) { want := tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantCustomSessionDataStored: wantCustomSessionDataStored, wantGroups: goodGroups, } @@ -1090,7 +1128,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1114,7 +1152,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1142,15 +1180,15 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCAccessTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCAccessTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "id_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: goodGroups, wantUpstreamOIDCValidateTokenCall: &expectedUpstreamValidateTokens{ oidcUpstreamName, @@ -1207,15 +1245,15 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: goodGroups, wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), false), @@ -1236,15 +1274,15 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: []string{"new-group1", "new-group2", "new-group3"}, wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), @@ -1265,15 +1303,15 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: []string{"new-group1", "new-group2", "new-group3"}, wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), @@ -1294,15 +1332,15 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: []string{}, // the user no longer belongs to any groups wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), @@ -1323,15 +1361,15 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: goodGroups, // the same groups as from the initial login wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), @@ -1348,7 +1386,7 @@ func TestRefreshGrant(t *testing.T) { PerformRefreshGroups: []string{"new-group1", "new-group2", "new-group3"}, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -1358,8 +1396,8 @@ func TestRefreshGrant(t *testing.T) { want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access"}, - wantGrantedScopes: []string{"openid", "offline_access"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, wantGroups: []string{"new-group1", "new-group2", "new-group3"}, wantUpstreamRefreshCall: happyLDAPUpstreamRefreshCall(), wantCustomSessionDataStored: happyLDAPCustomSessionData, @@ -1375,7 +1413,7 @@ func TestRefreshGrant(t *testing.T) { PerformRefreshGroups: []string{}, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -1385,9 +1423,120 @@ func TestRefreshGrant(t *testing.T) { want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, + wantGroups: []string{}, + wantUpstreamRefreshCall: happyLDAPUpstreamRefreshCall(), + wantCustomSessionDataStored: happyLDAPCustomSessionData, + }, + }, + }, + { + name: "ldap refresh grant when the upstream refresh when groups scope not requested on original request or refresh", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + PerformRefreshGroups: []string{}, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access"}, wantGrantedScopes: []string{"openid", "offline_access"}, - wantGroups: []string{}, + wantCustomSessionDataStored: happyLDAPCustomSessionData, + wantGroups: nil, + }, + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { + r.Body = happyRefreshRequestBody(refreshToken).WithScope("openid offline_access").ReadCloser() + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantGroups: nil, + wantUpstreamRefreshCall: happyLDAPUpstreamRefreshCall(), + wantCustomSessionDataStored: happyLDAPCustomSessionData, + }, + }, + }, + { + name: "oidc refresh grant when the upstream refresh when groups scope not requested on original request or refresh", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + "my-groups-claim": []string{"new-group1", "new-group2", "new-group3"}, // refreshed claims includes updated groups + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), + wantGroups: nil, + }, + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { + r.Body = happyRefreshRequestBody(refreshToken).WithScope("openid offline_access").ReadCloser() + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access"}, + wantGrantedScopes: []string{"openid", "offline_access"}, + wantGroups: nil, + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), + wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + }, + }, + }, + { + // fosite does not look at the scopes provided in refresh requests, although it is a valid parameter. + // even if 'groups' is not sent in the refresh request, we will send groups all the same. + name: "refresh grant when the upstream refresh when groups scope requested on original request but not refresh refresh", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + PerformRefreshGroups: []string{"new-group1", "new-group2", "new-group3"}, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, + customSessionData: happyLDAPCustomSessionData, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, + wantCustomSessionDataStored: happyLDAPCustomSessionData, + wantGroups: goodGroups, + }, + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { + r.Body = happyRefreshRequestBody(refreshToken).WithScope("openid offline_access").ReadCloser() + }, + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantSuccessBodyFields: []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "groups"}, + wantGroups: []string{"new-group1", "new-group2", "new-group3"}, // groups are updated even though the scope was not included wantUpstreamRefreshCall: happyLDAPUpstreamRefreshCall(), wantCustomSessionDataStored: happyLDAPCustomSessionData, }, @@ -1406,7 +1555,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1430,7 +1579,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1452,7 +1601,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1477,12 +1626,12 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience groups") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, - wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, + wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, wantGroups: goodGroups, wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), }, @@ -1494,8 +1643,8 @@ func TestRefreshGrant(t *testing.T) { want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, - wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, + wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, + wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience", "groups"}, wantGroups: goodGroups, wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), @@ -1515,7 +1664,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1605,7 +1754,7 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: nil, // this should not happen in practice - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(nil), }, refreshRequest: refreshRequestInputs{ @@ -1625,7 +1774,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamInitialRefreshToken}, }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: "", // this should not happen in practice @@ -1652,7 +1801,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamInitialRefreshToken}, }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: oidcUpstreamName, @@ -1679,7 +1828,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: "", // this should not happen in practice OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamInitialRefreshToken}, }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: oidcUpstreamName, @@ -1706,7 +1855,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: "not-an-allowed-provider-type", // this should not happen in practice OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamInitialRefreshToken}, }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: oidcUpstreamName, @@ -1733,7 +1882,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: nil, // this should not happen in practice }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: oidcUpstreamName, @@ -1763,7 +1912,7 @@ func TestRefreshGrant(t *testing.T) { UpstreamAccessToken: "", }, }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: oidcUpstreamName, @@ -1793,7 +1942,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamInitialRefreshToken}, }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: "this-name-will-not-be-found", // this could happen if the OIDCIdentityProvider was deleted since original login @@ -1825,7 +1974,7 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamInitialRefreshToken}, }, - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ // want the initial customSessionData to be unmodified ProviderName: oidcUpstreamName, @@ -1853,7 +2002,7 @@ func TestRefreshGrant(t *testing.T) { WithPerformRefreshError(errors.New("some upstream refresh error")).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1878,7 +2027,7 @@ func TestRefreshGrant(t *testing.T) { Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1910,7 +2059,7 @@ func TestRefreshGrant(t *testing.T) { Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1939,7 +2088,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -1970,7 +2119,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -2001,7 +2150,7 @@ func TestRefreshGrant(t *testing.T) { }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()), }, refreshRequest: refreshRequestInputs{ @@ -2027,7 +2176,7 @@ func TestRefreshGrant(t *testing.T) { PerformRefreshGroups: goodGroups, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2048,7 +2197,7 @@ func TestRefreshGrant(t *testing.T) { PerformRefreshGroups: goodGroups, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyActiveDirectoryCustomSessionData, @@ -2068,7 +2217,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: &psession.CustomSessionData{ ProviderUID: ldapUpstreamResourceUID, ProviderName: ldapUpstreamName, @@ -2104,7 +2253,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: &psession.CustomSessionData{ ProviderUID: activeDirectoryUpstreamResourceUID, ProviderName: activeDirectoryUpstreamName, @@ -2140,7 +2289,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: &psession.CustomSessionData{ ProviderUID: ldapUpstreamResourceUID, ProviderName: ldapUpstreamName, @@ -2180,7 +2329,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: &psession.CustomSessionData{ ProviderUID: ldapUpstreamResourceUID, ProviderName: ldapUpstreamName, @@ -2221,7 +2370,7 @@ func TestRefreshGrant(t *testing.T) { PerformRefreshErr: errors.New("Some error performing upstream refresh"), }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2249,7 +2398,7 @@ func TestRefreshGrant(t *testing.T) { PerformRefreshErr: errors.New("Some error performing upstream refresh"), }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyActiveDirectoryCustomSessionData, @@ -2272,7 +2421,7 @@ func TestRefreshGrant(t *testing.T) { name: "upstream ldap idp not found", idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2294,7 +2443,7 @@ func TestRefreshGrant(t *testing.T) { name: "upstream active directory idp not found", idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyActiveDirectoryCustomSessionData, @@ -2320,7 +2469,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2357,7 +2506,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, //fositeSessionData: &openid.DefaultSession{}, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( @@ -2399,7 +2548,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, //fositeSessionData: &openid.DefaultSession{}, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( @@ -2441,7 +2590,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, //fositeSessionData: &openid.DefaultSession{}, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( @@ -2483,7 +2632,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2509,7 +2658,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyActiveDirectoryCustomSessionData, @@ -2531,7 +2680,7 @@ func TestRefreshGrant(t *testing.T) { name: "upstream ldap idp not found", idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2553,7 +2702,7 @@ func TestRefreshGrant(t *testing.T) { name: "upstream active directory idp not found", idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyActiveDirectoryCustomSessionData, @@ -2579,7 +2728,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2616,7 +2765,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2657,7 +2806,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2696,7 +2845,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyLDAPCustomSessionData, @@ -2722,7 +2871,7 @@ func TestRefreshGrant(t *testing.T) { URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access groups") }, customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( happyActiveDirectoryCustomSessionData, @@ -2942,7 +3091,7 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p requireTokenEndpointBehavior(t, test.want, - goodGroups, // the old groups from the initial login + test.want.wantGroups, // the old groups from the initial login test.customSessionData, // the old custom session data from the initial login wantAtHashClaimInIDToken, wantNonceValueInIDToken, @@ -3174,7 +3323,6 @@ func simulateAuthEndpointHavingAlreadyRun( AuthTime: goodAuthTime, Extra: map[string]interface{}{ oidc.DownstreamUsernameClaim: goodUsername, - oidc.DownstreamGroupsClaim: goodGroups, }, }, Subject: "", // not used, note that callback_handler.go does not set this @@ -3193,6 +3341,10 @@ func simulateAuthEndpointHavingAlreadyRun( if strings.Contains(authRequest.Form.Get("scope"), "pinniped:request-audience") { authRequester.GrantScope("pinniped:request-audience") } + if strings.Contains(authRequest.Form.Get("scope"), "groups") { + authRequester.GrantScope("groups") + session.Fosite.Claims.Extra[oidc.DownstreamGroupsClaim] = goodGroups + } authResponder, err := oauthHelper.NewAuthorizeResponse(ctx, authRequester, session) require.NoError(t, err) return authResponder @@ -3429,10 +3581,13 @@ func requireValidStoredRequest( require.Equal(t, goodSubject, claims.Subject) // Our custom claims from the authorize endpoint should still be set. - require.Equal(t, map[string]interface{}{ + expectedExtra := map[string]interface{}{ "username": goodUsername, - "groups": toSliceOfInterface(wantGroups), - }, claims.Extra) + } + if wantGroups != nil { + expectedExtra["groups"] = toSliceOfInterface(wantGroups) + } + require.Equal(t, expectedExtra, claims.Extra) // We are in charge of setting these fields. For the purpose of testing, we ensure that the // sentinel test value is set correctly. @@ -3551,13 +3706,16 @@ func requireValidIDToken( // Note that there is a bug in fosite which prevents the `at_hash` claim from appearing in this ID token // during the initial authcode exchange, but does not prevent `at_hash` from appearing in the refreshed ID token. // We can add a workaround for this later. - idTokenFields := []string{"sub", "aud", "iss", "jti", "auth_time", "exp", "iat", "rat", "groups", "username"} + idTokenFields := []string{"sub", "aud", "iss", "jti", "auth_time", "exp", "iat", "rat", "username"} if wantAtHashClaimInIDToken { idTokenFields = append(idTokenFields, "at_hash") } if wantNonceValueInIDToken { idTokenFields = append(idTokenFields, "nonce") } + if wantGroupsInIDToken != nil { + idTokenFields = append(idTokenFields, "groups") + } // make sure that these are the only fields in the token var m map[string]interface{} diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 2fa1679b..b5e22ca1 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -25,6 +25,7 @@ import ( "golang.org/x/oauth2" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/strings/slices" configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" idpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" @@ -162,6 +163,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { deleteTestUser func(t *testing.T, username string) requestAuthorization func(t *testing.T, downstreamIssuer, downstreamAuthorizeURL, downstreamCallbackURL, username, password string, httpClient *http.Client) createIDP func(t *testing.T) string + downstreamScopes []string wantLocalhostCallbackToNeverHappen bool wantDownstreamIDTokenSubjectToMatch string wantDownstreamIDTokenUsernameToMatch func(username string) string @@ -329,6 +331,55 @@ func TestSupervisorLogin_Browser(t *testing.T) { }, wantDownstreamIDTokenGroups: env.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs, }, + { + name: "ldap without requesting groups scope", + maybeSkip: skipLDAPTests, + createIDP: func(t *testing.T) string { + idp, _ := createLDAPIdentityProvider(t, nil) + return idp.Name + }, + downstreamScopes: []string{"openid", "pinniped:request-audience", "offline_access"}, + requestAuthorization: func(t *testing.T, _, downstreamAuthorizeURL, _, _, _ string, httpClient *http.Client) { + requestAuthorizationUsingCLIPasswordFlow(t, + downstreamAuthorizeURL, + env.SupervisorUpstreamLDAP.TestUserMailAttributeValue, // username to present to server during login + env.SupervisorUpstreamLDAP.TestUserPassword, // password to present to server during login + httpClient, + false, + ) + }, + // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( + "ldaps://"+env.SupervisorUpstreamLDAP.Host+ + "?base="+url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase)+ + "&sub="+base64.RawURLEncoding.EncodeToString([]byte(env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue)), + ) + "$", + // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { + return "^" + regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserMailAttributeValue) + "$" + }, + wantDownstreamIDTokenGroups: []string{}, + }, + { + name: "oidc without requesting groups scope", + maybeSkip: skipNever, + createIDP: func(t *testing.T) string { + spec := basicOIDCIdentityProviderSpec() + spec.Claims = idpv1alpha1.OIDCClaims{ + Username: env.SupervisorUpstreamOIDC.UsernameClaim, + Groups: env.SupervisorUpstreamOIDC.GroupsClaim, + } + spec.AuthorizationConfig = idpv1alpha1.OIDCAuthorizationConfig{ + AdditionalScopes: env.SupervisorUpstreamOIDC.AdditionalScopes, + } + return testlib.CreateTestOIDCIdentityProvider(t, spec, idpv1alpha1.PhaseReady).Name + }, + downstreamScopes: []string{"openid", "pinniped:request-audience", "offline_access"}, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowOIDC, + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" }, + wantDownstreamIDTokenGroups: nil, + }, { name: "ldap with browser flow", maybeSkip: skipLDAPTests, @@ -1123,6 +1174,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { tt.breakRefreshSessionData, tt.createTestUser, tt.deleteTestUser, + tt.downstreamScopes, tt.wantLocalhostCallbackToNeverHappen, tt.wantDownstreamIDTokenSubjectToMatch, tt.wantDownstreamIDTokenUsernameToMatch, @@ -1260,6 +1312,7 @@ func testSupervisorLogin( breakRefreshSessionData func(t *testing.T, pinnipedSession *psession.PinnipedSession, idpName, username string), createTestUser func(t *testing.T) (string, string), deleteTestUser func(t *testing.T, username string), + downstreamScopes []string, wantLocalhostCallbackToNeverHappen bool, wantDownstreamIDTokenSubjectToMatch string, wantDownstreamIDTokenUsernameToMatch func(username string) string, @@ -1372,6 +1425,10 @@ func testSupervisorLogin( // Start a callback server on localhost. localCallbackServer := startLocalCallbackServer(t) + if downstreamScopes == nil { + downstreamScopes = []string{"openid", "pinniped:request-audience", "offline_access", "groups"} + } + // Form the OAuth2 configuration corresponding to our CLI client. // Note that this is not using response_type=form_post, so the Supervisor will redirect to the callback endpoint // directly, without using the Javascript form_post HTML page to POST back to the callback endpoint. The e2e @@ -1381,7 +1438,7 @@ func testSupervisorLogin( ClientID: "pinniped-cli", Endpoint: discovery.Endpoint(), RedirectURL: localCallbackServer.URL, - Scopes: []string{"openid", "pinniped:request-audience", "offline_access", "groups"}, + Scopes: downstreamScopes, } // Build a valid downstream authorize URL for the supervisor. @@ -1414,9 +1471,9 @@ func testSupervisorLogin( require.NoError(t, err) t.Logf("got callback request: %s", testlib.MaskTokens(callback.URL.String())) - if wantErrorType == "" { + if wantErrorType == "" { // nolint:nestif require.Equal(t, stateParam.String(), callback.URL.Query().Get("state")) - require.ElementsMatch(t, []string{"openid", "pinniped:request-audience", "offline_access", "groups"}, strings.Split(callback.URL.Query().Get("scope"), " ")) + require.ElementsMatch(t, downstreamScopes, strings.Split(callback.URL.Query().Get("scope"), " ")) authcode := callback.URL.Query().Get("code") require.NotEmpty(t, authcode) @@ -1427,7 +1484,10 @@ func testSupervisorLogin( tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier()) require.NoError(t, err) - expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat", "username", "groups"} + expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat", "username"} + if slices.Contains(downstreamScopes, "groups") { + expectedIDTokenClaims = append(expectedIDTokenClaims, "groups") + } verifyTokenResponse(t, tokenResponse, discovery, downstreamOAuth2Config, nonceParam, expectedIDTokenClaims, wantDownstreamIDTokenSubjectToMatch, wantDownstreamIDTokenUsernameToMatch(username), wantDownstreamIDTokenGroups) @@ -1464,7 +1524,10 @@ func testSupervisorLogin( require.NoError(t, err) // When refreshing, expect to get an "at_hash" claim, but no "nonce" claim. - expectRefreshedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "rat", "username", "groups", "at_hash"} + expectRefreshedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "rat", "username", "at_hash"} + if slices.Contains(downstreamScopes, "groups") { + expectRefreshedIDTokenClaims = append(expectRefreshedIDTokenClaims, "groups") + } verifyTokenResponse(t, refreshedTokenResponse, discovery, downstreamOAuth2Config, "", expectRefreshedIDTokenClaims, wantDownstreamIDTokenSubjectToMatch, wantDownstreamIDTokenUsernameToMatch(username), refreshedGroups) diff --git a/test/integration/supervisor_warnings_test.go b/test/integration/supervisor_warnings_test.go index 74b5aab0..27c58179 100644 --- a/test/integration/supervisor_warnings_test.go +++ b/test/integration/supervisor_warnings_test.go @@ -119,6 +119,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { "--concierge-authenticator-name", authenticator.Name, "--oidc-session-cache", sessionCachePath, "--credential-cache", credentialCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a cli-based login. @@ -171,7 +172,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { })) // construct the cache key - downstreamScopes := []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience"} + downstreamScopes := []string{coreosoidc.ScopeOfflineAccess, coreosoidc.ScopeOpenID, "pinniped:request-audience", "groups"} sort.Strings(downstreamScopes) sessionCacheKey := oidcclient.SessionCacheKey{ Issuer: downstream.Spec.Issuer, From c70a0b99a846b399027abdae1a2fdace636d5c8c Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 22 Jun 2022 10:58:08 -0700 Subject: [PATCH 4/6] Don't do ldap group search when group scope not specified Signed-off-by: Margo Crawford --- internal/authenticators/authenticators.go | 2 +- .../active_directory_upstream_watcher.go | 6 +-- .../active_directory_upstream_watcher_test.go | 38 +++++++++--------- internal/oidc/auth/auth_handler.go | 2 +- internal/oidc/login/post_login_handler.go | 2 +- .../provider/dynamic_upstream_idp_provider.go | 7 ++-- internal/oidc/token/token_handler.go | 5 ++- .../testutil/oidctestutil/oidctestutil.go | 6 +-- internal/upstreamldap/upstreamldap.go | 39 ++++++++++++------- internal/upstreamldap/upstreamldap_test.go | 19 ++++----- test/integration/ldap_client_test.go | 22 +++++++++-- 11 files changed, 88 insertions(+), 60 deletions(-) diff --git a/internal/authenticators/authenticators.go b/internal/authenticators/authenticators.go index e343ecd1..9d675c2a 100644 --- a/internal/authenticators/authenticators.go +++ b/internal/authenticators/authenticators.go @@ -31,7 +31,7 @@ import ( // See k8s.io/apiserver/pkg/authentication/authenticator/interfaces.go for the token authenticator // interface, as well as the Response type. type UserAuthenticator interface { - AuthenticateUser(ctx context.Context, username, password string) (*Response, bool, error) + AuthenticateUser(ctx context.Context, username, password string, grantedScopes []string) (*Response, bool, error) } type Response struct { diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 4aaa41b9..03bdf332 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -338,7 +338,7 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){ "objectGUID": microsoftUUIDFromBinaryAttr("objectGUID"), }, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ pwdLastSetAttribute: upstreamldap.AttributeUnchangedSinceLogin(pwdLastSetAttribute), userAccountControlAttribute: validUserAccountControl, userAccountControlComputedAttribute: validComputedUserAccountControl, @@ -437,7 +437,7 @@ func getDomainFromDistinguishedName(distinguishedName string) (string, error) { return strings.Join(domainComponents[1:], "."), nil } -func validUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttributes) error { +func validUserAccountControl(entry *ldap.Entry, _ provider.RefreshAttributes) error { userAccountControl, err := strconv.Atoi(entry.GetAttributeValue(userAccountControlAttribute)) if err != nil { return err @@ -450,7 +450,7 @@ func validUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttribut return nil } -func validComputedUserAccountControl(entry *ldap.Entry, _ provider.StoredRefreshAttributes) error { +func validComputedUserAccountControl(entry *ldap.Entry, _ provider.RefreshAttributes) error { userAccountControl, err := strconv.Atoi(entry.GetAttributeValue(userAccountControlComputedAttribute)) if err != nil { return err diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go index ce69b4af..dc5fa693 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -222,7 +222,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -564,7 +564,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -633,7 +633,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: "sAMAccountName", }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -705,7 +705,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -784,7 +784,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -847,7 +847,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -997,7 +997,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1146,7 +1146,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1217,7 +1217,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1483,7 +1483,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, GroupAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"sAMAccountName": groupSAMAccountNameWithDomainSuffix}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1542,7 +1542,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1605,7 +1605,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1668,7 +1668,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1879,7 +1879,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { GroupNameAttribute: testGroupNameAttrName, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -1941,7 +1941,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { SkipGroupRefresh: true, }, UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": microsoftUUIDFromBinaryAttr("objectGUID")}, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ "pwdLastSet": upstreamldap.AttributeUnchangedSinceLogin("pwdLastSet"), "userAccountControl": validUserAccountControl, "msDS-User-Account-Control-Computed": validComputedUserAccountControl, @@ -2083,8 +2083,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { expectedRefreshAttributeChecks := copyOfExpectedValueForResultingCache.RefreshAttributeChecks actualRefreshAttributeChecks := actualConfig.RefreshAttributeChecks - copyOfExpectedValueForResultingCache.RefreshAttributeChecks = map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{} - actualConfig.RefreshAttributeChecks = map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{} + copyOfExpectedValueForResultingCache.RefreshAttributeChecks = map[string]func(*ldap.Entry, provider.RefreshAttributes) error{} + actualConfig.RefreshAttributeChecks = map[string]func(*ldap.Entry, provider.RefreshAttributes) error{} require.Equal(t, len(expectedRefreshAttributeChecks), len(actualRefreshAttributeChecks)) for k, v := range expectedRefreshAttributeChecks { require.NotNil(t, actualRefreshAttributeChecks[k]) @@ -2333,7 +2333,7 @@ func TestValidUserAccountControl(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - err := validUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) + err := validUserAccountControl(tt.entry, provider.RefreshAttributes{}) if tt.wantErr != "" { require.Error(t, err) @@ -2394,7 +2394,7 @@ func TestValidComputedUserAccountControl(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - err := validComputedUserAccountControl(tt.entry, provider.StoredRefreshAttributes{}) + err := validComputedUserAccountControl(tt.entry, provider.RefreshAttributes{}) if tt.wantErr != "" { require.Error(t, err) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 67b1581b..adbbec7c 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -131,7 +131,7 @@ func handleAuthRequestForLDAPUpstreamCLIFlow( return nil } - authenticateResponse, authenticated, err := ldapUpstream.AuthenticateUser(r.Context(), username, password) + authenticateResponse, authenticated, err := ldapUpstream.AuthenticateUser(r.Context(), username, password, authorizeRequester.GetGrantedScopes()) if err != nil { plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", ldapUpstream.GetName()) return httperr.New(http.StatusBadGateway, "unexpected error during upstream authentication") diff --git a/internal/oidc/login/post_login_handler.go b/internal/oidc/login/post_login_handler.go index dafe6e06..bc851c54 100644 --- a/internal/oidc/login/post_login_handler.go +++ b/internal/oidc/login/post_login_handler.go @@ -60,7 +60,7 @@ func NewPostHandler(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvider } // Attempt to authenticate the user with the upstream IDP. - authenticateResponse, authenticated, err := ldapUpstream.AuthenticateUser(r.Context(), username, password) + authenticateResponse, authenticated, err := ldapUpstream.AuthenticateUser(r.Context(), username, password, authorizeRequester.GetGrantedScopes()) if err != nil { plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", ldapUpstream.GetName()) // There was some problem during authentication with the upstream, aside from bad username/password. diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index befcddb8..a5eabea5 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -108,17 +108,18 @@ type UpstreamLDAPIdentityProviderI interface { authenticators.UserAuthenticator // PerformRefresh performs a refresh against the upstream LDAP identity provider - PerformRefresh(ctx context.Context, storedRefreshAttributes StoredRefreshAttributes) (groups []string, err error) + PerformRefresh(ctx context.Context, storedRefreshAttributes RefreshAttributes) (groups []string, err error) } -// StoredRefreshAttributes contains information about the user from the original login request +// RefreshAttributes contains information about the user from the original login request // and previous refreshes. -type StoredRefreshAttributes struct { +type RefreshAttributes struct { Username string Subject string DN string Groups []string AdditionalAttributes map[string]string + GrantedScopes []string } type DynamicUpstreamIDPProvider interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 6c3bf575..76727a12 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -181,7 +181,7 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, } groupsScope := slices.Contains(grantedScopes, oidc.DownstreamGroupsScope) - if groupsScope { + if groupsScope { //nolint:nestif // If possible, update the user's group memberships. The configured groups claim name (if there is one) may or // may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the // claim name. It could also be missing because the claim was originally found in the ID token during login, but @@ -333,12 +333,13 @@ func upstreamLDAPRefresh(ctx context.Context, providerCache oidc.UpstreamIdentit return errorsx.WithStack(errMissingUpstreamSessionInternalError()) } // run PerformRefresh - groups, err := p.PerformRefresh(ctx, provider.StoredRefreshAttributes{ + groups, err := p.PerformRefresh(ctx, provider.RefreshAttributes{ Username: username, Subject: subject, DN: dn, Groups: oldGroups, AdditionalAttributes: additionalAttributes, + GrantedScopes: grantedScopes, }) if err != nil { return errUpstreamRefreshError().WithHint( diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 508e4bf0..fb1e8a7a 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -92,7 +92,7 @@ type ValidateTokenAndMergeWithUserInfoArgs struct { type ValidateRefreshArgs struct { Ctx context.Context Tok *oauth2.Token - StoredAttributes provider.StoredRefreshAttributes + StoredAttributes provider.RefreshAttributes } type TestUpstreamLDAPIdentityProvider struct { @@ -116,7 +116,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetName() string { return u.Name } -func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { +func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string, grantedScopes []string) (*authenticators.Response, bool, error) { return u.AuthenticateFunc(ctx, username, password) } @@ -124,7 +124,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL { return u.URL } -func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, storedRefreshAttributes provider.StoredRefreshAttributes) ([]string, error) { +func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, storedRefreshAttributes provider.RefreshAttributes) ([]string, error) { if u.performRefreshArgs == nil { u.performRefreshArgs = make([]*PerformRefreshArgs, 0) } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index ddee048b..7317d939 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -16,6 +16,10 @@ import ( "strings" "time" + "go.pinniped.dev/internal/oidc" + + "k8s.io/utils/strings/slices" + "github.com/go-ldap/ldap/v3" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" @@ -118,7 +122,7 @@ type ProviderConfig struct { GroupAttributeParsingOverrides map[string]func(*ldap.Entry) (string, error) // RefreshAttributeChecks are extra checks that attributes in a refresh response are as expected. - RefreshAttributeChecks map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error + RefreshAttributeChecks map[string]func(*ldap.Entry, provider.RefreshAttributes) error } // UserSearchConfig contains information about how to search for users in the upstream LDAP IDP. @@ -175,7 +179,7 @@ func (p *Provider) GetConfig() ProviderConfig { return p.c } -func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes provider.StoredRefreshAttributes) ([]string, error) { +func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes provider.RefreshAttributes) ([]string, error) { t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches userDN := storedRefreshAttributes.DN @@ -238,6 +242,10 @@ func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes p if p.c.GroupSearch.SkipGroupRefresh { return storedRefreshAttributes.Groups, nil } + // if we were not granted the groups scope, we should not search for groups or return any. + if !slices.Contains(storedRefreshAttributes.GrantedScopes, oidc.DownstreamGroupsScope) { + return nil, nil + } mappedGroupNames, err := p.searchGroupsForUserDN(conn, userDN) if err != nil { @@ -398,23 +406,23 @@ func (p *Provider) TestConnection(ctx context.Context) error { // authentication for a given end user's username. It runs the same logic as AuthenticateUser except it does // not bind as that user, so it does not test their password. It returns the same values that a real call to // AuthenticateUser with the correct password would return. -func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) (*authenticators.Response, bool, error) { +func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string, grantedScopes []string) (*authenticators.Response, bool, error) { endUserBindFunc := func(conn Conn, foundUserDN string) error { // Act as if the end user bind always succeeds. return nil } - return p.authenticateUserImpl(ctx, username, endUserBindFunc) + return p.authenticateUserImpl(ctx, username, grantedScopes, endUserBindFunc) } // Authenticate an end user and return their mapped username, groups, and UID. Implements authenticators.UserAuthenticator. -func (p *Provider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { +func (p *Provider) AuthenticateUser(ctx context.Context, username, password string, grantedScopes []string) (*authenticators.Response, bool, error) { endUserBindFunc := func(conn Conn, foundUserDN string) error { return conn.Bind(foundUserDN, password) } - return p.authenticateUserImpl(ctx, username, endUserBindFunc) + return p.authenticateUserImpl(ctx, username, grantedScopes, endUserBindFunc) } -func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) { +func (p *Provider) authenticateUserImpl(ctx context.Context, username string, grantedScopes []string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) { t := trace.FromContext(ctx).Nest("slow ldap authenticate user attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches @@ -443,7 +451,7 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi return nil, false, fmt.Errorf(`error binding as %q before user search: %w`, p.c.BindUsername, err) } - response, err := p.searchAndBindUser(conn, username, bindFunc) + response, err := p.searchAndBindUser(conn, username, grantedScopes, bindFunc) if err != nil { p.traceAuthFailure(t, err) return nil, false, err @@ -540,7 +548,7 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e return searchBase, nil } -func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, error) { +func (p *Provider) searchAndBindUser(conn Conn, username string, grantedScopes []string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, error) { searchResult, err := conn.Search(p.userSearchRequest(username)) if err != nil { plog.All(`error searching for user`, @@ -586,9 +594,12 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c return nil, err } - mappedGroupNames, err := p.searchGroupsForUserDN(conn, userEntry.DN) - if err != nil { - return nil, err + var mappedGroupNames []string + if slices.Contains(grantedScopes, oidc.DownstreamGroupsScope) { + mappedGroupNames, err = p.searchGroupsForUserDN(conn, userEntry.DN) + if err != nil { + return nil, err + } } mappedRefreshAttributes := make(map[string]string) @@ -822,8 +833,8 @@ func (p *Provider) traceRefreshFailure(t *trace.Trace, err error) { ) } -func AttributeUnchangedSinceLogin(attribute string) func(*ldap.Entry, provider.StoredRefreshAttributes) error { - return func(entry *ldap.Entry, storedAttributes provider.StoredRefreshAttributes) error { +func AttributeUnchangedSinceLogin(attribute string) func(*ldap.Entry, provider.RefreshAttributes) error { + return func(entry *ldap.Entry, storedAttributes provider.RefreshAttributes) error { prevAttributeValue := storedAttributes.AdditionalAttributes[attribute] newValues := entry.GetRawAttributeValues(attribute) diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index b4ee6bdf..9a9ca782 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -638,8 +638,8 @@ func TestEndUserAuthentication(t *testing.T) { username: testUpstreamUsername, password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { - p.RefreshAttributeChecks = map[string]func(entry *ldap.Entry, attributes provider.StoredRefreshAttributes) error{ - "some-attribute-to-check-during-refresh": func(entry *ldap.Entry, attributes provider.StoredRefreshAttributes) error { + p.RefreshAttributeChecks = map[string]func(entry *ldap.Entry, attributes provider.RefreshAttributes) error{ + "some-attribute-to-check-during-refresh": func(entry *ldap.Entry, attributes provider.RefreshAttributes) error { return nil }, } @@ -676,8 +676,8 @@ func TestEndUserAuthentication(t *testing.T) { username: testUpstreamUsername, password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { - p.RefreshAttributeChecks = map[string]func(entry *ldap.Entry, attributes provider.StoredRefreshAttributes) error{ - "some-attribute-to-check-during-refresh": func(entry *ldap.Entry, attributes provider.StoredRefreshAttributes) error { + p.RefreshAttributeChecks = map[string]func(entry *ldap.Entry, attributes provider.RefreshAttributes) error{ + "some-attribute-to-check-during-refresh": func(entry *ldap.Entry, attributes provider.RefreshAttributes) error { return nil }, } @@ -1167,7 +1167,7 @@ func TestEndUserAuthentication(t *testing.T) { ldapProvider := New(*tt.providerConfig) - authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password) + authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password, []string{"groups"}) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != "": @@ -1199,7 +1199,7 @@ func TestEndUserAuthentication(t *testing.T) { } // Skip tt.bindEndUserMocks since DryRunAuthenticateUser() never binds as the end user. - authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username) + authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username, []string{"groups"}) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != "": @@ -1318,7 +1318,7 @@ func TestUpstreamRefresh(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupSearchGroupNameAttribute, }, - RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.StoredRefreshAttributes) error{ + RefreshAttributeChecks: map[string]func(*ldap.Entry, provider.RefreshAttributes) error{ pwdLastSetAttribute: AttributeUnchangedSinceLogin(pwdLastSetAttribute), }, } @@ -1772,11 +1772,12 @@ func TestUpstreamRefresh(t *testing.T) { initialPwdLastSetEncoded := base64.RawURLEncoding.EncodeToString([]byte("132801740800000000")) ldapProvider := New(*tt.providerConfig) subject := "ldaps://ldap.example.com:8443?base=some-upstream-user-base-dn&sub=c29tZS11cHN0cmVhbS11aWQtdmFsdWU" - groups, err := ldapProvider.PerformRefresh(context.Background(), provider.StoredRefreshAttributes{ + groups, err := ldapProvider.PerformRefresh(context.Background(), provider.RefreshAttributes{ Username: testUserSearchResultUsernameAttributeValue, Subject: subject, DN: tt.refreshUserDN, AdditionalAttributes: map[string]string{pwdLastSetAttribute: initialPwdLastSetEncoded}, + GrantedScopes: []string{"groups"}, }) if tt.wantErr != "" { require.Error(t, err) @@ -2149,7 +2150,7 @@ func TestAttributeUnchangedSinceLogin(t *testing.T) { tt := test t.Run(tt.name, func(t *testing.T) { initialValRawEncoded := base64.RawURLEncoding.EncodeToString([]byte(initialVal)) - err := AttributeUnchangedSinceLogin(attributeName)(tt.entry, provider.StoredRefreshAttributes{AdditionalAttributes: map[string]string{attributeName: initialValRawEncoded}}) + err := AttributeUnchangedSinceLogin(attributeName)(tt.entry, provider.RefreshAttributes{AdditionalAttributes: map[string]string{attributeName: initialValRawEncoded}}) if tt.wantErr != "" { require.Error(t, err) require.Equal(t, tt.wantErr, err.Error()) diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 584b68f5..3541dfa0 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -73,6 +73,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { name string username string password string + grantedScopes []string provider *upstreamldap.Provider wantError string wantAuthResponse *authenticators.Response @@ -114,6 +115,18 @@ func TestLDAPSearch_Parallel(t *testing.T) { ExtraRefreshAttributes: map[string]string{}, }, }, + { + name: "groups scope not in granted scopes", + username: "pinny", + password: pinnyPassword, + grantedScopes: []string{}, + provider: upstreamldap.New(*providerConfig(nil)), + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: nil}, + DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", + ExtraRefreshAttributes: map[string]string{}, + }, + }, { name: "when the user search filter is already wrapped by parenthesis", username: "pinny", @@ -636,7 +649,10 @@ func TestLDAPSearch_Parallel(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password) + if tt.grantedScopes == nil { + tt.grantedScopes = []string{"groups"} + } + authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password, tt.grantedScopes) switch { case tt.wantError != "": @@ -694,9 +710,7 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) { authUserCtx, authUserCtxCancelFunc := context.WithTimeout(context.Background(), 2*time.Minute) defer authUserCtxCancelFunc() - authResponse, authenticated, err := provider.AuthenticateUser(authUserCtx, - env.SupervisorUpstreamLDAP.TestUserCN, env.SupervisorUpstreamLDAP.TestUserPassword, - ) + authResponse, authenticated, err := provider.AuthenticateUser(authUserCtx, env.SupervisorUpstreamLDAP.TestUserCN, env.SupervisorUpstreamLDAP.TestUserPassword, []string{"groups"}) resultCh <- authUserResult{ response: authResponse, authenticated: authenticated, From dac03956807a68f9caabffe71392036aefa4b450 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 22 Jun 2022 14:19:55 -0700 Subject: [PATCH 5/6] Add a couple tests, address pr comments Signed-off-by: Margo Crawford --- internal/oidc/token/token_handler.go | 11 ++++-- internal/upstreamldap/upstreamldap.go | 6 +-- internal/upstreamldap/upstreamldap_test.go | 45 ++++++++++++++++++++-- 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 76727a12..c0044fc5 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -303,9 +303,12 @@ func upstreamLDAPRefresh(ctx context.Context, providerCache oidc.UpstreamIdentit return err } subject := session.Fosite.Claims.Subject - oldGroups, err := getDownstreamGroupsFromPinnipedSession(session) - if err != nil { - return err + var oldGroups []string + if slices.Contains(grantedScopes, oidc.DownstreamGroupsScope) { + oldGroups, err = getDownstreamGroupsFromPinnipedSession(session) + if err != nil { + return err + } } s := session.Custom @@ -410,7 +413,7 @@ func getDownstreamGroupsFromPinnipedSession(session *psession.PinnipedSession) ( } downstreamGroupsInterface := extra[oidc.DownstreamGroupsClaim] if downstreamGroupsInterface == nil { - return nil, nil + return nil, errorsx.WithStack(errMissingUpstreamSessionInternalError()) } downstreamGroupsInterfaceList, ok := downstreamGroupsInterface.([]interface{}) if !ok { diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 7317d939..cfbd437f 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -16,19 +16,17 @@ import ( "strings" "time" - "go.pinniped.dev/internal/oidc" - - "k8s.io/utils/strings/slices" - "github.com/go-ldap/ldap/v3" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/utils/strings/slices" "k8s.io/utils/trace" "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/endpointaddr" + "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/downstreamsession" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 9a9ca782..892c9f25 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -174,6 +174,7 @@ func TestEndUserAuthentication(t *testing.T) { name string username string password string + grantedScopes []string providerConfig *ProviderConfig searchMocks func(conn *mockldapconn.MockConn) bindEndUserMocks func(conn *mockldapconn.MockConn) @@ -286,6 +287,25 @@ func TestEndUserAuthentication(t *testing.T) { info.Groups = []string{} }), }, + { + name: "when groups scope isn't granted, don't do group search", + username: testUpstreamUsername, + password: testUpstreamPassword, + grantedScopes: []string{}, + providerConfig: providerConfig(nil), + searchMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + bindEndUserMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) + }, + wantAuthResponse: expectedAuthResponse(func(r *authenticators.Response) { + info := r.User.(*user.DefaultInfo) + info.Groups = nil + }), + }, { name: "when the UsernameAttribute is dn and there is a user search filter provided", username: testUpstreamUsername, @@ -1167,7 +1187,11 @@ func TestEndUserAuthentication(t *testing.T) { ldapProvider := New(*tt.providerConfig) - authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password, []string{"groups"}) + if tt.grantedScopes == nil { + tt.grantedScopes = []string{"groups"} + } + + authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password, tt.grantedScopes) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != "": @@ -1199,7 +1223,7 @@ func TestEndUserAuthentication(t *testing.T) { } // Skip tt.bindEndUserMocks since DryRunAuthenticateUser() never binds as the end user. - authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username, []string{"groups"}) + authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username, tt.grantedScopes) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != "": @@ -1331,6 +1355,7 @@ func TestUpstreamRefresh(t *testing.T) { tests := []struct { name string providerConfig *ProviderConfig + grantedScopes []string setupMocks func(conn *mockldapconn.MockConn) refreshUserDN string dialError error @@ -1465,6 +1490,17 @@ func TestUpstreamRefresh(t *testing.T) { }, wantGroups: nil, // do not update groups }, + { + name: "happy path where group search is configured but groups scope isn't included", + providerConfig: providerConfig(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch(nil)).Return(happyPathUserSearchResult, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + grantedScopes: []string{}, + wantGroups: nil, + }, { name: "error where dial fails", providerConfig: providerConfig(nil), @@ -1769,6 +1805,9 @@ func TestUpstreamRefresh(t *testing.T) { tt.refreshUserDN = testUserSearchResultDNValue // default for all tests } + if tt.grantedScopes == nil { + tt.grantedScopes = []string{"groups"} + } initialPwdLastSetEncoded := base64.RawURLEncoding.EncodeToString([]byte("132801740800000000")) ldapProvider := New(*tt.providerConfig) subject := "ldaps://ldap.example.com:8443?base=some-upstream-user-base-dn&sub=c29tZS11cHN0cmVhbS11aWQtdmFsdWU" @@ -1777,7 +1816,7 @@ func TestUpstreamRefresh(t *testing.T) { Subject: subject, DN: tt.refreshUserDN, AdditionalAttributes: map[string]string{pwdLastSetAttribute: initialPwdLastSetEncoded}, - GrantedScopes: []string{"groups"}, + GrantedScopes: tt.grantedScopes, }) if tt.wantErr != "" { require.Error(t, err) From 8adc1ce345f7b9b518f1c22a6b63c42e678b9ac7 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 22 Jun 2022 16:16:32 -0700 Subject: [PATCH 6/6] Fix failing active directory integration test Signed-off-by: Margo Crawford --- test/integration/supervisor_warnings_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/integration/supervisor_warnings_test.go b/test/integration/supervisor_warnings_test.go index 27c58179..04f77b25 100644 --- a/test/integration/supervisor_warnings_test.go +++ b/test/integration/supervisor_warnings_test.go @@ -263,6 +263,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { "--concierge-authenticator-name", authenticator.Name, "--oidc-session-cache", sessionCachePath, "--credential-cache", credentialCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", }) // Run "kubectl get namespaces" which should trigger a cli-based login. @@ -406,6 +407,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) { "--oidc-skip-listen", "--oidc-ca-bundle", testCABundlePath, "--oidc-session-cache", sessionCachePath, + "--oidc-scopes", "offline_access,openid,pinniped:request-audience,groups", "--credential-cache", credentialCachePath, })