[Server] Oauth2 based on middleware#221
[Server] Oauth2 based on middleware#221sveneld wants to merge 3 commits intomodelcontextprotocol:mainfrom
Conversation
sveneld
requested review from
CodeWithKyrian,
Nyholm and
chr-hertel
as code owners
|
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
changed the title
chr-hertel
added
Server
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
This was a bit unexpected to me since I saw this in inspector:
| // 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'; |
There was a problem hiding this comment.
I think it's fair to hard-code the values here instead of using getenv
| // 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); | ||
| } | ||
| }; |
There was a problem hiding this comment.
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')) |
There was a problem hiding this comment.
to be more consistent with other examples:
| ->setSession(new FileSessionStore('/tmp/mcp-sessions')) | |
| ->setSession(new FileSessionStore(__DIR__.'/sessions')) |
| $response = $this->httpClient->sendRequest($request); | ||
|
|
||
| if ($response->getStatusCode() >= 400) { | ||
| throw new \RuntimeException(sprintf( |
There was a problem hiding this comment.
please use package specific exceptions: Mcp\Exception\RuntimeException
|
|
||
| $response = $this->httpClient->sendRequest($request); | ||
|
|
||
| if ($response->getStatusCode() >= 400) { |
There was a problem hiding this comment.
i wonder if this would be:
| 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); |
There was a problem hiding this comment.
$tokenIssuer can be inlined:
| return \in_array($tokenIssuer, $expectedIssuers, true); | |
| return \in_array($claims['iss'], $expectedIssuers, true); |
There was a problem hiding this comment.
would be great to have tests for this class
| * | ||
| * @author Volodymyr Panivko <sveneld300@gmail.com> | ||
| */ | ||
| class AuthorizationResult |
There was a problem hiding this comment.
| class AuthorizationResult | |
| final class AuthorizationResult |
|
@chr-hertel I updated pull request |
|
Can you please rebase this now when the Middleware Handler is merged? |
sveneld
force-pushed
the
oauth_middleware
branch
from
84ad698 to
9d801c9
Compare
Signed-off-by: Volodymyr Panivko <sveneld300@gmail.com>
sveneld
force-pushed
the
oauth_middleware
branch
from
9d801c9 to
eb2a422
Compare
Done. |
Nyholm
left a comment
Nyholm
left a comment
There was a problem hiding this comment.
Great. Looks good.
I only have small things to comment at. I have not tested it out.
| return $handler->handle($this->applyAttributes($request, $result->getAttributes())); | ||
| } | ||
|
|
||
| return $this->buildErrorResponse($request, $result); |
There was a problem hiding this comment.
For my OCD, can you please make the last line the "successful" one. The method reads:
if
-> error
if
-> error
if
-> error
if
-> success
error
But if you have strong feelings about writing if (false === $result->isAllowed()) { then keep as is
|
|
||
| private function isMetadataRequest(ServerRequestInterface $request): bool | ||
| { | ||
| if (empty($this->metadataPaths) || 'GET' !== $request->getMethod()) { |
There was a problem hiding this comment.
In general, try to avoid empty(). It is too ambiguous.
| if (empty($this->metadataPaths) || 'GET' !== $request->getMethod()) { | |
| if ([] === $this->metadataPaths || 'GET' !== $request->getMethod()) { |
| } | ||
|
|
||
| // Auto-generate from request if metadata paths are configured | ||
| if (empty($this->metadataPaths)) { |
There was a problem hiding this comment.
| if (empty($this->metadataPaths)) { | |
| if ([] === $this->metadataPaths) { |
| $parts[] = 'error_description="' . $this->escapeHeaderValue($result->getErrorDescription()) . '"'; | ||
| } | ||
|
|
||
| if (empty($parts)) { |
There was a problem hiding this comment.
| if (empty($parts)) { | |
| if ([] === $parts) { |
| return '' !== $scope; | ||
| })); | ||
|
|
||
| return empty($normalized) ? null : $normalized; |
| $response = $this->httpClient->sendRequest($request); | ||
|
|
||
| if (200 === $response->getStatusCode()) { | ||
| $this->upstreamMetadata = json_decode((string)$response->getBody(), true) ?? []; |
There was a problem hiding this comment.
| $this->upstreamMetadata = json_decode((string)$response->getBody(), true) ?? []; | |
| $this->upstreamMetadata = json_decode($response->getBody()->__toString(), true) ?? []; |
| )); | ||
| } | ||
|
|
||
| $body = (string)$response->getBody(); |
There was a problem hiding this comment.
| $body = (string)$response->getBody(); | |
| $body = $response->getBody()->__toString(); |
| $this->assertSame(200, $response->getStatusCode()); | ||
| $this->assertSame('application/json', $response->getHeaderLine('Content-Type')); | ||
|
|
||
| $payload = json_decode((string) $response->getBody(), true, 512, \JSON_THROW_ON_ERROR); |
There was a problem hiding this comment.
| $payload = json_decode((string) $response->getBody(), true, 512, \JSON_THROW_ON_ERROR); | |
| $payload = json_decode($response->getBody()->__toString(), true, 512, \JSON_THROW_ON_ERROR); |
| )); | ||
| } | ||
|
|
||
| $body = (string) $response->getBody(); |
There was a problem hiding this comment.
| $body = (string) $response->getBody(); | |
| $body = $response->getBody()->__toString(); |
| }, | ||
| "require-dev": { | ||
| "firebase/php-jwt": "^6.10", | ||
| "guzzlehttp/guzzle": "^7.0", |
There was a problem hiding this comment.
It does not look like Guzzle is used.
I suggest removing this.
3 participants
Motivation and Context
Implementation of OAuth based on middlewares from #218
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context