devise icon indicating copy to clipboard operation
devise copied to clipboard

Validation crashes for password-less authentication

Open kicferk1 opened this issue 4 years ago • 5 comments

Environment

  • Ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]
  • Rails 6.1.1
  • Devise 4.7.3

Current behavior

Context: On my project we implemented passwordless authentication - magic link gets sent to user email address to log in. We wanted to do away with passwords entirely, so I got to removing any mention of passwords from our user model.

A User model defined by the following:

class User < ApplicationRecord
  devise : validatable, :registerable, :trackable, :confirmable, :passwordless_authenticatable
  
  def password_required?
    false
  end
end

With the following table from schema:

create_table "users", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
    t.string "full_name", null: false
    t.string "email", default: "", null: false
    t.string "login_token"
    t.datetime "login_token_valid_until"
    t.datetime "remember_created_at"
    t.datetime "last_sign_in_at"
    t.datetime "current_sign_in_at"
    t.inet "current_sign_in_ip"
    t.inet "last_sign_in_ip"
    t.integer "sign_in_count", default: 0, null: false
    t.string "confirmation_token"
    t.datetime "confirmed_at"
    t.datetime "confirmation_sent_at"
    t.datetime "created_at", precision: 6, null: false
    t.datetime "updated_at", precision: 6, null: false
    t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
    t.index ["email"], name: "index_users_on_email", unique: true
  end

will cause a crash whenever it attempts validation. As mentioned in https://github.com/heartcombo/devise/issues/4913

That's because there is no password method defined on that User.

Expected behavior

Password validation does not cause crashes when the User model does not require password.

Suggested solution

I believe the crash is caused by the line

validates_length_of       :password, within: password_length, allow_blank: true

inside validatable. And I think it could be fixed by changing it to be

validates_length_of       :password, within: password_length, allow_blank: true, if: :password_required?

I checked that it fixed my issue locally, but I don't know if it goes against the philosophy of what validatable should do.

kicferk1 avatar Feb 16 '21 15:02 kicferk1

FWIW I encountered this same issue with a gem I wrote to add a passwordless auth strategy to Devise: https://github.com/abevoelker/devise-passwordless/blob/bad1cdc4a45be0a85c7f4410e8254b0ef2b90c10/lib/devise/models/magic_link_authenticatable.rb#L12

I actually just remembered to open a ticket to report it, and lo and behold this issue is already open. So I'm +1'ing a fix for this.

abevoelker avatar Mar 14 '21 05:03 abevoelker

Funny enough, this was changed on purpose almost 10 years ago: https://github.com/heartcombo/devise/commit/a59410a254ef3d5ca6fecf2b2f8f3b93b448a179, but it wasn't clearly documented why. 🤷‍♂️

carlosantoniodasilva avatar Mar 14 '21 12:03 carlosantoniodasilva

Perhaps instead of checking :password_required? just checking if the model responds to password instead would be acceptable?

validates_length_of       :password, within: password_length, allow_blank: true, if: -> (x) { x.respond_to?(:password) }

:password_required? may have some semantics related to when a :password field exists on the model, but presumably it doesn't make sense to check the length if the field doesn't exist?

abevoelker avatar Apr 18 '21 16:04 abevoelker

@kicferk1 It looks like you approve of abevoelker's solution. Can this ticket be closed?

timlawrenz avatar Mar 06 '24 14:03 timlawrenz

@timlawrenz I am happy for this ticket to be closed.

Kicferk avatar Mar 06 '24 14:03 Kicferk