From cb890343ff94e5507f7fd0f3b5971e2a4b2f4617 Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 23 May 2022 15:09:22 +0200 Subject: [PATCH] feat(targeted_user_link): add targeted user link to wrap expert invitation in order to avoid access issue when the expert is connected with another account feat(user.merge): ensure to merge user.targeted_user_link Update app/models/targeted_user_link.rb Co-authored-by: LeSim Update app/models/targeted_user_link.rb Co-authored-by: LeSim Update app/models/targeted_user_link.rb Co-authored-by: LeSim feat(db/create_targeted_user_links): ensure not null with fk --- .../concerns/create_avis_concern.rb | 4 +- .../targeted_user_links_controller.rb | 16 ++++++ app/mailers/avis_mailer.rb | 3 +- app/models/avis.rb | 2 + app/models/targeted_user_link.rb | 32 +++++++++++ app/models/user.rb | 2 + .../avis_mailer/avis_invitation.html.haml | 7 ++- app/views/targeted_user_links/show.html.haml | 19 +++++++ config/brakeman.ignore | 30 +++++++++-- config/routes.rb | 2 + db/migrate/20220520134041_enable_pgcrypto.rb | 10 ++++ ...220520134042_create_targeted_user_links.rb | 13 +++++ db/schema.rb | 12 +++++ .../experts/avis_controller_spec.rb | 1 + .../targeted_user_links_controller_spec.rb | 54 +++++++++++++++++++ spec/factories/targeted_user_links.rb | 12 +++++ spec/mailers/avis_mailer_spec.rb | 9 ++-- spec/mailers/previews/avis_mailer_preview.rb | 3 +- spec/models/targeted_user_link_spec.rb | 13 +++++ spec/models/user_spec.rb | 12 +++++ spec/system/instructeurs/expert_spec.rb | 7 ++- 21 files changed, 241 insertions(+), 22 deletions(-) create mode 100644 app/controllers/targeted_user_links_controller.rb create mode 100644 app/models/targeted_user_link.rb create mode 100644 app/views/targeted_user_links/show.html.haml create mode 100644 db/migrate/20220520134041_enable_pgcrypto.rb create mode 100644 db/migrate/20220520134042_create_targeted_user_links.rb create mode 100644 spec/controllers/targeted_user_links_controller_spec.rb create mode 100644 spec/factories/targeted_user_links.rb create mode 100644 spec/models/targeted_user_link_spec.rb diff --git a/app/controllers/concerns/create_avis_concern.rb b/app/controllers/concerns/create_avis_concern.rb index 74965976c..a3c1a006f 100644 --- a/app/controllers/concerns/create_avis_concern.rb +++ b/app/controllers/concerns/create_avis_concern.rb @@ -20,6 +20,7 @@ module CreateAvisConcern if (instructeur_or_expert.is_a?(Instructeur)) && !instructeur_or_expert.follows.exists?(dossier: dossier) instructeur_or_expert.follow(dossier) end + create_results = Avis.create( expert_emails.flat_map do |email| user = User.create_or_promote_to_expert(email, SecureRandom.hex) @@ -47,7 +48,8 @@ module CreateAvisConcern persisted.each do |avis| avis.dossier.demander_un_avis!(avis) if avis.dossier == dossier - AvisMailer.avis_invitation(avis).deliver_later + targeted_link = TargetedUserLink.create!(target_context: 'avis', target_model_type: Avis.name, target_model_id: avis.id, user: avis.expert.user) + AvisMailer.avis_invitation(avis, targeted_link).deliver_later sent_emails_addresses << avis.expert.email # the email format is already verified, we update value to nil avis.update_column(:email, nil) diff --git a/app/controllers/targeted_user_links_controller.rb b/app/controllers/targeted_user_links_controller.rb new file mode 100644 index 000000000..bb5fa1b27 --- /dev/null +++ b/app/controllers/targeted_user_links_controller.rb @@ -0,0 +1,16 @@ +class TargetedUserLinksController < ApplicationController + before_action :set_targeted_user_link, only: [:show] + def show + if @targeted_user_link.invalid_signed_in_user?(current_user) + render + else + redirect_to @targeted_user_link.redirect_url(Rails.application.routes.url_helpers) + end + end + + private + + def set_targeted_user_link + @targeted_user_link = TargetedUserLink.find_by(id: params[:id]) + end +end diff --git a/app/mailers/avis_mailer.rb b/app/mailers/avis_mailer.rb index d21053e5a..02e00f28b 100644 --- a/app/mailers/avis_mailer.rb +++ b/app/mailers/avis_mailer.rb @@ -4,10 +4,11 @@ class AvisMailer < ApplicationMailer layout 'mailers/layout' - def avis_invitation(avis) + def avis_invitation(avis, targeted_user_link) if avis.dossier.visible_by_administration? @avis = avis email = @avis.expert&.email + @url = targeted_user_link_url(targeted_user_link) subject = "Donnez votre avis sur le dossier nº #{@avis.dossier.id} (#{@avis.dossier.procedure.libelle})" mail(to: email, subject: subject) diff --git a/app/models/avis.rb b/app/models/avis.rb index 0015aa991..183f52c73 100644 --- a/app/models/avis.rb +++ b/app/models/avis.rb @@ -27,6 +27,8 @@ class Avis < ApplicationRecord has_one :expert, through: :experts_procedure has_one :procedure, through: :experts_procedure + has_many :targeted_user_links, dependent: :destroy, inverse_of: :target_model, foreign_key: 'target_model_id' + FILE_MAX_SIZE = 20.megabytes validates :piece_justificative_file, content_type: AUTHORIZED_CONTENT_TYPES, diff --git a/app/models/targeted_user_link.rb b/app/models/targeted_user_link.rb new file mode 100644 index 000000000..bb397d30e --- /dev/null +++ b/app/models/targeted_user_link.rb @@ -0,0 +1,32 @@ +# == Schema Information +# +# Table name: targeted_user_links +# +# id :uuid not null, primary key +# target_context :string not null +# target_model_type :string not null +# created_at :datetime not null +# updated_at :datetime not null +# target_model_id :bigint not null +# user_id :bigint not null +# +class TargetedUserLink < ApplicationRecord + belongs_to :user + belongs_to :target_model, polymorphic: true, optional: false + + enum target_context: { :avis => 'avis' } + + def invalid_signed_in_user?(signed_in_user) + signed_in_user && signed_in_user != self.user + end + + def redirect_url(url_helper) + case target_context + when "avis" + avis = target_model + avis.expert.user.active? ? + url_helper.expert_avis_path(avis.procedure, avis) : + url_helper.sign_up_expert_avis_path(avis.procedure, avis, email: avis.expert.email) + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index f5bc350e3..5f2f63758 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -44,6 +44,7 @@ class User < ApplicationRecord self.ignored_columns = [:administrateur_id, :instructeur_id, :expert_id] has_many :dossiers, dependent: :destroy + has_many :targeted_user_links, dependent: :destroy has_many :invites, dependent: :destroy has_many :dossiers_invites, through: :invites, source: :dossier has_many :deleted_dossiers @@ -219,6 +220,7 @@ class User < ApplicationRecord old_user.dossiers.update_all(user_id: id) old_user.invites.update_all(user_id: id) old_user.merge_logs.update_all(user_id: id) + old_user.targeted_user_links.update_all(user_id: id) # Move or merge old user's roles to the user [ diff --git a/app/views/avis_mailer/avis_invitation.html.haml b/app/views/avis_mailer/avis_invitation.html.haml index 319d0aa39..a4a6dafc0 100644 --- a/app/views/avis_mailer/avis_invitation.html.haml +++ b/app/views/avis_mailer/avis_invitation.html.haml @@ -1,9 +1,8 @@ - content_for(:title, 'Invitation à donner votre avis') -- avis_link = @avis.expert.user.active?.present? ? expert_avis_url(@avis.procedure, @avis) : sign_up_expert_avis_url(@avis.procedure, @avis.id, email: @avis.expert.email) - content_for(:footer) do Merci de ne pas répondre à cet email. Donnez votre avis - = link_to("sur #{APPLICATION_NAME}", avis_link) + = link_to("sur #{APPLICATION_NAME}", @url) ou = succeed '.' do = mail_to(@avis.claimant.email, "contactez la personne qui vous a invité") @@ -25,9 +24,9 @@ - if @avis.expert.user.active?.present? %p - = round_button("Donner votre avis", avis_link, :primary) + = round_button("Donner votre avis", @url, :primary) - else %p - = round_button("Inscrivez-vous pour donner votre avis", avis_link, :primary) + = round_button("Inscrivez-vous pour donner votre avis", @url, :primary) = render partial: "layouts/mailers/signature" diff --git a/app/views/targeted_user_links/show.html.haml b/app/views/targeted_user_links/show.html.haml new file mode 100644 index 000000000..a87414420 --- /dev/null +++ b/app/views/targeted_user_links/show.html.haml @@ -0,0 +1,19 @@ +- content_for(:title, "Whoops. Mauvais compte") + +.merci.text-center.mb-7 + .container + = image_tag('user/envoi-dossier.svg', alt: '', class: 'mt-8') + %h1.mt-4.mb-3.mx-0 Attention, ce lien ne vous est pas destiné + %p.send.m-2.text-lg + L'invitation est à destination de + %strong.test-target-email= @targeted_user_link.user.email + %br + Mais vous êtes connecté avec + %strong.test-current-email= current_user.email + %p.m-2 + Veuillez vous reconnecter avec la bonne adresse email + + .flex.column.align-center + = link_to "/users/sign_out", method: :delete, class: "button primary" do + %span.fa.fa-sign-out + Me déconnecter diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 692e07c58..51d0fa259 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -15,7 +15,7 @@ "type": "controller", "class": "Users::DossiersController", "method": "merci", - "line": 201, + "line": 206, "file": "app/controllers/users/dossiers_controller.rb", "rendered": { "name": "users/dossiers/merci", @@ -56,7 +56,7 @@ "type": "controller", "class": "FranceConnect::ParticulierController", "method": "merge", - "line": 48, + "line": 47, "file": "app/controllers/france_connect/particulier_controller.rb", "rendered": { "name": "france_connect/particulier/merge", @@ -92,6 +92,26 @@ "confidence": "Medium", "note": "" }, + { + "warning_type": "Redirect", + "warning_code": 18, + "fingerprint": "8396f884c5596d6c1148c7326b63e7e7bfabf9b0c1daf6b3ce0c1ff127b59cb4", + "check_name": "Redirect", + "message": "Possible unprotected redirect", + "file": "app/controllers/targeted_user_links_controller.rb", + "line": 7, + "link": "https://brakemanscanner.org/docs/warning_types/redirect/", + "code": "redirect_to(TargetedUserLink.find_by(:id => params[:id]).redirect_url(Rails.application.routes.url_helpers))", + "render_path": null, + "location": { + "type": "method", + "class": "TargetedUserLinksController", + "method": "show" + }, + "user_input": "TargetedUserLink.find_by(:id => params[:id]).redirect_url(Rails.application.routes.url_helpers)", + "confidence": "High", + "note": "" + }, { "warning_type": "SQL Injection", "warning_code": 0, @@ -119,7 +139,7 @@ "check_name": "Redirect", "message": "Possible unprotected redirect", "file": "app/controllers/instructeurs/procedures_controller.rb", - "line": 191, + "line": 168, "link": "https://brakemanscanner.org/docs/warning_types/redirect/", "code": "redirect_to(Export.find_or_create_export(export_format, current_instructeur.groupe_instructeurs.where(:procedure => procedure), **export_options).file.service_url)", "render_path": null, @@ -133,6 +153,6 @@ "note": "" } ], - "updated": "2022-04-05 14:21:07 +0200", - "brakeman_version": "5.1.1" + "updated": "2022-05-23 16:40:50 +0200", + "brakeman_version": "5.2.2" } diff --git a/config/routes.rb b/config/routes.rb index 766a08fe3..8dbc15950 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -113,6 +113,8 @@ Rails.application.routes.draw do get 'password_complexity' => 'password_complexity#show', as: 'show_password_complexity' + resources :targeted_user_links, only: [:show] + # # Main routes # diff --git a/db/migrate/20220520134041_enable_pgcrypto.rb b/db/migrate/20220520134041_enable_pgcrypto.rb new file mode 100644 index 000000000..3c62c3676 --- /dev/null +++ b/db/migrate/20220520134041_enable_pgcrypto.rb @@ -0,0 +1,10 @@ +class EnablePgcrypto < ActiveRecord::Migration[6.1] + # see: https://pawelurbanek.com/uuid-order-rails -> use uuid for id + def up + enable_extension "pgcrypto" + end + + def down + disable_extension "pgcrypto" + end +end diff --git a/db/migrate/20220520134042_create_targeted_user_links.rb b/db/migrate/20220520134042_create_targeted_user_links.rb new file mode 100644 index 000000000..0e570492c --- /dev/null +++ b/db/migrate/20220520134042_create_targeted_user_links.rb @@ -0,0 +1,13 @@ +class CreateTargetedUserLinks < ActiveRecord::Migration[6.1] + def change + # avoid target links with pk sequence + create_table :targeted_user_links, id: :uuid do |t| + t.string :target_context, null: false + t.bigint :target_model_id, null: false + t.string :target_model_type, null: false + t.references :user, foreign_key: true, null: false + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index f9469116c..75fa2e458 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -13,6 +13,7 @@ ActiveRecord::Schema.define(version: 2022_05_20_173939) do # These are extensions that must be enabled in order to support this database + enable_extension "pgcrypto" enable_extension "plpgsql" enable_extension "unaccent" @@ -751,6 +752,16 @@ ActiveRecord::Schema.define(version: 2022_05_20_173939) do t.index ["unlock_token"], name: "index_super_admins_on_unlock_token", unique: true end + create_table "targeted_user_links", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.datetime "created_at", precision: 6, null: false + t.string "target_context", null: false + t.bigint "target_model_id", null: false + t.string "target_model_type", null: false + t.datetime "updated_at", precision: 6, null: false + t.bigint "user_id", null: false + t.index ["user_id"], name: "index_targeted_user_links_on_user_id" + end + create_table "task_records", id: false, force: :cascade do |t| t.string "version", null: false end @@ -901,6 +912,7 @@ ActiveRecord::Schema.define(version: 2022_05_20_173939) do add_foreign_key "received_mails", "procedures" add_foreign_key "refused_mails", "procedures" add_foreign_key "services", "administrateurs" + add_foreign_key "targeted_user_links", "users" add_foreign_key "traitements", "dossiers" add_foreign_key "trusted_device_tokens", "instructeurs" add_foreign_key "types_de_champ", "procedure_revisions", column: "revision_id" diff --git a/spec/controllers/experts/avis_controller_spec.rb b/spec/controllers/experts/avis_controller_spec.rb index 655d21b63..c702f027a 100644 --- a/spec/controllers/experts/avis_controller_spec.rb +++ b/spec/controllers/experts/avis_controller_spec.rb @@ -285,6 +285,7 @@ describe Experts::AvisController, type: :controller do it { expect(flash.alert).to eq(["toto.fr : Email n'est pas valide"]) } it { expect(flash.notice).to eq("Une demande d’avis a été envoyée à titi@titimail.com") } it { expect(Avis.count).to eq(old_avis_count + 1) } + it { expect(TargetedUserLink.where(target_model: Avis.joins(expert: :user).where(user: { email: 'titi@titimail.com' })).count).to eq(1) } end context 'when the previous avis is public' do diff --git a/spec/controllers/targeted_user_links_controller_spec.rb b/spec/controllers/targeted_user_links_controller_spec.rb new file mode 100644 index 000000000..7e7c976ca --- /dev/null +++ b/spec/controllers/targeted_user_links_controller_spec.rb @@ -0,0 +1,54 @@ +describe TargetedUserLinksController, type: :controller do + describe '#show' do + let!(:expert) { create(:expert, user: expert_user) } + let!(:avis) { create(:avis, experts_procedure: expert_procedure) } + let!(:expert_procedure) { create(:experts_procedure, expert: expert) } + let!(:targeted_user_link) { create(:targeted_user_link, target_context: :avis, target_model: avis, user: expert_user) } + + context 'not connected as active expert' do + let(:expert_user) { create(:user, last_sign_in_at: 2.days.ago) } + + before { get :show, params: { id: targeted_user_link.id } } + + it 'redirects to expert_avis_url' do + expect(response).to redirect_to(expert_avis_path(avis.procedure, avis)) + end + end + + context 'not connected as inactive expert' do + let(:expert_user) { create(:user, last_sign_in_at: nil) } + + before { get :show, params: { id: targeted_user_link.id } } + + it 'redirects to sign_up_expert_avis_url' do + expect(response).to redirect_to(sign_up_expert_avis_path(avis.procedure, avis, email: expert_user.email)) + end + end + + context 'connected as expected user' do + let(:expert_user) { create(:user, last_sign_in_at: 2.days.ago) } + + before do + sign_in(targeted_user_link.user) + get :show, params: { id: targeted_user_link.id } + end + + it 'redirects to expert_avis_url' do + expect(response).to redirect_to(expert_avis_path(avis.procedure, avis)) + end + end + + context 'connected as different user' do + let(:expert_user) { create(:user, last_sign_in_at: 2.days.ago) } + + before do + sign_in(create(:expert).user) + get :show, params: { id: targeted_user_link.id } + end + + it 'renders error page ' do + expect(response).to have_http_status(200) + end + end + end +end diff --git a/spec/factories/targeted_user_links.rb b/spec/factories/targeted_user_links.rb new file mode 100644 index 000000000..1e7e4a0cc --- /dev/null +++ b/spec/factories/targeted_user_links.rb @@ -0,0 +1,12 @@ +FactoryBot.define do + factory :targeted_user_link do + target_context { TargetedUserLink.target_contexts[:avis] } + target_model { create(:avis) } + transient do + user {} + end + after(:build) do |targeted_user_link| + targeted_user_link.user = targeted_user_link.target_model.expert.user + end + end +end diff --git a/spec/mailers/avis_mailer_spec.rb b/spec/mailers/avis_mailer_spec.rb index af0bedd97..0e82e1234 100644 --- a/spec/mailers/avis_mailer_spec.rb +++ b/spec/mailers/avis_mailer_spec.rb @@ -5,17 +5,14 @@ RSpec.describe AvisMailer, type: :mailer do let(:dossier) { create(:dossier, :en_construction) } let(:experts_procedure) { create(:experts_procedure, expert: expert, procedure: dossier.procedure) } let(:avis) { create(:avis, dossier: dossier, claimant: claimant, experts_procedure: experts_procedure, introduction: 'intro') } + let(:targeted_user_link) { create(:targeted_user_link, target_context: :avis, target_model: avis, user: expert) } - subject { described_class.avis_invitation(avis.reload) } + subject { described_class.avis_invitation(avis.reload, targeted_user_link) } it { expect(subject.subject).to eq("Donnez votre avis sur le dossier nº #{avis.dossier.id} (#{avis.dossier.procedure.libelle})") } it { expect(subject.body).to have_text("Vous avez été invité par\r\n#{avis.claimant.email}\r\nà donner votre avis sur le dossier nº #{avis.dossier.id} de la démarche :\r\n#{avis.dossier.procedure.libelle}") } it { expect(subject.body).to include(avis.introduction) } - it { expect(subject.body).to include(instructeur_avis_url(avis.dossier.procedure.id, avis)) } - - context 'when the recipient is not already registered' do - it { expect(subject.body).to include(sign_up_expert_avis_url(avis.dossier.procedure.id, avis.id, email: avis.expert.email)) } - end + it { expect(subject.body).to include(targeted_user_link_url(targeted_user_link)) } context 'when the dossier has been deleted before the avis was sent' do before { dossier.update(hidden_by_user_at: 1.hour.ago) } diff --git a/spec/mailers/previews/avis_mailer_preview.rb b/spec/mailers/previews/avis_mailer_preview.rb index f9abd2c85..561616e1f 100644 --- a/spec/mailers/previews/avis_mailer_preview.rb +++ b/spec/mailers/previews/avis_mailer_preview.rb @@ -3,9 +3,10 @@ class AvisMailerPreview < ActionMailer::Preview def avis_invitation 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) + targeted_link = TargetedUserLink.build(target_model: avis, user: avis.expert.user, target_context: :avis) avis.dossier = Dossier.new(id: 1) avis.dossier.procedure = Procedure.new(libelle: 'Démarche pour faire des marches') avis.introduction = 'Il faudrait vérifier le certificat de conformité.' - AvisMailer.avis_invitation(avis) + AvisMailer.avis_invitation(avis, targeted_link) end end diff --git a/spec/models/targeted_user_link_spec.rb b/spec/models/targeted_user_link_spec.rb new file mode 100644 index 000000000..e615a3f08 --- /dev/null +++ b/spec/models/targeted_user_link_spec.rb @@ -0,0 +1,13 @@ +require 'rails_helper' + +RSpec.describe TargetedUserLink, type: :model do + describe 'Validation' do + let(:targeted_user_link) { build(:targeted_user_link) } + + context 'target_context' do + it 'is bullet proof' do + expect { targeted_user_link.target_context = :kc }.to raise_error(ArgumentError, "'kc' is not a valid target_context") + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 088f2bcd6..a9715c397 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -476,5 +476,17 @@ describe User, type: :model do end end end + + context 'and the old account had targeted_user_links' do + let(:expert) { create(:expert, user: old_user) } + let(:expert_procedure) { create(:experts_procedure, expert: expert) } + let!(:targeted_user_link) { create(:targeted_user_link, user: old_user, target_model: create(:avis, experts_procedure: expert_procedure)) } + + it 'transfers the targeted_user_link' do + subject + targeted_user.reload + expect(targeted_user.targeted_user_links).to include(targeted_user_link) + end + end end end diff --git a/spec/system/instructeurs/expert_spec.rb b/spec/system/instructeurs/expert_spec.rb index 8ac292d13..2631f3248 100644 --- a/spec/system/instructeurs/expert_spec.rb +++ b/spec/system/instructeurs/expert_spec.rb @@ -46,11 +46,10 @@ describe 'Inviting an expert:', js: true do expect(Avis.count).to eq(4) expect(emails_sent_to(expert.email.to_s).size).to eq(1) expect(emails_sent_to(expert2.email.to_s).size).to eq(1) - invitation_email = open_email(expert.email.to_s) - avis = expert.avis.find_by(dossier: dossier) - sign_up_link = sign_up_expert_avis_path(avis.dossier.procedure, avis, email: avis.expert.email) - expect(invitation_email.body).to include(sign_up_link) + targeted_user_link = TargetedUserLink.joins(:user).where(user: { email: expert.email.to_s }).first + targeted_user_url = targeted_user_link_url(targeted_user_link) + expect(invitation_email.body).to include(targeted_user_url) end context 'when experts submitted their answer' do