diff --git a/pkg/api/authz/resources.go b/pkg/api/authz/resources.go index 3faa4943a5..661e723990 100644 --- a/pkg/api/authz/resources.go +++ b/pkg/api/authz/resources.go @@ -29,6 +29,7 @@ var apiResources = map[string][]string{ "GET /api/mcp-audit-logs/{mcp_id}", "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", "POST /api/mcp-server-instances", "DELETE /api/mcp-server-instances/{mcp_server_instance_id}", "DELETE /api/mcp-server-instances/{mcp_server_instance_id}/oauth", diff --git a/pkg/api/handlers/generic_oauth_validation_test.go b/pkg/api/handlers/generic_oauth_validation_test.go index cdbffd2c3e..a43751a161 100644 --- a/pkg/api/handlers/generic_oauth_validation_test.go +++ b/pkg/api/handlers/generic_oauth_validation_test.go @@ -68,7 +68,7 @@ func TestValidateGenericOAuthConfigRejectsIssuerMismatch(t *testing.T) { } func TestValidateGenericOAuthConfigRejectsDiscoveryHTTPError(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { http.Error(w, "unavailable", http.StatusServiceUnavailable) })) t.Cleanup(server.Close) @@ -83,8 +83,8 @@ func TestValidateGenericOAuthConfigRejectsDiscoveryHTTPError(t *testing.T) { } func TestValidateGenericOAuthConfigRejectsMalformedDiscovery(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte(`{`)) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{`)) })) t.Cleanup(server.Close) diff --git a/pkg/api/handlers/serverinstances.go b/pkg/api/handlers/serverinstances.go index df8d54e763..63be3ca474 100644 --- a/pkg/api/handlers/serverinstances.go +++ b/pkg/api/handlers/serverinstances.go @@ -22,14 +22,16 @@ import ( ) type ServerInstancesHandler struct { - acrHelper *accesscontrolrule.Helper - serverURL string + acrHelper *accesscontrolrule.Helper + mcpOAuthChecker MCPOAuthChecker + serverURL string } -func NewServerInstancesHandler(acrHelper *accesscontrolrule.Helper, serverURL string) *ServerInstancesHandler { +func NewServerInstancesHandler(acrHelper *accesscontrolrule.Helper, mcpOAuthChecker MCPOAuthChecker, serverURL string) *ServerInstancesHandler { return &ServerInstancesHandler{ - acrHelper: acrHelper, - serverURL: serverURL, + acrHelper: acrHelper, + mcpOAuthChecker: mcpOAuthChecker, + serverURL: serverURL, } } @@ -193,6 +195,32 @@ func (h *ServerInstancesHandler) ClearOAuthCredentials(req api.Context) error { return nil } +func (h *ServerInstancesHandler) GetOAuthURL(req api.Context) error { + var instance v1.MCPServerInstance + if err := req.Get(&instance, req.PathValue("mcp_server_instance_id")); err != nil { + return err + } + if instance.Spec.UserID != req.User.GetUID() { + return types.NewErrNotFound("MCP server instance not found") + } + + // allowMissingConfig: report OAuth state even when the instance is not yet + // fully configured (e.g. required headers unset) so the probe never 500s on + // a half-set-up instance. For remote catalog servers (NeedsURL=false) this + // 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) + } + + 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 req.Write(map[string]string{"oauthURL": u}) +} + func (h *ServerInstancesHandler) ConfigureServerInstance(req api.Context) error { var mcpServerInstance v1.MCPServerInstance if err := req.Get(&mcpServerInstance, req.PathValue("mcp_server_instance_id")); err != nil { diff --git a/pkg/api/handlers/serverinstances_test.go b/pkg/api/handlers/serverinstances_test.go new file mode 100644 index 0000000000..aa0e1fff02 --- /dev/null +++ b/pkg/api/handlers/serverinstances_test.go @@ -0,0 +1,45 @@ +package handlers + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/obot-platform/obot/apiclient/types" + "github.com/obot-platform/obot/pkg/api" + 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" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kuser "k8s.io/apiserver/pkg/authentication/user" +) + +// GetOAuthURL must not leak another user's instance: the ownership guard runs +// before any server/credential resolution, so a non-owner gets NotFound and the +// nil mcpOAuthChecker is never reached. +func TestServerInstanceGetOAuthURLRejectsNonOwner(t *testing.T) { + instance := v1.MCPServerInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "instance-1", + Namespace: system.DefaultNamespace, + }, + Spec: v1.MCPServerInstanceSpec{ + UserID: "owner-uid", + MCPServerName: "server-1", + }, + } + + req := httptest.NewRequest(http.MethodGet, "/api/mcp-server-instances/instance-1/oauth-url", nil) + req.SetPathValue("mcp_server_instance_id", "instance-1") + + err := (&ServerInstancesHandler{}).GetOAuthURL(api.Context{ + ResponseWriter: httptest.NewRecorder(), + Request: req, + Storage: newFakeStorage(t, &instance), + User: &kuser.DefaultInfo{UID: "intruder-uid"}, + }) + + require.Error(t, err) + assert.True(t, types.IsNotFound(err), "expected not found error, got %v", err) +} diff --git a/pkg/api/router/router.go b/pkg/api/router/router.go index f457c77aab..be302e7b16 100644 --- a/pkg/api/router/router.go +++ b/pkg/api/router/router.go @@ -90,7 +90,7 @@ func Router(ctx context.Context, services *services.Services) (http.Handler, err mcpGateway := mcpgateway.NewHandler(services.MCPSessionManager) mcpAuditLogs := mcpgateway.NewAuditLogHandler() auditLogExports := handlers.NewAuditLogExportHandler(services.GatewayClient) - serverInstances := handlers.NewServerInstancesHandler(services.AccessControlRuleHelper, services.ServerURL) + serverInstances := handlers.NewServerInstancesHandler(services.AccessControlRuleHelper, oauthChecker, services.ServerURL) systemMCPServers := handlers.NewSystemMCPServerHandler(services.MCPSessionManager) userDefaultRoleSettings := handlers.NewUserDefaultRoleSettingHandler() setupHandler := setup.NewHandler(services.ServerURL, services.Bootstrapper) @@ -159,6 +159,7 @@ func Router(ctx context.Context, services *services.Services) (http.Handler, err mux.HandleFunc("POST /api/mcp-server-instances/{mcp_server_instance_id}/deconfigure", serverInstances.DeconfigureServerInstance) 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) // MCP Catalogs (admin only) mux.HandleFunc("GET /api/mcp-catalogs", mcpCatalogs.List) diff --git a/pkg/oidcjwt/smokeclient/main.go b/pkg/oidcjwt/smokeclient/main.go index 656d5a2883..0612c5f047 100644 --- a/pkg/oidcjwt/smokeclient/main.go +++ b/pkg/oidcjwt/smokeclient/main.go @@ -58,7 +58,7 @@ func main() { os.Exit(1) } }() - defer server.Shutdown(context.Background()) + defer func() { _ = server.Shutdown(context.Background()) }() token, err := mintJWT(priv, "smoke-key", *issuerURL, *audience, *subject, *email, splitCSV(*roles), *eligible, *ttl) exitOnErr("mint jwt", err) diff --git a/pkg/oidcjwt/testutil/testutil.go b/pkg/oidcjwt/testutil/testutil.go index cf176275bf..1ca07cadb5 100644 --- a/pkg/oidcjwt/testutil/testutil.go +++ b/pkg/oidcjwt/testutil/testutil.go @@ -42,7 +42,7 @@ func NewTestIssuer(t *testing.T, priv *rsa.PrivateKey, kid string) (*TestIssuer, "subject_types_supported": []string{"public"}, }) }) - mux.HandleFunc("/.well-known/jwks.json", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/.well-known/jwks.json", func(w http.ResponseWriter, _ *http.Request) { ti.mu.Lock() defer ti.mu.Unlock()