Merge pull request #6567 from betagouv/main

2021-10-20-01

Gemfile.lock

@@ -462,7 +462,7 @@ GEM
       ast (~> 2.4.1)
     pdf-core (0.9.0)
     pg (1.2.3)
-    phonelib (0.6.48)
+    phonelib (0.6.53)
     prawn (2.4.0)
       pdf-core (~> 0.9.0)
       ttfunk (~> 1.7)

app/controllers/application_controller.rb

@@ -72,7 +72,11 @@ class ApplicationController < ActionController::Base
   alias_method :pundit_user, :current_account
 
   def localization_enabled?
-    ENV.fetch('LOCALIZATION_ENABLED', 'false') == 'true' || cookies[:locale].present?
+    ENV.fetch('LOCALIZATION_ENABLED', 'false') == 'true' || cookies[:locale].present? || !browser_prefers_french?
+  end
+
+  def browser_prefers_french?
+    http_accept_language.compatible_language_from(I18n.available_locales) == 'fr'
   end
 
   def set_locale(locale)

app/controllers/france_connect/particulier_controller.rb

@@ -45,7 +45,7 @@ class FranceConnect::ParticulierController < ApplicationController
   def merge_with_existing_account
     user = User.find_by(email: sanitized_email_params)
 
-    if user.valid_for_authentication? { user.valid_password?(password_params) }
+    if user.present? && user.valid_for_authentication? { user.valid_password?(password_params) }
       if !user.can_france_connect?
         flash.alert = "#{user.email} ne peut utiliser FranceConnect"
 

app/controllers/manager/procedures_controller.rb

@@ -47,12 +47,22 @@ module Manager
     end
 
     def add_administrateur
-      administrateur = Administrateur.by_email(current_super_admin.email)
+      add_self = params[:email].blank?
+      administrateur_email = add_self ? current_super_admin.email : params[:email]
+      administrateur = Administrateur.by_email(administrateur_email)
       if administrateur
-        AdministrateursProcedure.create(procedure: procedure, administrateur: administrateur, manager: true)
-        flash[:notice] = "L’administrateur \"#{administrateur.email}\" est ajouté à la démarche pour la journée."
+        AdministrateursProcedure.create(procedure: procedure, administrateur: administrateur, manager: add_self)
+        if add_self
+          flash[:notice] = "L’administrateur \"#{administrateur_email}\" est ajouté à la démarche pour la journée."
+        else
+          flash[:notice] = "L’administrateur \"#{administrateur_email}\" est ajouté à la démarche."
+        end
       else
-        flash[:alert] = "Vous n’êtes pas connecté en tant qu’administrateur."
+        if add_self
+          flash[:alert] = "Vous n’êtes pas connecté en tant qu’administrateur."
+        else
+          flash[:alert] = "L’administrateur \"#{administrateur_email}\" est introuvable."
+        end
       end
       redirect_to manager_procedure_path(procedure)
     end

app/controllers/manager/users_controller.rb

@@ -2,11 +2,9 @@ module Manager
   class UsersController < Manager::ApplicationController
     def update
       user = User.find(params[:id])
-      preexisting_user = User.find_by(email: targeted_email)
+      targeted_user = User.find_by(email: targeted_email)
 
-      if user.administrateur.present?
-        flash[:error] = "« #{targeted_email} » est un administrateur. On ne sait pas encore faire."
-      elsif preexisting_user.nil?
+      if targeted_user.nil?
         user.skip_reconfirmation!
         user.update(email: targeted_email)
 
@@ -16,18 +14,18 @@ module Manager
           flash[:error] = user.errors.full_messages.to_sentence
         end
       else
-        user.dossiers.update_all(user_id: preexisting_user.id)
+        user.dossiers.update_all(user_id: targeted_user.id)
 
-        if preexisting_user.instructeur.nil?
-          user.instructeur&.update(user: preexisting_user)
-        else
-          preexisting_user.instructeur.merge(user.instructeur)
-        end
-
-        if preexisting_user.expert.nil?
-          user.expert&.update(user: preexisting_user)
-        else
-          preexisting_user.expert.merge(user.expert)
+        [
+          [user.instructeur, targeted_user.instructeur],
+          [user.expert, targeted_user.expert],
+          [user.administrateur, targeted_user.administrateur]
+        ].each do |old_role, targeted_role|
+          if targeted_role.nil?
+            old_role&.update(user: targeted_user)
+          else
+            targeted_role.merge(old_role)
+          end
         end
 
         flash[:notice] = "Le compte « #{targeted_email} » a absorbé le compte « #{user.email} »."

