-
Notifications
You must be signed in to change notification settings - Fork 17
feat: add conformance tests for SEP-990 #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe this should be client_secret_basic or private_key_jwt since ID-JAG is only supposed to work with confidential clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the tokenEndpointAuthMethodsSupported to mentioned parameters. However currently client_secret_basic is considered and private_key_jwt would be checked upon later.
| const idpIdToken = await createIdpIdToken( | ||
| this.idpPrivateKey!, | ||
| this.idpServer.getUrl(), | ||
| this.authServer.getUrl() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out, since ID token would generally be provided by user kept this as a bypass value initially but now it is been corrected.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the auth server shouldn't bet getting a token-exchange request, that request should be going to the IdP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated the ID-JAG and access token exchange steps.
| scopes: [], | ||
| additionalFields: { | ||
| issued_token_type: | ||
| 'urn:ietf:params:oauth:token-type:authorization_grant', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the ID-JAG flow, so should be urn:ietf:params:oauth:token-type:id-jag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| * 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed two separate flow check methods and kept only one full flow step.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the auth server doesn't need to support token-exchange, I think you've combined the IdP and AS in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated the ID-JAG and access token exchange steps.
|
|
||
| if ( | ||
| !subjectToken || | ||
| subjectTokenType !== 'urn:ietf:params:oauth:token-type:id_token' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the id_token should never be hitting the AS url, this function is called in the AS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated the ID-JAG and access token exchange steps.
| sub: userId, | ||
| grant_type: 'authorization_grant' | ||
| }) | ||
| .setProtectedHeader({ alg: 'ES256' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.setProtectedHeader({ alg: 'ES256', typ: 'oauth-id-jag+jwt' })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| return { | ||
| token: authorizationGrant, | ||
| scopes: [], | ||
| additionalFields: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
6c0de83 to
ec2e5ab
Compare
|
Hi @pcarleton , Thanks for the review. I've made the changes as required and rebased with latest main, please have a look at the new changes once you are available. |
Description:
Adds conformance tests for SEP-990 which introduces Enterprise Managed OAuth for machine-to-machine authentication using enterprise identity providers without user interaction.
Summary
oauth-id-jag+jwt), audience claims, and confidential client authenticationMotivation and Context
SEP-990 introduces Enterprise Managed OAuth, enabling machine-to-machine authentication flows for cross-app access in enterprise environments. This allows applications to authenticate using enterprise identity providers (IdPs) through a two-step OAuth flow:
These conformance tests ensure SDK implementations correctly handle the complete cross-app access flow including proper endpoint separation, token type validation, and audience claim verification.
How Has This Been Tested?
Breaking Changes
None.
Types of changes
Checklist
References