Use project specific checking for custom field visibility in journals

app/controllers/journals_controller.rb

@@ -130,36 +130,28 @@
   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

app/models/custom_field.rb

@@ -91,6 +91,10 @@
 
   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)

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

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

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

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

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

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 }
 

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) }

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

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