More validations and error handling for create LoginRequest endpoint

go.mod

@@ -8,7 +8,7 @@ require (
 	github.com/google/go-cmp v0.4.0
 	github.com/spf13/cobra v1.0.0
 	github.com/stretchr/testify v1.6.1
-	github.com/suzerain-io/placeholder-name-api v0.0.0-20200714184318-8ad91581433a
+	github.com/suzerain-io/placeholder-name-api v0.0.0-20200723201114-466aa86ace18
 	github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200714203950-a414963b4f95
 	golang.org/x/tools v0.0.0-20200707134715-9e0a013e855f // indirect
 	k8s.io/api v0.19.0-rc.0

go.sum

@@ -521,6 +521,8 @@ github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s
 github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=
 github.com/suzerain-io/placeholder-name-api v0.0.0-20200714184318-8ad91581433a h1:ycG6TufZM7ZDrgBklkXEMauvSyh44JQDvSUdJawAjGA=
 github.com/suzerain-io/placeholder-name-api v0.0.0-20200714184318-8ad91581433a/go.mod h1:bNHheAnmAISdW/ZYTnhCmg8QQKwA5WD64ZvPdsTrWjw=
+github.com/suzerain-io/placeholder-name-api v0.0.0-20200723201114-466aa86ace18 h1:iR4hjldaCcIwDxld0OIG5EHIQrwnmJ+KVBqsEfXrvcI=
+github.com/suzerain-io/placeholder-name-api v0.0.0-20200723201114-466aa86ace18/go.mod h1:bNHheAnmAISdW/ZYTnhCmg8QQKwA5WD64ZvPdsTrWjw=
 github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200714203950-a414963b4f95 h1:O1aoszdMJEekLXD7pNWP23vq0g8eXLp40AQgY8Hj+Sw=
 github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200714203950-a414963b4f95/go.mod h1:NwirXEgd1VaA+6R0W7PYL/ae6wfPT3vA+tu7POAArjU=
 github.com/tdakkota/asciicheck v0.0.0-20200416190851-d7f85be797a2 h1:Xr9gkxfOP0KQWXKNqmwe8vEeSUiUj4Rlee9CMVX2ZUQ=

pkg/registry/loginrequest/rest.go

@@ -16,6 +16,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/registry/rest"
+	"k8s.io/klog/v2"
 
 	placeholderapi "github.com/suzerain-io/placeholder-name-api/pkg/apis/placeholder"
 )
@@ -53,7 +54,16 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 
 	// TODO refactor all validation checks into a validation function in another package (e.g. see subjectaccessreqview api in k8s)
 
-	// TODO also validate .spec.type
+	if len(loginRequest.Spec.Type) == 0 {
+		errs := field.ErrorList{field.Required(field.NewPath("spec", "type"), "type must be supplied")}
+		return nil, apierrors.NewInvalid(placeholderapi.Kind(loginRequest.Kind), loginRequest.Name, errs)
+	}
+
+	if loginRequest.Spec.Type != placeholderapi.TokenLoginCredentialType {
+		errs := field.ErrorList{field.Invalid(field.NewPath("spec", "type"), loginRequest.Spec.Type, "unrecognized type")}
+		return nil, apierrors.NewInvalid(placeholderapi.Kind(loginRequest.Kind), loginRequest.Name, errs)
+	}
+
 	token := loginRequest.Spec.Token
 	if token == nil || len(token.Value) == 0 {
 		errs := field.ErrorList{field.Required(field.NewPath("spec", "token", "value"), "token must be supplied")}
@@ -93,22 +103,40 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		}
 	}()
 
-	// TODO do the actual business logic of this endpoint here
+	_, authenticated, err := r.webhook.AuthenticateToken(cancelCtx, token.Value)
-
-	_, _, err := r.webhook.AuthenticateToken(cancelCtx, token.Value)
 	if err != nil {
-		return nil, fmt.Errorf("authenticate token failed: %w", err)
+		klog.Warningf("webhook authentication failure: %v", err)
+		return failureResponse(), nil
 	}
 
-	// make a new object so that we do not return the original token in the response
+	var out *placeholderapi.LoginRequest
-	out := &placeholderapi.LoginRequest{
+	if authenticated {
-		Status: placeholderapi.LoginRequestStatus{
+		out = successfulResponse()
-			ExpirationTimestamp:   nil,
+	} else {
-			Token:                 "snorlax",
+		out = failureResponse()
-			ClientCertificateData: "",
-			ClientKeyData:         "",
-		},
 	}
 
 	return out, nil
 }
+
+func successfulResponse() *placeholderapi.LoginRequest {
+	return &placeholderapi.LoginRequest{
+		Status: placeholderapi.LoginRequestStatus{
+			Credential: &placeholderapi.LoginRequestCredential{
+				ExpirationTimestamp:   nil,
+				Token:                 "snorlax",
+				ClientCertificateData: "",
+				ClientKeyData:         "",
+			},
+		},
+	}
+}
+
+func failureResponse() *placeholderapi.LoginRequest {
+	return &placeholderapi.LoginRequest{
+		Status: placeholderapi.LoginRequestStatus{
+			Credential: nil,
+			Message:    "authentication failed",
+		},
+	}
+}

pkg/registry/loginrequest/rest_test.go

@@ -32,6 +32,7 @@ type FakeToken struct {
 	cancelled                             bool
 	webhookStartedRunningNotificationChan chan bool
 	returnErr                             error
+	returnUnauthenticated                 bool
 }
 
 func (f *FakeToken) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) {
@@ -47,7 +48,7 @@ func (f *FakeToken) AuthenticateToken(ctx context.Context, token string) (*authe
 	case <-ctx.Done():
 		f.cancelled = true
 	}
-	return &authenticator.Response{}, true, f.returnErr
+	return &authenticator.Response{}, !f.returnUnauthenticated, f.returnErr
 }
 
 func callCreate(ctx context.Context, storage *REST, loginRequest *placeholderapi.LoginRequest) (runtime.Object, error) {
@@ -61,9 +62,13 @@ func callCreate(ctx context.Context, storage *REST, loginRequest *placeholderapi
 }
 
 func validLoginRequest() *placeholderapi.LoginRequest {
+	return validLoginRequestWithToken("a token")
+}
+
+func validLoginRequestWithToken(token string) *placeholderapi.LoginRequest {
 	return loginRequest(placeholderapi.LoginRequestSpec{
 		Type:  placeholderapi.TokenLoginCredentialType,
-		Token: &placeholderapi.LoginRequestTokenCredential{Value: "a token"},
+		Token: &placeholderapi.LoginRequestTokenCredential{Value: token},
 	})
 }
 
@@ -86,27 +91,66 @@ func requireAPIError(t *testing.T, response runtime.Object, err error, expectedE
 	require.Contains(t, status.Status().Message, expectedErrorMessage)
 }
 
-func TestCreateSucceedsWhenGivenAToken(t *testing.T) {
+func TestCreateSucceedsWhenGivenATokenAndTheWebhookAuthenticatesTheToken(t *testing.T) {
-	webhook := FakeToken{}
+	webhook := FakeToken{
+		returnUnauthenticated: false,
+	}
 	storage := NewREST(&webhook)
 	requestToken := "a token"
-	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
+
-		Type:  placeholderapi.TokenLoginCredentialType,
+	response, err := callCreate(context.Background(), storage, validLoginRequestWithToken(requestToken))
-		Token: &placeholderapi.LoginRequestTokenCredential{Value: requestToken},
-	}))
 
 	require.NoError(t, err)
 	require.Equal(t, response, &placeholderapi.LoginRequest{
 		Status: placeholderapi.LoginRequestStatus{
-			ExpirationTimestamp:   nil,
+			Credential: &placeholderapi.LoginRequestCredential{
-			Token:                 "snorlax",
+				ExpirationTimestamp:   nil,
-			ClientCertificateData: "",
+				Token:                 "snorlax",
-			ClientKeyData:         "",
+				ClientCertificateData: "",
+				ClientKeyData:         "",
+			},
+			Message: "",
 		},
 	})
 	require.Equal(t, requestToken, webhook.calledWithToken)
 }
 
