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

Forwarded ExpiredTokenException #11964

Merged
merged 2 commits into from
Oct 23, 2018
Merged

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Oct 21, 2018

Fixes #11919

This fixes an issue when the oauth part requests a token that is expired. Because of the migration path it woudl just return the InvalidTokenException instead of the ExpiredTokenException resulting in not renewing the token properly.

This was working all properly when introduced. But it got broken by the move to the PublicKey Tokens.

Guess this means time for integration and acceptance tests on the OAuth endpoint. I just need to think how to make the tokens directly expire then ;)

CC: @Dagefoerde

Fixes #11919

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@Dagefoerde
Copy link
Member

Whoa, I shouldn't debug things that rely on an internet connection while I am on the train 😱 I was going to say your fix doesn't work, but: no, it does!
...at least it seams so for now. I still don't know after which amount of time it was invalidated previously, so I am going to test this again in the morning.

Guess this means time for integration and acceptance tests on the OAuth endpoint.

Yess! 👍 :)

Will report back tomorrow. Thanks for now, @rullzer! :)

@rullzer
Copy link
Member Author

rullzer commented Oct 21, 2018

I still don't know after which amount of time it was invalidated previously, so I am going to test this again in the morning.

Normal time out for a token is an hour

@Dagefoerde
Copy link
Member

was 4 hours, so that's covered :)

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

I had troubles reproducing the issue. There is some logic to handle ExpiredTokenException in

} catch (ExpiredTokenException $e) {
$appToken = $e->getToken();

so it looks right to me to throw the exception up.

@rullzer
Copy link
Member Author

rullzer commented Oct 21, 2018

@danielkesselberg yeah it is tricky to fake. Requires a lot of waiting etc. Or manual manipulation. I'll try to come up with writing acceptance tests so we can make sure it keeps working ;)

@Dagefoerde
Copy link
Member

Dagefoerde commented Oct 22, 2018

@rullzer I know I didn't really need to test this anymore, but I did so anyway. :D the issue remains solved as I successfully got a new token this morning. Thanks again.

I think manual manipulation would be the way to go, right? Like changing the expires timestamp to something two hours ago...

(Either GitHub or I have connectivity issues. I removed the spare comments...)

1 similar comment
@Dagefoerde
Copy link
Member

@rullzer I know I didn't really need to test this anymore, but I did so anyway. :D the issue remains solved as I successfully got a new token this morning. Thanks again.

I think manual manipulation would be the way to go, right? Like changing the expires timestamp to something two hours ago...

(Either GitHub or I have connectivity issues. I removed the spare comments...)

@rullzer rullzer requested a review from Dagefoerde October 23, 2018 12:38
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Makes sense!

@rullzer rullzer merged commit a11bef2 into master Oct 23, 2018
@rullzer rullzer deleted the bug/11919/do_not_always_fallback branch October 23, 2018 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth 2 refresh tokens expire early
5 participants