From d02a1eae8101da99f0fe4d1f7fd337ac9c28e9bb Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Wed, 14 Sep 2022 01:38:18 +0200
Subject: [PATCH] Validate password strength for internal users

---
 Gemfile                                       |  1 +
 Gemfile.lock                                  |  2 ++
 app/models/internal_user.rb                   |  8 +++++++
 config/locales/de.yml                         |  4 ++++
 config/locales/en.yml                         |  4 ++++
 .../internal_users_controller_spec.rb         | 23 ++++++++++++++++---
 6 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/Gemfile b/Gemfile
index ae504b95..ca7b3088 100644
--- a/Gemfile
+++ b/Gemfile
@@ -49,6 +49,7 @@ gem 'telegraf'
 gem 'tubesock'
 gem 'turbolinks'
 gem 'whenever', require: false
+gem 'zxcvbn-ruby', require: 'zxcvbn'
 
 # Error Tracing
 gem 'mnemosyne-ruby'
diff --git a/Gemfile.lock b/Gemfile.lock
index 8f488bed..a07f47d6 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -540,6 +540,7 @@ GEM
     xpath (3.2.0)
       nokogiri (~> 1.8)
     zeitwerk (2.6.0)
+    zxcvbn-ruby (1.2.0)
 
 PLATFORMS
   ruby
@@ -623,6 +624,7 @@ DEPENDENCIES
   web-console
   webmock
   whenever
+  zxcvbn-ruby
 
 BUNDLED WITH
    2.3.17
diff --git a/app/models/internal_user.rb b/app/models/internal_user.rb
index 977b8c0f..32779e8a 100644
--- a/app/models/internal_user.rb
+++ b/app/models/internal_user.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'zxcvbn'
+
 class InternalUser < User
   authenticates_with_sorcery!
 
@@ -7,6 +9,7 @@ class InternalUser < User
 
   validates :email, presence: true, uniqueness: true
   validates :password, confirmation: true, if: -> { password_void? && validate_password? }, on: :update, presence: true
+  validate :password_strength, if: -> { password_void? && validate_password? }, on: :update
   validates :role, inclusion: {in: ROLES}
 
   def activated?
@@ -25,6 +28,11 @@ class InternalUser < User
   end
   private :validate_password?
 
+  def password_strength
+    result = Zxcvbn.test(password, [email, name, 'CodeOcean'])
+    errors.add(:password, :weak) if result.score < 4
+  end
+
   def teacher?
     role == 'teacher'
   end
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 1743d35e..81b24259 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -238,6 +238,10 @@ de:
           at_most_one_main_file: dürfen höchstens eine Hauptdatei enthalten
           late_submission_deadline_not_alone: darf nicht ohne eine reguläre Abgabefrist verwendet werden
           late_submission_deadline_not_before_submission_deadline: darf nicht vor der reguläre Abgabefrist liegen
+        internal_user:
+          attributes:
+            password:
+              weak: ist zu schwach. Versuchen Sie es mit einem  langen Passwort, welches Groß-, Kleinbuchstaben, Zahlen und Sonderzeichen enthält.
   admin:
     dashboard:
       show:
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 7d4ba314..2c03136f 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -238,6 +238,10 @@ en:
           at_most_one_main_file: must include at most one main file
           late_submission_deadline_not_alone: must not be used without a regular submission deadline
           late_submission_deadline_not_before_submission_deadline: must not be before the submission deadline passed
+        internal_user:
+          attributes:
+            password:
+              weak: is too weak. Try to use a long password with upper and lower case letters, numbers and special characters.
   admin:
     dashboard:
       show:
diff --git a/spec/controllers/internal_users_controller_spec.rb b/spec/controllers/internal_users_controller_spec.rb
index 27122894..1040c324 100644
--- a/spec/controllers/internal_users_controller_spec.rb
+++ b/spec/controllers/internal_users_controller_spec.rb
@@ -282,11 +282,28 @@ describe InternalUsersController do
 
         expect_assigns(user: :user)
 
-        it 'changes the password' do
-          expect(InternalUser.authenticate(user.email, password)).to eq(user)
+        context 'with a weak password' do
+          let(:password) { 'foo' }
+
+          it 'does not change the password' do
+            expect { perform_request.call }.not_to change { user.reload.crypted_password }
+            expect(InternalUser.authenticate(user.email, password)).not_to eq(user)
+          end
+
+          expect_http_status(:ok)
+          expect_template(:reset_password)
         end
 
-        expect_redirect(:sign_in)
+        context 'with a strong password' do
+          let(:password) { SecureRandom.hex(128) }
+
+          it 'changes the password' do
+            expect { perform_request.call }.not_to change { user.reload.crypted_password }
+            expect(InternalUser.authenticate(user.email, password)).to eq(user)
+          end
+
+          expect_redirect(:sign_in)
+        end
       end
 
       context 'without a matching password confirmation' do