Introduce config to allow for password complexity #5727

kykyi · 2024-11-22T03:50:09Z

In relation to #5591

This PR introduces application config to allow for password complexity to be granularly managed with new validation options for:

presence of a lower case letter
presence of a upper case letter
presence of a number
presence of a special character (from a list of special characters that is configurable)

These are all false by default, and configurable like:

Devise.setup do |config|
    config.password_complexity = {
      upper: true,    # require upper
      lower: false,    # don't require lower
      digit: true,    # require digit
      special: true,  # require special character
      special_characters: ["!", "?", "@", "\"]
    }
end

Note

I haven't run any linting, I couldn't find instructions on what config was used for that 😄
I haven't updated any docs, please advise where I need to update if at all 🙏

to be validated in :validatable with lower case, upper case, numbers, and configurable special character presence to be validated on.

kykyi · 2024-11-24T23:30:35Z

Hey @nashby if I could please request a review 😄

kykyi · 2024-11-24T23:54:28Z

lib/devise.rb

+  # 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
+


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

…complexity

kykyi · 2024-12-03T09:31:29Z

Polite bump @nashby @carlosantoniodasilva 😇

datpmt · 2024-12-09T07:55:10Z

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
}

kykyi · 2024-12-09T10:03:12Z

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?

datpmt · 2024-12-09T11:28:21Z

stuck with the default 8 character minimum

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! 👍

kykyi · 2024-12-10T00:35:57Z

@datpmt updated to use your dict style ✅

datpmt · 2024-12-11T09:25:19Z

test/models/validatable_test.rb

@@ -1,10 +1,9 @@
 # encoding: UTF-8
 # frozen_string_literal: true
-
-require 'test_helper'
+require "test_helper"


Suggested change

require "test_helper"

require 'test_helper'

Prefer single-quoted strings when you don't need string interpolation or special symbols.
References: rubocop

datpmt · 2024-12-11T09:27:45Z

test/models/validatable_test.rb

+  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]


Useless assignment to variable - original_value.

datpmt · 2024-12-11T09:32:15Z

test/models/validatable_test.rb


 class ValidatableTest < ActiveSupport::TestCase
-  test 'should require email to be set' do
+  test 'should require email to be set' do      


Suggested change

test 'should require email to be set' do

test 'should require email to be set' do

remove redundant space

nvasilevski · 2024-12-18T17:05:52Z

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

kykyi · 2024-12-18T22:01:43Z

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 ➕

timdiggins · 2024-12-19T16:30:03Z

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

fthobe · 2025-01-05T16:48:29Z

Picking up on a friendly invitation @nvasilevski given here
Mates, I would like to break some glass here.

I do not believe that what you write is wrong, the contrary, I believe some very valid points have been made. I am, as probably also you, sure that we are going towards a mixture of MFA and / or passwordless login and that's overall a good thing, but the environments around us are frequently not there yet.

The problem is the world:

password complexity / length policies are frequenty requirements to pass audits;
password complexity / length should have a reference implementation to avoid errors in implementation;
higher complexity while not scaling as well as length is still better than nothing.

I would also mention that the Password Guidance of the NCSC that you linked is not the only opinion and as much as I love the UK, this guideline is derived from NIST. The reasoning behind it was that statistics had shown that strong requirements create weaker user generated passwords:
Simply users went with "myf!stname$" instead of "some-very-s3cret-words-1n-a-$afe" as they struggled to remember too complex and to long passwords that needed to be changed frequently. Same applied to permutations "my-secretpassword$" became "my-secretpassword$$" upon frequently forced changes. Something that thanks to hashes and salts can (fortunately) not be tracked by a platform.

But all of there guidance needs to be seen in context of the remaining document that advises also:

having password managers;
having preferably time generated OTP tokens;
being hosted on a system by somebody who understands crafting secure servers and infrastructure.

I think @kykyi made a great PR covering everything from configurability to effective resolution of the issue, maybe you could give it a look with different eyes (fearing weak integrations instead of fearing false sense of security) and we add some lines into documentation to make this a better PR covering also that the entire UTF-8 character set is only as great as the password length.

I'd really appreciate a feedback from you guys on this and I really appreciate what you are doing here, just keep in mind that stuff like NIST / NCSC guidelines are forward looking and not backward looking and need to be seen in context of the entire document. In a perfect world we would all have a password manager and OTP token for every password, but well, we all have grandmothers / fathers and parents having issues remembering 5 letters, alfanumeric or not, struggling to use a password manager. Given what devise is, it should have everything on board to make an informed decision on this topic and implement it.

kykyi · 2025-01-05T21:42:38Z

Thanks for the perspective and for weighing in @fthobe 🙏 . I'll reopen and wait for a maintainer to merge/comment/close just so the issue can be resolved 😄

fthobe · 2025-01-05T21:48:16Z

Thanks for the perspective and for weighing in @fthobe 🙏 . I'll reopen and wait for a maintainer to merge/comment/close just so the issue can be resolved 😄

Actually @nvasilevski did not obligate you to close it, sometimes things need some time to get traction and sometimes topics move in waves.

