Skip to content

feat: add conformance test for authorization server metadata#170

Open
Michito-Okai wants to merge 1 commit intomodelcontextprotocol:mainfrom
Hitachi:authorization-server-metadata
Open

feat: add conformance test for authorization server metadata#170
Michito-Okai wants to merge 1 commit intomodelcontextprotocol:mainfrom
Hitachi:authorization-server-metadata

Conversation

@Michito-Okai
Copy link

Motivation and Context

Described in #169

How Has This Been Tested?

Breaking Changes

  • add authorization server mode (-- authorization)
  • add url option for authorization server mode (--url )
  • add a conformance test for authorization server metadata

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Copy link

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Michito-Okai Hello, I added some review comments. Could you check them?

implements ClientScenarioForAuthorizationServer
{
name = 'authorization-server-metadata-endpoint';
specVersions: SpecVersion[] = ['2025-03-26', '2025-06-18', '2025-11-25'];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid duplication, instead of defining spec version here, could you import SpecVersion of types.ts like

import {
  ClientScenarioForAuthorizationServer,
  ConformanceCheck,
  SpecVersion 
} from '../../types';

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed. Could you check this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid duplication, could you remove ['2025-03-26', '2025-06-18', '2025-11-25'] ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SpecVersion of types.ts is a type declaration. Therefore, these are not duplicates.

export function getClientScenarioForAuthorizationServer(
name: string
): ClientScenarioForAuthorizationServer | undefined {
return clientScenariosForAuthorizationServer.get(name);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getClientScenarioForAuthorizationServer declares its return type as ClientScenarioForAuthorizationServer but that type isn't imported. Could you add it to the import from '../types' on line 1 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed. Could you check this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

versionScenarios = listClientScenariosForSpec(version);
} else if (command === 'authorization') {
versionScenarios =
listClientScenariosForAuthorizationServerForSpec(version);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is called inside filterScenariosBySpecVersion but was never imported from './scenarios'. It's exported from index.ts but missing from the import block at line18-34 of index.ts. Could you import that ? like

import {
  listScenarios,
  listClientScenarios,
  listActiveClientScenarios,
  listPendingClientScenarios,
  listAuthScenarios,
  listMetadataScenarios,
  listCoreScenarios,
  listExtensionScenarios,
  listBackcompatScenarios,
  listScenariosForSpec,
  listClientScenariosForSpec,
  getScenarioSpecVersions,
  listClientScenariosForAuthorizationServer,
  listClientScenariosForAuthorizationServerForSpec,
  ALL_SPEC_VERSIONS
} from './scenarios';

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed. Could you check this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

src/index.ts Outdated
authorizationServerScenarios = filterScenariosBySpecVersion(
authorizationServerScenarios,
specVersionFilter,
'client'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorization instead of client ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed. Could you check this?

src/index.ts Outdated
console.error(` ${err.path.join('.')}: ${err.message}`);
});
console.error('\nAvailable authorization server scenarios:');
listClientScenarios().forEach((s) => console.error(` - ${s}`));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listClientScenariosForAuthorizationServer instead of listClientScenarios ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed. Could you check this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed it.

src/index.ts Outdated
console.log('');
}
console.log(
'Authorization server scenarios (test against a authorization server):'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an authorization server instead of a authorization server ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed. Could you check this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

run(serverUrl: string): Promise<ConformanceCheck[]>;
}

export interface ClientScenarioForAuthorizationServer {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both interfaces ClientScenario and ClientScenarioForAuthorizationServer have the exact same shape. Consider using a type alias (type ClientScenarioForAuthorizationServer = ClientScenario) or just reusing ClientScenario directly?.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shape of ClientScenarioForAuthorizationServer is likely to change with enhancements, so I defined ClientScenarioForAuthorizationServer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification.

let errorMessage: string | undefined;
let details: any;

const fail = (msg: string) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fail is never called.
All validation methods throw instead, and the catch block sets status and errorMessage directly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example,

    } catch (error) {
      status = 'FAILURE';
      errorMessage = error instanceof Error ? error.message : String(error);
    }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed. Could you check this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

);
}

