Fixed /auth endpoint returns a 202 status code regardless of whether the token has expired #2302
Conversation
|
Hi @mohsek, thank you for the contribution but we do have a contribution guide and a PR description template that is automatically applied when you open a PR via the GitHub UI. Please update your description according to: https://github.com/oauth2-proxy/oauth2-proxy/blob/master/.github/PULL_REQUEST_TEMPLATE.md Especially a changelog entry and testing is missing. |
|
Furthermore, we need to check if this would interfere with the token refresh flow. Especially this PR: #1955 |
mohsek
changed the title
mohsek
changed the title
|
This has been a topic that has come up a few times throughout the OAuth2 Proxy project, and, I think changing it would be a breaking change. We know in the past we have had users that have not cared about tokens and their expiry, they just want to limit logins to a particular group, eg using the hosted domain functionality on Google, but don't care how long that session exists for. Changing the default here has the possibility to severely degrade the user experience of use cases such as this. If we want to have this change, it needs to be gated and switched to default in a major release |
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
This is still a desired feature. |
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
Still a relevant feature |
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
still desired feature. |
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
Still a relevant feature |
| @@ -17,7 +17,8 @@ | |||
| - [#2282](https://github.com/oauth2-proxy/oauth2-proxy/pull/2282) Fixed checking Google Groups membership using Google Application Credentials (@kvanzuijlen) | |||
| - [#2183](https://github.com/oauth2-proxy/oauth2-proxy/pull/2183) Allowing relative redirect url though an option | |||
| - [#1866](https://github.com/oauth2-proxy/oauth2-proxy/pull/1866) Add support for unix socker as upstream (@babs) | |||
|
|
|||
| - [#2302](https://github.com/oauth2-proxy/oauth2-proxy/pull/2302) Fixed /auth endpoint returns code 202 regardless of whether the token has expired (@mohsek) | |||
There was a problem hiding this comment.
| - [#2302](https://github.com/oauth2-proxy/oauth2-proxy/pull/2302) Fixed /auth endpoint returns code 202 regardless of whether the token has expired (@mohsek) | |
| - [#2302](https://github.com/oauth2-proxy/oauth2-proxy/pull/2302) Bugfix: /auth endpoint now returns invalid authentication for expired tokens (@mohsek) |
|
I agree with Joel this might be a breaking change for some users. There is another PR that wants to introduce a new option to actively skip validation of expiry. Which might be a good match for the changes proposed here. |
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
@tuunit What is the alternative PR you mention? |
|
This is a pretty unintuitive behaviour for new users of oauth2-proxy though IMO. When I have my application behind the proxy I expect the access token to be valid, otherwise why would I be using oauth2-proxy. Gating it behind a flag makes sense for backwards-compatiblity, but what exactly is going to break here, the refresh happens more frequently? Is that a breaking change? I would prefer the "secure" option to be the default, so refreshing the access token, since Keycloak or whatever OIDC Provider provided the access token said "hey this token is valid for X Time, please refresh it when that time is over. |
Not everyone has the refresh configured and/or working correctly, so for certain users, it would limit the length of their sessions to whatever their IDP configures, rather than giving them that control at the OAuth2 Proxy layer
Yes, but lets introduce a way to retain the old behaviour, and flick the breaking change at a major version boundary |
Description
the endpoint /oauth2/auth doesn't check the expiration date of the token in the session. so when calling the endpoint it return always a response 202 if the session exists, but it never checks if the session has expired.
Motivation and Context
fixes #1836 (complementary to #1955)
How Has This Been Tested?
tested with real environment with oidc (wso2) and nginx
Checklist: