Closed jonathansimmons closed 1 year ago
One additional note; while manually adding status: :see_other
to the redirect does cause log the request as a 303, I noticed if I don't manually provide it the server logs a 302
.
IF I've configured config.responder.redirect_status = :see_other
shouldn't that 302
dynamically be a 303
, or is devise doing something behind the scenes to treat 302
's as 303
's
If I disable :authenticate_employee!
, the only thing that changes in the flow described above is that the second request to /forms
fails to load because it's trying to call current_employee
.
This leads me to believe that this is more of a turbo problem than Devise, but if anyone sees anything obviously wrong with my attempt to redirect, I'd love the help.
I put the check_concurrent_session callback before the authenticate_employee!
I believe you want authenticate to come first, since if there's no one authenticated the other check probably doesn't matter.
Hard coded :see_other as the status if the check_concurrent_session redirect.
This should be what you want if you're implementing Turbo only. (it works with 302 but recommends 303)
One additional note; while manually adding status: :see_other to the redirect does cause log the request as a 303, I noticed if I don't manually provide it the server logs a 302.
IF I've configured config.responder.redirect_status = :see_other shouldn't that 302 dynamically be a 303, or is devise doing something behind the scenes to treat 302's as 303's
This is Rails behavior, 302 is the default in Rails... the configuration in Devise/Responders is for them alone, and/or if you use respond_with
from Responders. Manual redirect_to
still use Rails defaults, which is 302 for now (may change to 303 in the future)
If I disable :authenticate_employee!, the only thing that changes in the flow described above is that the second request to /forms fails to load because it's trying to call current_employee.
current_employee
should force authentication I believe, but still you want the authenticate_employee!
call there.
At a glance I don't see anything wrong with the setup. I tried setting that scenario up on a test app with Devise + Turbo I had here, and it worked:
https://user-images.githubusercontent.com/26328/222145313-499d0bcc-7b81-41cb-869a-e2eaf0287b09.mp4
As you can see, both when refreshing the page on another session, or clicking a link that should redirect me to an authenticated page, it redirected me back to the login page with a CUSTOM MESSAGE flash.
Here's my sample code based on that blog post and the snippet you shared: (note that I'm using a user
model/resource)
class Users::SessionsController < Devise::SessionsController
skip_before_action :check_concurrent_session
def create
super
set_login_token
end
private
def set_login_token
token = Devise.friendly_token
session[:login_token] = token
current_user.current_login_token = token
current_user.save
end
end
class ApplicationController < ActionController::Base
before_action :check_concurrent_session
private
def check_concurrent_session
return unless user_signed_in? && account_in_use?
sign_out(:user)
redirect_to new_user_session_path, alert: "CUSTOM MESSAGE"
end
def account_in_use?
current_user && !(session[:login_token] == current_user.current_login_token)
end
def after_sign_out_path_for(_)
new_user_session_path
end
end
Logs for the refresh: (GET /)
Started GET "/" for ::1 at 2023-03-01 09:55:11 -0300
Processing by HomeController#index as HTML
User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."id" = ? ORDER BY "users"."id" ASC LIMIT ? [["id", 6], ["LIMIT", 1]]
↳ app/controllers/application_controller.rb:7:in `check_concurrent_session'
Redirected to http://localhost:3000/users/sign_in
Filter chain halted as :check_concurrent_session rendered or redirected
Completed 302 Found in 4ms (ActiveRecord: 0.2ms | Allocations: 1517)
Started GET "/users/sign_in" for ::1 at 2023-03-01 09:55:11 -0300
Processing by Users::SessionsController#new as HTML
Rendering layout layouts/application.html.erb
Rendering users/sessions/new.html.erb within layouts/application
Rendered devise/shared/_links.html.erb (Duration: 0.5ms | Allocations: 467)
Rendered users/sessions/new.html.erb within layouts/application (Duration: 2.1ms | Allocations: 1565)
Rendered layout layouts/application.html.erb (Duration: 5.2ms | Allocations: 3708)
Completed 200 OK in 7ms (Views: 5.8ms | ActiveRecord: 0.0ms | Allocations: 4464)
Logs for the Turbo click: (GET /users/edit -- the link I clicked)
Started GET "/users/edit" for ::1 at 2023-03-01 09:55:17 -0300
Processing by Devise::RegistrationsController#edit as HTML
User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."id" = ? ORDER BY "users"."id" ASC LIMIT ? [["id", 6], ["LIMIT", 1]]
Redirected to http://localhost:3000/users/sign_in
Filter chain halted as :check_concurrent_session rendered or redirected
Completed 302 Found in 5ms (ActiveRecord: 0.2ms | Allocations: 1899)
Started GET "/users/sign_in" for ::1 at 2023-03-01 09:55:17 -0300
Processing by Users::SessionsController#new as HTML
Rendering layout layouts/application.html.erb
Rendering users/sessions/new.html.erb within layouts/application
Rendered devise/shared/_links.html.erb (Duration: 0.4ms | Allocations: 467)
Rendered users/sessions/new.html.erb within layouts/application (Duration: 1.9ms | Allocations: 1565)
Rendered layout layouts/application.html.erb (Duration: 4.0ms | Allocations: 3681)
Completed 200 OK in 6ms (Views: 4.6ms | ActiveRecord: 0.0ms | Allocations: 4352)
Also note that I didn't change the redirect to 303/see other but it works the same.
It doesn't seem to be a Devise specific issue from what I can tell. Using turbo-rails 1.3.3 if that helps.
One caveat I wanted to mention regarding that implementation: if you're using password resets, it automatically logs users in by default, and will skip your set_login_token
logic. You might need to account for that in your implementation too, or disable sign_in_after_reset_password
to prevent that and force users to login using the normal login form.
@carlosantoniodasilva Thank you so much for the investment in this. Your answers gave me the clarity and confirmation that I needed to keep digging further in a different space.
We are seeing a double load on links that don't have data: { turbo: false }
which I don't think should be required but is not a devise problem as I first thought and we'll sort that out why Turbo is acting in such a way another day.
For now, your input helped me resolve my issue.
Sounds good @jonathansimmons, glad it was helpful. Good luck!
Environment
Current behavior
The problem is that if we sign out and redirect a user in a custom callback before the devise
authenticate_employee!
fires, the redirect in our custom is being ignored, and the last redirect, in the case from deviseauthenticate_employee
wins, which means our custom redirect/alert is lost.More context
We're writing a quick single session-tracking function using the principles from this blog post: https://rubyyagi.com/devise-only-allow-one-session-per-user-at-the-same-time/
TLDR; the article describes a simple plan to track a unique token in session and on the user model during
devise/sessions#create
. We then have a callback that runs afterauthenticate_employee!
that checks if the user signing in has a session token that matches what we've stored in the session. If not, we log that user out and redirect them to the login screen with a custom notice explaining why they were signed out.This check and subsequent sign-out are both working, but are being overridden by the subsequent request.
Scenario 1 (working): User A is signed in, and User B signs in at their computer with User A's credentials. If User A refreshes their current page, they are signed out and see our custom notice.
Scenario 2 (not working): User A is signed in, and User B signs in at their computer with User A's credentials. If User A clicks a turbo-enabled link, they are signed out but redirected to the login screen with the default sign-in notice.
Expected behavior
Scenario 2 should function the same as Scenario 1.
Troubleshooting
After watching the logs during this flow, I noticed the following request flow for Scenario 2
check_concurrent_session
fires, sees that User A's session token is invalid (because user B signed in last), and signs out User A. A redirect then fires to send the user to the sign-in page. Before that request finishes, the controller appears to process andauthenticate_employee!
because the requested page in step 2 is requested a second time. That second request then sends its own redirect but with the stock devise signed_out notice.Example logs from this flow:
As you can see, the
/forms
route is being requested twice.Callback code
Troubleshooting
check_concurrent_session
callback before theauthenticate_employee!
check_concurrent_session
callback in an attempt to prevent any further callbacks from firingcheck_concurrent_session
redirect.Undesired fix
So can anyone provide any insight into why turbo is still requesting the page dispute the
check_concurrent_session
redirecting elsewhere?