Merge pull request #680 from koic/select_the_advertised_authorization_server_by_the_stored_issuer

Select the advertised authorization server by the stored issuer

Author: koic

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`.
 

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;

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