From 6bad27282572691a8dacbec39b7fd672872c751d Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 11 Jan 2024 10:44:27 +0100 Subject: [PATCH 1/8] style --- app/models/france_connect_information.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/france_connect_information.rb b/app/models/france_connect_information.rb index 8f7da310a..b9f370953 100644 --- a/app/models/france_connect_information.rb +++ b/app/models/france_connect_information.rb @@ -26,7 +26,7 @@ class FranceConnectInformation < ApplicationRecord def create_merge_token! merge_token = SecureRandom.uuid - update(merge_token: merge_token, merge_token_created_at: Time.zone.now) + update(merge_token:, merge_token_created_at: Time.zone.now) merge_token end From 4c7b494c9d6fca0b7be3c9c861321a0dc50adc17 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 10 Jan 2024 21:09:40 +0100 Subject: [PATCH 2/8] be consistent in france_connect_email sanitation --- .../france_connect/particulier_controller.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/controllers/france_connect/particulier_controller.rb b/app/controllers/france_connect/particulier_controller.rb index 92ae65efa..f4c784941 100644 --- a/app/controllers/france_connect/particulier_controller.rb +++ b/app/controllers/france_connect/particulier_controller.rb @@ -14,7 +14,7 @@ class FranceConnect::ParticulierController < ApplicationController fci = FranceConnectService.find_or_retrieve_france_connect_information(params[:code]) if fci.user.nil? - preexisting_unlinked_user = User.find_by(email: fci.email_france_connect.downcase) + preexisting_unlinked_user = User.find_by(email: sanitize(fci.email_france_connect)) if preexisting_unlinked_user.nil? fci.associate_user!(fci.email_france_connect) @@ -67,7 +67,7 @@ class FranceConnect::ParticulierController < ApplicationController end def mail_merge_with_existing_account - user = User.find_by(email: @fci.email_france_connect.downcase) + user = User.find_by(email: sanitize(@fci.email_france_connect.downcase)) if user.can_france_connect? @fci.update(user: user) @fci.delete_merge_token! @@ -146,6 +146,10 @@ class FranceConnect::ParticulierController < ApplicationController end def sanitized_email_params - params[:email]&.gsub(/[[:space:]]/, ' ')&.strip&.downcase + sanitize(params[:email]) + end + + def sanitize(string) + string&.gsub(/[[:space:]]/, ' ')&.strip&.downcase end end From 65aa07ecbeb184f5fd0acd20a93aef946b6e9229 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 10 Jan 2024 21:09:57 +0100 Subject: [PATCH 3/8] the merge_token issued for password check can be reused for the confirmation by email route Thus by passing the password check or the email possession check --- .../france_connect/particulier_controller_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/controllers/france_connect/particulier_controller_spec.rb b/spec/controllers/france_connect/particulier_controller_spec.rb index 360424f17..81fc1a460 100644 --- a/spec/controllers/france_connect/particulier_controller_spec.rb +++ b/spec/controllers/france_connect/particulier_controller_spec.rb @@ -340,6 +340,8 @@ describe FranceConnect::ParticulierController, type: :controller do context 'when an account with the same email exists' do let!(:user) { create(:user, email: email) } + before { allow(controller).to receive(:sign_in).and_call_original } + render_views it 'asks for the corresponding password' do @@ -352,6 +354,15 @@ describe FranceConnect::ParticulierController, type: :controller do expect(response.body).to include('entrez votre mot de passe') end + + it 'cannot use the merge token in the email confirmation route' do + subject + fci.reload + + get :mail_merge_with_existing_account, params: { merge_token: fci.merge_token } + expect(controller).not_to have_received(:sign_in) + expect(flash[:alert]).to be_present + end end end From 90f145e17a19a463cfc671972aef66dda8650670 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 10 Jan 2024 12:43:17 +0100 Subject: [PATCH 4/8] add email_merge_token to france connect information --- app/models/france_connect_information.rb | 15 +++++++++++++++ ..._token_column_to_france_connect_information.rb | 10 ++++++++++ db/schema.rb | 5 ++++- 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20240110113623_add_email_merge_token_column_to_france_connect_information.rb diff --git a/app/models/france_connect_information.rb b/app/models/france_connect_information.rb index b9f370953..81ea02f7a 100644 --- a/app/models/france_connect_information.rb +++ b/app/models/france_connect_information.rb @@ -31,14 +31,29 @@ class FranceConnectInformation < ApplicationRecord merge_token end + def create_email_merge_token! + email_merge_token = SecureRandom.uuid + update(email_merge_token:, email_merge_token_created_at: Time.zone.now) + + email_merge_token + end + def valid_for_merge? (MERGE_VALIDITY.ago < merge_token_created_at) && user_id.nil? end + def valid_for_email_merge? + (MERGE_VALIDITY.ago < email_merge_token_created_at) && user_id.nil? + end + def delete_merge_token! update(merge_token: nil, merge_token_created_at: nil) end + def delete_email_merge_token! + update(email_merge_token: nil, email_merge_token_created_at: nil) + end + def full_name [given_name, family_name].compact.join(" ") end diff --git a/db/migrate/20240110113623_add_email_merge_token_column_to_france_connect_information.rb b/db/migrate/20240110113623_add_email_merge_token_column_to_france_connect_information.rb new file mode 100644 index 000000000..43c14f4ac --- /dev/null +++ b/db/migrate/20240110113623_add_email_merge_token_column_to_france_connect_information.rb @@ -0,0 +1,10 @@ +class AddEmailMergeTokenColumnToFranceConnectInformation < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def change + add_column :france_connect_informations, :email_merge_token, :string + add_column :france_connect_informations, :email_merge_token_created_at, :datetime + + add_index :france_connect_informations, :email_merge_token, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 236a8eb66..87a077727 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[7.0].define(version: 2023_12_21_142727) do +ActiveRecord::Schema[7.0].define(version: 2024_01_10_113623) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -626,6 +626,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_12_21_142727) do t.datetime "created_at", precision: nil, null: false t.jsonb "data" t.string "email_france_connect" + t.string "email_merge_token" + t.datetime "email_merge_token_created_at" t.string "family_name" t.string "france_connect_particulier_id" t.string "gender" @@ -634,6 +636,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_12_21_142727) do t.datetime "merge_token_created_at", precision: nil t.datetime "updated_at", precision: nil, null: false t.integer "user_id" + t.index ["email_merge_token"], name: "index_france_connect_informations_on_email_merge_token" t.index ["merge_token"], name: "index_france_connect_informations_on_merge_token" t.index ["user_id"], name: "index_france_connect_informations_on_user_id" end From fca28a3ebdbd72f3b7f0033e67fdcbc04b638135 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 11 Jan 2024 10:40:44 +0100 Subject: [PATCH 5/8] use email_merge_token in mail_merge_with_existing_account route --- .../france_connect/particulier_controller.rb | 4 ++++ config/routes.rb | 2 +- .../france_connect/particulier_controller_spec.rb | 12 ++++++------ 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/controllers/france_connect/particulier_controller.rb b/app/controllers/france_connect/particulier_controller.rb index f4c784941..ffe24c883 100644 --- a/app/controllers/france_connect/particulier_controller.rb +++ b/app/controllers/france_connect/particulier_controller.rb @@ -141,6 +141,10 @@ class FranceConnect::ParticulierController < ApplicationController params[:merge_token] end + def email_merge_token_params + params[:email_merge_token] + end + def password_params params[:password] end diff --git a/config/routes.rb b/config/routes.rb index de03dee2f..fce4e44a5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -177,7 +177,7 @@ Rails.application.routes.draw do get 'particulier' => 'particulier#login' get 'particulier/callback' => 'particulier#callback' get 'particulier/merge/:merge_token' => 'particulier#merge', as: :particulier_merge - get 'particulier/mail_merge_with_existing_account/:merge_token' => 'particulier#mail_merge_with_existing_account', as: :particulier_mail_merge_with_existing_account + get 'particulier/mail_merge_with_existing_account/:email_merge_token' => 'particulier#mail_merge_with_existing_account', as: :particulier_mail_merge_with_existing_account post 'particulier/resend_and_renew_merge_confirmation' => 'particulier#resend_and_renew_merge_confirmation', as: :particulier_resend_and_renew_merge_confirmation post 'particulier/merge_with_existing_account' => 'particulier#merge_with_existing_account' post 'particulier/merge_with_new_account' => 'particulier#merge_with_new_account' diff --git a/spec/controllers/france_connect/particulier_controller_spec.rb b/spec/controllers/france_connect/particulier_controller_spec.rb index 81fc1a460..d09d24ac9 100644 --- a/spec/controllers/france_connect/particulier_controller_spec.rb +++ b/spec/controllers/france_connect/particulier_controller_spec.rb @@ -268,10 +268,10 @@ describe FranceConnect::ParticulierController, type: :controller do describe '#mail_merge_with_existing_account' do let(:fci) { FranceConnectInformation.create!(user_info) } - let!(:merge_token) { fci.create_merge_token! } + let!(:email_merge_token) { fci.create_email_merge_token! } context 'when the merge_token is ok and the user is found' do - subject { post :mail_merge_with_existing_account, params: { merge_token: fci.merge_token } } + subject { post :mail_merge_with_existing_account, params: { email_merge_token: } } let!(:user) { create(:user, email: email, password: 'abcdefgh') } @@ -298,8 +298,8 @@ describe FranceConnect::ParticulierController, type: :controller do end end - context 'when the merge_token is not ok' do - subject { post :mail_merge_with_existing_account, params: { merge_token: 'ko' } } + context 'when the email_merge_token is not ok' do + subject { post :mail_merge_with_existing_account, params: { email_merge_token: 'ko' } } let!(:user) { create(:user, email: email) } @@ -308,7 +308,7 @@ describe FranceConnect::ParticulierController, type: :controller do fci.reload expect(fci.user).to be_nil - expect(fci.merge_token).not_to be_nil + expect(fci.email_merge_token).not_to be_nil expect(controller.current_user).to be_nil expect(response).to redirect_to(root_path) end @@ -359,7 +359,7 @@ describe FranceConnect::ParticulierController, type: :controller do subject fci.reload - get :mail_merge_with_existing_account, params: { merge_token: fci.merge_token } + get :mail_merge_with_existing_account, params: { email_merge_token: fci.merge_token } expect(controller).not_to have_received(:sign_in) expect(flash[:alert]).to be_present end From ca08b80c3e933d72a26d3622083d10c000e5c0dc Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 11 Jan 2024 11:31:36 +0100 Subject: [PATCH 6/8] use email_merge_token in mailer --- app/mailers/user_mailer.rb | 6 +++--- .../user_mailer/france_connect_merge_confirmation.haml | 7 ++++--- spec/mailers/user_mailer_spec.rb | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 510d65791..db28953fe 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -20,9 +20,9 @@ class UserMailer < ApplicationMailer mail(to: requested_email, subject: @subject) end - def france_connect_merge_confirmation(email, merge_token, merge_token_created_at) - @merge_token = merge_token - @merge_token_created_at = merge_token_created_at + def france_connect_merge_confirmation(email, email_merge_token, email_merge_token_created_at) + @email_merge_token = email_merge_token + @email_merge_token_created_at = email_merge_token_created_at @subject = "Veuillez confirmer la fusion de compte" mail(to: email, subject: @subject) diff --git a/app/views/user_mailer/france_connect_merge_confirmation.haml b/app/views/user_mailer/france_connect_merge_confirmation.haml index f7fb0f664..2f6c99a33 100644 --- a/app/views/user_mailer/france_connect_merge_confirmation.haml +++ b/app/views/user_mailer/france_connect_merge_confirmation.haml @@ -1,16 +1,17 @@ - content_for(:title, @subject) +- merge_link = france_connect_particulier_mail_merge_with_existing_account_url(email_merge_token: @email_merge_token) %p Bonjour, %p Pour confirmer la fusion de votre compte, veuillez cliquer sur le lien suivant : -= round_button 'Je confirme', france_connect_particulier_mail_merge_with_existing_account_url(merge_token: @merge_token), :primary += round_button 'Je confirme', merge_link, :primary %p - Vous pouvez aussi visiter ce lien : #{link_to france_connect_particulier_mail_merge_with_existing_account_url(merge_token: @merge_token), france_connect_particulier_mail_merge_with_existing_account_url(merge_token: @merge_token)} + Vous pouvez aussi visiter ce lien : #{link_to merge_link, merge_link} -%p Ce lien est valide #{distance_of_time_in_words(FranceConnectInformation::MERGE_VALIDITY)}, jusqu'à #{@merge_token_created_at.strftime("%d-%m-%Y à %H:%M (%Z)")} +%p Ce lien est valide #{distance_of_time_in_words(FranceConnectInformation::MERGE_VALIDITY)}, jusqu'à #{@email_merge_token_created_at.strftime("%d-%m-%Y à %H:%M (%Z)")} %p Si vous n’êtes pas à l’origine de cette demande, vous pouvez ignorer ce message. Et si vous avez besoin d’assistance, n’hésitez pas à nous contacter à diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 54b2675c3..7382504f1 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -65,7 +65,7 @@ RSpec.describe UserMailer, type: :mailer do subject { described_class.france_connect_merge_confirmation(email, code, 15.minutes.from_now) } it { expect(subject.to).to eq([email]) } - it { expect(subject.body).to include(france_connect_particulier_mail_merge_with_existing_account_url(merge_token: code)) } + it { expect(subject.body).to include(france_connect_particulier_mail_merge_with_existing_account_url(email_merge_token: code)) } context 'without SafeMailer configured' do it { expect(subject[BalancerDeliveryMethod::FORCE_DELIVERY_METHOD_HEADER]&.value).to eq(nil) } From e12dbe7aad60be66a6adcfc42c6747cb4be74dae Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 11 Jan 2024 10:44:19 +0100 Subject: [PATCH 7/8] use email_merge_token in email merge --- .../france_connect/particulier_controller.rb | 24 +++++++++++++++++-- .../particulier_controller_spec.rb | 2 ++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/app/controllers/france_connect/particulier_controller.rb b/app/controllers/france_connect/particulier_controller.rb index ffe24c883..99829f90c 100644 --- a/app/controllers/france_connect/particulier_controller.rb +++ b/app/controllers/france_connect/particulier_controller.rb @@ -1,6 +1,7 @@ class FranceConnect::ParticulierController < ApplicationController before_action :redirect_to_login_if_fc_aborted, only: [:callback] - before_action :securely_retrieve_fci, only: [:merge, :merge_with_existing_account, :merge_with_new_account, :mail_merge_with_existing_account, :resend_and_renew_merge_confirmation] + before_action :securely_retrieve_fci, only: [:merge, :merge_with_existing_account, :merge_with_new_account, :resend_and_renew_merge_confirmation] + before_action :securely_retrieve_fci_from_email_merge_token, only: [:mail_merge_with_existing_account] def login if FranceConnectService.enabled? @@ -96,14 +97,33 @@ class FranceConnect::ParticulierController < ApplicationController end def resend_and_renew_merge_confirmation + @fci.create_email_merge_token! + UserMailer.france_connect_merge_confirmation( + @fci.email_france_connect, + @fci.email_merge_token, + @fci.email_merge_token_created_at + ) + .deliver_later + merge_token = @fci.create_merge_token! - UserMailer.france_connect_merge_confirmation(@fci.email_france_connect, merge_token, @fci.merge_token_created_at).deliver_later redirect_to france_connect_particulier_merge_path(merge_token), notice: t('france_connect.particulier.flash.confirmation_mail_sent') end private + def securely_retrieve_fci_from_email_merge_token + @fci = FranceConnectInformation.find_by(email_merge_token: email_merge_token_params) + + if @fci.nil? || !@fci.valid_for_email_merge? + flash.alert = t('france_connect.particulier.flash.merger_token_expired', application_name: APPLICATION_NAME) + + redirect_to root_path + else + @fci.delete_email_merge_token! + end + end + def securely_retrieve_fci @fci = FranceConnectInformation.find_by(merge_token: merge_token_params) diff --git a/spec/controllers/france_connect/particulier_controller_spec.rb b/spec/controllers/france_connect/particulier_controller_spec.rb index d09d24ac9..8383e259a 100644 --- a/spec/controllers/france_connect/particulier_controller_spec.rb +++ b/spec/controllers/france_connect/particulier_controller_spec.rb @@ -281,6 +281,7 @@ describe FranceConnect::ParticulierController, type: :controller do expect(fci.user).to eq(user) expect(fci.merge_token).to be_nil + expect(fci.email_merge_token).to be_nil expect(controller.current_user).to eq(user) expect(flash[:notice]).to eq("Les comptes FranceConnect et #{APPLICATION_NAME} sont à présent fusionnés") end @@ -371,6 +372,7 @@ describe FranceConnect::ParticulierController, type: :controller do let(:merge_token) { fci.create_merge_token! } it 'renew token' do expect { post :resend_and_renew_merge_confirmation, params: { merge_token: merge_token } }.to change { fci.reload.merge_token } + expect(fci.email_merge_token).to be_present expect(response).to redirect_to(france_connect_particulier_merge_path(fci.reload.merge_token)) end end From 586e4ed6138403f3bebfd7dbaae57f924efdbf1a Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 11 Jan 2024 11:21:09 +0100 Subject: [PATCH 8/8] to be sure, delete possible email_merge_token if correct password merge --- app/controllers/france_connect/particulier_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/france_connect/particulier_controller.rb b/app/controllers/france_connect/particulier_controller.rb index 99829f90c..e05924243 100644 --- a/app/controllers/france_connect/particulier_controller.rb +++ b/app/controllers/france_connect/particulier_controller.rb @@ -58,6 +58,7 @@ class FranceConnect::ParticulierController < ApplicationController else @fci.update(user: user) @fci.delete_merge_token! + @fci.delete_email_merge_token! flash.notice = t('france_connect.particulier.flash.connection_done', application_name: APPLICATION_NAME) connect_france_connect_particulier(user)