diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index d88923567..138b2e4fb 100644 --- a/app/controllers/invites_controller.rb +++ b/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), diff --git a/app/controllers/targeted_user_links_controller.rb b/app/controllers/targeted_user_links_controller.rb index bb5fa1b27..299fe6abf 100644 --- a/app/controllers/targeted_user_links_controller.rb +++ b/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 diff --git a/app/mailers/invite_mailer.rb b/app/mailers/invite_mailer.rb index 4764cea52..541798cef 100644 --- a/app/mailers/invite_mailer.rb +++ b/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 diff --git a/app/models/invite.rb b/app/models/invite.rb index 28d1e3038..3b9031856 100644 --- a/app/models/invite.rb +++ b/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) } diff --git a/app/models/targeted_user_link.rb b/app/models/targeted_user_link.rb index bb397d30e..9420715ca 100644 --- a/app/models/targeted_user_link.rb +++ b/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? ? diff --git a/app/views/invite_mailer/invite_guest.html.haml b/app/views/invite_mailer/invite_guest.html.haml index 2c0ba2874..6ed9c7430 100644 --- a/app/views/invite_mailer/invite_guest.html.haml +++ b/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_for_link, url_for_link + = link_to @url, @url = render partial: "layouts/mailers/signature" diff --git a/app/views/invite_mailer/invite_user.html.haml b/app/views/invite_mailer/invite_user.html.haml index f11a95bfc..ecde24697 100644 --- a/app/views/invite_mailer/invite_user.html.haml +++ b/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_for_link, url_for_link + = link_to @url, @url = 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 index a87414420..60b037419 100644 --- a/app/views/targeted_user_links/show.html.haml +++ b/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 diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 51d0fa259..e4015fcaa 100644 --- a/config/brakeman.ignore +++ b/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" } diff --git a/db/migrate/20220613130322_change_column_null_for_targeted_user_links.rb b/db/migrate/20220613130322_change_column_null_for_targeted_user_links.rb new file mode 100644 index 000000000..d41534658 --- /dev/null +++ b/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 diff --git a/db/migrate/20220617142759_add_target_model_id_index_to_targeted_user_links.rb b/db/migrate/20220617142759_add_target_model_id_index_to_targeted_user_links.rb new file mode 100644 index 000000000..7ce0a9f09 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 727ce85b3..290368762 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/controllers/targeted_user_links_controller_spec.rb b/spec/controllers/targeted_user_links_controller_spec.rb index 7e7c976ca..7e32facb3 100644 --- a/spec/controllers/targeted_user_links_controller_spec.rb +++ b/spec/controllers/targeted_user_links_controller_spec.rb @@ -1,53 +1,94 @@ 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) } + let!(:targeted_user_link) { create(:targeted_user_link, target_context: target_context, target_model: target_model, user: user) } - context 'not connected as active expert' do - let(:expert_user) { create(:user, last_sign_in_at: 2.days.ago) } + 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) } - before { get :show, params: { id: targeted_user_link.id } } + context 'not connected as active expert' do + let(:user) { create(:user, last_sign_in_at: 2.days.ago) } - it 'redirects to expert_avis_url' do - expect(response).to redirect_to(expert_avis_path(avis.procedure, avis)) + before { get :show, params: { id: targeted_user_link.id } } + + it 'redirects to expert_avis_url' do + expect(response).to redirect_to(expert_avis_path(target_model.procedure, target_model)) + end + end + + context 'not connected as inactive expert' do + 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(target_model.procedure, target_model, email: user.email)) + end + end + + context 'connected as expected user' do + let(: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(target_model.procedure, 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 end - context 'not connected as inactive expert' do - let(:expert_user) { create(:user, last_sign_in_at: nil) } + context 'with invite' do + let(:target_context) { 'invite' } + let(:target_model) { create(:invite, user: user) } - 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 } + 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 - 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(:user) { create(:user, last_sign_in_at: 2.days.ago) } - 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 - before do - sign_in(create(:expert).user) - get :show, params: { id: targeted_user_link.id } + it 'renders error page ' do + expect(response).to have_http_status(200) + end end - it 'renders error page ' do - expect(response).to have_http_status(200) + 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 diff --git a/spec/factories/targeted_user_links.rb b/spec/factories/targeted_user_links.rb index 1e7e4a0cc..5a130bb31 100644 --- a/spec/factories/targeted_user_links.rb +++ b/spec/factories/targeted_user_links.rb @@ -6,7 +6,12 @@ FactoryBot.define do user {} end after(:build) do |targeted_user_link| - targeted_user_link.user = targeted_user_link.target_model.expert.user + 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 diff --git a/spec/mailers/invite_mailer_spec.rb b/spec/mailers/invite_mailer_spec.rb new file mode 100644 index 000000000..ebce58778 --- /dev/null +++ b/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 diff --git a/spec/models/targeted_user_link_spec.rb b/spec/models/targeted_user_link_spec.rb index e615a3f08..bbc950141 100644 --- a/spec/models/targeted_user_link_spec.rb +++ b/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 diff --git a/spec/system/users/invite_spec.rb b/spec/system/users/invite_spec.rb index 9ece00f57..b79d27d96 100644 --- a/spec/system/users/invite_spec.rb +++ b/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 - # Click the invitation link - visit invite_path(invite, params: { email: invite.email }) - expect(page).to have_current_path(new_user_registration_path, ignore_query: true) - expect(page).to have_field('user_email', with: invite.email) + scenario 'an invited user can register using the targeted_user_link sent in the invitation email thru the ' do + log_in(owner) + navigate_to_brouillon(dossier) - # Create the account - sign_up_with invite.email, user_password - expect(page).to have_content('lien d’activation') + fill_in 'Texte obligatoire', with: 'Some edited value' - # Confirm the account - # (The user should be redirected to the dossier they was invited on) - click_confirmation_link_for invite.email - expect(page).to have_content('Votre compte a bien été confirmé.') - expect(page).to have_current_path(brouillon_dossier_path(dossier)) + send_invite_to "user_invite@exemple.fr" + + expect { + perform_enqueued_jobs + }.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