Skip to content

Commit

Permalink
Add pwned_password_check_enabled option
Browse files Browse the repository at this point in the history
  • Loading branch information
TylerRick committed Apr 27, 2020
1 parent 8e056ad commit 423dc75
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 13 deletions.
11 changes: 5 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Add the :pwned_password module to your existing Devise model.

```ruby
class AdminUser < ApplicationRecord
devise :database_authenticatable,
devise :database_authenticatable,
:recoverable, :rememberable, :trackable, :validatable, :pwned_password
end
```
Expand Down Expand Up @@ -86,15 +86,14 @@ config.pwned_password_read_timeout = 2

### Disabling in test environments

Currently this module cannot be mocked out for test environments. Because an API call is made this can slow down tests, or make test fixtures needlessly complex (dynamically generated passwords). The module can be disabled in test environments like this.
Because calling a remote API can slow down tests, and requiring non-pwned passwords can make test fixtures needlessly complex (dynamically generated passwords), you probably want to disable the `pwned_password` check in your tests. You can disable the `pwned_password` check for the test environments by adding this to your `config/initializers/devise.rb` file:

```ruby
class User < ApplicationRecord
devise :invitable ... :validatable, :lockable
devise :pwned_password unless Rails.env.test?
end
config.pwned_password_check_enabled = !Rails.env.test?
```

If there are any tests that required the check to be enabled (such as tests for specifically testing the flow/behavior for what should happen when a user does try to use, or already have, a pwned password), you can temporarily set `Devise.pwned_password_check_enabled = true` for the duration of the test (just be sure to reset it back at the end).

## Installation
Add this line to your application's Gemfile:

Expand Down
4 changes: 3 additions & 1 deletion lib/devise/pwned_password.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
require "devise/pwned_password/model"

module Devise
mattr_accessor :min_password_matches, :min_password_matches_warn, :pwned_password_check_on_sign_in,
mattr_accessor :pwned_password_check_enabled,
:min_password_matches, :min_password_matches_warn, :pwned_password_check_on_sign_in,
:pwned_password_open_timeout, :pwned_password_read_timeout
@@pwned_password_check_enabled = true
@@min_password_matches = 1
@@min_password_matches_warn = nil
@@pwned_password_check_on_sign_in = true
Expand Down
3 changes: 2 additions & 1 deletion lib/devise/pwned_password/hooks/pwned_password.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# frozen_string_literal: true

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
if user.class.respond_to?(:pwned_password_check_enabled) && user.class.pwned_password_check_enabled &&
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)
password && auth.authenticated?(opts[:scope]) && user.respond_to?(:password_pwned?) && user.password_pwned?(password)
end
Expand Down
17 changes: 13 additions & 4 deletions lib/devise/pwned_password/model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ module PwnedPassword
extend ActiveSupport::Concern

included do
validate :not_pwned_password, if: :password_required?
before_validation :reset_pwned
validate :not_pwned_password, if: :check_pwned_password?
end

module ClassMethods
Expand All @@ -25,6 +26,11 @@ module ClassMethods
Devise::Models.config(self, :pwned_password_read_timeout)
end

def check_pwned_password?
Devise.pwned_password_check_enabled &&
(Devise.activerecord51? ? :will_save_change_to_encrypted_password? : :encrypted_password_changed?)
end

def pwned?
@pwned ||= false
end
Expand All @@ -35,9 +41,7 @@ def pwned_count

# Returns true if password is present in the PwnedPasswords dataset
def password_pwned?(password)
@pwned = false
@pwned_count = 0

reset_pwned
options = {
headers: { "User-Agent" => "devise_pwned_password" },
read_timeout: self.class.pwned_password_read_timeout,
Expand All @@ -63,6 +67,11 @@ def password_pwned?(password)

private

def reset_pwned
@pwned = false
@pwned_count = 0
end

def not_pwned_password
# This deliberately fails silently on 500's etc. Most apps won't want to tie the ability to sign up users to the availability of a third-party API.
if password_pwned?(password)
Expand Down
10 changes: 9 additions & 1 deletion test/devise/pwned_password_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class Devise::PwnedPassword::Test < ActiveSupport::TestCase
def setup
User.min_password_matches = 1
User.min_password_matches_warn = nil
Devise.pwned_password_check_enabled = true
end

class WhenPawned < Devise::PwnedPassword::Test
Expand All @@ -22,13 +23,20 @@ class WhenPawned < Devise::PwnedPassword::Test
assert user.valid?
assert user.pwned_count > 0
end

test "when pwned_password_check_enabled = false, is considered valid" do
user = pwned_password_user
Devise.pwned_password_check_enabled = false
assert user.valid?
assert_equal 0, user.pwned_count
end
end

class WhenNotPawned < Devise::PwnedPassword::Test
test "should accept validation and set pwned_count" do
user = valid_password_user
assert user.valid?
assert_equal user.pwned_count, 0
assert_equal 0, user.pwned_count
end

test "when password changed to a pwned password: should add error if pwned_count > min_password_matches_warn || pwned_count > min_password_matches" do
Expand Down

0 comments on commit 423dc75

Please sign in to comment.