Be patient, open source is neither fast nor democratic 😅

fthobe · 2025-01-07T18:07:10Z

@timdiggins Hey, I would be super interested in your oppinion on my comment :)

fthobe · 2025-01-16T08:39:53Z

Ping @nvasilevski @datpmt @timdiggins

Could you retake a look at this PR and the comments made above.

datpmt · 2025-01-16T08:51:06Z

Ping @nvasilevski @datpmt @timdiggins

Could you retake a look at this PR and the comments made above.

First of all, I completely agree that the length of a password significantly impacts its strength.

In fact, many websites and company security policies require users to apply complexity rules to their passwords. This means that web applications using Devise have a valid reason to implement password complexity as a feature. Whether to enable it or not is up to the owner’s discretion.

I believe Devise should offer this option.

fthobe · 2025-01-16T10:41:15Z

I believe Devise should offer this option.

@datpmt Is this PR for you acceptable or does it need additional work?

timdiggins · 2025-01-16T10:50:27Z

@fthobe Im not a maintainer/committee here so my 10c isn't worth so much 😀

A pragmatic response: My sense is that there isn't much maintainer involvement and devise is going into decline - there are CI-fix and bug fix PRs that have had no maintainer involvement so I don't think there's feature development will gain any traction.

fthobe · 2025-01-16T11:04:40Z

@timdiggins a lot of stuff still relies on it so we need to work with what we have.

I honestly think it's a very mature product.

Anyhow I'd really appreciate your opinion.

@carlosantoniodasilva does this PR have a shot to be integrated?

datpmt · 2025-01-16T12:28:42Z

I believe Devise should offer this option.

@datpmt Is this PR for you acceptable or does it need additional work?

I will take the time to run it locally and provide a review as soon as possible.

fthobe · 2025-01-16T12:38:48Z

@kykyi you should also put some thought in documentation.

nvasilevski · 2025-01-16T14:42:52Z

In fact, many websites and company security policies require users to apply complexity rules to their passwords.

Unless you meant that these applications are obligated to enforce such policy by a law that is out of their control I don't find this argument to be strong. Many applications make poor engineering and design decisions, including choosing unnecessary password/email validations. It shouldn't be a reason for libraries and frameworks to accommodate these poor choices.

Having said that,

password complexity / length policies are frequenty requirements to pass audits;
password complexity / length should have a reference implementation to avoid errors in implementation;

These are strong arguments in favor of having this feature added as an optional one. There is little developers can do when it comes to complying with the law. And having a shared implementation in such cases is a huge win. I was coming from Rails perspective where the feature would become the only implementation of a "strong password" concept making it look like the only correct way to do strong passwords. Here proposal is different, it's an option that isn't enabled by default so I have nothing of a substance to say against. I just hope that applications won't reach for this feature without absolute necessity and let me use my long, secure, easy to remember and easy to type passwords.

fthobe · 2025-01-16T14:52:56Z

Hey @nvasilevski

Here Proposal is different, it's an option that isn't enabled by default so I have nothing of a substance to say against. I just hope that applications won't reach for this feature without absolute necessity and let me use my long, secure, easy to remember and easy to type passwords.

Man, to be honest, it's a mess.
PCI Compliancy just to name one standard (probably the most used one) is directly asking for complexity + MFA. This practically means by law as all ecommerce companies over a certain volume need to adhere.
NIST is saying the opposite.

It should be everybody's choice guided by strong conventions how to do it (which means here) and that's why I am pushing for it.

Also I believe it's not good to run for anything to a separate gem, this is really low hanging fruit and could be done with relatively little effort here and thanks to @kykyi we are half the way there :)

fthobe · 2025-01-20T09:57:29Z

@datpmt i have some time on Wednesday to test this with spree and solidus. Is there anything I should be particularly attentive off?

datpmt · 2025-01-20T15:15:28Z

@datpmt i have some time on Wednesday to test this with spree and solidus. Is there anything I should be particularly attentive off?

@fthobe You need to pay attention to the require_special and special_characters params. I think require_special is somewhat redundant. If I do it, I will only use special_characters. If it's empty, then no special character is required, and vice versa.

fthobe · 2025-01-20T15:20:47Z

@kykyi can you implement a change? @datpmt argument of uniting special and special_characters is very valid to keep everything streamlined. I can test it out on Wednesday with some gems that use devise.

…ly a special character is required

fthobe · 2025-01-21T08:08:03Z

@kykyi thanks!

@datpmt this should be as you wanted it.

fthobe · 2025-01-21T08:30:44Z

@kykyi also given the small amount of files involved you could consider to squash the commits as "merge..." is not a great commit Name for clarity.

kykyi force-pushed the feautre/provide-config-options-for-password-complexity branch from 7fd350f to 98a037a Compare November 22, 2024 04:02

Introduce config to allow for password complexity

a6301cc

to be validated in :validatable with lower case, upper case, numbers, and configurable special character presence to be validated on.

kykyi force-pushed the feautre/provide-config-options-for-password-complexity branch from 98a037a to a6301cc Compare November 22, 2024 04:03

