Merge pull request #7369 from betagouv/US/ensure_current_user_is_expert_email

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

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)

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

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)

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,

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

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
       [

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"

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

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

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
   #

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

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

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"

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

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

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

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) }

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

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

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

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