Merge pull request #7462 from betagouv/US/invite-with-targeted-user-link

feat(invite): wrap invitations with targeted_user_links

app/controllers/invites_controller.rb

@@ -5,7 +5,6 @@ class InvitesController < ApplicationController
   def create
     email = params[:invite_email].downcase
     @dossier = current_user.dossiers.visible_by_user.find(params[:dossier_id])
-
     invite = Invite.create(
       dossier: @dossier,
       user: User.find_by(email: email),

app/controllers/targeted_user_links_controller.rb

@@ -11,6 +11,6 @@ class TargetedUserLinksController < ApplicationController
   private
 
   def set_targeted_user_link
-    @targeted_user_link = TargetedUserLink.find_by(id: params[:id])
+    @targeted_user_link = TargetedUserLink.find(params[:id])
   end
 end

app/mailers/invite_mailer.rb

@@ -4,7 +4,10 @@ class InviteMailer < ApplicationMailer
 
   def invite_user(invite)
     subject = "Participez à l'élaboration d’un dossier"
-
+    targeted_user_link = invite.targeted_user_link || invite.create_targeted_user_link(target_context: 'invite',
+                                                                                       target_model: invite,
+                                                                                       user: invite.user)
+    @url = targeted_user_link_url(targeted_user_link)
     if invite.user.present?
       send_mail(invite, subject, invite.email_sender)
     end
@@ -12,6 +15,9 @@ class InviteMailer < ApplicationMailer
 
   def invite_guest(invite)
     subject = "#{invite.email_sender} vous invite à consulter un dossier"
+    targeted_user_link = invite.targeted_user_link || invite.create_targeted_user_link(target_context: 'invite',
+                                                                                       target_model: invite)
+    @url = targeted_user_link_url(targeted_user_link)
 
     send_mail(invite, subject, invite.email_sender)
   end

app/models/invite.rb

@@ -16,6 +16,7 @@ class Invite < ApplicationRecord
 
   belongs_to :dossier, optional: false
   belongs_to :user, optional: true
+  has_one :targeted_user_link, dependent: :destroy, inverse_of: :target_model, foreign_key: 'target_model_id'
 
   before_validation -> { sanitize_email(:email) }
 

app/models/targeted_user_link.rb

@@ -8,20 +8,36 @@
 #  created_at        :datetime         not null
 #  updated_at        :datetime         not null
 #  target_model_id   :bigint           not null
-#  user_id           :bigint           not null
+#  user_id           :bigint
 #
 class TargetedUserLink < ApplicationRecord
-  belongs_to :user
+  belongs_to :user, optional: true
   belongs_to :target_model, polymorphic: true, optional: false
 
-  enum target_context: { :avis => 'avis' }
+  enum target_context: { avis: 'avis', invite: 'invite' }
 
   def invalid_signed_in_user?(signed_in_user)
     signed_in_user && signed_in_user != self.user
   end
 
+  def target_email
+    case target_context
+    when 'avis'
+      user.email
+    when 'invite'
+      target_model.user&.email || target_model.email
+    else
+      raise 'invalid target_context'
+    end
+  end
+
   def redirect_url(url_helper)
     case target_context
+    when "invite"
+      invite = target_model
+      invite.user&.active? ?
+      url_helper.invite_path(invite) :
+      url_helper.invite_path(invite, params: { email: invite.email })
     when "avis"
       avis = target_model
       avis.expert.user.active? ?

app/views/invite_mailer/invite_guest.html.haml

@@ -20,7 +20,6 @@
   Afin de répondre à cette invitation, merci de vous inscrire avec l'adresse email
   = @invite.email
   sur
-  - url_for_link = invite_url(@invite, params: { email: @invite.email })
+  = link_to @url, @url
-  = link_to url_for_link, url_for_link
 
 = render partial: "layouts/mailers/signature"

app/views/invite_mailer/invite_user.html.haml

@@ -13,7 +13,6 @@
 
 %p
   Pour le consulter, merci de suivre ce lien :
-  - url_for_link = invite_url(@invite)
+  = link_to @url, @url
-  = link_to url_for_link, url_for_link
 
 = render partial: "layouts/mailers/signature"

app/views/targeted_user_links/show.html.haml

@@ -6,7 +6,7 @@
     %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
+      %strong.test-target-email= @targeted_user_link.target_email
       %br
       Mais vous êtes connecté avec
       %strong.test-current-email= current_user.email

config/brakeman.ignore

@@ -15,7 +15,7 @@
           "type": "controller",
           "class": "Users::DossiersController",
           "method": "merci",
-          "line": 206,
+          "line": 204,
           "file": "app/controllers/users/dossiers_controller.rb",
           "rendered": {
             "name": "users/dossiers/merci",
@@ -95,20 +95,20 @@
     {
       "warning_type": "Redirect",
       "warning_code": 18,
-      "fingerprint": "8396f884c5596d6c1148c7326b63e7e7bfabf9b0c1daf6b3ce0c1ff127b59cb4",
+      "fingerprint": "589e457617e6ad3d35f454b4af8ed326817930d6a724febcf680d7e03c6aeaa2",
       "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))",
+      "code": "redirect_to(TargetedUserLink.find(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)",
+      "user_input": "TargetedUserLink.find(params[:id]).redirect_url(Rails.application.routes.url_helpers)",
       "confidence": "High",
       "note": ""
     },
@@ -119,7 +119,7 @@
       "check_name": "SQL",
       "message": "Possible SQL injection",
       "file": "app/models/concerns/dossier_filtering_concern.rb",
-      "line": 18,
+      "line": 25,
       "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
       "code": "where(\"#{values.count} OR #{\"(#{ProcedurePresentation.sanitized_column(table, column)} ILIKE ?)\"}\", *values.map do\n \"%#{value}%\"\n end)",
       "render_path": null,
@@ -153,6 +153,6 @@
       "note": ""
     }
   ],
-  "updated": "2022-05-23 16:40:50 +0200",
+  "updated": "2022-06-15 16:15:28 +0200",
   "brakeman_version": "5.2.2"
 }

db/migrate/20220613130322_change_column_null_for_targeted_user_links.rb

@@ -0,0 +1,5 @@
+class ChangeColumnNullForTargetedUserLinks < ActiveRecord::Migration[6.1]
+  def change
+    safety_assured { change_column_null :targeted_user_links, :user_id, true }
+  end
+end

db/migrate/20220617142759_add_target_model_id_index_to_targeted_user_links.rb

@@ -0,0 +1,8 @@
+class AddTargetModelIdIndexToTargetedUserLinks < ActiveRecord::Migration[6.1]
+  include Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :targeted_user_links, :target_model_id
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_05_31_100040) do
+ActiveRecord::Schema.define(version: 2022_06_17_142759) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
@@ -758,7 +758,8 @@ ActiveRecord::Schema.define(version: 2022_05_31_100040) do
     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.bigint "user_id"
+    t.index ["target_model_id"], name: "index_targeted_user_links_on_target_model_id"
     t.index ["user_id"], name: "index_targeted_user_links_on_user_id"
   end
 

spec/controllers/targeted_user_links_controller_spec.rb

@@ -1,32 +1,35 @@
 describe TargetedUserLinksController, type: :controller do
   describe '#show' do
-    let!(:expert) { create(:expert, user: expert_user) }
+    let!(:targeted_user_link) { create(:targeted_user_link, target_context: target_context, target_model: target_model, user: user) }
-    let!(:avis) { create(:avis, experts_procedure: expert_procedure) }
+
+    context 'avis' do
+      let(:target_context) { :avis }
+      let!(:expert) { create(:expert, user: user) }
+      let!(:target_model) { 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) }
+        let(: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))
+          expect(response).to redirect_to(expert_avis_path(target_model.procedure, target_model))
         end
       end
 
       context 'not connected as inactive expert' do
-      let(:expert_user) { create(:user, last_sign_in_at: nil) }
+        let(: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))
+          expect(response).to redirect_to(sign_up_expert_avis_path(target_model.procedure, target_model, email: user.email))
         end
       end
 
       context 'connected as expected user' do
-      let(:expert_user) { create(:user, last_sign_in_at: 2.days.ago) }
+        let(:user) { create(:user, last_sign_in_at: 2.days.ago) }
 
         before do
           sign_in(targeted_user_link.user)
