Merge pull request #11039 from demarches-simplifiees/work_on_enum_filter

ETQ Instructeur: corrections sur les filtres des champs choix multiples et liste liée

app/models/columns/champ_column.rb

@@ -32,15 +32,16 @@ class Columns::ChampColumn < Column
   end
 
   def filtered_ids(dossiers, search_terms)
+    relation = dossiers.with_type_de_champ(stable_id)
+
     if type == :enum
-      dossiers.with_type_de_champ(stable_id)
-        .filter_enum(:champs, column, search_terms).ids
+      relation.where(champs: { column => search_terms }).ids
     elsif type == :enums
-      dossiers.with_type_de_champ(stable_id)
-        .filter_array_enum(:champs, column, search_terms).ids
+      # in a multiple drop down list, the value are stored as '["v1", "v2"]'
+      quoted_search_terms = search_terms.map { %{"#{_1}"} }
+      relation.filter_ilike(:champs, column, quoted_search_terms).ids
     else
-      dossiers.with_type_de_champ(stable_id)
-        .filter_ilike(:champs, column, search_terms).ids
+      relation.filter_ilike(:champs, column, search_terms).ids
     end
   end
 

app/models/columns/linked_drop_down_column.rb

@@ -17,10 +17,28 @@ class Columns::LinkedDropDownColumn < Columns::ChampColumn
     )
   end
 
-  def filtered_ids(dossiers, values)
-    dossiers.with_type_de_champ(@column)
-      .filter_ilike(:champs, :value, values)
-      .ids
+  def filtered_ids(dossiers, search_terms)
+    relation = dossiers.with_type_de_champ(@stable_id)
+
+    case path
+    when :value
+      search_terms.flat_map do |search_term|
+        # when looking for "section 1 / option A",
+        # the value must contain both "section 1" and "option A"
+        primary, *secondary = search_term.split(%r{[[:space:]]*/[[:space:]]*})
+        safe_terms = [primary, *secondary].map { "%#{safe_like(_1)}%" }
+
+        relation.where("champs.value ILIKE ALL (ARRAY[?])", safe_terms).ids
+      end.uniq
+    when :primary
+      primary_terms = search_terms.map { |term| %{["#{safe_like(term)}","%"]} }
+
+      relation.where("champs.value ILIKE ANY (array[?])", primary_terms).ids
+    when :secondary
+      secondary_terms = search_terms.map { |term| %{["%","#{safe_like(term)}"]} }
+
+      relation.where("champs.value ILIKE ANY (array[?])", secondary_terms).ids
+    end
   end
 
   private
@@ -44,4 +62,6 @@ class Columns::LinkedDropDownColumn < Columns::ChampColumn
   rescue JSON::ParserError
     []
   end
+
+  def safe_like(q) = ActiveRecord::Base.sanitize_sql_like(q)
 end

app/models/concerns/dossier_filtering_concern.rb

@@ -28,22 +28,13 @@ module DossierFilteringConcern
       end
     }
 
-    scope :filter_ilike, lambda { |table, column, values|
+    scope :filter_ilike, lambda { |table, column, search_terms|
+      safe_quoted_terms = search_terms.map { "%#{sanitize_sql_like(_1)}%" }
       table_column = DossierFilterService.sanitized_column(table, column)
-      q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ')
-      where(q, *(values.map { |value| "%#{value}%" }))
+
+      where("#{table_column} LIKE ANY (ARRAY[?])", safe_quoted_terms)
     }
 
-    scope :filter_enum, lambda { |table, column, values|
-      table_column = DossierFilterService.sanitized_column(table, column)
-      q = Array.new(values.count, "(#{table_column} = ?)").join(' OR ')
-      where(q, *(values))
-    }
-
-    scope :filter_array_enum, lambda { |table, column, values|
-      table_column = DossierFilterService.sanitized_column(table, column)
-      q = Array.new(values.count, "(#{table_column} = ?)").join(' OR ')
-      where(q, *(values. map { |value| "[\"#{value}\"]" }))
-    }
+    def sanitize_sql_like(q) = ActiveRecord::Base.sanitize_sql_like(q)
   end
 end

config/brakeman.ignore

@@ -44,29 +44,6 @@
       ],
       "note": ""
     },
-    {
-      "warning_type": "SQL Injection",
-      "warning_code": 0,
-      "fingerprint": "5092b33433aef8fe42b688a780325f3791a77b39e55131256c78cebc3c14c0a3",
-      "check_name": "SQL",
-      "message": "Possible SQL injection",
-      "file": "app/models/concerns/dossier_filtering_concern.rb",
-      "line": 46,
-      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
-      "code": "where(\"#{values.count} OR #{\"(#{DossierFilterService.sanitized_column(table, column)} = ?)\"}\", *values.map do\n \"[\\\"#{value}\\\"]\"\n end)",
-      "render_path": null,
-      "location": {
-        "type": "method",
-        "class": "DossierFilteringConcern",
-        "method": null
-      },
-      "user_input": "values.count",
-      "confidence": "Medium",
-      "cwe_id": [
-        89
-      ],
-      "note": "filtered by rails query params where(something: ?, values)"
-    },
     {
       "warning_type": "SQL Injection",
       "warning_code": 0,
@@ -120,7 +97,7 @@
       "check_name": "SQL",
       "message": "Possible SQL injection",
       "file": "app/models/columns/json_path_column.rb",
-      "line": 24,
+      "line": 26,
       "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
       "code": "dossiers.with_type_de_champ(stable_id).where(\"champs.value_json @? '#{jsonpath} ? (@ like_regex \\\"#{quote_string(search_terms.join(\"|\"))}\\\" flag \\\"i\\\")'\")",
       "render_path": null,
@@ -136,29 +113,6 @@
       ],
       "note": "escaped by hand"
     },
-    {
-      "warning_type": "SQL Injection",
-      "warning_code": 0,
-      "fingerprint": "91ff8031e7c639c95fe6c244867349a72078ef456d8b3507deaf2bdb9bf62fe2",
-      "check_name": "SQL",
-      "message": "Possible SQL injection",
-      "file": "app/models/concerns/dossier_filtering_concern.rb",
-      "line": 34,
-      "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
-      "code": "where(\"#{values.count} OR #{\"(#{DossierFilterService.sanitized_column(table, column)} ILIKE ?)\"}\", *values.map do\n \"%#{value}%\"\n end)",
-      "render_path": null,
-      "location": {
-        "type": "method",
-        "class": "DossierFilteringConcern",
-        "method": null
-      },
-      "user_input": "values.count",
-      "confidence": "Medium",
-      "cwe_id": [
-        89
-      ],
-      "note": "filtered by rails query params where(something: ?, values)"
-    },
     {
       "warning_type": "Cross-Site Scripting",
       "warning_code": 2,
@@ -219,20 +173,20 @@
     {
       "warning_type": "SQL Injection",
       "warning_code": 0,
-      "fingerprint": "aaff41afa7bd5a551cd2e3a385071090cb53c95caa40fad3785cd3d68c9b939c",
+      "fingerprint": "afd2a1a41bd87fa62e065671670bd9bd8cc503ca4cbd3cfdb74a38a794146926",
       "check_name": "SQL",
       "message": "Possible SQL injection",
       "file": "app/models/concerns/dossier_filtering_concern.rb",
-      "line": 40,
+      "line": 35,
       "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
-      "code": "where(\"#{values.count} OR #{\"(#{DossierFilterService.sanitized_column(table, column)} = ?)\"}\", *values)",
+      "code": "where(\"#{DossierFilterService.sanitized_column(table, column)} LIKE ANY (ARRAY[?])\", search_terms.map do\n \"%#{sanitize_sql_like(_1)}%\"\n end)",
       "render_path": null,
       "location": {
         "type": "method",
         "class": "DossierFilteringConcern",
         "method": null
       },
-      "user_input": "values.count",
+      "user_input": "DossierFilterService.sanitized_column(table, column)",
       "confidence": "Medium",
       "cwe_id": [
         89
@@ -295,6 +249,6 @@
       "note": "Current is not a model"
     }
   ],
-  "updated": "2024-11-04 09:56:55 +0100",
+  "updated": "2024-11-12 17:33:07 +0100",
   "brakeman_version": "6.1.2"
 }

spec/models/columns/linked_drop_down_column_spec.rb

@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+describe Columns::LinkedDropDownColumn do
+  describe '#filtered_ids' do
+    let(:procedure) { create(:procedure, types_de_champ_public: [{ type: :linked_drop_down_list, libelle: 'linked' }]) }
+    let(:type_de_champ) { procedure.active_revision.types_de_champ_public.first }
+    let(:kept_dossier) { create(:dossier, procedure: procedure) }
+    let(:discarded_dossier) { create(:dossier, procedure: procedure) }
+
+    subject { column.filtered_ids(Dossier.all, search_terms) }
+
+    context 'when path is :value' do
+      let(:column) { procedure.find_column(label: 'linked') }
+
+      before do
+        kept_dossier.champs.find_by(stable_id: type_de_champ.stable_id)
+          .update(value: %{["section 1","option A"]})
+
+        discarded_dossier.champs.find_by(stable_id: type_de_champ.stable_id)
+          .update(value: %{["section 1","option B"]})
+      end
+
+      describe 'when looking for a part' do
+        let(:search_terms) { ['option A'] }
+
+        it { is_expected.to eq([kept_dossier.id]) }
+      end
+
+      describe 'when looking for the aggregated value' do
+        let(:search_terms) { ['section 1  /  option A'] }
+
+        it { is_expected.to match_array([kept_dossier.id]) }
+      end
+
+      describe 'when looking for the aggregated value or a common value' do
+        let(:search_terms) { ['section 1  /  option A', 'section'] }
+
+        it { is_expected.to match_array([kept_dossier.id, discarded_dossier.id]) }
+      end
+
+      describe 'when looking for a shared string' do
+        let(:search_terms) { ['option'] }
+
+        it { is_expected.to match_array([kept_dossier.id, discarded_dossier.id]) }
+      end
+    end
+
+    context 'when path is not :value' do
+      before do
+        kept_dossier.champs.find_by(stable_id: type_de_champ.stable_id)
+          .update(value: %{["1","2"]})
+
+        discarded_dossier.champs.find_by(stable_id: type_de_champ.stable_id)
+          .update(value: %{["2","1"]})
+      end
+
+      context 'when path is :primary' do
+        let(:column) { procedure.find_column(label: 'linked (Primaire)') }
+
+        describe 'when looking kept part' do
+          let(:search_terms) { ['1'] }
+
+          it { is_expected.to eq([kept_dossier.id]) }
+        end
+      end
+
+      context 'when path is :secondary' do
+        let(:column) { procedure.find_column(label: 'linked (Secondaire)') }
+
+        describe 'when looking kept part' do
+          let(:search_terms) { ['2'] }
+
+          it { is_expected.to eq([kept_dossier.id]) }
+        end
+      end
+    end
+  end
+end

spec/services/dossier_filter_service_spec.rb

@@ -510,20 +510,42 @@ describe DossierFilterService do
       end
 
       context 'with enums type_de_champ' do
-        let(:filter) { [type_de_champ.libelle, 'Favorable'] }
-        let(:types_de_champ_public) { [{ type: :multiple_drop_down_list, options: ['Favorable', 'Defavorable'] }] }
+        let(:filter) { [type_de_champ.libelle, search_term] }
+        let(:types_de_champ_public) { [{ type: :multiple_drop_down_list, options: ['champ', 'champignon'] }] }
 
         before do
           kept_champ = kept_dossier.champs.find_by(stable_id: type_de_champ.stable_id)
-          kept_champ.value = ['Favorable']
+          kept_champ.value = ['champ', 'champignon']
           kept_champ.save!
 
           discarded_champ = discarded_dossier.champs.find_by(stable_id: type_de_champ.stable_id)
-          discarded_champ.value = ['Defavorable']
+          discarded_champ.value = ['champignon']
           discarded_champ.save!
         end
 
-        it { is_expected.to contain_exactly(kept_dossier.id) }
+        context 'with single value' do
+          let(:search_term) { 'champ' }
+
+          it { is_expected.to contain_exactly(kept_dossier.id) }
+        end
+
+        context 'with multiple search values' do
+          let(:search_term) { 'champignon' }
+
+          it { is_expected.to contain_exactly(kept_dossier.id, discarded_dossier.id) }
+        end
+
+        context 'test if I could break a regex with %' do
+          let(:search_term) { '%' }
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'test if I could break a regex with .' do
+          let(:search_term) { '.*' }
+
+          it { is_expected.to be_empty }
+        end
       end
     end