feat: add conformance tests for SEP-990 #110
Conversation
|
🙌 thanks for this! will aim to get you comments by tomorrow, but i'm very excited to have a test for this. one thing missing from this tool currently is marking tests as optional. Current state is MUST = FAIL, SHOULD = WARNING, and for tiering we want SHOULD to be required (want the best SDK's to do the SHOULD's). extensions are a new axis, where we want to say "this feature is optional, but within the test, the same MUST/SHOULD logic applies" which will apply here and to client credentials which is currently not differentiated. I'm just noting the concept we need to add, not in the scope of this PR. |
There was a problem hiding this comment.
A few high level comments:
- The top of the PR description seems outdated / copy pasted? (the SSE polling bit)
- It looks like we're mixing AS and IdP endpoints, let's keep those separate w/ separate handlers
- let's stick to 1 end-to-end test to start w/ many checks. each test that spins up a server is a cost on CI for every SDK, so we want to keep the # low.
- I stuck with comments on the test since that's the most important part, but the example will also need some changes.
- if you could include a negative test (i.e. an example client that implements it incorrectly, and so it will fail the test), that'd be great.
- please add this to the "extensions" list of tests (may need to manage merge conflicts, this jostled a bit for the tiering kickoff)
| 'urn:ietf:params:oauth:grant-type:token-exchange', | ||
| 'urn:ietf:params:oauth:grant-type:jwt-bearer' | ||
| ], | ||
| tokenEndpointAuthMethodsSupported: ['none'], |
There was a problem hiding this comment.
i believe this should be client_secret_basic or private_key_jwt since ID-JAG is only supposed to work with confidential clients.
| const idpIdToken = await createIdpIdToken( | ||
| this.idpPrivateKey!, | ||
| this.idpServer.getUrl(), | ||
| this.authServer.getUrl() |
There was a problem hiding this comment.
the IdP ID token should not have the auth server as its audience. it should be the "idp_client_id" which is distinct from the client id used to talk to the authorization server.
it's the id-jag that should have the authServer as its audience.
| tokenEndpointAuthMethodsSupported: ['none'], | ||
| onTokenRequest: async ({ grantType, body, timestamp, authBaseUrl }) => { | ||
| // Handle token exchange (IDP ID token -> authorization grant) | ||
| if (grantType === 'urn:ietf:params:oauth:grant-type:token-exchange') { |
There was a problem hiding this comment.
the auth server shouldn't bet getting a token-exchange request, that request should be going to the IdP.
| scopes: [], | ||
| additionalFields: { | ||
| issued_token_type: | ||
| 'urn:ietf:params:oauth:token-type:authorization_grant', |
There was a problem hiding this comment.
This is the ID-JAG flow, so should be urn:ietf:params:oauth:token-type:id-jag
| * using RFC 8693 token exchange, and then exchange that grant for an access token | ||
| * using RFC 7523 JWT Bearer grant. | ||
| */ | ||
| export class CrossAppAccessTokenExchangeScenario implements Scenario { |
There was a problem hiding this comment.
let's start with just 1 test for the full flow. each test adds integration cost, and these 2 are redundant with the full e2e test.
| // Start auth server with both token exchange and JWT bearer grant support | ||
| const authApp = createAuthServer(this.checks, this.authServer.getUrl, { | ||
| grantTypesSupported: [ | ||
| 'urn:ietf:params:oauth:grant-type:token-exchange', |
There was a problem hiding this comment.
the auth server doesn't need to support token-exchange, I think you've combined the IdP and AS in this test.
|
|
||
| if ( | ||
| !subjectToken || | ||
| subjectTokenType !== 'urn:ietf:params:oauth:token-type:id_token' |
There was a problem hiding this comment.
the id_token should never be hitting the AS url, this function is called in the AS.
| sub: userId, | ||
| grant_type: 'authorization_grant' | ||
| }) | ||
| .setProtectedHeader({ alg: 'ES256' }) |
There was a problem hiding this comment.
.setProtectedHeader({ alg: 'ES256', typ: 'oauth-id-jag+jwt' })
| return { | ||
| token: authorizationGrant, | ||
| scopes: [], | ||
| additionalFields: { |
There was a problem hiding this comment.
i don't think this field is respected. since this handler needs to be on the IdP anyway, it's probably better to implement a handler on the fake IdP directly rather than try to shoehorn into the createAuthServer interfaces
commit: |
2 participants
Adds conformance tests for SEP-990 which introduces SSE polling via server-side disconnect.
Motivation and Context
How Has This Been Tested?
Tested with modelcontextprotocol/typescript-sdk#1328
Breaking Changes
Types of changes
Checklist
Additional context