Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kykyi
Copy link

@kykyi kykyi commented Nov 22, 2024

In relation to #5591

This PR introduces application config to allow for password complexity to 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_length = 8..128 # (existing)
      config.password_requires_lowercase = true
      config.password_requires_uppercase = true
      config.password_requires_number = true
      config.password_requires_special_character = true
      config.password_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 🙏

@kykyi kykyi force-pushed the feautre/provide-config-options-for-password-complexity branch from 7fd350f to 98a037a Compare November 22, 2024 04:02
to be validated in :validatable with lower case,
upper case, numbers, and configurable special character
presence to be validated on.
@kykyi kykyi force-pushed the feautre/provide-config-options-for-password-complexity branch from 98a037a to a6301cc Compare November 22, 2024 04:03
@kykyi
Copy link
Author

kykyi commented Nov 24, 2024

Hey @nashby if I could please request a review 😄

lib/devise.rb Outdated
Comment on lines 123 to 142
# 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

Copy link
Author

@kykyi kykyi Nov 24, 2024

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

@kykyi
Copy link
Author

kykyi commented Dec 3, 2024

Polite bump @nashby @carlosantoniodasilva 😇

@datpmt
Copy link

datpmt commented Dec 9, 2024

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
Copy link
Author

kykyi commented Dec 9, 2024

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
Copy link

datpmt commented Dec 9, 2024

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
Copy link
Author

kykyi commented Dec 10, 2024

@datpmt updated to use your dict style ✅

@@ -1,10 +1,9 @@
# encoding: UTF-8
# frozen_string_literal: true

require 'test_helper'
require "test_helper"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require "test_helper"
require 'test_helper'

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

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]
Copy link

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.


class ValidatableTest < ActiveSupport::TestCase
test 'should require email to be set' do
test 'should require email to be set' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

remove redundant space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants