Fix MCP instance OAuth redirects

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",

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")
+			}
+		})
+	}
+}

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",

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 {

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
+}

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)

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
+}