app/controllers/users/profil_controller.rb

@@ -1,9 +1,5 @@
 module Users
   class ProfilController < UserController
-    before_action :redirect_if_instructeur,
-      only: :update_email,
-      if: -> { instructeur_signed_in? }
-
     def show
       @waiting_transfers = current_user.dossiers.joins(:transfer).group('dossier_transfers.email').count.to_a
     end
@@ -15,7 +11,9 @@ module Users
     end
 
     def update_email
-      if current_user.update(update_email_params)
+      if current_user.instructeur? && !target_email_allowed?
+        flash.alert = t('.email_not_allowed', contact_email: CONTACT_EMAIL, requested_email: requested_email)
+      elsif current_user.update(update_email_params)
         flash.notice = t('devise.registrations.update_needs_confirmation')
       elsif current_user.errors&.details&.dig(:email)&.any? { |e| e[:error] == :taken }
         UserMailer.account_already_taken(current_user, requested_email).deliver_later
@@ -44,8 +42,8 @@ module Users
       update_email_params[:email]
     end
 
-    def redirect_if_instructeur
-      redirect_to profil_path
+    def target_email_allowed?
+      LEGIT_ADMIN_DOMAINS.any? { |d| requested_email.end_with?(d) }
     end
 
     def next_owner_email

app/models/administrateur.rb

@@ -96,6 +96,36 @@ class Administrateur < ApplicationRecord
     destroy
   end
 
+  def merge(old_admin)
+    return if old_admin.nil?
+
+    procedures_with_new_admin, procedures_without_new_admin = old_admin.procedures
+      .partition { |p| p.administrateurs.exists?(id) }
+
+    procedures_with_new_admin.each do |p|
+      p.administrateurs.delete(old_admin)
+    end
+
+    procedures_without_new_admin.each do |p|
+      p.administrateurs << self
+      p.administrateurs.delete(old_admin)
+    end
+
+    old_admin.services.update_all(administrateur_id: id)
+
+    instructeurs_with_new_admin, instructeurs_without_new_admin = old_admin.instructeurs
+      .partition { |i| i.administrateurs.exists?(id) }
+
+    instructeurs_with_new_admin.each do |i|
+      i.administrateurs.delete(old_admin)
+    end
+
+    instructeurs_without_new_admin.each do |i|
+      i.administrateurs << self
+      i.administrateurs.delete(old_admin)
+    end
+  end
+
   # required to display feature flags field in manager
   def features
   end

app/views/manager/procedures/show.html.erb

@@ -64,6 +64,10 @@ as well as a link to its edit page.
       <dd class="attribute-data attribute-data--<%=attribute.html_class%>">
         <%= render_field attribute, page: page %>
         <% if attribute.name == 'administrateurs' %>
+          <%= form_tag(add_administrateur_manager_procedure_path(procedure), style: 'margin-top: 1rem;') do %>
+            <%= email_field_tag(:email, '', placeholder: 'Email', autocapitalize: 'off', autocorrect: 'off', spellcheck: 'false', style: 'margin-bottom: 1rem;width:24rem;') %>
+            <button>Ajouter un administrateur</button>
+          <% end %>
           <% if procedure.administrateurs.find { |admin| admin.email == current_super_admin.email } %>
             <p style="margin-top: 20px;">Vous êtes déjà administrateur sur cette démarche</p>
           <% else %>

app/views/users/profil/show.html.haml

@@ -18,10 +18,16 @@
         %p
           Pour finaliser votre changement d’adresse, vérifiez vos emails et cliquez sur le lien de confirmation.
 
-    - if !instructeur_signed_in?
-      = form_for @current_user, url: update_email_path, method: :patch, html: { class: 'form' } do |f|
-        = f.email_field :email, value: nil, placeholder: 'Nouvelle adresse email', required: true
-        = f.submit "Changer mon adresse", class: 'button primary'
+    - if current_user.instructeur?
+      %p.mb-4
+        = t('.email_explications_html',
+          contact_email: CONTACT_EMAIL,
+          application_name: APPLICATION_NAME,
+          legit_admin_domains: LEGIT_ADMIN_DOMAINS.join(', '))
+
+    = form_for @current_user, url: update_email_path, method: :patch, html: { class: 'form' } do |f|
+      = f.email_field :email, value: nil, placeholder: 'Nouvelle adresse email', required: true
+      = f.submit "Changer mon adresse", class: 'button primary'
 
   - if !instructeur_signed_in?
     .card

