-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce config to allow for password complexity #5727
Introduce config to allow for password complexity #5727
Conversation
7fd350f
to
98a037a
Compare
to be validated in :validatable with lower case, upper case, numbers, and configurable special character presence to be validated on.
98a037a
to
a6301cc
Compare
Hey @nashby if I could please request a review 😄 |
lib/devise.rb
Outdated
# Validate presence of lower case letter in password | ||
mattr_accessor :password_requires_lowercase | ||
@@password_requires_lowercase = false | ||
|
||
# Validate presence of upper case letter in password | ||
mattr_accessor :password_requires_uppercase | ||
@@password_requires_uppercase = false | ||
|
||
# Validate presence of special character in password | ||
mattr_accessor :password_requires_special_character | ||
@@password_requires_special_character = false | ||
|
||
# Special character options | ||
mattr_accessor :password_special_characters | ||
@@password_special_characters = "!?@#$%^&*()_+-=[]{}|:;<>,./" | ||
|
||
# Validate presence of a number in password | ||
mattr_accessor :password_requires_number | ||
@@password_requires_number = false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there could be some more general config like:
@@require_complex_password = false
Which if true
would require all of these individual pieces?
So the validations could become:
validates_format_of :password, with: /\p{Upper}/, if: -> { password_requires_uppercase || require_complex_password }, message: :must_contain_uppercase
Polite bump @nashby @carlosantoniodasilva 😇 |
how about modify the following configurations in the initializer file as below? config.password_complexity = {
upper: 1, # At least 1 uppercase letter
lower: 2, # At least 2 lowercase letters
digit: 3, # At least 3 digits
special: 4, # At least 4 special characters
} |
Thanks @datpmt seems like an elegant solution ✅ One issue which I could see arise however could be a clash between this and password length minimums? For ex, if you set the above, but stuck with the default 8 character minimum, you couldn't satisfy all the configured preferences. I think something like this could use your nicer syntax but also be more ergonomic with the wider validation system: config.password_complexity = {
upper: true, # require upper
lower: false, # don't require lower
digit: true, # require digit
special: true, # require special character
special_characters: ["!", "?", "@", "\"]
} What do you think? |
config.password_complexity = {
upper: true, # require upper
lower: false, # don't require lower
digit: true, # require digit
# special: true, # redundant
special_characters: ["!", "?", "@", "\"] # empty <=> special: false
} @kykyi Ah I see. Cool! Let do it! 👍 |
@datpmt updated to use your dict style ✅ |
test/models/validatable_test.rb
Outdated
@@ -1,10 +1,9 @@ | |||
# encoding: UTF-8 | |||
# frozen_string_literal: true | |||
|
|||
require 'test_helper' | |||
require "test_helper" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require "test_helper" | |
require 'test_helper' |
Prefer single-quoted strings when you don't need string interpolation or special symbols.
References: rubocop
test/models/validatable_test.rb
Outdated
def with_password_requirement(requirement, value) | ||
# Change the password requirement and restore it after the block is executed | ||
original_password_complexity= User.public_send("password_complexity") | ||
original_value = original_password_complexity[requirement] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless assignment to variable - original_value
.
test/models/validatable_test.rb
Outdated
|
||
class ValidatableTest < ActiveSupport::TestCase | ||
test 'should require email to be set' do | ||
test 'should require email to be set' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test 'should require email to be set' do | |
test 'should require email to be set' do |
remove redundant space
36e5f42
to
776a657
Compare
Sorry for stirring the pot but I wanted to cross-post an opinion that presence of upper-cased letters, special characters and numbers has very little to do with password strength and what really contributes to the password strength is the length of the password. I'm genuinely worried that enforcing these password requirements from default will only contribute to poor user experience and potentially less secure passwords overall More context - rails/rails#53984 (comment) Upd: I overlooked the fact that all these requirements are disabled by default which is good. So perhaps it's still useful for applications that have to comply with regulations that are out of their control. I just don't think that setting these requirements should be encouraged |
Agreed @nvasilevski I think whilst this change pushes users to increase the entropy of their passwords, setting these and forgetting could lead devs into a false sense of security ➕ |
I agree with @nvasilevski - here's a specific argument against complexity requirements from the UK's National Cyber Security Centre: https://www.ncsc.gov.uk/collection/passwords/updating-your-approach#PasswordGuidance:UpdatingYourApproach-Donotusecomplexityrequirements Recommend closure of the issue for Devise |
In relation to #5591
This PR introduces application config to allow for password complexity to granularly managed with new validation options for:
These are all
false
by default, and configurable like:Note