Merge pull request #7141 from betagouv/fix_nonce_agent_connect

Ajoute le controle du nonce et du state dans le flow openid d'agent connect

app/controllers/agent_connect/agent_controller.rb

@@ -1,16 +1,26 @@
 # doc: https://github.com/france-connect/Documentation-AgentConnect
 class AgentConnect::AgentController < ApplicationController
   before_action :redirect_to_login_if_fc_aborted, only: [:callback]
+  before_action :check_state, only: [:callback]
+
+  STATE_COOKIE_NAME = :agentConnect_state
+  NONCE_COOKIE_NAME = :agentConnect_nonce
 
   def index
   end
 
   def login
-    redirect_to AgentConnectService.authorization_uri
+    uri, state, nonce = AgentConnectService.authorization_uri
+
+    cookies.encrypted[STATE_COOKIE_NAME] = state
+    cookies.encrypted[NONCE_COOKIE_NAME] = nonce
+
+    redirect_to uri
   end
 
   def callback
-    user_info = AgentConnectService.user_info(params[:code])
+    user_info = AgentConnectService.user_info(params[:code], cookies.encrypted[NONCE_COOKIE_NAME])
+    cookies.encrypted[NONCE_COOKIE_NAME] = nil
 
     instructeur = Instructeur.find_by(agent_connect_id: user_info['sub'])
 
@@ -50,4 +60,13 @@ class AgentConnect::AgentController < ApplicationController
     flash.alert = t('errors.messages.france_connect.connexion')
     redirect_to(new_user_session_path)
   end
+
+  def check_state
+    if cookies.encrypted[STATE_COOKIE_NAME] != params[:state]
+      flash.alert = t('errors.messages.france_connect.connexion')
+      redirect_to(new_user_session_path)
+    else
+      cookies.encrypted[STATE_COOKIE_NAME] = nil
+    end
+  end
 end

app/services/agent_connect_service.rb

@@ -1,4 +1,6 @@
 class AgentConnectService
+  include OpenIDConnect
+
   def self.enabled?
     ENV.fetch("AGENT_CONNECT_ENABLED", "enabled") == "enabled"
   end
@@ -6,19 +8,41 @@ class AgentConnectService
   def self.authorization_uri
     client = AgentConnectClient.new
 
-    client.authorization_uri(
+    state = SecureRandom.hex(16)
+    nonce = SecureRandom.hex(16)
+
+    uri = client.authorization_uri(
       scope: [:openid, :email],
-      state: SecureRandom.hex(16),
+      state: state,
-      nonce: SecureRandom.hex(16),
+      nonce: nonce,
       acr_values: 'eidas1'
     )
+
+    [uri, state, nonce]
   end
 
-  def self.user_info(code)
+  def self.user_info(code, nonce)
     client = AgentConnectClient.new(code)
 
-    client.access_token!(client_auth_method: :secret)
+    access_token = client.access_token!(client_auth_method: :secret)
+
+    discover = find_discover
+    id_token = ResponseObject::IdToken.decode(access_token.id_token, discover.jwks)
+
+    id_token.verify!(
+      client_id: AGENT_CONNECT[:identifier],
+      issuer: discover.issuer,
+      nonce: nonce
+    )
+
+    access_token
       .userinfo!
       .raw_attributes
   end
+
+  private
+
+  def self.find_discover
+    Discovery::Provider::Config.discover!("#{AGENT_CONNECT_BASE_URL}/api/v2")
+  end
 end

spec/controllers/agent_connect/agent_controller_spec.rb

@@ -1,15 +1,40 @@
 describe AgentConnect::AgentController, type: :controller do
+  describe '#login' do
+    let(:uri) { 'https://agent-connect.fr' }
+    let(:state) { 'state' }
+    let(:nonce) { 'nonce' }
+
+    before do
+      expect(AgentConnectService).to receive(:authorization_uri).and_return([uri, state, nonce])
+      get :login
+    end
+
+    it do
+      expect(state_cookie).to eq(state)
+      expect(nonce_cookie).to eq(nonce)
+      expect(response).to redirect_to(uri)
+    end
+  end
+
   describe '#callback' do
     let(:email) { 'i@email.com' }
-    subject { get :callback, params: { code: code } }
+    let(:original_state) { 'original_state' }
+    let(:nonce) { 'nonce' }
+    subject { get :callback, params: { code: code, state: state } }
+
+    before do
+      cookies.encrypted[controller.class::STATE_COOKIE_NAME] = original_state
+      cookies.encrypted[controller.class::NONCE_COOKIE_NAME] = nonce
+    end
 
     context 'when the callback code is correct' do
       let(:code) { 'correct' }
+      let(:state) { original_state }
       let(:user_info) { { 'sub' => 'sub', 'email' => ' I@email.com' } }
 
       context 'and user_info returns some info' do
         before do
-          expect(AgentConnectService).to receive(:user_info).and_return(user_info)
+          expect(AgentConnectService).to receive(:user_info).with(code, nonce).and_return(user_info)
         end
 
         context 'and the instructeur does not have an account yet' do
@@ -26,6 +51,8 @@ describe AgentConnect::AgentController, type: :controller do
             expect(last_user.confirmed_at).to be_present
             expect(last_user.instructeur.agent_connect_id).to eq('sub')
             expect(response).to redirect_to(instructeur_procedures_path)
+            expect(state_cookie).to be_nil
+            expect(nonce_cookie).to be_nil
           end
         end
 
@@ -77,8 +104,22 @@ describe AgentConnect::AgentController, type: :controller do
       end
     end
 
+    context 'when the callback state is not the original' do
+      let(:code) { 'correct' }
+      let(:state) { 'another state' }
+
+      before { subject }
+
+      it 'aborts the processus' do
+        expect { subject }.to change { User.count }.by(0).and change { Instructeur.count }.by(0)
+
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
+
     context 'when the callback code is blank' do
       let(:code) { '' }
+      let(:state) { original_state }
 
       it 'aborts the processus' do
         expect { subject }.to change { User.count }.by(0).and change { Instructeur.count }.by(0)
@@ -87,4 +128,12 @@ describe AgentConnect::AgentController, type: :controller do
       end
     end
   end
+
+  def state_cookie
+    cookies.encrypted[controller.class::STATE_COOKIE_NAME]
+  end
+
+  def nonce_cookie
+    cookies.encrypted[controller.class::NONCE_COOKIE_NAME]
+  end
 end