review comments & rebase

pkg/server/config.go

@@ -447,7 +447,7 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
 		// 2. Rest of the handlers up to Authz
 		// 3. Scoping handlers to ensure that the request is scoped to the user's clusters before authz is done.
 		// 4. Rest of the handlers.
-		apiHandler = WithScoping(apiHandler)
+		apiHandler = withScoping(apiHandler)
 		apiHandler = genericapiserver.DefaultBuildHandlerChainFromImpersonationToAuthz(apiHandler, genericConfig)
 		apiHandler = WithImpersonationGatekeeper(apiHandler)
 		apiHandler = genericapiserver.DefaultBuildHandlerChainFromStartToBeforeImpersonation(apiHandler, genericConfig)

pkg/server/handler.go

@@ -274,6 +274,13 @@ var (
 	}
 )
 
+const (
+	// kcpImpersonationHeader is the header key for impersonation marker.
+	// This should never be exposed to the anything outside WithImpersonationGatekeeper and withScoping.
+	// We use headers headers already has all the helpers to manipulate the request :) it just easy :)
+	kcpImpersonationHeader = "X-Kcp-Impersonated"
+)
+
 // WithImpersonationGatekeeper checks the request for impersonations and validates them,
 // if they are valid. If they are not, will return a 403.
 // We check for impersonation in the request headers, early to avoid it being propagated to
@@ -293,10 +300,14 @@ func WithImpersonationGatekeeper(handler http.Handler) http.Handler {
 				}
 			}
 		}
-		if len(impersonationUser) == 0 && len(impersonationGroups) == 0 && len(impersonationExtras) == 0 { // No user and group to impersonate - just pass the request.
+		// If no impersonation is requested, just pass the request.
+		if len(impersonationUser) == 0 && len(impersonationGroups) == 0 && len(impersonationExtras) == 0 {
 			handler.ServeHTTP(w, req)
 			return
 		}
+		// If imepersonation is requested, validate and add "marker" for withScoping to scope the request.
+		// This is done so we know that request was impersonated as upstream code will remove any sign of impersonation.
+		req.Header.Set(kcpImpersonationHeader, "true")
 
 		requester, exists := request.UserFrom(req.Context())
 		if !exists {
@@ -317,10 +328,16 @@ func WithImpersonationGatekeeper(handler http.Handler) http.Handler {
 	})
 }
 
-// WithScoping scopes the request to the cluster it is intended for.
-func WithScoping(handler http.Handler) http.Handler {
+// withScoping scopes the request to the cluster it is intended for.
+func withScoping(handler http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		user, exists := request.UserFrom(req.Context())
+		if req.Header.Get(kcpImpersonationHeader) != "true" { // no impersonation, no scoping.
+			handler.ServeHTTP(w, req)
+			return
+		}
+		req.Header.Del(kcpImpersonationHeader) // remove the marker.
+
+		u, exists := request.UserFrom(req.Context())
 		if !exists {
 			responsewriters.ErrorNegotiated(
 				apierrors.NewForbidden(schema.GroupResource{}, "", fmt.Errorf("impersonation is invalid for the requestor")),
@@ -336,25 +353,22 @@ func WithScoping(handler http.Handler) http.Handler {
 			return
 		}
 
-		extra := user.GetExtra()
+		extra := u.GetExtra()
 		if extra == nil {
 			extra = map[string][]string{}
 		}
+		userScoped := &user.DefaultInfo{
+			Name:   u.GetName(),
+			UID:    u.GetUID(),
+			Groups: u.GetGroups(),
+			Extra:  extra,
+		}
+
 		extra["authentication.kcp.io/scopes"] = append(extra["authentication.kcp.io/scopes"], fmt.Sprintf("cluster:%s", cluster.Name))
-		handler.ServeHTTP(w, req.WithContext(request.WithUser(req.Context(), copyUserWithExtra(user, extra))))
+		handler.ServeHTTP(w, req.WithContext(request.WithUser(req.Context(), userScoped)))
 	})
 }
 
-// copyUserWithExtra returns a copy of the user with the extra field set to the provided value.
-func copyUserWithExtra(u user.Info, extra map[string][]string) user.Info {
-	return &user.DefaultInfo{
-		Name:   u.GetName(),
-		UID:    u.GetUID(),
-		Groups: u.GetGroups(),
-		Extra:  extra,
-	}
-}
-
 // maxUserPrivilege returns the highest privilege level found among the user's groups.
 func maxUserPrivilege(userGroups []string) privilege {
 	max := unprivileged

pkg/server/handler_test.go

@@ -468,3 +468,133 @@ func TestWithImpersonationGatekeeper(t *testing.T) {
 		})
 	}
 }
