feat: fully support Basic Authorization header at token request#1847
feat: fully support Basic Authorization header at token request#1847challenger71498 wants to merge 15 commits intomodelcontextprotocol:mainfrom
Conversation
This test case only contains client_id and client_secret at Basic Authentication header, assuming that there is no client_id and client_secret at form data.
This data class contains creds plus auth method
Retrieve client_id from auth header or the body, then retrieve client via that client_id. After that, compare auth method.
`match` causes coverage error on python 3.10.
|
|
||
| return client | ||
|
|
||
| async def _get_credentials(self, request: Request) -> ClientCredentials: |
There was a problem hiding this comment.
Note: the core extraction logic of client_id and client_secret is as same as before.
|
|
||
| @pytest.mark.anyio | ||
| async def test_wrong_auth_method_without_valid_credentials_fails( | ||
| async def test_wrong_auth_method_fails( |
There was a problem hiding this comment.
Note: test fails as same as before, only exception message has been changed, due to the auth method mismatch.
| # RFC 6749: authentication failures return "invalid_client" | ||
| assert error_response["error"] == "invalid_client" | ||
| assert "Missing or invalid Basic authentication" in error_response["error_description"] | ||
| assert "Expected client_secret_basic authentication method" in error_response["error_description"] |
There was a problem hiding this comment.
Note: test fails as same as before, only exception message has been changed, due to the auth method mismatch.
|
|
||
| @pytest.mark.anyio | ||
| async def test_basic_auth_client_id_mismatch_fails( | ||
| async def test_basic_auth_takes_precedence( |
There was a problem hiding this comment.
Note: the behaviour of the test has been changed. After the changes, basic auth takes precedence over form data, so even if there is a mismatch, the middleware retrieves the client_id from the header.
This behaviour is intended due to RFC 6749 Section 2.3.1:
Including the client credentials in the request-body using the two parameters is NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme (or other password-based HTTP authentication schemes).
If this is inappropriate, alternatives are:
- behave AS-IS (throw mismatch exception)
- disallow dup auth method
- throw exception which indicates that more than one authentication method was used.
|
@ochafik @pcarleton I saw you were involved in #1334 which added the I added my point of view on the issue this PR resolves in #1315 (comment) —* Including the realization that MCP is about OAuth 2.1 draft 13 and not RFC 6749, but that both standards seem to agree that * human-crafted em dash |
2 participants
Summary
This PR implements OAuth client authentication by supporting HTTP Basic authentication as defined in RFC 6749 Section 2.3.1., especially when
client_idis provided only from the header.The key changes are:
client_idoptional in token request body when using Basic auth (as the spec allows)Previously, the code would first read
client_idfrom the form body and then validate against the auth method. This caused issues when clients used Basic auth without includingclient_idin the request body, which is valid per the OAuth spec.Motivation and Context
client_secret_basicshould send credentials via the Authorization header, and theclient_idin the request body is optional in this case.client_idin the body even when using Basic auth, which was non-compliant with the spec.How Has This Been Tested?
Unit Test
client_idin request bodyclient_idclient_idin bodyE2E Test
npx @modelcontextprotocol/inspector+ OAuth provider mockup, and worked as expected.Screenshots
Breaking Changes
Types of changes
Checklist
I have added or updated documentation as neededno docs need to be updatedAdditional context