diff --git a/app/contracts/users/base_contract.rb b/app/contracts/users/base_contract.rb index 2b1580e5808..5c04d1e1588 100644 --- a/app/contracts/users/base_contract.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 6f2f75a890a..d1c68d85a2f 100644 --- a/app/models/user.rb +++ b/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, diff --git a/docs/api/apiv3/components/schemas/user_create_model.yml b/docs/api/apiv3/components/schemas/user_create_model.yml index ea7b8fd9b62..292cf37b04d 100644 --- a/docs/api/apiv3/components/schemas/user_create_model.yml +++ b/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 diff --git a/docs/api/apiv3/paths/user.yml b/docs/api/apiv3/paths/user.yml index f6699d2a8f4..62d3ea02560 100644 --- a/docs/api/apiv3/paths/user.yml +++ b/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: diff --git a/docs/api/apiv3/paths/user_form.yml b/docs/api/apiv3/paths/user_form.yml index 005034e1ed8..ce1481694ec 100644 --- a/docs/api/apiv3/paths/user_form.yml +++ b/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 diff --git a/lib/api/v3/users/user_representer.rb b/lib/api/v3/users/user_representer.rb index 141b1fb9097..70a1c02e1c0 100644 --- a/lib/api/v3/users/user_representer.rb +++ b/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) diff --git a/spec/contracts/users/update_contract_spec.rb b/spec/contracts/users/update_contract_spec.rb index 033d4fc6675..4482582cb47 100644 --- a/spec/contracts/users/update_contract_spec.rb +++ b/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) } diff --git a/spec/features/users/my_spec.rb b/spec/features/users/my_spec.rb index 7825ef6e11f..270c60c8ac1 100644 --- a/spec/features/users/my_spec.rb +++ b/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 diff --git a/spec/requests/api/v3/user/update_user_resource_spec.rb b/spec/requests/api/v3/user/update_user_resource_spec.rb index b609e554287..4558d94ce1a 100644 --- a/spec/requests/api/v3/user/update_user_resource_spec.rb +++ b/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 diff --git a/spec/services/users/set_attributes_service_integration_spec.rb b/spec/services/users/set_attributes_service_integration_spec.rb index 734aed0fcd4..10b47188c55 100644 --- a/spec/services/users/set_attributes_service_integration_spec.rb +++ b/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