Only overwrite password_required? / password if undefined

[fschwahn]

I'm using magic links along with the possibility to use email/password (ie. database_authenticatable). The methods from database_authenticatable are overwritten in

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

Lines 8 to 15 in c38db38

	def password_required?
	false
	end
	# Not having a password method breaks the :validatable module
	def password
	nil
	end

Would it be possible to define those methods only if they are previously undefined?

[abevoelker]

Sounds like a good idea for now - if you want to make a PR I'd accept it.

I haven't done much yet with mixing passwordless + password (database_authenticatable) in the same model yet but I'd definitely like to support it better as I know there is demand (e.g. #7).

One thought is that apps using both of these modules together may have different semantic expectations for their users logging in, e.g.

1. App #⁠1: All users have a saved password; any user may login via email OR password (i.e. accept both methods)
2. App #⁠2: Only some users have a saved password; those users must login via password; everyone else logs in via email

Presumably different semantics may lead to different implementations of the two methods you highlighted based on which type of user is logging in? In any case I can think about it more and deal with it later

[fschwahn]

Presumably different semantics may lead to different implementations of the two methods you highlighted based on which type of user is logging in?

I have exactly that need, and solved it by adding an active_for_magic_link_authentication?-method (the name mirrors a similar method in devise: https://github.com/heartcombo/devise/blob/6d32d2447cc0f3739d9732246b5a5bde98d9e032/lib/devise/models/authenticatable.rb#L93-L95).

I monkey-patched the authentication strategy to check for that:

module OptionalMagicLinkAuthentication
  def validate(resource, &block)
    super { resource.active_for_magic_link_authentication? }
  end
end

Devise::Strategies::MagicLinkAuthenticatable.include(OptionalMagicLinkAuthentication)

And I also adjusted Devise::Passwordless::SessionsController to return a proper error message to the user.

[SethHorsley]

1. App #⁠1: All users have a saved password; any user may login via email OR password (i.e. accept both methods)
2. App #⁠2: Only some users have a saved password; those users must login via password; everyone else logs in via email

#32 fixes this

[abevoelker]

I'm a year late but this should now be fixed on master via adc78bb

@fschwahn I appreciate your suggestions as that's the approach I ended up taking! 🤘 Thanks for your contributions in opening this and providing code snippets.

I just have one final breaking change to implement (#36) before finalizing the 1.0 gem release, which I hope to have ready for tomorrow.

I am now closing this issue as resolved, but feel free to re-open if I've missed something or if you have other concerns. 🙇

[fschwahn]

Great, thank you!

[codyrobbins]

Unfortunately the fix in adc78bb doesn’t appear to actually solve the problem here as far as I can tell. If you’re using the magic_link_authenticatable strategy in tandem with database_authenticable and still want passwords to be required when creating users, it doesn’t work out of the box. I think the problem is that the unless instance_methods.include?(:password_required?) conditional is being evaluated in the context of the Devise::Models::MagicLinkAuthenticatable module itself, so the method always doesn’t exist and thus always ends up getting defined. The same problem exists for #password.

My first thought was to wrap the conditional in included to cause it to be evaluated in the context of the included model. However, this fix then introduces the opposite of the original problem, namely that #password_required? is now always already defined (when you’re using validatable) and it’s not possible to disable the password requirement when just using the magic_link_authenticatable strategy. It appears this is because Devise always orders the inclusion of its modules in the order in which the calls to Devise#add_module are evaluated rather than the order in which they are specified in the devise call on the model. (This seems weird to me! I’ve always assumed the order they were specified on the model was significant, but it’s not.) Because magic_link_authenticatable is added after the builtin modules, #password_required? is at this point always already defined by validatable.

I don’t see an obvious way to fix this to support both use cases (authentication only by magic_link_authenticatable where password is not required, and authentication by both magic_link_authenticatable and database_authenticable where password is required). I originally thought the conditional could be extended to check devise_modules.include?(:database_authenticatable), but devise_modules is not populated at the time the module is included. The base implementation of #password_required? in validatable is more involved than just returning true and ideally it shouldn’t be necessary to copy-and-paste this implementation into your own app in case the underlying Devise implementation changes in the future.

So, I’m not sure how to fix this but unfortunately I think it’s still not working correctly currently! I have a draft PR at #56 which has the fix with included and a new test. You can see that the old tests pass and the new test fails without the included fix, but the old tests fail and the new test passes with the included fix.

For context, I have a slightly different use case than those I’ve seen discussed anywhere else so far. We have regular users that always log in via password, but need to generate passwordless login links that are provided to a separate customer service application via an API that ultimately give customer service agents the ability to log in as a user when dealing with customer service requests. I feel like this gem is a bit too opinionated regarding the assumption that passwordless authentication is going to replace all other forms of authentication, when it really could just be a general provider of a passwordless authentication strategy that may or may not coexist with alternate methods.

[codyrobbins]

For anyone else trying to figure out what you have to do here to strictly add authentication via (and generation of) magic links, but keep everything else about your current authentication set up exactly the same, this gem supports it and makes it easy, but I couldn’t find a clearly articulated step-by-step method for doing so. This is what worked for me:

1. Add magic_link_authenticatable to the devise call on the user model, leaving database_authenticatable and any other modules you’re already using in place.

class User < ApplicationRecord
  devise(
    :database_authenticatable,
    :magic_link_authenticatable,
    :registerable,
    :validatable,
    …
  )

2. If you’re using validatable and want to require passwords on users, you will need to temporarily patch the issue under discussion in this thread.

3. Use the route helper to generate the magic link. (I feel like this is a bit of a magic incantation and the gem should ideally be providing a straightforward helper to do this, e.g. magic_link_url @user.)

magic_link_url(
  @user,
  user: {
    email:      @user.email,
    token:      @user.encode_passwordless_token,
    remember_me: false
  }
)

Note that you do not need to make any changes to your existing routes.