CV2-4595 Validate password strength in check (#1888)

* CV2-4595: validate password strength

* CV2-4595: fix tests

app/models/concerns/user_invitation.rb

@@ -132,7 +132,12 @@ def self.accept_team_user_invitation(tu, token, options)
       tu.save!
       # options should have password & username keys
       user = User.find_by_invitation_token(token, true)
-      password = options[:password] || Devise.friendly_token.first(8)
+      password = options[:password]
+      if password.blank?
+        # Generate random passsword that match password_complexity validation
+        samples = [('a'..'z'), ('A'..'Z'), (0..9), ['@', '#', '$', '%', '&']].map(&:to_a)
+        password = Devise.friendly_token.first(8).concat(samples.map(&:sample).shuffle.join)
+      end
       unless user.nil?
         invitable = User.accept_invitation!(:invitation_token => token, :password => password)
         user.update_columns(raw_invitation_token: nil, completed_signup: false)

app/models/concerns/user_private.rb

@@ -58,6 +58,11 @@ def validate_duplicate_email
     end
   end
 
+  def password_complexity
+    return if password.blank? || password =~ /^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).{8,70}$/
+    errors.add :password, I18n.t(:error_password_not_strong)
+  end
+
   def handle_duplicate_email(u)
     if u.is_active?
       provider = u.get_user_provider(self.email)

app/models/user.rb

@@ -43,8 +43,7 @@ class ToSOrPrivacyPolicyReadError < StandardError; end
 
   mount_uploader :image, ImageUploader
   validates :image, size: true
-  validate :user_is_member_in_current_team
-  validate :validate_duplicate_email
+  validate :user_is_member_in_current_team, :validate_duplicate_email, :password_complexity
   validate :languages_format, unless: proc { |u| u.settings.nil? }
   validates :api_key_id, absence: true, if: proc { |u| u.type.nil? }
   validates_presence_of :name

config/locales/en.yml

@@ -308,6 +308,7 @@ en:
   media_exists: This item already exists
   source_exists: This source already exists
   email_exists: has already been taken
+  error_password_not_strong: "Complexity requirement not met. Length should be 8-70 characters and include at least: 1 uppercase, 1 lowercase, 1 digit and 1 special character"
   banned_user: Sorry, your account has been banned from %{app_name}. Please contact
     the support team if you think this is an error.
   devise:

db/seeds.rb

@@ -56,7 +56,7 @@ class Setup
   def initialize(existing_user_email)
     @existing_user_email = existing_user_email
     @user_names = Array.new(3) { Faker::Name.first_name.downcase }
-    @user_passwords = Array.new(3) { Faker::Internet.password(min_length: 8) }
+    @user_passwords = Array.new(3) { random_complex_password }
     @user_emails = @user_names.map { |name| Faker::Internet.safe_email(name: name) }
     @team_names = Array.new(4) { Faker::Company.name }
 

lib/sample_data.rb

@@ -27,6 +27,16 @@ def random_machine_name
     name.downcase
   end
 
+  def random_complex_password(length = 10)
+    length -= 4
+    low = random_string(1).downcase
+    up  = random_string(1).upcase
+    num = ('0'..'9').to_a
+    u = ['@', '#', '$', '%', '&'].to_a
+    complex = (num.sample(1) + u.sample(1)).join
+    random_string(length).concat(low, up, complex)
+  end
+
   def random_ip
     "%d.%d.%d.%d" % [rand(256), rand(256), rand(256), rand(256)]
   end
@@ -70,7 +80,7 @@ def create_bot_user(options = {})
     u.name = options[:name] || random_string
     u.login = options.has_key?(:login) ? options[:login] : random_string
     u.email = options[:email] || "#{random_string}@#{random_string}.com"
-    u.password = options[:password] || random_string
+    u.password = options[:password] || random_complex_password
     u.password_confirmation = options[:password_confirmation] || u.password
     u.is_admin = options[:is_admin] if options.has_key?(:is_admin)
     u.api_key_id = options.has_key?(:api_key_id) ? options[:api_key_id] : create_api_key.id
