Closed unleashed closed 3 years ago
/cc @pehala
@unleashed the PR looks good to me, but it doesn't solve all the issues mentioned in the PR description.
First, I think services configured with OIDC should have been using oauth_* endpoints and not the other ones. I guess we didn't document and communicate that well enough.
Also, if Porta is storing app keys for OIDC services, there's something wrong and we should open an issue to discuss that. As you said, that does not make much sense to me.
Lastly, we should investigate whether we can delete the Redirect validator. I suspect we can, but we should check first. Can you open an issue for that? :pray: I think that would allow us to simplify some code.
Rebased on top of current master to pick up the test helper to have an OAuth service fixture.
bors r=@davidor
Build succeeded:
We recently received tickets from users that were having issues getting OIDC to work with the 3scale Istio Adapter.
The reason is that Porta stores
client_secret
s asapp_key
s of applications. This is not needed (and arguably should not be done at all), but the effect is that any service configured as "oauth" calling theauthrep.xml
orauthorize.xml
endpoints must provide the client secret as the app_key parameter.This does not make sense at all, since the premise is that the caller provides a signed OIDC token (ie. via the Authorization header with a bearer token authz type) and this is validated by the proxy / SM API client before calling into Apisonator. There's no gain in providing yet another secret to Apisonator itself.
This change considers whether a service is configured to use OIDC when receiving auth.xml calls (while leaving the oauth_auth.xml calls alone), and changes the requirement that a valid
app_key
must be provided so that if none such key is provided the call will succeed, but if it is provided then success will depend on providing a correct one.Additionally the pre-existing behaviour of being able to use
user_key
s with these kinds of services is kept, even if it does not make sense, to avoid any potential breakage (this is why only oauth_auth*.xml endpoints will require theapp_id
parameter).This PR also paves the way to deprecate the oauth_auth.xml endpoints, as after this lands there will be nothing in oauth_auth.xml that cannot be done already via authorize and authrep.