Add unit tests for clientsecretrequest

Co-authored-by: Ryan Richard <richardry@vmware.com>
Co-authored-by: Benjamin A. Petersen <ben@benjaminapetersen.me>

internal/oidcclientsecretstorage/oidcclientsecretstorage_test.go

@@ -265,38 +265,6 @@ func TestSet(t *testing.T) {
 			},
 			wantErr: "failed to create client secret for uid some-example-uid1: failed to create oidc-client-secret for signature c29tZS1leGFtcGxlLXVpZDE: some create error",
 		},
-		{
-			name:           "conflict: editing wrong resource version",
-			rv:             "22",
-			oidcClientName: "some-client",
-			oidcClientUID:  types.UID("some-example-uid1"),
-			hashes:         []string{"foo", "bar"},
-			seedSecret: &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq",
-					Namespace: namespace,
-					Labels: map[string]string{
-						"storage.pinniped.dev/type": "oidc-client-secret",
-					},
-					ResourceVersion: "23",
-					OwnerReferences: []metav1.OwnerReference{{
-						APIVersion: "config.supervisor.pinniped.dev/v1alpha1",
-						Kind:       "OIDCClient",
-						Name:       "some-client",
-						UID:        "some-example-uid1",
-					}},
-				},
-				Type: "storage.pinniped.dev/oidc-client-secret",
-				Data: map[string][]byte{
-					"pinniped-storage-data":    []byte(`{"hashes":["foo"],"version":"1"}`),
-					"pinniped-storage-version": []byte("1"),
-				},
-			},
-			wantActions: []coretesting.Action{
-				coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq"),
-			},
-			wantErr: "failed to update client secret for uid some-example-uid1: Operation cannot be fulfilled on Secret \"pinniped-storage-oidc-client-secret-onxw2zjnmv4gc3lqnrss25ljmqyq\": resource version 23 does not match expected value: 22",
-		},
 		{
 			name:           "failed to update",
 			rv:             "23",
@@ -326,7 +294,7 @@ func TestSet(t *testing.T) {
 			},
 			addReactors: func(clientSet *fake.Clientset) {
 				clientSet.PrependReactor("update", "secrets", func(action coretesting.Action) (bool, runtime.Object, error) {
-					return true, nil, errors.New("some update error")
+					return true, nil, errors.New("some update error maybe a conflict or something else")
 				})
 			},
 			wantActions: []coretesting.Action{
@@ -352,7 +320,7 @@ func TestSet(t *testing.T) {
 					},
 				}),
 			},
-			wantErr: "failed to update client secret for uid some-example-uid1: failed to update oidc-client-secret for signature c29tZS1leGFtcGxlLXVpZDE at resource version 23: some update error",
+			wantErr: "failed to update client secret for uid some-example-uid1: failed to update oidc-client-secret for signature c29tZS1leGFtcGxlLXVpZDE at resource version 23: some update error maybe a conflict or something else",
 		},
 	}
 

internal/registry/clientsecretrequest/rest.go