@@ -104,7 +114,7 @@ def create_user(options = {})
     u.login = options.has_key?(:login) ? options[:login] : random_string
     u.token = options.has_key?(:token) ? options[:token] : random_string(50)
     u.email = options[:email] || "#{random_string}@#{random_string}.com"
-    u.password = options.has_key?(:password) ? options[:password] : random_string
+    u.password = options.has_key?(:password) ? options[:password] : random_complex_password
     u.password_confirmation = options[:password_confirmation] || u.password
     u.current_team_id = options[:current_team_id] if options.has_key?(:current_team_id)
     u.is_admin = options[:is_admin] if options.has_key?(:is_admin)

test/controllers/graphql_controller_9_test.rb

@@ -181,7 +181,8 @@ def setup
   end
 
   test "should handle user 2FA" do
-    u = create_user password: 'test1234'
+    p1 = random_complex_password
+    u = create_user password: p1
     t = create_team
     create_team_user team: t, user: u
     authenticate_with_user(u)
@@ -197,16 +198,16 @@ def setup
     post :create, params: { query: query, team: t.slug }
     assert_response :success
     # Enable/Disable 2FA
-    query = "mutation userTwoFactorAuthentication {userTwoFactorAuthentication(input: { clientMutationId: \"1\", id: #{u.id}, otp_required: #{true}, password: \"test1234\", qrcode: \"#{u.current_otp}\" }) { success }}"
+    query = "mutation userTwoFactorAuthentication {userTwoFactorAuthentication(input: { clientMutationId: \"1\", id: #{u.id}, otp_required: #{true}, password: \"#{p1}\", qrcode: \"#{u.current_otp}\" }) { success }}"
     post :create, params: { query: query, team: t.slug }
     assert_response :success
     assert u.reload.otp_required_for_login?
-    query = "mutation userTwoFactorAuthentication {userTwoFactorAuthentication(input: { clientMutationId: \"1\", id: #{u.id}, otp_required: #{false}, password: \"test1234\" }) { success }}"
+    query = "mutation userTwoFactorAuthentication {userTwoFactorAuthentication(input: { clientMutationId: \"1\", id: #{u.id}, otp_required: #{false}, password: \"#{p1}\" }) { success }}"
     post :create, params: { query: query, team: t.slug }
     assert_response :success
     assert_not u.reload.otp_required_for_login?
     # Disable with invalid uid
-    query = "mutation userTwoFactorAuthentication {userTwoFactorAuthentication(input: { clientMutationId: \"1\", id: #{invalid_uid}, otp_required: #{false}, password: \"test1234\" }) { success }}"
+    query = "mutation userTwoFactorAuthentication {userTwoFactorAuthentication(input: { clientMutationId: \"1\", id: #{invalid_uid}, otp_required: #{false}, password: \"#{p1}\" }) { success }}"
     post :create, params: { query: query, team: t.slug }
     assert_response :success
   end

test/controllers/graphql_controller_test.rb

@@ -647,38 +647,42 @@ def setup
   end
 
   test "should change password if token is found and passwords are present and match" do
+    p1 = random_complex_password
     u = create_user
     t = u.send_reset_password_instructions
-    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}\", password: \"123456789\", password_confirmation: \"123456789\" }) { success } }"
+    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}\", password: \"#{p1}\", password_confirmation: \"#{p1}\" }) { success } }"
     post :create, params: { query: query }
     sleep 1
     assert_response :success
     assert !JSON.parse(@response.body).has_key?('errors')
   end
 
   test "should not change password if token is not found and passwords are present and match" do
+    p1 = random_complex_password
     u = create_user
     t = u.send_reset_password_instructions
