Modified sessions controller to return 401 for destroy action with no user signed in #4878
Conversation
| @@ -62,7 +62,10 @@ def verify_signed_out_user | |||
| if all_signed_out? | |||
| 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)
| @@ -88,6 +88,30 @@ class SessionsControllerTest < Devise::ControllerTestCase | |||
| 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! |
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.