config/env.example.optional

@@ -78,3 +78,6 @@ DS_ENV="staging"
 
 # API Particulier https://api.gouv.fr/les-api/api-particulier
 # API_PARTICULIER_URL="https://particulier.api.gouv.fr/api"
+
+# Les instructeurs et administrateurs peuvent changer leur email vers ces domaines
+# LEGIT_ADMIN_DOMAINS = "domaine_1.com;domaine_2.com"

config/initializers/legit_admin_domains.rb

@@ -0,0 +1,2 @@
+domains = ["gouv.fr", "sante.fr", "cnafmail.fr", "cnamts.fr", "cci.fr", "caf.fr", "msa.fr"]
+LEGIT_ADMIN_DOMAINS = ENV["LEGIT_ADMIN_DOMAINS"]&.split(';') || domains

config/locales/views/users/profil/fr.yml

@@ -10,6 +10,19 @@ fr:
           one: "Le nouveau propriétaire %{email} doit confirmer le transfert d'un dossier en suivant les instructions reçues dans son mail."
           other: "Le nouveau propriétaire %{email} doit confirmer le transfert de vos %{count} dossiers en suivant les instructions reçues dans son mail."
         transfer_confirmation: "Confirmez-vous le transfert ?"
+        email_explications_html: >
+          <b class="bold">%{application_name}</b> doit s'assurer que votre compte est utilisé dans un cadre professionnel.
+          <br>
+          <br>
+          Nous pouvons autoriser automatiquement les changements d'email vers les domaines suivants&nbsp;:
+          <br>
+          %{legit_admin_domains}
+          <br>
+          <br>
+          Si ce n'est pas votre cas, contactez le support&nbsp;:
+          <a href="mailto:%{contact_email}">%{contact_email}</a>
+      update_email:
+        email_not_allowed: "L’email %{requested_email} ne peut être utilisé, contactez le support : <a href='mailto:%{contact_email}'>%{contact_email}</a>"
       transfer_all_dossiers:
         new_transfer: 
           one: "Le transfert d'un dossier à %{email} est en cours"

spec/controllers/france_connect/particulier_controller_spec.rb

@@ -189,6 +189,17 @@ describe FranceConnect::ParticulierController, type: :controller do
 
     it_behaves_like "a method that needs a valid merge token"
 
+    context 'when the user is not found' do
+      it 'does not log' do
+        subject
+        fci.reload
+
+        expect(fci.user).to be_nil
+        expect(fci.merge_token).not_to be_nil
+        expect(controller.current_user).to be_nil
+      end
+    end
+
     context 'when the credentials are ok' do
       let!(:user) { create(:user, email: email, password: password) }
 

spec/controllers/manager/users_controller_spec.rb

@@ -17,7 +17,7 @@ describe Manager::UsersController, type: :controller do
   end
 
   describe '#update' do
-    let(:user) { create(:user, email: 'ancien.email@domaine.fr') }
+    let(:user) { create(:user, email: 'ancien.email@domaine.fr', password: '{My-$3cure-p4ssWord}') }
 
     subject { patch :update, params: { id: user.id, user: { email: nouvel_email } } }
 
@@ -30,17 +30,6 @@ describe Manager::UsersController, type: :controller do
 
           expect(User.find_by(id: user.id).email).to eq(nouvel_email)
         end
-
-        context 'and the user is an administrateur' do
-          let(:user) { create(:administrateur).user }
-
-          it 'rejects the modification' do
-            subject
-
-            expect(flash[:error]).to match("« nouvel.email@domaine.fr » est un administrateur. On ne sait pas encore faire.")
-            expect(user.reload.email).not_to eq(nouvel_email)
-          end
-        end
       end
 
       describe 'with an invalid email' do
@@ -56,8 +45,8 @@ describe Manager::UsersController, type: :controller do
     end
 
     context 'when the targeted email exists' do
-      let(:preexisting_user) { create(:user, email: 'email.existant@domaine.fr') }
-      let(:nouvel_email) { preexisting_user.email }
+      let(:targeted_user) { create(:user, email: 'email.existant@domaine.fr', password: '{My-$3cure-p4ssWord}') }
+      let(:nouvel_email) { targeted_user.email }
 
       context 'and the old account has a dossier' do
         let!(:dossier) { create(:dossier, user: user) }