-    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}x\", password: \"123456789\", password_confirmation: \"123456789\" }) { success } }"
+    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}x\", password: \"#{p1}\", password_confirmation: \"#{p1}\" }) { success } }"
     post :create, params: { query: query }
     sleep 1
     assert_response 400
   end
 
   test "should not change password if token is found but passwords are not present" do
+    p1 = random_complex_password
     u = create_user
     t = u.send_reset_password_instructions
-    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}\", password: \"123456789\" }) { success } }"
+    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}\", password: \"#{p1}\" }) { success } }"
     post :create, params: { query: query }
     sleep 1
     assert_response :success
     assert JSON.parse(@response.body).has_key?('errors')
   end
 
   test "should not change password if token is found but passwords do not match" do
+    p1 = random_complex_password
     u = create_user
     t = u.send_reset_password_instructions
-    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}\", password: \"123456789\", password_confirmation: \"12345678\" }) { success } }"
+    query = "mutation changePassword { changePassword(input: { clientMutationId: \"1\", reset_password_token: \"#{t}\", password: \"#{p1}\", password_confirmation: \"12345678\" }) { success } }"
     post :create, params: { query: query }
     sleep 1
     assert_response 400

test/controllers/omniauth_callbacks_controller_test.rb

@@ -190,7 +190,8 @@ def teardown
   end
 
   test "should connect when current user set" do
-    u = create_user login: 'test', password: '12345678', password_confirmation: '12345678', email: '[email protected]'
+    p1 = random_complex_password
+    u = create_user login: 'test', password: p1, password_confirmation: p1, email: '[email protected]'
     u.confirm
     authenticate_with_user(u)
     request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:twitter]

test/controllers/registrations_controller_test.rb

@@ -16,8 +16,9 @@ def teardown
   end
 
   test "should create user" do
+    p1 = random_complex_password
     assert_difference 'User.count' do
-      post :create, params: { api_user: { password: '12345678', password_confirmation: '12345678', email: '[email protected]', login: 'test', name: 'Test' } }
+      post :create, params: { api_user: { password: p1, password_confirmation: p1, email: '[email protected]', login: 'test', name: 'Test' } }
       assert_response 401 # needs to confirm before login
     end
   end
@@ -32,67 +33,76 @@ def teardown
       User.send_user_invitation(members)
     end
     User.current = Team.current = nil
+    p1 = random_complex_password
     assert_no_difference 'User.count' do
-      post :create, params: { api_user: { password: '12345678', password_confirmation: '12345678', email: email, login: 'test', name: 'Test' } }
+      post :create, params: { api_user: { password: p1, password_confirmation: p1, email: email, login: 'test', name: 'Test' } }
       assert_response :success
     end
   end
 
   test "should create user if confirmed" do
+    p1 = random_complex_password
     User.any_instance.stubs(:confirmation_required?).returns(false)
     assert_difference 'User.count' do
-      post :create, params: { api_user: { password: '12345678', password_confirmation: '12345678', email: '[email protected]', login: 'test', name: 'Test' } }
+      post :create, params: { api_user: { password: p1, password_confirmation: p1, email: '[email protected]', login: 'test', name: 'Test' } }
       assert_response :success
     end
     User.any_instance.unstub(:confirmation_required?)
   end
 
   test "should not create user if password is missing" do
+    p1 = random_complex_password
     assert_no_difference 'User.count' do
-      post :create, params: { api_user: { password_confirmation: '12345678', email: '[email protected]', login: 'test', name: 'Test' } }
+      post :create, params: { api_user: { password_confirmation: p1, email: '[email protected]', login: 'test', name: 'Test' } }
       assert_response 400
     end
   end
 
   test "should not create user if password is too short" do
+    p1 = '1234'
     assert_no_difference 'User.count' do
-      post :create, params: { api_user: { password: '123456', password_confirmation: '123456', email: '[email protected]', login: 'test', name: 'Test' } }
+      post :create, params: { api_user: { password: p1, password_confirmation: p1, email: '[email protected]', login: 'test', name: 'Test' } }
       assert_response 400
     end
   end
 
   test "should not create user if password don't match" do
