From 29eb3dd384a60fe3bcbb3031c883d39f021f2dbe Mon Sep 17 00:00:00 2001
From: "Benjamin A. Petersen" <ben@benjaminapetersen.me>
Date: Tue, 30 Apr 2024 12:17:34 -0400
Subject: [PATCH 1/7] Update GitHub UpstreamAuthorizeRedirectURL to generate
 URLs

---
 .../resolved_github_provider.go               | 15 +++++++++--
 .../resolved_github_provider_test.go          | 25 +++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go
index 4b17120ef..5da2d7a4e 100644
--- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go
+++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go
@@ -8,6 +8,8 @@ import (
 	"errors"
 	"fmt"
 
+	"golang.org/x/oauth2"
+
 	"go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
 	"go.pinniped.dev/internal/federationdomain/resolvedprovider"
 	"go.pinniped.dev/internal/federationdomain/upstreamprovider"
@@ -68,8 +70,17 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedire
 	state *resolvedprovider.UpstreamAuthorizeRequestState,
 	downstreamIssuerURL string,
 ) (string, error) {
-	fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamAuthorizeRedirectURL() called with state: %#v, downstreamIssuerURL %s", state, downstreamIssuerURL)
-	return "", errors.New("function UpstreamAuthorizeRedirectURL not yet implemented for GitHub IDP")
+	upstreamOAuthConfig := oauth2.Config{
+		ClientID: p.Provider.GetClientID(),
+		Endpoint: oauth2.Endpoint{
+			AuthURL: p.Provider.GetAuthorizationURL(),
+		},
+		RedirectURL: fmt.Sprintf("%s/callback", downstreamIssuerURL),
+	}
+	redirectURL := upstreamOAuthConfig.AuthCodeURL(
+		state.EncodedStateParam,
+	)
+	return redirectURL, nil
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) Login(
diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
index cc017cc08..3f68f7fa5 100644
--- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
+++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
@@ -8,8 +8,10 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"golang.org/x/oauth2"
 
 	"go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
+	"go.pinniped.dev/internal/federationdomain/resolvedprovider"
 	"go.pinniped.dev/internal/idtransform"
 	"go.pinniped.dev/internal/psession"
 	"go.pinniped.dev/internal/upstreamgithub"
@@ -31,6 +33,11 @@ func TestFederationDomainResolvedGitHubIdentityProvider(t *testing.T) {
 		Provider: upstreamgithub.New(upstreamgithub.ProviderConfig{
 			Name:        "fake-provider-config",
 			ResourceUID: "fake-resource-uid",
+			OAuth2Config: &oauth2.Config{
+				ClientID:     "clientID12345",
+				ClientSecret: "clientSecret6789",
+				RedirectURL:  "some/redirect/url",
+			},
 		}),
 		SessionProviderType: psession.ProviderTypeGitHub,
 		Transforms:          transforms,
@@ -40,6 +47,11 @@ func TestFederationDomainResolvedGitHubIdentityProvider(t *testing.T) {
 	require.Equal(t, upstreamgithub.New(upstreamgithub.ProviderConfig{
 		Name:        "fake-provider-config",
 		ResourceUID: "fake-resource-uid",
+		OAuth2Config: &oauth2.Config{
+			ClientID:     "clientID12345",
+			ClientSecret: "clientSecret6789",
+			RedirectURL:  "some/redirect/url",
+		},
 	}), subject.GetProvider())
 	require.Equal(t, psession.ProviderTypeGitHub, subject.GetSessionProviderType())
 	require.Equal(t, v1alpha1.IDPTypeGitHub, subject.GetIDPDiscoveryType())
@@ -50,4 +62,17 @@ func TestFederationDomainResolvedGitHubIdentityProvider(t *testing.T) {
 		UpstreamUsername: "fake-upstream-username",
 		GitHub:           &psession.GitHubSessionData{},
 	}))
+	redirectURL, err := subject.UpstreamAuthorizeRedirectURL(
+		&resolvedprovider.UpstreamAuthorizeRequestState{
+			EncodedStateParam: "encodedStateParam12345",
+			PKCE:              "pkce6789",
+			Nonce:             "nonce1289",
+		},
+		"https://localhost/fake/path",
+	)
+	require.NoError(t, err)
+	require.Equal(t,
+		"?client_id=clientID12345&redirect_uri=https%3A%2F%2Flocalhost%2Ffake%2Fpath%2Fcallback&response_type=code&state=encodedStateParam12345",
+		redirectURL,
+	)
 }

From 7277d00e1add41b45beddd6f4c104703bdf33f09 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Wed, 8 May 2024 11:38:38 -0700
Subject: [PATCH 2/7] refactor upstreamgithub.ProviderConfig to hold more
 config

---
 .../github_upstream_watcher.go                |  42 +++-
 .../github_upstream_watcher_test.go           | 190 ++++++++++++++----
 .../resolved_github_provider.go               |  24 +--
 .../resolved_github_provider_test.go          |  97 +++++----
 .../upstreamprovider/upsteam_provider.go      |  32 +--
 .../authorizationcode/authorizationcode.go    |  11 +-
 internal/psession/pinniped_session.go         |   1 +
 internal/supervisor/server/server.go          |   1 +
 .../oidctestutil/testgithubprovider.go        |  41 +---
 internal/upstreamgithub/upstreamgithub.go     |  60 +++---
 .../upstreamgithub/upstreamgithub_test.go     |  35 ++--
 11 files changed, 341 insertions(+), 193 deletions(-)

diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go
index bfcbba5bb..c6f65ff98 100644
--- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go
+++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go
@@ -58,6 +58,9 @@ const (
 	ClientCredentialsObtained string = "ClientCredentialsObtained" //nolint:gosec // this is not a credential
 	GitHubConnectionValid     string = "GitHubConnectionValid"
 	ClaimsValid               string = "ClaimsValid"
+
+	defaultHost       = "github.com"
+	defaultApiBaseURL = "https://api.github.com"
 )
 
 // UpstreamGitHubIdentityProviderICache is a thread safe cache that holds a list of validated upstream GitHub IDP configurations.
@@ -73,6 +76,7 @@ type gitHubWatcherController struct {
 	gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer
 	secretInformer                 corev1informers.SecretInformer
 	clock                          clock.Clock
+	dialFunc                       func(network, addr string, config *tls.Config) (*tls.Conn, error)
 }
 
 // New instantiates a new controllerlib.Controller which will populate the provided UpstreamGitHubIdentityProviderICache.
@@ -85,6 +89,7 @@ func New(
 	log plog.Logger,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 	clock clock.Clock,
+	dialFunc func(network, addr string, config *tls.Config) (*tls.Conn, error),
 ) controllerlib.Controller {
 	c := gitHubWatcherController{
 		namespace:                      namespace,
@@ -94,6 +99,7 @@ func New(
 		gitHubIdentityProviderInformer: gitHubIdentityProviderInformer,
 		secretInformer:                 secretInformer,
 		clock:                          clock,
+		dialFunc:                       dialFunc,
 	}
 
 	return controllerlib.New(
@@ -202,7 +208,7 @@ func (c *gitHubWatcherController) validateClientSecret(secretName string) (*meta
 	}, clientID, clientSecret, nil
 }
 
-func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizationsSpec) (*metav1.Condition, v1alpha1.GitHubAllowedAuthOrganizationsPolicy) {
+func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizationsSpec) *metav1.Condition {
 	var policy v1alpha1.GitHubAllowedAuthOrganizationsPolicy
 	if organizationsSpec.Policy != nil {
 		policy = *organizationsSpec.Policy
@@ -217,7 +223,7 @@ func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizations
 			Status:  metav1.ConditionTrue,
 			Reason:  upstreamwatchers.ReasonSuccess,
 			Message: fmt.Sprintf("spec.allowAuthentication.organizations.policy (%q) is valid", policy),
-		}, policy
+		}
 	}
 
 	if len(organizationsSpec.Allowed) > 0 {
@@ -226,7 +232,7 @@ func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizations
 			Status:  metav1.ConditionFalse,
 			Reason:  "Invalid",
 			Message: "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed",
-		}, policy
+		}
 	}
 
 	return &metav1.Condition{
@@ -234,7 +240,7 @@ func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizations
 		Status:  metav1.ConditionFalse,
 		Reason:  "Invalid",
 		Message: "spec.allowAuthentication.organizations.policy must be 'AllGitHubUsers' when spec.allowAuthentication.organizations.allowed is empty",
-	}, policy
+	}
 }
 
 func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) (
@@ -256,7 +262,7 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro
 	userAndGroupCondition, groupNameAttribute, usernameAttribute := validateUserAndGroupAttributes(upstream)
 	conditions = append(conditions, userAndGroupCondition)
 
-	organizationPolicyCondition, policy := validateOrganizationsPolicy(&upstream.Spec.AllowAuthentication.Organizations)
+	organizationPolicyCondition := validateOrganizationsPolicy(&upstream.Spec.AllowAuthentication.Organizations)
 	conditions = append(conditions, organizationPolicyCondition)
 
 	hostCondition, hostPort := validateHost(upstream.Spec.GitHubAPI)
@@ -295,22 +301,36 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro
 		upstreamgithub.ProviderConfig{
 			Name:               upstream.Name,
 			ResourceUID:        upstream.UID,
-			Host:               hostURL,
+			APIBaseURL:         apiBaseUrl(*upstream.Spec.GitHubAPI.Host, hostURL),
 			GroupNameAttribute: groupNameAttribute,
 			UsernameAttribute:  usernameAttribute,
 			OAuth2Config: &oauth2.Config{
 				ClientID:     clientID,
 				ClientSecret: clientSecret,
+				// See https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
+				Endpoint: oauth2.Endpoint{
+					AuthURL:       fmt.Sprintf("%s/login/oauth/authorize", hostURL),
+					DeviceAuthURL: "", // we do not use device code flow
+					TokenURL:      fmt.Sprintf("%s/login/oauth/access_token", hostURL),
+					AuthStyle:     oauth2.AuthStyleInParams,
+				},
+				RedirectURL: "", // this will be different for each FederationDomain, so we do not set it here
+				Scopes:      []string{"read:user", "read:org"},
 			},
-			AllowedOrganizations:    upstream.Spec.AllowAuthentication.Organizations.Allowed,
-			OrganizationLoginPolicy: policy,
-			AuthorizationURL:        fmt.Sprintf("%s/login/oauth/authorize", hostURL),
-			HttpClient:              httpClient,
+			AllowedOrganizations: upstream.Spec.AllowAuthentication.Organizations.Allowed,
+			HttpClient:           httpClient,
 		},
 	)
 	return provider, k8sutilerrors.NewAggregate(applicationErrors)
 }
 