kykyi commented Nov 24, 2024

View reviewed changes

Merge branch 'main' into feautre/provide-config-options-for-password-…

ea11968

…complexity

kykyi added 2 commits December 10, 2024 00:31

Update password complexity to be a hash instead of individual keys

d463f9a

Update docs on Devise::Models

a57aac3

datpmt reviewed Dec 11, 2024

View reviewed changes

Update from PR review

776a657

kykyi force-pushed the feautre/provide-config-options-for-password-complexity branch from 36e5f42 to 776a657 Compare December 15, 2024 22:37

kykyi requested a review from datpmt December 17, 2024 00:55

kykyi mentioned this pull request Dec 18, 2024

Add has_complex_password method for out of the box password complexity validation rails/rails#53984

Open

4 tasks

kykyi closed this Dec 19, 2024

kykyi reopened this Jan 5, 2025

fthobe mentioned this pull request Jan 20, 2025

Utilize Devise Location helpers solidusio-contrib/solidus_social#109

Open

Remove require_special to make the presence of special_characters imp…

44dca10

…ly a special character is required

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce config to allow for password complexity #5727

Introduce config to allow for password complexity #5727

kykyi commented Nov 22, 2024 •

edited

Loading

kykyi commented Nov 24, 2024

kykyi Nov 24, 2024 •

edited

Loading

kykyi commented Dec 3, 2024

datpmt commented Dec 9, 2024

kykyi commented Dec 9, 2024

datpmt commented Dec 9, 2024 •

edited

Loading

kykyi commented Dec 10, 2024

datpmt Dec 11, 2024

datpmt Dec 11, 2024

datpmt Dec 11, 2024

nvasilevski commented Dec 18, 2024 •

edited

Loading

kykyi commented Dec 18, 2024

timdiggins commented Dec 19, 2024

fthobe commented Jan 5, 2025 •

edited

Loading

kykyi commented Jan 5, 2025

fthobe commented Jan 5, 2025

fthobe commented Jan 7, 2025

fthobe commented Jan 16, 2025

datpmt commented Jan 16, 2025

fthobe commented Jan 16, 2025 •

edited

Loading

timdiggins commented Jan 16, 2025 •

edited

Loading

fthobe commented Jan 16, 2025

datpmt commented Jan 16, 2025

fthobe commented Jan 16, 2025

nvasilevski commented Jan 16, 2025

fthobe commented Jan 16, 2025 •

edited

Loading

fthobe commented Jan 20, 2025

datpmt commented Jan 20, 2025

fthobe commented Jan 20, 2025 •

edited

Loading

fthobe commented Jan 21, 2025 •

edited

Loading

fthobe commented Jan 21, 2025

	test 'should require email to be set' do
	test 'should require email to be set' do

Introduce config to allow for password complexity #5727

Are you sure you want to change the base?

Introduce config to allow for password complexity #5727

Conversation

kykyi commented Nov 22, 2024 • edited Loading

kykyi commented Nov 24, 2024

kykyi Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

kykyi commented Dec 3, 2024

datpmt commented Dec 9, 2024

kykyi commented Dec 9, 2024

datpmt commented Dec 9, 2024 • edited Loading

kykyi commented Dec 10, 2024

datpmt Dec 11, 2024

Choose a reason for hiding this comment

datpmt Dec 11, 2024

Choose a reason for hiding this comment

datpmt Dec 11, 2024

Choose a reason for hiding this comment

nvasilevski commented Dec 18, 2024 • edited Loading

kykyi commented Dec 18, 2024

timdiggins commented Dec 19, 2024

fthobe commented Jan 5, 2025 • edited Loading

kykyi commented Jan 5, 2025

fthobe commented Jan 5, 2025

fthobe commented Jan 7, 2025

fthobe commented Jan 16, 2025

datpmt commented Jan 16, 2025

fthobe commented Jan 16, 2025 • edited Loading

timdiggins commented Jan 16, 2025 • edited Loading

fthobe commented Jan 16, 2025

datpmt commented Jan 16, 2025

fthobe commented Jan 16, 2025

nvasilevski commented Jan 16, 2025

fthobe commented Jan 16, 2025 • edited Loading

fthobe commented Jan 20, 2025

datpmt commented Jan 20, 2025

fthobe commented Jan 20, 2025 • edited Loading

fthobe commented Jan 21, 2025 • edited Loading

fthobe commented Jan 21, 2025

kykyi commented Nov 22, 2024 •

edited

Loading

kykyi Nov 24, 2024 •

edited

Loading

datpmt commented Dec 9, 2024 •

edited

Loading

nvasilevski commented Dec 18, 2024 •

edited

Loading

fthobe commented Jan 5, 2025 •

edited

Loading

fthobe commented Jan 16, 2025 •

edited

Loading

timdiggins commented Jan 16, 2025 •

edited

Loading

fthobe commented Jan 16, 2025 •

edited

Loading

fthobe commented Jan 20, 2025 •

edited

Loading

fthobe commented Jan 21, 2025 •

edited

Loading