feat: add conformance test for authorization server metadata#170
feat: add conformance test for authorization server metadata#170Michito-Okai wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
tnorimat
left a comment
There was a problem hiding this comment.
@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']; |
There was a problem hiding this comment.
To avoid duplication, instead of defining spec version here, could you import SpecVersion of types.ts like
import {
ClientScenarioForAuthorizationServer,
ConformanceCheck,
SpecVersion
} from '../../types';
?
There was a problem hiding this comment.
I fixed. Could you check this?
| export function getClientScenarioForAuthorizationServer( | ||
| name: string | ||
| ): ClientScenarioForAuthorizationServer | undefined { | ||
| return clientScenariosForAuthorizationServer.get(name); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
I fixed. Could you check this?
| versionScenarios = listClientScenariosForSpec(version); | ||
| } else if (command === 'authorization') { | ||
| versionScenarios = | ||
| listClientScenariosForAuthorizationServerForSpec(version); |
There was a problem hiding this comment.
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';
There was a problem hiding this comment.
I fixed. Could you check this?
src/index.ts
Outdated
| authorizationServerScenarios = filterScenariosBySpecVersion( | ||
| authorizationServerScenarios, | ||
| specVersionFilter, | ||
| 'client' |
There was a problem hiding this comment.
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}`)); |
There was a problem hiding this comment.
listClientScenariosForAuthorizationServer instead of listClientScenarios ?
There was a problem hiding this comment.
I fixed. Could you check this?
src/index.ts
Outdated
| console.log(''); | ||
| } | ||
| console.log( | ||
| 'Authorization server scenarios (test against a authorization server):' |
There was a problem hiding this comment.
an authorization server instead of a authorization server ?
There was a problem hiding this comment.
I fixed. Could you check this?
| run(serverUrl: string): Promise<ConformanceCheck[]>; | ||
| } | ||
|
|
||
| export interface ClientScenarioForAuthorizationServer { |
There was a problem hiding this comment.
Both interfaces ClientScenario and ClientScenarioForAuthorizationServer have the exact same shape. Consider using a type alias (type ClientScenarioForAuthorizationServer = ClientScenario) or just reusing ClientScenario directly?.
There was a problem hiding this comment.
The shape of ClientScenarioForAuthorizationServer is likely to change with enhancements, so I defined ClientScenarioForAuthorizationServer.
| let errorMessage: string | undefined; | ||
| let details: any; | ||
|
|
||
| const fail = (msg: string) => { |
There was a problem hiding this comment.
The fail is never called.
All validation methods throw instead, and the catch block sets status and errorMessage directly.
There was a problem hiding this comment.
For example,
} catch (error) {
status = 'FAILURE';
errorMessage = error instanceof Error ? error.message : String(error);
}
There was a problem hiding this comment.
I fixed. Could you check this?
| ); | ||
| } | ||
|
|
||
| const expectedIssuer = serverUrl.replace( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}`;
There was a problem hiding this comment.
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.
| if (options.server || (!options.client && !options.server)) { | ||
| if ( | ||
| options.server || | ||
| (!options.client && !options.server && !options.authorization) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
9617458 to
eacc9e7
Compare
Motivation and Context
Described in #169
How Has This Been Tested?
--url URL of the authorization server metadata endpoint
--url invalid URL
Breaking Changes
Types of changes
Checklist