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("/")