Merge pull request #25 from cfryanr/finishing_webhook_invocation

Finishing touches on webhook invocation

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-20200724000517-dc602fd8d75e
 	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-20200724000517-dc602fd8d75e h1:PavRTcjOk0SUtIRUY4OaFXB3CVYFLTIlxLptUawLB1o=
+github.com/suzerain-io/placeholder-name-api v0.0.0-20200724000517-dc602fd8d75e/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,44 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		}
 	}()
 
-	// TODO do the actual business logic of this endpoint here
+	authResponse, 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(authResponse)
-			ExpirationTimestamp:   nil,
+	} else {
-			Token:                 "snorlax",
+		out = failureResponse()
-			ClientCertificateData: "",
-			ClientKeyData:         "",
-		},
 	}
 
 	return out, nil
 }
+
+func successfulResponse(authResponse *authenticator.Response) *placeholderapi.LoginRequest {
+	return &placeholderapi.LoginRequest{
+		Status: placeholderapi.LoginRequestStatus{
+			Credential: &placeholderapi.LoginRequestCredential{
+				ExpirationTimestamp:   nil,
+				Token:                 "snorlax",
+				ClientCertificateData: "",
+				ClientKeyData:         "",
+			},
+			User: &placeholderapi.User{
+				Name:   authResponse.User.GetName(),
+				Groups: authResponse.User.GetGroups(),
+			},
+		},
+	}
+}
+
+func failureResponse() *placeholderapi.LoginRequest {
+	return &placeholderapi.LoginRequest{
+		Status: placeholderapi.LoginRequestStatus{
+			Credential: nil,
+			Message:    "authentication failed",
+		},
+	}
+}

pkg/registry/loginrequest/rest_test.go

@@ -11,12 +11,12 @@ import (
 	"testing"
 	"time"
 
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-
 	"github.com/stretchr/testify/require"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
+	"k8s.io/apiserver/pkg/authentication/user"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
 
@@ -32,6 +32,8 @@ type FakeToken struct {
 	reachedTimeout                        bool
 	cancelled                             bool
 	webhookStartedRunningNotificationChan chan bool
+	returnResponse                        *authenticator.Response
+	returnUnauthenticated                 bool
 	returnErr                             error
 }
 
@@ -48,7 +50,7 @@ func (f *FakeToken) AuthenticateToken(ctx context.Context, token string) (*authe
 	case <-ctx.Done():
 		f.cancelled = true
 	}
-	return &authenticator.Response{}, true, f.returnErr
+	return f.returnResponse, !f.returnUnauthenticated, f.returnErr
 }
 
 func callCreate(ctx context.Context, storage *REST, loginRequest *placeholderapi.LoginRequest) (runtime.Object, error) {
@@ -62,9 +64,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},
 	})
 }
 
@@ -78,29 +84,93 @@ func loginRequest(spec placeholderapi.LoginRequestSpec) *placeholderapi.LoginReq
 	}
 }
 