+    p1 = random_complex_password
     assert_no_difference 'User.count' do
-      post :create, params: { api_user: { password: '12345678', password_confirmation: '12345677', email: '[email protected]', login: 'test', name: 'Test' } }
+      post :create, params: { api_user: { password: random_complex_password, password_confirmation: random_complex_password, email: '[email protected]', login: 'test', name: 'Test' } }
       assert_response 400
     end
   end
 
   test "should not create user if email is not present" do
+    p1 = random_complex_password
     assert_no_difference 'User.count' do
-      post :create, params: { api_user: { password: '12345678', password_confirmation: '12345678', email: '', login: 'test', name: 'Test' } }
+      post :create, params: { api_user: { password: p1, password_confirmation: p1, email: '', login: 'test', name: 'Test' } }
       assert_response 400
     end
   end
 
   test "should create user if login is not present" do
+    p1 = random_complex_password
     assert_difference 'User.count' do
-      post :create, params: { api_user: { password: '12345678', password_confirmation: '12345678', email: '[email protected]', login: '', name: 'Test' } }
+      post :create, params: { api_user: { password: p1, password_confirmation: p1, email: '[email protected]', login: '', name: 'Test' } }
       assert_response 401 # needs to confirm before login
     end
   end
 
   test "should not create user if name is not present" do
+    p1 = random_complex_password
     assert_no_difference 'User.count' do
-      post :create, params: { api_user: { password: '12345678', password_confirmation: '12345678', email: '[email protected]', login: 'test', name: '' } }
+      post :create, params: { api_user: { password: p1, password_confirmation: p1, email: '[email protected]', login: 'test', name: '' } }
       assert_response 400
     end
   end
 
   test "should update only a few attributes" do
-    u = create_user name: 'Foo', login: 'test', token: 'test', email: '[email protected]', password: '12345678'
+    p1 = random_complex_password
+    u = create_user name: 'Foo', login: 'test', token: 'test', email: '[email protected]', password: p1
     authenticate_with_user(u)
-    post :update, params: { api_user: { name: 'Bar', login: 'bar', token: 'bar', email: '[email protected]', current_password: '12345678' } }
+    post :update, params: { api_user: { name: 'Bar', login: 'bar', token: 'bar', email: '[email protected]', current_password: p1 } }
     assert_response :success
     u = u.reload
     assert_equal 'Bar', u.name
@@ -103,20 +113,23 @@ def teardown
   end
 
   test "should not update account if not logged in" do
-    post :update, params: { api_user: { name: 'Bar', login: 'bar', token: 'bar', email: '[email protected]', current_password: '12345678' } }
+    p1 = random_complex_password
+    post :update, params: { api_user: { name: 'Bar', login: 'bar', token: 'bar', email: '[email protected]', current_password: p1 } }
     assert_response 401
   end
 
   test "should not update account" do
-    u = create_user name: 'Foo', login: 'test', token: 'test', email: '[email protected]', password: '12345678'
+    p1 = random_complex_password
+    u = create_user name: 'Foo', login: 'test', token: 'test', email: '[email protected]', password: p1
     authenticate_with_user(u)
-    post :update, params: { api_user: { name: 'Bar', login: 'bar', token: 'bar', email: '[email protected]', current_password: '12345678', password: '123', password_confirmation: '123' } }
+    post :update, params: { api_user: { name: 'Bar', login: 'bar', token: 'bar', email: '[email protected]', current_password: p1, password: '123', password_confirmation: '123' } }
     assert_response 400
     u = u.reload
   end
 
   test "should destroy account" do
-    u = create_user name: 'Foo', login: 'test', token: 'test', email: '[email protected]', password: '12345678'
+    p1 = random_complex_password
+    u = create_user name: 'Foo', login: 'test', token: 'test', email: '[email protected]', password: p1
     authenticate_with_user(u)
     assert_difference 'User.count', -1 do
       delete :destroy, params: {}