From 81a94696d030ea7df59f2b311b80b3195ea87833 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 1/2] 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 ++ 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 ++++++++ 8 files changed, 77 insertions(+), 27 deletions(-) diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb index ff2da2d45fc0..fa5f1f3f67d5 100644 --- a/app/controllers/journals_controller.rb +++ b/app/controllers/journals_controller.rb @@ -130,36 +130,28 @@ def ensure_valid_for_diffing 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 8bc7898fddc1..f272bd52cb97 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 10c3aadcb33f..be8e38c81173 100644 --- a/app/models/project_custom_field.rb +++ b/app/models/project_custom_field.rb @@ -126,6 +126,10 @@ def mappings_with_view_project_attributes_permission(user, project) # rubocop:di 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/work_package_custom_field.rb b/app/models/work_package_custom_field.rb index e598c4138221..a021fc7084ed 100644 --- a/app/models/work_package_custom_field.rb +++ b/app/models/work_package_custom_field.rb @@ -56,6 +56,10 @@ def summable? %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 5d72cce6dd12..fcba1d50ab36 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 OnVisibleTypeAndProject # * 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 34e7fd29adab..37f47f5ee30a 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 Visible 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 b854bdfecfbb..426d227bfa1d 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 @@ 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 e5ab29c22d4f..de0fab5f5c88 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 @@ 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 From 491f5d36a4856cdbd6e61ad63bb5111d101b367f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 18 May 2026 20:42:36 +0200 Subject: [PATCH 2/2] Also use visible scope for activities view --- .../lib/journal_changes.rb | 13 +-- .../project_attributes_activity_spec.rb | 50 +++++++++++- spec/models/journal_spec.rb | 81 +++++++++++++++++++ 3 files changed, 134 insertions(+), 10 deletions(-) diff --git a/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb b/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb index 2105a569e3ff..9cbcce7eaf79 100644 --- a/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb +++ b/lib_static/plugins/acts_as_journalized/lib/journal_changes.rb @@ -75,7 +75,7 @@ def get_attachments_changes def get_custom_comments_changes return unless journable.respond_to?(:custom_comments) - association = ->(journal) { filter_admin_only_custom_fields(journal.custom_comment_journals) } + association = ->(journal) { filter_invisible_custom_fields(journal.custom_comment_journals) } ::Acts::Journalized::Differ::Association.new( predecessor, @@ -97,7 +97,7 @@ def get_custom_fields_changes relation = relation.with(cf_mappings: journable.project_custom_field_project_mappings) .joins("INNER JOIN cf_mappings USING (custom_field_id)") end - filter_admin_only_custom_fields(relation) + filter_invisible_custom_fields(relation) } ::Acts::Journalized::Differ::Association.new( @@ -152,10 +152,11 @@ def get_agenda_items_changes private - def filter_admin_only_custom_fields(relation) - return relation if User.current.admin? - return relation unless journable.admin_only_custom_fields_allowed? + def filter_invisible_custom_fields(relation) + cf_class = journable.class.custom_field_class + return relation if cf_class.nil? - relation.joins(:custom_field).where(custom_fields: { admin_only: false }) + project = journable.is_a?(::Project) ? journable : journable.try(:project) + relation.where(custom_field_id: cf_class.visible(User.current, project:).select(:id)) end end diff --git a/spec/features/activities/project_attributes_activity_spec.rb b/spec/features/activities/project_attributes_activity_spec.rb index 0d4508ef43b5..c85e51d067c9 100644 --- a/spec/features/activities/project_attributes_activity_spec.rb +++ b/spec/features/activities/project_attributes_activity_spec.rb @@ -33,8 +33,8 @@ RSpec.describe "Project attributes activity", :js do let(:user) do create(:user, member_with_permissions: { - project => %i[view_work_packages edit_work_packages], - project2 => %i[view_work_packages edit_work_packages] + project => %i[view_work_packages edit_work_packages view_project_attributes], + project2 => %i[view_work_packages edit_work_packages view_project_attributes] }) end let(:parent_project) { create(:project, name: "parent") } @@ -122,8 +122,10 @@ def generate_trackable_activity_on_project(project) # custom fields activity_page.expect_activity("#{list_project_custom_field.name} " \ "set to #{project.send(list_project_custom_field.attribute_getter)}") - activity_page.expect_activity("#{version_project_custom_field.name} " \ - "set to #{old_version[project].name}, #{next_version[project].name}") + sorted_version_names = [old_version[project], next_version[project]] + .sort_by { |v| v.id.to_s } + .map(&:name).join(", ") + activity_page.expect_activity("#{version_project_custom_field.name} set to #{sorted_version_names}") activity_page.expect_activity("#{bool_project_custom_field.name} set to Yes") activity_page.expect_activity("#{user_project_custom_field.name} set to #{current_user.name}") activity_page.expect_activity("#{int_project_custom_field.name} set to 42") @@ -135,6 +137,46 @@ def generate_trackable_activity_on_project(project) end end + describe "custom field visibility enforcement on the project activity page" do + let!(:string_cf) { create(:string_project_custom_field) } + let(:cf_project) { create(:project) } + let(:activity_page) { Pages::Projects::Activity.new(cf_project) } + + let(:user_without_cf_permission) do + create(:user, member_with_permissions: { cf_project => %i[view_work_packages] }) + end + let(:user_with_cf_permission) do + create(:user, member_with_permissions: { cf_project => %i[view_work_packages view_project_attributes] }) + end + + before do + cf_project.update!(custom_field_values: { "#{string_cf.id}": "initial" }) + cf_project.update!(custom_field_values: { "#{string_cf.id}": "changed" }) + end + + context "when the user lacks view_project_attributes" do + current_user { user_without_cf_permission } + + it "does not show the custom field change in the activity feed" do + activity_page.visit! + activity_page.show_details + + expect(page).to have_no_text(string_cf.name) + end + end + + context "when the user has view_project_attributes" do + current_user { user_with_cf_permission } + + it "shows the custom field change in the activity feed" do + activity_page.visit! + activity_page.show_details + + expect(page).to have_text(string_cf.name) + end + end + end + describe "the general activities page" do let(:activity_page) { Pages::Activity.new } diff --git a/spec/models/journal_spec.rb b/spec/models/journal_spec.rb index df68cb34bcf0..f9e852b99393 100644 --- a/spec/models/journal_spec.rb +++ b/spec/models/journal_spec.rb @@ -146,6 +146,87 @@ end end + describe "#details custom field visibility" do + context "for a project journal" do + shared_let(:project) { create(:project) } + shared_let(:project_cf) { create(:string_project_custom_field) } + + before_all do + project.update!(custom_field_values: { "#{project_cf.id}": "initial" }) + project.update!(custom_field_values: { "#{project_cf.id}": "changed" }) + end + + let(:journal) { project.journals.last } + let(:cf_key) { "custom_fields_#{project_cf.id}" } + + context "when the user lacks view_project_attributes" do + let(:user) { create(:user, member_with_permissions: { project => [] }) } + + before { login_as(user) } + + it "omits the CF change from details" do + expect(journal.details.keys).not_to include(cf_key) + end + end + + context "when the user has view_project_attributes" do + let(:user) { create(:user, member_with_permissions: { project => [:view_project_attributes] }) } + + before { login_as(user) } + + it "includes the CF change in details" do + expect(journal.details.keys).to include(cf_key) + end + end + + context "when the user is an admin" do + let(:user) { create(:admin) } + + before { login_as(user) } + + it "includes the CF change in details" do + expect(journal.details.keys).to include(cf_key) + end + end + end + + context "for a work package journal" do + shared_let(:type) { create(:type) } + shared_let(:project) { create(:project, types: [type]) } + shared_let(:wp_cf) { create(:string_wp_custom_field, types: [type], projects: [project]) } + shared_let(:work_package) do + create(:work_package, project:, type:, custom_field_values: { "#{wp_cf.id}": "initial" }) + end + + before_all do + work_package.update!(custom_field_values: { "#{wp_cf.id}": "changed" }) + end + + let(:journal) { work_package.journals.last } + let(:cf_key) { "custom_fields_#{wp_cf.id}" } + + context "when the user is a project member (CF visible via type/project)" do + let(:user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) } + + before { login_as(user) } + + it "includes the CF change in details" do + expect(journal.details.keys).to include(cf_key) + end + end + + context "when the user is not a member of the project (CF not visible)" do + let(:user) { create(:user) } + + before { login_as(user) } + + it "omits the CF change from details" do + expect(journal.details.keys).not_to include(cf_key) + end + end + end + end + describe "#create" do context "without a data foreign key" do subject { create(:work_package_journal, data: nil) }