const expectedIssuer = serverUrl.replace(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only removes the first occurrence. If the URL were e.g. https://example.com/.well-known/oauth-authorization-server/tenant, the replace would produce https://example.com//tenant (note the path portion is kept). Per RFC 8414 §3, the issuer for a path-aware metadata URL /.well-known/oauth-authorization-server/<path> should be https://example.com/<path>. The current approach works correctly for the root case but may be incorrect for path-based issuers.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example,

const url = new URL(serverUrl);
const wellKnownPrefix = '/.well-known/oauth-authorization-server';
const suffix = url.pathname.startsWith(wellKnownPrefix + '/')
  ? url.pathname.slice(wellKnownPrefix.length)
  : '';
const expectedIssuer = `${url.origin}${suffix}`;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if the URL were e.g. https://example.com/.well-known/oauth-authorization-server/tenant, the replace would produce not https://example.com//tenant but https://example.com/tenant. Therefore I think that the current approach works correctly for path-based issuers.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification.

if (options.server || (!options.client && !options.server)) {
if (
options.server ||
(!options.client && !options.server && !options.authorization)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server command supports running a single scenario (--scenario), suite selection (suite), expected-failures baselines, and verbose output (--verbose). The new authorization command has none of these, making it less flexible. This may be intentional for an initial implementation but is worth noting. It is up to you to consider this point by this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario, suite, expected-failures and verbose options are outside the scope of this PR. I am considering to add these options in a future enhancement. How does that sound?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I understand.

@Michito-Okai Michito-Okai force-pushed the authorization-server-metadata branch from 9617458 to eacc9e7 Compare February 27, 2026 02:37
body: Record<string, any>,
serverUrl: string
): void {
this.assertString(body.authorization_endpoint, 'authorization_endpoint');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the throw / catch pattern for issues that block further progress (e.g. a non-200 response), but for issues where there may be multiple distinct issues, I'd prefer we either add multiple failing checks to the list of checks returned OR include all validation failures in one check so that the user gets feedback on the maximum possible list of failures to check when they get a test failure.

e.g. I want to avoid this situation:

  • Run test, fails w/ missing authorization_endpoint
  • Run test again, fails w/ missing token_endpoint
  • Run test again, fails with missing response_types_supported

And instead have:

  • Run test, fails w/ missing authorization_endpoint, token_endpoint, response_types_supported

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcarleton
I fixed as follows. Could you check this?
When test fails in validateMetadataBody, failures are not handled with a throw / catch pattern, but instead all checks are consolidated and verified within a single test run.

- Return a JSON response including issuer, authorization_endpoint, token_endpoint and response_types_supported
- The issuer value MUST match the URI obtained by removing the well-known URI string from the authorization server metadata URI.`;

async run(serverUrl: string): Promise<ConformanceCheck[]> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mentioned in the issue, but I think we want serverUrl to be the issuerUrl, and we test that the metadata is at one of the .well-known paths.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcarleton
I fixed as follows. Could you check this?
run receives argument as the authorization server issuer.
createWellKnownUrl create the authorization server metadata url based the authorization server issuer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for OAuth metadata, but I was assuming we would also check OIDC metadata routes.

Basically I would imagine we wouldn't want a server to marked as failing if it's using OIDC metadata, since we treat that as valid.

I believe that would require us probing URL paths.

Copy link

@tnorimat tnorimat Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Michito-Okai Hello, as we discussed, to incorporate @pcarleton comments, how about the following by following the MCP 2025-11-25 spec https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#authorization-server-metadata-discovery ?

  • The test receives an issuer(server) url (mandatory) and a tenant/realm part (optional)
  • If the tenant/realm part exist, creating an well-know url and access the url by the following the order that the MCP 2025-11-25 spec described.
1. OAuth 2.0 Authorization Server Metadata with path insertion: https://auth.example.com/.well-known/oauth-authorization-server/tenant1
2. OpenID Connect Discovery 1.0 with path insertion: https://auth.example.com/.well-known/openid-configuration/tenant1
3. OpenID Connect Discovery 1.0 path appending: https://auth.example.com/tenant1/.well-known/openid-configuration

If the test can successfully fetches the metadata from one of them, the test proceeds (no need to do all the three patterns above). If the test cannot fetch the metadata from any of them, the test returns a failure.

  • If the tenant/realm part does not exist, creating an well-know url and access the url by the following the order that the MCP 2025-11-25 spec described.
1. OAuth 2.0 Authorization Server Metadata: https://auth.example.com/.well-known/oauth-authorization-server
2. OpenID Connect Discovery 1.0: https://auth.example.com/.well-known/openid-configuration

If the test can successfully fetches the metadata from one of them, the test proceeds (no need to do all the two patters above). If the test cannot fetch the metadata from any of them, the test returns a failure.

@Michito-Okai Michito-Okai force-pushed the authorization-server-metadata branch from eacc9e7 to 29f04b5 Compare March 18, 2026 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants