review cleanup, remove TODOs

apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl

@@ -16,6 +16,7 @@ const (
 	IDPTypeLDAP            IDPType = "ldap"
 	IDPTypeActiveDirectory IDPType = "activedirectory"
 	IDPTypeGitHub          IDPType = "github"
+
 	IDPFlowCLIPassword     IDPFlow = "cli_password"
 	IDPFlowBrowserAuthcode IDPFlow = "browser_authcode"
 )

cmd/pinniped/cmd/login_oidc.go

@@ -304,8 +304,7 @@ func flowOptions(
 	}
 
 	switch requestedIDPType {
-	// TODO: Decide if we can bundle GitHub here long term or if it needs its own case
+	case idpdiscoveryv1alpha1.IDPTypeOIDC:
-	case idpdiscoveryv1alpha1.IDPTypeOIDC, idpdiscoveryv1alpha1.IDPTypeGitHub:
 		switch requestedFlow {
 		case idpdiscoveryv1alpha1.IDPFlowCLIPassword:
 			return useCLIFlow, nil

internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go

@@ -371,8 +371,7 @@ func TestController(t *testing.T) {
 		wantErr                 string
 		wantLogs                []string
 		wantResultingCache      []*upstreamgithub.ProviderConfig
-		// wantResultingCache      []*oidctestutil.TestUpstreamGitHubIdentityProvider
+		wantResultingUpstreams  []v1alpha1.GitHubIdentityProvider
-		wantResultingUpstreams []v1alpha1.GitHubIdentityProvider
 	}{
 		{
 			name: "no GitHubIdentityProviders",
@@ -1763,7 +1762,6 @@ func TestController(t *testing.T) {
 				var actualIDP *upstreamgithub.Provider
 				for _, possibleIDP := range actualIDPList {
 					if possibleIDP.GetName() == tt.wantResultingCache[i].Name {
-						// For this check, we know that the actual IDPs are going to have type upstreamgithub.ProviderConfig
 						var ok bool
 						actualIDP, ok = possibleIDP.(*upstreamgithub.Provider)
 						require.True(t, ok)

internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go

@@ -5,6 +5,7 @@ package resolvedgithub
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
@@ -45,13 +46,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryType() v
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryFlows() []v1alpha1.IDPFlow {
-	// TODO: review and see if this is actually true to follow the OIDC model
+	return []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode}
-	flows := []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode}
-	// TODO: coming as a later feature?  The UpstreamGithubIdentityProviderI does not currently impl this func
-	// if p.Provider.AllowsPasswordGrant() {
-	// 	flows = append(flows, v1alpha1.IDPFlowCLIPassword)
-	// }
-	return flows
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) GetTransforms() *idtransform.TransformationPipeline {
@@ -73,38 +68,34 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedire
 	state *resolvedprovider.UpstreamAuthorizeRequestState,
 	downstreamIssuerURL string,
 ) (string, error) {
-	// TODO: implement
 	fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamAuthorizeRedirectURL() called with state: %#v, downstreamIssuerURL %s", state, downstreamIssuerURL)
-	return "", nil
+	return "", errors.New("function UpstreamAuthorizeRedirectURL not yet implemented for GitHub IDP")
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) Login(
-	ctx context.Context, //nolint:all
+	_ context.Context,
 	submittedUsername string,
 	submittedPassword string,
 ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
-	// TODO: implement
 	fmt.Printf("GithubResolvedIdentityProvider ~ Login() called with submittedUserName %s, submittedPassword %s", submittedUsername, submittedPassword)
-	return nil, nil, nil
+	return nil, nil, errors.New("function Login not yet implemented for GitHub IDP")
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback(
-	ctx context.Context, //nolint:all
+	_ context.Context,
 	authCode string,
 	pkce pkce.Code,
 	nonce nonce.Nonce,
 	redirectURI string,
 ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
-	// TODO: implement
 	fmt.Printf("GithubResolvedIdentityProvider ~ LoginFromCallback() called with authCode: %s, pkce: %#v, nonce: %#v, redirectURI: %s", authCode, pkce, nonce, redirectURI)
-	return nil, nil, nil
+	return nil, nil, errors.New("function LoginFromCallback not yet implemented for GitHub IDP")
 }
 
 func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh(
-	ctx context.Context, //nolint:all
+	_ context.Context,
 	identity *resolvedprovider.Identity,
 ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) {
-	// TODO: implement
 	fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamRefresh() called with identity %#v", identity)
-	return nil, nil
+	return nil, errors.New("function UpstreamRefresh not yet implemented for GitHub IDP")
 }

internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go

@@ -2,3 +2,5 @@
 // SPDX-License-Identifier: Apache-2.0
 
 package resolvedgithub
+
+// TODO: write some tests.

internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go

@@ -76,9 +76,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) CloneIDPSpecificSessionDa
 			return nil
 		}
 		return session.ActiveDirectory.Clone()
-	case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases
+	case psession.ProviderTypeOIDC, psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases
-		fallthrough
-	case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases
 		fallthrough
 	default:
 		return nil
@@ -153,9 +151,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login(
 			UserDN:                 authenticateResponse.DN,
 			ExtraRefreshAttributes: authenticateResponse.ExtraRefreshAttributes,
 		}
-	case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases
+	case psession.ProviderTypeOIDC, psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases
-		fallthrough
-	case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases
 		fallthrough
 	default:
 		return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(fmt.Errorf("unexpected provider type %q", p.GetSessionProviderType()))
@@ -209,9 +205,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh(
 		}
 		dn = sessionData.UserDN
 		additionalAttributes = sessionData.ExtraRefreshAttributes
-	case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases
+	case psession.ProviderTypeOIDC, psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases
-		fallthrough
-	case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases
 		fallthrough
 	default:
 		// This shouldn't really happen.

internal/federationdomain/upstreamprovider/upsteam_provider.go

@@ -128,7 +128,6 @@ type UpstreamLDAPIdentityProviderI interface {
 	PerformRefresh(ctx context.Context, storedRefreshAttributes RefreshAttributes, idpDisplayName string) (groups []string, err error)
 }
 
-// TODO: Impl this interface thoroughly to support GitHub login.
 type UpstreamGithubIdentityProviderI interface {
 	UpstreamIdentityProviderI
 

internal/fositestorage/authorizationcode/authorizationcode.go

@@ -190,7 +190,6 @@ func (e *errSerializationFailureWithCause) Error() string {
 	return fmt.Sprintf("%s: %s", fosite.ErrSerializationFailure, e.cause)
 }
 
-// TODO: need to revisit this, there is a unit test now failing.
 // ExpectedAuthorizeCodeSessionJSONFromFuzzing is used for round tripping tests.
 // It is exported to allow integration tests to use it.
 const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{

internal/psession/pinniped_session.go

@@ -144,7 +144,6 @@ func (s *ActiveDirectorySessionData) Clone() *ActiveDirectorySessionData {
 	}
 }
 
-// TODO: flesh this out, GitHub will need additional data.
 type GitHubSessionData struct {
 }
 

internal/testutil/oidctestutil/testgithubprovider.go

@@ -105,7 +105,6 @@ func NewTestUpstreamGitHubIdentityProviderBuilder() *TestUpstreamGitHubIdentityP
 	return &TestUpstreamGitHubIdentityProviderBuilder{}
 }
 
-// TODO: flesh this out.
 type TestUpstreamGitHubIdentityProvider struct {
 	Name                           string
 	ClientID                       string

internal/testutil/testidplister/testidplister.go

@@ -243,8 +243,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToPasswordCredentialsGra
 			actualArgs = upstreamOIDC.PasswordCredentialsGrantAndValidateTokensArgs(0)
 		}
 	}
-	// TODO: probably add GitHub loop once we flesh out the structs
-
 	require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams,
 		"should have been exactly one call to PasswordCredentialsGrantAndValidateTokens() by all OIDC upstreams",
 	)
@@ -260,7 +258,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToPasswordCredentialsG
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
 		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.PasswordCredentialsGrantAndValidateTokensCallCount()
 	}
-	// TODO: probably add GitHub loop once we flesh out the structs
 	require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams,
 		"expected exactly zero calls to PasswordCredentialsGrantAndValidateTokens()",
 	)
