Updates to tests and some error assertions

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 ""

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
+	extra := session.Fosite.Claims.Extra
-	if fositeSession == nil {
+	if extra == nil {
-		return fmt.Errorf("fosite session not found")
+		return errorsx.WithStack(errMissingUpstreamSessionInternalError)
 	}
-	claims := fositeSession.Claims
+	downstreamUsernameInterface := extra["username"]
-	if claims == nil {
+	if downstreamUsernameInterface == nil {
-		return fmt.Errorf("fosite session claims not found")
+		return errorsx.WithStack(errMissingUpstreamSessionInternalError)
 	}
-	extra := claims.Extra
+	downstreamUsername := downstreamUsernameInterface.(string)
-	downstreamUsername := extra["username"].(string)
+	downstreamSubject := session.Fosite.Claims.Subject
-	downstreamSubject := claims.Subject
 
 	customSessionData := session.Custom
 	if customSessionData == nil {

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
+		name                      string
-		idps                           *oidctestutil.UpstreamIDPListerBuilder
+		idps                      *oidctestutil.UpstreamIDPListerBuilder
-		authcodeExchange               authcodeExchangeInputs
+		authcodeExchange          authcodeExchangeInputs
-		authEndpointInitialSessionData *psession.CustomSessionData
+		refreshRequest            refreshRequestInputs
-		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{
+				customSessionData: happyLDAPCustomSessionData,
-					ProviderUID:  ldapUpstreamResourceUID,
-					ProviderName: ldapUpstreamName,
-					ProviderType: ldapUpstreamType,
-					LDAP: &psession.LDAPSessionData{
-						UserDN: ldapUpstreamDN,
-					},
-				},
 				want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(
-					&psession.CustomSessionData{
+					happyLDAPCustomSessionData,
-						ProviderUID:  ldapUpstreamResourceUID,
-						ProviderName: ldapUpstreamName,
-						ProviderType: ldapUpstreamType,
-						LDAP: &psession.LDAPSessionData{
-							UserDN: ldapUpstreamDN,
-						},
-					},
 				),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: happyRefreshTokenResponseForLDAP(
-					&psession.CustomSessionData{
+					happyLDAPCustomSessionData,
-						ProviderUID:  ldapUpstreamResourceUID,
-						ProviderName: ldapUpstreamName,
-						ProviderType: ldapUpstreamType,
-						LDAP: &psession.LDAPSessionData{
-							UserDN: ldapUpstreamDN,
-						},
-					},
 				),
 			},
 		},
@@ -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{
+				customSessionData: happyActiveDirectoryCustomSessionData,
-					ProviderUID:  activeDirectoryUpstreamResourceUID,
-					ProviderName: activeDirectoryUpstreamName,
-					ProviderType: activeDirectoryUpstreamType,
-					ActiveDirectory: &psession.ActiveDirectorySessionData{
-						UserDN: activeDirectoryUpstreamDN,
-					},
-				},
 				want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(
-					&psession.CustomSessionData{
+					happyActiveDirectoryCustomSessionData,
-						ProviderUID:  activeDirectoryUpstreamResourceUID,
-						ProviderName: activeDirectoryUpstreamName,
-						ProviderType: activeDirectoryUpstreamType,
-						ActiveDirectory: &psession.ActiveDirectorySessionData{
-							UserDN: activeDirectoryUpstreamDN,
-						},
-					},
 				),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: happyRefreshTokenResponseForActiveDirectory(
-					&psession.CustomSessionData{
+					happyActiveDirectoryCustomSessionData,
-						ProviderUID:  activeDirectoryUpstreamResourceUID,
-						ProviderName: activeDirectoryUpstreamName,
-						ProviderType: activeDirectoryUpstreamType,
-						ActiveDirectory: &psession.ActiveDirectorySessionData{
-							UserDN: activeDirectoryUpstreamDN,
-						},
-					},
 				),
 			},
 		},
@@ -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{
+				customSessionData: happyLDAPCustomSessionData,
-					ProviderUID:  ldapUpstreamResourceUID,
-					ProviderName: ldapUpstreamName,
-					ProviderType: ldapUpstreamType,
-					LDAP: &psession.LDAPSessionData{
-						UserDN: ldapUpstreamDN,
-					},
-				},
 				want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(
-					&psession.CustomSessionData{
+					happyLDAPCustomSessionData,
-						ProviderUID:  ldapUpstreamResourceUID,
-						ProviderName: ldapUpstreamName,
-						ProviderType: ldapUpstreamType,
-						LDAP: &psession.LDAPSessionData{
-							UserDN: ldapUpstreamDN,
-						},
-					},
 				),
 			},
 			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{
+				customSessionData: happyActiveDirectoryCustomSessionData,
-					ProviderUID:  activeDirectoryUpstreamResourceUID,
-					ProviderName: activeDirectoryUpstreamName,
-					ProviderType: activeDirectoryUpstreamType,
-					ActiveDirectory: &psession.ActiveDirectorySessionData{
-						UserDN: activeDirectoryUpstreamDN,
-					},
-				},
 				want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(
-					&psession.CustomSessionData{
+					happyActiveDirectoryCustomSessionData,
-						ProviderUID:  activeDirectoryUpstreamResourceUID,
-						ProviderName: activeDirectoryUpstreamName,
-						ProviderType: activeDirectoryUpstreamType,
-						ActiveDirectory: &psession.ActiveDirectorySessionData{
-							UserDN: activeDirectoryUpstreamDN,
-						},
-					},
 				),
 			},
 			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{
+				customSessionData: happyLDAPCustomSessionData,
-					ProviderUID:  ldapUpstreamResourceUID,
-					ProviderName: ldapUpstreamName,
-					ProviderType: ldapUpstreamType,
-					LDAP: &psession.LDAPSessionData{
-						UserDN: ldapUpstreamDN,
-					},
-				},
 				want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(
-					&psession.CustomSessionData{
+					happyLDAPCustomSessionData,
-						ProviderUID:  ldapUpstreamResourceUID,
-						ProviderName: ldapUpstreamName,
-						ProviderType: ldapUpstreamType,
-						LDAP: &psession.LDAPSessionData{
-							UserDN: ldapUpstreamDN,
-						},
-					},
 				),
 			},
 			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{
+				customSessionData: happyActiveDirectoryCustomSessionData,
-					ProviderUID:  activeDirectoryUpstreamResourceUID,
-					ProviderName: activeDirectoryUpstreamName,
-					ProviderType: activeDirectoryUpstreamType,
-					ActiveDirectory: &psession.ActiveDirectorySessionData{
-						UserDN: activeDirectoryUpstreamDN,
-					},
-				},
 				want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(
-					&psession.CustomSessionData{
+					happyActiveDirectoryCustomSessionData,
-						ProviderUID:  activeDirectoryUpstreamResourceUID,
-						ProviderName: activeDirectoryUpstreamName,
-						ProviderType: activeDirectoryUpstreamType,
-						ActiveDirectory: &psession.ActiveDirectorySessionData{
-							UserDN: activeDirectoryUpstreamDN,
-						},
-					},
 				),
 			},
 			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)

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
+		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
+		Attributes:   p.userSearchRequestedAttributes(),
-		Controls:     nil,                               // this could be used to enable paging, but we're already limiting the result max size
+		Controls:     nil, // this could be used to enable paging, but we're already limiting the result max size
 	}
 }