Skip to content

Commit

Permalink
Bug fix for password not being required when used in tandem with the …
Browse files Browse the repository at this point in the history
…`database_authenticatable` and `validatable` modules.

This fix properly ensures that the `#password_required?` and `#password` methods are not redefined by `Devise::Models::MagicLinkAuthenticatable` if they already exist in the model. The check for the existing methods via `instance_methods.include?` must be done in the context of the class the module is being included into and not in the module itself.

See #13 for full context.
  • Loading branch information
codyrobbins committed Apr 16, 2024
1 parent 904eb53 commit 5d95f84
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 18 deletions.
39 changes: 21 additions & 18 deletions lib/devise/models/magic_link_authenticatable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,29 @@ module Models
module MagicLinkAuthenticatable
extend ActiveSupport::Concern

# Models using the :database_authenticatable strategy will already
# have #password_required? and #password defined - we will defer
# to those methods if they already exist so that people can use
# both strategies together. Otherwise, for :magic_link_authenticatable-
# only users, we define them in order to disable password validations:

unless instance_methods.include?(:password_required?)
def password_required?
false
included do
# Models using the :database_authenticatable strategy will already
# have #password defined and models using the :validatable strategy
# will already have #password_required? defined - we will defer
# to those methods if they already exist so that people can use
# both strategies together. Otherwise, for :magic_link_authenticatable-
# only users, we define them in order to disable password validations:

unless instance_methods.include?(:password_required?)
def password_required?
false
end
end
end

unless instance_methods.include?(:password)
# Not having a #password method breaks the :validatable module
#
# NOTE I proposed a change to Devise to fix this:
# https://github.com/heartcombo/devise/issues/5346#issuecomment-822022834
# As of yet it hasn't been accepted due to unknowns of the legacy code's purpose
def password
nil
unless instance_methods.include?(:password)
# Not having a #password method breaks the :validatable module
#
# NOTE I proposed a change to Devise to fix this:
# https://github.com/heartcombo/devise/issues/5346#issuecomment-822022834
# As of yet it hasn't been accepted due to unknowns of the legacy code's purpose
def password
nil
end
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,25 @@
expect(page).to have_css("p.combined_user span.email", text: email)
expect(page).to have_text("Welcome! You have signed up successfully.")
end

it "validates password" do
email = "[email protected]"

visit "/combined_users/sign_up"

expect(page.status_code).to be(200)
expect(page).to have_css("h2", text: "Sign up")

# It creates a user in the database
fill_in "Email", with: email
expect { click_button "Sign up" }.not_to change { CombinedUser.count }

# No emails are sent
expect(ActionMailer::Base.deliveries).to be_empty

# Sign in fails with custom error message
expect(page).to have_css("h2", text: "Sign up")
expect(page).to have_css("#error_explanation", text: "1 error prohibited this combined user from being saved:\nPassword can't be blank")
expect(page).not_to have_text("Welcome! You have signed up successfully.")
end
end

0 comments on commit 5d95f84

Please sign in to comment.