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

OAuth2 userInfoUrl request method cannot be configured #11013

Closed
mdesmet opened this issue Feb 10, 2022 · 3 comments · Fixed by #11014
Closed

OAuth2 userInfoUrl request method cannot be configured #11013

mdesmet opened this issue Feb 10, 2022 · 3 comments · Fixed by #11014

Comments

@mdesmet
Copy link
Contributor

mdesmet commented Feb 10, 2022

We are using Trino with a custom third party OIDC provider, however this custom OIDC provider doesn't support calling the user info endpoint using POST.

The Openid spec specifies that both GET and POST should work, however unfortunately this is not the case (see https://openid.net/specs/openid-connect-core-1_0.html#UserInfoRequest).

We would like to make this behaviour configurable with POST as default.

@hashhar
Copy link
Member

hashhar commented Feb 11, 2022

cc: @kokosing @lukasz-walkiewicz

@kokosing
Copy link
Member

I think you should report a bug to your OIDC provider. I don't like to adjust Trino to OIDC vendor bugs.

What's the vendor? Can we have tests for them? How we would know that this workaround is no longer needed? Cc @lukasz-walkiewicz

@mdesmet
Copy link
Contributor Author

mdesmet commented Feb 16, 2022

In this case it's a custom OIDC provider from a third party. We have limited control or reach to have it resolved.

As mentioned in the PR, according the openid spec, GET is recommended, while we use POST in Trino. However it's also said in the spec that both GET and POST should be supported, so I follow your reasoning. However just saying, there are actually a lot of custom OIDC providers in the wild. From an adoption perspective, if it enables a few customers to get started quickly with Trino compared to the negligible amount of complexity of adding and maintaining this configuration parameter, why not do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants