Validate current user password confirmation when changing passwords through API

https://community.openproject.org/work_packages/74335

app/contracts/users/base_contract.rb

@@ -78,7 +78,16 @@ module Users
     private
 
     def password_writable?
-      user.admin? || user.id == model.id
+      return true if user.admin? && !editing_self?
+
+      editing_self? && current_password_valid?
+    end
+
+    def current_password_valid?
+      return true if model.password.blank?
+
+      provided_current_password = model.current_password_input
+      provided_current_password.present? && model.check_password?(provided_current_password)
     end
 
     def identity_url_writable?
@@ -90,10 +99,14 @@ module Users
     # but just an accessor, so we need to identify it being written there.
     # It is only present when freshly written
     def validate_password_writable
-      # Only admins or the user themselves can set the password
+      return if model.password.blank?
       return if password_writable?
 
-      errors.add :password, :error_readonly if model.password.present?
+      if editing_self?
+        errors.add :current_password, :invalid
+      else
+        errors.add :password, :error_readonly
+      end
     end
 
     def validate_identity_url_writable

app/models/user.rb

@@ -134,7 +134,7 @@ class User < Principal
 
   acts_as_customizable admin_only_allowed: true
 
-  attr_accessor :password, :password_confirmation, :last_before_login_on
+  attr_accessor :password, :password_confirmation, :last_before_login_on, :current_password_input
 
   validates :login,
             :firstname,

docs/api/apiv3/components/schemas/user_create_model.yml

@@ -20,11 +20,25 @@ properties:
   password:
     type: string
     description: |-
-      The users password.
+      The user's password.
 
       *Conditions:*
 
-      Only writable on creation, not on update.
+      Writable on create.
+
+      Writable on update only when:
+      - the caller updates their own account
+      - `currentPassword` is provided and valid
+  currentPassword:
+    type: string
+    description: |-
+      The user's current password.
+
+      *Conditions:*
+
+      Required when changing `password` for a self update (`PATCH /api/v3/users/me` or `PATCH /api/v3/users/{id}` where `id` is the caller).
+
+      Ignored for non-self updates (for example, administrators updating other users).
   firstName:
     type: string
     maxLength: 30
@@ -45,6 +59,7 @@ properties:
 example:
   login: j.sheppard
   password: idestroyedsouvereign
+  currentPassword: iusedtobuildstarships
   firstName: John
   lastName: Sheppard
   email: shep@mail.com

docs/api/apiv3/paths/user.yml

@@ -8,13 +8,13 @@ delete:
     - Users
     - Principals
   parameters:
-    - description: User id
+    - description: User id. Use `me` to reference current user, if any.
       example: 1
       in: path
       name: id
       required: true
       schema:
-        type: integer
+        type: string
   responses:
     '202':
       description: |-
@@ -94,14 +94,16 @@ patch:
   description: |-
     Updates the user's writable attributes.
     When calling this endpoint the client provides a single object, containing at least the properties and links that are required, in the body.
+
+    Password updates for self-service account changes require both `password` and `currentPassword`.
   parameters:
-    - description: User id
+    - description: User id. Use `me` to reference current user, if any.
       example: 1
       in: path
       name: id
       required: true
       schema:
-        type: integer
+        type: string
   requestBody:
     content:
       application/json:
@@ -151,13 +153,23 @@ patch:
         application/hal+json:
           schema:
             $ref: '../components/schemas/error_response.yml'
-          example:
-            _embedded:
-              details:
-                attribute: email
-            _type: Error
-            errorIdentifier: urn:openproject-org:api:v3:errors:PropertyConstraintViolation
-            message: The email address is already taken.
+          examples:
+            email constraint violation:
+              value:
+                _embedded:
+                  details:
+                    attribute: email
+                _type: Error
+                errorIdentifier: urn:openproject-org:api:v3:errors:PropertyConstraintViolation
+                message: The email address is already taken.
+            invalid current password:
+              value:
+                _embedded:
+                  details:
+                    attribute: currentPassword
+                _type: Error
+                errorIdentifier: urn:openproject-org:api:v3:errors:PropertyConstraintViolation
+                message: Current password is invalid.
       description: |-
         Returned if:
 

docs/api/apiv3/paths/user_form.yml

@@ -2,13 +2,13 @@
 ---
 post:
   parameters:
-  - description: User id
+  - description: User id. Use `me` to reference current user, if any.
     example: 1
     in: path
     name: id
     required: true
     schema:
-      type: integer
+      type: string
   responses:
     '200':
       description: OK
@@ -54,6 +54,9 @@ post:
       $ref: "../components/responses/unsupported_media_type.yml"
   tags:
   - Users
-  description: ''
+  description: |-
+    Validates a user update payload.
+
+    For self password changes, provide both `password` and `currentPassword`.
   operationId: User_update_form
   summary: User update form

lib/api/v3/users/user_representer.rb

