Allow internal users to change their password

MrSerth · MrSerth · commit 860657b132f3 · 2024-11-16T12:37:10.000+01:00
Also, admins are allowed to change the password of other internal users.
diff --git a/app/controllers/internal_users_controller.rb b/app/controllers/internal_users_controller.rb
@@ -6,7 +6,7 @@ class InternalUsersController < ApplicationController
   before_action :require_user!, except: %i[activate forgot_password reset_password]
   before_action :require_activation_token, only: :activate
   before_action :require_reset_password_token, only: :reset_password
-  before_action :set_user, only: MEMBER_ACTIONS
+  before_action :set_user, only: MEMBER_ACTIONS + %i[change_password]
   before_action :collect_set_and_unset_study_group_memberships, only: MEMBER_ACTIONS + %i[create]
   after_action :verify_authorized, except: %i[activate forgot_password reset_password]
 
@@ -58,13 +58,28 @@ def reset_password
 
   def update
     # Let's skip the password validation if the user is edited through
-    # the form by another user. Otherwise, the update might fail if an
+    # the form here. Otherwise, the update might fail if an
     # activation_token or password_reset_token is present
-    @user.validate_password = current_user == @user
+    @user.validate_password = false
     @user.platform_admin = platform_admin_param if current_user.admin?
     update_and_respond(object: @user, params: internal_user_params)
   end
 
+  def change_password
+    return render :change_password unless request.patch? || request.put?
+
+    if @user != current_user || @user.valid_password?(params[:internal_user].delete(:current_password))
+      # Always validate the password strength when changing the password
+      @user.validate_password = true
+      update_password
+    else
+      respond_to do |format|
+        @user.errors.add(:base, :current_password_invalid)
+        respond_with_invalid_object(format, object: @user, template: :change_password)
+      end
+    end
+  end
+
   def destroy
     destroy_and_respond(object: @user)
   end
@@ -75,14 +90,15 @@ def authorize!
     authorize(@user || @users)
   end
 
-  def change_password
+  def update_password
     respond_to do |format|
       if @user.update(params[:internal_user].permit(:password, :password_confirmation))
         @user.change_password!(params[:internal_user][:password])
-        format.html { redirect_to(sign_in_path, notice: t('internal_users.reset_password.success')) }
+        redirect_target = current_user ? internal_user_path(@user) : sign_in_path
+        format.html { redirect_to(redirect_target, notice: t('internal_users.reset_password.success')) }
         format.json { head :ok }
       else
-        respond_with_invalid_object(format, object: @user, template: :reset_password)
+        respond_with_invalid_object(format, object: @user, template: action_name.to_sym)
       end
     end
   end
diff --git a/app/models/internal_user.rb b/app/models/internal_user.rb
@@ -8,8 +8,8 @@ class InternalUser < User
   attr_accessor :validate_password
 
   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 :password, confirmation: true, if: -> { password_void? || validate_password? }, on: :update, presence: true
+  validate :password_strength, if: -> { password_void? || validate_password? }, on: :update
 
   accepts_nested_attributes_for :study_group_memberships
 
@@ -23,7 +23,7 @@ def password_void?
   private :password_void?
 
   def validate_password?
-    return true if @validate_password.nil?
+    return false if @validate_password.nil?
 
     @validate_password
   end
diff --git a/app/policies/internal_user_policy.rb b/app/policies/internal_user_policy.rb
@@ -13,8 +13,8 @@ def show?
     admin? || @record == @user || teacher_in_study_group?
   end
 
-  def change_codeharbor_link?
-    admin? || @record == @user
+  %i[change_codeharbor_link? change_password?].each do |action|
+    define_method(action) { admin? || @record == @user }
   end
 
   class Scope < Scope
diff --git a/app/views/internal_users/change_password.html.slim b/app/views/internal_users/change_password.html.slim
@@ -0,0 +1,16 @@
+h1 = t('.headline')
+
+= form_for(@user, url: change_password_internal_user_path) do |f|
+  = render('shared/form_errors', object: @user)
+  .mb-3
+    = f.label(:password, class: 'form-label')
+    = f.password_field(:password, class: 'form-control', required: true, autocomplete: 'new-password')
+  .mb-3
+    = f.label(:password_confirmation, class: 'form-label')
+    = f.password_field(:password_confirmation, class: 'form-control', required: true, autocomplete: 'new-password')
+  - if @user == current_user
+    .mb-3
+      = f.label(:current_password, class: 'form-label')
+      = f.password_field(:current_password, class: 'form-control', required: true, autocomplete: 'current-password')
+      .help-block.form-text = t('.hints.current_password')
+  .actions = submit_tag(t('.submit'), class: 'btn btn-primary')
diff --git a/app/views/internal_users/show.html.slim b/app/views/internal_users/show.html.slim
@@ -30,3 +30,4 @@ h1
 
 - if @user == current_user || current_user.admin?
   = row(label: 'codeharbor_link.profile_label', value: @user.codeharbor_link.nil? ? link_to(t('codeharbor_link.new'), polymorphic_path([@user, CodeharborLink], action: :new), class: 'btn btn-secondary') : link_to(t('codeharbor_link.edit'), polymorphic_path([@user, @user.codeharbor_link], action: :edit), class: 'btn btn-secondary'))
