From 04336d02944e9c6cf0dc7899d3807c31e3d69b7a Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 12 Nov 2026 00:43:20 +0900 Subject: [PATCH] Select the advertised authorization server by the stored issuer ## Motivation and Context Protected resource metadata may list several authorization servers, and the flow always took the first. With `issuer:` on `ClientCredentialsProvider` and `CrossAppAccessProvider` that could not work: a resource listing the configured server second was refused as a different authorization server, although its metadata named the right one. A registered client fared the same: its stored client information records the server that issued the registration, and it was registered again at the first entry instead. The flow now prefers the entry naming the issuer recorded in the provider's client information, with the single trailing slash tolerance the issuer checks apply, and falls back to the first entry when nothing is recorded or listed; the checks that follow still decide, so a configured issuer the resource does not list is refused as before. The Python SDK's client credentials provider selects the advertised server the same way for its configured issuer. ## How Has This Been Tested? test/mcp/client/oauth/flow_test.rb: a resource listing the configured issuer second runs both machine-to-machine grants and the refresh at that server, a trailing slash difference still matches, an issuer the resource does not list is refused, a provider without an issuer keeps the first entry, and a registered client keeps its authorization server; before the change the tests that expect the second entry fail by picking the first. ## Breaking Changes None. --- docs/_client/authorization.md | 8 +- lib/mcp/client/oauth/flow.rb | 51 +++++++---- test/mcp/client/oauth/flow_test.rb | 136 +++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 21 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index 292d03ae..98d7607c 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -321,10 +321,10 @@ Keyword arguments: - `private_key`, `signing_algorithm`: Required with `private_key_jwt` - the key (a PEM string or `OpenSSL::PKey::PKey`, never written to `storage`) signs the client assertion with `"ES256"` or `"RS256"`; `client_secret` must not be set, because the private key is the credential. -- `issuer`: Optional. The issuer identifier of the authorization server the credentials belong to. A different authorization server raises - `Flow::AuthorizationError`. A single trailing slash difference is accepted, as in the TypeScript SDK. Omitting `issuer:` is deprecated - and emits a warning at construction. Without it, credentials go to whichever authorization server the MCP server names, as before. - To use credentials for another authorization server, construct a provider with that server's `issuer:` and credentials. +- `issuer`: Optional. The issuer identifier of the authorization server the credentials belong to. A different authorization server raises `Flow::AuthorizationError`. + A single trailing slash difference is accepted, as in the TypeScript SDK. When the protected resource metadata lists several authorization servers, + the one naming `issuer:` is used. Omitting `issuer:` is deprecated and emits a warning at construction. Without it, credentials go to whichever authorization server + the MCP server names, as before. To use credentials for another authorization server, construct a provider with that server's `issuer:` and credentials. - `scope`, `storage`, `authorization_request_validator`, `token_request_params`, `http_client_customizer`: Optional, same meaning as on `Provider`. Use `token_request_params` for a parameter the authorization server requires on the `client_credentials` grant, such as Auth0's `audience`. diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 72b79706..741f7d68 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -380,9 +380,7 @@ def ensure_client_credentials_issuer!(client_info, as_metadata:) stored_issuer = client_info_required_value(client_info, "issuer") return if stored_issuer.nil? - current_issuer = as_metadata["issuer"] - return if stored_issuer == current_issuer - return if stored_issuer == "#{current_issuer}/" || "#{stored_issuer}/" == current_issuer + return if issuers_match?(stored_issuer, as_metadata["issuer"]) raise AuthorizationError, "Stored client credentials are bound to a different authorization server " \ @@ -633,7 +631,7 @@ def locate_authorization_server(server_url:, resource_metadata_url:) end if prm - authorization_server = first_authorization_server(prm) + authorization_server = advertised_authorization_server(prm) ensure_secure_url!(authorization_server, label: "PRM `authorization_servers` entry") ensure_routable_destination!( authorization_server, @@ -722,31 +720,50 @@ def ensure_legacy_issuer_matches!(expected:, returned:) ensure_issuer_matches!(expected: expected, returned: returned) end - # Reads `authorization_servers` from a PRM document and returns - # the first entry, raising `AuthorizationError` for any of the malformed - # shapes a non-compliant server could emit (missing field, non-Array - # value, empty Array, non-String first entry). Centralizing this lets - # both the full flow and the refresh flow share the same defensive - # parse instead of each one duplicating a Hash-and-Array check. - def first_authorization_server(prm) + # Reads `authorization_servers` from a PRM document and returns the entry to use, raising `AuthorizationError` for any of + # the malformed shapes a non-compliant server could emit (missing field, non-Array value, empty Array, non-String entry). + # Centralizing this lets both the full flow and the refresh flow share the same defensive parse instead of each one duplicating + # a Hash-and-Array check. + # + # When the provider's stored client information records an `issuer` (the `issuer:` of a machine-to-machine provider, + # or the authorization server that issued a registration), the entry naming that issuer is preferred over the first one, + # as the Python SDK does: a protected resource may list several authorization servers, and the client's credentials are + # usable at only one of them. Without a match the first entry is used as before, and the issuer checks that follow decide. + def advertised_authorization_server(prm) authorization_servers = prm["authorization_servers"] unless authorization_servers.is_a?(Array) - raise AuthorizationError, - "Protected resource metadata `authorization_servers` is not an array " \ - "(got #{authorization_servers.class})." + raise AuthorizationError, "Protected resource metadata `authorization_servers` is not an array (got #{authorization_servers.class})." end if authorization_servers.empty? raise AuthorizationError, "Protected resource metadata has no authorization_servers." end - first = authorization_servers.first - unless first.is_a?(String) && !first.empty? + stored_issuer = stored_client_information_issuer + chosen = authorization_servers.find { |entry| entry.is_a?(String) && issuers_match?(entry, stored_issuer) } if stored_issuer + chosen ||= authorization_servers.first + + unless chosen.is_a?(String) && !chosen.empty? raise AuthorizationError, "Protected resource metadata `authorization_servers[0]` is not a non-empty string." end - first + chosen + end + + # The `issuer` recorded in the provider's stored client information, or `nil`. + def stored_client_information_issuer + info = @provider.client_information + return unless info.is_a?(Hash) + + client_info_required_value(info, "issuer") + end + + # Whether two issuer identifiers name the same authorization server: identical, or differing by one trailing slash, + # as the TypeScript SDK's `issuersMatch` accepts (an issuer derived from a URL is often rendered with the slash + # the metadata leaves out). Any other difference keeps them distinct. + def issuers_match?(first, second) + first == second || first == "#{second}/" || "#{first}/" == second end # Walks candidate metadata URLs and returns the parsed body of the first 2xx response that is a JSON object; diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index f10efd5b..5e21d2a0 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -745,6 +745,104 @@ def test_run_machine_to_machine_grants_refuse_other_differences_in_the_configure end end + def test_run_machine_to_machine_grants_use_the_advertised_authorization_server_naming_the_configured_issuer + other_auth_base = "https://other-auth.example.com" + stub_authorization_servers([other_auth_base, @auth_base]) + stub_other_authorization_server(other_auth_base) + + [ClientCredentialsProvider, CrossAppAccessProvider].each do |provider_class| + args = { client_id: "test-client", client_secret: "test-secret", issuer: @auth_base } + args[:assertion_provider] = ->(**) { "id-jag-assertion" } if provider_class == CrossAppAccessProvider + provider = provider_class.new(**args) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + end + + assert_requested(:post, "#{@auth_base}/token", times: 2) + assert_not_requested(:post, "#{other_auth_base}/token") + end + + def test_run_machine_to_machine_grants_match_the_advertised_authorization_server_despite_one_trailing_slash + other_auth_base = "https://other-auth.example.com" + stub_authorization_servers([other_auth_base, @auth_base]) + stub_other_authorization_server(other_auth_base) + provider = ClientCredentialsProvider.new(client_id: "test-client", client_secret: "test-secret", issuer: "#{@auth_base}/") + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_not_requested(:post, "#{other_auth_base}/token") + end + + def test_run_machine_to_machine_grants_refuse_a_configured_issuer_the_protected_resource_does_not_list + other_auth_base = "https://other-auth.example.com" + stub_authorization_servers([other_auth_base, "https://third-auth.example.com"]) + stub_other_authorization_server(other_auth_base) + provider = ClientCredentialsProvider.new(client_id: "test-client", client_secret: "test-secret", issuer: @auth_base) + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/bound to a different authorization server/, error.message) + assert_not_requested(:post, "#{other_auth_base}/token") + end + + def test_run_without_a_stored_issuer_uses_the_first_advertised_authorization_server + other_auth_base = "https://other-auth.example.com" + stub_authorization_servers([other_auth_base, @auth_base]) + stub_other_authorization_server(other_auth_base) + provider = client_credentials_provider + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:post, "#{other_auth_base}/token") + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_refresh_machine_to_machine_grants_use_the_advertised_authorization_server_naming_the_configured_issuer + other_auth_base = "https://other-auth.example.com" + stub_authorization_servers([other_auth_base, @auth_base]) + stub_other_authorization_server(other_auth_base) + provider = ClientCredentialsProvider.new(client_id: "test-client", client_secret: "test-secret", issuer: @auth_base) + provider.save_tokens("refresh_token" => "refresh-token", "issuer" => @auth_base) + + result = Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:refreshed, result) + assert_not_requested(:post, "#{other_auth_base}/token") + end + + def test_run_authorization_code_keeps_the_authorization_server_that_registered_the_client + other_auth_base = "https://other-auth.example.com" + state_holder = {} + provider = Provider.new( + client_metadata: { + redirect_uris: ["http://localhost:0/callback"], + grant_types: ["authorization_code"], + response_types: ["code"], + token_endpoint_auth_method: "none", + }, + redirect_uri: "http://localhost:0/callback", + redirect_handler: ->(url) { state_holder[:state] = URI.decode_www_form(url.query).to_h.fetch("state") }, + callback_handler: -> { ["test-auth-code", state_holder[:state]] }, + ) + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + assert_equal(@auth_base, provider.client_information["issuer"]) + + stub_authorization_servers([other_auth_base, @auth_base]) + stub_other_authorization_server(other_auth_base) + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:post, "#{@auth_base}/register", times: 1) + assert_requested(:post, "#{@auth_base}/token", times: 2) + assert_not_requested(:post, "#{other_auth_base}/token") + end + def test_refresh_machine_to_machine_grants_accept_one_trailing_slash_difference_in_the_configured_issuer [ClientCredentialsProvider, CrossAppAccessProvider].each do |provider_class| args = { client_id: "test-client", client_secret: "test-secret", issuer: "#{@auth_base}/" } @@ -772,6 +870,44 @@ def test_run_client_credentials_does_not_bind_to_an_issuer_the_validator_refuses assert_nil(provider.client_information["issuer"]) end + # Points the Protected Resource Metadata at the given authorization servers, in that order. + def stub_authorization_servers(issuers) + stub_request(:get, @prm_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: issuers), + ) + end + + # Serves metadata and endpoints for another authorization server that every grant could run at, so a flow + # that picks it over the configured one fails on the assertions about where the requests went, not earlier. + def stub_other_authorization_server(issuer) + stub_request(:get, "#{issuer}/.well-known/oauth-authorization-server").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate( + issuer: issuer, + authorization_endpoint: "#{issuer}/authorize", + token_endpoint: "#{issuer}/token", + registration_endpoint: "#{issuer}/register", + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "client_credentials", "refresh_token", "urn:ietf:params:oauth:grant-type:jwt-bearer"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none", "client_secret_basic"], + ), + ) + stub_request(:post, "#{issuer}/register").to_return( + status: 201, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(client_id: "other-client", client_name: "ruby-sdk-test"), + ) + stub_request(:post, "#{issuer}/token").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "other-token", token_type: "Bearer"), + ) + end + # Points the Protected Resource Metadata at another authorization server with its own metadata and token endpoint. def stub_authorization_server_switch_to(issuer) endpoint_base = issuer.delete_suffix("/")