-
Notifications
You must be signed in to change notification settings - Fork 110
[Server] Oauth2 based on middleware #221
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: main
Are you sure you want to change the base?
Conversation
|
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
left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Works right away 💪 👍
| // 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.
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
| // 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.
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')) |
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.
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.
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
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.
below as well, please
|
|
||
| $response = $this->httpClient->sendRequest($request); | ||
|
|
||
| if ($response->getStatusCode() >= 400) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$tokenIssuer can be inlined:
| return \in_array($tokenIssuer, $expectedIssuers, true); | |
| return \in_array($claims['iss'], $expectedIssuers, true); |
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.
would be great to have tests for this class
| * | ||
| * @author Volodymyr Panivko <sveneld300@gmail.com> | ||
| */ | ||
| class AuthorizationResult |
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.
| class AuthorizationResult | |
| final class AuthorizationResult |
|
@chr-hertel I updated pull request |

Motivation and Context
Implementation of OAuth based on middlewares from #218
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context