From cd86d57763459e253ccb88b9296e2e41b2619e8f Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Thu, 25 Apr 2024 11:07:55 -0400 Subject: [PATCH] review cleanup, remove TODOs --- .../types_supervisor_idp_discovery.go.tmpl | 1 + cmd/pinniped/cmd/login_oidc.go | 3 +-- .../github_upstream_watcher_test.go | 4 +-- .../resolved_github_provider.go | 27 +++++++------------ .../resolved_github_provider_test.go | 2 ++ .../resolvedldap/resolved_ldap_provider.go | 12 +++------ .../upstreamprovider/upsteam_provider.go | 1 - .../authorizationcode/authorizationcode.go | 1 - internal/psession/pinniped_session.go | 1 - .../oidctestutil/testgithubprovider.go | 1 - .../testutil/testidplister/testidplister.go | 10 +------ internal/upstreamgithub/upstreamgithub.go | 13 --------- 12 files changed, 18 insertions(+), 58 deletions(-) diff --git a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl b/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl index 303acdbfb..1b16bfe90 100644 --- a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl +++ b/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" ) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 7d73ab9ee..4369d12ab 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/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, idpdiscoveryv1alpha1.IDPTypeGitHub: + case idpdiscoveryv1alpha1.IDPTypeOIDC: switch requestedFlow { case idpdiscoveryv1alpha1.IDPFlowCLIPassword: return useCLIFlow, nil diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index 92562b641..fa8401714 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/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) diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index faca23e6b..4b17120ef 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/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 - 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 + return []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode} } 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") } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index 274144d56..be58da09f 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/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. diff --git a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go index d993ec12f..a700ae4fc 100644 --- a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go +++ b/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 - fallthrough - case psession.ProviderTypeGitHub: // 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 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 - fallthrough - case psession.ProviderTypeGitHub: // 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 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 - fallthrough - case psession.ProviderTypeGitHub: // 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 default: // This shouldn't really happen. diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upsteam_provider.go index 4717fb01c..ab5920bdf 100644 --- a/internal/federationdomain/upstreamprovider/upsteam_provider.go +++ b/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 diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index f43437081..cf3be23ef 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/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 = `{ diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index a1f34a11b..d825ce037 100644 --- a/internal/psession/pinniped_session.go +++ b/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 { } diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go index 5b985815b..83c1825cd 100644 --- a/internal/testutil/oidctestutil/testgithubprovider.go +++ b/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 diff --git a/internal/testutil/testidplister/testidplister.go b/internal/testutil/testidplister/testidplister.go index 8195949e0..13b94b29f 100644 --- a/internal/testutil/testidplister/testidplister.go +++ b/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()", ) diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go index 678ab0ae1..88fa1c79a 100644 --- a/internal/upstreamgithub/upstreamgithub.go +++ b/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 -}