Skip to content
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

feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 #4680

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scandinave
Copy link
Contributor

What this PR changes/adds

This PR makes the OAuth2 implementation ask for a token with a resource from param to follow the RFC-8707.

Why it does that

This allows an IDP to set the correct aud claim in the access token given to the connector.

Further notes

Linked Issue(s)

Closes #4668

@scandinave scandinave changed the title Add resource parameter to the OAuth2 token request to follow RFC-8707 feat : Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 17, 2024
@scandinave scandinave changed the title feat : Add resource parameter to the OAuth2 token request to follow RFC-8707 WIP feat : Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 17, 2024
@scandinave scandinave changed the title WIP feat : Add resource parameter to the OAuth2 token request to follow RFC-8707 feat : Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 17, 2024
@scandinave scandinave changed the title feat : Add resource parameter to the OAuth2 token request to follow RFC-8707 feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 17, 2024
@scandinave scandinave changed the title feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 [WIP] feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 17, 2024
Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

PRs need to be accompanied by tests. Please create unit tests that cover the changes.

@jimmarino jimmarino added the enhancement New feature or request label Dec 18, 2024
@scandinave scandinave changed the title [WIP] feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 18, 2024
@scandinave
Copy link
Contributor Author

I've updated tests to validate the new parameter.

Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

This can change existing behavior in some cases. If the resource is not set as a param, it should not be set on the client OAuth request.

Also, this should be an opt-in configuration parameter that is off by default (please document with @Setting). This behavior also needs to be tested.

@scandinave
Copy link
Contributor Author

I was thinking that the resource will always be set because there is always a counterPartyAddress right? Connector validates the audience of the access token when receiving request. If no resource is set, the audience will not match.

But I can definitely make a configurable option to enable this on demands for people that don't care about that.

I will rework my PR.

@scandinave scandinave changed the title feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 [WIP] feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 18, 2024
@jimmarino
Copy link
Contributor

I was thinking that the resource will always be set because there is always a counterPartyAddress right? Connector validates the audience of the access token when receiving request. If no resource is set, the audience will not match.

But I can definitely make a configurable option to enable this on demands for people that don't care about that.

I will rework my PR.

Yes, please do to preserve the existing behavior.

@scandinave
Copy link
Contributor Author

scandinave commented Dec 18, 2024

I've had a test in the Daps extension that highlight the current situation. It is called Rfc8707IntegrationTest. With this PR, if we have the DAPS extension that tells the IDP to set a specific audience and we also enabled the new resource parameter feature, the audience will have the value sent via the resource parameter.

What behavior do you expect for this ? :

  • Let the IDP implementation choose the value that it takes into account (this is the behavior with the current PR state.)
  • Make the Daps override the resource parameter
  • Make the Daps throw a warning/error telling why it can't due its job.

@scandinave scandinave changed the title [WIP] feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 feat: Add resource parameter to the OAuth2 token request to follow RFC-8707 Dec 19, 2024
@scandinave scandinave requested a review from jimmarino December 19, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support of resource parameter when a connector request an access token from an IDP
2 participants