From 595a8e45bb06b3cfe9bbf848f83d786b2428f2ed Mon Sep 17 00:00:00 2001 From: Hemanta Banerjee Date: Fri, 3 Jul 2026 10:25:37 +0800 Subject: [PATCH 1/3] test: prove MCP catalog header contracts --- apiclient/types/mcpserver_test.go | 59 +++++++ .../mcpcatalog/mcpcatalog_sync_test.go | 152 ++++++++++++++++++ pkg/mcp/types_test.go | 75 +++++++++ 3 files changed, 286 insertions(+) create mode 100644 pkg/controller/handlers/mcpcatalog/mcpcatalog_sync_test.go diff --git a/apiclient/types/mcpserver_test.go b/apiclient/types/mcpserver_test.go index 74b07b6a1c..226446865e 100644 --- a/apiclient/types/mcpserver_test.go +++ b/apiclient/types/mcpserver_test.go @@ -260,6 +260,65 @@ func TestMapCatalogEntryToServer_RemoteFixedURL(t *testing.T) { } } +func TestMapCatalogEntryToServer_RemoteFixedURLKeepsRemoteHeadersAsServerConfig(t *testing.T) { + header := MCPHeader{Name: "API Key", Key: "X-API-Key", Required: true, Sensitive: true} + catalogEntry := MCPServerCatalogEntryManifest{ + Name: "Test Remote Server", + Runtime: RuntimeRemote, + RemoteConfig: &RemoteCatalogConfig{ + FixedURL: "https://api.example.com/mcp", + Headers: []MCPHeader{header}, + }, + } + + result, err := MapCatalogEntryToServer(catalogEntry, "", false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if result.MultiUserConfig != nil { + t.Fatalf("Expected remote headers to remain server config, got multi-user config %#v", result.MultiUserConfig) + } + if result.RemoteConfig == nil || len(result.RemoteConfig.Headers) != 1 { + t.Fatalf("Expected one remote header, got %#v", result.RemoteConfig) + } + if result.RemoteConfig.Headers[0] != header { + t.Fatalf("Expected remote header %#v, got %#v", header, result.RemoteConfig.Headers[0]) + } +} + +func TestMapCatalogEntryToServer_RemoteFixedURLCopiesMultiUserHeadersSeparately(t *testing.T) { + header := MCPHeader{Name: "API Key", Key: "X-API-Key", Required: true, Sensitive: true} + catalogEntry := MCPServerCatalogEntryManifest{ + Name: "Test Remote Server", + Runtime: RuntimeRemote, + RemoteConfig: &RemoteCatalogConfig{ + FixedURL: "https://api.example.com/mcp", + }, + MultiUserConfig: &MultiUserConfig{ + UserDefinedHeaders: []MCPHeader{header}, + }, + } + + result, err := MapCatalogEntryToServer(catalogEntry, "", false) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if result.RemoteConfig == nil { + t.Fatal("Expected RemoteConfig to be populated") + } + if len(result.RemoteConfig.Headers) != 0 { + t.Fatalf("Expected no remote headers, got %#v", result.RemoteConfig.Headers) + } + if result.MultiUserConfig == nil || len(result.MultiUserConfig.UserDefinedHeaders) != 1 { + t.Fatalf("Expected one multi-user header, got %#v", result.MultiUserConfig) + } + if result.MultiUserConfig.UserDefinedHeaders[0] != header { + t.Fatalf("Expected multi-user header %#v, got %#v", header, result.MultiUserConfig.UserDefinedHeaders[0]) + } +} + func TestMapCatalogEntryToServer_RemoteHostname(t *testing.T) { catalogEntry := MCPServerCatalogEntryManifest{ Name: "Test Remote Server", diff --git a/pkg/controller/handlers/mcpcatalog/mcpcatalog_sync_test.go b/pkg/controller/handlers/mcpcatalog/mcpcatalog_sync_test.go new file mode 100644 index 0000000000..09c8c03612 --- /dev/null +++ b/pkg/controller/handlers/mcpcatalog/mcpcatalog_sync_test.go @@ -0,0 +1,152 @@ +package mcpcatalog + +import ( + "context" + "os" + "path/filepath" + "slices" + "testing" + "time" + + "github.com/obot-platform/nah/pkg/router" + gatewayclient "github.com/obot-platform/obot/pkg/gateway/client" + gatewaydb "github.com/obot-platform/obot/pkg/gateway/db" + v1 "github.com/obot-platform/obot/pkg/storage/apis/obot.obot.ai/v1" + storagescheme "github.com/obot-platform/obot/pkg/storage/scheme" + storageservices "github.com/obot-platform/obot/pkg/storage/services" + "github.com/obot-platform/obot/pkg/system" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + kclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestSyncSkipsRecentCatalogAndForceSyncPrunesRemovedEntries(t *testing.T) { + ctx := context.Background() + catalogDir := t.TempDir() + writeCatalogEntry(t, catalogDir, "alpha.yaml", "Alpha") + writeCatalogEntry(t, catalogDir, "beta.yaml", "Beta") + + catalog := &v1.MCPCatalog{ + TypeMeta: metav1.TypeMeta{ + APIVersion: v1.SchemeGroupVersion.String(), + Kind: "MCPCatalog", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: system.DefaultCatalog, + Namespace: system.DefaultNamespace, + }, + Spec: v1.MCPCatalogSpec{ + DisplayName: "Default", + SourceURLs: []string{catalogDir}, + }, + } + storageClient := newCatalogFakeClient(catalog) + gatewayClient := newCatalogGatewayClient(t, storageClient) + handler := &Handler{gatewayClient: gatewayClient} + + resp := &router.ResponseWrapper{} + require.NoError(t, handler.Sync(syncRequest(ctx, storageClient, catalog), resp)) + require.ElementsMatch(t, []string{"Alpha", "Beta"}, catalogEntryManifestNames(t, ctx, storageClient)) + + require.NoError(t, os.Remove(filepath.Join(catalogDir, "beta.yaml"))) + synced := getCatalog(t, ctx, storageClient) + + resp = &router.ResponseWrapper{} + require.NoError(t, handler.Sync(syncRequest(ctx, storageClient, synced), resp)) + require.True(t, resp.Delay > 0 && resp.Delay <= time.Hour) + require.ElementsMatch(t, []string{"Alpha", "Beta"}, catalogEntryManifestNames(t, ctx, storageClient)) + + forceSync := getCatalog(t, ctx, storageClient) + forceSync.Annotations[v1.MCPCatalogSyncAnnotation] = "true" + require.NoError(t, storageClient.Update(ctx, forceSync)) + + resp = &router.ResponseWrapper{} + require.NoError(t, handler.Sync(syncRequest(ctx, storageClient, getCatalog(t, ctx, storageClient)), resp)) + require.Equal(t, time.Hour, resp.Delay) + require.Equal(t, []string{"Alpha"}, catalogEntryManifestNames(t, ctx, storageClient)) + + afterForce := getCatalog(t, ctx, storageClient) + require.NotContains(t, afterForce.Annotations, v1.MCPCatalogSyncAnnotation) +} + +func newCatalogFakeClient(objects ...kclient.Object) kclient.WithWatch { + restMapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{v1.SchemeGroupVersion}) + restMapper.Add(v1.SchemeGroupVersion.WithKind("MCPCatalog"), meta.RESTScopeNamespace) + restMapper.Add(v1.SchemeGroupVersion.WithKind("MCPServerCatalogEntry"), meta.RESTScopeNamespace) + + return fake.NewClientBuilder(). + WithScheme(storagescheme.Scheme). + WithRESTMapper(restMapper). + WithStatusSubresource(&v1.MCPCatalog{}, &v1.MCPServerCatalogEntry{}). + WithObjects(objects...). + Build() +} + +func newCatalogGatewayClient(t *testing.T, storageClient kclient.WithWatch) *gatewayclient.Client { + t.Helper() + + services, err := storageservices.New(storageservices.Config{DSN: "sqlite://:memory:"}) + require.NoError(t, err) + db, err := gatewaydb.New(services.DB.DB, services.DB.SQLDB, true) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate()) + + ctx, cancel := context.WithCancel(context.Background()) + client := gatewayclient.New(ctx, db, storageClient, nil, nil, nil, time.Hour, 100, 1) + t.Cleanup(func() { + cancel() + require.NoError(t, client.Close()) + }) + return client +} + +func syncRequest(ctx context.Context, storageClient kclient.WithWatch, catalog *v1.MCPCatalog) router.Request { + return router.Request{ + Client: storageClient, + Object: catalog, + Ctx: ctx, + Namespace: catalog.Namespace, + Name: catalog.Name, + Key: catalog.Namespace + "/" + catalog.Name, + } +} + +func writeCatalogEntry(t *testing.T, dir, filename, name string) { + t.Helper() + + content := `name: ` + name + ` +description: ` + name + ` remote MCP +runtime: remote +serverUserType: multiUser +remoteConfig: + fixedURL: https://example.com/mcp +` + require.NoError(t, os.WriteFile(filepath.Join(dir, filename), []byte(content), 0o600)) +} + +func catalogEntryManifestNames(t *testing.T, ctx context.Context, storageClient kclient.Client) []string { + t.Helper() + + var entries v1.MCPServerCatalogEntryList + require.NoError(t, storageClient.List(ctx, &entries, kclient.InNamespace(system.DefaultNamespace))) + names := make([]string, 0, len(entries.Items)) + for _, entry := range entries.Items { + names = append(names, entry.Spec.Manifest.Name) + } + slices.Sort(names) + return names +} + +func getCatalog(t *testing.T, ctx context.Context, storageClient kclient.Client) *v1.MCPCatalog { + t.Helper() + + var catalog v1.MCPCatalog + require.NoError(t, storageClient.Get(ctx, kclient.ObjectKey{Namespace: system.DefaultNamespace, Name: system.DefaultCatalog}, &catalog)) + if catalog.Annotations == nil { + catalog.Annotations = map[string]string{} + } + return &catalog +} diff --git a/pkg/mcp/types_test.go b/pkg/mcp/types_test.go index 9d7c5b3caf..fa61838324 100644 --- a/pkg/mcp/types_test.go +++ b/pkg/mcp/types_test.go @@ -181,6 +181,81 @@ func TestServerToServerConfig_MultiUserPassthroughHeaders(t *testing.T) { } } +func TestServerToServerConfig_RemoteHeadersAreDeploymentConfigAndMultiUserHeadersArePassthrough(t *testing.T) { + baseURL := "http://localhost:8080" + requiredHeader := types.MCPHeader{Key: "X-API-Key", Required: true, Sensitive: true} + tests := []struct { + name string + manifest types.MCPServerManifest + credEnv map[string]string + expectedMissing []string + expectedHeaders []string + expectedPassthroughHeaders []string + }{ + { + name: "remote header without deployment credential is missing server config", + manifest: types.MCPServerManifest{ + Runtime: types.RuntimeRemote, + RemoteConfig: &types.RemoteRuntimeConfig{ + URL: "https://example.com/mcp", + Headers: []types.MCPHeader{requiredHeader}, + }, + }, + expectedMissing: []string{"X-API-Key"}, + }, + { + name: "multi-user header is passthrough and not missing server config", + manifest: types.MCPServerManifest{ + Runtime: types.RuntimeRemote, + RemoteConfig: &types.RemoteRuntimeConfig{ + URL: "https://example.com/mcp", + }, + MultiUserConfig: &types.MultiUserConfig{ + UserDefinedHeaders: []types.MCPHeader{requiredHeader}, + }, + }, + expectedPassthroughHeaders: []string{"X-API-Key"}, + }, + { + name: "remote header with deployment credential becomes server header", + manifest: types.MCPServerManifest{ + Runtime: types.RuntimeRemote, + RemoteConfig: &types.RemoteRuntimeConfig{ + URL: "https://example.com/mcp", + Headers: []types.MCPHeader{requiredHeader}, + }, + }, + credEnv: map[string]string{"X-API-Key": "server-secret"}, + expectedHeaders: []string{"X-API-Key=server-secret"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mcpServer := v1.MCPServer{ + Spec: v1.MCPServerSpec{ + Manifest: tt.manifest, + }, + } + mcpServer.Name = "test-server" + + config, missing, err := ServerToServerConfig(mcpServer, mcpServer.ValidConnectURLs(baseURL), baseURL, "test-user-id", "test-scope", "test-catalog", tt.credEnv, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !slices.Equal(missing, tt.expectedMissing) { + t.Fatalf("expected missing config %v, got %v", tt.expectedMissing, missing) + } + if !slices.Equal(config.Headers, tt.expectedHeaders) { + t.Fatalf("expected server headers %v, got %v", tt.expectedHeaders, config.Headers) + } + if !slices.Equal(config.PassthroughHeaderNames, tt.expectedPassthroughHeaders) { + t.Fatalf("expected passthrough headers %v, got %v", tt.expectedPassthroughHeaders, config.PassthroughHeaderNames) + } + }) + } +} + func TestServerToServerConfig_StaticHeaders_Remote(t *testing.T) { baseURL := "http://localhost:8080" tests := []struct { From d2e856a60ff71f6f7e3544e3231d99cd1e60ae3c Mon Sep 17 00:00:00 2001 From: Hemanta Banerjee Date: Fri, 3 Jul 2026 11:08:43 +0800 Subject: [PATCH 2/3] fix: redirect MCP instance OAuth after Obot login --- pkg/api/handlers/serverinstances.go | 33 +++++++++++++--- pkg/api/handlers/serverinstances_test.go | 48 ++++++++++++++++++++++++ pkg/api/router/router.go | 1 + 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/pkg/api/handlers/serverinstances.go b/pkg/api/handlers/serverinstances.go index 63be3ca474..b82ebc796b 100644 --- a/pkg/api/handlers/serverinstances.go +++ b/pkg/api/handlers/serverinstances.go @@ -196,12 +196,35 @@ func (h *ServerInstancesHandler) ClearOAuthCredentials(req api.Context) error { } func (h *ServerInstancesHandler) GetOAuthURL(req api.Context) error { + u, err := h.oauthURLForInstance(req) + if err != nil { + return err + } + + return req.Write(map[string]string{"oauthURL": u}) +} + +func (h *ServerInstancesHandler) RedirectOAuthURL(req api.Context) error { + u, err := h.oauthURLForInstance(req) + if err != nil { + return err + } + if u == "" { + http.Redirect(req.ResponseWriter, req.Request, "/auth/oauth/complete", http.StatusFound) + return nil + } + + http.Redirect(req.ResponseWriter, req.Request, u, http.StatusFound) + return nil +} + +func (h *ServerInstancesHandler) oauthURLForInstance(req api.Context) (string, error) { var instance v1.MCPServerInstance if err := req.Get(&instance, req.PathValue("mcp_server_instance_id")); err != nil { - return err + return "", err } if instance.Spec.UserID != req.User.GetUID() { - return types.NewErrNotFound("MCP server instance not found") + return "", types.NewErrNotFound("MCP server instance not found") } // allowMissingConfig: report OAuth state even when the instance is not yet @@ -210,15 +233,15 @@ func (h *ServerInstancesHandler) GetOAuthURL(req api.Context) error { // is identical to the strict path. server, serverConfig, _, err := serverFromMCPServerInstance(req, instance, true) if err != nil { - return fmt.Errorf("failed to resolve MCP server instance: %w", err) + return "", fmt.Errorf("failed to resolve MCP server instance: %w", err) } u, err := h.mcpOAuthChecker.CheckForMCPAuth(req, server, serverConfig, req.User.GetUID(), instance.Name, "") if err != nil { - return fmt.Errorf("failed to get OAuth URL: %w", err) + return "", fmt.Errorf("failed to get OAuth URL: %w", err) } - return req.Write(map[string]string{"oauthURL": u}) + return u, nil } func (h *ServerInstancesHandler) ConfigureServerInstance(req api.Context) error { diff --git a/pkg/api/handlers/serverinstances_test.go b/pkg/api/handlers/serverinstances_test.go index aa0e1fff02..83dad64f52 100644 --- a/pkg/api/handlers/serverinstances_test.go +++ b/pkg/api/handlers/serverinstances_test.go @@ -7,6 +7,7 @@ import ( "github.com/obot-platform/obot/apiclient/types" "github.com/obot-platform/obot/pkg/api" + "github.com/obot-platform/obot/pkg/mcp" v1 "github.com/obot-platform/obot/pkg/storage/apis/obot.obot.ai/v1" "github.com/obot-platform/obot/pkg/system" "github.com/stretchr/testify/assert" @@ -43,3 +44,50 @@ func TestServerInstanceGetOAuthURLRejectsNonOwner(t *testing.T) { require.Error(t, err) assert.True(t, types.IsNotFound(err), "expected not found error, got %v", err) } + +func TestServerInstanceRedirectOAuthURLRedirectsOwnedInstance(t *testing.T) { + instance := v1.MCPServerInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "instance-1", + Namespace: system.DefaultNamespace, + }, + Spec: v1.MCPServerInstanceSpec{ + UserID: "owner-uid", + MCPServerName: "server-1", + }, + } + server := v1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "server-1", + Namespace: system.DefaultNamespace, + }, + Spec: v1.MCPServerSpec{ + NeedsURL: true, + }, + } + + req := httptest.NewRequest(http.MethodGet, "/api/mcp-server-instances/instance-1/oauth-redirect", nil) + req.SetPathValue("mcp_server_instance_id", "instance-1") + rec := httptest.NewRecorder() + + err := (&ServerInstancesHandler{ + mcpOAuthChecker: fixedOAuthChecker{url: "https://oauth.example.test/authorize?state=state-1"}, + }).RedirectOAuthURL(api.Context{ + ResponseWriter: rec, + Request: req, + Storage: newFakeStorage(t, &instance, &server), + User: &kuser.DefaultInfo{UID: "owner-uid"}, + }) + + require.NoError(t, err) + assert.Equal(t, http.StatusFound, rec.Code) + assert.Equal(t, "https://oauth.example.test/authorize?state=state-1", rec.Header().Get("Location")) +} + +type fixedOAuthChecker struct { + url string +} + +func (f fixedOAuthChecker) CheckForMCPAuth(api.Context, v1.MCPServer, mcp.ServerConfig, string, string, string) (string, error) { + return f.url, nil +} diff --git a/pkg/api/router/router.go b/pkg/api/router/router.go index be302e7b16..856817770a 100644 --- a/pkg/api/router/router.go +++ b/pkg/api/router/router.go @@ -160,6 +160,7 @@ func Router(ctx context.Context, services *services.Services) (http.Handler, err mux.HandleFunc("DELETE /api/mcp-server-instances/{mcp_server_instance_id}", serverInstances.DeleteServerInstance) mux.HandleFunc("DELETE /api/mcp-server-instances/{mcp_server_instance_id}/oauth", serverInstances.ClearOAuthCredentials) mux.HandleFunc("GET /api/mcp-server-instances/{mcp_server_instance_id}/oauth-url", serverInstances.GetOAuthURL) + mux.HandleFunc("GET /api/mcp-server-instances/{mcp_server_instance_id}/oauth-redirect", serverInstances.RedirectOAuthURL) // MCP Catalogs (admin only) mux.HandleFunc("GET /api/mcp-catalogs", mcpCatalogs.List) From e9a50159695bd351a7a237abc3bb0f3a71675e2d Mon Sep 17 00:00:00 2001 From: Hemanta Banerjee Date: Fri, 3 Jul 2026 13:30:14 +0800 Subject: [PATCH 3/3] fix: allow MCP instance OAuth redirects --- pkg/api/authz/mcpoauth_test.go | 41 ++++++++++++++++++++++++++++++++++ pkg/api/authz/resources.go | 1 + 2 files changed, 42 insertions(+) diff --git a/pkg/api/authz/mcpoauth_test.go b/pkg/api/authz/mcpoauth_test.go index d1b3e0425d..2d7f98ea4f 100644 --- a/pkg/api/authz/mcpoauth_test.go +++ b/pkg/api/authz/mcpoauth_test.go @@ -117,3 +117,44 @@ func TestMCPGroupDeniesNonMCPAPIRoutes(t *testing.T) { }) } } + +func TestAPIGroupAllowsOwnedMCPServerInstanceOAuthRoutes(t *testing.T) { + storage := clientfake.NewClientBuilder().WithScheme(storagescheme.Scheme).WithObjects(&v1.MCPServerInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "instance-1", + Namespace: system.DefaultNamespace, + }, + Spec: v1.MCPServerInstanceSpec{ + UserID: "owner-uid", + }, + }).Build() + authorizer := NewAuthorizer(nil, storage, storage, false, nil, nil, false) + apiUser := &user.DefaultInfo{ + Name: "owner", + UID: "owner-uid", + Groups: []string{types.GroupAPI, types.GroupAuthenticated}, + } + + tests := []struct { + name string + path string + }{ + { + name: "oauth url", + path: "/api/mcp-server-instances/instance-1/oauth-url", + }, + { + name: "oauth redirect", + path: "/api/mcp-server-instances/instance-1/oauth-redirect", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tt.path, nil) + if allowed := authorizer.Authorize(req, apiUser); !allowed { + t.Fatalf("Authorize() = false, want true") + } + }) + } +} diff --git a/pkg/api/authz/resources.go b/pkg/api/authz/resources.go index 661e723990..9ec6ce673d 100644 --- a/pkg/api/authz/resources.go +++ b/pkg/api/authz/resources.go @@ -30,6 +30,7 @@ var apiResources = map[string][]string{ "GET /api/mcp-server-instances", "GET /api/mcp-server-instances/{mcp_server_instance_id}", "GET /api/mcp-server-instances/{mcp_server_instance_id}/oauth-url", + "GET /api/mcp-server-instances/{mcp_server_instance_id}/oauth-redirect", "POST /api/mcp-server-instances", "DELETE /api/mcp-server-instances/{mcp_server_instance_id}", "DELETE /api/mcp-server-instances/{mcp_server_instance_id}/oauth",