Use an interface instead of a concrete type for UpstreamOIDCIdentityProvider

Because we want it to implement an AuthcodeExchanger interface and
do it in a way that will be more unit test-friendly than the underlying
library that we intend to use inside its implementation.

internal/oidc/auth/auth_handler.go

@@ -88,12 +88,12 @@ func NewHandler(
 		}
 
 		upstreamOAuthConfig := oauth2.Config{
-			ClientID: upstreamIDP.ClientID,
+			ClientID: upstreamIDP.GetClientID(),
 			Endpoint: oauth2.Endpoint{
-				AuthURL: upstreamIDP.AuthorizationURL.String(),
+				AuthURL: upstreamIDP.GetAuthorizationURL().String(),
 			},
-			RedirectURL: fmt.Sprintf("%s/callback/%s", issuer, upstreamIDP.Name),
-			Scopes:      upstreamIDP.Scopes,
+			RedirectURL: fmt.Sprintf("%s/callback/%s", issuer, upstreamIDP.GetName()),
+			Scopes:      upstreamIDP.GetScopes(),
 		}
 
 		encodedStateParamValue, err := upstreamStateParam(authorizeRequester, nonceValue, csrfValue, pkceValue, upstreamStateEncoder)
@@ -150,7 +150,7 @@ func grantOpenIDScopeIfRequested(authorizeRequester fosite.AuthorizeRequester) {
 	}
 }
 