@@ -34,12 +37,12 @@ describe TargetedUserLinksController, type: :controller do
         end
 
         it 'redirects to expert_avis_url' do
-        expect(response).to redirect_to(expert_avis_path(avis.procedure, avis))
+          expect(response).to redirect_to(expert_avis_path(target_model.procedure, target_model))
         end
       end
 
       context 'connected as different user' do
-      let(:expert_user) { create(:user, last_sign_in_at: 2.days.ago) }
+        let(:user) { create(:user, last_sign_in_at: 2.days.ago) }
 
         before do
           sign_in(create(:expert).user)
@@ -51,4 +54,42 @@ describe TargetedUserLinksController, type: :controller do
         end
       end
     end
+
+    context 'with invite' do
+      let(:target_context) { 'invite' }
+      let(:target_model) { create(:invite, user: user) }
+
+      context 'connected with expected user' do
+        let(:user) { build(: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 'works' do
+          expect(response).to redirect_to(invite_path(target_model))
+        end
+      end
+
+      context 'connected as different user' do
+        let(: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
+
+      context 'when invite user does not exists' do
+        let(:user) { nil }
+        before { get :show, params: { id: targeted_user_link.id } }
+        it 'works' do
+          expect(response).to redirect_to(invite_path(target_model, email: target_model.email))
+        end
+      end
+    end
+  end
 end

spec/factories/targeted_user_links.rb

@@ -6,7 +6,12 @@ FactoryBot.define do
       user {}
     end
     after(:build) do |targeted_user_link|
+      case targeted_user_link.target_context
+      when 'avis'
         targeted_user_link.user = targeted_user_link.target_model.expert.user
+      when 'invite'
+        targeted_user_link.user = targeted_user_link.target_model&.user
+      end
     end
   end
 end

spec/mailers/invite_mailer_spec.rb

@@ -0,0 +1,51 @@
+RSpec.describe InviteMailer, type: :mailer do
+  let(:deliver) { mailer.deliver_now }
+
+  describe '.invite_user' do
+    let(:mailer) { InviteMailer.invite_user(invite) }
+
+    let(:invite) { create(:invite, user: create(:user)) }
+    it 'creates a target_user_link' do
+      expect { deliver }
+        .to change { TargetedUserLink.where(target_model: invite, user: invite.user).count }
+        .from(0).to(1)
+    end
+
+    context 'when it fails' do
+      it 'creates only one target_user_link' do
+         send_mail_values = [:raise, true]
+         allow_any_instance_of(InviteMailer).to receive(:send_mail) do
+           v = send_mail_values.shift
+           v == :raise ? raise("boom") : v
+         end
+         mailer.body rescue nil
+         mailer.body
+         expect(TargetedUserLink.where(target_model: invite, user: invite.user).count).to eq(1)
+       end
+    end
+  end
+
+  describe '.invite_guest' do
+    let(:mailer) { InviteMailer.invite_guest(invite) }
+    let(:invite) { create(:invite, user: nil, email: 'kikoo@lol.fr') }
+
+    it 'creates a target_user_link' do
+      expect { deliver }
+        .to change { TargetedUserLink.where(target_model: invite, user: nil).count }
+        .from(0).to(1)
+    end
+
+    context 'when it fails' do
+      it 'creates only one target_user_link' do
+         send_mail_values = [:raise, true]
+         allow_any_instance_of(InviteMailer).to receive(:send_mail) do
+           v = send_mail_values.shift
+           v == :raise ? raise("boom") : v
+         end
+         mailer.body rescue nil
+         mailer.body
+         expect(TargetedUserLink.where(target_model: invite, user: invite.user).count).to eq(1)
+       end
+    end
+  end
+end

spec/models/targeted_user_link_spec.rb

@@ -4,7 +4,7 @@ RSpec.describe TargetedUserLink, type: :model do
   describe 'Validation' do
     let(:targeted_user_link) { build(:targeted_user_link) }
 
-    context 'target_context' do
+    describe '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

spec/system/users/invite_spec.rb

@@ -14,6 +14,7 @@ describe 'Invitations' do
       navigate_to_brouillon(dossier)
 
       fill_in 'Texte obligatoire', with: 'Some edited value'
+
       send_invite_to "user_invite@exemple.fr"
 
       expect(page).to have_current_path(brouillon_dossier_path(dossier))
@@ -26,23 +27,72 @@ describe 'Invitations' do
 
     context 'when inviting someone without an existing account' do
       let(:invite) { create(:invite, dossier: dossier, user: nil) }
-      let(:user_password) { 'my-s3cure-p4ssword' }
 
-      scenario 'an invited user can register using the registration link sent in the invitation email' do
+      scenario 'an invited user can register using the targeted_user_link sent in the invitation email thru the ' do
-        # Click the invitation link
+        log_in(owner)
-        visit invite_path(invite, params: { email: invite.email })
+        navigate_to_brouillon(dossier)
-        expect(page).to have_current_path(new_user_registration_path, ignore_query: true)
-        expect(page).to have_field('user_email', with: invite.email)
 
-        # Create the account
+        fill_in 'Texte obligatoire', with: 'Some edited value'
-        sign_up_with invite.email, user_password
-        expect(page).to have_content('lien d’activation')
 
-        # Confirm the account
+        send_invite_to "user_invite@exemple.fr"
-        # (The user should be redirected to the dossier they was invited on)
+
-        click_confirmation_link_for invite.email
+        expect {
-        expect(page).to have_content('Votre compte a bien été confirmé.')
+          perform_enqueued_jobs
-        expect(page).to have_current_path(brouillon_dossier_path(dossier))
+        }.to change { TargetedUserLink.count }.from(0).to(1)
+
+        invitation_email = open_email("user_invite@exemple.fr")
+        targeted_user_link = TargetedUserLink.last
+        expect(invitation_email).to have_link(targeted_user_link_url(targeted_user_link))
+        invitation_email.click_on targeted_user_link_url(targeted_user_link)
+        expect(page).to have_current_path("/users/sign_up?user%5Bemail%5D=user_invite%40exemple.fr")
+      end
+    end
+
+    context 'when inviting someone with an existing account' do
+      let(:user) { create(:user) }
+
+      scenario 'an invited user can sign in using the targeted_user_link link sent in the invitation email' do
+        log_in(owner)
+        navigate_to_brouillon(dossier)
+
+        fill_in 'Texte obligatoire', with: 'Some edited value'
+        send_invite_to user.email
+
+        expect {
+          perform_enqueued_jobs
+        }.to change { TargetedUserLink.count }.from(0).to(1)
+
+        invitation_email = open_email(user.email)
+        targeted_user_link = TargetedUserLink.last
+        expect(invitation_email).to have_link(targeted_user_link_url(targeted_user_link))
+        invitation_email.click_on targeted_user_link_url(targeted_user_link)
+        expect(page).to have_current_path("/users/sign_in")
+      end
+    end
+
+    context 'when visiting targeted_user_link having an invite without user, but signed with another account' do
+      let(:invite) { create(:invite, user: nil, email: 'target@email.com') }
+      let!(:targeted_user_link) { create(:targeted_user_link, target_context: 'invite', target_model: invite) }
+      let!(:another_user) { create(:user) }
+
+      scenario 'the connected user is alterted he is not using the expected account' do
+        log_in(another_user)
+        visit targeted_user_link_path(targeted_user_link)
+        expect(page).to have_current_path(targeted_user_link_path(targeted_user_link))
+        expect(page).to have_content("L'invitation est à destination de #{targeted_user_link.target_email}")
+      end
+    end
+
+    context 'when visiting targeted_user_link having an invite with user, but signed with another account' do
+      let(:invite) { create(:invite, user: create(:user)) }
+      let!(:targeted_user_link) { create(:targeted_user_link, target_context: 'invite', target_model: invite, user: invite.user) }
+      let!(:another_user) { create(:user) }
+
+      scenario 'the connected user is alterted he is not using the expected account' do
+        log_in(another_user)
+        visit targeted_user_link_path(targeted_user_link)
+        expect(page).to have_current_path(targeted_user_link_path(targeted_user_link))
+        expect(page).to have_content("L'invitation est à destination de #{targeted_user_link.target_email}")
       end
     end