+func TestCreateSucceedsWithAnUnauthenticatedStatusWhenGivenATokenAndTheWebhookDoesNotAuthenticateTheToken(t *testing.T) {
+	webhook := FakeToken{
+		returnUnauthenticated: true,
+	}
+	storage := NewREST(&webhook)
+	requestToken := "a token"
+
+	response, err := callCreate(context.Background(), storage, validLoginRequestWithToken(requestToken))
+
+	require.NoError(t, err)
+	require.Equal(t, response, &placeholderapi.LoginRequest{
+		Status: placeholderapi.LoginRequestStatus{
+			Credential: nil,
+			Message:    "authentication failed",
+		},
+	})
+	require.Equal(t, requestToken, webhook.calledWithToken)
+}
+
+func TestCreateSucceedsWithAnUnauthenticatedStatusWhenWebhookFails(t *testing.T) {
+	webhook := FakeToken{
+		returnErr: errors.New("some webhook error"),
+	}
+	storage := NewREST(&webhook)
+
+	response, err := callCreate(context.Background(), storage, validLoginRequest())
+
+	require.NoError(t, err)
+	require.Equal(t, response, &placeholderapi.LoginRequest{
+		Status: placeholderapi.LoginRequestStatus{
+			Credential: nil,
+			Message:    "authentication failed",
+		},
+	})
+}
+
 func TestCreateDoesNotPassAdditionalContextInfoToTheWebhook(t *testing.T) {
 	webhook := FakeToken{}
 	storage := NewREST(&webhook)
@@ -118,6 +162,74 @@ func TestCreateDoesNotPassAdditionalContextInfoToTheWebhook(t *testing.T) {
 	require.Nil(t, webhook.calledWithContext.Value("context-key"))
 }
 
+func TestCreateFailsWhenGivenTheWrongInputType(t *testing.T) {
+	notALoginRequest := runtime.Unknown{}
+	response, err := NewREST(&FakeToken{}).Create(
+		genericapirequest.NewContext(),
+		&notALoginRequest,
+		rest.ValidateAllObjectFunc,
+		&metav1.CreateOptions{})
+
+	requireAPIError(t, response, err, apierrors.IsBadRequest, "not a LoginRequest")
+}
+
+func TestCreateFailsWhenTokenIsNilInRequest(t *testing.T) {
+	storage := NewREST(&FakeToken{})
+	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
+		Type:  placeholderapi.TokenLoginCredentialType,
+		Token: nil,
+	}))
+
+	requireAPIError(t, response, err, apierrors.IsInvalid,
+		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`)
+}
+
+func TestCreateFailsWhenTypeInRequestIsMissing(t *testing.T) {
+	storage := NewREST(&FakeToken{})
+	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
+		Type:  "",
+		Token: &placeholderapi.LoginRequestTokenCredential{Value: "a token"},
+	}))
+
+	requireAPIError(t, response, err, apierrors.IsInvalid,
+		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.type: Required value: type must be supplied`)
+}
+
+func TestCreateFailsWhenTypeInRequestIsNotLegal(t *testing.T) {
+	storage := NewREST(&FakeToken{})
+	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
+		Type:  "this in an invalid type",
+		Token: &placeholderapi.LoginRequestTokenCredential{Value: "a token"},
+	}))
+
+	requireAPIError(t, response, err, apierrors.IsInvalid,
+		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.type: Invalid value: "this in an invalid type": unrecognized type`)
+}
+
+func TestCreateFailsWhenTokenValueIsEmptyInRequest(t *testing.T) {
+	storage := NewREST(&FakeToken{})
+	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
+		Type:  placeholderapi.TokenLoginCredentialType,
+		Token: &placeholderapi.LoginRequestTokenCredential{Value: ""},
+	}))
+
+	requireAPIError(t, response, err, apierrors.IsInvalid,
+		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`)
+}
+
+func TestCreateFailsWhenRequestOptionsDryRunIsNotEmpty(t *testing.T) {
+	response, err := NewREST(&FakeToken{}).Create(
+		genericapirequest.NewContext(),
+		validLoginRequest(),
+		rest.ValidateAllObjectFunc,
+		&metav1.CreateOptions{
+			DryRun: []string{"some dry run flag"},
+		})
+
+	requireAPIError(t, response, err, apierrors.IsInvalid,
+		`.placeholder.suzerain-io.github.io "request name" is invalid: dryRun: Unsupported value: []string{"some dry run flag"}`)
+}
+
 func TestCreateCancelsTheWebhookInvocationWhenTheCallToCreateIsCancelledItself(t *testing.T) {
 	webhookStartedRunningNotificationChan := make(chan bool)
 	webhook := FakeToken{
@@ -169,61 +281,3 @@ func TestCreateAllowsTheWebhookInvocationToFinishWhenTheCallToCreateIsNotCancell
 	require.True(t, webhook.reachedTimeout)
 	require.Equal(t, context.Canceled, webhook.calledWithContext.Err()) // the inner context is cancelled (in this case by the "defer")
 }
-
-func TestCreateFailsWhenWebhookFails(t *testing.T) {
-	webhook := FakeToken{
-		returnErr: errors.New("some webhook error"),
-	}
-	storage := NewREST(&webhook)
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	_, err := callCreate(ctx, storage, validLoginRequest())
-	require.EqualError(t, err, "authenticate token failed: some webhook error")
-}
-
-func TestCreateFailsWhenGivenTheWrongInputType(t *testing.T) {
-	notALoginRequest := runtime.Unknown{}
-	response, err := NewREST(&FakeToken{}).Create(
-		genericapirequest.NewContext(),
-		&notALoginRequest,
-		rest.ValidateAllObjectFunc,
-		&metav1.CreateOptions{})
-
-	requireAPIError(t, response, err, apierrors.IsBadRequest, "not a LoginRequest")
-}
-
-func TestCreateFailsWhenTokenIsNilInRequest(t *testing.T) {
-	storage := NewREST(&FakeToken{})
-	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
-		Type:  placeholderapi.TokenLoginCredentialType,
-		Token: nil,
-	}))
-
-	requireAPIError(t, response, err, apierrors.IsInvalid,
-		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`)
-}
-
-func TestCreateFailsWhenTokenValueIsEmptyInRequest(t *testing.T) {
-	storage := NewREST(&FakeToken{})
-	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
-		Type:  placeholderapi.TokenLoginCredentialType,
-		Token: &placeholderapi.LoginRequestTokenCredential{Value: ""},
-	}))
-
-	requireAPIError(t, response, err, apierrors.IsInvalid,
-		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`)
-}
-
-func TestCreateFailsWhenRequestOptionsDryRunIsNotEmpty(t *testing.T) {
-	response, err := NewREST(&FakeToken{}).Create(
-		genericapirequest.NewContext(),
-		validLoginRequest(),
-		rest.ValidateAllObjectFunc,
-		&metav1.CreateOptions{
-			DryRun: []string{"some dry run flag"},
-		})
-
-	requireAPIError(t, response, err, apierrors.IsInvalid,
-		`.placeholder.suzerain-io.github.io "request name" is invalid: dryRun: Unsupported value: []string{"some dry run flag"}`)
-}

