Merge pull request #1254 from betagouv/partially_clean_fc

Partially clean fc

app/controllers/france_connect/particulier_controller.rb

@@ -1,42 +1,30 @@
 class FranceConnect::ParticulierController < ApplicationController
   def login
-    client = FranceConnectParticulierClient.new
+    redirect_to FranceConnectService.authorization_uri
-
-    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
   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
-      france_connect_information = FranceConnectInformation.find_by_france_connect_particulier user_infos
+      .find_by(france_connect_particulier_id: fetched_fc_information[:france_connect_particulier_id])
 
-      france_connect_information = FranceConnectInformation.create(
+    if france_connect_information.nil?
-          {gender: user_infos[:gender],
+      fetched_fc_information.save
-           given_name: user_infos[:given_name],
+      france_connect_information = fetched_fc_information
-           family_name: user_infos[:family_name],
+    end
-           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?
 
-      user = france_connect_information.user
+    user = france_connect_information.user
-      salt = FranceConnectSaltService.new(france_connect_information).salt
+    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?
+    if user.nil?
-
+      redirect_to france_connect_particulier_new_path(fci_id: france_connect_information.id, salt: salt)
-      connect_france_connect_particulier user
+    else
+      connect_france_connect_particulier(user)
     end
   rescue Rack::OAuth2::Client::Error => e
     Rails.logger.error e.message

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

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

app/models/france_connect_particulier_client.rb

@@ -1,16 +1,9 @@
 class FranceConnectParticulierClient < OpenIDConnect::Client
-  def initialize params={}
+  def initialize(code = nil)
-    super(
+    super(FRANCE_CONNECT[:particulier])
-        identifier: FRANCE_CONNECT.particulier_identifier,
-        secret: FRANCE_CONNECT.particulier_secret,
 
-        redirect_uri: FRANCE_CONNECT.particulier_redirect_uri,
+    if code.present?
-
+      self.authorization_code = code
-        authorization_endpoint: FRANCE_CONNECT.particulier_authorization_endpoint,
+    end
-        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
   end
 end

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

app/services/france_connect_service.rb

@@ -1,12 +1,27 @@
 class FranceConnectService
-  def self.retrieve_user_informations_particulier code
+  def self.authorization_uri
-    client = FranceConnectParticulierClient.new code: code
+    client = FranceConnectParticulierClient.new
 
-    access_token = client.access_token!(client_auth_method: :secret)
+    client.authorization_uri(
-    user_info = access_token.userinfo!
+      scope: [:profile, :email],
-    hash = Hashie::Mash.new user_info.raw_attributes
+      state: SecureRandom.hex(16),
+      nonce: SecureRandom.hex(16))
+  end
 
-    hash.france_connect_particulier_id = hash.sub
+  def self.retrieve_user_informations_particulier(code)
-    hash
+    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

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"
+}

config/initializers/france_connect.rb

@@ -1,14 +1,22 @@
-FRANCE_CONNECT = if !Rails.env.test?
+FRANCE_CONNECT = if Rails.env.test?
-  file_path = "#{Rails.root}/config/france_connect.yml"
+  {
-  Hashie::Mash.load(file_path)
+    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({
+  fc_config_file_path = "#{Rails.root}/config/france_connect.yml"
-    particulier_identifier: 'plop',
+
-    particulier_secret: 'plip',
+  # FIXME: with a yaml with a { particulier: {} } structure
-    particulier_redirect_uri: 'https://bidon.com/endpoint',
+  config_hash = YAML.safe_load(File.read(fc_config_file_path))
-    particulier_authorization_endpoint: 'https://bidon.com/endpoint',
+    .reduce({}) { |acc, (key, value)| acc[key.gsub('particulier_', '')] = value; acc }
-    particulier_token_endpoint: 'https://bidon.com/endpoint',
+    .symbolize_keys
-    particulier_userinfo_endpoint: 'https://bidon.com/endpoint',
+
-    particulier_logout_endpoint: 'https://bidon.com/endpoint',
+  { particulier: config_hash }
-  })
 end

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

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

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,
+  let(:user_info) do
-                                     given_name: given_name,
+    {
-                                     family_name: family_name,
+      france_connect_particulier_id: france_connect_particulier_id,
-                                     birthdate: birthdate,
+      given_name: given_name,
-                                     birthplace: birthplace,
+      family_name: family_name,
-                                     gender: gender,
+      birthdate: birthdate,
-                                     email: email)
+      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

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

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
+    subject { FranceConnectParticulierClient.new(code) }
-      expect(described_class).to be < OpenIDConnect::Client
+
-    end
     context 'when given code in params' do
       let(:code) { 'plop' }
-      subject { described_class.new(code: code) }
+
-      it 'set authorisation code' do
+      before { allow_any_instance_of(FranceConnectParticulierClient).to receive(:authorization_code=) }
-        expect_any_instance_of(described_class).to receive(:authorization_code=).with(code)
+
-        described_class.new(code: code)
+      it { is_expected.to have_received(:authorization_code=).with(code) }
-      end
     end
   end
 end

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
+    it 'returns user informations' do
-      expect(subject.given_name).to eq(given_name)
+      expect(subject).to have_attributes({
-      expect(subject.family_name).to eq(family_name)
+        given_name: given_name,
-      expect(subject.birthdate).to eq(birthdate)
+        family_name: family_name,
-      expect(subject.gender).to eq(gender)
+        birthdate: DateTime.parse(birthdate),
-      expect(subject.email).to eq(email)
+        birthplace: birthplace,
-      expect(subject.phone).to eq(phone)
+        gender: gender,
-      expect(subject.birthplace).to eq(birthplace)
+        email_france_connect: email,
-      expect(subject.france_connect_particulier_id).to eq(france_connect_particulier_id)
+        france_connect_particulier_id: france_connect_particulier_id })
     end
   end
 end