diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index caff98f32..aaac49263 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -113,9 +113,6 @@ func handleAuthRequestForLDAPUpstream( username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() dn := userDNFromAuthenticatedResponse(authenticateResponse) - if dn == "" { - return httperr.New(http.StatusInternalServerError, "unexpected error during upstream authentication") - } customSessionData := &psession.CustomSessionData{ ProviderUID: ldapUpstream.GetResourceUID(), @@ -491,7 +488,7 @@ func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentit } func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string { - // This error shouldn't happen, but do some error checking anyway so it doesn't panic + // We should always have something here, but do some error checking anyway so it doesn't panic dnSlice := authenticatedResponse.User.GetExtra()["userDN"] if len(dnSlice) != 1 { return "" diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index fee06a637..d26b86424 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -6,7 +6,6 @@ package token import ( "context" - "fmt" "net/http" "github.com/ory/fosite" @@ -76,17 +75,16 @@ func NewHandler( func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, providerCache oidc.UpstreamIdentityProvidersLister) error { session := accessRequest.GetSession().(*psession.PinnipedSession) - fositeSession := session.Fosite - if fositeSession == nil { - return fmt.Errorf("fosite session not found") + extra := session.Fosite.Claims.Extra + if extra == nil { + return errorsx.WithStack(errMissingUpstreamSessionInternalError) } - claims := fositeSession.Claims - if claims == nil { - return fmt.Errorf("fosite session claims not found") + downstreamUsernameInterface := extra["username"] + if downstreamUsernameInterface == nil { + return errorsx.WithStack(errMissingUpstreamSessionInternalError) } - extra := claims.Extra - downstreamUsername := extra["username"].(string) - downstreamSubject := claims.Subject + downstreamUsername := downstreamUsernameInterface.(string) + downstreamSubject := session.Fosite.Claims.Subject customSessionData := session.Custom if customSessionData == nil { diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 50825b804..0cec17b62 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1020,12 +1020,28 @@ func TestRefreshGrant(t *testing.T) { return tokens } + happyActiveDirectoryCustomSessionData := &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + } + happyLDAPCustomSessionData := &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + } tests := []struct { - name string - idps *oidctestutil.UpstreamIDPListerBuilder - authcodeExchange authcodeExchangeInputs - authEndpointInitialSessionData *psession.CustomSessionData - refreshRequest refreshRequestInputs + name string + idps *oidctestutil.UpstreamIDPListerBuilder + authcodeExchange authcodeExchangeInputs + refreshRequest refreshRequestInputs + modifyRefreshTokenStorage func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) }{ { name: "happy path refresh grant with openid scope granted (id token returned)", @@ -1544,35 +1560,14 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForLDAP( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, }, @@ -1585,35 +1580,14 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForActiveDirectory( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, }, @@ -1779,23 +1753,9 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1821,23 +1781,9 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1858,23 +1804,9 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1894,23 +1826,9 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1925,6 +1843,85 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "fosite session is empty", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{} + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "username not found in extra field", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + //fositeSessionData: &openid.DefaultSession{}, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Extra: map[string]interface{}{}, + }, + } + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, } for _, test := range tests { test := test @@ -1952,6 +1949,10 @@ func TestRefreshGrant(t *testing.T) { // Send the refresh token back and preform a refresh. firstRefreshToken := parsedAuthcodeExchangeResponseBody["refresh_token"].(string) require.NotEmpty(t, firstRefreshToken) + + if test.modifyRefreshTokenStorage != nil { + test.modifyRefreshTokenStorage(t, oauthStore, firstRefreshToken) + } reqContext := context.WithValue(context.Background(), struct{ name string }{name: "test"}, "request-context") req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyRefreshRequestBody(firstRefreshToken).ReadCloser()).WithContext(reqContext) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index fad037119..33ca62f5a 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -641,9 +641,9 @@ func (p *Provider) refreshUserSearchRequest(dn string) *ldap.SearchRequest { SizeLimit: 2, TimeLimit: 90, TypesOnly: false, - Filter: "(objectClass=*)", // we already have the dn, so the filter doesn't matter - Attributes: p.userSearchRequestedAttributes(), // TODO this will need to include some other AD attributes - Controls: nil, // this could be used to enable paging, but we're already limiting the result max size + Filter: "(objectClass=*)", // we already have the dn, so the filter doesn't matter + Attributes: p.userSearchRequestedAttributes(), + Controls: nil, // this could be used to enable paging, but we're already limiting the result max size } }