Merge branch 'main' into issue-cert

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.
+	//  TODO Since we are an aggregated API, we should investigate to see if the kube API server is already invoking admission hooks for us.
-	//  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
-	//  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
-	//  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.
-	//  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()

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 {
+func requireSuccessfulResponseWithAuthenticationFailureMessage(t *testing.T, err error, response runtime.Object) {
-	return &authenticator.Response{User: &user.DefaultInfo{}}
+	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)
+	requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response)
-	require.Equal(t, response, &placeholderapi.LoginRequest{
-		Status: placeholderapi.LoginRequestStatus{
-			Credential: nil,
-			Message:    "authentication failed",
-		},
-	})
 	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)
+	requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response)
-	require.Equal(t, response, &placeholderapi.LoginRequest{
+}
-		Status: placeholderapi.LoginRequestStatus{
+
-			Credential: nil,
+func TestCreateSucceedsWithAnUnauthenticatedStatusWhenWebhookDoesNotReturnAnyUserInfo(t *testing.T) {
-			Message:    "authentication failed",
+	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{
+		&metav1.CreateOptions{})
-			DryRun: []string{},
-		})
 	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())

test/integration/loginrequest_test.go

@@ -114,51 +114,9 @@ func TestLoginRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T) {
 	require.Nil(t, response.Status.Credential)
 }
 
-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)