From fea9ecf623d9d966316053216a47ca5b169c0fe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 18 May 2026 13:30:54 +0200 Subject: [PATCH] Use project specific checking for custom field visibility in journals --- app/controllers/journals_controller.rb | 38 ++++++++----------- app/models/custom_field.rb | 4 ++ app/models/project_custom_field.rb | 4 ++ .../scopes/visible.rb | 2 +- app/models/work_package_custom_field.rb | 4 ++ .../scopes/on_visible_type_and_project.rb | 10 ++++- .../scopes/visible.rb | 4 +- .../scopes/visible_spec.rb | 24 ++++++++++++ .../on_visible_type_and_project_spec.rb | 16 ++++++++ 9 files changed, 78 insertions(+), 28 deletions(-) diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb index ff2da2d45fc..fa5f1f3f67d 100644 --- a/app/controllers/journals_controller.rb +++ b/app/controllers/journals_controller.rb @@ -130,36 +130,28 @@ class JournalsController < ApplicationController end def ensure_custom_value_valid_for_diffing(cf_id) - custom_field = CustomField.select(:field_format, :admin_only).find_by(id: cf_id) + custom_field = CustomField.find_by(id: cf_id) + return render_403 if deleted_cf_forbidden?(custom_field) + return if custom_field.nil? - if !allowed_to_view_custom_field_changes?(custom_field) - render_403 - elsif custom_field && custom_field.field_format != "text" - render_404 - end + return render_403 unless custom_field.visible?(User.current, project: @journable.project) + render_404 if custom_field.field_format != "text" end def ensure_custom_comment_valid_for_diffing(cf_id) - custom_field = CustomField.select(:admin_only).find_by(id: cf_id) + custom_field = CustomField.find_by(id: cf_id) + return render_403 if deleted_cf_forbidden?(custom_field) + return if custom_field.nil? - if !allowed_to_view_custom_field_changes?(custom_field) - render_403 - end + render_403 unless custom_field.visible?(User.current, project: @journable.project) end - def allowed_to_view_custom_field_changes?(custom_field) - return true if User.current.admin? - - if @journable.is_a?(Project) && !User.current.allowed_in_project?(:view_project_attributes, @journable) - return false - end - - if @journable.admin_only_custom_fields_allowed? - # don't reveal changes of deleted custom fields if those could have admin_only mark - custom_field && !custom_field.admin_only - else - true - end + # When a CF is deleted we can no longer call visible? on it. For journables that support + # admin-only CFs (e.g. Project), block non-admins because the deleted CF could have been admin-only. + def deleted_cf_forbidden?(custom_field) + custom_field.nil? && + @journable.admin_only_custom_fields_allowed? && + !User.current.admin? end def journals_index_title diff --git a/app/models/custom_field.rb b/app/models/custom_field.rb index 8bc7898fddc..f272bd52cb9 100644 --- a/app/models/custom_field.rb +++ b/app/models/custom_field.rb @@ -91,6 +91,10 @@ class CustomField < ApplicationRecord after_destroy :destroy_help_text + def visible?(usr = User.current, project: nil) + self.class.visible(usr).exists?(id: id) + end + # make sure int, float, date, and bool are not searchable def check_searchability self.searchable = false if %w(int float date bool user version).include?(field_format) diff --git a/app/models/project_custom_field.rb b/app/models/project_custom_field.rb index 10c3aadcb33..be8e38c8117 100644 --- a/app/models/project_custom_field.rb +++ b/app/models/project_custom_field.rb @@ -126,6 +126,10 @@ class ProjectCustomField < CustomField end end + def visible?(usr = User.current, project: nil) + self.class.visible(usr, project:).exists?(id: id) + end + def type_name :label_project_plural end diff --git a/app/models/time_entry_custom_fields/scopes/visible.rb b/app/models/time_entry_custom_fields/scopes/visible.rb index 714b84758c3..c929334e848 100644 --- a/app/models/time_entry_custom_fields/scopes/visible.rb +++ b/app/models/time_entry_custom_fields/scopes/visible.rb @@ -33,7 +33,7 @@ module TimeEntryCustomFields::Scopes extend ActiveSupport::Concern class_methods do - def visible(_user = User.current) + def visible(_user = User.current, **) all end end diff --git a/app/models/work_package_custom_field.rb b/app/models/work_package_custom_field.rb index e598c413822..a021fc7084e 100644 --- a/app/models/work_package_custom_field.rb +++ b/app/models/work_package_custom_field.rb @@ -56,6 +56,10 @@ class WorkPackageCustomField < CustomField %w[int float].include?(field_format) end + def visible?(usr = User.current, project: nil) + self.class.visible(usr, project:).exists?(id: id) + end + def type_name :label_work_package_plural end diff --git a/app/models/work_package_custom_fields/scopes/on_visible_type_and_project.rb b/app/models/work_package_custom_fields/scopes/on_visible_type_and_project.rb index 5d72cce6dd1..fcba1d50ab3 100644 --- a/app/models/work_package_custom_fields/scopes/on_visible_type_and_project.rb +++ b/app/models/work_package_custom_fields/scopes/on_visible_type_and_project.rb @@ -39,11 +39,17 @@ module WorkPackageCustomFields::Scopes # * on a type which in turn is active in a project the user has access to # * on a project the user has access to # Both conditions need to be met on the same project. - def on_visible_type_and_project(user = User.current) + # + # Pass +project:+ to restrict the check to a single known project instead of + # scanning all projects visible to the user. + def on_visible_type_and_project(user = User.current, project: nil) + visible_projects = Project.visible(user) + visible_projects = visible_projects.where(id: project.id) if project&.persisted? + where(<<~SQL.squish) EXISTS ( SELECT 1 - FROM (#{Project.visible(user).select(:id).to_sql}) vp + FROM (#{visible_projects.select(:id).to_sql}) vp JOIN projects_types pt ON pt.project_id = vp.id JOIN custom_fields_types cft diff --git a/app/models/work_package_custom_fields/scopes/visible.rb b/app/models/work_package_custom_fields/scopes/visible.rb index 34e7fd29ada..37f47f5ee30 100644 --- a/app/models/work_package_custom_fields/scopes/visible.rb +++ b/app/models/work_package_custom_fields/scopes/visible.rb @@ -33,11 +33,11 @@ module WorkPackageCustomFields::Scopes extend ActiveSupport::Concern class_methods do - def visible(user = User.current) + def visible(user = User.current, project: nil) if user.allowed_in_any_project?(:select_custom_fields) all else - on_visible_type_and_project(user) + on_visible_type_and_project(user, project:) end end end diff --git a/spec/models/project_custom_fields/scopes/visible_spec.rb b/spec/models/project_custom_fields/scopes/visible_spec.rb index b854bdfecfb..426d227bfa1 100644 --- a/spec/models/project_custom_fields/scopes/visible_spec.rb +++ b/spec/models/project_custom_fields/scopes/visible_spec.rb @@ -106,4 +106,28 @@ RSpec.describe ProjectCustomFields::Scopes::Visible do end end end + + describe ".visible with project:" do + context "when user has view_project_attributes in one project but project: is a different project" do + let(:role) { create(:project_role, permissions: %i(view_project_attributes)) } + let(:current_user) { create(:user, member_with_roles: { project => role }) } + + subject { ProjectCustomField.visible(current_user, project: other_project) } + + it "returns nothing (CF not active in the specified project)" do + expect(subject).to be_empty + end + end + + context "when user has view_project_attributes and project: matches their project" do + let(:role) { create(:project_role, permissions: %i(view_project_attributes)) } + let(:current_user) { create(:user, member_with_roles: { project => role }) } + + subject { ProjectCustomField.visible(current_user, project:) } + + it "returns only CFs active in that project" do + expect(subject).to contain_exactly(project_cf) + end + end + end end diff --git a/spec/models/work_package_custom_fields/scopes/on_visible_type_and_project_spec.rb b/spec/models/work_package_custom_fields/scopes/on_visible_type_and_project_spec.rb index e5ab29c22d4..de0fab5f5c8 100644 --- a/spec/models/work_package_custom_fields/scopes/on_visible_type_and_project_spec.rb +++ b/spec/models/work_package_custom_fields/scopes/on_visible_type_and_project_spec.rb @@ -40,5 +40,21 @@ RSpec.describe WorkPackageCustomFields::Scopes::OnVisibleTypeAndProject do it "returns custom fields for types that are enabled in projects the user can see" do expect(subject).to contain_exactly(type_enabled_and_member_cf, type_enabled_for_all_cf) end + + context "with project: provided" do + subject { WorkPackageCustomField.on_visible_type_and_project(user, project: project_with_user_and_feature) } + + it "returns only fields enabled in the given project" do + expect(subject).to contain_exactly(type_enabled_and_member_cf, type_enabled_for_all_cf) + end + + context "when the project has a different type than where the CF is active" do + subject { WorkPackageCustomField.on_visible_type_and_project(user, project: project_with_user_and_bug) } + + it "returns nothing" do + expect(subject).to be_empty + end + end + end end end