@@ -11,7 +11,6 @@ import (
 	"io"
 	"strings"
 
-	"golang.org/x/crypto/bcrypt"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	genericvalidation "k8s.io/apimachinery/pkg/api/validation"
@@ -35,23 +34,36 @@ import (
 // This value is expected to be increased over time to match CPU improvements.
 const Cost = 12
 
-func NewREST(resource schema.GroupResource, secrets corev1client.SecretInterface, clients configv1alpha1clientset.OIDCClientInterface, namespace string, cost int) *REST {
+type byteHasher func(password []byte, cost int) ([]byte, error)
+
+func NewREST(
+	resource schema.GroupResource,
+	secretsClient corev1client.SecretInterface,
+	oidcClientsClient configv1alpha1clientset.OIDCClientInterface,
+	namespace string,
+	cost int,
+	randByteGenerator io.Reader,
+	byteHasher byteHasher,
+) *REST {
 	return &REST{
-		secretStorage:  oidcclientsecretstorage.New(secrets),
+		secretStorage:     oidcclientsecretstorage.New(secretsClient),
-		clients:        clients,
+		oidcClientsClient: oidcClientsClient,
-		namespace:      namespace,
+		namespace:         namespace,
-		cost:           cost,
+		cost:              cost,
-		tableConvertor: rest.NewDefaultTableConvertor(resource),
+		randByteGenerator: randByteGenerator,
+		byteHasher:        byteHasher,
+		tableConvertor:    rest.NewDefaultTableConvertor(resource),
 	}
 }
 
 type REST struct {
-	secretStorage  *oidcclientsecretstorage.OIDCClientSecretStorage
+	secretStorage     *oidcclientsecretstorage.OIDCClientSecretStorage
-	clients        configv1alpha1clientset.OIDCClientInterface
+	oidcClientsClient configv1alpha1clientset.OIDCClientInterface
-	namespace      string
+	namespace         string
-	rand           io.Reader
+	randByteGenerator io.Reader
-	cost           int
+	cost              int
-	tableConvertor rest.TableConvertor
+	byteHasher        byteHasher
+	tableConvertor    rest.TableConvertor
 }
 
 // Assert that our *REST implements all the optional interfaces that we expect it to implement.
@@ -114,17 +126,17 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 	t.Step("validateRequest")
 
 	// Find the specified OIDCClient.
-	oidcClient, err := r.clients.Get(ctx, req.Name, metav1.GetOptions{})
+	oidcClient, err := r.oidcClientsClient.Get(ctx, req.Name, metav1.GetOptions{})
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			traceValidationFailure(t, fmt.Sprintf("client %q does not exist", req.Name))
 			errs := field.ErrorList{field.NotFound(field.NewPath("metadata", "name"), req.Name)}
 			return nil, apierrors.NewInvalid(kindFromContext(ctx), req.Name, errs)
 		}
-		traceFailureWithError(t, "clients.Get", err)
+		traceFailureWithError(t, "oidcClientsClient.Get", err)
 		return nil, apierrors.NewInternalError(fmt.Errorf("getting client %q failed", req.Name))
 	}
-	t.Step("clients.Get")
+	t.Step("oidcClientsClient.Get")
 
 	// Using the OIDCClient's UID, check to see if the storage Secret for its client secrets already exists.
 	// Note that when it does not exist, this Get() function will not return an error, and will return nil rv and hashes.
@@ -138,14 +150,14 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 	// If requested, generate a new client secret and add it to the list.
 	var secret string
 	if req.Spec.GenerateNewSecret {
-		secret, err = generateSecret(r.rand)
+		secret, err = generateSecret(r.randByteGenerator)
 		if err != nil {
 			traceFailureWithError(t, "generateSecret", err)
 			return nil, apierrors.NewInternalError(fmt.Errorf("client secret generation failed"))
 		}
 		t.Step("generateSecret")
 
-		hash, err := bcrypt.GenerateFromPassword([]byte(secret), r.cost)
+		hash, err := r.byteHasher([]byte(secret), r.cost)
 		if err != nil {
 			traceFailureWithError(t, "bcrypt.GenerateFromPassword", err)
 			return nil, apierrors.NewInternalError(fmt.Errorf("hash generation failed"))
@@ -165,8 +177,9 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 	if req.Spec.GenerateNewSecret || needsRevoke {
 		// Each bcrypt comparison is expensive, and we do not want a large list to cause wasted CPU.
 		if len(hashes) > 5 {
-			return nil, apierrors.NewRequestEntityTooLargeError(
+			return nil, apierrors.NewBadRequest(
-				fmt.Sprintf("OIDCClient %s has too many secrets, spec.revokeOldSecrets must be true", oidcClient.Name))
+				fmt.Sprintf("OIDCClient %s has too many secrets, spec.revokeOldSecrets must be true", oidcClient.Name),
+			)
 		}
 
 		// Create or update the storage Secret for client secrets.

internal/supervisor/apiserver/apiserver.go

@@ -5,9 +5,11 @@ package apiserver
 
 import (
 	"context"
+	"crypto/rand"
 	"fmt"
 	"sync"
 
+	"golang.org/x/crypto/bcrypt"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -80,7 +82,15 @@ func (c completedConfig) New() (*PinnipedServer, error) {
 	for _, f := range []func() (schema.GroupVersionResource, rest.Storage){
 		func() (schema.GroupVersionResource, rest.Storage) {
 			clientSecretReqGVR := c.ExtraConfig.ClientSecretSupervisorGroupVersion.WithResource("oidcclientsecretrequests")
-			clientSecretReqStorage := clientsecretrequest.NewREST(clientSecretReqGVR.GroupResource(), c.ExtraConfig.Secrets, c.ExtraConfig.OIDCClients, c.ExtraConfig.Namespace, clientsecretrequest.Cost)
+			clientSecretReqStorage := clientsecretrequest.NewREST(
+				clientSecretReqGVR.GroupResource(),
+				c.ExtraConfig.Secrets,
+				c.ExtraConfig.OIDCClients,
+				c.ExtraConfig.Namespace,
+				clientsecretrequest.Cost,
+				rand.Reader,
+				bcrypt.GenerateFromPassword,
+			)
 			return clientSecretReqGVR, clientSecretReqStorage
 		},
 	} {