@@ -65,63 +54,35 @@ describe Manager::UsersController, type: :controller do
         it 'transfers the dossier' do
           subject
 
-          expect(preexisting_user.dossiers).to match([dossier])
+          expect(targeted_user.dossiers).to match([dossier])
         end
       end
 
-      context 'and the old account belongs to an instructeur and expert' do
+      context 'and the old account belongs to an instructeur, expert and administrateur' do
         let!(:instructeur) { create(:instructeur, user: user) }
         let!(:expert) { create(:expert, user: user) }
+        let!(:administrateur) { create(:administrateur, user: user) }
 
         it 'transfers instructeur account' do
           subject
-          preexisting_user.reload
+          targeted_user.reload
 
-          expect(preexisting_user.instructeur).to match(instructeur)
-          expect(preexisting_user.expert).to match(expert)
+          expect(targeted_user.instructeur).to match(instructeur)
+          expect(targeted_user.expert).to match(expert)
+          expect(targeted_user.administrateur).to match(administrateur)
           expect(flash[:notice]).to match("Le compte « email.existant@domaine.fr » a absorbé le compte « ancien.email@domaine.fr ».")
         end
 
-        context 'and the preexisting account owns an instructeur and expert as well' do
-          let!(:preexisting_instructeur) { create(:instructeur, user: preexisting_user) }
-          let!(:preexisting_expert) { create(:expert, user: preexisting_user) }
+        context 'and the targeted account owns an instructeur and expert as well' do
+          let!(:targeted_instructeur) { create(:instructeur, user: targeted_user) }
+          let!(:targeted_expert) { create(:expert, user: targeted_user) }
+          let!(:targeted_administrateur) { create(:administrateur, user: targeted_user) }
 
-          context 'and the source instructeur has some procedures and dossiers' do
-            let!(:procedure) { create(:procedure, instructeurs: [instructeur]) }
-            let(:dossier) { create(:dossier) }
-            let(:administrateur) { create(:administrateur) }
-            let!(:commentaire) { create(:commentaire, instructeur: instructeur, dossier: dossier) }
-            let!(:bulk_message) { BulkMessage.create!(instructeur: instructeur, body: 'body', sent_at: Time.zone.now) }
-
-            before do
-              user.instructeur.followed_dossiers << dossier
-              user.instructeur.administrateurs << administrateur
-            end
-
-            it 'transferts all the stuff' do
-              subject
-              preexisting_user.reload
-
-              expect(procedure.instructeurs).to match([preexisting_user.instructeur])
-              expect(preexisting_user.instructeur.followed_dossiers).to match([dossier])
-              expect(preexisting_user.instructeur.administrateurs).to match([administrateur])
-              expect(preexisting_user.instructeur.commentaires).to match([commentaire])
-              expect(preexisting_user.instructeur.bulk_messages).to match([bulk_message])
-            end
-          end
-
-          context 'and the source expert has some avis and commentaires' do
-            let(:dossier) { create(:dossier) }
-            let(:experts_procedure) { create(:experts_procedure, expert: user.expert, procedure: dossier.procedure) }
-            let!(:avis) { create(:avis, dossier: dossier, claimant: create(:instructeur), experts_procedure: experts_procedure) }
-            let!(:commentaire) { create(:commentaire, expert: expert, dossier: dossier) }
-
-            it 'transfers the avis' do
-              subject
-
-              expect(preexisting_user.expert.avis).to match([avis])
-              expect(preexisting_user.expert.commentaires).to match([commentaire])
-            end
+          it 'merge the account' do
+            expect_any_instance_of(Instructeur).to receive(:merge)
+            expect_any_instance_of(Expert).to receive(:merge)
+            expect_any_instance_of(Administrateur).to receive(:merge)
+            subject
           end
         end
       end

spec/controllers/users/profil_controller_spec.rb

@@ -90,12 +90,24 @@ describe Users::ProfilController, type: :controller do
       let!(:user) { create(:instructeur, email: instructeur_email).user }
 
       before do
-        patch :update_email, params: { user: { email: 'loulou@lou.com' } }
+        patch :update_email, params: { user: { email: requested_email } }
         user.reload
       end
 
