Addressing code review changes

- changed to use custom authenticators.Response rather than the k8s one
  that doesn't include space for a DN
- Added more checking for correct idp type in token handler
- small style changes

Signed-off-by: Margo Crawford <margaretc@vmware.com>

internal/authenticators/authenticators.go

@@ -7,7 +7,7 @@ package authenticators
 import (
 	"context"
 
-	"k8s.io/apiserver/pkg/authentication/authenticator"
+	"k8s.io/apiserver/pkg/authentication/user"
 )
 
 // This interface is similar to the k8s token authenticator, but works with username/passwords instead
@@ -31,5 +31,10 @@ 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) (*authenticator.Response, bool, error)
+	AuthenticateUser(ctx context.Context, username, password string) (*Response, bool, error)
+}
+
+type Response struct {
+	User user.Info
+	DN   string
 }

internal/oidc/auth/auth_handler.go

@@ -15,9 +15,9 @@ import (
 	"github.com/ory/fosite/token/jwt"
 	"github.com/pkg/errors"
 	"golang.org/x/oauth2"
-	"k8s.io/apiserver/pkg/authentication/authenticator"
 
 	supervisoroidc "go.pinniped.dev/generated/latest/apis/supervisor/oidc"
+	"go.pinniped.dev/internal/authenticators"
 	"go.pinniped.dev/internal/httputil/httperr"
 	"go.pinniped.dev/internal/httputil/securityheader"
 	"go.pinniped.dev/internal/oidc"
@@ -112,7 +112,7 @@ func handleAuthRequestForLDAPUpstream(
 	subject := downstreamSubjectFromUpstreamLDAP(ldapUpstream, authenticateResponse)
 	username = authenticateResponse.User.GetName()
 	groups := authenticateResponse.User.GetGroups()
-	dn := userDNFromAuthenticatedResponse(authenticateResponse)
+	dn := authenticateResponse.DN
 
 	customSessionData := &psession.CustomSessionData{
 		ProviderUID:  ldapUpstream.GetResourceUID(),
@@ -482,16 +482,7 @@ func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken
 	return nil
 }
 
-func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticator.Response) string {
+func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticators.Response) string {
 	ldapURL := *ldapUpstream.GetURL()
 	return downstreamsession.DownstreamLDAPSubject(authenticateResponse.User.GetUID(), ldapURL)
 }
-
-func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string {
-	// 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 ""
-	}
-	return dnSlice[0]
-}

internal/oidc/auth/auth_handler_test.go

