-
Notifications
You must be signed in to change notification settings - Fork 684
Fix MCP Auth issues in API to MCP usecase #13574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 4 commits
c245128
27917de
f1cdcd4
f396d30
099537d
f800a0e
4e2140a
c7016ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -52,6 +52,8 @@ | |||||||||||||||||
| import java.util.HashMap; | ||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||
|
|
||||||||||||||||||
| import static org.wso2.carbon.apimgt.impl.APIConstants.MCP_HTTP_METHOD; | ||||||||||||||||||
|
|
||||||||||||||||||
| public class McpInitHandler extends AbstractHandler implements ManagedLifecycle { | ||||||||||||||||||
| private static final Log log = LogFactory.getLog(McpInitHandler.class); | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -159,6 +161,7 @@ private String buildMCPRequest(MessageContext messageContext) throws McpExceptio | |||||||||||||||||
|
|
||||||||||||||||||
| method = request.getMethod(); | ||||||||||||||||||
| messageContext.setProperty(APIMgtGatewayConstants.MCP_METHOD, method); | ||||||||||||||||||
| messageContext.setProperty(MCP_HTTP_METHOD, method); | ||||||||||||||||||
| messageContext.setProperty(APIMgtGatewayConstants.MCP_REQUEST_BODY, request); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (StringUtils.equals(method, APIConstants.MCP.METHOD_TOOL_CALL)) { | ||||||||||||||||||
|
|
@@ -183,10 +186,14 @@ private String buildMCPRequest(MessageContext messageContext) throws McpExceptio | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| if (backendOperation != null) { | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log Improvement Suggestion No: 2
Suggested change
|
||||||||||||||||||
| messageContext.setProperty("MCP_HTTP_METHOD", backendOperation.getVerb()); | ||||||||||||||||||
| messageContext.setProperty(MCP_HTTP_METHOD, backendOperation.getVerb()); | ||||||||||||||||||
| messageContext.setProperty("MCP_API_ELECTED_RESOURCE", backendOperation.getTarget()); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } else if (StringUtils.equals(method, APIConstants.MCP.METHOD_INITIALIZE)) { | ||||||||||||||||||
| Map<String, String> transportHeaderMap = (Map<String, String>) axis2MC.getProperty | ||||||||||||||||||
| (org.apache.axis2.context.MessageContext.TRANSPORT_HEADERS); | ||||||||||||||||||
|
Comment on lines
+194
to
+195
|
||||||||||||||||||
| Map<String, String> transportHeaderMap = (Map<String, String>) axis2MC.getProperty | |
| (org.apache.axis2.context.MessageContext.TRANSPORT_HEADERS); | |
| Map<String, String> transportHeaderMap = (Map<String, String>) axis2MC.getProperty( | |
| org.apache.axis2.context.MessageContext.TRANSPORT_HEADERS); | |
| if (transportHeaderMap == null) { | |
| transportHeaderMap = new HashMap<>(); | |
| axis2MC.setProperty(org.apache.axis2.context.MessageContext.TRANSPORT_HEADERS, transportHeaderMap); | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,12 +48,15 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.gateway.handlers.security.authenticator.InternalAPIKeyAuthenticator; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.gateway.handlers.security.basicauth.BasicAuthAuthenticator; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.gateway.handlers.security.oauth.OAuthAuthenticator; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.gateway.internal.DataHolder; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.gateway.internal.ServiceReferenceHolder; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.gateway.utils.GatewayUtils; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.gateway.utils.MCPUtils; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.impl.APIConstants; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.impl.APIManagerConfiguration; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.impl.APIManagerConfigurationService; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.impl.dto.KeyManagerDto; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.impl.factory.KeyManagerHolder; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.impl.utils.APIUtil; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.keymgt.model.entity.API; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.wso2.carbon.apimgt.tracing.TracingSpan; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -475,10 +478,13 @@ public boolean handleRequest(MessageContext messageContext) { | |||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (APIConstants.API_TYPE_MCP.equalsIgnoreCase(apiType) && isMCPNoAuthRequest) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| log.debug("Skipping authentication for MCP request" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| + ", method: " + messageContext.getProperty(APIMgtGatewayConstants.MCP_METHOD)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Check if we need to handle same as the no auth case for REST | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (log.isDebugEnabled()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| log.debug("Skipping authentication for MCP request" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| + ", method: " + messageContext.getProperty(APIMgtGatewayConstants.MCP_METHOD)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| handleNoAuthentication(messageContext); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| setAPIParametersToMessageContext(messageContext); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return ExtensionListenerUtil.postProcessRequest(messageContext, type); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+485
to
+487
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log Improvement Suggestion No: 3
Suggested change
Comment on lines
+481
to
+487
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (log.isDebugEnabled()) { | |
| log.debug("Skipping authentication for MCP request" | |
| + ", method: " + messageContext.getProperty(APIMgtGatewayConstants.MCP_METHOD)); | |
| } | |
| handleNoAuthentication(messageContext); | |
| setAPIParametersToMessageContext(messageContext); | |
| return ExtensionListenerUtil.postProcessRequest(messageContext, type); | |
| if (ExtensionListenerUtil.preProcessRequest(messageContext, type)) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Skipping authentication for MCP request" | |
| + ", method: " + messageContext.getProperty(APIMgtGatewayConstants.MCP_METHOD)); | |
| } | |
| handleNoAuthentication(messageContext); | |
| setAPIParametersToMessageContext(messageContext); | |
| return ExtensionListenerUtil.postProcessRequest(messageContext, type); | |
| } | |
| return false; |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resource_metadata URL construction now hardcodes https:// and uses hostHeader directly, which can omit the gateway port when the Host header is missing/invalid and you fall back to APIUtil.getHostAddress(). Previously MCPUtils.getGatewayServerURL(...) used vhost httpsUrl (including port). Consider restoring vhost-based resolution or appending the configured HTTPS port/offset when building the URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log Improvement Suggestion No: 4
| if (keyManagerDto != null && keyManagerDto.getKeyManager() != null) { | |
| try { | |
| org.wso2.carbon.apimgt.api.model.KeyManagerConfiguration config = | |
| keyManagerDto.getKeyManager().getKeyManagerConfiguration(); | |
| return (String) config.getParameter(APIConstants.KeyManager.CLIENT_REGISTRATION_ENDPOINT); | |
| } catch (APIManagementException e) { | |
| if (keyManagerDto != null && keyManagerDto.getKeyManager() != null) { | |
| try { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieving DCR endpoint from key manager: " + keyManagerDto.getName()); | |
| } | |
| org.wso2.carbon.apimgt.api.model.KeyManagerConfiguration config = | |
| keyManagerDto.getKeyManager().getKeyManagerConfiguration(); | |
| return (String) config.getParameter(APIConstants.KeyManager.CLIENT_REGISTRATION_ENDPOINT); | |
| } catch (APIManagementException e) { | |
| log.error("Error while retrieving key manager configuration for MCP DCR support", e); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -68,7 +68,6 @@ | |||||||||||||||||
| import java.security.cert.Certificate; | ||||||||||||||||||
| import java.text.ParseException; | ||||||||||||||||||
| import java.util.ArrayList; | ||||||||||||||||||
| import java.util.Arrays; | ||||||||||||||||||
| import java.util.Date; | ||||||||||||||||||
| import java.util.HashMap; | ||||||||||||||||||
| import java.util.HashSet; | ||||||||||||||||||
|
|
@@ -78,6 +77,8 @@ | |||||||||||||||||
| import java.util.Set; | ||||||||||||||||||
| import javax.cache.Cache; | ||||||||||||||||||
|
|
||||||||||||||||||
| import static org.wso2.carbon.apimgt.impl.APIConstants.MCP_HTTP_METHOD; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * A Validator class to validate JWT tokens in an API request. | ||||||||||||||||||
| */ | ||||||||||||||||||
|
|
@@ -173,7 +174,7 @@ public AuthenticationContext authenticate(SignedJWTInfo signedJWTInfo, MessageCo | |||||||||||||||||
|
|
||||||||||||||||||
| String apiType = (String) synCtx.getProperty(APIMgtGatewayConstants.API_TYPE); | ||||||||||||||||||
| if (org.apache.commons.lang3.StringUtils.equals(APIConstants.API_TYPE_MCP, apiType)) { | ||||||||||||||||||
| Object mcpMethodProperty = synCtx.getProperty("MCP_HTTP_METHOD"); | ||||||||||||||||||
| Object mcpMethodProperty = synCtx.getProperty(MCP_HTTP_METHOD); | ||||||||||||||||||
|
Comment on lines
175
to
+177
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log Improvement Suggestion No: 5
Suggested change
|
||||||||||||||||||
| if (mcpMethodProperty != null) { | ||||||||||||||||||
| httpMethod = mcpMethodProperty.toString(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+177
to
180
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log Improvement Suggestion No: 6
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,6 +63,8 @@ | |||||||||||||||||||
| import java.util.Set; | ||||||||||||||||||||
| import javax.cache.Cache; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import static org.wso2.carbon.apimgt.impl.APIConstants.MCP_HTTP_METHOD; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * An API consumer authenticator which authenticates user requests using | ||||||||||||||||||||
| * the OAuth protocol. This implementation uses some default token/delimiter | ||||||||||||||||||||
|
|
@@ -224,7 +226,7 @@ public AuthenticationResponse authenticate(MessageContext synCtx) throws APIMana | |||||||||||||||||||
| String matchingResource = (String) synCtx.getProperty(APIConstants.API_ELECTED_RESOURCE); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (StringUtils.equals(APIConstants.API_TYPE_MCP, apiType)) { | ||||||||||||||||||||
| httpMethod = synCtx.getProperty("MCP_HTTP_METHOD").toString(); | ||||||||||||||||||||
| httpMethod = synCtx.getProperty(MCP_HTTP_METHOD).toString(); | ||||||||||||||||||||
| matchingResource = (String) synCtx.getProperty("MCP_API_ELECTED_RESOURCE"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
228
to
231
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log Improvement Suggestion No: 7
Suggested change
|
||||||||||||||||||||
| SignedJWTInfo signedJWTInfo = null; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -334,9 +334,11 @@ public static List<String> getAllScopes(API api) { | |
| for (URLMapping urlMapping : api.getResources()) { | ||
| if (urlMapping.getScopes() != null) { | ||
| allScopes.addAll(urlMapping.getScopes()); | ||
| allScopes.add("default"); | ||
| } | ||
| } | ||
| } | ||
| allScopes.add("default"); | ||
| return allScopes; | ||
|
Comment on lines
336
to
341
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -81,10 +81,6 @@ public static boolean validateRequest(McpRequest request) throws McpException { | |||||||||||||||||||||||||||||||||
| throw new McpException(APIConstants.MCP.RpcConstants.INVALID_REQUEST_CODE, | ||||||||||||||||||||||||||||||||||
| APIConstants.MCP.RpcConstants.INVALID_REQUEST_MESSAGE, "Missing method field"); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if (!APIConstants.MCP.ALLOWED_METHODS.contains(method)) { | ||||||||||||||||||||||||||||||||||
| throw new McpException(APIConstants.MCP.RpcConstants.METHOD_NOT_FOUND_CODE, | ||||||||||||||||||||||||||||||||||
| APIConstants.MCP.RpcConstants.METHOD_NOT_FOUND_MESSAGE, "Method " + method + " not found"); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Object id = request.getId(); | ||||||||||||||||||||||||||||||||||
| if (id == null && !APIConstants.MCP.METHOD_NOTIFICATION_INITIALIZED.equals(method)) { | ||||||||||||||||||||||||||||||||||
|
|
@@ -150,13 +146,7 @@ public static void validateInitializeRequest(Object id, McpRequest requestObject | |||||||||||||||||||||||||||||||||
| if (requestObject.getParams() != null) { | ||||||||||||||||||||||||||||||||||
| Params params = requestObject.getParams(); | ||||||||||||||||||||||||||||||||||
| String protocolVersion = params.getProtocolVersion(); | ||||||||||||||||||||||||||||||||||
| if (!StringUtils.isEmpty(protocolVersion)) { | ||||||||||||||||||||||||||||||||||
| if (!APIConstants.MCP.SUPPORTED_PROTOCOL_VERSIONS.contains(protocolVersion)) { | ||||||||||||||||||||||||||||||||||
| throw new McpExceptionWithId(id, APIConstants.MCP.RpcConstants.INVALID_PARAMS_CODE, | ||||||||||||||||||||||||||||||||||
| APIConstants.MCP.PROTOCOL_MISMATCH_ERROR, | ||||||||||||||||||||||||||||||||||
| MCPPayloadGenerator.getInitializeErrorBody(protocolVersion)); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||
| if (StringUtils.isEmpty(protocolVersion)) { | ||||||||||||||||||||||||||||||||||
| throw new McpException(APIConstants.MCP.RpcConstants.INVALID_REQUEST_CODE, | ||||||||||||||||||||||||||||||||||
| APIConstants.MCP.RpcConstants.INVALID_REQUEST_MESSAGE, "Missing protocolVersion field"); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+149
to
152
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log Improvement Suggestion No: 9
Suggested change
|
||||||||||||||||||||||||||||||||||
| } | |
| } | |
| if (!APIConstants.MCP.SUPPORTED_PROTOCOL_VERSIONS.contains(protocolVersion)) { | |
| throw new McpException(APIConstants.MCP.RpcConstants.INVALID_REQUEST_CODE, | |
| APIConstants.MCP.RpcConstants.INVALID_REQUEST_MESSAGE, | |
| "Unsupported protocolVersion: " + protocolVersion); | |
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,6 @@ | |||||
| /* | ||||||
| * Copyright (c) 2017, WSO2 Inc. (http://www.wso2.org) All Rights Reserved. | ||||||
| * | ||||||
| * WSO2 Inc. licenses this file to you under the Apache License, | ||||||
| * Version 2.0 (the "License"); you may not use this file except | ||||||
| * in compliance with the License. | ||||||
| * You may obtain a copy of the License at | ||||||
|
|
@@ -33,12 +32,17 @@ | |||||
| import org.powermock.api.mockito.PowerMockito; | ||||||
| import org.powermock.core.classloader.annotations.PrepareForTest; | ||||||
| import org.powermock.modules.junit4.PowerMockRunner; | ||||||
| import org.wso2.carbon.apimgt.api.model.KeyManager; | ||||||
| import org.wso2.carbon.apimgt.api.model.KeyManagerConfiguration; | ||||||
| import org.wso2.carbon.apimgt.common.gateway.extensionlistener.ExtensionListener; | ||||||
| import org.wso2.carbon.apimgt.gateway.APIMgtGatewayConstants; | ||||||
| import org.wso2.carbon.apimgt.gateway.internal.DataHolder; | ||||||
| import org.wso2.carbon.apimgt.gateway.utils.GatewayUtils; | ||||||
| import org.wso2.carbon.apimgt.impl.APIConstants; | ||||||
| import org.wso2.carbon.apimgt.impl.APIManagerConfiguration; | ||||||
| import org.wso2.carbon.apimgt.impl.APIManagerConfigurationService; | ||||||
| import org.wso2.carbon.apimgt.impl.dto.KeyManagerDto; | ||||||
| import org.wso2.carbon.apimgt.impl.factory.KeyManagerHolder; | ||||||
| import org.wso2.carbon.apimgt.impl.dto.ExtendedJWTConfigurationDto; | ||||||
| import org.wso2.carbon.apimgt.impl.internal.ServiceReferenceHolder; | ||||||
| import org.wso2.carbon.apimgt.impl.utils.APIUtil; | ||||||
|
|
@@ -47,7 +51,10 @@ | |||||
| import org.wso2.carbon.metrics.manager.Timer; | ||||||
| import org.wso2.carbon.utils.multitenancy.MultitenantUtils; | ||||||
|
|
||||||
| import java.util.ArrayList; | ||||||
| import java.util.Collections; | ||||||
|
||||||
| import java.util.Collections; |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test stubs KeyManagerHolder.getKeyManagerByName(anyString(), "default"), but GatewayUtils is statically mocked in setup() so GatewayUtils.getTenantDomain() returns null; anyString() won’t match null and the stub won’t apply, causing getDcrEndpoint() to return null and the assertion to fail. Either stub GatewayUtils.getTenantDomain() to a non-null tenant domain, or change the matcher to accept null (e.g., Mockito.any() for the first arg).
| Mockito.when(KeyManagerHolder.getKeyManagerByName(Mockito.anyString(), Mockito.eq("default"))).thenReturn(keyManagerDto); | |
| Mockito.when(KeyManagerHolder.getKeyManagerByName(Mockito.any(), Mockito.eq("default"))).thenReturn(keyManagerDto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log Improvement Suggestion No: 1