abevoelker/devise-passwordless

User is not logged out when logging in with new session

giorgenes opened this issue · 4 comments

giorgenes commented

No sure if this is by design or not, but when I follow a magic link and a user is already logged in, the new user isn't logged in.

I believe the previous user should be logged off and the new user logged in instead.

abevoelker commented

That's definitely not by design! I'm traveling this week but should be able to dig in more by the end of the week / next weekend. Or, PRs gladly accepted to fix!

  • ❤️1
drewlustro commented

Also experiencing this! I tried to throw the following code in the magic link controller, but it does not work. Old session prevails.

class MagicLinksController < Devise::MagicLinksController
  skip_before_action :initialize_navigation_data

  def show
    Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)

    super
  end
end
  • 👍1
abevoelker commented

(Copying and pasting this to all open issues/PRs:)

Hey all, per #64 I unfortunately won't have much time for the foreseeable future to maintain devise-passwordless to fix the open bugs and work on new features. I'm not abandoning this project, but due to some life issues it's just at the bottom of my priority list for now.

Anyone who wants to step up and be a maintainer to shepherd the project forward would be welcomed! I just ask that you've opened a PR, or written an issue, or can otherwise demonstrate some familiarity/competence with the project. You can reply to #64 or message me privately (through email or socials since GitHub doesn't have DMs) if interested. Thank you ✌️

abevoelker commented

I started a branch to work on this, with a failing test case: https://github.com/abevoelker/devise-passwordless/tree/log-out-old-user-on-magic-link

Also experiencing this! I tried to throw the following code in the magic link controller, but it does not work. Old session prevails.

class MagicLinksController < Devise::MagicLinksController
  skip_before_action :initialize_navigation_data

  def show
    Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)

    super
  end
end

@drewlustro Your code is likely not running due to the

prepend_before_action :require_no_authentication, only: :show

check which short-circuits if there's already a user signed in.

You can comment out that line, or delete it. You can also do the signed in check with Warden for the specific resource. Example:

class Devise::MagicLinksController < DeviseController
  #prepend_before_action :require_no_authentication, only: :show
  prepend_before_action :allow_params_authentication!, only: :show
  prepend_before_action(only: [:show]) { request.env["devise.skip_timeout"] = true }

  # GET /resource/magic_link
  def show
    # Sign out the current user if one is signed in
    sign_out(resource_name) if warden.authenticated?(resource_name)
    # ...

The downside to doing this at the top-level of the controller is you'll be signing out users before checking the validity of the token. So if someone is able to craft a GET request to that URL within your app (e.g. if they can embed an <img src="/users/magic_link">), even with an empty or invalid token, it will sign your current user out.

Unfortunately, Warden really wants to short-circuit authentication strategies if the user is already signed in. So it won't even run our token decode logic in

devise-passwordless/lib/devise/strategies/magic_link_authenticatable.rb

Line 22 in b5ec674

def authenticate!
if the user is already signed in.

Relevant Warden code where this happens: https://github.com/wardencommunity/warden/blob/88d2f59adf5d650238c1e93072635196aea432dc/lib/warden/proxy.rb#L334

At first glance, it seems to me that to handle this scenario we'd either have to:

  1. Hoist token decoding logic up to the magic links controller, perhaps passing decoded stuff down into the Warden strategy (otherwise you're doing a double-decode). This kind of neuters the Warden strategy, and may interfere with the composability of combining other Devise/Warden plugins
  2. Do some funky stuff with Warden, maybe with a custom Proxy, to avoid its current short-circuiting logic. I'd want to be really sure we're not causing a vulnerability with this approach though by changing Warden's behavior

It's kind of silly something seemingly so simple causes such a headache! Maybe a better solution will occur to me or someone else can suggest something I'm missing

  • ❤️1