concurent use of logging, via password and passwordless, missed case

[loicginoux]

Hello,
I see you recently fixed this issue #13
In my case it seems like there is a missing case in which the current implementation does not seem to work.
I have a model User, with 2 roles (guest and customer).
I want user with role guest to be able to login only via passwordless strategy
I want user with role customer to be able to login only via password strategy

I tried with something like this:

class User < ApplicationRecord
  devise  :database_authenticatable,
          :registerable,
          :recoverable,
          :rememberable,
          :trackable,
          :validatable,
          :confirmable,
          :magic_link_authenticatable,

  enum role: [:guest, :customer]

  def active_for_authentication?
    super && !guest?
  end

  def active_for_magic_link_authentication?
    super && guest?
  end
end

but it seems like when I sign in with a customer user, and a valid password, the callback defined in devise-passwordless-1.0.1/lib/devise/hooks/magic_link_authenticatable.rb

on after_set_user it will try user.active_for_magic_link_authentication? and will return false.
I will then have an error response like "you are not a guest user" (my custom message coming from magic_link_inactive_message).
Is there something I missed or it's not possible to use both strategy conditionnally on the same resource ?

I think you miss a case in your specs https://github.com/abevoelker/devise-passwordless/blob/adc78bb90d6fee55490bb712a0a373c056ce190b/spec/dummy_app_config/shared_source/all/spec/system/combined_user/sign_in_spec.rb

context "signing in with password" do
  context "with passwordless authentication disabled" do
    it "successfully logs in combined user using password" do ...

I would say this should pass and it does not currently.

version used: v1.0.1

[abevoelker]

What do your routes look like? Annoyingly there does need to be separate paths for password vs passwordless logins because they use different SessionsControllers. In the spec you mentioned and in the README it's done like this:

devise-passwordless/spec/dummy_app_config/shared_source/all/config/routes.rb

Lines 34 to 41 in adc78bb

	# Combined users which can behave as either password or passwordless users
	devise_for :combined_users
	# Passwordless login gets its own namespace because it uses a separate sessions controller
	namespace "passwordless" do
	devise_for :combined_users,
	#only: [:sessions, :registrations, :passwords],
	controllers: { sessions: "devise/passwordless/sessions" }
	end

I would say this should pass and it does not currently.

That spec runs on every commit, and it does pass. You can test locally with https://github.com/nektos/act

[loicginoux]

ok thank you... I guess it will be something about my current setup then..
I guess showing you my current route won't be of much help because I had to mix features from your gem and the devise_token_auth gem because my rails app is a JSON API.
So I cannot just do like in your example.
What I have at the moment is something like this:

namespace :api, defaults: { format: :json }, constraints: { format: 'json' } do
  namespace :auth do
    devise_scope :user do
      post 'guest/sign_in', to: 'sessions#guest_sign_in'
      get 'guest/magic_link', to: 'sessions#guest_magic_link'
    end
  end
end

so
POST /api/auth/guest/sign_in goes to my custom session controller with a method guest_sign_in that sends the email and return a json response
and
GET /api/auth/guest/magic_link also goes to a custom controller method where I authenticate like you do on your gem and sends back token as json response.

I'll try with 2 définitions of devise_for in routes.rb. What I didn't get is how, by declaring 2 routing paths for the 2 separate sign in strategy, it will help prevent this bug from happening.
In other word how come, in your example, if I call POST /combined_users/sign_in it won't enter inside the hook from devise/hooks/magic_link_authenticatable.rb.
I'll continue investigate, thank you for the quick response anyway

[loicginoux]

I could not identify the problem but it might be because of my custom implementation. It still does not work with a custom namespace but anyway, I managed to find a solution go around this problem. I guess I can close the issue

[abevoelker]

Sorry kinda slammed at the moment but would like to look into this later to make this work for your use case better. Not sure when I'll be able to dig in more, possibly next week

[loicginoux]

Thank you, at the moment, as a workaround, guest user have a long password and cannot reset it, and customer users won't have access to passwordless login via UI, even if they find the api endpoint to send the mail, we think it's not a huge problem

[tbelliard]

I encountered the same issue as loicginoux, who had the correct analysis: when activating both database_authenticable and magic_link_authenticatable and making active_for_magic_link_authentication? return false on the User model made password authentication inoperable, because the warden hook does reset the session when active_for_magic_link_authentication? returns false without testing the actual source of successful authentication.

I published pull request #47 as an attempt to fix the issue by testing the winning strategy in the after_set_user hook. I also added a test case to cover this scenario.

I am not especially familiar with the inner workings of Devise and Warden, so not totally sure my approach here is correct; for now at least I have not observed any negative side effects, and I do not think the fix should affect existing setups.

[abevoelker]

@tbelliard Thanks very much for digging in and providing the fix! Fantastic work. I have merged it.

@loicginoux Merging the PR seems to have auto-closed the issue, however if you feel this didn't resolve your problem feel free to reopen. I appreciate you reporting this issue.