-func TestCreateSucceedsWhenGivenAToken(t *testing.T) {
+func requireAPIError(t *testing.T, response runtime.Object, err error, expectedErrorTypeChecker func(err error) bool, expectedErrorMessage string) {
-	webhook := FakeToken{}
+	t.Helper()
+	require.Nil(t, response)
+	require.True(t, expectedErrorTypeChecker(err))
+	var status apierrors.APIStatus
+	errors.As(err, &status)
+	require.Contains(t, status.Status().Message, expectedErrorMessage)
+}
+
+func emptyWebhookSuccessResponse() *authenticator.Response {
+	return &authenticator.Response{User: &user.DefaultInfo{}}
+}
+
+func TestCreateSucceedsWhenGivenATokenAndTheWebhookAuthenticatesTheToken(t *testing.T) {
+	webhook := FakeToken{
+		returnResponse: &authenticator.Response{
+			User: &user.DefaultInfo{
+				Name:   "test-user",
+				Groups: []string{"test-group-1", "test-group-2"},
+			},
+		},
+		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,
+			User: &placeholderapi.User{
-			Token:                 "snorlax",
+				Name:   "test-user",
-			ClientCertificateData: "",
+				Groups: []string{"test-group-1", "test-group-2"},
-			ClientKeyData:         "",
+			},
+			Credential: &placeholderapi.LoginRequestCredential{
+				ExpirationTimestamp:   nil,
+				Token:                 "snorlax",
+				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{}
+	webhook := FakeToken{
+		returnResponse: emptyWebhookSuccessResponse(),
+	}
 	storage := NewREST(&webhook)
 	ctx := context.WithValue(context.Background(), contextKey{}, "context-value")
 
@@ -110,11 +180,80 @@ 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{
 		timeout:                               time.Second * 2,
 		webhookStartedRunningNotificationChan: webhookStartedRunningNotificationChan,
+		returnResponse:                        emptyWebhookSuccessResponse(),
 	}
 	storage := NewREST(&webhook)
 	ctx, cancel := context.WithCancel(context.Background())
@@ -141,6 +280,7 @@ func TestCreateAllowsTheWebhookInvocationToFinishWhenTheCallToCreateIsNotCancell
 	webhook := FakeToken{
 		timeout:                               0,
 		webhookStartedRunningNotificationChan: webhookStartedRunningNotificationChan,
+		returnResponse:                        emptyWebhookSuccessResponse(),
 	}
 	storage := NewREST(&webhook)
 	ctx, cancel := context.WithCancel(context.Background())
@@ -161,80 +301,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{})
-
-	require.Nil(t, response)
-	require.True(t, apierrors.IsBadRequest(err))
-	var status apierrors.APIStatus
-	errors.As(err, &status)
-	require.Contains(t, status.Status().Message, "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,
-	}))
-
-	require.Nil(t, response)
-	require.True(t, apierrors.IsInvalid(err))
-	var status apierrors.APIStatus
-	errors.As(err, &status)
-	require.Equal(t,
-		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`,
-		status.Status().Message)
-}
-
-func TestCreateFailsWhenTokenValueIsEmptyInRequest(t *testing.T) {
-	storage := NewREST(&FakeToken{})
-	response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{
-		Type:  placeholderapi.TokenLoginCredentialType,
-		Token: &placeholderapi.LoginRequestTokenCredential{Value: ""},
-	}))
-
-	require.Nil(t, response)
-	require.True(t, apierrors.IsInvalid(err))
-	var status apierrors.APIStatus
-	errors.As(err, &status)
-	require.Equal(t,
-		`.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`,
-		status.Status().Message)
-}
-
-func TestCreateFailsWhenRequestOptionsDryRunIsNotEmpty(t *testing.T) {
-	response, err := NewREST(&FakeToken{}).Create(
-		genericapirequest.NewContext(),
-		validLoginRequest(),
-		rest.ValidateAllObjectFunc,
-		&metav1.CreateOptions{
-			DryRun: []string{"some dry run flag"},
-		})
-
-	require.Nil(t, response)
-	require.True(t, apierrors.IsInvalid(err))
-	var status apierrors.APIStatus
-	errors.As(err, &status)
-	require.Equal(t,
-		`.placeholder.suzerain-io.github.io "request name" is invalid: dryRun: Unsupported value: []string{"some dry run flag"}`,
-		status.Status().Message)
-}

test/integration/loginrequest_test.go

@@ -40,20 +40,44 @@ 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)
+
+	require.NotNil(t, response.Status.User)
+	require.NotEmpty(t, response.Status.User.Name)
+	require.Contains(t, response.Status.User.Groups, "tmc:member")
+}
+
+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.Nil(t, response.Status.User)
-	require.Empty(t, response.Status.ClientKeyData)
+	require.Equal(t, "authentication failed", response.Status.Message)
-	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)