diff --git a/app/controllers/france_connect/particulier_controller.rb b/app/controllers/france_connect/particulier_controller.rb index 3d0e0a0f7..283660636 100644 --- a/app/controllers/france_connect/particulier_controller.rb +++ b/app/controllers/france_connect/particulier_controller.rb @@ -1,42 +1,30 @@ class FranceConnect::ParticulierController < ApplicationController def login - client = FranceConnectParticulierClient.new - - session[:state] = SecureRandom.hex(16) - session[:nonce] = SecureRandom.hex(16) - - authorization_uri = client.authorization_uri( - scope: [:profile, :email], - state: session[:state], - nonce: session[:nonce] - ) - redirect_to URI.parse(authorization_uri).to_s + redirect_to FranceConnectService.authorization_uri end def callback - return redirect_to new_user_session_path if !params.has_key?(:code) + if params[:code].nil? + return redirect_to new_user_session_path + end - user_infos = FranceConnectService.retrieve_user_informations_particulier(params[:code]) + fetched_fc_information = FranceConnectService.retrieve_user_informations_particulier(params[:code]) - if user_infos.present? - france_connect_information = FranceConnectInformation.find_by_france_connect_particulier user_infos + france_connect_information = FranceConnectInformation + .find_by(france_connect_particulier_id: fetched_fc_information[:france_connect_particulier_id]) - france_connect_information = FranceConnectInformation.create( - {gender: user_infos[:gender], - given_name: user_infos[:given_name], - family_name: user_infos[:family_name], - email_france_connect: user_infos[:email], - birthdate: user_infos[:birthdate], - birthplace: user_infos[:birthplace], - france_connect_particulier_id: user_infos[:france_connect_particulier_id]} - ) if france_connect_information.nil? + if france_connect_information.nil? + fetched_fc_information.save + france_connect_information = fetched_fc_information + end - user = france_connect_information.user - salt = FranceConnectSaltService.new(france_connect_information).salt + user = france_connect_information.user + salt = FranceConnectSaltService.new(france_connect_information).salt - return redirect_to france_connect_particulier_new_path(fci_id: france_connect_information.id, salt: salt) if user.nil? - - connect_france_connect_particulier user + if user.nil? + redirect_to france_connect_particulier_new_path(fci_id: france_connect_information.id, salt: salt) + else + connect_france_connect_particulier(user) end rescue Rack::OAuth2::Client::Error => e Rails.logger.error e.message diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 8c77f7aac..ea455dbfc 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -49,11 +49,8 @@ class Users::SessionsController < Sessions::SessionsController sign_out :user case connected_with_france_connect - when 'entreprise' - redirect_to FRANCE_CONNECT.entreprise_logout_endpoint - return when 'particulier' - redirect_to FRANCE_CONNECT.particulier_logout_endpoint + redirect_to FRANCE_CONNECT[:particulier][:logout_endpoint] return end end diff --git a/app/models/france_connect_information.rb b/app/models/france_connect_information.rb index 6bf7f7a85..76e804399 100644 --- a/app/models/france_connect_information.rb +++ b/app/models/france_connect_information.rb @@ -2,8 +2,4 @@ class FranceConnectInformation < ActiveRecord::Base belongs_to :user validates :france_connect_particulier_id, presence: true, allow_blank: false, allow_nil: false - - def self.find_by_france_connect_particulier user_info - FranceConnectInformation.find_by(france_connect_particulier_id: user_info[:france_connect_particulier_id]) - end end diff --git a/app/models/france_connect_particulier_client.rb b/app/models/france_connect_particulier_client.rb index bc0e99e9c..072b5ab90 100644 --- a/app/models/france_connect_particulier_client.rb +++ b/app/models/france_connect_particulier_client.rb @@ -1,16 +1,9 @@ class FranceConnectParticulierClient < OpenIDConnect::Client - def initialize params={} - super( - identifier: FRANCE_CONNECT.particulier_identifier, - secret: FRANCE_CONNECT.particulier_secret, + def initialize(code = nil) + super(FRANCE_CONNECT[:particulier]) - redirect_uri: FRANCE_CONNECT.particulier_redirect_uri, - - authorization_endpoint: FRANCE_CONNECT.particulier_authorization_endpoint, - token_endpoint: FRANCE_CONNECT.particulier_token_endpoint, - userinfo_endpoint: FRANCE_CONNECT.particulier_userinfo_endpoint, - logout_endpoint: FRANCE_CONNECT.particulier_logout_endpoint - ) - self.authorization_code = params[:code] if params.has_key? :code + if code.present? + self.authorization_code = code + end end end diff --git a/app/services/france_connect_salt_service.rb b/app/services/france_connect_salt_service.rb index a28476952..97d5d83d0 100644 --- a/app/services/france_connect_salt_service.rb +++ b/app/services/france_connect_salt_service.rb @@ -11,6 +11,6 @@ class FranceConnectSaltService end def salt - Digest::MD5.hexdigest(model.france_connect_particulier_id + model.given_name + model.family_name + FRANCE_CONNECT.particulier_secret + DateTime.now.to_date.to_s) + Digest::MD5.hexdigest(model.france_connect_particulier_id + model.given_name + model.family_name + FRANCE_CONNECT[:particulier][:secret] + DateTime.now.to_date.to_s) end end diff --git a/app/services/france_connect_service.rb b/app/services/france_connect_service.rb index 883169e70..bbfe73794 100644 --- a/app/services/france_connect_service.rb +++ b/app/services/france_connect_service.rb @@ -1,12 +1,27 @@ class FranceConnectService - def self.retrieve_user_informations_particulier code - client = FranceConnectParticulierClient.new code: code + def self.authorization_uri + client = FranceConnectParticulierClient.new - access_token = client.access_token!(client_auth_method: :secret) - user_info = access_token.userinfo! - hash = Hashie::Mash.new user_info.raw_attributes + client.authorization_uri( + scope: [:profile, :email], + state: SecureRandom.hex(16), + nonce: SecureRandom.hex(16)) + end - hash.france_connect_particulier_id = hash.sub - hash + def self.retrieve_user_informations_particulier(code) + client = FranceConnectParticulierClient.new(code) + + user_info = client.access_token!(client_auth_method: :secret) + .userinfo! + .raw_attributes + + FranceConnectInformation.new( + gender: user_info[:gender], + given_name: user_info[:given_name], + family_name: user_info[:family_name], + email_france_connect: user_info[:email], + birthdate: user_info[:birthdate], + birthplace: user_info[:birthplace], + france_connect_particulier_id: user_info[:sub]) end end diff --git a/config/brakeman.ignore b/config/brakeman.ignore new file mode 100644 index 000000000..a8ca1ae00 --- /dev/null +++ b/config/brakeman.ignore @@ -0,0 +1,26 @@ +{ + "ignored_warnings": [ + { + "warning_type": "Redirect", + "warning_code": 18, + "fingerprint": "a0a4cede6d50308b90bd747efd0a2ebd58947fbd5d100349ccd640c60413b1a9", + "check_name": "Redirect", + "message": "Possible unprotected redirect", + "file": "app/controllers/france_connect/particulier_controller.rb", + "line": 3, + "link": "http://brakemanscanner.org/docs/warning_types/redirect/", + "code": "redirect_to(FranceConnectParticulierClient.new.authorization_uri)", + "render_path": null, + "location": { + "type": "method", + "class": "FranceConnect::ParticulierController", + "method": "login" + }, + "user_input": "FranceConnectParticulierClient.new.authorization_uri", + "confidence": "High", + "note": "We trust FC OpenId implem" + } + ], + "updated": "2018-01-11 15:53:22 +0100", + "brakeman_version": "3.7.0" +} diff --git a/config/initializers/france_connect.rb b/config/initializers/france_connect.rb index c9e9d11cf..a3702335b 100644 --- a/config/initializers/france_connect.rb +++ b/config/initializers/france_connect.rb @@ -1,14 +1,22 @@ -FRANCE_CONNECT = if !Rails.env.test? - file_path = "#{Rails.root}/config/france_connect.yml" - Hashie::Mash.load(file_path) +FRANCE_CONNECT = if Rails.env.test? + { + particulier: { + identifier: 'plop', + secret: 'plip', + redirect_uri: 'https://bidon.com/endpoint', + authorization_endpoint: 'https://bidon.com/endpoint', + token_endpoint: 'https://bidon.com/endpoint', + userinfo_endpoint: 'https://bidon.com/endpoint', + logout_endpoint: 'https://bidon.com/endpoint', + } + } else - Hashie::Mash.new({ - particulier_identifier: 'plop', - particulier_secret: 'plip', - particulier_redirect_uri: 'https://bidon.com/endpoint', - particulier_authorization_endpoint: 'https://bidon.com/endpoint', - particulier_token_endpoint: 'https://bidon.com/endpoint', - particulier_userinfo_endpoint: 'https://bidon.com/endpoint', - particulier_logout_endpoint: 'https://bidon.com/endpoint', - }) + fc_config_file_path = "#{Rails.root}/config/france_connect.yml" + + # FIXME: with a yaml with a { particulier: {} } structure + config_hash = YAML.safe_load(File.read(fc_config_file_path)) + .reduce({}) { |acc, (key, value)| acc[key.gsub('particulier_', '')] = value; acc } + .symbolize_keys + + { particulier: config_hash } end diff --git a/spec/controllers/france_connect/particulier_controller_spec.rb b/spec/controllers/france_connect/particulier_controller_spec.rb index 279f7b9ef..9bc9e61fd 100644 --- a/spec/controllers/france_connect/particulier_controller_spec.rb +++ b/spec/controllers/france_connect/particulier_controller_spec.rb @@ -11,7 +11,7 @@ describe FranceConnect::ParticulierController, type: :controller do let(:email) { 'test@test.com' } let(:password) { '' } - let(:user_info) { Hashie::Mash.new(france_connect_particulier_id: france_connect_particulier_id, given_name: given_name, family_name: family_name, birthdate: birthdate, birthplace: birthplace, gender: gender, email: email, password: password) } + let(:user_info) { { france_connect_particulier_id: france_connect_particulier_id, given_name: given_name, family_name: family_name, birthdate: birthdate, birthplace: birthplace, gender: gender, email_france_connect: email } } describe '.auth' do it 'redirect to france connect serveur' do @@ -31,7 +31,8 @@ describe FranceConnect::ParticulierController, type: :controller do context 'when params code is present' do context 'when code is correct' do before do - allow(FranceConnectService).to receive(:retrieve_user_informations_particulier).and_return(user_info) + allow(FranceConnectService).to receive(:retrieve_user_informations_particulier) + .and_return(FranceConnectInformation.new(user_info)) end context 'when france_connect_particulier_id exist in database' do diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 6cbaac682..2263df65e 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -118,7 +118,7 @@ describe Users::SessionsController, type: :controller do let(:loged_in_with_france_connect) { 'particulier' } it 'redirect to france connect logout page' do - expect(response).to redirect_to(FRANCE_CONNECT.particulier_logout_endpoint) + expect(response).to redirect_to(FRANCE_CONNECT[:particulier][:logout_endpoint]) end end @@ -161,7 +161,7 @@ describe Users::SessionsController, type: :controller do user.update_attributes(loged_in_with_france_connect: 'particulier') sign_in user delete :destroy - expect(@response.headers["Location"]).to eq(FRANCE_CONNECT.particulier_logout_endpoint) + expect(@response.headers["Location"]).to eq(FRANCE_CONNECT[:particulier][:logout_endpoint]) end context "when associated administrateur" do diff --git a/spec/features/france_connect/france_connect_particulier_spec.rb b/spec/features/france_connect/france_connect_particulier_spec.rb index 720523714..7f1b263af 100644 --- a/spec/features/france_connect/france_connect_particulier_spec.rb +++ b/spec/features/france_connect/france_connect_particulier_spec.rb @@ -10,14 +10,17 @@ feature 'France Connect Particulier Connexion' do let(:email) { 'plop@plop.com' } let(:france_connect_particulier_id) { 'blabla' } - let(:user_info) { Hashie::Mash.new(france_connect_particulier_id: france_connect_particulier_id, - given_name: given_name, - family_name: family_name, - birthdate: birthdate, - birthplace: birthplace, - gender: gender, - email: email) - } + let(:user_info) do + { + france_connect_particulier_id: france_connect_particulier_id, + given_name: given_name, + family_name: family_name, + birthdate: birthdate, + birthplace: birthplace, + gender: gender, + email_france_connect: email + } + end context 'when user is on login page' do before do @@ -44,7 +47,7 @@ feature 'France Connect Particulier Connexion' do before do allow_any_instance_of(FranceConnectParticulierClient).to receive(:authorization_uri).and_return(france_connect_particulier_callback_path(code: code)) - allow(FranceConnectService).to receive(:retrieve_user_informations_particulier).and_return(user_info) + allow(FranceConnectService).to receive(:retrieve_user_informations_particulier).and_return(FranceConnectInformation.new(user_info)) end context 'when is the first connexion' do diff --git a/spec/models/france_connect_information_spec.rb b/spec/models/france_connect_information_spec.rb index 330fc679c..96413d621 100644 --- a/spec/models/france_connect_information_spec.rb +++ b/spec/models/france_connect_information_spec.rb @@ -8,20 +8,4 @@ describe FranceConnectInformation, type: :model do it { is_expected.to allow_value('mon super projet').for(:france_connect_particulier_id) } end end - - describe '.find_by_france_connect_particulier' do - let(:user_info) { {france_connect_particulier_id: '123456'} } - - subject { described_class.find_by_france_connect_particulier user_info } - - context 'when france_connect_particulier_id is prensent in database' do - let!(:france_connect_information) { create(:france_connect_information, france_connect_particulier_id: '123456') } - - it { is_expected.to eq france_connect_information } - end - - context 'when france_connect_particulier_id is prensent in database' do - it { is_expected.to eq nil } - end - end end diff --git a/spec/models/france_connect_particulier_client_spec.rb b/spec/models/france_connect_particulier_client_spec.rb index 7f3168d13..b73825d7d 100644 --- a/spec/models/france_connect_particulier_client_spec.rb +++ b/spec/models/france_connect_particulier_client_spec.rb @@ -2,16 +2,14 @@ require 'spec_helper' describe FranceConnectParticulierClient do describe '.initialize' do - it 'create an openid client' do - expect(described_class).to be < OpenIDConnect::Client - end + subject { FranceConnectParticulierClient.new(code) } + context 'when given code in params' do let(:code) { 'plop' } - subject { described_class.new(code: code) } - it 'set authorisation code' do - expect_any_instance_of(described_class).to receive(:authorization_code=).with(code) - described_class.new(code: code) - end + + before { allow_any_instance_of(FranceConnectParticulierClient).to receive(:authorization_code=) } + + it { is_expected.to have_received(:authorization_code=).with(code) } end end end diff --git a/spec/services/france_connect_service_spec.rb b/spec/services/france_connect_service_spec.rb index 7374ecb32..342b8eb2e 100644 --- a/spec/services/france_connect_service_spec.rb +++ b/spec/services/france_connect_service_spec.rb @@ -7,7 +7,7 @@ describe FranceConnectService do let(:given_name) { 'plop1' } let(:family_name) { 'plop2' } - let(:birthdate) { 'plop3' } + let(:birthdate) { '2012-12-31' } let(:gender) { 'plop4' } let(:birthplace) { 'plop5' } let(:email) { 'plop@emaiL.com' } @@ -29,15 +29,15 @@ describe FranceConnectService do subject end - it 'returns user informations in a object' do - expect(subject.given_name).to eq(given_name) - expect(subject.family_name).to eq(family_name) - expect(subject.birthdate).to eq(birthdate) - expect(subject.gender).to eq(gender) - expect(subject.email).to eq(email) - expect(subject.phone).to eq(phone) - expect(subject.birthplace).to eq(birthplace) - expect(subject.france_connect_particulier_id).to eq(france_connect_particulier_id) + it 'returns user informations' do + expect(subject).to have_attributes({ + given_name: given_name, + family_name: family_name, + birthdate: DateTime.parse(birthdate), + birthplace: birthplace, + gender: gender, + email_france_connect: email, + france_connect_particulier_id: france_connect_particulier_id }) end end end