Merge pull request #2691 from betagouv/fix_2449_encrypted_token

Fix 2449 encrypted token

app/controllers/api/v1/dossiers_controller.rb

@@ -1,4 +1,6 @@
 class API::V1::DossiersController < APIController
+  before_action :fetch_procedure_and_check_token
+
   DEFAULT_PAGE_SIZE = 100
 
   resource_description do
@@ -13,8 +15,7 @@ class API::V1::DossiersController < APIController
   error code: 404, desc: "Démarche inconnue"
 
   def index
-    procedure = administrateur.procedures.find(params[:procedure_id])
-    dossiers = procedure.dossiers.state_not_brouillon.page(params[:page]).per(per_page)
+    dossiers = @procedure.dossiers.state_not_brouillon.page(params[:page]).per(per_page)
 
     render json: { dossiers: dossiers.map{ |dossier| DossiersSerializer.new(dossier) }, pagination: pagination(dossiers) }, status: 200
   rescue ActiveRecord::RecordNotFound
@@ -28,8 +29,7 @@ class API::V1::DossiersController < APIController
   error code: 404, desc: "Démarche ou dossier inconnu"
 
   def show
-    procedure = administrateur.procedures.find(params[:procedure_id])
-    dossier = procedure.dossiers.find(params[:id])
+    dossier = @procedure.dossiers.find(params[:id])
 
     respond_to do |format|
       format.json { render json: { dossier: DossierSerializer.new(dossier).as_json }, status: 200 }
@@ -38,6 +38,8 @@ class API::V1::DossiersController < APIController
     render json: {}, status: 404
   end
 
+  private
+
   def pagination(dossiers)
     {
       page: dossiers.current_page,
@@ -49,4 +51,15 @@ class API::V1::DossiersController < APIController
   def per_page # inherited value from will_paginate
     [params[:resultats_par_page]&.to_i || DEFAULT_PAGE_SIZE, 1000].min
   end
+
+  def fetch_procedure_and_check_token
+    @procedure = Procedure.includes(:administrateur).find(params[:procedure_id])
+
+    if !valid_token_for_administrateur?(@procedure.administrateur)
+      render json: {}, status: :unauthorized
+    end
+
+  rescue ActiveRecord::RecordNotFound
+    render json: {}, status: :not_found
+  end
 end

app/controllers/api/v1/procedures_controller.rb

@@ -1,4 +1,6 @@
 class API::V1::ProceduresController < APIController
+  before_action :fetch_procedure_and_check_token
+
   resource_description do
     description AUTHENTICATION_TOKEN_DESCRIPTION
   end
@@ -9,11 +11,19 @@ class API::V1::ProceduresController < APIController
   error code: 404, desc: "Démarche inconnue"
 
   def show
-    procedure = administrateur.procedures.find(params[:id]).decorate
+    render json: { procedure: ProcedureSerializer.new(@procedure.decorate).as_json }
+  end
 
-    render json: { procedure: ProcedureSerializer.new(procedure).as_json }
-  rescue ActiveRecord::RecordNotFound => e
-    Rails.logger.error(e.message)
-    render json: {}, status: 404
+  private
+
+  def fetch_procedure_and_check_token
+    @procedure = Procedure.includes(:administrateur).find(params[:id])
+
+    if !valid_token_for_administrateur?(@procedure.administrateur)
+      render json: {}, status: :unauthorized
+    end
+
+  rescue ActiveRecord::RecordNotFound
+    render json: {}, status: :not_found
   end
 end

app/controllers/api_controller.rb

@@ -7,48 +7,22 @@ class APIController < ApplicationController
     ```
   EOS
 
-  # deny request with an empty token as we do not want it
-  # to match the first admin with an empty token
-  # it should not happen as an empty token is serialized by ''
-  # and a administrateur without token has admin.api_token == nil
-  before_action :ensure_token_is_present
-  before_action :authenticate_user
   before_action :default_format_json
 
-  def authenticate_user
-    if !valid_token?
-      request_http_token_authentication
-    end
-  end
-
   protected
 
-  def valid_token?
-    administrateur.present?
+  def valid_token_for_administrateur?(administrateur)
+    administrateur.valid_api_token?(token)
   end
 
-  def administrateur
-    @administrateur ||= (authenticate_with_bearer_token || authenticate_with_param_token)
-  end
-
-  def authenticate_with_bearer_token
-    authenticate_with_http_token do |token, options|
-      Administrateur.find_by(api_token: token)
-    end
-  end
-
-  def authenticate_with_param_token
-    Administrateur.find_by(api_token: params[:token])
-  end
+  private
 
   def default_format_json
     request.format = "json" if !request.params[:format]
   end
 
-  def ensure_token_is_present
-    if params[:token].blank? && header_token.blank?
-      render json: {}, status: 401
-    end
+  def token
+    params_token.presence || header_token
   end
 
   def header_token
@@ -58,4 +32,8 @@ class APIController < ApplicationController
     end
     received_token
   end
+
+  def params_token
+    params[:token]
+  end
 end

app/models/administrateur.rb

@@ -39,10 +39,16 @@ class Administrateur < ApplicationRecord
   def renew_api_token
     api_token = Administrateur.generate_unique_secure_token
     encrypted_token = BCrypt::Password.create(api_token)
-    update(api_token: api_token, encrypted_token: encrypted_token)
+    update(encrypted_token: encrypted_token)
     api_token
   end
 
+  def valid_api_token?(api_token)
+    BCrypt::Password.new(encrypted_token) == api_token
+  rescue BCrypt::Errors::InvalidHash
+    false
+  end
+
   def registration_state
     if active?
       'Actif'

db/migrate/20180926145604_delete_api_token_column_from_administrateur.rb

@@ -0,0 +1,5 @@
+class DeleteAPITokenColumnFromAdministrateur < ActiveRecord::Migration[5.2]
+  def change
+    remove_column :administrateurs, :api_token, :string
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2018_09_25_084403) do
+ActiveRecord::Schema.define(version: 2018_09_26_145604) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -50,7 +50,6 @@ ActiveRecord::Schema.define(version: 2018_09_25_084403) do
     t.string "last_sign_in_ip"
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.string "api_token"
     t.boolean "active", default: false
     t.jsonb "features", default: {}, null: false
     t.string "encrypted_token"

spec/controllers/api/v1/dossiers_controller_spec.rb

@@ -1,14 +1,15 @@
 require 'spec_helper'
 
 describe API::V1::DossiersController do
-  let(:admin) { create(:administrateur, :with_api_token) }
+  let(:admin) { create(:administrateur) }
+  let(:token) { admin.renew_api_token }
   let(:procedure) { create(:procedure, :with_two_type_de_piece_justificative, :with_type_de_champ, :with_type_de_champ_private, administrateur: admin) }
   let(:wrong_procedure) { create(:procedure) }
 
   it { expect(described_class).to be < APIController }
 
   describe 'GET index (with bearer token)' do
-    let(:authorization_header) { ActionController::HttpAuthentication::Token.encode_credentials(admin.api_token) }
+    let(:authorization_header) { ActionController::HttpAuthentication::Token.encode_credentials(token) }
     let(:retour) do
       request.env['HTTP_AUTHORIZATION'] = authorization_header
       get :index, params: { procedure_id: procedure_id }
@@ -23,7 +24,7 @@ describe API::V1::DossiersController do
   end
 
   describe 'GET index' do
-    let(:retour) { get :index, params: { token: admin.api_token, procedure_id: procedure_id } }
+    let(:retour) { get :index, params: { token: token, procedure_id: procedure_id } }
 
     subject { retour }
 
@@ -34,7 +35,7 @@ describe API::V1::DossiersController do
 
     context 'when procedure does not belong to admin' do
       let(:procedure_id) { wrong_procedure.id }
-      it { expect(subject.code).to eq('404') }
+      it { expect(subject.code).to eq('401') }
     end
 
     context 'when procedure is found and belongs to admin' do
@@ -62,7 +63,7 @@ describe API::V1::DossiersController do
       end
 
       describe 'with custom resultats_par_page' do
-        let(:retour) { get :index, params: { token: admin.api_token, procedure_id: procedure_id, resultats_par_page: 18 } }
+        let(:retour) { get :index, params: { token: token, procedure_id: procedure_id, resultats_par_page: 18 } }
         subject { body[:pagination] }
         it { is_expected.to have_key(:resultats_par_page) }
         it { expect(subject[:resultats_par_page]).to eq(18) }
@@ -81,7 +82,7 @@ describe API::V1::DossiersController do
       end
 
       context 'when there are multiple pages' do
-        let(:retour) { get :index, params: { token: admin.api_token, procedure_id: procedure_id, page: 2 } }
+        let(:retour) { get :index, params: { token: token, procedure_id: procedure_id, page: 2 } }
 
         let!(:dossier1) { create(:dossier, :with_entreprise, procedure: procedure, state: Dossier.states.fetch(:en_construction)) }
         let!(:dossier2) { create(:dossier, :with_entreprise, procedure: procedure, state: Dossier.states.fetch(:en_construction)) }
@@ -102,7 +103,7 @@ describe API::V1::DossiersController do
   end
 
   describe 'GET show' do
-    let(:retour) { get :show, params: { token: admin.api_token, procedure_id: procedure_id, id: dossier_id } }
+    let(:retour) { get :show, params: { token: token, procedure_id: procedure_id, id: dossier_id } }
     subject { retour }
 
     context 'when procedure is not found' do
@@ -114,7 +115,7 @@ describe API::V1::DossiersController do
     context 'when procedure exists and does not belong to current admin' do
       let(:procedure_id) { wrong_procedure.id }
       let(:dossier_id) { 1 }
-      it { expect(subject.code).to eq('404') }
+      it { expect(subject.code).to eq('401') }
     end
 
     context 'when procedure is found and belongs to current admin' do

spec/controllers/api/v1/procedures_controller_spec.rb

@@ -1,31 +1,34 @@
-require 'spec_helper'
-
 describe API::V1::ProceduresController, type: :controller do
-  let(:admin) { create(:administrateur, :with_api_token) }
+  let!(:admin) { create(:administrateur, :with_api_token) }
+  let!(:token) { admin.renew_api_token }
+
   it { expect(described_class).to be < APIController }
 
   describe 'GET show' do
-    context 'when procedure does not exist' do
-      subject { get :show, params: { id: 999_999_999, token: admin.api_token } }
-      it { expect(subject.status).to eq(404) }
-    end
-    context 'when procedure does not belong to administrateur' do
-      let(:procedure) { create(:procedure, administrateur: create(:administrateur)) }
-      subject { get :show, params: { id: procedure, token: admin.api_token } }
-      it { expect(subject.status).to eq(404) }
-    end
-    context 'when procedure exist' do
-      let(:procedure) { create(:procedure, :with_two_type_de_piece_justificative, :with_type_de_champ, administrateur: admin) }
-      subject { get :show, params: { id: procedure, token: admin.api_token } }
+    subject { get :show, params: { id: procedure_id, token: token } }
 
-      it 'return REST code 200', :show_in_doc do
-        expect(subject.status).to eq(200)
-      end
+    context 'when procedure does not exist' do
+      let(:procedure_id) { 999_999_999 }
+
+      it { is_expected.to have_http_status(404) }
+    end
+
+    context 'when procedure belongs to administrateur without token' do
+      let(:procedure_id) { create(:procedure).id }
+
+      it { is_expected.to have_http_status(401) }
+    end
+
+    context 'when procedure exist' do
+      let(:procedure_id) { create(:procedure, administrateur: admin).id }
+
+      it { is_expected.to have_http_status(200) }
 
       describe 'body' do
         let(:module_api_carto) { create(:module_api_carto, use_api_carto: true, quartiers_prioritaires: true, cadastre: true) }
         let(:procedure) { create(:procedure, :with_type_de_champ, :with_two_type_de_piece_justificative, module_api_carto: module_api_carto, administrateur: admin) }
-        let(:response) { get :show, params: { id: procedure.id, token: admin.api_token } }
+        let(:response) { get :show, params: { id: procedure.id, token: token } }
+
         subject { JSON.parse(response.body, symbolize_names: true)[:procedure] }
 
         it { expect(subject[:id]).to eq(procedure.id) }
@@ -38,9 +41,12 @@ describe API::V1::ProceduresController, type: :controller do
         it { expect(subject[:total_dossier]).to eq(procedure.total_dossier) }
         it { is_expected.to have_key(:types_de_champ) }
         it { expect(subject[:types_de_champ]).to be_an(Array) }
+
         describe 'type_de_champ' do
           subject { super()[:types_de_champ][0] }
+
           let(:champ) { procedure.types_de_champ.first }
+
           it { expect(subject[:id]).to eq(champ.id) }
           it { expect(subject[:libelle]).to eq(champ.libelle) }
           it { expect(subject[:type_champ]).to eq(champ.type_champ) }
@@ -50,16 +56,22 @@ describe API::V1::ProceduresController, type: :controller do
 
         it { is_expected.to have_key(:types_de_piece_justificative) }
         it { expect(subject[:types_de_piece_justificative]).to be_an(Array) }
+
         describe 'type_de_piece_jointe' do
           subject { super()[:types_de_piece_justificative][0] }
+
           let(:pj) { procedure.types_de_piece_justificative.first }
+
           it { expect(subject[:id]).to eq(pj.id) }
           it { expect(subject[:libelle]).to eq(pj.libelle) }
           it { expect(subject[:description]).to eq(pj.description) }
         end
+
         it { is_expected.to have_key(:geographic_information) }
+
         describe 'geographic_information' do
           subject { super()[:geographic_information] }
+
           it { expect(subject[:use_api_carto]).to be_truthy }
           it { expect(subject[:quartiers_prioritaires]).to be_truthy }
           it { expect(subject[:cadastre]).to be_truthy }

spec/controllers/api_controller_spec.rb

@@ -1,55 +1,38 @@
 require 'spec_helper'
 
 describe APIController, type: :controller do
-  controller(APIController) do
-    def show
-      render json: {}, satus: 200
+  describe 'valid_token_for_administrateur?' do
+    let!(:admin) { create(:administrateur) }
+
+    subject { controller.send(:'valid_token_for_administrateur?', admin) }
+
+    context 'when the admin has not any token' do
+      context 'and the token is not given' do
+        it { is_expected.to be false }
+      end
     end
 
-    def index
-      render json: {}, satus: 200
-    end
-  end
+    context 'when the admin has a token' do
+      let!(:token) { admin.renew_api_token }
 
-  describe 'GET index' do
-    let!(:administrateur) { create(:administrateur) }
-    let!(:administrateur_with_token) { create(:administrateur, :with_api_token) }
+      context 'and the token is given by params' do
+        before { controller.params[:token] = token }
 
-    context 'when token is missing' do
-      subject { get :index }
-
-      it { expect(subject.status).to eq(401) }
-    end
-
-    context 'when token is empty' do
-      subject { get :index, params: { token: nil } }
-
-      it { expect(subject.status).to eq(401) }
-    end
-
-    context 'when token does not exist' do
-      let(:token) { 'invalid_token' }
-
-      subject { get :index, params: { token: token } }
-
-      it { expect(subject.status).to eq(401) }
-    end
-
-    context 'when token exist in the params' do
-      subject { get :index, params: { token: administrateur_with_token.api_token } }
-
-      it { expect(subject.status).to eq(200) }
-    end
-
-    context 'when token exist in the header' do
-      before do
-        valid_headers = { 'Authorization' => "Bearer token=#{administrateur_with_token.api_token}" }
-        request.headers.merge!(valid_headers)
+        it { is_expected.to be true }
       end
 
-      subject { get(:index) }
+      context 'and the token is given by header' do
+        before do
+          valid_headers = { 'Authorization' => "Bearer token=#{token}" }
+          request.headers.merge!(valid_headers)
+        end
 
-      it { expect(subject.status).to eq(200) }
+        it { is_expected.to be true }
+      end
+
+      context 'and the token is not given' do
+        it { is_expected.to be false }
+      end
     end
   end
 end

spec/models/administrateur_spec.rb

@@ -33,21 +33,15 @@ describe Administrateur, type: :model do
   end
 
   describe "#renew_api_token" do
-    let(:administrateur) { create(:administrateur) }
+    let!(:administrateur) { create(:administrateur) }
+    let!(:token) { administrateur.renew_api_token }
 
-    before do
-      administrateur.renew_api_token
-      administrateur.reload
-    end
-
-    it { expect(administrateur.api_token).to be_present }
-    it { expect(administrateur.api_token).not_to eq(administrateur.encrypted_token) }
-    it { expect(BCrypt::Password.new(administrateur.encrypted_token)).to eq(administrateur.api_token) }
+    it { expect(BCrypt::Password.new(administrateur.encrypted_token)).to eq(token) }
 
     context 'when it s called twice' do
-      let!(:previous_token) { administrateur.api_token }
+      let!(:new_token) { administrateur.renew_api_token }
 
-      it { expect(previous_token).not_to eq(administrateur.renew_api_token) }
+      it { expect(new_token).not_to eq(token) }
     end
   end
 

spec/views/admin/gestionnaires/index.html.haml_spec.rb

@@ -1,8 +1,7 @@
 require 'spec_helper'
 
 describe 'admin/gestionnaires/index.html.haml', type: :view do
-  let(:token) { 'super_token' }
-  let(:admin) { create(:administrateur, api_token: token) }
+  let(:admin) { create(:administrateur) }
 
   before do
     assign(:gestionnaires, (smart_listing_create :gestionnaires,