From 56aadedc8c3776ea9bd9ee8a78c8d837eb9948de Mon Sep 17 00:00:00 2001 From: taitus Date: Fri, 28 Jul 2023 09:30:38 +0200 Subject: [PATCH 1/3] Update devise-security.rb In these commits ffe9ac7 0d8def3 we updated the devise-security version. In these versions the 'password_regex' configuration key and some comments were changed. We update this file in order to use the new configuration key 'password_complexity' and keep comments updated. --- config/initializers/devise-security.rb | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/config/initializers/devise-security.rb b/config/initializers/devise-security.rb index 6de4d3f80..f7ff3bc28 100644 --- a/config/initializers/devise-security.rb +++ b/config/initializers/devise-security.rb @@ -3,20 +3,24 @@ Devise.setup do |config| # Configure security extension for devise # Should the password expire (e.g 3.months) - # config.expire_password_after = false config.expire_password_after = 1.year - # Need 1 char of A-Z, a-z and 0-9 - # config.password_regex = /(?=.*\d)(?=.*[a-z])(?=.*[A-Z])/ + # Need 1 char each of: A-Z, a-z, 0-9, and a punctuation mark or symbol + # You may use "digits" in place of "digit" and "symbols" in place of + # "symbol" based on your preference + # config.password_complexity = { digit: 1, lower: 1, symbol: 1, upper: 1 } # How many passwords to keep in archive # config.password_archiving_count = 5 - # Deny old password (true, false, count) - # config.deny_old_passwords = true + # Deny old passwords (true, false, number_of_old_passwords_to_check) + # Examples: + # config.deny_old_passwords = false # allow old passwords + # config.deny_old_passwords = true # will deny all the old passwords + # config.deny_old_passwords = 3 # will deny new passwords that matches with the last 3 passwords # enable email validation for :secure_validatable. (true, false, validation_options) - # dependency: need an email validator like rails_email_validator + # dependency: see https://github.com/devise-security/devise-security/blob/master/README.md#e-mail-validation # config.email_validation = true # captcha integration for recover form @@ -36,6 +40,9 @@ Devise.setup do |config| # Time period for account expiry from last_activity_at # config.expire_after = 90.days + + # Allow password to equal the email + # config.allow_passwords_equal_to_email = false end module Devise From fe9da7988fbb1511fab6d985aa7b4c155cfe6c50 Mon Sep 17 00:00:00 2001 From: taitus Date: Fri, 28 Jul 2023 10:21:24 +0200 Subject: [PATCH 2/3] Enable password_complexity As it seems that adding complexity to the password is something that might be wanted from the Consul applications, we added the necessary changes to allow it. In this version we simply: - Uncomment the configuration variable "password_complexity" - Set this variable without any restrictions - Adapt the application so that everything still works normally. One of the things that had to be done to adapt the application was to remove the overwriting of the "self.included" method. The original idea of overwriting the "self.included" method seems to be the possibility of being able to overwrite the :current_equal_password_validation validation. The problem comes from the fact that by only calling that validation, the rest of the validations that are defined (in this case "password_complexity") are no longer applied. It seems like a good idea to remove the overwrite of the "self.included" method to allow all the defined validations to be applied and simply overwrite the :current_equal_password_validation method so that everything behaves the same. :allow_passwords_equal_to_email configuration has been enabled too, in order to allow existing records with this configuration. Another change made was to uncomment the line: and to keep everything working the same set the value to false: config.email_validation = false. This change has had to be made because in the documentation of devise-security it says the following: In other words, if we want to use the :secure_validatable module we have to enable this configuration even if its value is "false". If we kept the configuration variable commented out: The following error appears: "uninitialized constant Devise::Models::SecureValidatable::EmailValidator". So it has been verified that if before making any change we decommented the line and added the value of "false", the application worked as normal. --- config/initializers/devise-security.rb | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/config/initializers/devise-security.rb b/config/initializers/devise-security.rb index f7ff3bc28..dde3e34e3 100644 --- a/config/initializers/devise-security.rb +++ b/config/initializers/devise-security.rb @@ -8,7 +8,7 @@ Devise.setup do |config| # Need 1 char each of: A-Z, a-z, 0-9, and a punctuation mark or symbol # You may use "digits" in place of "digit" and "symbols" in place of # "symbol" based on your preference - # config.password_complexity = { digit: 1, lower: 1, symbol: 1, upper: 1 } + config.password_complexity = { digit: 0, lower: 0, symbol: 0, upper: 0 } # How many passwords to keep in archive # config.password_archiving_count = 5 @@ -21,7 +21,7 @@ Devise.setup do |config| # enable email validation for :secure_validatable. (true, false, validation_options) # dependency: see https://github.com/devise-security/devise-security/blob/master/README.md#e-mail-validation - # config.email_validation = true + config.email_validation = false # captcha integration for recover form # config.captcha_for_recover = true @@ -42,7 +42,7 @@ Devise.setup do |config| # config.expire_after = 90.days # Allow password to equal the email - # config.allow_passwords_equal_to_email = false + config.allow_passwords_equal_to_email = true end module Devise @@ -58,14 +58,6 @@ module Devise end module SecureValidatable - def self.included(base) - base.extend ClassMethods - assert_secure_validations_api!(base) - base.class_eval do - validate :current_equal_password_validation - end - end - def current_equal_password_validation if !new_record? && !encrypted_password_change.nil? && !erased? dummy = self.class.new From 7c771b28b5692b39176363257f33df1baeb5d227 Mon Sep 17 00:00:00 2001 From: taitus Date: Mon, 9 Oct 2023 09:16:08 +0200 Subject: [PATCH 3/3] Add password complexity --- app/assets/javascripts/managers.js | 29 +++++++++--- app/models/user.rb | 8 ++++ .../account/edit_password_manually.html.erb | 5 +- config/initializers/devise-security.rb | 2 +- config/secrets.yml.example | 4 ++ spec/models/user_spec.rb | 33 +++++++++++++ spec/system/users_auth_spec.rb | 47 +++++++++++++++++++ 7 files changed, 120 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/managers.js b/app/assets/javascripts/managers.js index b66914f2a..0308e4ba1 100644 --- a/app/assets/javascripts/managers.js +++ b/app/assets/javascripts/managers.js @@ -2,16 +2,33 @@ "use strict"; App.Managers = { generatePassword: function() { - var chars, possible_chars; - possible_chars = "aAbcdeEfghiJkmnpqrstuUvwxyz23456789"; - chars = Array.apply(null, { - length: 12 + var pass, possible_chars, possible_digits, possible_symbols, password_complexity; + password_complexity = $(".generate-random-value").data("password-complexity"); + possible_chars = "abcdefghijklmnopqrstuvwxyz"; + possible_digits = "123456789"; + possible_symbols = "-_.,;!?"; + + pass = Array.apply(null, { + length: 8 }).map(function() { var i; i = Math.floor(Math.random() * possible_chars.length); return possible_chars.charAt(i); - }); - return chars.join(""); + }).join(""); + + for (var i = 0; i < password_complexity.upper; i++) { + pass += possible_chars.charAt(Math.floor(Math.random() * possible_chars.length)).toUpperCase(); + } + + for (var i = 0; i < password_complexity.digit; i++) { + pass += possible_digits.charAt(Math.floor(Math.random() * possible_digits.length)); + } + + for (var i = 0; i < password_complexity.symbol; i++) { + pass += possible_symbols.charAt(Math.floor(Math.random() * possible_symbols.length)); + } + + return pass; }, togglePassword: function(type) { $("#user_password").prop("type", type); diff --git a/app/models/user.rb b/app/models/user.rb index ae195ec54..38eb7911e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -416,6 +416,14 @@ class User < ApplicationRecord update!(subscriptions_token: SecureRandom.base58(32)) if subscriptions_token.blank? end + def self.password_complexity + if Tenant.current_secrets.dig(:security, :password_complexity) + { digit: 1, lower: 1, symbol: 0, upper: 1 } + else + { digit: 0, lower: 0, symbol: 0, upper: 0 } + end + end + private def clean_document_number diff --git a/app/views/management/account/edit_password_manually.html.erb b/app/views/management/account/edit_password_manually.html.erb index 4f4ea6b4e..b86bafaa8 100644 --- a/app/views/management/account/edit_password_manually.html.erb +++ b/app/views/management/account/edit_password_manually.html.erb @@ -12,7 +12,10 @@ - <%= link_to t("management.account.edit.password.random"), "#", class: "generate-random-value float-right" %> + <%= link_to t("management.account.edit.password.random"), + "#", + class: "generate-random-value float-right", + data: { "password-complexity": User.password_complexity } %> <%= f.submit t("management.account.edit.password.save"), class: "button success" %> diff --git a/config/initializers/devise-security.rb b/config/initializers/devise-security.rb index dde3e34e3..6cc8edc25 100644 --- a/config/initializers/devise-security.rb +++ b/config/initializers/devise-security.rb @@ -8,7 +8,7 @@ Devise.setup do |config| # Need 1 char each of: A-Z, a-z, 0-9, and a punctuation mark or symbol # You may use "digits" in place of "digit" and "symbols" in place of # "symbol" based on your preference - config.password_complexity = { digit: 0, lower: 0, symbol: 0, upper: 0 } + config.password_complexity = { digit: 0, lower: 0, symbol: 0, upper: 0 } # Overwritten in User model # How many passwords to keep in archive # config.password_archiving_count = 5 diff --git a/config/secrets.yml.example b/config/secrets.yml.example index a5aa854c4..c2c736321 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -21,6 +21,7 @@ development: multitenancy: false security: last_sign_in: false + password_complexity: false secret_key_base: 56792feef405a59b18ea7db57b4777e855103882b926413d4afdfb8c0ea8aa86ea6649da4e729c5f5ae324c0ab9338f789174cf48c544173bc18fdc3b14262e4 <<: *maps @@ -54,6 +55,7 @@ staging: multitenancy: false security: last_sign_in: false + password_complexity: false tenants: # If you've enabled multitenancy, you can overwrite secrets for a # specific tenant with: @@ -92,6 +94,7 @@ preproduction: multitenancy: false security: last_sign_in: false + password_complexity: false tenants: # If you've enabled multitenancy, you can overwrite secrets for a # specific tenant with: @@ -135,6 +138,7 @@ production: multitenancy: false security: last_sign_in: false + password_complexity: false tenants: # If you've enabled multitenancy, you can overwrite secrets for a # specific tenant with: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d9ce655ad..2ff3a9cb5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -849,4 +849,37 @@ describe User do expect(user.subscriptions_token).to eq "already_set" end end + + describe ".password_complexity" do + it "returns no complexity when the secrets aren't configured" do + expect(User.password_complexity).to eq({ digit: 0, lower: 0, symbol: 0, upper: 0 }) + end + + context "when secrets are configured" do + before do + allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge( + security: { + password_complexity: true + }, + tenants: { + tolerant: { + security: { + password_complexity: false + } + } + } + )) + end + + it "uses the general secrets for the main tenant" do + expect(User.password_complexity).to eq({ digit: 1, lower: 1, symbol: 0, upper: 1 }) + end + + it "uses the tenant secrets for a tenant" do + allow(Tenant).to receive(:current_schema).and_return("tolerant") + + expect(User.password_complexity).to eq({ digit: 0, lower: 0, symbol: 0, upper: 0 }) + end + end + end end diff --git a/spec/system/users_auth_spec.rb b/spec/system/users_auth_spec.rb index f5934515a..60f3c8877 100644 --- a/spec/system/users_auth_spec.rb +++ b/spec/system/users_auth_spec.rb @@ -755,4 +755,51 @@ describe "Users" do expect(page).to have_content "must be different than the current password." end + + context "Regular authentication with password complexity enabled" do + before do + allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge( + security: { password_complexity: true } + )) + end + + context "Sign up" do + scenario "Success with password" do + message = "You have been sent a message containing a verification link. Please click on this" \ + " link to activate your account." + visit "/" + click_link "Register" + + fill_in "Username", with: "Manuela Carmena" + fill_in "Email", with: "manuela@consul.dev" + fill_in "Password", with: "ValidPassword1234" + fill_in "Confirm password", with: "ValidPassword1234" + check "user_terms_of_service" + + click_button "Register" + + expect(page).to have_content message + + confirm_email + + expect(page).to have_content "Your account has been confirmed." + end + + scenario "Errors on sign up" do + visit "/" + click_link "Register" + + fill_in "Username", with: "Manuela Carmena" + fill_in "Email", with: "manuela@consul.dev" + fill_in "Password", with: "invalid_password" + fill_in "Confirm password", with: "invalid_password" + check "user_terms_of_service" + + click_button "Register" + + expect(page).to have_content "must contain at least one digit, must contain at least one" \ + " upper-case letter" + end + end + end end