Return 422 status when login fails + treat `turbo_stream` as a navigational format #5340

ghiculescu · 2021-02-02T23:08:35Z

Devise FailureApp now returns a 422 status when running the recall flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the new Rails defaults for form errors, as well as with new libraries like Turbo.

By default, the recall flow only runs on the SessionsController.

:turbo_stream is also treated as a navigational format which means Devise will return redirects where appropriate rather than 401s for Turbo requests.

Fixes #5325

santib

Looks good to me. Should this PR also upgrade the responders gem to the version that responds 422 on error?

ghiculescu · 2021-02-02T23:32:54Z

Yes, it should. Once there's a new release of responders I can do that. The last release was a few weeks ago https://github.com/heartcombo/responders/releases

@santib do you mind requesting changes on this PR so we don't forget to do that?

carlosantoniodasilva · 2021-02-03T00:32:15Z

Thanks @ghiculescu, I'll take a closer look in the next few days. And I have yet to release a new responders version with that change, hopefully coming soon to a rubygems near you. :)

ghiculescu · 2021-02-17T19:33:32Z

@carlosantoniodasilva friendly bump here, anything I can do to help get new versions of Devise + Responders with these improvements shipped?

Petercopter · 2021-02-18T05:43:39Z

I've tested these changes locally with hotwire-rails.

I'm getting the 422 when session creation fails
Seeing the flash message
Logs are reporting Processing by SessionsController#new as TURBO_STREAM

Looks good to me!

Thanks to everyone I've been following along with while we get Hotwire going! 👍 🎉

carlosantoniodasilva · 2021-02-18T11:18:05Z

@ghiculescu thanks. Aside from testing this myself, I need to think some more about the breaking change and how this can affect other apps, and a game plan for releasing it... I'm not sure Devise is ready for a major bump, without other things being considered, and I'm not sure we actually need one yet. Anyway, this is all on me now, and I'm working through it.

@Petercopter thanks for testing & confirming this works for you as well.

jasonfb · 2021-03-13T16:00:30Z

I just found this (through debugging) and filed this on Stack Overflow (mostly for the next person)

https://stackoverflow.com/questions/66615478/turbo-rails-with-devise-does-not-redirect-consistently-rails-6-1-3-devise-4-7-3

My reproduction app against Devise 4.7.3 can be found here:
https://github.com/jasonfb/TR001

(I guess we should consider Devise 4.7.3 non-compatible with Turbo-rails --- it would be nice if there was a way to warn upgraders as I think more and more people will come across this as they upgrade to turbo-rails)

I'd love to be able to upgrade my Rails 6.1 app for Turbo-Rails today, but my upgrade is riddled with these Devise bugs-- mostly redirects that don't redirect in the browser but the action has happened on the backend.

I see this pull appears to fix everything (🎉 !), true? false? truthy? falsy?

in master soon? Should I wait a bit? I guess if it were in master I could point my gem to the master branch of devise? Any tips appreciated, even just a monkey patch to get me going until the fix is released.

carlosantoniodasilva · 2021-03-13T17:43:02Z

@jasonfb Devise stable (4.7.3) or master do not support Turbo fully, this PR is aimed to do that, you can try pointing to the PR's branch and test it out yourself to see how that works. I cannot guarantee or give an ETA about a Devise stable release that supports Turbo yet.

And FWIW, I don't consider this a bug per-se, but a change driven by a new library out there that Devise can accommodate to work better out of the box.

tmaier · 2021-04-14T20:47:53Z

@jasonfb maybe this screencast helps you: https://gorails.com/episodes/devise-hotwire-turbo

ghiculescu · 2021-05-06T20:31:28Z

FYI: if you want to use this for now, you can put this in your Gemfile:

gem "devise", github: "ghiculescu/devise", branch: "error-code-422" # https://github.com/heartcombo/devise/pull/5340 not yet merged
gem "responders", github: "heartcombo/responders" # https://github.com/heartcombo/responders/pull/223 not yet released

adrianvalenz · 2021-05-13T09:59:30Z

FYI: if you want to use this for now, you can put this in your Gemfile:

gem "devise", github: "ghiculescu/devise", branch: "error-code-422" # https://github.com/heartcombo/devise/pull/5340 not yet merged
gem "responders", github: "heartcombo/responders" # https://github.com/heartcombo/responders/pull/223 not yet released

Thank you for this. I just added these two gems and devise/hotwire working seamlessly together. There was no need to edit any file.

JoeWoodward · 2021-05-27T06:31:11Z

@ghiculescu thanks. Aside from testing this myself, I need to think some more about the breaking change and how this can affect other apps, and a game plan for releasing it... I'm not sure Devise is ready for a major bump, without other things being considered, and I'm not sure we actually need one yet. Anyway, this is all on me now, and I'm working through it.

@Petercopter thanks for testing & confirming this works for you as well.

@carlosantoniodasilva are there any plans for getting this PR merged at some point? I'm currently disabling turbo on all my devise forms but would really love to let turbo work its magic. If there's a plan/timeline on when this PR will be merged I'll happily wait, otherwise I'll go ahead and use the PR branch, however that is out of date now so I'll have to fork and rebase so would rather just use devise if a release is not too far off.
We're eager to use Turbo and with Devise being a staple in the Rails ecosphere it is putting us (my company) off upgrading all of our Rails apps to Turbo and I'm sure others too.

Thank you for your work. Not trying to pressure you, just looking for some insight so I can take appropriate actions.

silva96 · 2021-07-04T14:54:31Z

Hi, when is this going to be released?

archonic · 2021-07-12T01:31:00Z

I've also tested this (via @ghiculescu's comment) and it fixed the 422 rendering issue when submitting an erroneous form.

It doesn't seem to solve all turbo compatibility however. When submitting a valid form to Registrations#create, the line respond_with resource, location: after_inactive_sign_up_path_for(resource) will render a 200. In that case the client is just left looking at the submitted form.

ghiculescu · 2021-07-12T15:17:08Z

@archonic good pickup, I didn't use Registrations in the app I tested with so I missed that. Can you propose a fix?

archonic · 2021-07-12T15:59:33Z

I'm still working out what the issue is to be honest. A redirect (302 and 303) also doesn't render on the client even though the server is rendering the redirect and the page that is the redirect location.

yrashk · 2021-07-24T16:18:51Z

@ghiculescu @archonic I've used @ghiculescu's branch and it seems to have worked fine for me. I do use registrations and I run system tests on this and I tried it manually as well.

I use a copy of @ghiculescu's branch, rebased on top of 4.8.0: https://github.com/hackerintro/devise/tree/error-code-422

It's not in production yet, but if there's indeed a problem, it'd be great to figure out what it is before it'll be deployed. I am happy to help working this out, @ghiculescu @archonic

ghiculescu · 2022-02-03T15:51:22Z

Thanks @baarkerlounger ! I've pushed those commits to this branch, with you as the co-author. It seems like they broke some tests but in an expected way. Any chance you could look into them? If you push them to your branch I can cherry-pick them there, just let me know or tag me in the commits.

@jasonfb maybe? I'd be (pleasantly!) surprised if this PR is no longer needed. It'd be helpful to know what Rails / Devise / Turbo versions you are using.

baarkerlounger · 2022-02-04T13:31:29Z

@ghiculescu baarkerlounger@6d0b6b5

ghiculescu · 2022-03-01T19:23:29Z

@carlosantoniodasilva saw your comment. I'm happy to help with anything needed to get this done either here or on responders. If you prefer email I'm alex@workforce.com.

jasonfb · 2022-03-22T21:05:31Z

@ghiculescu -- I swear to g-d I saw it working very briefly somewhere around Rails 7.0.1 or 7.02 but if you say I'm crazy that's possible too.

as of today 2022-03-22, this branch is in fact not working for me, I have it specified like this:

gem 'devise', git: 'https://github.com/ghiculescu/devise.git', branch: "error-code-422"

the Rails app I'm testing against is 7.0.2.2 and turbo-rails 1.0.1 but I will try against Rails 7.0.2.3 next to see if that makes a difference.