+  = row(label: 'internal_user.password', value: link_to(t('.change_password'), change_password_internal_user_path(@user), class: 'btn btn-secondary'))
diff --git a/config/locales/de/internal_user.yml b/config/locales/de/internal_user.yml
@@ -6,6 +6,7 @@ de:
         activated: Aktiviert
         consumer: Konsument
         consumer_id: Konsument
+        current_password: Aktuelles Passwort
         email: E-Mail
         name: Name
         password: Passwort
@@ -20,6 +21,12 @@ de:
       headline: Registrierung abschließen
       submit: Passwort speichern
       success: Sie haben Ihre Registrierung erfolgreich abgeschlossen.
+    change_password:
+      headline: Passwort ändern
+      hints:
+        current_password: Bitte geben Sie Ihr aktuelles Passwort ein, um Ihre Identität zu bestätigen.
+      submit: Passwort ändern
+      success: Sie haben Ihr Passwort erfolgreich geändert.
     forgot_password:
       headline: Passwort zurücksetzen
       submit: Anweisungen zum Zurücksetzen senden
@@ -29,11 +36,13 @@ de:
       study_groups: Lerngruppen
     index:
       activate: Aktivieren
+      change_password: Passwort ändern
       forgot_password: Passwort zurücksetzen
       reset_password: Passwort zurücksetzen
     reset_password:
       headline: Passwort zurücksetzen
       submit: Passwort speichern
       success: Sie haben Ihr Passwort erfolgreich geändert.
     show:
+      change_password: Passwort ändern
       link: Profil
diff --git a/config/locales/de/meta/activerecord.errors.yml b/config/locales/de/meta/activerecord.errors.yml
@@ -17,6 +17,7 @@ de:
           attributes:
             password:
               weak: ist zu schwach. Versuchen Sie es mit einem langen Passwort, welches Groß-, Kleinbuchstaben, Zahlen und Sonderzeichen enthält.
+          current_password_invalid: Das aktuelle Passwort ist nicht korrekt.
         programming_group:
           invalid_partner_id: Die Personen-ID '%{partner_id}' ist ungültig und wurde entfernt. Bitte überprüfen Sie die Personen-IDs der Programmierpartner:innen.
           size_too_large: Die Größe dieser Programmiergruppe ist zu groß. Geben Sie nur eine andere Personen-ID an.
diff --git a/config/locales/en/internal_user.yml b/config/locales/en/internal_user.yml
@@ -6,10 +6,11 @@ en:
         activated: Activated
         consumer: Consumer
         consumer_id: Consumer
+        current_password: Current Password
         email: Email
         name: Name
         password: Password
-        password_confirmation: Passwort Confirmation
+        password_confirmation: Password Confirmation
         platform_admin: Platform Admin
     models:
       internal_user:
@@ -20,6 +21,12 @@ en:
       headline: Complete Registration
       submit: Set Password
       success: You successfully completed your registration.
+    change_password:
+      headline: Change Password
+      hints:
+        current_password: Please enter your current password to confirm your identity.
+      submit: Change Password
+      success: You successfully changed your password.
     forgot_password:
       headline: Reset Password
       submit: Send Password Reset Instructions
@@ -29,11 +36,13 @@ en:
       study_groups: Study Groups
     index:
       activate: Activate
+      change_password: Change Password
       forgot_password: Reset Password
       reset_password: Reset Password
     reset_password:
       headline: Reset Password
       submit: Set Password
       success: You successfully changed your password.
     show:
+      change_password: Change Password
       link: Profile
diff --git a/config/locales/en/meta/activerecord.errors.yml b/config/locales/en/meta/activerecord.errors.yml
@@ -17,6 +17,7 @@ en:
           attributes:
             password:
               weak: is too weak. Try to use a long password with upper and lower case letters, numbers and special characters.
+          current_password_invalid: The current password is not correct.
         programming_group:
           invalid_partner_id: The user ID '%{partner_id}' is invalid and was removed. Please check the user IDs of your programming partners.
           size_too_large: The size of this programming group is too large. Enter at most one other user ID to work with.
diff --git a/config/routes.rb b/config/routes.rb
@@ -138,6 +138,7 @@
     member do
       match 'activate', to: 'internal_users#activate', via: %i[get patch put]
       match 'reset_password', to: 'internal_users#reset_password', via: %i[get patch put]
+      match 'change_password', to: 'internal_users#change_password', via: %i[get patch put]
     end
     resources :codeharbor_links, only: %i[new create edit update destroy]
   end
