Merge remote-tracking branch 'origin/release/17.3' into release/17.4

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/controllers/account_controller.rb

@@ -356,10 +356,9 @@ class AccountController < ApplicationController
   end
 
   def direct_login(user)
-    if flash.empty?
-      ps = {}.tap do |p|
-        p[:origin] = params[:back_url] if params[:back_url]
-      end
+    if !flash_message_pending?
+      ps = {}
+      ps[:origin] = params[:back_url] if params[:back_url]
 
       redirect_to direct_login_provider_url(ps)
     elsif Setting.login_required?
@@ -447,6 +446,12 @@ class AccountController < ApplicationController
     redirect_to signin_path
   end
 
+  def flash_message_pending?
+    # confirm that the flash contains a message to be rendered, but ignore
+    # other flash content (such as _csp_appends)
+    flash.keys.map(&:to_s).intersect?(%w[notice warning error])
+  end
+
   def apply_csp_appends
     appends = flash[:_csp_appends]
     return unless appends

app/models/members/scopes/visible.rb

@@ -45,12 +45,28 @@ module Members::Scopes
       private
 
       def visible_for_non_admins(user)
+        visible_member_ids = unscoped do
+          visible_project_members(user).or(shared_work_package_members(user)).select(:id)
+        end
+
+        where(id: visible_member_ids)
+      end
+
+      # Project-wide memberships in projects where the user may view or manage members.
+      def visible_project_members(user)
         view_members = Project.allowed_to(user, :view_members)
         manage_members = Project.allowed_to(user, :manage_members)
-        view_work_packages = Project.allowed_to(user, :view_shared_work_packages)
 
         where(project_id: view_members.or(manage_members).select(:id), entity_type: nil)
-          .or(where(project_id: view_work_packages.select(:id), entity_type: WorkPackage.name))
+      end
+
+      # Work package shares the user may list, limited to entities they can view.
+      def shared_work_package_members(user)
+        view_shared_work_packages_projects = Project.allowed_to(user, :view_shared_work_packages)
+        visible_shared_work_package_ids = WorkPackage.visible(user).select(:id)
+
+        where(project_id: view_shared_work_packages_projects.select(:id), entity_type: WorkPackage.name)
+          .where(entity_id: visible_shared_work_package_ids)
       end
 
       def visible_for_admins

app/models/user.rb

@@ -140,7 +140,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

docs/system-admin-guide/authentication/saml/README.md

@@ -313,6 +313,10 @@ Using environment variables, you could also set this in the following way
 OPENPROJECT_OMNIAUTH__DIRECT__LOGIN__PROVIDER="saml" # This value should be the 'name' property of your configuration
 ```
 
+With the direct login feature activated, accessing the page without authentication will immediately redirect the user to your Single Sign-On (SSO) portal.
+
+A dedicated route `/login/internal` is available for internal authentication, which does not redirect to the SSO portal.
+**We strongly advise** you to maintain an internal administrative login, as you won’t be able to access the application otherwise.
 
 
 ## Instructions for common SAML providers

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)

modules/reporting/app/controllers/cost_reports_controller.rb

@@ -164,6 +164,8 @@ class CostReportsController < ApplicationController
   # specified record or renders the updated table on XHR
   def update
     if save_query? # save
+      raise ActiveRecord::RecordNotFound unless @query
+
       return deny_access unless allowed_in_report?(:save, @query)
 
       old_query = @query
@@ -199,6 +201,8 @@ class CostReportsController < ApplicationController
   # Rename a record and update its publicity. Redirects to the updated record or
   # renders the updated name on XHR
   def rename
+    raise ActiveRecord::RecordNotFound unless @query
+
     return deny_access unless allowed_in_report?(:rename, @query)
 
     @query.name = params[:query_name]

modules/reporting/spec/controllers/cost_reports_controller_spec.rb

@@ -105,68 +105,66 @@ RSpec.describe CostReportsController do
       end
     end
   end
-
   describe "POST rename" do
-    let(:owner) { build(:user) }
-    let(:cost_query) { create(:public_cost_query, user: owner, project:, name: "Original name") }
+    let(:user) { create(:user) }
+    let(:owner) { create(:user) }
+    let(:cost_query) { create(:public_cost_query, user: owner, project:, name: "Public report") }
 
-    context "with only :view_cost_entries permission" do
+    context "when only save_private_cost_reports is granted" do
       before do
-        is_member project, user, [:view_cost_entries]
+        is_member project, user, %i[view_cost_entries save_private_cost_reports]
+        post :rename, params: { id: cost_query.id, project_id: project.identifier, query_name: "HACKED" }
       end
 
-      it "returns 403 Forbidden" do
-        post :rename, params: { id: cost_query.id, project_id: project.identifier, query_name: "my update" }
-
+      it "returns forbidden" do
         expect(response).to have_http_status(:forbidden)
       end
 
       it "does not rename the report" do
-        post :rename, params: { id: cost_query.id, project_id: project.identifier, query_name: "my update" }
-
-        expect(cost_query.reload.name).to eq("Original name")
+        expect(cost_query.reload.name).to eq("Public report")
       end
     end
 
-    context "with :save_cost_reports permission" do
+    context "when save_cost_reports is granted" do
       before do
         is_member project, user, %i[view_cost_entries save_cost_reports]
+        post :rename, params: { id: cost_query.id, project_id: project.identifier, query_name: "Renamed report" }
       end
 
       it "renames the report" do
-        post :rename, params: { id: cost_query.id, project_id: project.identifier, query_name: "my update" }
+        expect(cost_query.reload.name).to eq("Renamed report")
+      end
 
-        expect(response).to have_http_status(:redirect)
-        expect(cost_query.reload.name).to eq("my update")
+      it "redirects to show" do
+        expect(response).to redirect_to(action: "show", id: cost_query.id)
       end
     end
   end
 
   describe "POST update" do
-    let(:owner) { build(:user) }
+    let(:user) { create(:user) }
+    let(:owner) { create(:user) }
     let(:cost_query) { create(:public_cost_query, user: owner, project:) }
 
-    context "with only :view_cost_entries permission" do
-      before do
-        is_member project, user, [:view_cost_entries]
-      end
+    context "when only save_private_cost_reports is granted" do
+      it "returns forbidden and leaves query unchanged" do
+        is_member project, user, %i[view_cost_entries save_private_cost_reports]
+        serialized = cost_query.reload.serialized.deep_dup
 
-      it "returns 403 Forbidden" do
-        post :update, params: { id: cost_query.id, project_id: project.identifier, save_query: 1, set_filter: 1 }
+        post :update, params: { id: cost_query.id, project_id: project.identifier, save_query: 1 }
 
         expect(response).to have_http_status(:forbidden)
+        expect(cost_query.reload.serialized).to eq(serialized)
       end
     end
 
-    context "with :save_cost_reports permission" do
-      before do
+    context "when save_cost_reports is granted" do
+      it "updates and persists the query" do
         is_member project, user, %i[view_cost_entries save_cost_reports]
-      end
 
-      it "updates the report and redirects to show" do
-        post :update, params: { id: cost_query.id, project_id: project.identifier, save_query: 1, set_filter: 1 }
-
-        expect(response).to redirect_to(action: "show", id: cost_query.id)
+        expect do
+          post :update, params: { id: cost_query.id, project_id: project.identifier, save_query: 1 }
+        end.to change { cost_query.reload.serialized }
       end
     end
   end

spec/contracts/users/update_contract_spec.rb

@@ -245,6 +245,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/auth/omniauth_spec.rb

@@ -90,7 +90,7 @@ RSpec.describe "Omniauth authentication" do
     end
 
     context "with direct login",
-            with_config: { omniauth_direct_login_provider: "developer" } do
+            with_settings: { omniauth_direct_login_provider: "developer" } do
       it "goes directly to the developer sign in and then redirect to the back url" do
         visit my_account_path
         # requires login, redirects to developer login which is why we see the login form now
@@ -110,7 +110,7 @@ RSpec.describe "Omniauth authentication" do
   end
 
   describe "sign out a user with direct login and login required",
-           with_config: { omniauth_direct_login_provider: "developer", login_required: true } do
+           with_settings: { omniauth_direct_login_provider: "developer", login_required: true } do
     it "shows a notice that the user has been logged out" do
       visit signout_path
 
@@ -217,14 +217,30 @@ RSpec.describe "Omniauth authentication" do
     end
 
     context "with direct login enabled and login required",
-            with_config: { omniauth_direct_login_provider: "developer" } do
-      before do
-        allow(Setting).to receive(:login_required?).and_return(true)
-      end
-
+            with_settings: { omniauth_direct_login_provider: "developer", login_required: true } do
       it_behaves_like "registration with registration by email" do
         let(:login_path) { "/auth/developer" }
       end
+
+      context "when authorizing an external OAuth app" do
+        let(:oauth_client) { create(:oauth_application, redirect_uri: "https://rp.example.com/callback") }
+
+        it "logs in and registers successfully" do
+          visit oauth_authorization_path(
+            client_id: oauth_client.uid,
+            redirect_uri: oauth_client.redirect_uri,
+            response_type: "code",
+            prompt: "login"
+          )
+
+          SeleniumHubWaiter.wait
+          fill_in "email", with: user.mail # login form developer strategy
+
+          click_link_or_button "Sign In"
+
+          expect(page).to have_current_path(oauth_authorization_path, ignore_query: true)
+        end
+      end
     end
   end
 
@@ -255,11 +271,7 @@ RSpec.describe "Omniauth authentication" do
     end
 
     context "with direct login and login required",
-            with_config: { omniauth_direct_login_provider: "developer" } do
-      before do
-        allow(Setting).to receive(:login_required?).and_return(true)
-      end
-
+            with_settings: { omniauth_direct_login_provider: "developer", login_required: true } do
       it_behaves_like "omniauth signin error" do
         let(:login_path) { signin_path }
         let(:instructions) { "You can try to sign in again by clicking" }

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

spec/models/members/scopes/visible_spec.rb

@@ -98,6 +98,26 @@ RSpec.describe Members::Scopes::Visible do
                                            manage_members_member,
                                            view_shared_work_packages_work_package_share
       end
+
+      context "when another work package is shared with a different user" do
+        let(:other_principal) { create(:user) }
+        let(:inaccessible_work_package) { create(:work_package, project: view_shared_work_packages_project) }
+        let!(:share_for_other_principal) do
+          create(:member,
+                 project: view_shared_work_packages_project,
+                 entity: inaccessible_work_package,
+                 principal: other_principal,
+                 roles: [create(:edit_work_package_role)])
+        end
+
+        it "does not display shares for work packages the user cannot view" do
+          expect(subject).not_to include(share_for_other_principal)
+        end
+
+        it "still includes shares on work packages visible to the user" do
+          expect(subject).to include(view_shared_work_packages_work_package_share)
+        end
+      end
     end
   end
 end

spec/models/queries/members/members_query_integration_spec.rb

@@ -58,7 +58,7 @@ RSpec.describe Queries::Members::MemberQuery, "Integration" do
     let(:work_package) { create(:work_package, project:) }
     let(:user) { create(:user) }
     let(:role) { create(:project_role, permissions: %i[manage_members view_shared_work_packages]) }
-    let(:wp_role) { create(:work_package_role) }
+    let(:wp_role) { create(:view_work_package_role) }
     let!(:project_membership) { create(:member, principal: user, project:, roles: [role]) }
     let!(:wp_membership) { create(:member, principal: user, project:, entity: work_package, roles: [wp_role]) }
 

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

@@ -382,4 +382,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