Modified sessions controller to return 401 for destroy action with no user signed in#4878
Modified sessions controller to return 401 for destroy action with no user signed in#4878tegon merged 4 commits intoheartcombo:5-rcfrom aamarill:fix-4782
Conversation
| set_flash_message! :notice, :already_signed_out | ||
|
|
||
| respond_to_on_destroy | ||
| respond_to do |format| |
There was a problem hiding this comment.
We could avoid the respond_to duplication changing the #respond_to_on_destroy to accept the status as parameter, something like this:
def respond_to_on_destroy(status: :no_content)
respond_to do |format|
format.all { head status }
format.any(*navigational_formats) { redirect_to after_sign_out_path_for(resource_name) }
end
endThen, here we just do respond_to_on_destroy(status: :unauthorized). WDTY?
There was a problem hiding this comment.
Maybe the default :no_content isn't a good idea. I like the idea to have the status explicited in the caller.
respond_to_on_destroy(status: :no_content)
respond_to_on_destroy(status: :unauthorized)| assert_equal 204, @response.status | ||
| end | ||
|
|
||
| test "#destroy returns 204 status if the requested format is not navigational" do |
There was a problem hiding this comment.
This behavior is already tested in the test above. IMHO changing the test name would be enough.
#destroy doesn't set the flash and returns 204 status if the requested format is not navigational
| assert_equal 401, @response.status | ||
| end | ||
|
|
||
| test "#destroy returns 302 status if user is not signed in and the requested format is navigational" do |
|
@aamarill are you still going to work on this? I'm asking to know if I should assign it to someone else. |
|
@tegon please go ahead and assign to someone else. Thank you |
|
@aamarill Since the changes were small, I committed them directly to your branch. Thanks for your contribution! |
It's an unauthenticated request, so return 401 Unauthorized like most other similar requests. Signed-off-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
It's an unauthenticated request, so return 401 Unauthorized like most other similar requests. Signed-off-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
It's an unauthenticated request, so return 401 Unauthorized like most other similar requests. Signed-off-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
It's an unauthenticated request, so return 401 Unauthorized like most other similar requests. Signed-off-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
|
Update: this was originally merged into a "v5" branch that never got into main or released. I just cherry-picked it with a few changes to main to finally (hopefully?) get it released soon: 9a149ff |
…ombo#4878) It's an unauthenticated request, so return 401 Unauthorized like most other similar requests. Signed-off-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
This PR is for issue #4782 @tegon
To clarify the 3 tests added:
1. Authenticated non-navigational request - returns 204.
2. Unauthenticated non-navigational request - returns 401.
3. Unauthenticated navigational request - returns 302.