@@ -283,7 +280,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToExchangeAuthcodeAndVal
 			actualArgs = upstreamOIDC.ExchangeAuthcodeAndValidateTokensArgs(0)
 		}
 	}
-	// TODO: probably add GitHub loop once we flesh out the structs
 	require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams,
 		"should have been exactly one call to ExchangeAuthcodeAndValidateTokens() by all OIDC upstreams",
 	)
@@ -299,7 +295,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToExchangeAuthcodeAndV
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
 		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.ExchangeAuthcodeAndValidateTokensCallCount()
 	}
-	// TODO: probably add GitHub loop once we flesh out the structs
+
 	require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams,
 		"expected exactly zero calls to ExchangeAuthcodeAndValidateTokens()",
 	)
@@ -384,7 +380,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken(
 			actualArgs = upstreamOIDC.ValidateTokenAndMergeWithUserInfoArgs(0)
 		}
 	}
-	// TODO: probably add GitHub loop once we flesh out the structs
 	require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams,
 		"should have been exactly one call to ValidateTokenAndMergeWithUserInfo() by all OIDC upstreams",
 	)
@@ -400,7 +395,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
 		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.ValidateTokenAndMergeWithUserInfoCallCount()
 	}
-	// TODO: probably add GitHub loop once we flesh out structs
 	require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams,
 		"expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()",
 	)