diff --git a/spec/controllers/internal_users_controller_spec.rb b/spec/controllers/internal_users_controller_spec.rb
@@ -106,6 +106,153 @@
     end
   end
 
+  describe 'GET #change_password' do
+    context 'without a valid session' do
+      before { get :change_password, params: {id: user.id} }
+
+      expect_redirect(:sign_in)
+    end
+
+    context 'with a valid session' do
+      before do
+        allow(controller).to receive(:current_user).and_return(user)
+        get :change_password, params: {id: user.id}
+      end
+
+      expect_assigns(user: :user)
+      expect_http_status(:ok)
+      expect_template(:change_password)
+    end
+
+    context 'with a valid session and a user that is not the current user' do
+      let(:second_user) { create(:teacher, consumer:) }
+
+      before do
+        allow(controller).to receive(:current_user).and_return(user)
+        get :change_password, params: {id: second_user.id}
+      end
+
+      context 'when the current user is not an admin' do
+        let(:user) { create(:teacher, consumer:) }
+
+        expect_flash_message(:alert, I18n.t('application.not_authorized'))
+        expect_redirect(:root)
+      end
+
+      context 'when the current user is an admin' do
+        expect_assigns(user: :second_user)
+        expect_http_status(:ok)
+        expect_template(:change_password)
+      end
+    end
+  end
+
+  describe 'PUT #change_password' do
+    before { allow(controller).to receive(:current_user).and_return(user) }
+
+    context 'without a valid session' do
+      before { put :reset_password, params: {id: user.id} }
+
+      expect_flash_message(:alert, I18n.t('application.not_authorized'))
+      expect_redirect(:root)
+    end
+
+    context 'with a valid session' do
+      before do
+        allow(controller).to receive(:current_user).and_return(user)
+        user.change_password!(current_password)
+      end
+
+      let(:password) { 'foo' }
+      let(:current_password) { '$tr0ngP4ssw0rD' }
+
+      context 'with a matching password confirmation' do
+        let(:perform_request) { proc { put :change_password, params: {internal_user: {password:, password_confirmation: password, current_password:}, id: user.id} } }
+
+        before { perform_request.call }
+
+        expect_assigns(user: :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(:change_password)
+        end
+
+        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_flash_message(:notice, I18n.t('internal_users.change_password.success'))
+          expect_redirect { user }
+        end
+      end
+
+      context 'without a matching password confirmation' do
+        before do
+          put :change_password, params: {internal_user: {password:, password_confirmation: '', current_password:}, id: user.id}
+        end
+
+        expect_assigns(user: :user)
+        expect_http_status(:ok)
+        expect_template(:change_password)
+      end
+
+      context 'without a valid current password' do
+        before do
+          put :change_password, params: {internal_user: {password:, password_confirmation: password, current_password: ''}, id: user.id}
+        end
+
+        expect_assigns(user: :user)
+        expect_http_status(:ok)
+        expect_template(:change_password)
+      end
+    end
+
+    context 'with a valid session and a user that is not the current user' do
+      let(:second_user) { create(:teacher, consumer:) }
+      let(:password) { SecureRandom.hex(128) }
+      let(:perform_request) { proc { put :change_password, params: {internal_user: {password:, password_confirmation: password}, id: second_user.id} } }
+
+      before do
+        allow(controller).to receive(:current_user).and_return(user)
+        perform_request.call
+      end
+
+      context 'when the current user is not an admin' do
+        let(:user) { create(:teacher, consumer:) }
+
+        expect_flash_message(:alert, I18n.t('application.not_authorized'))
+        expect_redirect(:root)
+      end
+
+      context 'when the current user is an admin' do
+        context 'when the password is strong' do
+          expect_assigns(user: :second_user)
+          expect_redirect { second_user }
+        end
+
+        context 'when the password is weak' do
+          let(:password) { 'foo' }
+
+          expect_assigns(user: :second_user)
+          expect_http_status(:ok)
+          expect_template(:change_password)
+        end
+      end
+    end
+  end
+
   describe 'POST #create' do
     before { allow(controller).to receive(:current_user).and_return(user) }
 

Original file line number	Diff line number	Diff line change
`@@ -30,3 +30,4 @@ h1`
`30`	`30`
`31`	`31`	`- if @user == current_user \|\| current_user.admin?`
`32`	`32`	`= row(label: 'codeharbor_link.profile_label', value: @user.codeharbor_link.nil? ? link_to(t('codeharbor_link.new'), polymorphic_path([@user, CodeharborLink], action: :new), class: 'btn btn-secondary') : link_to(t('codeharbor_link.edit'), polymorphic_path([@user, @user.codeharbor_link], action: :edit), class: 'btn btn-secondary'))`
	`33`	`+ = row(label: 'internal_user.password', value: link_to(t('.change_password'), change_password_internal_user_path(@user), class: 'btn btn-secondary'))`