-func chooseUpstreamIDP(idpListGetter oidc.IDPListGetter) (*provider.UpstreamOIDCIdentityProvider, error) {
+func chooseUpstreamIDP(idpListGetter oidc.IDPListGetter) (provider.UpstreamOIDCIdentityProviderI, error) {
 	allUpstreamIDPs := idpListGetter.GetIDPList()
 	if len(allUpstreamIDPs) == 0 {
 		return nil, httperr.New(
@@ -163,7 +163,7 @@ func chooseUpstreamIDP(idpListGetter oidc.IDPListGetter) (*provider.UpstreamOIDC
 			"Too many upstream providers are configured (support for multiple upstreams is not yet implemented)",
 		)
 	}
-	return &allUpstreamIDPs[0], nil
+	return allUpstreamIDPs[0], nil
 }
 
 func generateValues(

internal/oidc/auth/auth_handler_test.go

@@ -113,7 +113,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 	upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth")
 	require.NoError(t, err)
 
-	upstreamOIDCIdentityProvider := provider.UpstreamOIDCIdentityProvider{
+	upstreamOIDCIdentityProvider := testutil.TestUpstreamOIDCIdentityProvider{
 		Name:             "some-idp",
 		ClientID:         "some-client-id",
 		AuthorizationURL: *upstreamAuthURL,
@@ -122,7 +122,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 
 	issuer := "https://my-issuer.com/some-path"
 
-	// Configure fosite the same way that the production code would, except use in-memory storage.
+	// Configure fosite the same way that the production code would, using NullStorage to turn off storage.
 	oauthStore := oidc.NullStorage{}
 	hmacSecret := []byte("some secret - must have at least 32 bytes")
 	require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes")
@@ -771,13 +771,13 @@ func TestAuthorizationEndpoint(t *testing.T) {
 		runOneTestCase(t, test, subject)
 
 		// Call the setter to change the upstream IDP settings.
-		newProviderSettings := provider.UpstreamOIDCIdentityProvider{
+		newProviderSettings := testutil.TestUpstreamOIDCIdentityProvider{
 			Name:             "some-other-idp",
 			ClientID:         "some-other-client-id",
 			AuthorizationURL: *upstreamAuthURL,
 			Scopes:           []string{"other-scope1", "other-scope2"},
 		}
-		test.idpListGetter.SetIDPList([]provider.UpstreamOIDCIdentityProvider{newProviderSettings})
+		test.idpListGetter.SetIDPList([]provider.UpstreamOIDCIdentityProviderI{provider.UpstreamOIDCIdentityProviderI(&newProviderSettings)})
 
 		// Update the expectations of the test case to match the new upstream IDP settings.
 		test.wantLocationHeader = urlWithQuery(upstreamAuthURL.String(),

internal/oidc/callback/callback_handler.go

@@ -10,6 +10,8 @@ import (
 	"net/url"
 	"path"
 
+	"github.com/ory/fosite"
+
 	"go.pinniped.dev/internal/httputil/httperr"
 	"go.pinniped.dev/internal/oidc"
 	"go.pinniped.dev/internal/oidc/csrftoken"
@@ -17,10 +19,7 @@ import (
 	"go.pinniped.dev/internal/plog"
 )
 
-func NewHandler(
-	idpListGetter oidc.IDPListGetter,
-	stateDecoder, cookieDecoder oidc.Decoder,
-) http.Handler {
+func NewHandler(idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provider, stateDecoder, cookieDecoder oidc.Decoder) http.Handler {
 	return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
 		state, err := validateRequest(r, stateDecoder, cookieDecoder)
 		if err != nil {
@@ -84,10 +83,10 @@ func validateRequest(r *http.Request, stateDecoder, cookieDecoder oidc.Decoder)
 	return state, nil
 }
 
-func findUpstreamIDPConfig(r *http.Request, idpListGetter oidc.IDPListGetter) *provider.UpstreamOIDCIdentityProvider {
+func findUpstreamIDPConfig(r *http.Request, idpListGetter oidc.IDPListGetter) *provider.UpstreamOIDCIdentityProviderI {
 	_, lastPathComponent := path.Split(r.URL.Path)
 	for _, p := range idpListGetter.GetIDPList() {
-		if p.Name == lastPathComponent {
+		if p.GetName() == lastPathComponent {
 			return &p
 		}
 	}

internal/oidc/callback/callback_handler_test.go

@@ -13,8 +13,11 @@ import (
 	"testing"
 
 	"github.com/gorilla/securecookie"
+	"github.com/ory/fosite"
+	"github.com/ory/fosite/storage"
 	"github.com/stretchr/testify/require"
 
+	"go.pinniped.dev/internal/oidc"
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/testutil"
 )
@@ -28,23 +31,41 @@ func TestCallbackEndpoint(t *testing.T) {
 		downstreamRedirectURI = "http://127.0.0.1/callback"
 	)
 
-	upstreamAuthURL, err := url.Parse("https://some-upstream-idp:8443/auth")
-	require.NoError(t, err)
-	otherUpstreamAuthURL, err := url.Parse("https://some-other-upstream-idp:8443/auth")
-	require.NoError(t, err)
+	// TODO use a fosite memory store and pass in a fostite oauthHelper
+	// TODO write a test double for UpstreamOIDCIdentityProviderI ID token with a claim called "the-user-claim" and put a username as the value of that claim
+	// TODO assert that after the callback request, the fosite storage has 1 authcode key saved,
+	//   and it is the same key that was returned in the redirect,
+	//   and the value in storage includes the username in the fosite session
+	// TODO do the same thing with the groups list (store it in the fosite session as JWT claim)
+	// TODO test for when UpstreamOIDCIdentityProviderI authcode exchange fails
+	// TODO wire in the callback endpoint into the oidc manager request router
+	// TODO update the upstream watcher controller to also populate the new fields
+	// TODO update the integration test
+	// TODO DO NOT store the upstream tokens (or maybe just the refresh token) for this story. In a future story, we can store them/it in some other storage interface indexed by the same authcode hash that fosite used for storage.
+	// TODO grab the upstream config name from the state param instead of the URL path
 
-	upstreamOIDCIdentityProvider := provider.UpstreamOIDCIdentityProvider{
-		Name:             happyUpstreamIDPName,
-		ClientID:         "some-client-id",
-		AuthorizationURL: *upstreamAuthURL,
-		Scopes:           []string{"scope1", "scope2"},
+	// Configure fosite the same way that the production code would, except use in-memory storage.
+	oauthStore := &storage.MemoryStore{
+		Clients:        map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()},
+		AuthorizeCodes: map[string]storage.StoreAuthorizeCode{},
+		PKCES:          map[string]fosite.Requester{},
+		IDSessions:     map[string]fosite.Requester{},
+	}
+	hmacSecret := []byte("some secret - must have at least 32 bytes")
+	require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes")
+	oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret)
+
+	upstreamOIDCIdentityProvider := testutil.TestUpstreamOIDCIdentityProvider{
+		Name:          happyUpstreamIDPName,
+		ClientID:      "some-client-id",
+		UsernameClaim: "the-user-claim",
+		Scopes:        []string{"scope1", "scope2"},
 	}
 
-	otherUpstreamOIDCIdentityProvider := provider.UpstreamOIDCIdentityProvider{
-		Name:             "other-upstream-idp-name",
-		ClientID:         "other-some-client-id",
-		AuthorizationURL: *otherUpstreamAuthURL,
-		Scopes:           []string{"other-scope1", "other-scope2"},
+	otherUpstreamOIDCIdentityProvider := testutil.TestUpstreamOIDCIdentityProvider{
+		Name:     "other-upstream-idp-name",
+		ClientID: "other-some-client-id",
+		Scopes:   []string{"other-scope1", "other-scope2"},
 	}
 
 	var stateEncoderHashKey = []byte("fake-hash-secret")
@@ -61,7 +82,7 @@ func TestCallbackEndpoint(t *testing.T) {
 
 	happyDownstreamState := "some-downstream-state"
 
-	happyOrignalRequestParams := url.Values{
+	happyOriginalRequestParams := url.Values{
 		"response_type":         []string{"code"},
 		"scope":                 []string{"openid profile email"},
 		"client_id":             []string{"pinniped-cli"},
@@ -77,7 +98,7 @@ func TestCallbackEndpoint(t *testing.T) {
 
 	happyState, err := happyStateCodec.Encode("s",
 		testutil.ExpectedUpstreamStateParamFormat{
-			P: happyOrignalRequestParams,
+			P: happyOriginalRequestParams,
 			N: happyNonce,
 			C: happyCSRF,
 			K: happyPKCE,
@@ -88,7 +109,7 @@ func TestCallbackEndpoint(t *testing.T) {
 
 	wrongCSRFValueState, err := happyStateCodec.Encode("s",
 		testutil.ExpectedUpstreamStateParamFormat{
-			P: happyOrignalRequestParams,
+			P: happyOriginalRequestParams,
 			N: happyNonce,
 			C: "wrong-csrf-value",
 			K: happyPKCE,
@@ -99,7 +120,7 @@ func TestCallbackEndpoint(t *testing.T) {
 
 	wrongVersionState, err := happyStateCodec.Encode("s",
 		testutil.ExpectedUpstreamStateParamFormat{
-			P: happyOrignalRequestParams,
+			P: happyOriginalRequestParams,
 			N: happyNonce,
 			C: happyCSRF,
 			K: happyPKCE,
@@ -260,7 +281,7 @@ func TestCallbackEndpoint(t *testing.T) {
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
-			subject := NewHandler(test.idpListGetter, happyStateCodec, happyCookieCodec)
+			subject := NewHandler(test.idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec)
 			req := httptest.NewRequest(test.method, test.path, nil)
 			if test.csrfCookie != "" {
 				req.Header.Set("Cookie", test.csrfCookie)
@@ -285,7 +306,7 @@ func TestCallbackEndpoint(t *testing.T) {
 				capturedAuthCode := submatches[1]
 				_ = capturedAuthCode
 
-				// Assert capturedAuthCode storage stuff...
+				// TODO Assert capturedAuthCode storage stuff...
 
 				// Assert that body contains anchor tag with redirect location.
 				anchorTagWithLocationHref := fmt.Sprintf("<a href=\"%s\">Found</a>.\n\n", html.EscapeString(actualLocation))

internal/oidc/oidc.go

@@ -106,5 +106,5 @@ func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLengthAtLeast32 []by
 }
 
 type IDPListGetter interface {
-	GetIDPList() []provider.UpstreamOIDCIdentityProvider
+	GetIDPList() []provider.UpstreamOIDCIdentityProviderI
 }

internal/oidc/provider/dynamic_upstream_idp_provider.go

@@ -4,48 +4,114 @@
 package provider
 
 import (
+	"context"
 	"net/url"
 	"sync"
+
+	"go.pinniped.dev/internal/oidcclient"
+	"go.pinniped.dev/internal/oidcclient/nonce"
+	"go.pinniped.dev/internal/oidcclient/pkce"
 )
 
-type UpstreamOIDCIdentityProvider struct {
+type UpstreamOIDCIdentityProviderI interface {
 	// A name for this upstream provider, which will be used as a component of the path for the callback endpoint
 	// hosted by the Supervisor.
-	Name string
+	GetName() string
 
-	// The Oauth client ID registered with the upstream provider to be used in the authorization flow.
-	ClientID string
+	// The Oauth client ID registered with the upstream provider to be used in the authorization code flow.
+	GetClientID() string
 
 	// The Authorization Endpoint fetched from discovery.
-	AuthorizationURL url.URL
+	GetAuthorizationURL() *url.URL
 
 	// Scopes to request in authorization flow.
-	Scopes []string
+	GetScopes() []string
+
+	// ID Token username claim name. May return empty string, in which case we will use some reasonable defaults.
+	GetUsernameClaim() string
+
+	// ID Token groups claim name. May return empty string, in which case we won't try to read groups from the upstream provider.
+	GetGroupsClaim() string
+
+	AuthcodeExchanger
+}
+
+// Performs upstream OIDC authorization code exchange and token validation.
+// Returns the validated raw tokens as well as the parsed claims of the ID token.
+type AuthcodeExchanger interface {
+	ExchangeAuthcodeAndValidateTokens(
+		ctx context.Context,
+		authcode string,
+		pkceCodeVerifier pkce.Code,
+		expectedIDTokenNonce nonce.Nonce,
+	) (tokens oidcclient.Token, parsedIDTokenClaims map[string]interface{}, err error)
+}
+
+type UpstreamOIDCIdentityProvider struct {
+	Name             string
+	ClientID         string
+	AuthorizationURL url.URL
+	UsernameClaim    string
+	GroupsClaim      string
+	Scopes           []string
+}
+
+func (u *UpstreamOIDCIdentityProvider) GetName() string {
+	return u.Name
+}
+
+func (u *UpstreamOIDCIdentityProvider) GetClientID() string {
+	return u.ClientID
+}
+
+func (u *UpstreamOIDCIdentityProvider) GetAuthorizationURL() *url.URL {
+	return &u.AuthorizationURL
+}
+
+func (u *UpstreamOIDCIdentityProvider) GetScopes() []string {
+	return u.Scopes
+}
+
+func (u *UpstreamOIDCIdentityProvider) GetUsernameClaim() string {
+	return u.UsernameClaim
+}
+
+func (u *UpstreamOIDCIdentityProvider) GetGroupsClaim() string {
+	return u.GroupsClaim
+}
+
+func (u *UpstreamOIDCIdentityProvider) ExchangeAuthcodeAndValidateTokens(
+	ctx context.Context,
+	authcode string,
+	pkceCodeVerifier pkce.Code,
+	expectedIDTokenNonce nonce.Nonce,
+) (oidcclient.Token, map[string]interface{}, error) {
+	panic("TODO implement me") // TODO
 }
 
 type DynamicUpstreamIDPProvider interface {
-	SetIDPList(oidcIDPs []UpstreamOIDCIdentityProvider)
-	GetIDPList() []UpstreamOIDCIdentityProvider
+	SetIDPList(oidcIDPs []UpstreamOIDCIdentityProviderI)
+	GetIDPList() []UpstreamOIDCIdentityProviderI
 }
 
 type dynamicUpstreamIDPProvider struct {
-	oidcProviders []UpstreamOIDCIdentityProvider
+	oidcProviders []UpstreamOIDCIdentityProviderI
 	mutex         sync.RWMutex
 }
 
 func NewDynamicUpstreamIDPProvider() DynamicUpstreamIDPProvider {
 	return &dynamicUpstreamIDPProvider{
-		oidcProviders: []UpstreamOIDCIdentityProvider{},
+		oidcProviders: []UpstreamOIDCIdentityProviderI{},
 	}
 }
 
-func (p *dynamicUpstreamIDPProvider) SetIDPList(oidcIDPs []UpstreamOIDCIdentityProvider) {
+func (p *dynamicUpstreamIDPProvider) SetIDPList(oidcIDPs []UpstreamOIDCIdentityProviderI) {
 	p.mutex.Lock() // acquire a write lock
 	defer p.mutex.Unlock()
 	p.oidcProviders = oidcIDPs
 }
 
-func (p *dynamicUpstreamIDPProvider) GetIDPList() []UpstreamOIDCIdentityProvider {
+func (p *dynamicUpstreamIDPProvider) GetIDPList() []UpstreamOIDCIdentityProviderI {
 	p.mutex.RLock() // acquire a read lock
 	defer p.mutex.RUnlock()
 	return p.oidcProviders

internal/oidc/provider/manager/manager_test.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 	"testing"
 
+	"go.pinniped.dev/internal/testutil"
+
 	"github.com/sclevine/spec"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/square/go-jose.v2"
@@ -107,14 +109,11 @@ func TestManager(t *testing.T) {
 
 			parsedUpstreamIDPAuthorizationURL, err := url.Parse(upstreamIDPAuthorizationURL)
 			r.NoError(err)
-			idpListGetter := provider.NewDynamicUpstreamIDPProvider()
-			idpListGetter.SetIDPList([]provider.UpstreamOIDCIdentityProvider{
-				{
-					Name:             "test-idp",
-					ClientID:         "test-client-id",
-					AuthorizationURL: *parsedUpstreamIDPAuthorizationURL,
-					Scopes:           []string{"test-scope"},
-				},
+			idpListGetter := testutil.NewIDPListGetter(testutil.TestUpstreamOIDCIdentityProvider{
+				Name:             "test-idp",
+				ClientID:         "test-client-id",
+				AuthorizationURL: *parsedUpstreamIDPAuthorizationURL,
+				Scopes:           []string{"test-scope"},
 			})
 
 			subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter)