instructeurs: fix ProcedurePresentation to use instructeur.user.email
The `joins` are declared explicitely in order to associate a predictable name to the joined table. Otherwise, when the query is joined with `:users`, ActiveRecord will alias the join automatically to solve the conflict. Unfortunately, the automatic resolution means that the table name becomes unpredictable, and thus unsuitable to perform queries on.
This commit is contained in:
Pierre de La Morinerie
parent
8b8213c301
commit
8e6930d257
3 changed files with 73 additions and 9 deletions
|
|
@ -94,7 +94,15 @@ class ProcedurePresentation < ApplicationRecord
|
|||
.where("champs.type_de_champ_id = #{column.to_i}")
|
||||
.order("champs.value #{order}")
|
||||
.pluck(:id)
|
||||
when 'self', 'user', 'individual', 'etablissement', 'followers_instructeurs', 'groupe_instructeur'
|
||||
when 'followers_instructeurs'
|
||||
assert_supported_column(table, column)
|
||||
# LEFT OUTER JOIN allows to keep dossiers without assignated instructeurs yet
|
||||
return dossiers
|
||||
.includes(:followers_instructeurs)
|
||||
.joins('LEFT OUTER JOIN users instructeurs_users ON instructeurs_users.instructeur_id = instructeurs.id')
|
||||
.order("instructeurs_users.email #{order}")
|
||||
.pluck(:id)
|
||||
when 'self', 'user', 'individual', 'etablissement', 'groupe_instructeur'
|
||||
return (table == 'self' ? dossiers : dossiers.includes(table))
|
||||
.order("#{self.class.sanitized_column(table, column)} #{order}")
|
||||
.pluck(:id)
|
||||
|
|
@ -130,7 +138,13 @@ class ProcedurePresentation < ApplicationRecord
|
|||
.includes(table)
|
||||
.filter_ilike(table, column, values)
|
||||
end
|
||||
when 'user', 'individual', 'followers_instructeurs'
|
||||
when 'followers_instructeurs'
|
||||
assert_supported_column(table, column)
|
||||
dossiers
|
||||
.includes(:followers_instructeurs)
|
||||
.joins('INNER JOIN users instructeurs_users ON instructeurs_users.instructeur_id = instructeurs.id')
|
||||
.filter_ilike('instructeurs_users', :email, values)
|
||||
when 'user', 'individual'
|
||||
dossiers
|
||||
.includes(table)
|
||||
.filter_ilike(table, column, values)
|
||||
|
|
@ -243,8 +257,12 @@ class ProcedurePresentation < ApplicationRecord
|
|||
def self.sanitized_column(association, column)
|
||||
table = if association == 'self'
|
||||
Dossier.table_name
|
||||
elsif (association_reflection = Dossier.reflect_on_association(association))
|
||||
association_reflection.klass.table_name
|
||||
else
|
||||
Dossier.reflect_on_association(association).klass.table_name
|
||||
# Allow filtering on a joined table alias (which doesn’t exist
|
||||
# in the ActiveRecord domain).
|
||||
association
|
||||
end
|
||||
|
||||
[table, column]
|
||||
|
|
@ -255,4 +273,10 @@ class ProcedurePresentation < ApplicationRecord
|
|||
def dossier_field_service
|
||||
@dossier_field_service ||= DossierFieldService.new
|
||||
end
|
||||
|
||||
def assert_supported_column(table, column)
|
||||
if table == 'followers_instructeurs' && column != 'email'
|
||||
raise ArgumentError, 'Table `followers_instructeurs` only supports the `email` column.'
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -7,10 +7,10 @@
|
|||
"check_name": "CrossSiteScripting",
|
||||
"message": "Unescaped model attribute",
|
||||
"file": "app/views/users/dossiers/merci.html.haml",
|
||||
"line": 26,
|
||||
"line": 28,
|
||||
"link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting",
|
||||
"code": "current_user.dossiers.includes(:procedure).find(params[:id]).procedure.monavis_embed",
|
||||
"render_path": [{"type":"controller","class":"Users::DossiersController","method":"merci","line":178,"file":"app/controllers/users/dossiers_controller.rb"}],
|
||||
"render_path": [{"type":"controller","class":"Users::DossiersController","method":"merci","line":177,"file":"app/controllers/users/dossiers_controller.rb"}],
|
||||
"location": {
|
||||
"type": "template",
|
||||
"template": "users/dossiers/merci"
|
||||
|
|
@ -46,7 +46,7 @@
|
|||
"check_name": "SQL",
|
||||
"message": "Possible SQL injection",
|
||||
"file": "app/models/procedure_presentation.rb",
|
||||
"line": 98,
|
||||
"line": 106,
|
||||
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
|
||||
"code": "((\"self\" == \"self\") ? (dossiers) : (dossiers.includes(\"self\"))).order(\"#{self.class.sanitized_column(\"self\", column)} #{order}\")",
|
||||
"render_path": null,
|
||||
|
|
@ -66,7 +66,7 @@
|
|||
"check_name": "SQL",
|
||||
"message": "Possible SQL injection",
|
||||
"file": "app/models/procedure_presentation.rb",
|
||||
"line": 94,
|
||||
"line": 95,
|
||||
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
|
||||
"code": "dossiers.includes(((\"type_de_champ\" == \"type_de_champ\") ? (:champs) : (:champs_private))).where(\"champs.type_de_champ_id = #{column.to_i}\").order(\"champs.value #{order}\")",
|
||||
"render_path": null,
|
||||
|
|
@ -78,8 +78,28 @@
|
|||
"user_input": "order",
|
||||
"confidence": "Weak",
|
||||
"note": "`column` and `order` come from the model, which is validated to prevent injection attacks. Furthermore, the sql injection attack on `column` would need to survive the `to_i`"
|
||||
},
|
||||
{
|
||||
"warning_type": "SQL Injection",
|
||||
"warning_code": 0,
|
||||
"fingerprint": "fd9d738975ccb93c8915833fceb3f43ac35410d653b8c64a1c92c1afc36d2177",
|
||||
"check_name": "SQL",
|
||||
"message": "Possible SQL injection",
|
||||
"file": "app/models/procedure_presentation.rb",
|
||||
"line": 102,
|
||||
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
|
||||
"code": "dossiers.includes(:followers_instructeurs).joins(\"LEFT OUTER JOIN users instructeurs_users ON instructeurs_users.instructeur_id = instructeurs.id\").order(\"instructeurs_users.email #{order}\")",
|
||||
"render_path": null,
|
||||
"location": {
|
||||
"type": "method",
|
||||
"class": "ProcedurePresentation",
|
||||
"method": "sorted_ids"
|
||||
},
|
||||
"user_input": "order",
|
||||
"confidence": "Weak",
|
||||
"note": ""
|
||||
}
|
||||
],
|
||||
"updated": "2019-07-02 11:23:21 -1000",
|
||||
"updated": "2019-10-16 16:19:43 +0200",
|
||||
"brakeman_version": "4.3.1"
|
||||
}
|
||||
|
|
|
|||
|
|
@ -379,6 +379,26 @@ describe ProcedurePresentation do
|
|||
end
|
||||
end
|
||||
|
||||
context 'for followers_instructeurs table' do
|
||||
let(:table) { 'followers_instructeurs' }
|
||||
let(:order) { 'asc' } # Desc works the same, no extra test required
|
||||
|
||||
let!(:dossier_a) { create(:dossier, :en_construction, procedure: procedure) }
|
||||
let!(:dossier_z) { create(:dossier, :en_construction, procedure: procedure) }
|
||||
let!(:dossier_without_instructeur) { create(:dossier, :en_construction, procedure: procedure) }
|
||||
|
||||
before do
|
||||
create(:follow, dossier: dossier_a, instructeur: create(:instructeur, email: 'abaca@exemple.fr'))
|
||||
create(:follow, dossier: dossier_z, instructeur: create(:instructeur, email: 'zythum@exemple.fr'))
|
||||
end
|
||||
|
||||
context 'for email column' do
|
||||
let(:column) { 'email' }
|
||||
|
||||
it { is_expected.to eq([dossier_a, dossier_z, dossier_without_instructeur].map(&:id)) }
|
||||
end
|
||||
end
|
||||
|
||||
context 'for other tables' do
|
||||
# All other columns and tables work the same so it’s ok to test only one
|
||||
let(:table) { 'etablissement' }
|
||||
|
|
@ -395,7 +415,7 @@ describe ProcedurePresentation do
|
|||
describe '#filtered_ids' do
|
||||
let(:procedure_presentation) { create(:procedure_presentation, assign_to: create(:assign_to, procedure: procedure), filters: { "suivis" => filter }) }
|
||||
|
||||
subject { procedure_presentation.filtered_ids(procedure.dossiers, 'suivis') }
|
||||
subject { procedure_presentation.filtered_ids(procedure.dossiers.joins(:user), 'suivis') }
|
||||
|
||||
context 'for self table' do
|
||||
context 'for created_at column' do
|
||||
|
|
|
|||
Loading…
Reference in a new issue