Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/api/authz/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 3 additions & 3 deletions pkg/api/handlers/generic_oauth_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)

Expand Down
38 changes: 33 additions & 5 deletions pkg/api/handlers/serverinstances.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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 {
Expand Down
45 changes: 45 additions & 0 deletions pkg/api/handlers/serverinstances_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
3 changes: 2 additions & 1 deletion pkg/api/router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/oidcjwt/smokeclient/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/oidcjwt/testutil/testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
Loading