@@ -423,7 +417,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeToken(
 			actualArgs = upstreamOIDC.RevokeTokenArgs(0)
 		}
 	}
-	// TODO: probably add GitHub loop once we flesh out structs
 	require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams,
 		"should have been exactly one call to RevokeToken() by all OIDC upstreams",
 	)
@@ -439,7 +432,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeToken(t *testi
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
 		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.RevokeTokenCallCount()
 	}
-	// TODO: probably add GitHub loop once we flesh out structs
 	require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams,
 		"expected exactly zero calls to RevokeToken()",
 	)

internal/upstreamgithub/upstreamgithub.go

@@ -5,15 +5,12 @@
 package upstreamgithub
 
 import (
-	"context"
 	"net/http"
 
 	"golang.org/x/oauth2"
-
 	"k8s.io/apimachinery/pkg/types"
 
 	"go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
-	"go.pinniped.dev/internal/authenticators"
 	"go.pinniped.dev/internal/federationdomain/upstreamprovider"
 )
 
@@ -36,7 +33,6 @@ type Provider struct {
 }
 
 var _ upstreamprovider.UpstreamGithubIdentityProviderI = &Provider{}
-var _ authenticators.UserAuthenticator = &Provider{}
 
 // New creates a Provider. The config is not a pointer to ensure that a copy of the config is created,
 // making the resulting Provider use an effectively read-only configuration.
@@ -93,12 +89,3 @@ func (p *Provider) GetAuthorizationURL() string {
 func (p *Provider) GetHttpClient() *http.Client {
 	return p.c.HttpClient
 }
-
-// AuthenticateUser authenticates an end user and returns their mapped username, groups, and UID. Implements authenticators.UserAuthenticator.
-func (p *Provider) AuthenticateUser(
-	ctx context.Context,       //nolint:all
-	username, password string, //nolint:all
-) (*authenticators.Response, bool, error) {
-	// TODO: implement this, currently just placeholder to satisfy UserAuthenticator interface above
-	return nil, false, nil
-}