test/integration/loginrequest_test.go

@@ -40,20 +40,39 @@ func TestSuccessfulLoginRequest(t *testing.T) {
 	require.NotEmptyf(t, tmcClusterToken, "must specify PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN env var for integration tests")
 
 	response, err := makeRequest(t, v1alpha1.LoginRequestSpec{
+		Type:  v1alpha1.TokenLoginCredentialType,
 		Token: &v1alpha1.LoginRequestTokenCredential{Value: tmcClusterToken},
 	})
 
+	require.NoError(t, err)
+	require.Empty(t, response.Status.Message)
+
+	require.Empty(t, response.Spec)
+	require.NotNil(t, response.Status.Credential)
+	require.NotEmpty(t, response.Status.Credential.Token)
+	require.Empty(t, response.Status.Credential.ClientCertificateData)
+	require.Empty(t, response.Status.Credential.ClientKeyData)
+	require.Nil(t, response.Status.Credential.ExpirationTimestamp)
+}
+
+func TestFailedLoginRequestWhenTheRequestIsValidButTheTokenDoesNotAuthenticateTheUser(t *testing.T) {
+	response, err := makeRequest(t, v1alpha1.LoginRequestSpec{
+		Type:  v1alpha1.TokenLoginCredentialType,
+		Token: &v1alpha1.LoginRequestTokenCredential{Value: "not a good token"},
+	})
+
 	require.NoError(t, err)
 
 	require.Empty(t, response.Spec)
-	require.NotEmpty(t, response.Status.Token)
+	require.Nil(t, response.Status.Credential)
-	require.Empty(t, response.Status.ClientCertificateData)
+	require.Equal(t, "authentication failed", response.Status.Message)
-	require.Empty(t, response.Status.ClientKeyData)
-	require.Nil(t, response.Status.ExpirationTimestamp)
 }
 
 func TestLoginRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T) {
-	_, err := makeRequest(t, v1alpha1.LoginRequestSpec{})
+	_, err := makeRequest(t, v1alpha1.LoginRequestSpec{
+		Type:  v1alpha1.TokenLoginCredentialType,
+		Token: nil,
+	})
 
 	require.Error(t, err)
 	statusError, isStatus := err.(*errors.StatusError)