From e1f44e26545782cd4b97e11c6a12f7bf712530ce Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 24 Jul 2020 11:17:32 -0400 Subject: [PATCH 1/2] Condense discovery integration tests I think these tests do roughly the same thing... Signed-off-by: Andrew Keesler --- test/integration/loginrequest_test.go | 43 --------------------------- 1 file changed, 43 deletions(-) diff --git a/test/integration/loginrequest_test.go b/test/integration/loginrequest_test.go index 9799aa14..8305fa34 100644 --- a/test/integration/loginrequest_test.go +++ b/test/integration/loginrequest_test.go @@ -7,7 +7,6 @@ package integration import ( "context" - "encoding/json" "os" "testing" "time" @@ -66,51 +65,9 @@ func TestLoginRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T) { require.Equal(t, "spec.token.value", cause.Field) } -func TestGetDiscovery(t *testing.T) { - client := library.NewPlaceholderNameClientset(t) - - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - result, err := client.Discovery().RESTClient().Get().Do(ctx).Raw() - require.NoError(t, err) - - var parsedResult map[string]interface{} - err = json.Unmarshal(result, &parsedResult) - require.NoError(t, err) - require.Contains(t, parsedResult["paths"], "/apis/placeholder.suzerain-io.github.io") - require.Contains(t, parsedResult["paths"], "/apis/placeholder.suzerain-io.github.io/v1alpha1") -} - func TestGetAPIResourceList(t *testing.T) { - var expectedAPIResourceList = `{ - "kind": "APIResourceList", - "apiVersion": "v1", - "groupVersion": "placeholder.suzerain-io.github.io/v1alpha1", - "resources": [ - { - "name": "loginrequests", - "singularName": "", - "namespaced": false, - "kind": "LoginRequest", - "verbs": [ - "create" - ] - } - ] - }` - client := library.NewPlaceholderNameClientset(t) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - result, err := client.PlaceholderV1alpha1().RESTClient().Get().Do(ctx).Raw() - require.NoError(t, err) - require.JSONEq(t, expectedAPIResourceList, string(result)) - - // proposed: - groups, resources, err := client.Discovery().ServerGroupsAndResources() require.NoError(t, err) From 9bfec08d90d5fd1369d394a7510bbb9060288685 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 24 Jul 2020 11:00:29 -0700 Subject: [PATCH 2/2] More tests and more validations for create LoginRequest endpoint - Mostly testing the way that the validation webhooks are called - Also error when the auth webhook does not return user info, since we wouldn't know who you are in that case --- pkg/registry/loginrequest/rest.go | 16 ++-- pkg/registry/loginrequest/rest_test.go | 114 ++++++++++++++++++++----- 2 files changed, 101 insertions(+), 29 deletions(-) diff --git a/pkg/registry/loginrequest/rest.go b/pkg/registry/loginrequest/rest.go index 2d8d4bd6..692b8da1 100644 --- a/pkg/registry/loginrequest/rest.go +++ b/pkg/registry/loginrequest/rest.go @@ -71,13 +71,15 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation } // let dynamic admission webhooks have a chance to validate (but not mutate) as well - // TODO Are we okay with admission webhooks being able to see tokens? Maybe strip token out before passing obj to createValidation. - // Since we are an aggregated API, we should investigate to see if the kube API server is already invoking admission hooks for us. - // Even if it is, its okay to call it again here. If the kube API server is already calling the webhooks and passing - // the token, then there is probably no reason for us to avoid passing the token when we call the webhooks here, since - // they already got the token. + // TODO Since we are an aggregated API, we should investigate to see if the kube API server is already invoking admission hooks for us. + // Even if it is, its okay to call it again here. However, if the kube API server is already calling the webhooks and passing + // the token, then there is probably no reason for us to avoid passing the token when we call the webhooks here, since + // they already got the token. if createValidation != nil { - if err := createValidation(ctx, obj.DeepCopyObject()); err != nil { + requestForValidation := obj.DeepCopyObject() + loginRequestCopy, _ := requestForValidation.(*placeholderapi.LoginRequest) + loginRequestCopy.Spec.Token.Value = "" + if err := createValidation(ctx, requestForValidation); err != nil { return nil, err } } @@ -110,7 +112,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation } var out *placeholderapi.LoginRequest - if authenticated { + if authenticated && authResponse.User != nil && authResponse.User.GetName() != "" { out = successfulResponse(authResponse) } else { out = failureResponse() diff --git a/pkg/registry/loginrequest/rest_test.go b/pkg/registry/loginrequest/rest_test.go index 823ea2d6..43424276 100644 --- a/pkg/registry/loginrequest/rest_test.go +++ b/pkg/registry/loginrequest/rest_test.go @@ -65,7 +65,7 @@ func callCreate(ctx context.Context, storage *REST, loginRequest *placeholderapi } func validLoginRequest() *placeholderapi.LoginRequest { - return validLoginRequestWithToken("a token") + return validLoginRequestWithToken("some token") } func validLoginRequestWithToken(token string) *placeholderapi.LoginRequest { @@ -85,6 +85,15 @@ func loginRequest(spec placeholderapi.LoginRequestSpec) *placeholderapi.LoginReq } } +func webhookSuccessResponse() *authenticator.Response { + return &authenticator.Response{User: &user.DefaultInfo{ + Name: "some-user", + UID: "", + Groups: []string{}, + Extra: nil, + }} +} + func requireAPIError(t *testing.T, response runtime.Object, err error, expectedErrorTypeChecker func(err error) bool, expectedErrorMessage string) { t.Helper() require.Nil(t, response) @@ -94,8 +103,14 @@ func requireAPIError(t *testing.T, response runtime.Object, err error, expectedE require.Contains(t, status.Status().Message, expectedErrorMessage) } -func emptyWebhookSuccessResponse() *authenticator.Response { - return &authenticator.Response{User: &user.DefaultInfo{}} +func requireSuccessfulResponseWithAuthenticationFailureMessage(t *testing.T, err error, response runtime.Object) { + require.NoError(t, err) + require.Equal(t, response, &placeholderapi.LoginRequest{ + Status: placeholderapi.LoginRequestStatus{ + Credential: nil, + Message: "authentication failed", + }, + }) } func TestCreateSucceedsWhenGivenATokenAndTheWebhookAuthenticatesTheToken(t *testing.T) { @@ -141,13 +156,7 @@ func TestCreateSucceedsWithAnUnauthenticatedStatusWhenGivenATokenAndTheWebhookDo 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", - }, - }) + requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) require.Equal(t, requestToken, webhook.calledWithToken) } @@ -159,18 +168,38 @@ func TestCreateSucceedsWithAnUnauthenticatedStatusWhenWebhookFails(t *testing.T) 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", + requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) +} + +func TestCreateSucceedsWithAnUnauthenticatedStatusWhenWebhookDoesNotReturnAnyUserInfo(t *testing.T) { + webhook := FakeToken{ + returnResponse: &authenticator.Response{}, + } + storage := NewREST(&webhook) + + response, err := callCreate(context.Background(), storage, validLoginRequest()) + + requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) +} + +func TestCreateSucceedsWithAnUnauthenticatedStatusWhenWebhookReturnsAnEmptyUsername(t *testing.T) { + webhook := FakeToken{ + returnResponse: &authenticator.Response{ + User: &user.DefaultInfo{ + Name: "", + }, }, - }) + } + storage := NewREST(&webhook) + + response, err := callCreate(context.Background(), storage, validLoginRequest()) + + requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) } func TestCreateDoesNotPassAdditionalContextInfoToTheWebhook(t *testing.T) { webhook := FakeToken{ - returnResponse: emptyWebhookSuccessResponse(), + returnResponse: webhookSuccessResponse(), } storage := NewREST(&webhook) ctx := context.WithValue(context.Background(), contextKey{}, "context-value") @@ -244,13 +273,54 @@ func TestCreateFailsWhenValidationFails(t *testing.T) { func(ctx context.Context, obj runtime.Object) error { return fmt.Errorf("some validation error") }, - &metav1.CreateOptions{ - DryRun: []string{}, - }) + &metav1.CreateOptions{}) require.Nil(t, response) require.EqualError(t, err, "some validation error") } +func TestCreateDoesNotAllowValidationFunctionToMutateRequest(t *testing.T) { + webhook := FakeToken{ + returnResponse: webhookSuccessResponse(), + } + storage := NewREST(&webhook) + requestToken := "a token" + response, err := storage.Create( + context.Background(), + validLoginRequestWithToken(requestToken), + func(ctx context.Context, obj runtime.Object) error { + loginRequest, _ := obj.(*placeholderapi.LoginRequest) + loginRequest.Spec.Token.Value = "foobaz" + return nil + }, + &metav1.CreateOptions{}) + require.NoError(t, err) + require.NotEmpty(t, response) + require.Equal(t, requestToken, webhook.calledWithToken) // i.e. not called with foobaz +} + +func TestCreateDoesNotAllowValidationFunctionToSeeTheActualRequestToken(t *testing.T) { + webhook := FakeToken{ + returnResponse: webhookSuccessResponse(), + } + storage := NewREST(&webhook) + validationFunctionWasCalled := false + var validationFunctionSawTokenValue string + response, err := storage.Create( + context.Background(), + validLoginRequest(), + func(ctx context.Context, obj runtime.Object) error { + loginRequest, _ := obj.(*placeholderapi.LoginRequest) + validationFunctionWasCalled = true + validationFunctionSawTokenValue = loginRequest.Spec.Token.Value + return nil + }, + &metav1.CreateOptions{}) + require.NoError(t, err) + require.NotEmpty(t, response) + require.True(t, validationFunctionWasCalled) + require.Empty(t, validationFunctionSawTokenValue) +} + func TestCreateFailsWhenRequestOptionsDryRunIsNotEmpty(t *testing.T) { response, err := NewREST(&FakeToken{}).Create( genericapirequest.NewContext(), @@ -269,7 +339,7 @@ func TestCreateCancelsTheWebhookInvocationWhenTheCallToCreateIsCancelledItself(t webhook := FakeToken{ timeout: time.Second * 2, webhookStartedRunningNotificationChan: webhookStartedRunningNotificationChan, - returnResponse: emptyWebhookSuccessResponse(), + returnResponse: webhookSuccessResponse(), } storage := NewREST(&webhook) ctx, cancel := context.WithCancel(context.Background()) @@ -296,7 +366,7 @@ func TestCreateAllowsTheWebhookInvocationToFinishWhenTheCallToCreateIsNotCancell webhook := FakeToken{ timeout: 0, webhookStartedRunningNotificationChan: webhookStartedRunningNotificationChan, - returnResponse: emptyWebhookSuccessResponse(), + returnResponse: webhookSuccessResponse(), } storage := NewREST(&webhook) ctx, cancel := context.WithCancel(context.Background())