Dépréciation de la colonne Instructeur.email (#4411)

Dépréciation de la colonne Instructeur.email
This commit is contained in:
Pierre de La Morinerie 2019-11-04 11:02:32 +01:00 committed by GitHub
commit 91ddd157f0
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
37 changed files with 156 additions and 70 deletions

View file

@ -25,8 +25,6 @@ class Admin::AssignsController < AdminController
not_assign_scope,
partial: "admin/assigns/list_not_assign",
array: true
@instructeur ||= Instructeur.new
end
def update

View file

@ -7,13 +7,11 @@ class Admin::InstructeursController < AdminController
current_administrateur.instructeurs,
partial: "admin/instructeurs/list",
array: true
@instructeur ||= Instructeur.new
end
def create
email = params[:instructeur][:email].downcase
@instructeur = Instructeur.find_by(email: email)
@instructeur = Instructeur.by_email(email)
procedure_id = params[:procedure_id]
if @instructeur.nil?

View file

@ -81,7 +81,7 @@ module Instructeurs
def create_instructeur
email = params[:email]
password = params['instructeur']['password']
password = params[:user][:password]
# Not perfect because the password will not be changed if the user already exists
user = User.create_or_promote_to_instructeur(email, password)

View file

@ -44,9 +44,9 @@ module Instructeurs
end
def personnes_impliquees
@following_instructeurs_emails = dossier.followers_instructeurs.pluck(:email)
@following_instructeurs_emails = dossier.followers_instructeurs.map(&:email)
previous_followers = dossier.previous_followers_instructeurs - dossier.followers_instructeurs
@previous_following_instructeurs_emails = previous_followers.pluck(:email)
@previous_following_instructeurs_emails = previous_followers.map(&:email)
@avis_emails = dossier.avis.includes(:instructeur).map(&:email_to_display)
@invites_emails = dossier.invites.map(&:email)
@potential_recipients = dossier.groupe_instructeur.instructeurs.reject { |g| g == current_instructeur }

View file

@ -15,7 +15,7 @@ class Users::PasswordsController < Devise::PasswordsController
@devise_mapping = Devise.mappings[:administrateur]
params[:administrateur] = params[:user]
# uncomment to check password complexity for Instructeur
# elsif Instructeur.find_by(email: email)
# elsif Instructeur.by_email(email)
# @devise_mapping = Devise.mappings[:instructeur]
# params[:instructeur] = params[:user]
end
@ -46,7 +46,7 @@ class Users::PasswordsController < Devise::PasswordsController
def try_to_authenticate_instructeur
if user_signed_in?
instructeur = Instructeur.find_by(email: current_user.email)
instructeur = Instructeur.by_email(current_user.email)
if instructeur
sign_in(instructeur.user)

View file

@ -4,8 +4,8 @@ class WebhookController < ActionController::Base
def helpscout
email = params[:customer][:email].downcase
user = User.find_by(email: email)
instructeur = Instructeur.find_by(email: email)
administrateur = Administrateur.find_by(email: email)
instructeur = user.instructeur
administrateur = user.administrateur
html = []
if user

View file

@ -8,7 +8,7 @@ class GroupeInstructeurMailer < ApplicationMailer
subject = "Ajout dun instructeur dans le groupe \"#{group.label}\""
emails = @group.instructeurs.pluck(:email)
emails = @group.instructeurs.map(&:email)
mail(bcc: emails, subject: subject)
end
@ -19,7 +19,7 @@ class GroupeInstructeurMailer < ApplicationMailer
subject = "Suppression dun instructeur dans le groupe \"#{group.label}\""
emails = @group.instructeurs.pluck(:email)
emails = @group.instructeurs.map(&:email)
mail(bcc: emails, subject: subject)
end
end

View file

@ -74,7 +74,7 @@ class Administrateur < ApplicationRecord
end
def instructeur
Instructeur.find_by(email: email)
user.instructeur
end
def can_be_deleted?

View file

@ -50,7 +50,7 @@ class Avis < ApplicationRecord
private
def try_to_assign_instructeur
instructeur = Instructeur.find_by(email: email)
instructeur = Instructeur.by_email(email)
if instructeur
self.instructeur = instructeur
self.email = nil

View file

@ -365,7 +365,7 @@ class Dossier < ApplicationRecord
update(hidden_at: deleted_dossier.deleted_at)
if en_construction?
administration_emails = followers_instructeurs.present? ? followers_instructeurs.pluck(:email) : procedure.administrateurs.pluck(:email)
administration_emails = followers_instructeurs.present? ? followers_instructeurs.map(&:email) : procedure.administrateurs.pluck(:email)
administration_emails.each do |email|
DossierMailer.notify_deletion_to_administration(deleted_dossier, email).deliver_later
end

View file

@ -1,11 +1,8 @@
class Instructeur < ApplicationRecord
self.ignored_columns = ['features', 'encrypted_password', 'reset_password_token', 'reset_password_sent_at', 'remember_created_at', 'sign_in_count', 'current_sign_in_at', 'last_sign_in_at', 'current_sign_in_ip', 'last_sign_in_ip', 'failed_attempts', 'unlock_token', 'locked_at']
include EmailSanitizableConcern
self.ignored_columns = ['email', 'features', 'encrypted_password', 'reset_password_token', 'reset_password_sent_at', 'remember_created_at', 'sign_in_count', 'current_sign_in_at', 'last_sign_in_at', 'current_sign_in_ip', 'last_sign_in_ip', 'failed_attempts', 'unlock_token', 'locked_at']
has_and_belongs_to_many :administrateurs
before_validation -> { sanitize_email(:email) }
has_many :assign_to, dependent: :destroy
has_many :groupe_instructeurs, through: :assign_to
has_many :procedures, -> { distinct }, through: :groupe_instructeurs
@ -24,6 +21,16 @@ class Instructeur < ApplicationRecord
has_one :user
default_scope { eager_load(:user) }
def self.by_email(email)
Instructeur.eager_load(:user).find_by(users: { email: email })
end
def email
user.email
end
def follow(dossier)
begin
followed_dossiers << dossier

View file

@ -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 doesnt 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

View file

@ -70,7 +70,7 @@ class User < ApplicationRecord
if user.valid?
if user.instructeur_id.nil?
user.create_instructeur!(email: email)
user.create_instructeur!
end
user.instructeur.administrateurs << administrateurs

View file

@ -94,7 +94,7 @@ class DossierSerializer < ActiveModel::Serializer
end
def instructeurs
object.followers_instructeurs.pluck(:email)
object.followers_instructeurs.map(&:email)
end
def created_at

View file

@ -9,15 +9,15 @@
%h3
= t('dynamics.admin.procedure.onglet_instructeurs.add.title')
#procedure_new.section.section-label
= form_for @instructeur, url: { controller: 'admin/instructeurs', action: :create } do |f|
= form_with url: { controller: 'admin/instructeurs', action: :create } do
.row
.col-xs-5
= hidden_field_tag :procedure_id, params[:procedure_id]
= render partial: 'admin/instructeurs/informations', locals: { f: f }
= render partial: 'admin/instructeurs/informations'
.col-xs-2
%br
%br
= f.submit 'Valider', class: 'btn btn-info', style: 'float: left;', id: 'add-instructeur-email'
= submit_tag 'Ajouter', class: 'btn btn-info', style: 'float: left;', id: 'add-instructeur-email'
.col-xs-6
%h3.text-success Affectés
= smart_listing_render :instructeurs_assign

View file

@ -1,5 +1,4 @@
- { email: 'Email*' }.each do |key, value|
.form-group
%h4
= value
= f.text_field key, class: 'form-control', placeholder: value
.form-group
%h4
= 'Email *'
= text_field_tag 'instructeur[email]', nil, class: 'form-control', placeholder: 'laura.azema@exemple.gouv.fr'

View file

@ -18,11 +18,11 @@
.col-xs-6
%h3 Ajouter un instructeur
#procedure_new.section.section-label
= form_for @instructeur, url: { controller: 'admin/instructeurs', action: :create } do |f|
= form_with url: { controller: 'admin/instructeurs', action: :create } do
.row
.col-xs-5
= render partial: 'informations', locals: { f: f }
= render partial: 'admin/instructeurs/informations'
.col-xs-2
%br
%br
= f.submit 'Valider', class: 'btn btn-info', style: 'float: left;'
= submit_tag 'Ajouter', class: 'btn btn-info', style: 'float: left;'

View file

@ -4,7 +4,7 @@
%p.description= @dossier.procedure.libelle
%p.dossier Dossier nº #{@dossier.id}
.column
= form_for(Instructeur.new, url: { controller: "instructeurs/avis", action: :create_instructeur }, method: :post, html: { class: "form" }) do |f|
= form_for(User.new, url: { controller: "instructeurs/avis", action: :create_instructeur }, method: :post, html: { class: "form" }) do |f|
%h1 Créez-vous un compte
= f.label :email, "Email"

View file

@ -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"
}

View file

@ -0,0 +1,13 @@
class RemoveUniqueConstraintOnInstructeurEmails < ActiveRecord::Migration[5.2]
def up
# Drop the index entirely
remove_index :instructeurs, :email
# Add the index again, without the unicity constraint
add_index :instructeurs, :email
end
def down
remove_index :instructeurs, :email
add_index :instructeurs, :email, unique: true
end
end

View file

@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 2019_10_23_183120) do
ActiveRecord::Schema.define(version: 2019_10_24_150452) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@ -419,7 +419,7 @@ ActiveRecord::Schema.define(version: 2019_10_23_183120) do
t.datetime "updated_at"
t.text "encrypted_login_token"
t.datetime "login_token_created_at"
t.index ["email"], name: "index_instructeurs_on_email", unique: true
t.index ["email"], name: "index_instructeurs_on_email"
end
create_table "invites", id: :serial, force: :cascade do |t|

View file

@ -10,12 +10,10 @@ default_password = "this is a very complicated password !"
puts "Create test user '#{default_user}'"
Administration.create!(email: default_user, password: default_password)
administrateur = Administrateur.create!(email: default_user)
instructeur = Instructeur.create!(email: default_user)
User.create!(
user = User.create!(
email: default_user,
password: default_password,
confirmed_at: Time.zone.now,
administrateur: administrateur,
instructeur: instructeur
confirmed_at: Time.zone.now
)
user.create_instructeur!
user.create_administrateur!(email: user.email)

View file

@ -112,7 +112,7 @@ describe Admin::InstructeursController, type: :controller do
end
context 'when an other admin will add the same email' do
let(:instructeur) { Instructeur.find_by(email: email) }
let(:instructeur) { Instructeur.by_email(email) }
before do
create :instructeur, email: email, administrateurs: [admin]
@ -133,7 +133,7 @@ describe Admin::InstructeursController, type: :controller do
context 'when an other admin will add the same email with some uppercase in it' do
let(:email) { 'Test@Plop.com' }
let(:instructeur) { Instructeur.find_by(email: email.downcase) }
let(:instructeur) { Instructeur.by_email(email.downcase) }
before do
create :instructeur, email: email, administrateurs: [admin]

View file

@ -284,7 +284,7 @@ describe Instructeurs::AvisController, type: :controller do
let!(:avis) { create(:avis, email: invited_email, dossier: dossier) }
let(:avis_id) { avis.id }
let(:password) { 'démarches-simplifiées-pwd' }
let(:created_instructeur) { Instructeur.find_by(email: invited_email) }
let(:created_instructeur) { Instructeur.by_email(invited_email) }
let(:invitations_email) { true }
before do
@ -296,7 +296,7 @@ describe Instructeurs::AvisController, type: :controller do
post :create_instructeur, params: {
id: avis_id,
email: invited_email,
instructeur: {
user: {
password: password
}
}

View file

@ -50,7 +50,7 @@ describe Instructeurs::GroupeInstructeursController, type: :controller do
context 'of a new instructeur' do
let(:new_instructeur_email) { 'new_instructeur@mail.com' }
it { expect(gi_1_2.instructeurs.pluck(:email)).to include(new_instructeur_email) }
it { expect(gi_1_2.instructeurs.map(&:email)).to include(new_instructeur_email) }
it { expect(flash.notice).to be_present }
it { expect(response).to redirect_to(instructeur_groupe_path(procedure, gi_1_2)) }
end

View file

@ -100,7 +100,7 @@ describe NewAdministrateur::GroupeInstructeursController, type: :controller do
context 'of a new instructeur' do
let(:new_instructeur_email) { 'new_instructeur@mail.com' }
it { expect(gi_1_1.instructeurs.pluck(:email)).to include(new_instructeur_email) }
it { expect(gi_1_1.instructeurs.map(&:email)).to include(new_instructeur_email) }
it { expect(flash.notice).to be_present }
it { expect(response).to redirect_to(procedure_groupe_instructeur_path(procedure, gi_1_1)) }
end

View file

@ -2,9 +2,8 @@ FactoryBot.define do
sequence(:instructeur_email) { |n| "inst#{n}@inst.com" }
factory :instructeur do
email { generate(:instructeur_email) }
transient do
email { generate(:instructeur_email) }
password { 'somethingverycomplated!' }
end

View file

@ -12,7 +12,7 @@ feature 'As an instructeur', js: true do
fill_in :instructeur_email, with: instructeur_email
perform_enqueued_jobs do
click_button 'Valider'
click_button 'Ajouter'
end
end

View file

@ -181,7 +181,7 @@ feature 'Instructing a dossier:' do
def avis_sign_up(avis, email)
visit sign_up_instructeur_avis_path(avis, email)
fill_in 'instructeur_password', with: 'démarches-simplifiées-pwd'
fill_in 'user_password', with: 'démarches-simplifiées-pwd'
click_on 'Créer un compte'
expect(page).to have_current_path(instructeur_avis_index_path)
end

View file

@ -1,7 +1,7 @@
# Preview all emails at http://localhost:3000/rails/mailers/avis_mailer
class AvisMailerPreview < ActionMailer::Preview
def avis_invitation
gestionaire = Instructeur.new(id: 1, email: 'jeanmichel.de-chauvigny@exemple.fr')
gestionaire = Instructeur.new(id: 1, user: User.new(email: 'jeanmichel.de-chauvigny@exemple.fr'))
avis = Avis.new(id: 1, email: 'test@exemple.fr', claimant: gestionaire)
avis.dossier = Dossier.new(id: 1)
avis.dossier.procedure = Procedure.new(libelle: 'Démarche pour faire des marches')

View file

@ -44,11 +44,17 @@ class InstructeurMailerPreview < ActionMailer::Preview
private
def instructeur
Instructeur.new(id: 10, email: 'instructeur@administration.gouv.fr')
Instructeur.new(
id: 10,
user: User.new(email: 'instructeur@administration.gouv.fr')
)
end
def target_instructeur
Instructeur.new(id: 12, email: 'collegue@administration.gouv.fr')
Instructeur.new(
id: 12,
user: User.new(email: 'collegue@administration.gouv.fr')
)
end
def procedure

View file

@ -24,7 +24,7 @@ describe Administration, type: :model do
it 'creates a corresponding instructeur account for the email' do
subject
instructeur = Instructeur.find_by(email: valid_email)
instructeur = Instructeur.by_email(valid_email)
expect(instructeur).to be_present
end

View file

@ -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 its 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

View file

@ -401,7 +401,7 @@ describe Procedure do
subject { @procedure }
it { expect(subject.parent_procedure).to eq(procedure) }
it { expect(subject.defaut_groupe_instructeur.instructeurs.pluck(:email)).to eq([administrateur.email]) }
it { expect(subject.defaut_groupe_instructeur.instructeurs.map(&:email)).to eq([administrateur.email]) }
it 'should duplicate specific objects with different id' do
expect(subject.id).not_to eq(procedure.id)
@ -814,11 +814,15 @@ describe Procedure do
end
before do
Timecop.freeze(Time.utc(2019, 6, 1, 12, 0))
delays.each do |delay|
create_dossier(construction_date: 1.week.ago - delay, instruction_date: 1.week.ago - delay + 12.hours, processed_date: 1.week.ago)
end
end
after { Timecop.return }
context 'when there are several processed dossiers' do
let(:delays) { [1.day, 2.days, 2.days, 2.days, 2.days, 3.days, 3.days, 3.days, 3.days, 12.days] }

View file

@ -138,7 +138,7 @@ describe User, type: :model do
context 'with an existing instructeur' do
let(:old_admins) { [create(:administrateur)] }
let(:admins) { [create(:administrateur)] }
let!(:instructeur) { Instructeur.create(email: 'i@mail.com', administrateurs: old_admins) }
let!(:instructeur) { create(:instructeur, email: 'i@mail.com', administrateurs: old_admins) }
before do
User

View file

@ -9,7 +9,7 @@ describe 'admin/assigns/show.html.haml', type: :view do
before do
assign(:procedure, procedure)
assign(:instructeur, Instructeur.new)
assign(:instructeur, create(:instructeur))
assign(:instructeurs_assign, (smart_listing_create :instructeurs_assign,
assign_instructeurs,

View file

@ -8,7 +8,7 @@ describe 'admin/instructeurs/index.html.haml', type: :view do
admin.instructeurs,
partial: "admin/instructeurs/list",
array: true))
assign(:instructeur, Instructeur.new())
assign(:instructeur, create(:instructeur))
end
context 'Aucun instructeur' do