but I can't get my successful log in to work :

16:56:08 web.1  | Started POST "/users/sign_in" for 127.0.0.1 at 2022-03-22 16:56:08 -0400
16:56:08 web.1  | Processing by Devise::SessionsController#create as TURBO_STREAM
16:56:08 web.1  |   Parameters: {"authenticity_token"=>"[FILTERED]", "user"=>{"email"=>"noone@nowhere.com", "password"=>"[FILTERED]", "remember_me"=>"0"}, "commit"=>"Log in"}
16:56:08 web.1  |   User Load (0.7ms)  SELECT "users".* FROM "users" WHERE "users"."email" = $1 ORDER BY "users"."id" ASC LIMIT $2  [["email", "noone@nowhere.com"], ["LIMIT", 1]]
16:56:09 web.1  | Redirected to http://127.0.0.1:3000/
16:56:09 web.1  | Completed 302 Found in 302ms (ActiveRecord: 0.7ms | Allocations: 2404)
16:56:09 web.1  | 
16:56:09 web.1  | 
16:56:09 web.1  | Started GET "/" for 127.0.0.1 at 2022-03-22 16:56:09 -0400
16:56:09 web.1  | Processing by HomeController#index as TURBO_STREAM
16:56:09 web.1  |   Rendering home/index.turbo_stream.erb
16:56:09 web.1  |   Rendered home/index.turbo_stream.erb (Duration: 0.0ms | Allocations: 15)
16:56:09 web.1  | Completed 200 OK in 0ms (Views: 0.2ms | ActiveRecord: 0.0ms | Allocations: 357)

But what I don't understand is why Turbo just sits there, having requested the GET on the HomeController, the frontend just hangs-in-place with no error message in the console. Am I missing something obvious? it seems like this is the way Turbo is supposed to work, no?

This is supposed to be the successful login, and upon a window reload the user is now in fact logged in. But the operation just hangs in the window

Guess what!?

The failure case actually works fine against this branch! The page reloads as expected.

ghiculescu · 2022-03-23T20:54:20Z

I realise this might be a hard ask, but is it possible to share a repo that I can reproduce the issue with? It's hard to guess what's going on just from the gifs/logs.

Devise doesn't yet support Hotwire submission. As a result, the suggested (and easiest) approach is just to disable Hotwire submission of Devise forms for now. Here is the Devise issue: heartcombo/devise#5340

Devise doesn't yet really support Turbo. As a result, the suggested (and easiest) approach is just to disable Turbo submission of Devise forms for now. Here is the Devise issue: heartcombo/devise#5340

scarroll32 · 2022-06-16T20:50:35Z

Any update on this? Is it still needed or not?

jasonfb · 2022-06-16T20:59:52Z

#5478

dnalbach · 2022-07-16T16:12:33Z

@carlosantoniodasilva - thank you for all your work maintaining this project - you mentioned the following six months ago:

That said, I definitely have plans to work on making it fully Turbo-aware/compatible before 7 final is out. Thanks.

Rails 7 is out now and it looks like the Turbo support hasn't fully arrived yet. Life happens, and I've got a family, so I understand.

Is there anything that you need to help this along? Is it a money issue, a time issue, a design issue, or ?

I may be able to help with any or all of those including a bounty for Turbo support completion.

Is there somewhere that a coordinated group effort is taking place for Devise Turbo support that I could join and help? Please reach out to me if you are able to at:

daniel@fullstackerconsulting.com

joemasilotti · 2022-07-16T20:21:10Z

I'm also happy to help contribute if that's a blocker. I didn't see a Sponsor button anywhere on the repo, though.

maxwell · 2022-08-05T18:39:48Z

Wanted to flag this PR in turbo-rails hotwired/turbo-rails#367

If we had this merged in, I think it would be a useful tool in Devise fully supporting turbo, specifically making a bit nicer when linking to a resource wrapped in an auth check, and maintaining devise's default behavior of redirecting to sign in and restoring the location afterwords.

pepper-roni · 2022-10-28T04:24:27Z