+
+// TestWithScoping tests the WithScoping middleware.
+func TestWithScoping(t *testing.T) {
+	// Define test cases
+	tests := []struct {
+		name               string
+		user               user.Info
+		cluster            *request.Cluster
+		expectedStatus     int
+		handlerCalled      bool
+		expectedExtraScope string
+		skipScopingMarker  bool
+	}{
+		{
+			name:              "no scoping marker. Programmers error.",
+			user:              &mockUser{Name: "test-user", UID: "uid-123", Groups: []string{"group1"}, Extra: nil},
+			cluster:           &request.Cluster{Name: "cluster-1"},
+			expectedStatus:    http.StatusOK,
+			handlerCalled:     true,
+			skipScopingMarker: true,
+		},
+		{
+			name:           "No user in context",
+			user:           nil,
+			cluster:        &request.Cluster{Name: "cluster-1"},
+			expectedStatus: http.StatusForbidden,
+			handlerCalled:  false,
+		},
+		{
+			name: "No cluster in context",
+			user: &mockUser{
+				Name:   "test-user",
+				UID:    "uid-123",
+				Groups: []string{"group1"},
+				Extra:  nil,
+			},
+			expectedStatus: http.StatusForbidden,
+			handlerCalled:  false,
+		},
+		{
+			name: "Valid user and cluster with no existing extra scopes",
+			user: &mockUser{
+				Name:   "test-user",
+				UID:    "uid-456",
+				Groups: []string{"group1"},
+				Extra:  nil,
+			},
+			cluster:            &request.Cluster{Name: "cluster-2"},
+			expectedStatus:     http.StatusOK,
+			handlerCalled:      true,
+			expectedExtraScope: "cluster:cluster-2",
+		},
+		{
+			name: "Valid user and cluster with existing extra scopes",
+			user: &mockUser{
+				Name:   "test-user",
+				UID:    "uid-789",
+				Groups: []string{"group1"},
+				Extra: map[string][]string{
+					"existing.key": {"value1"},
+				},
+			},
+			cluster:            &request.Cluster{Name: "cluster-3"},
+			expectedStatus:     http.StatusOK,
+			handlerCalled:      true,
+			expectedExtraScope: "cluster:cluster-3",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			handlerCalledFlag := false
+
+			// Create a mock handler that sets the flag when called
+			mockHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				handlerCalledFlag = true
+
+				// Retrieve the user from the context to verify extra scopes
+				userFromCtx, exists := request.UserFrom(r.Context())
+				if !exists {
+					t.Errorf("Expected user in context, but not found")
+					w.WriteHeader(http.StatusInternalServerError)
+					return
+				}
+
+				// Verify that the extra scope is correctly added
+				extra := userFromCtx.GetExtra()
+				expectedScope := tt.expectedExtraScope
+				if expectedScope != "" {
+					scopes, ok := extra["authentication.kcp.io/scopes"]
+					assert.True(t, ok, "Expected 'authentication.kcp.io/scopes' in user extra")
+					assert.Contains(t, scopes, expectedScope, "Expected scope not found in user extra")
+				}
+
+				w.WriteHeader(http.StatusOK)
+			})
+
+			// Wrap the handler with WithScoping
+			wrappedHandler := withScoping(mockHandler)
+
+			// Create a new HTTP request
+			req := httptest.NewRequest(http.MethodGet, "http://kcp.io/foo", http.NoBody)
+			if !tt.skipScopingMarker {
+				req.Header.Set(kcpImpersonationHeader, "true")
+			}
+
+			// Set up context with user and cluster if provided
+			ctx := req.Context()
+			if tt.user != nil {
+				ctx = request.WithUser(ctx, tt.user)
+			}
+			if tt.cluster != nil {
+				ctx = request.WithCluster(ctx, *tt.cluster)
+			}
+			req = req.WithContext(ctx)
+
+			// Create a ResponseRecorder to capture the response
+			rr := httptest.NewRecorder()
+
+			// Serve the HTTP request
+			wrappedHandler.ServeHTTP(rr, req)
+
+			// Assert the expected status code
+			assert.Equal(t, tt.expectedStatus, rr.Code, "Unexpected status code")
+
+			// Assert whether the handler was called
+			assert.Equal(t, tt.handlerCalled, handlerCalledFlag, "Handler called state mismatch")
+		})
+	}
+}