Skip to content

Commit 12afebf

Browse files
committed
test(client/oauth): clarify auth comments and cover Basic across grants
1 parent e97d5c0 commit 12afebf

2 files changed

Lines changed: 24 additions & 31 deletions

File tree

‎lib/mcp/client/oauth/flow.rb‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -997,9 +997,9 @@ def exchange_refresh_token(as_metadata:, client_info:, refresh_token:, resource:
997997
post_to_token_endpoint(as_metadata: as_metadata, client_info: client_info, form: form)
998998
end
999999

1000-
# Submits a form-encoded request to the token endpoint, applying
1001-
# the client authentication method advertised in `client_information` and
1002-
# adding `client_id` (and `client_secret` when not using HTTP Basic).
1000+
# Submits a form-encoded token request using the authentication method
1001+
# stored in `client_information`. The method determines whether client
1002+
# credentials belong in the form body, a Basic header, or a JWT assertion.
10031003
def post_to_token_endpoint(as_metadata:, client_info:, form:)
10041004
client_id = client_info_required_value(client_info, "client_id")
10051005
unless client_id
@@ -1010,17 +1010,13 @@ def post_to_token_endpoint(as_metadata:, client_info:, form:)
10101010
client_secret = client_info_required_value(client_info, "client_secret")
10111011
token_endpoint_auth_method = client_info_value(client_info, "token_endpoint_auth_method")
10121012

1013-
# Each branch carries the client identity in exactly one place. Sending
1014-
# it twice reads as two client authentication methods to a strict
1015-
# authorization server (RFC 6749 Section 2.3), which rejects the request.
1013+
# Apply one client authentication method per request (RFC 6749 Section 2.3).
10161014
headers = {}
10171015
form = case token_endpoint_auth_method
10181016
when "private_key_jwt"
1019-
# RFC 7523 Section 2.2 JWT client assertion for the `private_key_jwt` method of
1020-
# the `io.modelcontextprotocol/oauth-client-credentials` extension (SEP-1046).
1021-
# The client identity travels in the assertion's `iss`/`sub` claims, so `client_id` is
1022-
# omitted from the body per RFC 7521 Section 4.2 (the `client_assertion` conveys the client identity).
1023-
# The audience is the issuer identifier that `ensure_issuer_matches!` already byte-validated.
1017+
# The assertion identifies the client through its `iss` and `sub`
1018+
# claims, so the body needs no separate `client_id` (RFC 7521 Section 4.2).
1019+
# Use the issuer already checked by `ensure_issuer_matches!` as the audience.
10241020
unless @provider.respond_to?(:client_assertion)
10251021
raise AuthorizationError,
10261022
"token_endpoint_auth_method is private_key_jwt but the provider does not " \
@@ -1032,24 +1028,22 @@ def post_to_token_endpoint(as_metadata:, client_info:, form:)
10321028
"client_assertion" => @provider.client_assertion(audience: as_metadata["issuer"]),
10331029
)
10341030
when "client_secret_post"
1035-
# RFC 6749 Section 2.3.1: both credentials travel in the request body.
1031+
# Send the client ID and available secret in the form body.
10361032
body = form.merge("client_id" => client_id)
10371033
body["client_secret"] = client_secret if client_secret
10381034
body
10391035
else
10401036
if client_secret && token_endpoint_auth_method != "none"
1041-
# RFC 6749 Section 2.3.1 recommends Basic for confidential clients and
1042-
# both Python and TypeScript SDKs default here when
1043-
# the authentication method is not explicitly stored. The Basic username
1044-
# is already the client identifier, so a body `client_id` adds nothing;
1045-
# Section 3.2.1 makes it a MAY, and servers that read it as a second
1046-
# authentication method reject the request with `invalid_request`.
1037+
# Basic is also the fallback when a secret is present but no method
1038+
# is stored. A body `client_id` is optional (RFC 6749 Section 3.2.1);
1039+
# omit it because some servers treat it alongside Basic as a second
1040+
# authentication method and reject the request with `invalid_request`.
10471041
headers["Authorization"] = "Basic " + basic_auth_credentials(client_id, client_secret)
10481042
form
10491043
else
1050-
# Public client, whether by an explicit `none` or the absence of a
1051-
# secret. `client_id` is the only thing identifying it, and the
1052-
# authorization_code grant requires it (RFC 6749 Section 3.2.1).
1044+
# With `none` or no secret, identify the client using `client_id`.
1045+
# This is required for unauthenticated authorization-code exchanges
1046+
# (RFC 6749 Section 3.2.1).
10531047
form.merge("client_id" => client_id)
10541048
end
10551049
end

‎test/mcp/client/oauth/flow_test.rb‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,8 @@ def test_run_client_credentials_with_client_secret_post_sends_credentials_in_bod
159159
end
160160

161161
def test_run_client_credentials_with_client_secret_basic_omits_client_id_from_body
162-
# With HTTP Basic the client identity already travels in the header,
163-
# and RFC 6749 Section 3.2.1 makes a body `client_id` a MAY. Servers
164-
# that read one as a second authentication method reject the whole
165-
# request with `invalid_request`, so omit it.
162+
# Explicit Basic authentication must keep both credentials in the header
163+
# to avoid servers treating body credentials as a second auth method.
166164
provider = client_credentials_provider(token_endpoint_auth_method: "client_secret_basic")
167165

168166
Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
@@ -1877,8 +1875,10 @@ def test_run_uses_basic_auth_when_token_endpoint_auth_method_is_client_secret_ba
18771875

18781876
expected = "Basic " + Base64.strict_encode64("pre-registered-client:pre-registered-secret")
18791877
assert_requested(:post, "#{@auth_base}/token") do |req|
1878+
form = URI.decode_www_form(req.body).to_h
18801879
req.headers["Authorization"] == expected &&
1881-
!URI.decode_www_form(req.body).to_h.key?("client_secret")
1880+
!form.key?("client_id") &&
1881+
!form.key?("client_secret")
18821882
end
18831883
end
18841884

@@ -2383,10 +2383,8 @@ def test_refresh_swaps_refresh_token_for_new_access_token
23832383
end
23842384

23852385
def test_refresh_with_client_secret_basic_omits_client_id_from_body
2386-
# The authorization-code exchange is often hand-rolled by the host app,
2387-
# so refresh is where a double-authenticated request first reaches a
2388-
# strict authorization server -- roughly an access token's lifetime
2389-
# after the grant was created.
2386+
# Stored credentials without an auth method default to Basic. Refresh
2387+
# must also omit the body `client_id` when using that fallback.
23902388
stub_request(:post, "#{@auth_base}/token")
23912389
.with(body: hash_including("grant_type" => "refresh_token"))
23922390
.to_return(
@@ -2409,7 +2407,8 @@ def test_refresh_with_client_secret_basic_omits_client_id_from_body
24092407
assert_requested(:post, "#{@auth_base}/token") do |req|
24102408
form = URI.decode_www_form(req.body).to_h
24112409
req.headers["Authorization"] == "Basic " + Base64.strict_encode64("conf-client:conf-secret") &&
2412-
!form.key?("client_id")
2410+
!form.key?("client_id") &&
2411+
!form.key?("client_secret")
24132412
end
24142413
end
24152414

0 commit comments

Comments
 (0)