Hey! I just ran into this issue! This seems to solve it, but its been sitting here. It sounds like the block is breaking support for non-turbo rails? Could we at least add data: {turbo: false} in another pr? Or even some info in the readme on state of devise and turbo and workarounds. Happy to help test or write things up to get it moving. For now ill run on this branch... thanks @ghiculescu !

1klap · 2022-10-31T13:45:54Z

i'm running this branch on production for the moment and have no issues with it. i'd love ofc to go back to a devise release as soon as this is shipped. can i help with anything?

i understand that is can be considered a breaking change, could it be extracted into an opt-in config?

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

Hey @ghiculescu, appreciate our work here, and sorry again for such a long time replying back.

I left a few thoughts below as I looked through the changes, but no need to change anything yourself here. I've been working on related changes #5548 on top of what you did here and a few other PRs/issues, to try to accommodate for turbo without making it a full breaking change, and integrating with responders. Your updates here were extremely helpful to get a better understanding of what was needed, and get that up and running, so thank you.

Please anyone feel free to give that one a shot, and leave any feedback or issue you may encounter there. Let's also keep this open for now, it can be closed if / when that one gets merged. I still have some bits to work out, and some tests to add/update, but the bulk of the code should be there. Thanks again.

carlosantoniodasilva · 2023-01-31T14:03:12Z

app/controllers/devise/passwords_controller.rb

@@ -47,7 +47,7 @@ def update
      respond_with resource, location: after_resetting_password_path_for(resource)
    else
      set_minimum_password_length
-      respond_with resource
+      respond_with resource, status: :unprocessable_entity


Were you having any issue with responders alone here, to have to enforce a status?

@baarkerlounger made that change in ghiculescu@eef20a3

The discussion is here #5340 (comment), I don't really remember any context beyond that.

Gotcha, thanks. I think those should be handled by default with responders now 👍

carlosantoniodasilva · 2023-01-31T14:05:48Z

app/controllers/devise/sessions_controller.rb

@@ -77,7 +77,7 @@ def respond_to_on_destroy
    # support returning empty response on GET request
    respond_to do |format|
      format.all { head :no_content }
-      format.any(*navigational_formats) { redirect_to after_sign_out_path_for(resource_name) }
+      format.any(*navigational_formats) { redirect_to after_sign_out_path_for(resource_name), status: :see_other }


Note: responders now has a configuration redirect_status that allows us to override this across the board.

This status is also going to be necessary on the registrations controller destroy action:

devise/app/controllers/devise/registrations_controller.rb

Line 70 in 3632ddf

respond_with_navigational(resource){ redirect_to after_sign_out_path_for(resource_name) }

carlosantoniodasilva · 2023-01-31T14:06:34Z

lib/devise/failure_app.rb

-      self.response = recall_app(warden_options[:recall]).call(request.env)
+      response_from_app = recall_app(warden_options[:recall]).call(request.env)
+      response_from_app[0] = recall_response_code(response_from_app[0])
+      self.response = response_from_app


👍 this is kind of what I had in mind too, change the status code here for failed login attempts.

I thought we could use responders though since we now have this configured there, so I went with that. This way, it can be overridden across the board and not only for this particular response.

carlosantoniodasilva · 2023-02-09T21:20:26Z

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.

ghiculescu mentioned this pull request Feb 2, 2021

Respond form submission errors with 422 status code #5325

Closed

ghiculescu changed the title ~~Return a 422 from FailureApp when login fails~~ Return 422 status when login fails Feb 2, 2021

ghiculescu mentioned this pull request Feb 2, 2021

Respond with 422 Unprocessable Entity for non-GET HTML/JS requests with errors heartcombo/responders#223

Merged

santib approved these changes Feb 2, 2021

View reviewed changes

ghiculescu force-pushed the error-code-422 branch 3 times, most recently from dc8cea8 to 20cdcab Compare February 3, 2021 18:06

ghiculescu changed the title ~~Return 422 status when login fails~~ Return 422 status when login fails + treat turbo_stream as a navigational format Feb 3, 2021

ghiculescu force-pushed the error-code-422 branch 2 times, most recently from 0c8c247 to ef40fca Compare February 3, 2021 19:10

marivaldo approved these changes Feb 3, 2021

View reviewed changes

davegudge mentioned this pull request Feb 19, 2021

Awkward behavior around redirects & error codes hotwired/turbo-rails#34

Closed

Petercopter mentioned this pull request Feb 21, 2021

Turbo new install - forms do not redirect hotwired/turbo-rails#122

Open

jasonfb mentioned this pull request Mar 13, 2021

page does not consistently redirect, on login/logout using Turbo-Rails with Devise 4.7.3 (known issue) -- #5358

Closed

Fix integration tests

b9b793c

jasonfb mentioned this pull request Mar 22, 2022

Rails 7 Cancel My Account #5478

Closed

tmaier mentioned this pull request Mar 27, 2022

turbo_stream fix bugs for rails 7 and above apps #5467

Closed

hartsick mentioned this pull request Mar 30, 2022

Disable Turbo form submission for Devise hartsick/ruBB#57

Merged

carlosantoniodasilva mentioned this pull request May 9, 2022

Settings in config/initilizers/devise.rb for Turbo? #5491

Closed

Pralish mentioned this pull request Jun 21, 2022

nikean.orgpasswords#create (ActionView::MissingTemplate) "Missing template users/passwords/create, users/passwords restarone/violet_rails#783

Closed

franklinjosmell mentioned this pull request Sep 15, 2022

Hacer compatible Devise con Turbo Fix #1 rubyperu/rubyperu-notion#3

Merged

carlosantoniodasilva mentioned this pull request Jan 31, 2023

Integrate with Hotwire/Turbo by configuring error and response statuses #5548

Merged

4 tasks

carlosantoniodasilva reviewed Jan 31, 2023

View reviewed changes

carlosantoniodasilva closed this in #5548 Feb 9, 2023

Return 422 status when login fails + treat turbo_stream as a navigational format #5340

Return 422 status when login fails + treat turbo_stream as a navigational format #5340

Conversation

ghiculescu commented Feb 2, 2021 • edited Loading

santib left a comment

Choose a reason for hiding this comment

ghiculescu commented Feb 2, 2021 • edited Loading

carlosantoniodasilva commented Feb 3, 2021

ghiculescu commented Feb 17, 2021

Petercopter commented Feb 18, 2021

carlosantoniodasilva commented Feb 18, 2021

jasonfb commented Mar 13, 2021 • edited Loading

carlosantoniodasilva commented Mar 13, 2021

tmaier commented Apr 14, 2021

ghiculescu commented May 6, 2021

adrianvalenz commented May 13, 2021

JoeWoodward commented May 27, 2021

silva96 commented Jul 4, 2021

archonic commented Jul 12, 2021 • edited Loading

ghiculescu commented Jul 12, 2021

archonic commented Jul 12, 2021 • edited Loading

yrashk commented Jul 24, 2021

ghiculescu commented Feb 3, 2022 • edited Loading

baarkerlounger commented Feb 4, 2022

ghiculescu commented Mar 1, 2022

jasonfb commented Mar 22, 2022

ghiculescu commented Mar 23, 2022

scarroll32 commented Jun 16, 2022

jasonfb commented Jun 16, 2022

dnalbach commented Jul 16, 2022

joemasilotti commented Jul 16, 2022

maxwell commented Aug 5, 2022

pepper-roni commented Oct 28, 2022

1klap commented Oct 31, 2022

carlosantoniodasilva left a comment

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

ghiculescu Jan 31, 2023

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

carlosantoniodasilva commented Feb 9, 2023

Return 422 status when login fails + treat `turbo_stream` as a navigational format #5340

Return 422 status when login fails + treat `turbo_stream` as a navigational format #5340

ghiculescu commented Feb 2, 2021 •

edited

Loading

ghiculescu commented Feb 2, 2021 •

edited

Loading

jasonfb commented Mar 13, 2021 •

edited

Loading

archonic commented Jul 12, 2021 •

edited

Loading

archonic commented Jul 12, 2021 •

edited

Loading

ghiculescu commented Feb 3, 2022 •

edited

Loading