michaelbanfield / devise-pwned_password

Devise extension that checks user passwords against the PwnedPasswords dataset
https://rubygems.org/gems/devise-pwned_password
MIT License
156 stars 29 forks source link

Check if existing user has weak passwords and force password change #37

Open scicco opened 3 years ago

scicco commented 3 years ago

Hello,

first of all thank you for you for this awesome gem!

Let's suppose an old User was stored before devise-security gem was added to application Gemfile. An existing User has a weak password stored. The User was expired. After some time the User need to be re-activated. After re-activation if he/she performs login with old unsafe password he/she will be able to login. I want to prevent this by forcing a password change.

I'm trying to validate the password for every user on login. If the password is not safe and recoverable module is active I'll send him/her a password reset mail.

I'm trying to implement this use case but my solution is not so good.

I have a Rails application with an User model with the following devise modules (from devise and devise-security gems)

#app/models/user.rb

class User < ApplicationRecord
  #...

  devise :database_authenticatable,
         :recoverable, :rememberable, :trackable, :registerable,
         :confirmable, :timeoutable, :lockable,
         :password_expirable, :secure_validatable, :password_archivable,
         :session_limitable, :expirable, :pwned_password
  #...       
end

and some devise related config files:

#config/initializers/devise.rb

Devise.setup do |config|
  config.mailer_sender = "no-reply@example.com"
  require "devise/orm/active_record"
  config.secret_key = 'myawesomeverylongsecretkey'
  config.case_insensitive_keys = [ :email ]
  config.strip_whitespace_keys = [ :email ]
  config.paranoid = true
  config.skip_session_storage = [:http_auth]
  config.stretches = Rails.env.test? ? 1 : 10
  config.reconfirmable = true
  config.password_length = 8..128
  config.maximum_attempts = 5
  config.reset_password_within = 6.hours
  config.sign_out_via = :delete
end
#config/initializers/devise_security.rb

Devise.setup do |config|
  config.password_complexity = /(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[.@#$%^&*()_\-+=!,;\[\]{}]).{8,}/
  config.email_validation = false
end

and a new key inside devise translation file:

#config/locales/devise.en.yml
en:
  devise:
    confirmations:
      #...
    failure:
      #...
      pwned: "Your password has previously appeared in a data breach and should never be used. Check your Email to change your password"
    #...

So far I've managed to make this work by putting this code inside an initializer:

#config/initializer/devise_pwned_passwords_hooks.rb

Warden::Manager.after_set_user do |user, auth, opts|
  if user.class.respond_to?(:pwned_password_check_on_sign_in) && user.class.pwned_password_check_on_sign_in
    password = auth.request.params.fetch(opts[:scope], {}).fetch(:password, nil)
    is_pwned = password && auth.authenticated?(opts[:scope]) && user.respond_to?(:password_pwned?) && user.password_pwned?(password)
    if is_pwned
      scope = opts[:scope]
      user.send_reset_password_instructions if user.respond_to?(:send_reset_password_instructions)
      auth.logout(scope)
      proxy = Devise::Hooks::Proxy.new(auth)
      Devise.sign_out_all_scopes ? proxy.sign_out : proxy.sign_out(scope)
      throw(:warden, :scope => scope, :reason => :pwned)
    end
  end
end

Warden::Manager.before_failure do |env, opts|
  if opts && opts[:reason] == :pwned
    flash = ActionDispatch::Flash::FlashHash.new
    flash.notice = I18n.t('devise.failure.pwned')
    env[ActionDispatch::Flash::KEY] = flash
  end
end

The problem with this approach is that after logout two flash messages will appear on login page:

(flash[:notice]) Your password has previously appeared in a data breach and should never be used. Check your Email to change your password (flash[:alert]): You need to sign in or sign up before continuing.

because error partial inside login page is looping on each flash key:

<div class="row">
  <div id="content" class="span9">
    <% flash.each do |name, message| %>
      <p><%= message %></p>
    <% end %>
  </div>
</div>

If I try to change Warden::Manager.before_failure hook to use flash.alert instead of flash.notice my custom message is overwritten with the unauthenticated message ("You need to sign in or sign up before continuing.")

Is there a clever way to achieve this business logic? Am I missing something?

Please advise.

Thank you in advance

scicco commented 3 years ago

I found a way:

#config/initializers/devise_pwned_passwords_hooks.rb

Warden::Manager.after_set_user except: :fetch do |user, auth, opts|
  if user.class.respond_to?(:pwned_password_check_on_sign_in) && user.class.pwned_password_check_on_sign_in
    password = auth.request.params.fetch(opts[:scope], {}).fetch(:password, nil)
    is_pwned = password && auth.authenticated?(opts[:scope]) && user.respond_to?(:password_pwned?) && user.password_pwned?(password)
    if is_pwned
      Devise.sign_out_all_scopes
      if defined?(::Devise::Models::Recoverable) && user.respond_to?(:send_reset_password_instructions)
        user.send_reset_password_instructions
        message = :pwned_recoverable
      else
        message = :pwned
      end
      scope = opts[:scope]
      auth.logout(scope)
      throw(:warden, :scope => scope, :message => message)
    end
  end
end
#config/locales/devise.en.yml

en:
  devise:
    failure:
      #...
      pwned: "Your password has previously appeared in a data breach and should never be used. Please contact Support Team to get assistance"
      pwned_recoverable: "Your password has previously appeared in a data breach and should never be used. Check your Email to change your password"
      #...

Hope this could be useful for others.