From 858356610c0884d16a771ae6670a1c9119120198 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 4 Dec 2020 15:40:17 -0800 Subject: [PATCH] Make assertions about how many secrets were stored by fosite in tests In both callback_handler_test.go and token_handler_test.go Signed-off-by: Aram Price --- internal/crud/crud.go | 9 ++-- .../fositestorage/accesstoken/accesstoken.go | 4 +- .../authorizationcode/authorizationcode.go | 4 +- .../openidconnect/openidconnect.go | 4 +- internal/fositestorage/pkce/pkce.go | 4 +- .../refreshtoken/refreshtoken.go | 4 +- .../oidc/callback/callback_handler_test.go | 42 +++++------------ internal/oidc/token/token_handler_test.go | 46 ++++++++++++++++--- internal/testutil/assertions.go | 13 ++++++ 9 files changed, 84 insertions(+), 46 deletions(-) diff --git a/internal/crud/crud.go b/internal/crud/crud.go index e04c5403..77160e52 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -22,8 +22,9 @@ import ( //nolint:gosec // ignore lint warnings that these are credentials const ( + SecretLabelKey = "storage.pinniped.dev/type" + secretNameFormat = "pinniped-storage-%s-%s" - secretLabelKey = "storage.pinniped.dev/type" secretTypeFormat = "storage.pinniped.dev/%s" secretVersion = "1" secretDataKey = "pinniped-storage-data" @@ -90,7 +91,7 @@ func (s *secretsStorage) validateSecret(secret *corev1.Secret) error { if secret.Type != s.secretType { return fmt.Errorf("%w: %s must equal %s", ErrSecretTypeMismatch, secret.Type, s.secretType) } - if labelResource := secret.Labels[secretLabelKey]; labelResource != s.resource { + if labelResource := secret.Labels[SecretLabelKey]; labelResource != s.resource { return fmt.Errorf("%w: %s must equal %s", ErrSecretLabelMismatch, labelResource, s.resource) } if !bytes.Equal(secret.Data[secretVersionKey], s.secretVersion) { @@ -121,7 +122,7 @@ func (s *secretsStorage) Delete(ctx context.Context, signature string) error { func (s *secretsStorage) DeleteByLabel(ctx context.Context, labelName string, labelValue string) error { list, err := s.secrets.List(ctx, metav1.ListOptions{ LabelSelector: labels.Set{ - secretLabelKey: s.resource, + SecretLabelKey: s.resource, labelName: labelValue, }.String(), }) @@ -158,7 +159,7 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, } labels := map[string]string{ - secretLabelKey: s.resource, // make it easier to find this stuff via kubectl + SecretLabelKey: s.resource, // make it easier to find this stuff via kubectl } for labelName, labelValue := range additionalLabels { labels[labelName] = labelValue diff --git a/internal/fositestorage/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go index 15623272..39e63f2b 100644 --- a/internal/fositestorage/accesstoken/accesstoken.go +++ b/internal/fositestorage/accesstoken/accesstoken.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "access-token" + ErrInvalidAccessTokenRequestVersion = constable.Error("access token request data has wrong version") ErrInvalidAccessTokenRequestData = constable.Error("access token request data must be present") @@ -42,7 +44,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) RevocationStorage { - return &accessTokenStorage{storage: crud.New("access-token", secrets)} + return &accessTokenStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *accessTokenStorage) RevokeAccessToken(ctx context.Context, requestID string) error { diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 6425804d..fc4cb1e7 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -20,6 +20,8 @@ import ( ) const ( + TypeLabelValue = "authcode" + ErrInvalidAuthorizeRequestData = constable.Error("authorization request data must be present") ErrInvalidAuthorizeRequestVersion = constable.Error("authorization request data has wrong version") @@ -39,7 +41,7 @@ type AuthorizeCodeSession struct { } func New(secrets corev1client.SecretInterface) oauth2.AuthorizeCodeStorage { - return &authorizeCodeStorage{storage: crud.New("authcode", secrets)} + return &authorizeCodeStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, signature string, requester fosite.Requester) error { diff --git a/internal/fositestorage/openidconnect/openidconnect.go b/internal/fositestorage/openidconnect/openidconnect.go index 51fd84a8..932e7d35 100644 --- a/internal/fositestorage/openidconnect/openidconnect.go +++ b/internal/fositestorage/openidconnect/openidconnect.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "oidc" + ErrInvalidOIDCRequestVersion = constable.Error("oidc request data has wrong version") ErrInvalidOIDCRequestData = constable.Error("oidc request data must be present") ErrMalformedAuthorizationCode = constable.Error("malformed authorization code") @@ -38,7 +40,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) openid.OpenIDConnectRequestStorage { - return &openIDConnectRequestStorage{storage: crud.New("oidc", secrets)} + return &openIDConnectRequestStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *openIDConnectRequestStorage) CreateOpenIDConnectSession(ctx context.Context, authcode string, requester fosite.Requester) error { diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go index ed82d0df..24d554d2 100644 --- a/internal/fositestorage/pkce/pkce.go +++ b/internal/fositestorage/pkce/pkce.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "pkce" + ErrInvalidPKCERequestVersion = constable.Error("pkce request data has wrong version") ErrInvalidPKCERequestData = constable.Error("pkce request data must be present") @@ -37,7 +39,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) pkce.PKCERequestStorage { - return &pkceStorage{storage: crud.New("pkce", secrets)} + return &pkceStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error { diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index 15f76539..5cccbf74 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "refresh-token" + ErrInvalidRefreshTokenRequestVersion = constable.Error("refresh token request data has wrong version") ErrInvalidRefreshTokenRequestData = constable.Error("refresh token request data must be present") @@ -42,7 +44,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) RevocationStorage { - return &refreshTokenStorage{storage: crud.New("refresh-token", secrets)} + return &refreshTokenStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index d73fb097..5a0e8c6c 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -18,18 +18,20 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes/fake" - kubetesting "k8s.io/client-go/testing" + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/fositestorage/openidconnect" + "go.pinniped.dev/internal/fositestorage/pkce" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/oidctestutil" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/oidctypes" - "go.pinniped.dev/pkg/oidcclient/pkce" + oidcpkce "go.pinniped.dev/pkg/oidcclient/pkce" ) const ( @@ -106,7 +108,7 @@ func TestCallbackEndpoint(t *testing.T) { happyExchangeAndValidateTokensArgs := &oidctestutil.ExchangeAuthcodeAndValidateTokenArgs{ Authcode: happyUpstreamAuthcode, - PKCECodeVerifier: pkce.Code(happyDownstreamPKCE), + PKCECodeVerifier: oidcpkce.Code(happyDownstreamPKCE), ExpectedIDTokenNonce: nonce.Nonce(happyDownstreamNonce), RedirectURI: happyUpstreamRedirectURI, } @@ -484,18 +486,8 @@ func TestCallbackEndpoint(t *testing.T) { } require.Len(t, client.Actions(), expectedNumberOfCreatedSecrets) - actualSecretNames := []string{} - for i := range client.Actions() { - actualAction := client.Actions()[i].(kubetesting.CreateActionImpl) - require.Equal(t, "create", actualAction.GetVerb()) - require.Equal(t, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, actualAction.GetResource()) - actualSecret := actualAction.GetObject().(*corev1.Secret) - require.Empty(t, actualSecret.Namespace) // because the secrets client is already scoped to a namespace - actualSecretNames = append(actualSecretNames, actualSecret.Name) - } - // One authcode should have been stored. - requireAnyStringHasPrefix(t, actualSecretNames, "pinniped-storage-authcode-") + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) storedRequestFromAuthcode, storedSessionFromAuthcode := validateAuthcodeStorage( t, @@ -508,7 +500,7 @@ func TestCallbackEndpoint(t *testing.T) { ) // One PKCE should have been stored. - requireAnyStringHasPrefix(t, actualSecretNames, "pinniped-storage-pkce-") + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: pkce.TypeLabelValue}, 1) validatePKCEStorage( t, @@ -522,7 +514,7 @@ func TestCallbackEndpoint(t *testing.T) { // One IDSession should have been stored, if the downstream actually requested the "openid" scope if test.wantGrantedOpenidScope { - requireAnyStringHasPrefix(t, actualSecretNames, "pinniped-storage-oidc") + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) validateIDSessionStorage( t, @@ -684,7 +676,7 @@ func (u *upstreamOIDCIdentityProviderBuilder) Build() oidctestutil.TestUpstreamO UsernameClaim: u.usernameClaim, GroupsClaim: u.groupsClaim, Scopes: []string{"scope1", "scope2"}, - ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce) (oidctypes.Token, map[string]interface{}, error) { + ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier oidcpkce.Code, expectedIDTokenNonce nonce.Nonce) (oidctypes.Token, map[string]interface{}, error) { return oidctypes.Token{}, u.idToken, u.authcodeExchangeErr }, } @@ -847,15 +839,3 @@ func castStoredAuthorizeRequest(t *testing.T, storedAuthorizeRequest fosite.Requ return storedRequest, storedSession } - -func requireAnyStringHasPrefix(t *testing.T, stringList []string, prefix string) { - t.Helper() - - containsPrefix := false - for i := range stringList { - if strings.HasPrefix(stringList[i], prefix) { - containsPrefix = true - } - } - require.Truef(t, containsPrefix, "list %v did not contain any strings with prefix %s", stringList, prefix) -} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index ba7a0252..a56a649c 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -26,8 +26,15 @@ import ( "github.com/pkg/errors" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" + "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes/fake" + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/fositestorage/accesstoken" + "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/fositestorage/openidconnect" + storagepkce "go.pinniped.dev/internal/fositestorage/pkce" + "go.pinniped.dev/internal/fositestorage/refreshtoken" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/jwks" @@ -394,7 +401,14 @@ func TestTokenEndpoint(t *testing.T) { } subject := NewHandler(oauthHelper) - // TODO add assertions about how many of each storage type exist at this point as a pre-condition + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 1) + if strings.Contains(authRequest.Form.Get("scope"), "openid") { + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) + } else { + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) + } req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -421,11 +435,18 @@ func TestTokenEndpoint(t *testing.T) { requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) + if wantOpenidScope { requireValidIDToken(t, m, jwtSigningKey) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) + } else { + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) } - - // TODO add assertions about how many of each storage type are remaining at this point } else { require.JSONEq(t, test.wantExactBody, rsp.Body.String()) } @@ -442,7 +463,10 @@ func TestTokenEndpoint(t *testing.T) { oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) subject := NewHandler(oauthHelper) - // TODO add assertions about how many of each storage type exist at this point as a pre-condition + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -469,7 +493,12 @@ func TestTokenEndpoint(t *testing.T) { requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) requireValidIDToken(t, m, jwtSigningKey) - // TODO add assertions about how many of each storage type are remaining at this point + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) // Second call - should be unsuccessful since auth code was already used. // @@ -488,7 +517,12 @@ func TestTokenEndpoint(t *testing.T) { requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) - // TODO add assertions about how many of each storage type are remaining at this point + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) }) } diff --git a/internal/testutil/assertions.go b/internal/testutil/assertions.go index e49233d5..53f5f73a 100644 --- a/internal/testutil/assertions.go +++ b/internal/testutil/assertions.go @@ -4,11 +4,15 @@ package testutil import ( + "context" "mime" "testing" "time" "github.com/stretchr/testify/require" + v12 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + v1 "k8s.io/client-go/kubernetes/typed/core/v1" ) func RequireTimeInDelta(t *testing.T, t1 time.Time, t2 time.Time, delta time.Duration) { @@ -39,3 +43,12 @@ func RequireEqualContentType(t *testing.T, actual string, expected string) { require.Equal(t, actualContentType, expectedContentType) require.Equal(t, actualContentTypeParams, expectedContentTypeParams) } + +func RequireNumberOfSecretsMatchingLabelSelector(t *testing.T, secrets v1.SecretInterface, labelSet labels.Set, expectedNumberOfSecrets int) { + t.Helper() + storedAuthcodeSecrets, err := secrets.List(context.Background(), v12.ListOptions{ + LabelSelector: labelSet.String(), + }) + require.NoError(t, err) + require.Len(t, storedAuthcodeSecrets.Items, expectedNumberOfSecrets) +}