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

fix: add status codes to redirects and failure app #5499

Closed

Conversation

KonnorRogers
Copy link

@KonnorRogers KonnorRogers commented Jun 16, 2022

  • Adds 303 (see_other) status codes to redirects

  • Adds a 401 status code when Warden fails to authenticate.

  • This does not directly allow Turbo streams, but does allow other solutions to hook into status codes to handle responses properly via Ajax. (Things like Mrujs or Request.js)

Additional notes

totally open to discussion on this. This arised from a personal fork I was using and am happy to close the PR should another solution arise.

@rafaelfranca
Copy link
Collaborator

This new behavior needs to be configurable, but default. Can you work towards that direction?

@KonnorRogers
Copy link
Author

@rafaelfranca I'll see about making a configurable status code. I previously tweeted about Devise + Warden authentication failures and @carlosantoniodasilva seemed apprehensive about the "failure_app" here. With its state, it can be configured to return a 401 or 200 (current behavior).

Do you think the failure_app as is should be left in with a configurable status code? Or leave it to a user to custom configure their own "failure_app" with a 401 status code?

@KonnorRogers
Copy link
Author

I havent added it to the generators yet nor added tests, but I reworked this PR so it does not add the "redirect_status_code" to "POST" requests since its not needed. It may be confusing since its not applied everywhere, but this is the change that enables the ability to use "fetch" requests with Devise.

@leastbad
Copy link

So, if I understand correctly, I could add this to my initializer:

Devise.setup do |config|
  config.redirect_status_code = 303
  config.failure_status_code = 422
  config.authentication_failure_status_code = 401
end

And suddenly, all of the problems that we have to come up with hacky workarounds for go away?

Magic. Lighting all of my black candles for this one.

@printfinn
Copy link

The proposed fix looks clean and flexible!

carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
@carlosantoniodasilva
Copy link
Member

The main branch should contain all that's necessary for fully working with Turbo now, based on the changes here and a few others. A new version will be released soon, but feel free to test it out from the main branch in the meantime, and report back on any issues. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants