Skip to content

Expired session: use 403 Forbidden instead of 410 Gone#11859

Merged
Ansku merged 3 commits intomasterfrom
unknown repository
Apr 8, 2020
Merged

Expired session: use 403 Forbidden instead of 410 Gone#11859
Ansku merged 3 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 6, 2020

Use 403 Forbidden instead of 410 Gone or 404 Not Found when a session is expired. Note that one 410 Gone has already been changed to 404 Not Found in #11556, but the corresponding client code was not adjusted. Since the client code already treats 404 specially, I've decided to use 403 instead (as has been earlier suggested in #4417), even though 404 would be more fitting for this error.

I've also added no-caching headers when 403 is now sent (i.e. when 410 was sent before).

Fixes #4417, updates #11556.

Check when you have completed
[ ] Valid tests for the pull request
[x] Contributing guidelines implemented


This change is Reviewable

…ent caching in more cases.

Change-Id: I9116d3b9858eb30676204c105080d821a1454316
@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 6, 2020

I've patched this (and #11556) into the latest released Vaadin 7.7 JARs and rebuild our app with that, and it seems to work fine. At least I cannot reproduce the heartbeat caching with Safari anymore...

@TatuLund
Copy link
Copy Markdown
Contributor

TatuLund commented Jan 7, 2020

Thanks for your contribution. We will review it.

At quick glance at least the ticket references in the comments are wrong they should be

#3226

and

#4167

@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 7, 2020

@TatuLund the ticket numbers are apparently from the old ticketing system (https://dev.vaadin.com/). Should these be updated also in the original comments, which were copied (and adjusted) by #11556 and this PR? These are https://github.com/vaadin/framework/pull/11859/files#diff-8c63a75131409798ae94bfc088628bc6R63-R69

@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 17, 2020

@TatuLund ping

@TatuLund
Copy link
Copy Markdown
Contributor

@ffdybuster

the ticket numbers are apparently from the old ticketing system

Yes, transition was done couple of years of ago.

Should these be updated also in the original comments

That would be preferred for sake of maintenance.

Change-Id: Icee23e774b3b98e8473dcdedbe11177a6db4f9ff
@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 17, 2020

@TatuLund thanks! I've updated the PR to adjust all these issue numbers.

@ghost
Copy link
Copy Markdown
Author

ghost commented Feb 25, 2020

@TatuLund is there a chance you can review this soon?

@felixfontein
Copy link
Copy Markdown

@TatuLund is there a chance this will get reviewed somewhen?

Copy link
Copy Markdown
Member

@Ansku Ansku left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Ansku Ansku merged commit 477e9fe into vaadin:master Apr 8, 2020
@felixfontein
Copy link
Copy Markdown

@TatuLund @Ansku thank you very much for your feedback, reviewing and merging! :)

Ansku pushed a commit that referenced this pull request Apr 24, 2020
Use 403 Forbidden instead of 410 Gone when session expired. Also prevent caching in more cases.
OlliTietavainenVaadin pushed a commit that referenced this pull request Apr 24, 2020
* Expired session: use 403 Forbidden instead of 410 Gone (#11859)

Use 403 Forbidden instead of 410 Gone when session expired. Also prevent caching in more cases.
@Ansku Ansku added this to the 8.11.0.alpha1 milestone Apr 24, 2020
@ghost ghost deleted the fix-410-gone branch August 31, 2020 06:02
pleku pushed a commit to vaadin/flow that referenced this pull request Jan 13, 2021
mshabarov pushed a commit to vaadin/flow that referenced this pull request Jan 22, 2021
caalador pushed a commit to vaadin/flow that referenced this pull request Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heartbeat should respond 404, not 410, when session expired

3 participants