@@ -185,6 +185,14 @@ module API
                    represented.password = represented.password_confirmation = fragment
                  }
 
+        property :current_password,
+                 as: :currentPassword,
+                 getter: ->(*) {},
+                 render_nil: false,
+                 setter: ->(fragment:, represented:, **) {
+                   represented.current_password_input = fragment
+                 }
+
         ##
         # Used while parsing JSON to initialize `ldap_auth_source_id` through the given link.
         def initialize_embedded_links!(data)

spec/contracts/users/update_contract_spec.rb

@@ -221,6 +221,34 @@ RSpec.describe Users::UpdateContract do
         it_behaves_like "contract is valid"
       end
 
+      describe "when changing the password" do
+        before do
+          user.password = "newpassword123!"
+          user.password_confirmation = "newpassword123!"
+        end
+
+        context "without current password" do
+          it_behaves_like "contract is invalid", current_password: :invalid
+        end
+
+        context "with wrong current password" do
+          before do
+            user.current_password_input = "wrong-password"
+          end
+
+          it_behaves_like "contract is invalid", current_password: :invalid
+        end
+
+        context "with valid current password" do
+          before do
+            user.current_password_input = "adminADMIN!"
+            allow(user).to receive(:check_password?).with("adminADMIN!").and_return(true)
+          end
+
+          it_behaves_like "contract is valid"
+        end
+      end
+
       context "when updated user authenticates through LDAP and basic attributes are changed" do
         let(:attributes) { super().merge(ldap_auth_source_id: create(:ldap_auth_source).id) }
 

spec/features/users/my_spec.rb

@@ -56,7 +56,9 @@ RSpec.describe "my", :js do
   end
 
   before do
-    login_as user
+    # Use a fresh AR instance to avoid leaking virtual attributes (e.g. password accessors)
+    # between examples into RequestStore.current_user.
+    login_as User.find(user.id)
 
     # Create dangling session
     session = Sessions::SqlBypass.new data: { user_id: user.id }, session_id: "other"
@@ -82,6 +84,7 @@ RSpec.describe "my", :js do
 
           expect_and_dismiss_flash type: :success, message: "Account was successfully updated."
 
+          user.reload
           expect(page).to have_select "Time zone", selected: "(UTC+01:00) Paris"
           expect(user.pref.time_zone).to eq "Europe/Paris"
         end
@@ -97,6 +100,7 @@ RSpec.describe "my", :js do
 
         expect_and_dismiss_flash type: :success, message: "Cuenta se actualizó correctamente."
 
+        user.reload
         expect(page).to have_select "Idioma", selected: "Español"
         expect(user.language).to eq "es"
       end
@@ -118,7 +122,6 @@ RSpec.describe "my", :js do
         end
 
         expect(page).to have_heading "Configurações de notificação"
-        expect(page).to have_heading "Alertas de data"
       end
     end
   end

spec/requests/api/v3/user/update_user_resource_spec.rb

@@ -366,4 +366,56 @@ RSpec.describe API::V3::Users::UsersAPI do
       expect(last_response).to have_http_status(:not_found)
     end
   end
+
+  describe "self update via /users/me" do
+    let(:current_user) { create(:user) }
+    let!(:user) { current_user }
+    let(:path) { api_v3_paths.user("me") }
+
+    describe "password update without current password" do
+      let(:parameters) { { password: "my!new!password123" } }
+
+      it "rejects the update and keeps the old password" do
+        send_request
+
+        expect(last_response).to have_http_status(:unprocessable_entity)
+        expect(current_user.reload.check_password?("adminADMIN!")).to be(true)
+        expect(current_user.check_password?("my!new!password123")).to be(false)
+      end
+    end
+
+    describe "password update with wrong current password" do
+      let(:parameters) do
+        {
+          password: "my!new!password123",
+          currentPassword: "wrong-password"
+        }
+      end
+
+      it "rejects the update and keeps the old password" do
+        send_request
+
+        expect(last_response).to have_http_status(:unprocessable_entity)
+        expect(current_user.reload.check_password?("adminADMIN!")).to be(true)
+        expect(current_user.check_password?("my!new!password123")).to be(false)
+      end
+    end
+
+    describe "password update with valid current password" do
+      let(:parameters) do
+        {
+          password: "my!new!password123",
+          currentPassword: "adminADMIN!"
+        }
+      end
+
+      it "updates the users password correctly" do
+        send_request
+        expect(last_response).to have_http_status(:ok)
+
+        updated_user = User.find(current_user.id)
+        expect(updated_user.check_password?("my!new!password123")).to be(true)
+      end
+    end
+  end
 end

spec/services/users/set_attributes_service_integration_spec.rb

@@ -31,7 +31,7 @@
 require "spec_helper"
 
 RSpec.describe Users::SetAttributesService, "Integration", type: :model do
-  shared_let(:input_user) { create(:user) }
+  let(:input_user) { create(:user) }
   let(:actor) { build_stubbed(:admin) }
 
   let(:instance) do