Skip to content

Conversation

@sveneld
Copy link

@sveneld sveneld commented Jan 12, 2026

Motivation and Context

Implementation of OAuth based on middlewares from #218

How Has This Been Tested?

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@chr-hertel
Copy link
Member

Thanks @sveneld - it will take some time for me to give some proper feedback here, just to let you know - needs quite some focus & attention - still really appreciated!

@chr-hertel chr-hertel changed the title Oauth2 based on middleware [Server] Oauth2 based on middleware Jan 23, 2026
@chr-hertel chr-hertel added Server Issues & PRs related to the Server component auth Issues and PRs related to Authentication / OAuth labels Jan 23, 2026
Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi again @sveneld! This is great already - went to a first round of very detailed review directly. Thanks again for working on this! 🙏

Didn't manage to go through all, but dropping the first round of comments.


5. **Use with MCP Inspector:**

The MCP Inspector doesn't support OAuth out of the box, but you can test using curl or build a custom client.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bit unexpected to me since I saw this in inspector:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works right away 💪 👍

Comment on lines 26 to 37
// Configuration from environment
// External URL is what clients use and what appears in tokens
$keycloakExternalUrl = getenv('KEYCLOAK_EXTERNAL_URL') ?: 'http://localhost:8180';
// Internal URL is how this server reaches Keycloak (Docker network)
$keycloakInternalUrl = getenv('KEYCLOAK_INTERNAL_URL') ?: 'http://keycloak:8080';
$keycloakRealm = getenv('KEYCLOAK_REALM') ?: 'mcp';
$mcpAudience = getenv('MCP_AUDIENCE') ?: 'mcp-server';

// Issuer is what appears in the token (external URL)
$issuer = rtrim($keycloakExternalUrl, '/').'/realms/'.$keycloakRealm;
// JWKS URI uses internal URL to reach Keycloak within Docker network
$jwksUri = rtrim($keycloakInternalUrl, '/').'/realms/'.$keycloakRealm.'/protocol/openid-connect/certs';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fair to hard-code the values here instead of using getenv

Comment on lines 39 to 46
// Create logger
$logger = new class extends AbstractLogger {
public function log($level, \Stringable|string $message, array $context = []): void
{
$logMessage = sprintf("[%s] %s\n", strtoupper($level), $message);
error_log($logMessage);
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use the logger() provided by the examples/bootstrap.php file? would be more consistent with other examples or is there a particular reason not to?

$server = Server::builder()
->setServerInfo('OAuth Keycloak Example', '1.0.0')
->setLogger($logger)
->setSession(new FileSessionStore('/tmp/mcp-sessions'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be more consistent with other examples:

Suggested change
->setSession(new FileSessionStore('/tmp/mcp-sessions'))
->setSession(new FileSessionStore(__DIR__.'/sessions'))

$response = $this->httpClient->sendRequest($request);

if ($response->getStatusCode() >= 400) {
throw new \RuntimeException(sprintf(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use package specific exceptions: Mcp\Exception\RuntimeException

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

below as well, please


$response = $this->httpClient->sendRequest($request);

if ($response->getStatusCode() >= 400) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this would be:

Suggested change
if ($response->getStatusCode() >= 400) {
if (200 !== $response->getStatusCode()) {

our expectations here are quite clear, right?

$tokenIssuer = $claims['iss'];
$expectedIssuers = \is_array($this->issuer) ? $this->issuer : [$this->issuer];

return \in_array($tokenIssuer, $expectedIssuers, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$tokenIssuer can be inlined:

Suggested change
return \in_array($tokenIssuer, $expectedIssuers, true);
return \in_array($claims['iss'], $expectedIssuers, true);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be great to have tests for this class

*
* @author Volodymyr Panivko <sveneld300@gmail.com>
*/
class AuthorizationResult
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class AuthorizationResult
final class AuthorizationResult

@sveneld
Copy link
Author

sveneld commented Jan 28, 2026

@chr-hertel I updated pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Issues and PRs related to Authentication / OAuth Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants