From 573b3d39e233e0f18bc616d1359d85bfe534e653 Mon Sep 17 00:00:00 2001 From: maatinito <15379878+maatinito@users.noreply.github.com> Date: Wed, 1 Dec 2021 16:15:00 -1000 Subject: [PATCH 1/2] Fix date_trunc sql queries for timezoned forks --- app/controllers/stats_controller.rb | 4 +- app/models/stat.rb | 6 +-- app/models/traitement.rb | 4 +- config/brakeman.ignore | 68 +++++++++++++++++++---------- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index eec5c5b2f..3d4e9e66d 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -141,7 +141,7 @@ class StatsController < ApplicationController association .where(date_attribute => min_date..max_date) - .group("DATE_TRUNC('month', #{date_attribute})") + .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") .count .to_a .sort_by { |a| a[0] } @@ -152,7 +152,7 @@ class StatsController < ApplicationController sum = 0 association .where("#{date_attribute} < ?", max_date) - .group("DATE_TRUNC('month', #{date_attribute})") + .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") .count .to_a .sort_by { |a| a[0] } diff --git a/app/models/stat.rb b/app/models/stat.rb index 37e1c131e..7e2068e3e 100644 --- a/app/models/stat.rb +++ b/app/models/stat.rb @@ -76,11 +76,11 @@ class Stat < ApplicationRecord end def last_four_months_hash(associations_with_date_attribute) - min_date = 3.months.ago.beginning_of_month.to_date + min_date = 3.months.ago.beginning_of_month timeseries = associations_with_date_attribute.map do |association, date_attribute| association .where(date_attribute => min_date..max_date) - .group("DATE_TRUNC('month', #{date_attribute})") + .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") .count end @@ -94,7 +94,7 @@ class Stat < ApplicationRecord timeseries = associations_with_date_attribute.map do |association, date_attribute| association .where("#{date_attribute} < ?", max_date) - .group("DATE_TRUNC('month', #{date_attribute})") + .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") .count end diff --git a/app/models/traitement.rb b/app/models/traitement.rb index a407842d6..4975bbcc1 100644 --- a/app/models/traitement.rb +++ b/app/models/traitement.rb @@ -43,9 +43,9 @@ class Traitement < ApplicationRecord .to_sql sql = <<~EOF - select date_trunc('month', r1.processed_at) as month, count(r1.processed_at) + select date_trunc('month', r1.processed_at::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL) as month, count(r1.processed_at) from (#{last_traitements_per_dossier}) as r1 - group by date_trunc('month', r1.processed_at) + group by date_trunc('month', r1.processed_at::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL) order by month desc EOF diff --git a/config/brakeman.ignore b/config/brakeman.ignore index a21e71bf4..d1cd809da 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -31,6 +31,26 @@ "confidence": "Weak", "note": "explicitely sanitized even if we are using html_safe" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "4254ed68100af9b496883716b1fd658e1943b2385a0d08de5a6ef5c600c1a8f9", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/traitement.rb", + "line": 51, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "ActiveRecord::Base.connection.execute(\"select date_trunc('month', r1.processed_at::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL) as month, count(r1.processed_at)\\nfrom (#{Traitement.select(\"max(traitements.processed_at) as processed_at\").termine.where(:dossier => Dossier.state_termine.where(:groupe_instructeur => groupe_instructeurs)).group(:dossier_id).to_sql}) as r1\\ngroup by date_trunc('month', r1.processed_at::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)\\norder by month desc\\n\")", + "render_path": null, + "location": { + "type": "method", + "class": "Traitement", + "method": "Traitement.count_dossiers_termines_by_month" + }, + "user_input": "Time.zone.formatted_offset", + "confidence": "Medium", + "note": "" + }, { "warning_type": "Cross-Site Scripting", "warning_code": 2, @@ -62,26 +82,6 @@ "confidence": "Weak", "note": "" }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "6c98e520dd368104bb0c81334875010711cd523afc28057ef86a10930f95c4b7", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/models/stat.rb", - "line": 83, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "association.where(date_attribute => ((3.months.ago.beginning_of_month.to_date..max_date))).group(\"DATE_TRUNC('month', #{date_attribute})\")", - "render_path": null, - "location": { - "type": "method", - "class": "Stat", - "method": "last_four_months_hash" - }, - "user_input": "date_attribute", - "confidence": "Weak", - "note": "no user input, fixed value" - }, { "warning_type": "SQL Injection", "warning_code": 0, @@ -102,6 +102,26 @@ "confidence": "Medium", "note": "The table and column are escaped, which should make this safe" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "c0f93612a68c32da58f327e0b5fa33dd42fd8beb2984cf023338c5aadbbdacca", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/stat.rb", + "line": 83, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "association.where(date_attribute => ((3.months.ago.beginning_of_month..max_date))).group(\"DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)\")", + "render_path": null, + "location": { + "type": "method", + "class": "Stat", + "method": "last_four_months_hash" + }, + "user_input": "date_attribute", + "confidence": "Weak", + "note": "" + }, { "warning_type": "Redirect", "warning_code": 18, @@ -125,13 +145,13 @@ { "warning_type": "SQL Injection", "warning_code": 0, - "fingerprint": "dc6d873aff3dc5e51e3349b17e1f35039b23d0bddbf04224b0f1bca3e4608c1e", + "fingerprint": "f2bb9bc6a56e44ab36ee18152c657395841cff354baed0a302b8d18650551529", "check_name": "SQL", "message": "Possible SQL injection", "file": "app/models/stat.rb", "line": 97, "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "association.where(\"#{date_attribute} < ?\", max_date).group(\"DATE_TRUNC('month', #{date_attribute})\")", + "code": "association.where(\"#{date_attribute} < ?\", max_date).group(\"DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)\")", "render_path": null, "location": { "type": "method", @@ -140,9 +160,9 @@ }, "user_input": "date_attribute", "confidence": "Weak", - "note": "no user input, fixed value" + "note": "" } ], - "updated": "2021-12-13 17:09:07 +0100", + "updated": "2021-12-01 17:39:08 -1000", "brakeman_version": "5.1.1" } From 0a31c8bc7936bf6abc842be52614594b993ed9c5 Mon Sep 17 00:00:00 2001 From: maatinito <15379878+maatinito@users.noreply.github.com> Date: Thu, 2 Dec 2021 15:46:45 -1000 Subject: [PATCH 2/2] refactor date_trunc queries using groupdate gem --- app/controllers/stats_controller.rb | 25 +++++--------- app/models/stat.rb | 40 ++++++++++------------ spec/controllers/stats_controller_spec.rb | 38 +++++++++++---------- spec/models/stat_spec.rb | 41 ++++++++++++----------- 4 files changed, 67 insertions(+), 77 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 3d4e9e66d..1202df9c8 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -27,8 +27,8 @@ class StatsController < ApplicationController "Terminé" => stat.dossiers_termines } - @procedures_cumulative = cumulative_hash(procedures, :published_at) - @procedures_in_the_last_4_months = last_four_months_hash(procedures, :published_at) + @procedures_cumulative = cumulative_month_serie(procedures, :published_at) + @procedures_in_the_last_4_months = last_four_months_serie(procedures, :published_at) @dossiers_cumulative = stat.dossiers_cumulative @dossiers_in_the_last_4_months = stat.dossiers_in_the_last_4_months @@ -136,27 +136,18 @@ class StatsController < ApplicationController end end - def last_four_months_hash(association, date_attribute) - min_date = 3.months.ago.beginning_of_month.to_date - + def last_four_months_serie(association, date_attribute) association - .where(date_attribute => min_date..max_date) - .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") + .group_by_month(date_attribute, last: 4, current: super_admin_signed_in?) .count - .to_a - .sort_by { |a| a[0] } - .map { |e| [I18n.l(e.first, format: "%B %Y"), e.last] } + .transform_keys { |date| l(date, format: "%B %Y") } end - def cumulative_hash(association, date_attribute) + def cumulative_month_serie(association, date_attribute) sum = 0 association - .where("#{date_attribute} < ?", max_date) - .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") + .group_by_month(date_attribute, current: super_admin_signed_in?) .count - .to_a - .sort_by { |a| a[0] } - .map { |x, y| { x => (sum += y) } } - .reduce({}, :merge) + .transform_values { |count| sum += count } end end diff --git a/app/models/stat.rb b/app/models/stat.rb index 7e2068e3e..ce0109c5b 100644 --- a/app/models/stat.rb +++ b/app/models/stat.rb @@ -30,11 +30,11 @@ class Stat < ApplicationRecord dossiers_deposes_entre_60_et_30_jours: states['dossiers_deposes_entre_60_et_30_jours'], dossiers_not_brouillon: states['not_brouillon'], dossiers_termines: states['termines'], - dossiers_cumulative: cumulative_hash([ + dossiers_cumulative: cumulative_month_serie([ [Dossier.state_not_brouillon, :depose_at], [DeletedDossier.where.not(state: :brouillon), :deleted_at] ]), - dossiers_in_the_last_4_months: last_four_months_hash([ + dossiers_in_the_last_4_months: last_four_months_serie([ [Dossier.state_not_brouillon, :depose_at], [DeletedDossier.where.not(state: :brouillon), :deleted_at] ]), @@ -75,39 +75,33 @@ class Stat < ApplicationRecord ) end - def last_four_months_hash(associations_with_date_attribute) - min_date = 3.months.ago.beginning_of_month + def last_four_months_serie(associations_with_date_attribute) timeseries = associations_with_date_attribute.map do |association, date_attribute| - association - .where(date_attribute => min_date..max_date) - .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") - .count + association.group_by_month(date_attribute, last: 4, current: false).count end - sum_hashes(*timeseries) - .to_a - .sort_by { |a| a[0] } - .map { |e| [I18n.l(e.first, format: "%B %Y"), e.last] } + month_serie(sum_hashes(*timeseries)) end - def cumulative_hash(associations_with_date_attribute) + def month_serie(date_serie) + date_serie.keys.sort.each_with_object({}) { |date, h| h[I18n.l(date, format: "%B %Y")] = date_serie[date] } + end + + def cumulative_month_serie(associations_with_date_attribute) timeseries = associations_with_date_attribute.map do |association, date_attribute| - association - .where("#{date_attribute} < ?", max_date) - .group("DATE_TRUNC('month', #{date_attribute}::TIMESTAMPTZ AT TIME ZONE '#{Time.zone.formatted_offset}'::INTERVAL)") - .count + association.group_by_month(date_attribute, current: false).count end + cumulative_serie(sum_hashes(*timeseries)) + end + + def cumulative_serie(sums) sum = 0 - sum_hashes(*timeseries) - .to_a - .sort_by { |a| a[0] } - .map { |x, y| { x => (sum += y) } } - .reduce({}, :merge) + sums.keys.sort.index_with { |date| sum += sums[date] } end def sum_hashes(*hashes) - {}.merge(*hashes) { |_k, hash_one_value, hash_two_value| hash_one_value + hash_two_value } + {}.merge(*hashes) { |_k, v1, v2| v1 + v2 } end def max_date diff --git a/spec/controllers/stats_controller_spec.rb b/spec/controllers/stats_controller_spec.rb index bb6c11914..4e9ae2e17 100644 --- a/spec/controllers/stats_controller_spec.rb +++ b/spec/controllers/stats_controller_spec.rb @@ -14,13 +14,15 @@ describe StatsController, type: :controller do let(:association) { Procedure.all } - subject { @controller.send(:last_four_months_hash, association, :updated_at) } + subject { @controller.send(:last_four_months_serie, association, :updated_at) } it do - expect(subject).to match_array([ - [I18n.l(62.days.ago.beginning_of_month, format: "%B %Y"), 2], - [I18n.l(31.days.ago.beginning_of_month, format: "%B %Y"), 1] - ]) + expect(subject).to eq({ + 4.months.ago => 0, + 3.months.ago => 0, + 62.days.ago => 2, + 31.days.ago => 1 + }.transform_keys { |date| I18n.l(date, format: '%B %Y') }) end end @@ -38,13 +40,15 @@ describe StatsController, type: :controller do let (:association) { Procedure.all } - subject { @controller.send(:last_four_months_hash, association, :updated_at) } + subject { @controller.send(:last_four_months_serie, association, :updated_at) } it do - expect(subject).to eq([ - [I18n.l(45.days.ago.beginning_of_month, format: "%B %Y"), 1], - [I18n.l(1.day.ago.beginning_of_month, format: "%B %Y"), 2] - ]) + expect(subject).to eq({ + 3.months.ago => 0, + 45.days.ago => 1, + 1.month.ago => 0, + 1.day.ago => 2 + }.transform_keys { |date| I18n.l(date, format: '%B %Y') }) end end end @@ -66,13 +70,13 @@ describe StatsController, type: :controller do context "while a super admin is logged in" do before { allow(@controller).to receive(:super_admin_signed_in?).and_return(true) } - subject { @controller.send(:cumulative_hash, association, :updated_at) } + subject { @controller.send(:cumulative_month_serie, association, :updated_at) } it do expect(subject).to eq({ - Time.utc(2016, 8, 1) => 2, - Time.utc(2016, 9, 1) => 4, - Time.utc(2016, 10, 1) => 5 + Date.new(2016, 8, 1) => 2, + Date.new(2016, 9, 1) => 4, + Date.new(2016, 10, 1) => 5 }) end end @@ -80,12 +84,12 @@ describe StatsController, type: :controller do context "while a super admin is not logged in" do before { allow(@controller).to receive(:super_admin_signed_in?).and_return(false) } - subject { @controller.send(:cumulative_hash, association, :updated_at) } + subject { @controller.send(:cumulative_month_serie, association, :updated_at) } it do expect(subject).to eq({ - Time.utc(2016, 8, 1) => 2, - Time.utc(2016, 9, 1) => 4 + Date.new(2016, 8, 1) => 2, + Date.new(2016, 9, 1) => 4 }) end end diff --git a/spec/models/stat_spec.rb b/spec/models/stat_spec.rb index 518cf04bf..e9fc2edc2 100644 --- a/spec/models/stat_spec.rb +++ b/spec/models/stat_spec.rb @@ -57,24 +57,24 @@ describe Stat do create(:dossier, state: :en_construction, depose_at: i.months.ago) create(:deleted_dossier, dossier_id: i + 100, state: :en_construction, deleted_at: i.month.ago) end - rs = Stat.send(:cumulative_hash, [ + rs = Stat.send(:cumulative_month_serie, [ [Dossier.state_not_brouillon, :depose_at], [DeletedDossier.where.not(state: :brouillon), :deleted_at] ]) expect(rs).to eq({ - 12.months.ago.utc.beginning_of_month => 2, - 11.months.ago.utc.beginning_of_month => 4, - 10.months.ago.utc.beginning_of_month => 6, - 9.months.ago.utc.beginning_of_month => 8, - 8.months.ago.utc.beginning_of_month => 10, - 7.months.ago.utc.beginning_of_month => 12, - 6.months.ago.utc.beginning_of_month => 14, - 5.months.ago.utc.beginning_of_month => 16, - 4.months.ago.utc.beginning_of_month => 18, - 3.months.ago.utc.beginning_of_month => 20, - 2.months.ago.utc.beginning_of_month => 22, - 1.month.ago.utc.beginning_of_month => 24 - }) + 12 => 2, + 11 => 4, + 10 => 6, + 9 => 8, + 8 => 10, + 7 => 12, + 6 => 14, + 5 => 16, + 4 => 18, + 3 => 20, + 2 => 22, + 1 => 24 + }.transform_keys { |i| i.months.ago.beginning_of_month.to_date }) end end @@ -85,15 +85,16 @@ describe Stat do create(:dossier, state: :en_construction, depose_at: i.months.ago) create(:deleted_dossier, dossier_id: i + 100, state: :en_construction, deleted_at: i.month.ago) end - rs = Stat.send(:last_four_months_hash, [ + rs = Stat.send(:last_four_months_serie, [ [Dossier.state_not_brouillon, :depose_at], [DeletedDossier.where.not(state: :brouillon), :deleted_at] ]) - expect(rs).to eq([ - ["août 2021", 2], - ["septembre 2021", 2], - ["octobre 2021", 2] - ]) + expect(rs).to eq({ + "juillet 2021" => 2, + "août 2021" => 2, + "septembre 2021" => 2, + "octobre 2021" => 2 + }) end end end