@@ -19,12 +19,12 @@ import (
 	"github.com/ory/fosite"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
-	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/client-go/kubernetes/fake"
 	v1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/utils/pointer"
 
+	"go.pinniped.dev/internal/authenticators"
 	"go.pinniped.dev/internal/here"
 	"go.pinniped.dev/internal/oidc"
 	"go.pinniped.dev/internal/oidc/csrftoken"
@@ -273,18 +273,18 @@ func TestAuthorizationEndpoint(t *testing.T) {
 	parsedUpstreamLDAPURL, err := url.Parse(upstreamLDAPURL)
 	require.NoError(t, err)
 
-	ldapAuthenticateFunc := func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) {
+	ldapAuthenticateFunc := func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) {
 		if username == "" || password == "" {
 			return nil, false, fmt.Errorf("should not have passed empty username or password to the authenticator")
 		}
 		if username == happyLDAPUsername && password == happyLDAPPassword {
-			return &authenticator.Response{
+			return &authenticators.Response{
 				User: &user.DefaultInfo{
 					Name:   happyLDAPUsernameFromAuthenticator,
 					UID:    happyLDAPUID,
 					Groups: happyLDAPGroups,
-					Extra:  map[string][]string{"userDN": {happyLDAPUserDN}},
 				},
+				DN: happyLDAPUserDN,
 			}, true, nil
 		}
 		return nil, false, nil
@@ -307,7 +307,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 	erroringUpstreamLDAPIdentityProvider := oidctestutil.TestUpstreamLDAPIdentityProvider{
 		Name:        ldapUpstreamName,
 		ResourceUID: ldapUpstreamResourceUID,
-		AuthenticateFunc: func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) {
+		AuthenticateFunc: func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) {
 			return nil, false, fmt.Errorf("some ldap upstream auth error")
 		},
 	}

internal/oidc/provider/dynamic_upstream_idp_provider.go

@@ -90,7 +90,7 @@ type UpstreamLDAPIdentityProviderI interface {
 	authenticators.UserAuthenticator
 
 	// PerformRefresh performs a refresh against the upstream LDAP identity provider
-	PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error
+	PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error
 }
 
 type DynamicUpstreamIDPProvider interface {

internal/oidc/token/token_handler.go

@@ -172,7 +172,9 @@ func findOIDCProviderByNameAndValidateUID(
 
 func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister, username string, subject string) error {
 	// if you have neither a valid ldap session config nor a valid active directory session config
-	if (s.LDAP == nil || s.LDAP.UserDN == "") && (s.ActiveDirectory == nil || s.ActiveDirectory.UserDN == "") {
+	validLDAP := s.ProviderType == psession.ProviderTypeLDAP && s.LDAP != nil && s.LDAP.UserDN != ""
+	validAD := s.ProviderType == psession.ProviderTypeActiveDirectory && s.ActiveDirectory != nil && s.ActiveDirectory.UserDN != ""
+	if !(validLDAP || validAD) {
 		return errorsx.WithStack(errMissingUpstreamSessionInternalError)
 	}
 
@@ -230,6 +232,9 @@ func getDownstreamUsernameFromPinnipedSession(session *psession.PinnipedSession)
 	if downstreamUsernameInterface == nil {
 		return "", errorsx.WithStack(errMissingUpstreamSessionInternalError)
 	}
-	downstreamUsername := downstreamUsernameInterface.(string)
+	downstreamUsername, ok := downstreamUsernameInterface.(string)
+	if !ok || len(downstreamUsername) == 0 {
+		return "", errorsx.WithStack(errMissingUpstreamSessionInternalError)
+	}
 	return downstreamUsername, nil
 }

internal/oidc/token/token_handler_test.go

@@ -1922,6 +1922,90 @@ func TestRefreshGrant(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "username in extra is not a string",
+			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{}{"username": 123},
+					},
+				}
+				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 in extra is an empty string",
+			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{}{"username": ""},
+					},
+				}
+				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: "when the ldap provider in the session storage is found but has the wrong resource UID during the refresh request",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{

internal/testutil/oidctestutil/oidctestutil.go

@@ -21,10 +21,10 @@ import (
 	"gopkg.in/square/go-jose.v2"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/client-go/kubernetes/fake"
 	v1 "k8s.io/client-go/kubernetes/typed/core/v1"
 
+	"go.pinniped.dev/internal/authenticators"
 	"go.pinniped.dev/internal/crud"
 	"go.pinniped.dev/internal/fositestorage/authorizationcode"
 	"go.pinniped.dev/internal/fositestorage/openidconnect"
@@ -80,7 +80,7 @@ type TestUpstreamLDAPIdentityProvider struct {
 	Name                    string
 	ResourceUID             types.UID
 	URL                     *url.URL
-	AuthenticateFunc        func(ctx context.Context, username, password string) (*authenticator.Response, bool, error)
+	AuthenticateFunc        func(ctx context.Context, username, password string) (*authenticators.Response, bool, error)
 	performRefreshCallCount int
 	performRefreshArgs      []*PerformRefreshArgs
 	PerformRefreshErr       error
@@ -96,7 +96,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetName() string {
 	return u.Name
 }
 
-func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) {
+func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) {
 	return u.AuthenticateFunc(ctx, username, password)
 }
 
@@ -104,7 +104,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL {
 	return u.URL
 }
 
-func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error {
+func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error {
 	if u.performRefreshArgs == nil {
 		u.performRefreshArgs = make([]*PerformRefreshArgs, 0)
 	}

internal/upstreamldap/upstreamldap.go

@@ -21,7 +21,6 @@ import (
 	"github.com/go-ldap/ldap/v3"
 	"github.com/google/uuid"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/utils/trace"
 
@@ -170,7 +169,7 @@ func (p *Provider) GetConfig() ProviderConfig {
 	return p.c
 }
 
-func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error {
+func (p *Provider) PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject 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
 	search := p.refreshUserSearchRequest(userDN)
@@ -372,7 +371,7 @@ 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) (*authenticator.Response, bool, error) {
+func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) (*authenticators.Response, bool, error) {
 	endUserBindFunc := func(conn Conn, foundUserDN string) error {
 		// Act as if the end user bind always succeeds.
 		return nil
@@ -381,14 +380,14 @@ func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string)
 }
 
 // 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) (*authenticator.Response, bool, error) {
+func (p *Provider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) {
 	endUserBindFunc := func(conn Conn, foundUserDN string) error {
 		return conn.Bind(foundUserDN, password)
 	}
 	return p.authenticateUserImpl(ctx, username, endUserBindFunc)
 }
 
-func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticator.Response, bool, error) {
+func (p *Provider) authenticateUserImpl(ctx context.Context, username 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
 
@@ -428,13 +427,13 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi
 		return nil, false, nil
 	}
 
-	response := &authenticator.Response{
+	response := &authenticators.Response{
 		User: &user.DefaultInfo{
 			Name:   mappedUsername,
 			UID:    mappedUID,
 			Groups: mappedGroupNames,
-			Extra:  map[string][]string{"userDN": {userDN}},
 		},
+		DN: userDN,
 	}
 	p.traceAuthSuccess(t)
 	return response, true, nil

internal/upstreamldap/upstreamldap_test.go

@@ -18,9 +18,9 @@ import (
 	"github.com/go-ldap/ldap/v3"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
-	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
 
+	"go.pinniped.dev/internal/authenticators"
 	"go.pinniped.dev/internal/certauthority"
 	"go.pinniped.dev/internal/endpointaddr"
 	"go.pinniped.dev/internal/mocks/mockldapconn"
@@ -151,17 +151,16 @@ func TestEndUserAuthentication(t *testing.T) {
 	}
 
 	// The auth response which matches the exampleUserSearchResult and exampleGroupSearchResult.
-	expectedAuthResponse := func(editFunc func(r *user.DefaultInfo)) *authenticator.Response {
+	expectedAuthResponse := func(editFunc func(r *user.DefaultInfo)) *authenticators.Response {
 		u := &user.DefaultInfo{
 			Name:   testUserSearchResultUsernameAttributeValue,
 			UID:    base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)),
 			Groups: []string{testGroupSearchResultGroupNameAttributeValue1, testGroupSearchResultGroupNameAttributeValue2},
-			Extra:  map[string][]string{"userDN": {testUserSearchResultDNValue}},
 		}
 		if editFunc != nil {
 			editFunc(u)
 		}
-		return &authenticator.Response{User: u}
+		return &authenticators.Response{User: u, DN: testUserSearchResultDNValue}
 	}
 
 	tests := []struct {
@@ -174,7 +173,7 @@ func TestEndUserAuthentication(t *testing.T) {
 		dialError                  error
 		wantError                  string
 		wantToSkipDial             bool
-		wantAuthResponse           *authenticator.Response
+		wantAuthResponse           *authenticators.Response
 		wantUnauthenticated        bool
 		skipDryRunAuthenticateUser bool // tests about when the end user bind fails don't make sense for DryRunAuthenticateUser()
 	}{
@@ -499,13 +498,13 @@ func TestEndUserAuthentication(t *testing.T) {
 			bindEndUserMocks: func(conn *mockldapconn.MockConn) {
 				conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1)
 			},
-			wantAuthResponse: &authenticator.Response{
+			wantAuthResponse: &authenticators.Response{
 				User: &user.DefaultInfo{
 					Name:   testUserSearchResultUsernameAttributeValue,
 					UID:    base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)),
 					Groups: []string{"a", "b", "c"},
-					Extra:  map[string][]string{"userDN": {testUserSearchResultDNValue}},
 				},
+				DN: testUserSearchResultDNValue,
 			},
 		},
 		{

test/integration/ldap_client_test.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
 
+	"go.pinniped.dev/internal/authenticators"
 	"go.pinniped.dev/internal/upstreamldap"
 	"go.pinniped.dev/test/testlib"
 )
@@ -677,7 +678,7 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) {
 }
 
 type authUserResult struct {
-	response      *authenticator.Response
+	response      *authenticators.Response
 	authenticated bool
 	err           error
 }