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

Fix password validator for short domains #10201

Merged
merged 4 commits into from Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 13 additions & 9 deletions decidim-core/app/validators/password_validator.rb
Expand Up @@ -5,7 +5,7 @@ class PasswordValidator < ActiveModel::EachValidator
MINIMUM_LENGTH = 10
MAX_LENGTH = 256
MIN_UNIQUE_CHARACTERS = 5
IGNORE_SIMILARITY_SHORTER_THAN = 4
IGNORE_SIMILARITY_SHORTER_THAN = Decidim.config.password_similarity_length
ADMIN_MINIMUM_LENGTH = Decidim.config.admin_password_min_length
ADMIN_REPETITION_TIMES = Decidim.config.admin_password_repetition_times
VALIDATION_METHODS = [
Expand Down Expand Up @@ -106,22 +106,26 @@ def nickname_included_in_password?
value.include?(record.nickname)
end

def check_domain_length(domain)
domain.split(".").each do |part|
next if part.length < IGNORE_SIMILARITY_SHORTER_THAN

return true if value.downcase.include?(part.downcase)
end

false
end

def email_included_in_password?
return false if !record.respond_to?(:email) || record.email.blank?

name, domain, _whatever = record.email.split("@")
value.include?(name) || (domain && value.include?(domain.split(".").first))
value.include?(name) || (domain && check_domain_length(domain))
end

def domain_included_in_password?
return false unless organization && organization.host
return true if value.include?(organization.host)

organization.host.split(".").each do |part|
next if part.length < IGNORE_SIMILARITY_SHORTER_THAN

return true if value.include?(part)
end
return true if value.include?(organization.host) || check_domain_length(organization.host)

false
end
Expand Down
5 changes: 5 additions & 0 deletions decidim-core/lib/decidim/core.rb
Expand Up @@ -481,6 +481,11 @@ def self.reset_all_column_information
[]
end

# Ignores strings similar to email / domain on password validation if too short
config_accessor :password_similarity_length do
4
end

# Defines if admins are required to have stronger passwords than other users
config_accessor :admin_password_strong do
true
Expand Down
47 changes: 46 additions & 1 deletion decidim-core/spec/validators/password_validator_spec.rb
Expand Up @@ -11,7 +11,7 @@
let(:record) do
double(
name: ::Faker::Name.name,
email: ::Faker::Internet.email,
email:,
nickname: ::Faker::Internet.username(specifier: 10..15),
current_organization: organization,
errors:,
Expand All @@ -22,6 +22,7 @@
)
end
let(:admin_record) { false }
let(:email) { ::Faker::Internet.email }
let(:previous_passwords) { [] }
let(:attribute) { "password" }
let(:options) do
Expand Down Expand Up @@ -140,6 +141,28 @@
end
end

describe "parts of email domain included in password" do
context "when less than 4 character parts" do
let(:email) { "john.doe@1.lvh.me" }
alecslupu marked this conversation as resolved.
Show resolved Hide resolved
let(:value) { "Summer1Snowlvhme" }

it "ignores domain validation" do
expect(validator).to be(true)
expect(record.errors[attribute]).to be_empty
end
end

context "when 4 or more character parts" do
let(:email) { "john.doe@decidim.com" }
alecslupu marked this conversation as resolved.
Show resolved Hide resolved
let(:value) { "Decidim1945" }

it "validates with domain" do
expect(validator).to be(false)
expect(record.errors[attribute]).to eq(["is too similar to your email"])
end
end
end

describe "name included in password" do
let(:value) { "foo#{record.name.delete(" ")}bar" }

Expand Down Expand Up @@ -167,6 +190,28 @@
end
end

describe "parts of organization host included in password" do
let(:organization) { create(:organization, host: "www.decidim.1.lvh.me") }

context "when less than 4 character parts" do
let(:value) { "Summer1Snowlvhme" }

it "ignores domain validation" do
expect(validator).to be(true)
expect(record.errors[attribute]).to be_empty
end
end

context "when 4 or more character parts" do
let(:value) { "Decidim1945" }

it "validates with domain" do
expect(validator).to be(false)
expect(record.errors[attribute]).to eq(["is too similar to this domain name"])
end
end
end

describe "common password" do
let(:value) { "qwerty12345" }

Expand Down
Expand Up @@ -391,6 +391,7 @@
config.follow_http_x_forwarded_host = Rails.application.secrets.decidim[:follow_http_x_forwarded_host].present?
config.maximum_conversation_message_length = Rails.application.secrets.decidim[:maximum_conversation_message_length].to_i
config.password_blacklist = Rails.application.secrets.decidim[:password_blacklist] if Rails.application.secrets.decidim[:password_blacklist].present?
config.password_similarity_length = Rails.application.secrets.decidim[:password_similarity_length] if Rails.application.secrets.decidim[:password_similarity_length].present?
config.allow_open_redirects = Rails.application.secrets.decidim[:allow_open_redirects] if Rails.application.secrets.decidim[:allow_open_redirects].present?
end

Expand Down
Expand Up @@ -39,6 +39,7 @@ decidim_default: &decidim_default
follow_http_x_forwarded_host: <%%= Decidim::Env.new("DECIDIM_FOLLOW_HTTP_X_FORWARDED_HOST").to_boolean_string %>
maximum_conversation_message_length: <%%= Decidim::Env.new("DECIDIM_MAXIMUM_CONVERSATION_MESSAGE_LENGTH", "1000").to_i %>
password_blacklist: <%%= Decidim::Env.new("DECIDIM_PASSWORD_BLACKLIST").to_array(separator: ", ").to_json %>
password_similarity_length: <%%= Decidim::Env.new("DECIDIM_PASSWORD_SIMILARITY_LENGTH", 4).to_i %>
allow_open_redirects: <%%= Decidim::Env.new("DECIDIM_ALLOW_OPEN_REDIRECTS").to_boolean_string %>
social_share_services: <%%= Decidim::Env.new("DECIDIM_SOCIAL_SHARE_SERVICES", "Twitter, Facebook, WhatsApp, Telegram").to_array.to_json %>
service_worker_enabled: <%%= Decidim::Env.new("DECIDIM_SERVICE_WORKER_ENABLED", Rails.env.exclude?("development")).to_boolean_string %>
Expand Down
Expand Up @@ -167,6 +167,7 @@
"DECIDIM_FOLLOW_HTTP_X_FORWARDED_HOST" => "true",
"DECIDIM_MAXIMUM_CONVERSATION_MESSAGE_LENGTH" => "1234",
"DECIDIM_PASSWORD_BLACKLIST" => "i-dont-like-this-password, i-dont,like,this,one,either, password123456",
"DECIDIM_PASSWORD_SIMILARITY_LENGTH" => "4",
"DECIDIM_ALLOW_OPEN_REDIRECTS" => "true",
"DECIDIM_ADMIN_PASSWORD_EXPIRATION_DAYS" => "93",
"DECIDIM_ADMIN_PASSWORD_MIN_LENGTH" => "18",
Expand Down Expand Up @@ -290,6 +291,7 @@
%w(decidim follow_http_x_forwarded_host) => false,
%w(decidim maximum_conversation_message_length) => 1000,
%w(decidim password_blacklist) => [],
%w(decidim password_similarity_length) => 4,
%w(decidim allow_open_redirects) => false,
%w(decidim admin_password expiration_days) => 90,
%w(decidim admin_password min_length) => 15,
Expand Down Expand Up @@ -394,6 +396,7 @@
%w(decidim follow_http_x_forwarded_host) => true,
%w(decidim maximum_conversation_message_length) => 1234,
%w(decidim password_blacklist) => ["i-dont-like-this-password", "i-dont,like,this,one,either", "password123456"],
%w(decidim password_similarity_length) => 4,
%w(decidim allow_open_redirects) => true,
%w(decidim admin_password expiration_days) => 93,
%w(decidim admin_password min_length) => 18,
Expand Down Expand Up @@ -488,6 +491,7 @@
"follow_http_x_forwarded_host" => false,
"maximum_conversation_message_length" => 1000,
"password_blacklist" => [],
"password_similarity_length" => 4,
"allow_open_redirects" => false,
"etherpad" => nil,
"maps" => nil
Expand Down Expand Up @@ -524,6 +528,7 @@
"follow_http_x_forwarded_host" => true,
"maximum_conversation_message_length" => 1234,
"password_blacklist" => ["i-dont-like-this-password", "i-dont,like,this,one,either", "password123456"],
"password_similarity_length" => 4,
"allow_open_redirects" => true,
"etherpad" => {
"server" => "http://a-etherpad-server.com",
Expand Down
5 changes: 5 additions & 0 deletions docs/modules/configure/pages/environment_variables.adoc
Expand Up @@ -639,6 +639,11 @@ Separate each item of the array with a comma AND a space, for instance:
|
|No

|DECIDIM_PASSWORD_SIMILARITY_LENGTH
|Amount of characters required by parts of a domain (site or email / separated by a dot) for them to be taken into account during a password validation
|4
|No

|DECIDIM_SOCIAL_SHARE_SERVICES
|The social networking services that will be available when using the "Share" feature on some resources.

Expand Down