Force passwords to contain specified character classes (#4221).

Patch by Takenori TAKAKI.


git-svn-id: http://svn.redmine.org/redmine/trunk@18411 e93f8b46-1217-0410-a6f0-8f06a7374b81

10 files changed, 80 insertions, 8 deletions

diff --git a/app/models/setting.rb b/app/models/setting.rb
index b5c8ca2e5..384b6e866 100644
--- a/app/models/setting.rb
+++ b/app/models/setting.rb
@@ -19,6 +19,13 @@
 
 class Setting < ActiveRecord::Base
 
+  PASSWORD_CHAR_CLASSES = {
+        'uppercase'     => /[A-Z]/,
+        'lowercase'     => /[a-z]/,
+        'digits'        => /[0-9]/,
+        'special_chars' => /[[:ascii:]&&[:graph:]&&[:^alnum:]]/
+    }
+
   DATE_FORMATS = [
         '%Y-%m-%d',
         '%d/%m/%Y',
diff --git a/app/models/user.rb b/app/models/user.rb
index 9003b253f..a8a80678a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -112,6 +112,9 @@ class User < Principal
   validates_length_of :firstname, :lastname, :maximum => 30
   validates_length_of :identity_url, maximum: 255
   validates_inclusion_of :mail_notification, :in => MAIL_NOTIFICATION_OPTIONS.collect(&:first), :allow_blank => true
+  Setting::PASSWORD_CHAR_CLASSES.each do |k, v|
+    validates_format_of :password, :with => v, :message => :"must_contain_#{k}", :allow_blank => true, :if => Proc.new {Setting.password_required_char_classes.include?(k)}
+  end
   validate :validate_password_length
   validate do
     if password_confirmation && password != password_confirmation
@@ -366,10 +369,22 @@ class User < Principal
 
   # Generate and set a random password on given length
   def random_password(length=40)
-    chars = ("a".."z").to_a + ("A".."Z").to_a + ("0".."9").to_a
-    chars -= %w(0 O 1 l)
+    chars_list = [('A'..'Z').to_a, ('a'..'z').to_a, ('0'..'9').to_a]
+    # auto-generated passwords contain special characters only when admins
+    # require users to use passwords which contains special characters
+    if Setting.password_required_char_classes.include?('special_chars')
+      chars_list << ("\x20".."\x7e").to_a.select {|c| c =~ Setting::PASSWORD_CHAR_CLASSES['special_chars']}
+    end
+    chars_list.each {|v| v.reject! {|c| %(0O1l|'"`*).include?(c)}}
+
     password = +''
-    length.times {|i| password << chars[SecureRandom.random_number(chars.size)] }
+    chars_list.each do |chars|
+      password << chars[SecureRandom.random_number(chars.size)]
+      length -= 1
+    end
+    chars = chars_list.flatten
+    length.times { password << chars[SecureRandom.random_number(chars.size)] }
+    password = password.split('').shuffle(random: SecureRandom).join
     self.password = password
     self.password_confirmation = password
     self
diff --git a/app/views/account/password_recovery.html.erb b/app/views/account/password_recovery.html.erb
index 24da8223b..0c275b476 100644
--- a/app/views/account/password_recovery.html.erb
+++ b/app/views/account/password_recovery.html.erb
@@ -9,6 +9,9 @@
       <label for="new_password"><%=l(:field_new_password)%> <span class="required">*</span></label>
       <%= password_field_tag 'new_password', nil, :size => 25 %>
       <em class="info"><%= l(:text_caracters_minimum, :count => Setting.password_min_length) %></em>
+      <% if Setting.password_required_char_classes.any? %>
+        <em class="info"><%= l(:text_characters_must_contain, :character_classes => Setting.password_required_char_classes.collect{|c| l("label_password_char_class_#{c}")}.join(", ")) %></em>
+    <% end %>
     </p>
 
     <p>
diff --git a/app/views/account/register.html.erb b/app/views/account/register.html.erb
index ade00adfc..f35e0e0cc 100644
--- a/app/views/account/register.html.erb
+++ b/app/views/account/register.html.erb
@@ -8,8 +8,11 @@
   <p><%= f.text_field :login, :size => 25, :required => true %></p>
 
   <p><%= f.password_field :password, :size => 25, :required => true %>
-  <em class="info"><%= l(:text_caracters_minimum, :count => Setting.password_min_length) %></em></p>
-
+  <em class="info"><%= l(:text_caracters_minimum, :count => Setting.password_min_length) %></em>
+  <% if Setting.password_required_char_classes.any? %>
+    <em class="info"><%= l(:text_characters_must_contain, :character_classes => Setting.password_required_char_classes.collect{|c| l("label_password_char_class_#{c}")}.join(", ")) %></em>
+  <% end %>
+  </p>
   <p><%= f.password_field :password_confirmation, :size => 25, :required => true %></p>
 <% end %>
 
diff --git a/app/views/my/password.html.erb b/app/views/my/password.html.erb
index 7a411e51a..4e123db48 100644
--- a/app/views/my/password.html.erb
+++ b/app/views/my/password.html.erb
@@ -9,7 +9,11 @@
 
 <p><label for="new_password"><%=l(:field_new_password)%> <span class="required">*</span></label>
 <%= password_field_tag 'new_password', nil, :size => 25 %>
-<em class="info"><%= l(:text_caracters_minimum, :count => Setting.password_min_length) %></em></p>
+  <em class="info"><%= l(:text_caracters_minimum, :count => Setting.password_min_length) %></em>
+  <% if Setting.password_required_char_classes.any? %>
+    <em class="info"><%= l(:text_characters_must_contain, :character_classes => Setting.password_required_char_classes.collect{|c| l("label_password_char_class_#{c}")}.join(", ")) %></em>
+  <% end %>
+</p>
 
 <p><label for="new_password_confirmation"><%=l(:field_password_confirmation)%> <span class="required">*</span></label>
 <%= password_field_tag 'new_password_confirmation', nil, :size => 25 %></p>
diff --git a/app/views/settings/_authentication.html.erb b/app/views/settings/_authentication.html.erb
index 4bf890d3f..9a39497b8 100644
--- a/app/views/settings/_authentication.html.erb
+++ b/app/views/settings/_authentication.html.erb
@@ -20,6 +20,8 @@
 
 <p><%= setting_text_field :password_min_length, :size => 6 %></p>
 
+<p><%= setting_multiselect :password_required_char_classes, Setting::PASSWORD_CHAR_CLASSES.keys.collect {|c| [l("label_password_char_class_#{c}"), c]} , :inline => true %></p>
+
 <p>
   <%= setting_select :password_max_age, [[l(:label_disabled), 0]] + [7, 30, 60, 90, 180, 365].collect{|days| [l('datetime.distance_in_words.x_days', :count => days), days.to_s]} %>
 </p>
diff --git a/app/views/users/_form.html.erb b/app/views/users/_form.html.erb
index ce5b1f6c7..bb20a4f9d 100644
--- a/app/views/users/_form.html.erb
+++ b/app/views/users/_form.html.erb
@@ -31,8 +31,13 @@
   <p><%= f.select :auth_source_id, ([[l(:label_internal), ""]] + @auth_sources.collect { |a| [a.name, a.id] }), {}, :onchange => "if (this.value=='') {$('#password_fields').show();} else {$('#password_fields').hide();}" %></p>
   <% end %>
   <div id="password_fields" style="<%= 'display:none;' if @user.auth_source %>">
-  <p><%= f.password_field :password, :required => true, :size => 25  %>
-  <em class="info"><%= l(:text_caracters_minimum, :count => Setting.password_min_length) %></em></p>
+  <p>
+    <%= f.password_field :password, :required => true, :size => 25  %>
+    <em class="info"><%= l(:text_caracters_minimum, :count => Setting.password_min_length) %></em>
+    <% if Setting.password_required_char_classes.any? %>
+      <em class="info"><%= l(:text_characters_must_contain, :character_classes => Setting.password_required_char_classes.collect{|c| l("label_password_char_class_#{c}")}.join(", ")) %></em>
+    <% end %>
+  </p>
   <p><%= f.password_field :password_confirmation, :required => true, :size => 25  %></p>
   <p><%= f.check_box :generate_password %></p>
   <p><%= f.check_box :must_change_passwd %></p>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index b4f37f0b7..d0c72ef69 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -132,6 +132,10 @@ en:
         earlier_than_minimum_start_date: "cannot be earlier than %{date} because of preceding issues"
         not_a_regexp: "is not a valid regular expression"
         open_issue_with_closed_parent: "An open issue cannot be attached to a closed parent task"
+        must_contain_uppercase: "must contain uppercase letters (A-Z)"
+        must_contain_lowercase: "must contain lowercase letters (a-z)"
+        must_contain_digits: "must contain digits (0-9)"
+        must_contain_special_chars: "must contain special characters (!, $, %, ...)"
 
   actionview_instancetag_blank_option: Please select
 
@@ -437,6 +441,7 @@ en:
   setting_openid: Allow OpenID login and registration
   setting_password_max_age: Require password change after
   setting_password_min_length: Minimum password length
+  setting_password_required_char_classes : Required character classes for passwords
   setting_lost_password: Allow password reset via email
   setting_new_project_user_role_id: Role given to a non-admin user who creates a project
   setting_default_projects_modules: Default enabled modules for new projects
@@ -1061,6 +1066,10 @@ en:
   label_issue_history_properties: Property changes
   label_issue_history_notes: Notes
   label_last_tab_visited: Last visited tab
+  label_password_char_class_uppercase: uppercase letters
+  label_password_char_class_lowercase: lowercase letters
+  label_password_char_class_digits: digits
+  label_password_char_class_special_chars: special characters
 
   button_login: Login
   button_submit: Submit
@@ -1152,6 +1161,7 @@ en:
   text_project_identifier_info: 'Only lower case letters (a-z), numbers, dashes and underscores are allowed.<br />Once saved, the identifier cannot be changed.'
   text_caracters_maximum: "%{count} characters maximum."
   text_caracters_minimum: "Must be at least %{count} characters long."
+  text_characters_must_contain: "Must contain %{character_classes}."
   text_length_between: "Length between %{min} and %{max} characters."
   text_tracker_no_workflow: No workflow defined for this tracker
   text_role_no_workflow: No workflow defined for this role
diff --git a/config/settings.yml b/config/settings.yml
index 6acfd9769..96b6eca7f 100644
--- a/config/settings.yml
+++ b/config/settings.yml
@@ -36,6 +36,9 @@ lost_password:
   security_notifications: 1
 unsubscribe:
   default: 1
+password_required_char_classes:
+  serialized: true
+  default: []
 password_min_length:
   format: int
   default: 8
diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb
index e03528809..519de8b04 100644
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -539,6 +539,18 @@ class UserTest < ActiveSupport::TestCase
     end
   end
 
+  def test_validate_password_format
+    Setting::PASSWORD_CHAR_CLASSES.each do |key, regexp|
+      with_settings :password_required_char_classes => key do
+        user = User.new(:firstname => "new", :lastname => "user", :login => "random", :mail => "random@somnet.foo")
+        p = 'PASSWDpasswd01234!@#$%'.gsub(regexp, '')
+        user.password, user.password_confirmation = p, p
+        assert !user.save
+        assert_equal 1, user.errors.count
+      end
+    end
+  end
+
   def test_name_format
     assert_equal 'John S.', @jsmith.name(:firstname_lastinitial)
     assert_equal 'Smith, John', @jsmith.name(:lastname_comma_firstname)
@@ -1058,6 +1070,14 @@ class UserTest < ActiveSupport::TestCase
     assert !u.password_confirmation.blank?
   end
 
+  def test_random_password_include_required_characters
+    with_settings :password_required_char_classes => Setting::PASSWORD_CHAR_CLASSES do
+      u = User.new(:firstname => "new", :lastname => "user", :login => "random", :mail => "random@somnet.foo")
+      u.random_password
+      assert u.valid?
+    end
+  end
+
   test "#change_password_allowed? should be allowed if no auth source is set" do
     user = User.generate!
     assert user.change_password_allowed?