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())