fix(model-registry): scope in-cluster K8s client to SAR-only interface in MCP deployment auth (#7259)

* fix(model-registry): scope in-cluster K8s client to SAR-only interface in MCP deployment auth

Move the MCP server SubjectAccessReview logic into the shared
KubernetesClientInterface (CanVerbMcpServersInNamespace) so
requireMcpDeploymentAccess uses the same K8s client infrastructure
as every other handler. This removes the separate InClusterConfig
path, the cached authorizationv1client.SubjectAccessReviewInterface,
and the package-level sync.Once from mcp_deployment_auth.go.

- InternalKubernetesClient: SAR with explicit user/groups
- TokenKubernetesClient: SelfSubjectAccessReview via user token
- requireMcpDeploymentAccess: delegates to factory client

https: //issues.redhat.com/browse/RHOAIENG-57223
Made-with: Cursor

* Update packages/model-registry/upstream/bff/internal/redhat/handlers/mcp_deployment_auth.go

Co-authored-by: Pushpa Padti <99261071+ppadti@users.noreply.github.com>

* Update packages/model-registry/upstream/bff/internal/redhat/handlers/mcp_deployment_auth.go

Co-authored-by: Pushpa Padti <99261071+ppadti@users.noreply.github.com>

* restore auth method guard in requireMcpDeploymentAccess

---------

Co-authored-by: Pushpa Padti <99261071+ppadti@users.noreply.github.com>

Author: manaswinidas

packages/model-registry/upstream/bff/internal/integrations/kubernetes/client.go

@@ -15,6 +15,9 @@ const CatalogSourceKey = "sources.yaml"
 const CatalogSourceDefaultConfigMapName = "default-catalog-sources"
 const CatalogSourceUserConfigMapName = "model-catalog-sources"
 
+const McpServerAPIGroup = "mcp.x-k8s.io"
+const McpServerResource = "mcpservers"
+
 type KubernetesClientInterface interface {
 	// Service discovery
 	GetServiceNames(ctx context.Context, namespace string) ([]string, error)
@@ -31,6 +34,7 @@ type KubernetesClientInterface interface {
 	CanListServicesInNamespace(ctx context.Context, identity *RequestIdentity, namespace string) (bool, error)
 	CanAccessServiceInNamespace(ctx context.Context, identity *RequestIdentity, namespace, serviceName string) (bool, error)
 	CanNamespaceAccessRegistry(ctx context.Context, identity *RequestIdentity, jobNamespace, registryName, registryNamespace string) (bool, error)
+	CanVerbMcpServersInNamespace(ctx context.Context, identity *RequestIdentity, namespace, verb string) (bool, error)
 	GetSelfSubjectRulesReview(ctx context.Context, identity *RequestIdentity, namespace string) ([]string, error)
 
 	// Meta

packages/model-registry/upstream/bff/internal/integrations/kubernetes/internal_k8s_client.go

@@ -149,6 +149,37 @@ func (kc *InternalKubernetesClient) CanNamespaceAccessRegistry(ctx context.Conte
 	return CanNamespaceAccessRegistry(ctx, kc.Client, kc.Logger, jobNamespace, registryName, registryNamespace)
 }
 
+func (kc *InternalKubernetesClient) CanVerbMcpServersInNamespace(ctx context.Context, identity *RequestIdentity, namespace, verb string) (bool, error) {
+	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+	defer cancel()
+
+	sar := &authv1.SubjectAccessReview{
+		Spec: authv1.SubjectAccessReviewSpec{
+			User:   identity.UserID,
+			Groups: identity.Groups,
+			ResourceAttributes: &authv1.ResourceAttributes{
+				Verb:      verb,
+				Group:     McpServerAPIGroup,
+				Resource:  McpServerResource,
+				Namespace: namespace,
+			},
+		},
+	}
+
+	resp, err := kc.Client.AuthorizationV1().SubjectAccessReviews().Create(ctx, sar, metav1.CreateOptions{})
+	if err != nil {
+		kc.Logger.Error("SAR failed for MCP server access", "user", identity.UserID, "verb", verb, "namespace", namespace, "error", err)
+		return false, err
+	}
+
+	if !resp.Status.Allowed {
+		kc.Logger.Warn("MCP server access denied", "user", identity.UserID, "verb", verb, "namespace", namespace)
+		return false, nil
+	}
+
+	return true, nil
+}
+
 // GetSelfSubjectRulesReview gets the rules for what a user can access in a namespace
 func (kc *InternalKubernetesClient) GetSelfSubjectRulesReview(ctx context.Context, identity *RequestIdentity, namespace string) ([]string, error) {
 	kc.Logger.Warn("GetSelfSubjectRulesReview not fully implemented for internal client",

packages/model-registry/upstream/bff/internal/integrations/kubernetes/token_k8s_client.go

@@ -234,6 +234,35 @@ func (kc *TokenKubernetesClient) CanNamespaceAccessRegistry(ctx context.Context,
 	return CanNamespaceAccessRegistry(ctx, kc.Client, kc.Logger, jobNamespace, registryName, registryNamespace)
 }
 
+func (kc *TokenKubernetesClient) CanVerbMcpServersInNamespace(ctx context.Context, _ *RequestIdentity, namespace, verb string) (bool, error) {
+	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+	defer cancel()
+
+	sar := &authv1.SelfSubjectAccessReview{
+		Spec: authv1.SelfSubjectAccessReviewSpec{
+			ResourceAttributes: &authv1.ResourceAttributes{
+				Verb:      verb,
+				Group:     McpServerAPIGroup,
+				Resource:  McpServerResource,
+				Namespace: namespace,
+			},
+		},
+	}
+
+	resp, err := kc.Client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, sar, metav1.CreateOptions{})
+	if err != nil {
+		kc.Logger.Error("self-SAR failed for MCP server access", "verb", verb, "namespace", namespace, "error", err)
+		return false, err
+	}
+
+	if !resp.Status.Allowed {
+		kc.Logger.Warn("MCP server access denied", "verb", verb, "namespace", namespace)
+		return false, nil
+	}
+
+	return true, nil
+}
+
 // RequestIdentity is unused because the token already represents the user identity.
 // This endpoint is used only on dev mode that is why is safe to ignore permissions errors
 func (kc *TokenKubernetesClient) GetNamespaces(ctx context.Context, _ *RequestIdentity) ([]corev1.Namespace, error) {

packages/model-registry/upstream/bff/internal/redhat/handlers/mcp_deployment_auth.go

@@ -1,59 +1,34 @@
 package handlers
 
 import (
-	"context"
 	"fmt"
 	"log/slog"
 	"net/http"
-	"sync"
-	"time"
-
-	authv1 "k8s.io/api/authorization/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/rest"
 
 	"github.com/kubeflow/model-registry/ui/bff/internal/api"
 	"github.com/kubeflow/model-registry/ui/bff/internal/config"
 	"github.com/kubeflow/model-registry/ui/bff/internal/constants"
 	k8s "github.com/kubeflow/model-registry/ui/bff/internal/integrations/kubernetes"
 )
 
-const (
-	mcpServerSARAPIGroup = "mcp.x-k8s.io"
-	mcpServerSARResource = "mcpservers"
-	sarRequestTimeout    = 30 * time.Second
-)
-
-var (
-	sarClientOnce sync.Once
-	sarClient     kubernetes.Interface
-)
-
-// initSARClient lazily initializes a typed K8s clientset using in-cluster config.
-// Returns nil when not running in a cluster (e.g. federated/dev mode).
-func initSARClient() kubernetes.Interface {
-	sarClientOnce.Do(func() {
-		cfg, err := rest.InClusterConfig()
-		if err != nil {
-			return
-		}
-		sarClient, _ = kubernetes.NewForConfig(cfg)
-	})
-	return sarClient
-}
-
-// requireMcpDeploymentAccess performs a SubjectAccessReview to verify that
-// the requesting user has the given verb on mcpservers in the target namespace.
+// requireMcpDeploymentAccess performs a permission check to verify that the
+// requesting user has the given verb on mcpservers in the target namespace.
+// SAR is only performed when the auth method is "internal" (in-cluster service
+// account). For user-token auth the K8s API server enforces authorization via
+// the user's own bearer token, so no server-side SAR is needed.
 func requireMcpDeploymentAccess(app *api.App, w http.ResponseWriter, r *http.Request, namespace, verb string) bool {
 	if app.Config().AuthMethod != config.AuthMethodInternal {
 		return true
 	}
 
-	client := initSARClient()
+	client, err := app.KubernetesClientFactory().GetClient(r.Context())
+	if err != nil {
+		app.ServerError(w, r, fmt.Errorf("failed to get Kubernetes client for MCP access check: %w", err))
+		return false
+	}
 	if client == nil {
-		app.Logger().Warn("SAR client unavailable in internal auth mode, skipping access check")
-		return true
+		app.ServerError(w, r, fmt.Errorf("kubernetes client factory returned nil client without error"))
+		return false
 	}
 
 	identity, ok := r.Context().Value(constants.RequestIdentityKey).(*k8s.RequestIdentity)
@@ -62,25 +37,9 @@ func requireMcpDeploymentAccess(app *api.App, w http.ResponseWriter, r *http.Req
 		return false
 	}
 
-	ctx, cancel := context.WithTimeout(r.Context(), sarRequestTimeout)
-	defer cancel()
-
-	sar := &authv1.SubjectAccessReview{
-		Spec: authv1.SubjectAccessReviewSpec{
-			User:   identity.UserID,
-			Groups: identity.Groups,
-			ResourceAttributes: &authv1.ResourceAttributes{
-				Verb:      verb,
-				Group:     mcpServerSARAPIGroup,
-				Resource:  mcpServerSARResource,
-				Namespace: namespace,
-			},
-		},
-	}
-
-	response, err := client.AuthorizationV1().SubjectAccessReviews().Create(ctx, sar, metav1.CreateOptions{})
+	allowed, err := client.CanVerbMcpServersInNamespace(r.Context(), identity, namespace, verb)
 	if err != nil {
-		app.Logger().Error("SAR check failed",
+		app.Logger().Error("MCP server access check failed",
 			slog.String("user", identity.UserID),
 			slog.String("verb", verb),
 			slog.String("namespace", namespace),
@@ -90,8 +49,8 @@ func requireMcpDeploymentAccess(app *api.App, w http.ResponseWriter, r *http.Req
 		return false
 	}
 
-	if !response.Status.Allowed {
-		app.Logger().Warn("SAR denied",
+	if !allowed {
+		app.Logger().Warn("MCP server access denied",
 			slog.String("user", identity.UserID),
 			slog.String("verb", verb),
 			slog.String("namespace", namespace),

packages/model-registry/upstream/bff/internal/repositories/model_transfer_jobs_test.go

@@ -140,6 +140,10 @@ func (f *fakeKubernetesClient) CanNamespaceAccessRegistry(ctx context.Context, i
 	return false, nil
 }
 
+func (f *fakeKubernetesClient) CanVerbMcpServersInNamespace(ctx context.Context, identity *k8s.RequestIdentity, namespace, verb string) (bool, error) {
+	return false, nil
+}
+
 func (f *fakeKubernetesClient) GetSelfSubjectRulesReview(ctx context.Context, identity *k8s.RequestIdentity, namespace string) ([]string, error) {
 	return nil, nil
 }