IdentityPython / idpy-oidc

Implementation of everything OIDC and OAuth2
Apache License 2.0
40 stars 22 forks source link

Optional resource attribute enforcement #82

Closed PeterBolha closed 11 months ago

PeterBolha commented 1 year ago

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 commented 1 year ago

Please, point the PR towards the main branch

ctriant commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.