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

Optional resource attribute enforcement #82

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

PeterBolha
Copy link
Contributor

@PeterBolha PeterBolha commented Oct 11, 2023

Fixes #81

Provides a wrapper method that does not return an error when the validate resource attribute policy is set but the attribute is not provided in the client's request.

@c00kiemon5ter
Copy link
Member

c00kiemon5ter commented Oct 11, 2023

Please, point the PR towards the main branch

@PeterBolha PeterBolha changed the base branch from develop to main October 11, 2023 10:23
@ctriant
Copy link
Contributor

ctriant commented Oct 23, 2023

Hey @PeterBolha thanks for the contribution!
I think it would be a good idea to replace the check regarding the existence of resource in request of the original validate_resource_indicators_policy with the one you propose on your optional_validate_resource_indicators_policy.

Thus, instead of returning
oauth2.AuthorizationErrorResponse( error="invalid_target", error_description="Missing resource parameter", ) we will return the actual request.

That way we'll not need the optional wrapper, and therefore a mechanism/switch to enable it.

@c00kiemon5ter does this sound reasonable?

@c00kiemon5ter
Copy link
Member

Yes, it makes sense. This is what the RFC specifies

In requests to the authorization server, a client MAY indicate the protected resource (a.k.a. resource server, application, API, etc.) to which it is requesting access by including the following parameter in the request. [...]

emphasis on MAY, meaning that requests without the resource query param should still be processed.

@PeterBolha PeterBolha force-pushed the optional-resource-attribute branch from 4760e56 to 463def8 Compare October 24, 2023 09:11
Copy link
Contributor

@ctriant ctriant left a comment

Choose a reason for hiding this comment

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

Fixes will be needed on tests after this change.

@PeterBolha
Copy link
Contributor Author

I have fixed the related test, however there are two more failing ones. I've rebased my branch to have the latest main branch. Just now I'm noticing that the latest main has these two errors popping up in tests. Should I wait for the fixes in main and rebase later or figure out something else?

@ctriant
Copy link
Contributor

ctriant commented Oct 24, 2023

I have fixed the related test, however there are two more failing ones. I've rebased my branch to have the latest main branch. Just now I'm noticing that the latest main has these two errors popping up in tests. Should I wait for the fixes in main and rebase later or figure out something else?

Thanks! Don't worry about these tests. Let's wait a bit before we merge this, and if necessary I'll ask you to rebase your branch later on.

@c00kiemon5ter c00kiemon5ter force-pushed the optional-resource-attribute branch from 26f4aa7 to 9cdf4ae Compare December 18, 2023 18:22
@c00kiemon5ter c00kiemon5ter dismissed ctriant’s stale review December 18, 2023 18:27

The task is complete

@rohe rohe merged commit 55a83be into IdentityPython:main Dec 18, 2023
5 checks passed
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.

Support for optional resource attribute validation
4 participants