Use project specific checking for custom field visibility in journals

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

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)

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

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

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

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

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

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

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