+func apiBaseUrl(upstreamSpecHost string, hostURL string) string {
+	if upstreamSpecHost != defaultHost {
+		return fmt.Sprintf("%s/api/v3", hostURL)
+	}
+	return defaultApiBaseURL
+}
+
 func validateHost(gitHubAPIConfig v1alpha1.GitHubAPIConfig) (*metav1.Condition, *endpointaddr.HostPort) {
 	buildInvalidHost := func(host, reason string) *metav1.Condition {
 		return &metav1.Condition{
@@ -376,7 +396,7 @@ func (c *gitHubWatcherController) validateGitHubConnection(
 		}, "", nil, nil
 	}
 
-	conn, tlsDialErr := tls.Dial("tcp", hostPort.Endpoint(), ptls.Default(certPool))
+	conn, tlsDialErr := c.dialFunc("tcp", hostPort.Endpoint(), ptls.Default(certPool))
 	if tlsDialErr != nil {
 		return &metav1.Condition{
 			Type:    GitHubConnectionValid,
diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go
index 110e8bcc4..8d42a5b89 100644
--- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go
+++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go
@@ -6,6 +6,7 @@ package githubupstreamwatcher
 import (
 	"bytes"
 	"context"
+	"crypto/tls"
 	"encoding/base64"
 	"fmt"
 	"net"
@@ -368,13 +369,17 @@ func TestController(t *testing.T) {
 		name                    string
 		githubIdentityProviders []runtime.Object
 		secrets                 []runtime.Object
+		mockDialer              func(network, addr string, config *tls.Config) (*tls.Conn, error)
 		wantErr                 string
 		wantLogs                []string
 		wantResultingCache      []*upstreamgithub.ProviderConfig
 		wantResultingUpstreams  []v1alpha1.GitHubIdentityProvider
 	}{
 		{
-			name: "no GitHubIdentityProviders",
+			name:                   "no GitHubIdentityProviders",
+			wantResultingCache:     []*upstreamgithub.ProviderConfig{},
+			wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{},
+			wantLogs:               []string{},
 		},
 		{
 			name:    "happy path with all fields",
@@ -386,17 +391,23 @@ func TestController(t *testing.T) {
 				{
 					Name:               "some-idp-name",
 					ResourceUID:        "some-resource-uid",
-					Host:               fmt.Sprintf("https://%s", *validFilledOutIDP.Spec.GitHubAPI.Host),
+					APIBaseURL:         fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
 					UsernameAttribute:  "id",
 					GroupNameAttribute: "name",
 					OAuth2Config: &oauth2.Config{
 						ClientID:     "some-client-id",
 						ClientSecret: "some-client-secret",
+						Endpoint: oauth2.Endpoint{
+							AuthURL:       fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							DeviceAuthURL: "", // not used
+							TokenURL:      fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							AuthStyle:     oauth2.AuthStyleInParams,
+						},
+						RedirectURL: "", // not used
+						Scopes:      []string{"read:user", "read:org"},
 					},
-					AllowedOrganizations:    []string{"organization1", "org2"},
-					OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations",
-					AuthorizationURL:        fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
-					HttpClient:              nil, // let the test runner populate this for us
+					AllowedOrganizations: []string{"organization1", "org2"},
+					HttpClient:           nil, // let the test runner populate this for us
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -436,16 +447,22 @@ func TestController(t *testing.T) {
 				{
 					Name:               "minimal-idp-name",
 					ResourceUID:        "minimal-uid",
-					Host:               fmt.Sprintf("https://%s", goodServerDomain),
+					APIBaseURL:         fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
 					UsernameAttribute:  "login",
 					GroupNameAttribute: "slug",
 					OAuth2Config: &oauth2.Config{
 						ClientID:     "some-client-id",
 						ClientSecret: "some-client-secret",
+						Endpoint: oauth2.Endpoint{
+							AuthURL:       fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							DeviceAuthURL: "", // not used
+							TokenURL:      fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							AuthStyle:     oauth2.AuthStyleInParams,
+						},
+						RedirectURL: "", // not used
+						Scopes:      []string{"read:user", "read:org"},
 					},
-					OrganizationLoginPolicy: "AllGitHubUsers",
-					AuthorizationURL:        fmt.Sprintf("https://%s/login/oauth/authorize", goodServerDomain),
-					HttpClient:              nil, // let the test runner populate this for us
+					HttpClient: nil, // let the test runner populate this for us
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -475,6 +492,80 @@ func TestController(t *testing.T) {
 				buildLogForUpdatingPhase("minimal-idp-name", "Ready"),
 			},
 		},
+		{
+			name:    "happy path using github.com",
+			secrets: []runtime.Object{goodSecret},
+			githubIdentityProviders: []runtime.Object{
+				func() runtime.Object {
+					githubIDP := validMinimalIDP.DeepCopy()
+					githubIDP.Spec.GitHubAPI.Host = ptr.To("github.com")
+					// don't change the CA because we are not really going to dial github.com in this test
+					return githubIDP
+				}(),
+			},
+			mockDialer: func(network, addr string, config *tls.Config) (*tls.Conn, error) {
+				require.Equal(t, "github.com:443", addr)
+				// don't actually dial github.com to avoid making external network calls in unit test
+				certPool, _, err := pinnipedcontroller.BuildCertPoolIDP(validMinimalIDP.Spec.GitHubAPI.TLS)
+				require.NoError(t, err)
+				configClone := config.Clone()
+				configClone.RootCAs = certPool
+				return tls.Dial(network, goodServerDomain, configClone)
+			},
+			wantResultingCache: []*upstreamgithub.ProviderConfig{
+				{
+					Name:               "minimal-idp-name",
+					ResourceUID:        "minimal-uid",
+					APIBaseURL:         "https://api.github.com",
+					UsernameAttribute:  "login",
+					GroupNameAttribute: "slug",
+					OAuth2Config: &oauth2.Config{
+						ClientID:     "some-client-id",
+						ClientSecret: "some-client-secret",
+						Endpoint: oauth2.Endpoint{
+							AuthURL:       "https://github.com:443/login/oauth/authorize",
+							DeviceAuthURL: "", // not used
+							TokenURL:      "https://github.com:443/login/oauth/access_token",
+							AuthStyle:     oauth2.AuthStyleInParams,
+						},
+						RedirectURL: "", // not used
+						Scopes:      []string{"read:user", "read:org"},
+					},
+					HttpClient: nil, // let the test runner populate this for us
+				},
+			},
+			wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
+				{
+					ObjectMeta: validMinimalIDP.ObjectMeta,
+					Spec: func() v1alpha1.GitHubIdentityProviderSpec {
+						githubIDP := validMinimalIDP.DeepCopy()
+						githubIDP.Spec.GitHubAPI.Host = ptr.To("github.com")
+						// don't change the CA because we are not really going to dial github.com in this test
+						return githubIDP.Spec
+					}(),
+					Status: v1alpha1.GitHubIdentityProviderStatus{
+						Phase: v1alpha1.GitHubPhaseReady,
+						Conditions: []metav1.Condition{
+							buildClaimsValidatedTrue(t),
+							buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName),
+							buildGitHubConnectionValidTrue(t, "github.com:443"),
+							buildHostValidTrue(t, "github.com"),
+							buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy),
+							buildTLSConfigurationValidTrue(t),
+						},
+					},
+				},
+			},
+			wantLogs: []string{
+				buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)),
+				buildLogForUpdatingClaimsValidTrue("minimal-idp-name"),
+				buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))),
+				buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "github.com"),
+				buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"),
+				buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, "github.com:443"),
+				buildLogForUpdatingPhase("minimal-idp-name", "Ready"),
+			},
+		},
 		{
 			name:    "happy path with IPv6",
 			secrets: []runtime.Object{goodSecret},
@@ -492,16 +583,22 @@ func TestController(t *testing.T) {
 				{
 					Name:               "minimal-idp-name",
 					ResourceUID:        "minimal-uid",
-					Host:               fmt.Sprintf("https://%s", goodServerIPv6Domain),
+					APIBaseURL:         fmt.Sprintf("https://%s/api/v3", goodServerIPv6Domain),
 					UsernameAttribute:  "login",
 					GroupNameAttribute: "slug",
 					OAuth2Config: &oauth2.Config{
 						ClientID:     "some-client-id",
 						ClientSecret: "some-client-secret",
+						Endpoint: oauth2.Endpoint{
+							AuthURL:       fmt.Sprintf("https://%s/login/oauth/authorize", goodServerIPv6Domain),
+							DeviceAuthURL: "", // not used
+							TokenURL:      fmt.Sprintf("https://%s/login/oauth/access_token", goodServerIPv6Domain),
+							AuthStyle:     oauth2.AuthStyleInParams,
+						},
+						RedirectURL: "", // not used
+						Scopes:      []string{"read:user", "read:org"},
 					},
-					OrganizationLoginPolicy: "AllGitHubUsers",
-					AuthorizationURL:        fmt.Sprintf("https://%s/login/oauth/authorize", goodServerIPv6Domain),
-					HttpClient:              nil, // let the test runner populate this for us
+					HttpClient: nil, // let the test runner populate this for us
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -573,32 +670,44 @@ func TestController(t *testing.T) {
 				{
 					Name:               "some-idp-name",
 					ResourceUID:        "some-resource-uid",
-					Host:               fmt.Sprintf("https://%s", *validFilledOutIDP.Spec.GitHubAPI.Host),
+					APIBaseURL:         fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
 					UsernameAttribute:  "id",
 					GroupNameAttribute: "name",
 					OAuth2Config: &oauth2.Config{
 						ClientID:     "some-client-id",
 						ClientSecret: "some-client-secret",
+						Endpoint: oauth2.Endpoint{
+							AuthURL:       fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							DeviceAuthURL: "", // not used
+							TokenURL:      fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							AuthStyle:     oauth2.AuthStyleInParams,
+						},
+						RedirectURL: "", // not used
+						Scopes:      []string{"read:user", "read:org"},
 					},
-					AllowedOrganizations:    []string{"organization1", "org2"},
-					OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations",
-					AuthorizationURL:        fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
-					HttpClient:              nil, // let the test runner populate this for us
+					AllowedOrganizations: []string{"organization1", "org2"},
+					HttpClient:           nil, // let the test runner populate this for us
 				},
 				{
 					Name:               "other-idp-name",
 					ResourceUID:        "some-resource-uid",
-					Host:               fmt.Sprintf("https://%s", *validFilledOutIDP.Spec.GitHubAPI.Host),
+					APIBaseURL:         fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
 					UsernameAttribute:  "login:id",
 					GroupNameAttribute: "name",
 					OAuth2Config: &oauth2.Config{
 						ClientID:     "other-client-id",
 						ClientSecret: "other-client-secret",
+						Endpoint: oauth2.Endpoint{
+							AuthURL:       fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							DeviceAuthURL: "", // not used
+							TokenURL:      fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
+							AuthStyle:     oauth2.AuthStyleInParams,
+						},
+						RedirectURL: "", // not used
+						Scopes:      []string{"read:user", "read:org"},
 					},
-					AllowedOrganizations:    []string{"organization1", "org2"},
-					OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations",
-					AuthorizationURL:        fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
-					HttpClient:              nil, // let the test runner populate this for us
+					AllowedOrganizations: []string{"organization1", "org2"},
+					HttpClient:           nil, // let the test runner populate this for us
 				},
 			},
 			wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -1727,6 +1836,11 @@ func TestController(t *testing.T) {
 
 			gitHubIdentityProviderInformer := supervisorInformers.IDP().V1alpha1().GitHubIdentityProviders()
 
+			dialer := tt.mockDialer
+			if dialer == nil {
+				dialer = tls.Dial
+			}
+
 			controller := New(
 				namespace,
 				cache,
@@ -1736,6 +1850,7 @@ func TestController(t *testing.T) {
 				logger,
 				controllerlib.WithInformer,
 				frozenClock,
+				dialer,
 			)
 
 			ctx, cancel := context.WithCancel(context.Background())
@@ -1759,25 +1874,22 @@ func TestController(t *testing.T) {
 			require.Equal(t, len(tt.wantResultingCache), len(actualIDPList))
 			for i := 0; i < len(tt.wantResultingCache); i++ {
 				// Do not expect any particular order in the cache
-				var actualIDP *upstreamgithub.Provider
+				var actualProvider *upstreamgithub.Provider
 				for _, possibleIDP := range actualIDPList {
 					if possibleIDP.GetName() == tt.wantResultingCache[i].Name {
 						var ok bool
-						actualIDP, ok = possibleIDP.(*upstreamgithub.Provider)
+						actualProvider, ok = possibleIDP.(*upstreamgithub.Provider)
 						require.True(t, ok)
 						break
 					}
 				}
 
-				require.Equal(t, tt.wantResultingCache[i].Name, actualIDP.GetName())
-				require.Equal(t, tt.wantResultingCache[i].ResourceUID, actualIDP.GetResourceUID())
-				require.Equal(t, tt.wantResultingCache[i].Host, actualIDP.GetHost())
-				require.Equal(t, tt.wantResultingCache[i].OAuth2Config.ClientID, actualIDP.GetClientID())
-				require.Equal(t, tt.wantResultingCache[i].GroupNameAttribute, actualIDP.GetGroupNameAttribute())
-				require.Equal(t, tt.wantResultingCache[i].UsernameAttribute, actualIDP.GetUsernameAttribute())
-				require.Equal(t, tt.wantResultingCache[i].AllowedOrganizations, actualIDP.GetAllowedOrganizations())
-				require.Equal(t, tt.wantResultingCache[i].OrganizationLoginPolicy, actualIDP.GetOrganizationLoginPolicy())
-				require.Equal(t, tt.wantResultingCache[i].AuthorizationURL, actualIDP.GetAuthorizationURL())
+				require.Equal(t, tt.wantResultingCache[i].Name, actualProvider.GetName())
+				require.Equal(t, tt.wantResultingCache[i].ResourceUID, actualProvider.GetResourceUID())
+				require.Equal(t, tt.wantResultingCache[i].OAuth2Config.ClientID, actualProvider.GetClientID())
+				require.Equal(t, tt.wantResultingCache[i].GroupNameAttribute, actualProvider.GetGroupNameAttribute())
+				require.Equal(t, tt.wantResultingCache[i].UsernameAttribute, actualProvider.GetUsernameAttribute())
+				require.Equal(t, tt.wantResultingCache[i].AllowedOrganizations, actualProvider.GetAllowedOrganizations())
 
 				require.GreaterOrEqual(t, len(tt.githubIdentityProviders), i+1, "there must be at least as many input identity providers as items in the cache")
 				githubIDP, ok := tt.githubIdentityProviders[i].(*v1alpha1.GitHubIdentityProvider)
@@ -1785,8 +1897,9 @@ func TestController(t *testing.T) {
 				certPool, _, err := pinnipedcontroller.BuildCertPoolIDP(githubIDP.Spec.GitHubAPI.TLS)
 				require.NoError(t, err)
 
-				compareTLSClientConfigWithinHttpClients(t, phttp.Default(certPool), actualIDP.GetHttpClient())
-				require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualIDP.GetOAuth2Config())
+				compareTLSClientConfigWithinHttpClients(t, phttp.Default(certPool), actualProvider.GetConfig().HttpClient)
+				require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualProvider.GetConfig().OAuth2Config)
+				require.Contains(t, tt.wantResultingCache[i].APIBaseURL, actualProvider.GetConfig().APIBaseURL)
 			}
 
 			// Verify the status conditions as reported in Kubernetes
@@ -2120,6 +2233,7 @@ func TestController_OnlyWantActions(t *testing.T) {
 				logger,
 				controllerlib.WithInformer,
 				frozenClock,
+				tls.Dial,
 			)
 
 			ctx, cancel := context.WithCancel(context.Background())
@@ -2230,6 +2344,7 @@ func TestGitHubUpstreamWatcherControllerFilterSecret(t *testing.T) {
 				logger,
 				observableInformers.WithInformer,
 				clock.RealClock{},
+				tls.Dial,
 			)
 
 			unrelated := &corev1.Secret{}
@@ -2299,6 +2414,7 @@ func TestGitHubUpstreamWatcherControllerFilterGitHubIDP(t *testing.T) {
 				logger,
 				observableInformers.WithInformer,
 				clock.RealClock{},
+				tls.Dial,
 			)
 
 			unrelated := &v1alpha1.GitHubIdentityProvider{}
diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go
index 5da2d7a4e..015c5fa6c 100644
--- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go
+++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go
@@ -19,7 +19,7 @@ import (
 	"go.pinniped.dev/pkg/oidcclient/pkce"
 )
 
-// FederationDomainResolvedGitHubIdentityProvider respresents a FederationDomainIdentityProvider which has
+// FederationDomainResolvedGitHubIdentityProvider represents a FederationDomainIdentityProvider which has
 // been resolved dynamically based on the currently loaded IDP CRs to include the provider.UpstreamGitHubIdentityProviderI
 // and other metadata about the provider.
 type FederationDomainResolvedGitHubIdentityProvider struct {
@@ -76,37 +76,33 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedire
 			AuthURL: p.Provider.GetAuthorizationURL(),
 		},
 		RedirectURL: fmt.Sprintf("%s/callback", downstreamIssuerURL),
+		Scopes:      p.Provider.GetScopes(),
 	}
-	redirectURL := upstreamOAuthConfig.AuthCodeURL(
-		state.EncodedStateParam,
-	)
+	redirectURL := upstreamOAuthConfig.AuthCodeURL(state.EncodedStateParam)
 	return redirectURL, nil
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) Login(
 	_ context.Context,
-	submittedUsername string,
-	submittedPassword string,
+	_ string,
+	_ string,
 ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
-	fmt.Printf("GithubResolvedIdentityProvider ~ Login() called with submittedUserName %s, submittedPassword %s", submittedUsername, submittedPassword)
 	return nil, nil, errors.New("function Login not yet implemented for GitHub IDP")
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback(
 	_ context.Context,
-	authCode string,
-	pkce pkce.Code,
-	nonce nonce.Nonce,
-	redirectURI string,
+	_ string,
+	_ pkce.Code,
+	_ nonce.Nonce,
+	_ string,
 ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
-	fmt.Printf("GithubResolvedIdentityProvider ~ LoginFromCallback() called with authCode: %s, pkce: %#v, nonce: %#v, redirectURI: %s", authCode, pkce, nonce, redirectURI)
 	return nil, nil, errors.New("function LoginFromCallback not yet implemented for GitHub IDP")
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh(
 	_ context.Context,
-	identity *resolvedprovider.Identity,
+	_ *resolvedprovider.Identity,
 ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) {
-	fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamRefresh() called with identity %#v", identity)
 	return nil, errors.New("function UpstreamRefresh not yet implemented for GitHub IDP")
 }
diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
index 3f68f7fa5..fcba5e3a3 100644
--- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
+++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
@@ -4,64 +4,80 @@
 package resolvedgithub
 
 import (
-	"context"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2"
 
-	"go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
+	idpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
+	idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
 	"go.pinniped.dev/internal/federationdomain/resolvedprovider"
-	"go.pinniped.dev/internal/idtransform"
 	"go.pinniped.dev/internal/psession"
+	"go.pinniped.dev/internal/testutil/transformtestutil"
 	"go.pinniped.dev/internal/upstreamgithub"
 )
 
-type fakeTransformer struct{}
-
-func (a fakeTransformer) Evaluate(_ context.Context, _ string, _ []string) (*idtransform.TransformationResult, error) {
-	return &idtransform.TransformationResult{}, nil
-}
-func (a fakeTransformer) Source() interface{} { return nil }
-
 func TestFederationDomainResolvedGitHubIdentityProvider(t *testing.T) {
-	fake := fakeTransformer{}
-	transforms := idtransform.NewTransformationPipeline()
-	transforms.AppendTransformation(fake)
-	subject := FederationDomainResolvedGitHubIdentityProvider{
-		DisplayName: "fake-display-name",
-		Provider: upstreamgithub.New(upstreamgithub.ProviderConfig{
-			Name:        "fake-provider-config",
-			ResourceUID: "fake-resource-uid",
-			OAuth2Config: &oauth2.Config{
-				ClientID:     "clientID12345",
-				ClientSecret: "clientSecret6789",
-				RedirectURL:  "some/redirect/url",
+	transforms := transformtestutil.NewRejectAllAuthPipeline(t)
+
+	provider := upstreamgithub.New(upstreamgithub.ProviderConfig{
+		Name:                 "fake-provider-config",
+		ResourceUID:          "fake-resource-uid",
+		APIBaseURL:           "https://fake-api-host.com",
+		UsernameAttribute:    idpv1alpha1.GitHubUsernameID,
+		GroupNameAttribute:   idpv1alpha1.GitHubUseTeamSlugForGroupName,
+		AllowedOrganizations: []string{"org1", "org2"},
+		HttpClient:           nil, // not needed yet for this test
+		OAuth2Config: &oauth2.Config{
+			ClientID:     "fake-client-id",
+			ClientSecret: "fake-client-secret",
+			Scopes:       []string{"read:user", "read:org"},
+			Endpoint: oauth2.Endpoint{
+				AuthURL:       "https://fake-authorization-url",
+				DeviceAuthURL: "",
+				TokenURL:      "https://fake-token-url",
+				AuthStyle:     oauth2.AuthStyleInParams,
 			},
-		}),
+		},
+	})
+
+	subject := FederationDomainResolvedGitHubIdentityProvider{
+		DisplayName:         "fake-display-name",
+		Provider:            provider,
 		SessionProviderType: psession.ProviderTypeGitHub,
 		Transforms:          transforms,
 	}
 
 	require.Equal(t, "fake-display-name", subject.GetDisplayName())
-	require.Equal(t, upstreamgithub.New(upstreamgithub.ProviderConfig{
-		Name:        "fake-provider-config",
-		ResourceUID: "fake-resource-uid",
-		OAuth2Config: &oauth2.Config{
-			ClientID:     "clientID12345",
-			ClientSecret: "clientSecret6789",
-			RedirectURL:  "some/redirect/url",
-		},
-	}), subject.GetProvider())
+	require.Equal(t, provider, subject.GetProvider())
 	require.Equal(t, psession.ProviderTypeGitHub, subject.GetSessionProviderType())
-	require.Equal(t, v1alpha1.IDPTypeGitHub, subject.GetIDPDiscoveryType())
-	require.Equal(t, []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode}, subject.GetIDPDiscoveryFlows())
+	require.Equal(t, idpdiscoveryv1alpha1.IDPTypeGitHub, subject.GetIDPDiscoveryType())
+	require.Equal(t, []idpdiscoveryv1alpha1.IDPFlow{idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode}, subject.GetIDPDiscoveryFlows())
 	require.Equal(t, transforms, subject.GetTransforms())
-	require.Equal(t, &psession.GitHubSessionData{}, subject.CloneIDPSpecificSessionDataFromSession(&psession.CustomSessionData{
+
+	originalCustomSession := &psession.CustomSessionData{
 		Username:         "fake-username",
 		UpstreamUsername: "fake-upstream-username",
-		GitHub:           &psession.GitHubSessionData{},
-	}))
+		GitHub:           &psession.GitHubSessionData{UpstreamAccessToken: "fake-upstream-access-token"},
+	}
+	clonedCustomSession := subject.CloneIDPSpecificSessionDataFromSession(originalCustomSession)
+	require.Equal(t,
+		&psession.GitHubSessionData{UpstreamAccessToken: "fake-upstream-access-token"},
+		clonedCustomSession,
+	)
+	require.NotSame(t, originalCustomSession, clonedCustomSession)
+
+	customSessionToBeMutated := &psession.CustomSessionData{
+		Username:         "fake-username2",
+		UpstreamUsername: "fake-upstream-username2",
+	}
+	subject.ApplyIDPSpecificSessionDataToSession(customSessionToBeMutated, &psession.GitHubSessionData{UpstreamAccessToken: "fake-upstream-access-token2"})
+	require.Equal(t, &psession.CustomSessionData{
+		Username:         "fake-username2",
+		UpstreamUsername: "fake-upstream-username2",
+		GitHub:           &psession.GitHubSessionData{UpstreamAccessToken: "fake-upstream-access-token2"},
+	}, customSessionToBeMutated)
+
 	redirectURL, err := subject.UpstreamAuthorizeRedirectURL(
 		&resolvedprovider.UpstreamAuthorizeRequestState{
 			EncodedStateParam: "encodedStateParam12345",
@@ -72,7 +88,12 @@ func TestFederationDomainResolvedGitHubIdentityProvider(t *testing.T) {
 	)
 	require.NoError(t, err)
 	require.Equal(t,
-		"?client_id=clientID12345&redirect_uri=https%3A%2F%2Flocalhost%2Ffake%2Fpath%2Fcallback&response_type=code&state=encodedStateParam12345",
+		"https://fake-authorization-url?"+
+			"client_id=fake-client-id&"+
+			"redirect_uri=https%3A%2F%2Flocalhost%2Ffake%2Fpath%2Fcallback&"+
+			"response_type=code&"+
+			"scope=read%3Auser+read%3Aorg&"+
+			"state=encodedStateParam12345",
 		redirectURL,
 	)
 }
diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upsteam_provider.go
index ab5920bdf..decee99aa 100644
--- a/internal/federationdomain/upstreamprovider/upsteam_provider.go
+++ b/internal/federationdomain/upstreamprovider/upsteam_provider.go
@@ -5,7 +5,6 @@ package upstreamprovider
 
 import (
 	"context"
-	"net/http"
 	"net/url"
 
 	"golang.org/x/oauth2"
@@ -131,12 +130,12 @@ type UpstreamLDAPIdentityProviderI interface {
 type UpstreamGithubIdentityProviderI interface {
 	UpstreamIdentityProviderI
 
-	// GetHost returns the hostname of the GitHub server. This is either "github.com" or a GitHub Enterprise Server.
-	GetHost() string
-
 	// GetClientID returns the OAuth client ID registered with the upstream provider to be used in the authorization code flow.
 	GetClientID() string
 
+	// GetScopes returns the scopes to request in authorization (authcode or password grant) flow.
+	GetScopes() []string
+
 	// GetUsernameAttribute returns the attribute from the GitHub API user response to use for the downstream username.
 	// See https://docs.github.com/en/rest/users/users?apiVersion=2022-11-28#get-the-authenticated-user.
 	// Note that this is a constructed value - do not expect that the result will exactly match one of the JSON fields.
@@ -147,23 +146,26 @@ type UpstreamGithubIdentityProviderI interface {
 	// Note that this is a constructed value - do not expect that the result will exactly match one of the JSON fields.
 	GetGroupNameAttribute() v1alpha1.GitHubGroupNameAttribute
 
-	// GetAllowedOrganizations returns a list of organizations configured to allow authentication. A user must have membership
-	// in at least one of these organizations to log in. Note that the user can specify a policy (returned by GetOrganizationLoginPolicy)
-	// to disregard organization membership for purposes of authentication.
-	//
-	// If this list is specified, only teams from the listed organizations should be represented as groups for the downstream token.
+	// GetAllowedOrganizations returns a list of organizations configured to allow authentication.
+	// If this list has contents, a user must have membership in at least one of these organizations to log in,
+	// and only teams from the listed organizations should be represented as groups for the downstream token.
+	// If this list is empty, then any user can log in regardless of org membership, and any observable
+	// teams memberships should be represented as groups for the downstream token.
 	GetAllowedOrganizations() []string
 
-	// GetOrganizationLoginPolicy must be "OnlyUsersFromAllowedOrganizations" if GetAllowedOrganizations has values.
-	// Otherwise, it must be "AllGitHubUsers", which means disregard the result of GetAllowedOrganizations.
-	GetOrganizationLoginPolicy() v1alpha1.GitHubAllowedAuthOrganizationsPolicy
-
 	// GetAuthorizationURL returns the authorization URL for the configured GitHub. This will look like:
 	// https://<spec.githubAPI.host>/login/oauth/authorize
 	// It will not include any query parameters or fragment. Any subdomains or port will come from <spec.githubAPI.host>.
 	// It will never include a username or password in the authority section.
 	GetAuthorizationURL() string
 
-	// GetHttpClient returns a http client configured with the provided CA bundle and a timeout.
-	GetHttpClient() *http.Client
+	// TODO: This interface should be easily mockable to avoid all interactions with the actual server.
+	//  What interactions with the server do we want to hide behind this interface? Something like this?
+	// 	  ExchangeAuthcode(ctx, authcode, redirectURI) (AccessToken, error)
+	//    GetUser(ctx, accessToken) (User, error)
+	//    GetUserOrgs(ctx, accessToken) ([]Org, error)
+	//    GetUserTeams(ctx, accessToken) ([]Team, error)
+	//  Or maybe higher level interface like this?
+	// 	  ExchangeAuthcode(ctx, authcode, redirectURI) (AccessToken, error)
+	//    GetUser(ctx, accessToken) (User, error) // in this case User would include team and org info
 }
diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go
index 31549194c..a29b0663b 100644
--- a/internal/fositestorage/authorizationcode/authorizationcode.go
+++ b/internal/fositestorage/authorizationcode/authorizationcode.go
@@ -383,15 +383,18 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{
 						"ȝƋ鬯犦獢9c5¤.岵": "浛a齙\\蹼偦歛"
 					}
 				},
-				"github": {}
+				"github": {
+					"upstreamAccessToken": " 皦pSǬŝ社Vƅȭǝ*擦28Dž"
+				}
 			}
 		},
 		"requestedAudience": [
-			"皦pSǬŝ社Vƅȭǝ*",
-			"Ƽĝ\"zvưã置bņ抰蛖a³"
+			"甍 ć\u003cʘ筫",
+			"蛖a³2ʫ承dʬ)ġ,TÀqy_"
 		],
 		"grantedAudience": [
-			"ʫ承dʬ)ġ,TÀqy_º"
+			"$+溪ŸȢŒų崓ļ憽",
+			"姧骦:駝重EȫʆɵʮGɃ"
 		]
 	},
 	"version": "7"
diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go
index d825ce037..41fd6d330 100644
--- a/internal/psession/pinniped_session.go
+++ b/internal/psession/pinniped_session.go
@@ -145,6 +145,7 @@ func (s *ActiveDirectorySessionData) Clone() *ActiveDirectorySessionData {
 }
 
 type GitHubSessionData struct {
+	UpstreamAccessToken string `json:"upstreamAccessToken"`
 }
 
 func (s *GitHubSessionData) Clone() *GitHubSessionData {
diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go
index 753c568e6..f0dfebbab 100644
--- a/internal/supervisor/server/server.go
+++ b/internal/supervisor/server/server.go
@@ -335,6 +335,7 @@ func prepareControllers(
 				plog.New(),
 				controllerlib.WithInformer,
 				clock.RealClock{},
+				tls.Dial,
 			),
 			singletonWorker).
 		WithController(
diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go
index 83c1825cd..b28a4487f 100644
--- a/internal/testutil/oidctestutil/testgithubprovider.go
+++ b/internal/testutil/oidctestutil/testgithubprovider.go
@@ -4,8 +4,6 @@
 package oidctestutil
 
 import (
-	"net/http"
-
 	"k8s.io/apimachinery/pkg/types"
 
 	"go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
@@ -15,16 +13,15 @@ import (
 
 type TestUpstreamGitHubIdentityProviderBuilder struct {
 	name                           string
-	clientID                       string
 	resourceUID                    types.UID
+	clientID                       string
+	scopes                         []string
 	displayNameForFederationDomain string
 	transformsForFederationDomain  *idtransform.TransformationPipeline
 	usernameAttribute              v1alpha1.GitHubUsernameAttribute
 	groupNameAttribute             v1alpha1.GitHubGroupNameAttribute
 	allowedOrganizations           []string
-	organizationLoginPolicy        v1alpha1.GitHubAllowedAuthOrganizationsPolicy
 	authorizationURL               string
-	httpClient                     *http.Client
 }
 
 func (u *TestUpstreamGitHubIdentityProviderBuilder) WithName(value string) *TestUpstreamGitHubIdentityProviderBuilder {
@@ -42,6 +39,11 @@ func (u *TestUpstreamGitHubIdentityProviderBuilder) WithClientID(value string) *
 	return u
 }
 
+func (u *TestUpstreamGitHubIdentityProviderBuilder) WithScopes(value []string) *TestUpstreamGitHubIdentityProviderBuilder {
+	u.scopes = value
+	return u
+}
+
 func (u *TestUpstreamGitHubIdentityProviderBuilder) WithDisplayNameForFederationDomain(value string) *TestUpstreamGitHubIdentityProviderBuilder {
 	u.displayNameForFederationDomain = value
 	return u
@@ -62,21 +64,11 @@ func (u *TestUpstreamGitHubIdentityProviderBuilder) WithAllowedOrganizations(val
 	return u
 }
 
-func (u *TestUpstreamGitHubIdentityProviderBuilder) WithOrganizationLoginPolicy(value v1alpha1.GitHubAllowedAuthOrganizationsPolicy) *TestUpstreamGitHubIdentityProviderBuilder {
-	u.organizationLoginPolicy = value
-	return u
-}
-
 func (u *TestUpstreamGitHubIdentityProviderBuilder) WithAuthorizationURL(value string) *TestUpstreamGitHubIdentityProviderBuilder {
 	u.authorizationURL = value
 	return u
 }
 
-func (u *TestUpstreamGitHubIdentityProviderBuilder) WithHttpClient(value *http.Client) *TestUpstreamGitHubIdentityProviderBuilder {
-	u.httpClient = value
-	return u
-}
-
 func (u *TestUpstreamGitHubIdentityProviderBuilder) Build() *TestUpstreamGitHubIdentityProvider {
 	if u.displayNameForFederationDomain == "" {
 		// default it to the CR name
@@ -90,14 +82,13 @@ func (u *TestUpstreamGitHubIdentityProviderBuilder) Build() *TestUpstreamGitHubI
 		Name:                           u.name,
 		ResourceUID:                    u.resourceUID,
 		ClientID:                       u.clientID,
+		Scopes:                         u.scopes,
 		DisplayNameForFederationDomain: u.displayNameForFederationDomain,
 		TransformsForFederationDomain:  u.transformsForFederationDomain,
 		UsernameAttribute:              u.usernameAttribute,
 		GroupNameAttribute:             u.groupNameAttribute,
 		AllowedOrganizations:           u.allowedOrganizations,
-		OrganizationLoginPolicy:        u.organizationLoginPolicy,
 		AuthorizationURL:               u.authorizationURL,
-		HttpClient:                     u.httpClient,
 	}
 }
 
@@ -109,15 +100,13 @@ type TestUpstreamGitHubIdentityProvider struct {
 	Name                           string
 	ClientID                       string
 	ResourceUID                    types.UID
-	Host                           string
+	Scopes                         []string
 	DisplayNameForFederationDomain string
 	TransformsForFederationDomain  *idtransform.TransformationPipeline
 	UsernameAttribute              v1alpha1.GitHubUsernameAttribute
 	GroupNameAttribute             v1alpha1.GitHubGroupNameAttribute
 	AllowedOrganizations           []string
-	OrganizationLoginPolicy        v1alpha1.GitHubAllowedAuthOrganizationsPolicy
 	AuthorizationURL               string
-	HttpClient                     *http.Client
 }
 
 var _ upstreamprovider.UpstreamGithubIdentityProviderI = &TestUpstreamGitHubIdentityProvider{}
@@ -130,8 +119,8 @@ func (u *TestUpstreamGitHubIdentityProvider) GetName() string {
 	return u.Name
 }
 
-func (u *TestUpstreamGitHubIdentityProvider) GetHost() string {
-	return u.Host
+func (u *TestUpstreamGitHubIdentityProvider) GetScopes() []string {
+	return u.Scopes
 }
 
 func (u *TestUpstreamGitHubIdentityProvider) GetClientID() string {
@@ -150,14 +139,6 @@ func (u *TestUpstreamGitHubIdentityProvider) GetAllowedOrganizations() []string
 	return u.AllowedOrganizations
 }
 
-func (u *TestUpstreamGitHubIdentityProvider) GetOrganizationLoginPolicy() v1alpha1.GitHubAllowedAuthOrganizationsPolicy {
-	return u.OrganizationLoginPolicy
-}
-
 func (u *TestUpstreamGitHubIdentityProvider) GetAuthorizationURL() string {
 	return u.AuthorizationURL
 }
-
-func (u *TestUpstreamGitHubIdentityProvider) GetHttpClient() *http.Client {
-	return u.HttpClient
-}
diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go
index 88fa1c79a..52e1120b7 100644
--- a/internal/upstreamgithub/upstreamgithub.go
+++ b/internal/upstreamgithub/upstreamgithub.go
@@ -16,16 +16,31 @@ import (
 
 // ProviderConfig holds the active configuration of an upstream GitHub provider.
 type ProviderConfig struct {
-	Name                    string
-	ResourceUID             types.UID
-	Host                    string
-	UsernameAttribute       v1alpha1.GitHubUsernameAttribute
-	GroupNameAttribute      v1alpha1.GitHubGroupNameAttribute
-	OAuth2Config            *oauth2.Config
-	AllowedOrganizations    []string
-	OrganizationLoginPolicy v1alpha1.GitHubAllowedAuthOrganizationsPolicy
-	AuthorizationURL        string
-	HttpClient              *http.Client
+	Name        string
+	ResourceUID types.UID
+
+	// APIBaseURL is the url of the GitHub API, not including the path to a specific API endpoint.
+	// According to the GitHub docs, it should be either https://api.github.com/ for cloud
+	// or https://HOSTNAME/api/v3/ for Enterprise Server.
+	APIBaseURL string
+
+	UsernameAttribute  v1alpha1.GitHubUsernameAttribute
+	GroupNameAttribute v1alpha1.GitHubGroupNameAttribute
+
+	// AllowedOrganizations, when empty, means to allow users from all orgs.
+	AllowedOrganizations []string
+
+	// HttpClient is a client that can be used to call the GitHub APIs and token endpoint.
+	// This client should be configured with the user-provided CA bundle and a timeout.
+	HttpClient *http.Client
+
+	// OAuth2Config contains ClientID, ClientSecret, Scopes, and Endpoint (which contains auth and token endpoint URLs,
+	// and auth style for the token endpoint).
+	// OAuth2Config will not be used to compute the authorize URL because the redirect back to the Supervisor's
+	// callback must be different per FederationDomain. It holds data that may be useful when calculating the
+	// authorize URL, so that data is exposed by interface methods. However, it can be used to call the token endpoint,
+	// for which there is no RedirectURL needed.
+	OAuth2Config *oauth2.Config
 }
 
 type Provider struct {
@@ -40,12 +55,6 @@ func New(config ProviderConfig) *Provider {
 	return &Provider{c: config}
 }
 
-// GetConfig is a reader for the config. Returns a copy of the config to keep the underlying config read-only.
-func (p *Provider) GetConfig() ProviderConfig {
-	return p.c
-}
-
-// GetName returns a name for this upstream provider.
 func (p *Provider) GetName() string {
 	return p.c.Name
 }
@@ -58,12 +67,8 @@ func (p *Provider) GetClientID() string {
 	return p.c.OAuth2Config.ClientID
 }
 
-func (p *Provider) GetOAuth2Config() *oauth2.Config {
-	return p.c.OAuth2Config
-}
-
-func (p *Provider) GetHost() string {
-	return p.c.Host
+func (p *Provider) GetScopes() []string {
+	return p.c.OAuth2Config.Scopes
 }
 
 func (p *Provider) GetUsernameAttribute() v1alpha1.GitHubUsernameAttribute {
@@ -78,14 +83,11 @@ func (p *Provider) GetAllowedOrganizations() []string {
 	return p.c.AllowedOrganizations
 }
 
-func (p *Provider) GetOrganizationLoginPolicy() v1alpha1.GitHubAllowedAuthOrganizationsPolicy {
-	return p.c.OrganizationLoginPolicy
-}
-
 func (p *Provider) GetAuthorizationURL() string {
-	return p.c.AuthorizationURL
+	return p.c.OAuth2Config.Endpoint.AuthURL
 }
 
-func (p *Provider) GetHttpClient() *http.Client {
-	return p.c.HttpClient
+// GetConfig returns the config. This is not part of the interface and is mostly just for testing.
+func (p *Provider) GetConfig() ProviderConfig {
+	return p.c
 }
diff --git a/internal/upstreamgithub/upstreamgithub_test.go b/internal/upstreamgithub/upstreamgithub_test.go
index d0160ea0b..9726717a1 100644
--- a/internal/upstreamgithub/upstreamgithub_test.go
+++ b/internal/upstreamgithub/upstreamgithub_test.go
@@ -18,16 +18,21 @@ func TestGitHubProvider(t *testing.T) {
 	subject := New(ProviderConfig{
 		Name:               "foo",
 		ResourceUID:        "resource-uid-12345",
-		Host:               "fake-host",
+		APIBaseURL:         "https://fake-base-url",
 		UsernameAttribute:  "fake-username-attribute",
 		GroupNameAttribute: "fake-group-name-attribute",
 		OAuth2Config: &oauth2.Config{
 			ClientID:     "fake-client-id",
 			ClientSecret: "fake-client-secret",
+			Scopes:       []string{"scope1", "scope2"},
+			Endpoint: oauth2.Endpoint{
+				AuthURL:       "https://fake-authorization-url",
+				DeviceAuthURL: "",
+				TokenURL:      "https://fake-token-url",
+				AuthStyle:     oauth2.AuthStyleInParams,
+			},
 		},
-		AllowedOrganizations:    []string{"fake-org", "fake-org2"},
-		OrganizationLoginPolicy: v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers,
-		AuthorizationURL:        "https://fake-authorization-url",
+		AllowedOrganizations: []string{"fake-org", "fake-org2"},
 		HttpClient: &http.Client{
 			Timeout: 1234509,
 		},
@@ -36,16 +41,21 @@ func TestGitHubProvider(t *testing.T) {
 	require.Equal(t, ProviderConfig{
 		Name:               "foo",
 		ResourceUID:        "resource-uid-12345",
-		Host:               "fake-host",
+		APIBaseURL:         "https://fake-base-url",
 		UsernameAttribute:  "fake-username-attribute",
 		GroupNameAttribute: "fake-group-name-attribute",
 		OAuth2Config: &oauth2.Config{
 			ClientID:     "fake-client-id",
 			ClientSecret: "fake-client-secret",
+			Scopes:       []string{"scope1", "scope2"},
+			Endpoint: oauth2.Endpoint{
+				AuthURL:       "https://fake-authorization-url",
+				DeviceAuthURL: "",
+				TokenURL:      "https://fake-token-url",
+				AuthStyle:     oauth2.AuthStyleInParams,
+			},
 		},
-		AllowedOrganizations:    []string{"fake-org", "fake-org2"},
-		OrganizationLoginPolicy: v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers,
-		AuthorizationURL:        "https://fake-authorization-url",
+		AllowedOrganizations: []string{"fake-org", "fake-org2"},
 		HttpClient: &http.Client{
 			Timeout: 1234509,
 		},
@@ -54,17 +64,12 @@ func TestGitHubProvider(t *testing.T) {
 	require.Equal(t, "foo", subject.GetName())
 	require.Equal(t, types.UID("resource-uid-12345"), subject.GetResourceUID())
 	require.Equal(t, "fake-client-id", subject.GetClientID())
-	require.Equal(t, &oauth2.Config{
-		ClientID:     "fake-client-id",
-		ClientSecret: "fake-client-secret",
-	}, subject.GetOAuth2Config())
-	require.Equal(t, "fake-host", subject.GetHost())
+	require.Equal(t, "fake-client-id", subject.GetClientID())
 	require.Equal(t, v1alpha1.GitHubUsernameAttribute("fake-username-attribute"), subject.GetUsernameAttribute())
 	require.Equal(t, v1alpha1.GitHubGroupNameAttribute("fake-group-name-attribute"), subject.GetGroupNameAttribute())
 	require.Equal(t, []string{"fake-org", "fake-org2"}, subject.GetAllowedOrganizations())
-	require.Equal(t, v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers, subject.GetOrganizationLoginPolicy())
 	require.Equal(t, "https://fake-authorization-url", subject.GetAuthorizationURL())
 	require.Equal(t, &http.Client{
 		Timeout: 1234509,
-	}, subject.GetHttpClient())
+	}, subject.GetConfig().HttpClient)
 }

From 6be92f92fb502b95a10a39c181cdefc632fd119e Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Wed, 8 May 2024 11:53:22 -0700
Subject: [PATCH 3/7] bump Supervisor session storage versions

---
 .../controller/supervisorstorage/garbage_collector_test.go   | 2 +-
 internal/fositestorage/accesstoken/accesstoken.go            | 3 ++-
 internal/fositestorage/accesstoken/accesstoken_test.go       | 2 +-
 .../fositestorage/authorizationcode/authorizationcode.go     | 5 +++--
 .../authorizationcode/authorizationcode_test.go              | 2 +-
 internal/fositestorage/openidconnect/openidconnect.go        | 3 ++-
 internal/fositestorage/openidconnect/openidconnect_test.go   | 2 +-
 internal/fositestorage/pkce/pkce.go                          | 3 ++-
 internal/fositestorage/pkce/pkce_test.go                     | 2 +-
 internal/fositestorage/refreshtoken/refreshtoken.go          | 3 ++-
 internal/fositestorage/refreshtoken/refreshtoken_test.go     | 2 +-
 test/integration/supervisor_storage_test.go                  | 2 +-
 12 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go
index c020ba92e..903ee7753 100644
--- a/internal/controller/supervisorstorage/garbage_collector_test.go
+++ b/internal/controller/supervisorstorage/garbage_collector_test.go
@@ -122,7 +122,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 	spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) {
 		const (
 			installedInNamespace         = "some-namespace"
-			currentSessionStorageVersion = "7" // update this when you update the storage version in the production code
+			currentSessionStorageVersion = "8" // update this when you update the storage version in the production code
 		)
 
 		var (
diff --git a/internal/fositestorage/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go
index c67b9cab7..bb712c336 100644
--- a/internal/fositestorage/accesstoken/accesstoken.go
+++ b/internal/fositestorage/accesstoken/accesstoken.go
@@ -35,7 +35,8 @@ const (
 	// Version 5 is when we added the UpstreamUsername and UpstreamGroups fields to psession.CustomSessionData.
 	// Version 6 is when we upgraded fosite in Dec 2023.
 	// Version 7 is when OIDCClients were given configurable ID token lifetimes.
-	accessTokenStorageVersion = "7"
+	// Version 8 is when GitHubIdentityProvider was added.
+	accessTokenStorageVersion = "8"
 )
 
 type RevocationStorage interface {
diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go
index 522c24c90..3d0abbc57 100644
--- a/internal/fositestorage/accesstoken/accesstoken_test.go
+++ b/internal/fositestorage/accesstoken/accesstoken_test.go
@@ -30,7 +30,7 @@ import (
 
 const (
 	namespace       = "test-ns"
-	expectedVersion = "7" // update this when you update the storage version in the production code
+	expectedVersion = "8" // update this when you update the storage version in the production code
 )
 
 var (
diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go
index a29b0663b..283a4f987 100644
--- a/internal/fositestorage/authorizationcode/authorizationcode.go
+++ b/internal/fositestorage/authorizationcode/authorizationcode.go
@@ -36,7 +36,8 @@ const (
 	// Version 5 is when we added the UpstreamUsername and UpstreamGroups fields to psession.CustomSessionData.
 	// Version 6 is when we upgraded fosite in Dec 2023.
 	// Version 7 is when OIDCClients were given configurable ID token lifetimes.
-	authorizeCodeStorageVersion = "7"
+	// Version 8 is when GitHubIdentityProvider was added.
+	authorizeCodeStorageVersion = "8"
 )
 
 var _ oauth2.AuthorizeCodeStorage = &authorizeCodeStorage{}
@@ -397,5 +398,5 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{
 			"姧骦:駝重EȫʆɵʮGɃ"
 		]
 	},
-	"version": "7"
+	"version": "8"
 }`
diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go
index a95d0e970..437f9b227 100644
--- a/internal/fositestorage/authorizationcode/authorizationcode_test.go
+++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go
@@ -43,7 +43,7 @@ import (
 
 const (
 	namespace       = "test-ns"
-	expectedVersion = "7" // update this when you update the storage version in the production code
+	expectedVersion = "8" // update this when you update the storage version in the production code
 )
 
 var (
diff --git a/internal/fositestorage/openidconnect/openidconnect.go b/internal/fositestorage/openidconnect/openidconnect.go
index a04bea4c3..1aa568321 100644
--- a/internal/fositestorage/openidconnect/openidconnect.go
+++ b/internal/fositestorage/openidconnect/openidconnect.go
@@ -36,7 +36,8 @@ const (
 	// Version 5 is when we added the UpstreamUsername and UpstreamGroups fields to psession.CustomSessionData.
 	// Version 6 is when we upgraded fosite in Dec 2023.
 	// Version 7 is when OIDCClients were given configurable ID token lifetimes.
-	oidcStorageVersion = "7"
+	// Version 8 is when GitHubIdentityProvider was added.
+	oidcStorageVersion = "8"
 )
 
 var _ openid.OpenIDConnectRequestStorage = &openIDConnectRequestStorage{}
diff --git a/internal/fositestorage/openidconnect/openidconnect_test.go b/internal/fositestorage/openidconnect/openidconnect_test.go
index 8f64299ed..bcfd10f19 100644
--- a/internal/fositestorage/openidconnect/openidconnect_test.go
+++ b/internal/fositestorage/openidconnect/openidconnect_test.go
@@ -29,7 +29,7 @@ import (
 
 const (
 	namespace       = "test-ns"
-	expectedVersion = "7" // update this when you update the storage version in the production code
+	expectedVersion = "8" // update this when you update the storage version in the production code
 )
 
 var (
diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go
index b0d371b6a..78091d8d4 100644
--- a/internal/fositestorage/pkce/pkce.go
+++ b/internal/fositestorage/pkce/pkce.go
@@ -34,7 +34,8 @@ const (
 	// Version 5 is when we added the UpstreamUsername and UpstreamGroups fields to psession.CustomSessionData.
 	// Version 6 is when we upgraded fosite in Dec 2023.
 	// Version 7 is when OIDCClients were given configurable ID token lifetimes.
-	pkceStorageVersion = "7"
+	// Version 8 is when GitHubIdentityProvider was added.
+	pkceStorageVersion = "8"
 )
 
 var _ pkce.PKCERequestStorage = &pkceStorage{}
diff --git a/internal/fositestorage/pkce/pkce_test.go b/internal/fositestorage/pkce/pkce_test.go
index ed38d51f6..10fd61955 100644
--- a/internal/fositestorage/pkce/pkce_test.go
+++ b/internal/fositestorage/pkce/pkce_test.go
@@ -29,7 +29,7 @@ import (
 
 const (
 	namespace       = "test-ns"
-	expectedVersion = "7" // update this when you update the storage version in the production code
+	expectedVersion = "8" // update this when you update the storage version in the production code
 )
 
 var (
diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go
index efc96071b..20e3f759c 100644
--- a/internal/fositestorage/refreshtoken/refreshtoken.go
+++ b/internal/fositestorage/refreshtoken/refreshtoken.go
@@ -35,7 +35,8 @@ const (
 	// Version 5 is when we added the UpstreamUsername and UpstreamGroups fields to psession.CustomSessionData.
 	// Version 6 is when we upgraded fosite in Dec 2023.
 	// Version 7 is when OIDCClients were given configurable ID token lifetimes.
-	refreshTokenStorageVersion = "7"
+	// Version 8 is when GitHubIdentityProvider was added.
+	refreshTokenStorageVersion = "8"
 )
 
 type RevocationStorage interface {
diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go
index 6075d0723..1944e5737 100644
--- a/internal/fositestorage/refreshtoken/refreshtoken_test.go
+++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go
@@ -30,7 +30,7 @@ import (
 
 const (
 	namespace       = "test-ns"
-	expectedVersion = "7" // update this when you update the storage version in the production code
+	expectedVersion = "8" // update this when you update the storage version in the production code
 )
 
 var (
diff --git a/test/integration/supervisor_storage_test.go b/test/integration/supervisor_storage_test.go
index 9da514620..12171d696 100644
--- a/test/integration/supervisor_storage_test.go
+++ b/test/integration/supervisor_storage_test.go
@@ -93,7 +93,7 @@ func TestAuthorizeCodeStorage(t *testing.T) {
 	// Note that CreateAuthorizeCodeSession() sets Active to true and also sets the Version before storing the session,
 	// so expect those here.
 	session.Active = true
-	session.Version = "7" // this is the value of the authorizationcode.authorizeCodeStorageVersion constant
+	session.Version = "8" // this is the value of the authorizationcode.authorizeCodeStorageVersion constant
 	expectedSessionStorageJSON, err := json.Marshal(session)
 	require.NoError(t, err)
 	require.JSONEq(t, string(expectedSessionStorageJSON), string(initialSecret.Data["pinniped-storage-data"]))

From 7c85a511a2d1ab17207b5845ae38d36f16245460 Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Wed, 8 May 2024 16:38:49 -0700
Subject: [PATCH 4/7] first draft of an e2e integration test for GitHub login
 (skip while WIP)

---
 hack/prepare-for-integration-tests.sh         | 23 ++++-
 .../resolved_github_provider_test.go          |  3 +
 internal/testutil/totp/totp.go                | 84 +++++++++++++++++
 test/integration/e2e_test.go                  | 89 +++++++++++++++++--
 test/integration/supervisor_discovery_test.go |  2 +-
 test/integration/supervisor_login_test.go     |  2 +-
 test/integration/supervisor_upstream_test.go  |  6 +-
 test/integration/supervisor_warnings_test.go  |  2 +-
 test/testlib/browsertest/browsertest.go       | 80 +++++++++++++++++
 test/testlib/client.go                        | 58 +++++++++++-
 test/testlib/env.go                           | 27 ++++--
 11 files changed, 355 insertions(+), 21 deletions(-)
 create mode 100644 internal/testutil/totp/totp.go

diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh
index 7159b9a15..24a6acec4 100755
--- a/hack/prepare-for-integration-tests.sh
+++ b/hack/prepare-for-integration-tests.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-# Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+# Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 # SPDX-License-Identifier: Apache-2.0
 
 #
@@ -31,6 +31,7 @@ clean_kind=no
 api_group_suffix="pinniped.dev" # same default as in the values.yaml ytt file
 dockerfile_path=""
 get_active_directory_vars="" # specify a filename for a script to get AD related env variables
+get_github_vars="" # specify a filename for a script to get GitHub related env variables
 alternate_deploy="undefined"
 pre_install="undefined"
 
@@ -68,6 +69,16 @@ while (("$#")); do
     get_active_directory_vars=$1
     shift
     ;;
+  --get-github-vars)
+    shift
+    # If there are no more command line arguments, or there is another command line argument but it starts with a dash, then error
+    if [[ "$#" == "0" || "$1" == -* ]]; then
+      log_error "--get-github-vars requires a script name to be specified"
+      exit 1
+    fi
+    get_github_vars=$1
+    shift
+    ;;
   --dockerfile-path)
     shift
     # If there are no more command line arguments, or there is another command line argument but it starts with a dash, then error
@@ -121,6 +132,7 @@ if [[ "$help" == "yes" ]]; then
   log_note "   -g, --api-group-suffix:          deploy Pinniped with an alternate API group suffix"
   log_note "   -s, --skip-build:                reuse the most recently built image of the app instead of building"
   log_note "   -a, --get-active-directory-vars: specify a script that exports active directory environment variables"
+  log_note "       --get-github-vars:           specify a script that exports GitHub environment variables"
   log_note "       --alternate-deploy:          specify an alternate deploy script to install all components of Pinniped"
   log_note "       --pre-install:               specify an pre-install script such as a build script"
   exit 1
@@ -453,6 +465,15 @@ if [[ "$get_active_directory_vars" != "" ]]; then
   source $get_active_directory_vars
 fi
 
+# We can't set up an in-cluster GitHub instance, but
+# if you have a GitHub account that you wish to run the tests against,
+# specify a script to set the GitHub environment variables.
+# You will need to set the environment variables that start with "PINNIPED_TEST_GITHUB_"
+# found in pinniped/test/testlib/env.go.
+if [[ "$get_github_vars" != "" ]]; then
+  source $get_github_vars
+fi
+
 read -r -d '' PINNIPED_TEST_CLUSTER_CAPABILITY_YAML << PINNIPED_TEST_CLUSTER_CAPABILITY_YAML_EOF || true
 ${pinniped_cluster_capability_file_content}
 PINNIPED_TEST_CLUSTER_CAPABILITY_YAML_EOF
diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
index fcba5e3a3..bf6d98a52 100644
--- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
+++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go
@@ -87,6 +87,9 @@ func TestFederationDomainResolvedGitHubIdentityProvider(t *testing.T) {
 		"https://localhost/fake/path",
 	)
 	require.NoError(t, err)
+	// Note that GitHub does not require (or document) the standard response_type=code param, but in manual testing
+	// of GitHub authorize endpoint, it seems to ignore the param. The oauth2 package wants to add the param, so
+	// we will let it.
 	require.Equal(t,
 		"https://fake-authorization-url?"+
 			"client_id=fake-client-id&"+
diff --git a/internal/testutil/totp/totp.go b/internal/testutil/totp/totp.go
new file mode 100644
index 000000000..7951b07ce
--- /dev/null
+++ b/internal/testutil/totp/totp.go
@@ -0,0 +1,84 @@
+package totp
+
+import (
+	"crypto/hmac"
+	"crypto/sha1" //nolint:gosec // This is an implementation of an RFC that used SHA-1
+	"encoding/base32"
+	"encoding/binary"
+	"fmt"
+	"github.com/stretchr/testify/require"
+	"math"
+	"strings"
+	"testing"
+	"time"
+)
+
+// This code is borrowed from
+// https://github.com/yitsushi/totp-cli/blob/b26f5673ae2e5cc682fc1f5ed771cb08a6403283/internal/security/otp.go
+// and
+// https://github.com/yitsushi/totp-cli/blob/b26f5673ae2e5cc682fc1f5ed771cb08a6403283/internal/security/error.go
+// which is MIT licensed. The MIT license allows copying.
+// We are choosing to copying rather than take on a whole new project dependency just for a small test helper.
+
+const (
+	mask1              = 0xf
+	mask2              = 0x7f
+	mask3              = 0xff
+	timeSplitInSeconds = 30
+	shift24            = 24
+	shift16            = 16
+	shift8             = 8
+	sumByteLength      = 8
+)
+
+// OTPError is an error describing an error during generation.
+type OTPError struct {
+	Message string
+}
+
+func (e OTPError) Error() string {
+	return "otp error: " + e.Message
+}
+
+// GenerateOTPCode generates a 6 digit TOTP from the secret Token.
+func GenerateOTPCode(t *testing.T, token string, when time.Time) (string, int64) {
+	t.Helper()
+
+	require.NotEmpty(t, token)
+
+	timer := uint64(math.Floor(float64(when.Unix()) / float64(timeSplitInSeconds)))
+	remainingTime := timeSplitInSeconds - when.Unix()%timeSplitInSeconds
+
+	// Remove spaces, some providers are giving us in a readable format,
+	// so they add spaces in there. If it's not removed while pasting in,
+	// remove it now.
+	token = strings.ReplaceAll(token, " ", "")
+
+	// It should be uppercase always
+	token = strings.ToUpper(token)
+
+	secretBytes, err := base32.StdEncoding.WithPadding(base32.NoPadding).DecodeString(token)
+	require.NoError(t, err)
+
+	length := 6
+
+	buf := make([]byte, sumByteLength)
+	mac := hmac.New(sha1.New, secretBytes)
+
+	binary.BigEndian.PutUint64(buf, timer)
+	_, _ = mac.Write(buf)
+	sum := mac.Sum(nil)
+
+	// http://tools.ietf.org/html/rfc4226#section-5.4
+	offset := sum[len(sum)-1] & mask1
+	value := int64(((int(sum[offset]) & mask2) << shift24) |
+		((int(sum[offset+1] & mask3)) << shift16) |
+		((int(sum[offset+2] & mask3)) << shift8) |
+		(int(sum[offset+3]) & mask3))
+
+	modulo := int32(value % int64(math.Pow10(length)))
+
+	format := fmt.Sprintf("%%0%dd", length)
+
+	return fmt.Sprintf(format, modulo), remainingTime
+}
diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go
index da42f9e4e..16bf2c323 100644
--- a/test/integration/e2e_test.go
+++ b/test/integration/e2e_test.go
@@ -158,7 +158,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 		testlib.WaitForFederationDomainStatusPhase(testCtx, t, federationDomain.Name, configv1alpha1.FederationDomainPhaseReady)
@@ -244,7 +244,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 		testlib.WaitForFederationDomainStatusPhase(testCtx, t, federationDomain.Name, configv1alpha1.FederationDomainPhaseReady)
@@ -332,7 +332,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 		testlib.WaitForFederationDomainStatusPhase(testCtx, t, federationDomain.Name, configv1alpha1.FederationDomainPhaseReady)
@@ -456,7 +456,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 		testlib.WaitForFederationDomainStatusPhase(testCtx, t, federationDomain.Name, configv1alpha1.FederationDomainPhaseReady)
@@ -587,7 +587,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 		testlib.WaitForFederationDomainStatusPhase(testCtx, t, federationDomain.Name, configv1alpha1.FederationDomainPhaseReady)
@@ -660,7 +660,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 		testlib.WaitForFederationDomainStatusPhase(testCtx, t, federationDomain.Name, configv1alpha1.FederationDomainPhaseReady)
@@ -1219,6 +1219,81 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 			sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, allScopes)
 	})
 
+	t.Run("with Supervisor GitHub upstream IDP and browser flow with with form_post automatic authcode delivery to CLI", func(t *testing.T) {
+		// TODO only skip this test when the GitHub test env vars are not set
+		t.Skip("always skipping for now, this test is still a work in progress and it always fails at the moment")
+
+		testCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+		t.Cleanup(cancel)
+
+		tempDir := t.TempDir() // 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.
+		browser := browsertest.OpenBrowser(t)
+
+		// TODO create clusterrolebinding for expected user and WaitForUserToHaveAccess. doesn't matter until login fully works.
+
+		// Create upstream GitHub provider and wait for it to become ready.
+		// TODO use return value when calling requireUserCanUseKubectlWithoutAuthenticatingAgain below
+		_ = testlib.CreateTestGitHubIdentityProvider(t, idpv1alpha1.GitHubIdentityProviderSpec{
+			AllowAuthentication: idpv1alpha1.GitHubAllowAuthenticationSpec{
+				Organizations: idpv1alpha1.GitHubOrganizationsSpec{
+					Policy: ptr.To(idpv1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers),
+				},
+			},
+			Client: idpv1alpha1.GitHubClientSpec{
+				SecretName: testlib.CreateGitHubClientCredentialsSecret(t,
+					env.SupervisorUpstreamGithub.GithubAppClientID,
+					env.SupervisorUpstreamGithub.GithubAppClientSecret,
+				).Name,
+			},
+		}, idpv1alpha1.GitHubPhaseReady)
+		testlib.WaitForFederationDomainStatusPhase(testCtx, t, federationDomain.Name, configv1alpha1.FederationDomainPhaseReady)
+		testlib.WaitForJWTAuthenticatorStatusPhase(testCtx, t, authenticator.Name, authv1alpha.JWTAuthenticatorPhaseReady)
+
+		// Use a specific session cache for this test.
+		sessionCachePath := tempDir + "/test-sessions.yaml"
+		credentialCachePath := tempDir + "/test-credentials.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,
+			"--credential-cache", credentialCachePath,
+			// use default for --oidc-scopes, which is to request all relevant scopes
+		})
+
+		// 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.
+		// TODO use return value when calling requireKubectlGetNamespaceOutput below
+		_ = startKubectlAndOpenAuthorizationURLInBrowser(testCtx, t, kubectlCmd, browser)
+
+		// Confirm that we got to the upstream IDP's login page, fill out the form, and submit the form.
+		browsertest.LoginToUpstreamGitHub(t, browser, env.SupervisorUpstreamGithub)
+
+		// Expect to be redirected to the downstream callback which is serving the form_post HTML.
+		t.Logf("waiting for response page %s", federationDomain.Spec.Issuer)
+		browser.WaitForURL(t, regexp.MustCompile(regexp.QuoteMeta(federationDomain.Spec.Issuer)))
+
+		// TODO When you turn off headless and watch this test run,
+		//   the browser is indeed redirected back to the Supervisor at this point with a code,
+		//   but the Supervisor's callback endpoint does not yet work for github IDPs so it returns an error page,
+		//   and the Supervisor's form_post page is not loaded, so it does not automatically post the callback to the CLI's callback listener.
+		//   The test eventually times out and fails at this point.
+
+		// TODO
+		//  formpostExpectSuccessState
+		//  requireKubectlGetNamespaceOutput
+		//  requireUserCanUseKubectlWithoutAuthenticatingAgain
+	})
+
 	t.Run("with multiple IDPs: one OIDC and one LDAP", func(t *testing.T) {
 		testlib.SkipTestWhenLDAPIsUnavailable(t, env)
 
@@ -1280,7 +1355,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 
diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go
index 9e8e8536b..340ea728d 100644
--- a/test/integration/supervisor_discovery_test.go
+++ b/test/integration/supervisor_discovery_test.go
@@ -828,7 +828,7 @@ func requireIDPsListedByIDPDiscoveryEndpoint(
 			Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 		},
 		Client: idpv1alpha1.OIDCClient{
-			SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+			SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 		},
 	}, idpv1alpha1.PhaseReady)
 
diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go
index ee5802a25..6947568d5 100644
--- a/test/integration/supervisor_login_test.go
+++ b/test/integration/supervisor_login_test.go
@@ -68,7 +68,7 @@ func TestSupervisorLogin_Browser(t *testing.T) {
 				CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)),
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}
 	}
diff --git a/test/integration/supervisor_upstream_test.go b/test/integration/supervisor_upstream_test.go
index fdd21269d..70c805ee1 100644
--- a/test/integration/supervisor_upstream_test.go
+++ b/test/integration/supervisor_upstream_test.go
@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package integration
@@ -60,7 +60,7 @@ Get "https://127.0.0.1:444444/invalid-url-that-is-really-really-long-nananananan
 				AdditionalScopes: []string{"email", "profile"},
 			},
 			Client: v1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, "test-client-id", "test-client-secret").Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, "test-client-id", "test-client-secret").Name,
 			},
 		}
 		upstream := testlib.CreateTestOIDCIdentityProvider(t, spec, v1alpha1.PhaseError)
@@ -98,7 +98,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + env.Su
 				AdditionalScopes: []string{"email", "profile"},
 			},
 			Client: v1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, "test-client-id", "test-client-secret").Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, "test-client-id", "test-client-secret").Name,
 			},
 		}
 		upstream := testlib.CreateTestOIDCIdentityProvider(t, spec, v1alpha1.PhaseReady)
diff --git a/test/integration/supervisor_warnings_test.go b/test/integration/supervisor_warnings_test.go
index b07f0a73a..ef321f2d8 100644
--- a/test/integration/supervisor_warnings_test.go
+++ b/test/integration/supervisor_warnings_test.go
@@ -417,7 +417,7 @@ func TestSupervisorWarnings_Browser(t *testing.T) {
 				Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
 			},
 			Client: idpv1alpha1.OIDCClient{
-				SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+				SecretName: testlib.CreateOIDCClientCredentialsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
 			},
 		}, idpv1alpha1.PhaseReady)
 		testlib.WaitForFederationDomainStatusPhase(ctx, t, downstream.Name, configv1alpha1.FederationDomainPhaseReady)
diff --git a/test/testlib/browsertest/browsertest.go b/test/testlib/browsertest/browsertest.go
index b9ddfa2e7..4cf1c3de8 100644
--- a/test/testlib/browsertest/browsertest.go
+++ b/test/testlib/browsertest/browsertest.go
@@ -20,6 +20,7 @@ import (
 	"github.com/chromedp/chromedp"
 	"github.com/stretchr/testify/require"
 
+	"go.pinniped.dev/internal/testutil/totp"
 	"go.pinniped.dev/test/testlib"
 )
 
@@ -357,6 +358,85 @@ func LoginToUpstreamOIDC(t *testing.T, b *Browser, upstream testlib.TestOIDCUpst
 	b.ClickFirstMatch(t, cfg.LoginButtonSelector)
 }
 
+// LoginToUpstreamGitHub expects the page to be redirected to GitHub.
+// It knows how to enter the test username/password and submit the upstream login form.
+func LoginToUpstreamGitHub(t *testing.T, b *Browser, upstream testlib.TestGithubUpstream) {
+	t.Helper()
+
+	// Expect to be redirected to the login page.
+	t.Logf("waiting for redirect to GitHub login page")
+	b.WaitForURL(t, regexp.MustCompile(`\Ahttps://github\.com/login.+\z`))
+
+	usernameSelector := "input#login_field"
+	passwordSelector := "input#password"
+	loginButtonSelector := "input[type=submit]"
+
+	// Wait for the login page to be rendered.
+	b.WaitForVisibleElements(t, usernameSelector, passwordSelector, loginButtonSelector)
+
+	// Fill in the username and password and click "submit".
+	t.Logf("logging into GitHub")
+	b.SendKeysToFirstMatch(t, usernameSelector, upstream.TestUserUsername)
+	b.SendKeysToFirstMatch(t, passwordSelector, upstream.TestUserPassword)
+	b.ClickFirstMatch(t, loginButtonSelector)
+
+	// Next, GitHub should go to a new page and prompt for the six digit MFA/OTP code.
+	otpSelector := "input#app_totp"
+
+	// Wait for the MFA page to be rendered.
+	t.Logf("waiting for GitHub MFA page")
+	b.WaitForVisibleElements(t, otpSelector)
+
+	code, codeRemainingLifetimeSeconds := totp.GenerateOTPCode(t, upstream.TestUserOTPSecret, time.Now())
+	if codeRemainingLifetimeSeconds < 2 {
+		t.Log("sleeping for 2 seconds before generating another OTP code")
+		time.Sleep(2 * time.Second)
+		code, _ = totp.GenerateOTPCode(t, upstream.TestUserOTPSecret, time.Now())
+	}
+
+	// Fill in the OTP code. We do not need to click "verify" because entering the code automatically submits the page.
+	t.Logf("entering GitHub OTP code")
+	b.SendKeysToFirstMatch(t, otpSelector, code)
+
+	t.Log("sleeping for 2 seconds before looking at page title")
+	time.Sleep(2 * time.Second)
+	pageTitle := b.Title(t)
+	t.Logf("saw page title %q", pageTitle)
+
+	// Next Github might go to another page asking if you authorize the GitHub App to act on your behalf,
+	// if this user has never authorized this app.
+	if strings.HasPrefix(pageTitle, "Authorize ") { // the title is "Authorize <App Name>"
+		// Wait for the authorize app page to be rendered.
+		t.Logf("waiting for GitHub authorize button")
+		// There are unfortunately two very similar buttons on this page:
+		// <button name="authorize" value="0" type="submit" data-view-component="true" class="ws-normal btn width-full mr-2">Cancel
+		// <button name="authorize" value="1" type="submit" data-view-component="true" class="js-oauth-authorize-btn ws-normal btn-primary btn width-full">Authorize
+		submitAuthorizeAppButtonSelector := "button.btn-primary"
+		b.WaitForVisibleElements(t, submitAuthorizeAppButtonSelector)
+		t.Logf("clicking authorize button")
+		b.ClickFirstMatch(t, submitAuthorizeAppButtonSelector)
+
+		t.Log("sleeping for 2 seconds before looking at page title again")
+		time.Sleep(2 * time.Second)
+		pageTitle = b.Title(t)
+		t.Logf("saw page title %q", pageTitle)
+	}
+
+	// TODO I only saw this happen once, so I did not get a chance to finish this code. Not sure if it will happen again?
+	// Next GitHub might ask if we want to configure a passkey for auth.
+	if strings.HasPrefix(pageTitle, "Passkey TODO GET THIS PAGE TITLE") {
+		// The link that we want to click looks like this:
+		// <input class="btn-link" type="submit" value="Don't ask again for this browser">
+		dontAskAgainLinkSelector := `input[value="Don't ask again for this browser"]`
+		// Wait for the passkey page to be rendered.
+		t.Logf("waiting for GitHub's don't ask again button")
+		b.WaitForVisibleElements(t, dontAskAgainLinkSelector)
+		// Tell it that we do not want to use a passkey.
+		t.Logf("clicking don't ask again button")
+		b.ClickFirstMatch(t, dontAskAgainLinkSelector)
+	}
+}
+
 // LoginToUpstreamLDAP expects the page to be redirected to the Supervisor's login UI for an LDAP/AD IDP.
 // It knows how to enter the test username/password and submit the upstream login form.
 func LoginToUpstreamLDAP(t *testing.T, b *Browser, issuer, username, password string) {
diff --git a/test/testlib/client.go b/test/testlib/client.go
index 1f820c03a..f1c296552 100644
--- a/test/testlib/client.go
+++ b/test/testlib/client.go
@@ -497,13 +497,23 @@ func CreateTestSecretBytes(t *testing.T, namespace string, baseName string, secr
 	return created
 }
 
-func CreateClientCredsSecret(t *testing.T, clientID string, clientSecret string) *corev1.Secret {
+func CreateOIDCClientCredentialsSecret(t *testing.T, clientID string, clientSecret string) *corev1.Secret {
+	t.Helper()
+	return createClientCredentialsSecret(t, clientID, clientSecret, "secrets.pinniped.dev/oidc-client")
+}
+
+func CreateGitHubClientCredentialsSecret(t *testing.T, clientID string, clientSecret string) *corev1.Secret {
+	t.Helper()
+	return createClientCredentialsSecret(t, clientID, clientSecret, "secrets.pinniped.dev/github-client")
+}
+
+func createClientCredentialsSecret(t *testing.T, clientID string, clientSecret string, secretType string) *corev1.Secret {
 	t.Helper()
 	env := IntegrationEnv(t)
 	return CreateTestSecret(t,
 		env.SupervisorNamespace,
 		"client-creds",
-		"secrets.pinniped.dev/oidc-client",
+		corev1.SecretType(secretType),
 		map[string]string{
 			"clientID":     clientID,
 			"clientSecret": clientSecret,
@@ -585,6 +595,50 @@ func createOIDCClientSecret(t *testing.T, forOIDCClient *configv1alpha1.OIDCClie
 	return generatedSecret
 }
 
+func CreateTestGitHubIdentityProvider(t *testing.T, spec idpv1alpha1.GitHubIdentityProviderSpec, expectedPhase idpv1alpha1.GitHubIdentityProviderPhase) *idpv1alpha1.GitHubIdentityProvider {
+	t.Helper()
+	return CreateTestGitHubIdentityProviderWithObjectMeta(t, spec, TestObjectMeta(t, "upstream-github-idp"), expectedPhase)
+}
+
+func CreateTestGitHubIdentityProviderWithObjectMeta(t *testing.T, spec idpv1alpha1.GitHubIdentityProviderSpec, objectMeta metav1.ObjectMeta, expectedPhase idpv1alpha1.GitHubIdentityProviderPhase) *idpv1alpha1.GitHubIdentityProvider {
+	t.Helper()
+	env := IntegrationEnv(t)
+	client := NewSupervisorClientset(t)
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+
+	upstreams := client.IDPV1alpha1().GitHubIdentityProviders(env.SupervisorNamespace)
+
+	// Create the GitHubIdentityProvider using GenerateName to get a random name.
+	created, err := upstreams.Create(ctx, &idpv1alpha1.GitHubIdentityProvider{
+		ObjectMeta: objectMeta,
+		Spec:       spec,
+	}, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	// Always clean this up after this point.
+	t.Cleanup(func() {
+		t.Logf("cleaning up test GitHubIdentityProvider %s/%s", created.Namespace, created.Name)
+		err := upstreams.Delete(context.Background(), created.Name, metav1.DeleteOptions{})
+		notFound := k8serrors.IsNotFound(err)
+		// It's okay if it is not found, because it might have been deleted by another part of this test.
+		if !notFound {
+			require.NoErrorf(t, err, "could not cleanup test GitHubIdentityProvider %s/%s", created.Namespace, created.Name)
+		}
+	})
+	t.Logf("created test GitHubIdentityProvider %s", created.Name)
+
+	// Wait for the GitHubIdentityProvider to enter the expected phase (or time out).
+	var result *idpv1alpha1.GitHubIdentityProvider
+	RequireEventuallyf(t, func(requireEventually *require.Assertions) {
+		var err error
+		result, err = upstreams.Get(ctx, created.Name, metav1.GetOptions{})
+		requireEventually.NoErrorf(err, "error while getting GitHubIdentityProvider %s/%s", created.Namespace, created.Name)
+		requireEventually.Equal(expectedPhase, result.Status.Phase)
+	}, 60*time.Second, 1*time.Second, "expected the GitHubIdentityProvider to go into phase %s, GitHubIdentityProvider was: %s", expectedPhase, Sdump(result))
+	return result
+}
+
 func CreateTestOIDCIdentityProvider(t *testing.T, spec idpv1alpha1.OIDCIdentityProviderSpec, expectedPhase idpv1alpha1.OIDCIdentityProviderPhase) *idpv1alpha1.OIDCIdentityProvider {
 	t.Helper()
 	return CreateTestOIDCIdentityProviderWithObjectMeta(t, spec, TestObjectMeta(t, "upstream-oidc-idp"), expectedPhase)
diff --git a/test/testlib/env.go b/test/testlib/env.go
index 74ea0ae90..a14bdca35 100644
--- a/test/testlib/env.go
+++ b/test/testlib/env.go
@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package testlib
@@ -62,10 +62,11 @@ type TestEnv struct {
 		ExpectedGroups   []string `json:"expectedGroups"`
 	} `json:"testUser"`
 
-	CLIUpstreamOIDC                   TestOIDCUpstream `json:"cliOIDCUpstream"`
-	SupervisorUpstreamOIDC            TestOIDCUpstream `json:"supervisorOIDCUpstream"`
-	SupervisorUpstreamLDAP            TestLDAPUpstream `json:"supervisorLDAPUpstream"`
-	SupervisorUpstreamActiveDirectory TestLDAPUpstream `json:"supervisorActiveDirectoryUpstream"`
+	CLIUpstreamOIDC                   TestOIDCUpstream   `json:"cliOIDCUpstream"`
+	SupervisorUpstreamOIDC            TestOIDCUpstream   `json:"supervisorOIDCUpstream"`
+	SupervisorUpstreamLDAP            TestLDAPUpstream   `json:"supervisorLDAPUpstream"`
+	SupervisorUpstreamActiveDirectory TestLDAPUpstream   `json:"supervisorActiveDirectoryUpstream"`
+	SupervisorUpstreamGithub          TestGithubUpstream `json:"supervisorGithubUpstream"`
 }
 
 type TestOIDCUpstream struct {
@@ -110,6 +111,14 @@ type TestLDAPUpstream struct {
 	TestDeactivatedUserPassword                     string   `json:"TestDeactivatedUserPassword"`
 }
 
+type TestGithubUpstream struct {
+	GithubAppClientID     string `json:"githubAppClientId"`
+	GithubAppClientSecret string `json:"githubAppClientSecret"`
+	TestUserUsername      string `json:"testUserUsername"`
+	TestUserPassword      string `json:"testUserPassword"`
+	TestUserOTPSecret     string `json:"testUserOTPSecret"`
+}
+
 // ProxyEnv returns a set of environment variable strings (e.g., to combine with os.Environ()) which set up the configured test HTTP proxy.
 func (e *TestEnv) ProxyEnv() []string {
 	if e.Proxy == "" {
@@ -319,6 +328,14 @@ func loadEnvVars(t *testing.T, result *TestEnv) {
 		GroupSearchBase:                                 wantEnv("PINNIPED_TEST_AD_USERS_DN", ""),
 	}
 
+	result.SupervisorUpstreamGithub = TestGithubUpstream{
+		GithubAppClientID:     wantEnv("PINNIPED_TEST_GITHUB_APP_CLIENT_ID", ""),
+		GithubAppClientSecret: wantEnv("PINNIPED_TEST_GITHUB_APP_CLIENT_SECRET", ""),
+		TestUserUsername:      wantEnv("PINNIPED_TEST_GITHUB_USER_USERNAME", ""),
+		TestUserPassword:      wantEnv("PINNIPED_TEST_GITHUB_USER_PASSWORD", ""),
+		TestUserOTPSecret:     wantEnv("PINNIPED_TEST_GITHUB_USER_OTP_SECRET", ""),
+	}
+
 	sort.Strings(result.SupervisorUpstreamLDAP.TestUserDirectGroupsCNs)
 	sort.Strings(result.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs)
 	sort.Strings(result.SupervisorUpstreamActiveDirectory.TestUserDirectGroupsCNs)

From 32e4e0d8358cc8881f0bd46a3c43bb218e6682b3 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Wed, 8 May 2024 22:39:16 -0500
Subject: [PATCH 5/7] Add TestGenerateOTPCode

---
 internal/testutil/totp/totp_test.go | 44 +++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 internal/testutil/totp/totp_test.go

diff --git a/internal/testutil/totp/totp_test.go b/internal/testutil/totp/totp_test.go
new file mode 100644
index 000000000..5ff32bdf8
--- /dev/null
+++ b/internal/testutil/totp/totp_test.go
@@ -0,0 +1,44 @@
+package totp
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGenerateOTPCode(t *testing.T) {
+	tests := []struct {
+		name                         string
+		token                        string
+		when                         time.Time
+		wantCode                     string
+		wantRemainingLifetimeSeconds int64
+	}{
+		{
+			name:                         "Use a token from online example",
+			token:                        "JBSWY3DPEHPK3PXP", // https://github.com/pquerna/otp/blob/3357de7c04813a328d6a1e4a514854213e0f8ce8/totp/totp.go#L180
+			when:                         time.Unix(1715205169, 0),
+			wantCode:                     "780919",
+			wantRemainingLifetimeSeconds: 11,
+		},
+		{
+			name:                         "Use a token that was randomly generated",
+			token:                        "EDAYKXL3TEYZNQ3O4N5KPSUAQQLZYUJG",
+			when:                         time.Unix(1715225917, 0),
+			wantCode:                     "920615",
+			wantRemainingLifetimeSeconds: 23,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			actualCode, actualRemainingLifetimeSeconds := GenerateOTPCode(t, test.token, test.when)
+
+			require.Equal(t, test.wantCode, actualCode)
+			require.Equal(t, test.wantRemainingLifetimeSeconds, actualRemainingLifetimeSeconds)
+		})
+	}
+}

From 75f024cb1928f2f6d0fea5dd5fafd5c89f504979 Mon Sep 17 00:00:00 2001
From: Joshua Casey <joshuatcasey@gmail.com>
Date: Thu, 9 May 2024 08:33:29 -0500
Subject: [PATCH 6/7] Fix lint

---
 internal/testutil/totp/totp.go      | 6 +++++-
 internal/testutil/totp/totp_test.go | 5 +++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/internal/testutil/totp/totp.go b/internal/testutil/totp/totp.go
index 7951b07ce..7e2c153cf 100644
--- a/internal/testutil/totp/totp.go
+++ b/internal/testutil/totp/totp.go
@@ -1,3 +1,6 @@
+// Copyright 2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
 package totp
 
 import (
@@ -6,11 +9,12 @@ import (
 	"encoding/base32"
 	"encoding/binary"
 	"fmt"
-	"github.com/stretchr/testify/require"
 	"math"
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/require"
 )
 
 // This code is borrowed from
diff --git a/internal/testutil/totp/totp_test.go b/internal/testutil/totp/totp_test.go
index 5ff32bdf8..3fa0320b3 100644
--- a/internal/testutil/totp/totp_test.go
+++ b/internal/testutil/totp/totp_test.go
@@ -1,3 +1,6 @@
+// Copyright 2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
 package totp
 
 import (
@@ -32,6 +35,8 @@ func TestGenerateOTPCode(t *testing.T) {
 	}
 
 	for _, test := range tests {
+		// This line can be removed when we upgrade golangci-lint to 1.58.1 and go1.22+
+		test := test
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 

From 0cdbb710d27ef2bb576730a1ab2e0a20ebc0f61d Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Thu, 9 May 2024 13:29:41 -0700
Subject: [PATCH 7/7] add test for github redirect in auth_handler_test.go

Co-authored-by: Joshua Casey <joshuatcasey@gmail.com>
---
 .../endpoints/auth/auth_handler_test.go       | 62 +++++++++++++++++++
 internal/testutil/totp/totp_test.go           |  1 -
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/internal/federationdomain/endpoints/auth/auth_handler_test.go b/internal/federationdomain/endpoints/auth/auth_handler_test.go
index 52cd06b29..b805a3fbd 100644
--- a/internal/federationdomain/endpoints/auth/auth_handler_test.go
+++ b/internal/federationdomain/endpoints/auth/auth_handler_test.go
@@ -55,6 +55,8 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 		ldapUpstreamResourceUID              = "ldap-resource-uid"
 		activeDirectoryUpstreamName          = "some-active-directory-idp"
 		activeDirectoryUpstreamResourceUID   = "active-directory-resource-uid"
+		githubUpstreamName                   = "some-github-idp"
+		githubUpstreamResourceUID            = "github-resource-uid"
 
 		oidcUpstreamIssuer                    = "https://my-upstream-issuer.com"
 		oidcUpstreamSubject                   = "abc123-some guid" // has a space character which should get escaped in URL
@@ -291,6 +293,15 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 			WithPasswordGrantError(errors.New("should not have used password grant on this instance"))
 	}
 
+	upstreamGitHubIdentityProviderBuilder := func() *oidctestutil.TestUpstreamGitHubIdentityProviderBuilder {
+		return oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder().
+			WithName(githubUpstreamName).
+			WithResourceUID(githubUpstreamResourceUID).
+			WithClientID("some-github-client-id").
+			WithAuthorizationURL(upstreamAuthURL.String()).
+			WithScopes([]string{"scope1", "scope2"}) // the scopes to request when starting the upstream authorization flow
+	}
+
 	passwordGrantUpstreamOIDCIdentityProviderBuilder := func() *oidctestutil.TestUpstreamOIDCIdentityProviderBuilder {
 		return oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
 			WithName(oidcPasswordGrantUpstreamName).
@@ -463,6 +474,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 	happyGetRequestPathForOIDCPasswordGrantUpstream := modifiedHappyGetRequestPath(map[string]string{"pinniped_idp_name": oidcPasswordGrantUpstreamName})
 	happyGetRequestPathForLDAPUpstream := modifiedHappyGetRequestPath(map[string]string{"pinniped_idp_name": ldapUpstreamName})
 	happyGetRequestPathForADUpstream := modifiedHappyGetRequestPath(map[string]string{"pinniped_idp_name": activeDirectoryUpstreamName})
+	happyGetRequestPathForGithubUpstream := modifiedHappyGetRequestPath(map[string]string{"pinniped_idp_name": githubUpstreamName})
 
 	modifiedHappyGetRequestPathForOIDCUpstream := func(queryOverrides map[string]string) string {
 		queryOverrides["pinniped_idp_name"] = oidcUpstreamName
@@ -480,6 +492,10 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 		queryOverrides["pinniped_idp_name"] = activeDirectoryUpstreamName
 		return modifiedHappyGetRequestPath(queryOverrides)
 	}
+	modifiedHappyGetRequestPathForGithubUpstream := func(queryOverrides map[string]string) string {
+		queryOverrides["pinniped_idp_name"] = githubUpstreamName
+		return modifiedHappyGetRequestPath(queryOverrides)
+	}
 
 	happyGetRequestQueryMapForOIDCUpstream := modifiedQueryMap(happyGetRequestQueryMap, map[string]string{"pinniped_idp_name": oidcUpstreamName})
 	happyGetRequestQueryMapForOIDCPasswordGrantUpstream := modifiedQueryMap(happyGetRequestQueryMap, map[string]string{"pinniped_idp_name": oidcPasswordGrantUpstreamName})
@@ -533,6 +549,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 		return urlWithQuery(upstreamAuthURL.String(), query)
 	}
 
+	expectedRedirectLocationForUpstreamGithub := func(expectedUpstreamState string) string {
+		query := map[string]string{
+			"response_type": "code",
+			"scope":         "scope1 scope2",
+			"client_id":     "some-github-client-id",
+			"state":         expectedUpstreamState,
+			"redirect_uri":  downstreamIssuer + "/callback",
+		}
+		return urlWithQuery(upstreamAuthURL.String(), query)
+	}
+
 	expectedHappyActiveDirectoryUpstreamCustomSession := &psession.CustomSessionData{
 		Username:         happyLDAPUsernameFromAuthenticator,
 		UpstreamUsername: happyLDAPUsernameFromAuthenticator,
@@ -711,6 +738,41 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 			wantUpstreamStateParamInLocationHeader: true,
 			wantBodyStringWithLocationInHref:       true,
 		},
+		{
+			name:                                   "GitHub upstream browser flow happy path using GET without a CSRF cookie",
+			idps:                                   testidplister.NewUpstreamIDPListerBuilder().WithGitHub(upstreamGitHubIdentityProviderBuilder().Build()),
+			generateCSRF:                           happyCSRFGenerator,
+			generatePKCE:                           happyPKCEGenerator,
+			generateNonce:                          happyNonceGenerator,
+			stateEncoder:                           happyStateEncoder,
+			cookieEncoder:                          happyCookieEncoder,
+			method:                                 http.MethodGet,
+			path:                                   happyGetRequestPathForGithubUpstream,
+			wantStatus:                             http.StatusSeeOther,
+			wantContentType:                        htmlContentType,
+			wantCSRFValueInCookieHeader:            happyCSRF,
+			wantLocationHeader:                     expectedRedirectLocationForUpstreamGithub(expectedUpstreamStateParam(nil, "", githubUpstreamName, "github")),
+			wantUpstreamStateParamInLocationHeader: true,
+			wantBodyStringWithLocationInHref:       true,
+		},
+		{
+			name:                                   "GitHub upstream browser flow happy path using GET without a CSRF cookie using a dynamic client",
+			idps:                                   testidplister.NewUpstreamIDPListerBuilder().WithGitHub(upstreamGitHubIdentityProviderBuilder().Build()),
+			kubeResources:                          addFullyCapableDynamicClientAndSecretToKubeResources,
+			generateCSRF:                           happyCSRFGenerator,
+			generatePKCE:                           happyPKCEGenerator,
+			generateNonce:                          happyNonceGenerator,
+			stateEncoder:                           happyStateEncoder,
+			cookieEncoder:                          happyCookieEncoder,
+			method:                                 http.MethodGet,
+			path:                                   modifiedHappyGetRequestPathForGithubUpstream(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}),
+			wantStatus:                             http.StatusSeeOther,
+			wantContentType:                        htmlContentType,
+			wantCSRFValueInCookieHeader:            happyCSRF,
+			wantLocationHeader:                     expectedRedirectLocationForUpstreamGithub(expectedUpstreamStateParam(map[string]string{"client_id": dynamicClientID, "scope": testutil.AllDynamicClientScopesSpaceSep}, "", githubUpstreamName, "github")),
+			wantUpstreamStateParamInLocationHeader: true,
+			wantBodyStringWithLocationInHref:       true,
+		},
 		{
 			name:                                   "LDAP upstream browser flow happy path using GET without a CSRF cookie",
 			idps:                                   testidplister.NewUpstreamIDPListerBuilder().WithLDAP(upstreamLDAPIdentityProviderBuilder().Build()),
diff --git a/internal/testutil/totp/totp_test.go b/internal/testutil/totp/totp_test.go
index 3fa0320b3..52d4ff197 100644
--- a/internal/testutil/totp/totp_test.go
+++ b/internal/testutil/totp/totp_test.go
@@ -35,7 +35,6 @@ func TestGenerateOTPCode(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		// This line can be removed when we upgrade golangci-lint to 1.58.1 and go1.22+
 		test := test
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()