-      it { expect(user.unconfirmed_email).to be_nil }
-      it { expect(response).to redirect_to(profil_path) }
+      context 'when the requested email is allowed' do
+        let(:requested_email) { 'legit@gouv.fr' }
+
+        it { expect(user.unconfirmed_email).to eq('legit@gouv.fr') }
+        it { expect(response).to redirect_to(profil_path) }
+        it { expect(flash.notice).to eq(I18n.t('devise.registrations.update_needs_confirmation')) }
+      end
+
+      context 'when the requested email is not allowed' do
+        let(:requested_email) { 'weird@gmail.com' }
+
+        it { expect(response).to redirect_to(profil_path) }
+        it { expect(flash.alert).to include('contactez le support') }
+      end
     end
   end
 

spec/models/administrateur_spec.rb

@@ -75,4 +75,92 @@ describe Administrateur, type: :model do
       expect(Administrateur.find_by(id: administrateur.id)).to be_nil
     end
   end
+
+  describe '#merge' do
+    let(:new_admin) { create(:administrateur) }
+    let(:old_admin) { create(:administrateur) }
+
+    subject { new_admin.merge(old_admin) }
+
+    context 'when the old admin does not exist' do
+      let(:old_admin) { nil }
+
+      it { expect { subject }.not_to raise_error }
+    end
+
+    context 'when the old admin has a procedure' do
+      let(:procedure) { create(:procedure) }
+
+      before do
+        old_admin.procedures << procedure
+        subject
+        [new_admin, old_admin].map(&:reload)
+      end
+
+      it 'transfers the procedure' do
+        expect(new_admin.procedures).to match_array(procedure)
+        expect(old_admin.procedures).to be_empty
+      end
+    end
+
+    context 'when both admins share a procedure' do
+      let(:procedure) { create(:procedure) }
+
+      before do
+        new_admin.procedures << procedure
+        old_admin.procedures << procedure
+        subject
+        [new_admin, old_admin].map(&:reload)
+      end
+
+      it 'removes the procedure from the old one' do
+        expect(old_admin.procedures).to be_empty
+      end
+    end
+
+    context 'when the old admin has a service' do
+      let(:service) { create(:service, administrateur: old_admin) }
+
+      before do
+        service
+        subject
+        [new_admin, old_admin].map(&:reload)
+      end
+
+      it 'transfers the service' do
+        expect(new_admin.services).to match_array(service)
+      end
+    end
+
+    context 'when the old admin has an instructeur' do
+      let(:instructeur) { create(:instructeur) }
+
+      before do
+        old_admin.instructeurs << instructeur
+        subject
+        [new_admin, old_admin].map(&:reload)
+      end
+
+      it 'transfers the instructeur' do
+        expect(new_admin.instructeurs).to match_array(instructeur)
+        expect(old_admin.instructeurs).to be_empty
+      end
+    end
+
+    context 'when both admins share an instructeur' do
+      let(:instructeur) { create(:instructeur) }
+
+      before do
+        old_admin.instructeurs << instructeur
+        new_admin.instructeurs << instructeur
+        subject
+        [new_admin, old_admin].map(&:reload)
+      end
+
+      it 'transfers the instructeur' do
+        expect(new_admin.instructeurs).to match_array(instructeur)
+        expect(old_admin.instructeurs).to be_empty
+      end
+    end
+  end
 end

spec/support/capybara.rb

@@ -49,6 +49,20 @@ Capybara::Screenshot.register_driver :headless_chrome do |driver, path|
 end
 
 RSpec.configure do |config|
+  # Set the user preferred language before Javascript feature specs.
+  #
+  # Features specs without Javascript run in a Rack stack, and respect the Accept-Language value.
+  # However specs using Javascript are run into a Headless Chrome, which doesn't support setting
+  # the default Accept-Language value reliably.
+  # So instead we set the locale cookie explicitly before each Javascript test.
+  config.before(:each, js: true) do
+    visit '/' # Webdriver needs visiting a page before setting the cookie
+    Capybara.current_session.driver.browser.manage.add_cookie(
+      name: :locale,
+      value: Rails.application.config.i18n.default_locale
+    )
+  end
+
   # Examples tagged with :capybara_ignore_server_errors will allow Capybara
   # to continue when an exception in raised by Rails.
   # This allows to test for error cases.

spec/views/new_administrateur/procedures/show.html.haml_spec.rb

@@ -34,7 +34,7 @@ describe 'new_administrateur/procedures/show.html.haml', type: :view do
       render
     end
 
-    describe 'archive button is visible', js: true do
+    describe 'archive button is visible' do
       it { expect(rendered).not_to have_css('#publish-procedure-link') }
       it { expect(rendered).to have_css('#close-procedure-link') }
     end