Merge pull request #2459 from betagouv/partially_fix_2449_encrypted_token

Partially fix 2449 encrypted token

Gemfile

@@ -61,6 +61,7 @@ gem 'fog-openstack'
 gem 'pg'
 
 gem 'rbnacl-libsodium'
+gem 'bcrypt'
 
 gem 'rgeo-geojson'
 gem 'leaflet-rails'

Gemfile.lock

@@ -812,6 +812,7 @@ DEPENDENCIES
   after_party
   apipie-rails
   axlsx (~> 3.0.0.pre)
+  bcrypt
   bootstrap-sass (~> 3.3.5)
   bootstrap-wysihtml5-rails (~> 0.3.3.8)
   brakeman

app/assets/stylesheets/new_design/profil.scss

@@ -0,0 +1,15 @@
+@import "constants";
+
+#profil-page {
+  b {
+    font-weight: bold;
+  }
+
+  .card {
+    margin: 3 * $default-spacer 0;
+  }
+
+  p {
+    margin: 16px auto;
+  }
+}

app/controllers/admin/profile_controller.rb

@@ -1,11 +0,0 @@
-class Admin::ProfileController < AdminController
-  def show
-    @administrateur = current_administrateur
-  end
-
-  def renew_api_token
-    flash[:notice] = "Votre token d'API a été regénéré."
-    current_administrateur.renew_api_token
-    redirect_to admin_profile_path
-  end
-end

app/controllers/api_controller.rb

@@ -7,6 +7,11 @@ 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
 
@@ -39,4 +44,18 @@ class APIController < ApplicationController
   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
+  end
+
+  def header_token
+    received_token = nil
+    authenticate_with_http_token do |token, _options|
+      received_token = token
+    end
+    received_token
+  end
 end

app/controllers/new_administrateur/profil_controller.rb

@@ -0,0 +1,12 @@
+module NewAdministrateur
+  class ProfilController < AdministrateurController
+    def show
+    end
+
+    def renew_api_token
+      @token = current_administrateur.renew_api_token
+      flash.now.notice = 'Votre jeton a été regénéré.'
+      render :show
+    end
+  end
+end

app/models/administrateur.rb

@@ -1,6 +1,7 @@
 class Administrateur < ApplicationRecord
   include CredentialsSyncableConcern
   include EmailSanitizableConcern
+  include ActiveRecord::SecureToken
 
   devise :database_authenticatable, :registerable, :async,
     :recoverable, :rememberable, :trackable, :validatable
@@ -13,7 +14,6 @@ class Administrateur < ApplicationRecord
   has_many :dossiers, -> { state_not_brouillon }, through: :procedures
 
   before_validation -> { sanitize_email(:email) }
-  before_save :ensure_api_token
 
   scope :inactive, -> { where(active: false) }
 
@@ -36,14 +36,11 @@ class Administrateur < ApplicationRecord
     self.inactive.find(id)
   end
 
-  def ensure_api_token
-    if api_token.nil?
-      self.api_token = generate_api_token
-    end
-  end
-
   def renew_api_token
-    update(api_token: generate_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)
+    api_token
   end
 
   def registration_state
@@ -116,13 +113,4 @@ class Administrateur < ApplicationRecord
   def owns?(procedure)
     id == procedure.administrateur_id
   end
-
-  private
-
-  def generate_api_token
-    loop do
-      token = SecureRandom.hex(20)
-      break token if !Administrateur.find_by(api_token: token)
-    end
-  end
 end

app/views/admin/profile/show.html.haml

@@ -1,8 +0,0 @@
-#profile_page
-%h2 Profil
-%hr
-%p
-  API TOKEN :
-  = @administrateur.api_token
-%p
-  = link_to "Regénérer mon token", admin_renew_api_token_path, method: :post, class: "btn btn-default", data: { confirm: "Confirmez-vous la regénération de votre token ? Les applications qui l'utilisent actuellement seront bloquées.", disable: true }

app/views/layouts/_navbar.html.haml

@@ -9,6 +9,8 @@
 .col-xs-10.no-padding
   #navbar-body
     .row
+      -# BEST WTF EVER
+      -# this begin rescue hides potentials bugs by displaying another navbar
       - begin
         = render partial: @navbar_url
       - rescue

app/views/layouts/navbars/_navbar_admin_procedurescontroller_index.html.haml

@@ -21,6 +21,6 @@
             = t('dynamics.admin.menu.instructeurs')
         %li.divider{ role: :separator }
         %li
-          = link_to(admin_profile_path, id: :profile) do
+          = link_to(profil_path, id: :profile) do
             %i.fa.fa-user
              Profil

app/views/new_administrateur/profil/show.html.haml

@@ -0,0 +1,25 @@
+= render partial: 'new_administrateur/breadcrumbs',
+  locals: { steps: [link_to('Tableau de bord', admin_procedures_path),
+                    'Profil'] }
+
+#profil-page.container
+  %h1 Profil
+
+  .card
+    .card-title Jeton d'identification de l'API (token)
+    %p Ce jeton est nécessaire pour effectuer des appels vers l'API de demarches-simplifiees.fr.
+
+    - if defined?(@token)
+      %p Jeton : <b>#{@token}</b>
+      %p Pour des raisons de sécurité, ce jeton ne sera plus ré-affiché, notez-le bien.
+
+    - else
+      %p Pour des raisons de sécurité, nous ne pouvons vous l'afficher que lors de sa génération.
+      %p Attention, si vous avez déjà des applications qui utilisent votre jeton, le regénérer bloquera leurs accès à l'API.
+
+    = link_to "Regénérer et afficher mon jeton",
+      renew_api_token_path,
+      method: :post,
+      class: "button primary",
+      data: { confirm: "Confirmez-vous la regénération de votre jeton ? Les applications qui l'utilisent actuellement seront bloquées.",
+              disable: true }

config/routes.rb

@@ -181,8 +181,6 @@ Rails.application.routes.draw do
     get 'procedures/draft' => 'procedures#draft'
     get 'procedures/path_list' => 'procedures#path_list'
     get 'procedures/available' => 'procedures#check_availability'
-    get 'profile' => 'profile#show', as: :profile
-    post 'renew_api_token' => 'profile#renew_api_token', as: :renew_api_token
 
     get 'change_dossier_state' => 'change_dossier_state#index'
     post 'change_dossier_state' => 'change_dossier_state#check'
@@ -370,6 +368,11 @@ Rails.application.routes.draw do
         patch 'add_to_procedure'
       end
     end
+
+    get 'profil' => 'profil#show'
+    post 'renew-api-token' => 'profil#renew_api_token'
+    # allow refresh 'renew api token' page
+    get 'renew-api-token' => redirect('/profil')
   end
 
   apipie

db/migrate/20180824142849_add_encrypted_token_column_to_administrateur.rb

@@ -0,0 +1,5 @@
+class AddEncryptedTokenColumnToAdministrateur < ActiveRecord::Migration[5.2]
+  def change
+    add_column :administrateurs, :encrypted_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_24_074121) do
+ActiveRecord::Schema.define(version: 2018_09_25_084403) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -53,6 +53,7 @@ ActiveRecord::Schema.define(version: 2018_09_24_074121) do
     t.string "api_token"
     t.boolean "active", default: false
     t.jsonb "features", default: {}, null: false
+    t.string "encrypted_token"
     t.index ["email"], name: "index_administrateurs_on_email", unique: true
     t.index ["reset_password_token"], name: "index_administrateurs_on_reset_password_token", unique: true
   end

lib/tasks/2018_08_24_encrypt_tokens.rake

@@ -0,0 +1,10 @@
+namespace :'2018_08_24_encrypt_tokens' do
+  task run: :environment do
+    Administrateur
+      .where
+      .not(api_token: nil)
+      .each do |admin|
+      admin.update(encrypted_token: BCrypt::Password.create(admin.api_token))
+    end
+  end
+end

spec/controllers/admin/profile_controller_spec.rb

@@ -1,16 +0,0 @@
-require 'spec_helper'
-
-describe Admin::ProfileController, type: :controller do
-  it { expect(described_class).to be < AdminController }
-  let(:administrateur) { create(:administrateur) }
-
-  before { sign_in(administrateur) }
-
-  describe 'POST #renew_api_token' do
-    subject { post :renew_api_token }
-
-    it { expect{ subject }.to change { administrateur.reload.api_token } }
-
-    it { subject; expect(response.status).to redirect_to(admin_profile_path) }
-  end
-end

spec/controllers/api/v1/dossiers_controller_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe API::V1::DossiersController do
-  let(:admin) { create(:administrateur) }
+  let(:admin) { create(:administrateur, :with_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) }
 

spec/controllers/api/v1/procedures_controller_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe API::V1::ProceduresController, type: :controller do
-  let(:admin) { create(:administrateur) }
+  let(:admin) { create(:administrateur, :with_api_token) }
   it { expect(described_class).to be < APIController }
 
   describe 'GET show' do

spec/controllers/api_controller_spec.rb

@@ -12,18 +12,43 @@ describe APIController, type: :controller do
   end
 
   describe 'GET index' do
+    let!(:administrateur) { create(:administrateur) }
+    let!(:administrateur_with_token) { create(:administrateur, :with_api_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' do
+
-      let(:administrateur) { create(:administrateur) }
+    context 'when token exist in the params' do
-      subject { get :index, params: { token: administrateur.api_token } }
+      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)
+      end
+
+      subject { get(:index) }
+
       it { expect(subject.status).to eq(200) }
     end
   end

spec/controllers/new_administrateur/profil_controller_spec.rb

@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe NewAdministrateur::ProfilController, type: :controller do
+  let(:administrateur) { create(:administrateur) }
+
+  before { sign_in(administrateur) }
+
+  describe 'POST #renew_api_token' do
+    before do
+      allow(administrateur).to receive(:renew_api_token)
+      allow(controller).to receive(:current_administrateur) { administrateur }
+      post :renew_api_token
+    end
+
+    it { expect(administrateur).to have_received(:renew_api_token) }
+    it { expect(response.status).to render_template(:show) }
+    it { expect(flash.notice).to eq('Votre jeton a été regénéré.') }
+  end
+end

spec/factories/administrateur.rb

@@ -4,4 +4,10 @@ FactoryBot.define do
     email { generate(:administrateur_email) }
     password { 'mon chien aime les bananes' }
   end
+
+  trait :with_api_token do
+    after(:create) do |admin|
+      admin.renew_api_token
+    end
+  end
 end

spec/features/admin/connection_spec.rb

@@ -43,12 +43,11 @@ feature 'Administrator connection' do
           page.find_by_id('profile').click
         end
         scenario 'it redirects to profile page' do
-          expect(page).to have_css('#profile_page')
+          expect(page).to have_css('#profil-page')
         end
         context 'when clicking on procedure' do
           before do
-            page.find_by_id('admin_menu').click
+            page.click_on('Tableau de bord').click
-            page.find_by_id('menu_item_procedure').click
           end
 
           scenario 'it redirects to procedure page' do

spec/models/administrateur_spec.rb

@@ -8,27 +8,6 @@ describe Administrateur, type: :model do
     it { is_expected.to have_many(:procedures) }
   end
 
-  describe 'after_save' do
-    subject { create(:administrateur) }
-    before do
-      subject.save
-    end
-    it { expect(subject.api_token).not_to be_blank }
-  end
-
-  describe 'generate_api_token' do
-    let(:token) { 'bullshit' }
-    let(:new_token) { 'pocket_master' }
-    let!(:admin_1) { create(:administrateur, api_token: token) }
-    before do
-      allow(SecureRandom).to receive(:hex).and_return(token, new_token)
-      admin_1.renew_api_token
-    end
-    it 'generate a token who does not already exist' do
-      expect(admin_1.api_token).to eq(new_token)
-    end
-  end
-
   context 'unified login' do
     it 'syncs credentials to associated user' do
       administrateur = create(:administrateur)
@@ -53,6 +32,25 @@ describe Administrateur, type: :model do
     end
   end
 
+  describe "#renew_api_token" do
+    let(:administrateur) { create(:administrateur) }
+
+    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) }
+
+    context 'when it s called twice' do
+      let!(:previous_token) { administrateur.api_token }
+
+      it { expect(previous_token).not_to eq(administrateur.renew_api_token) }
+    end
+  end
+
   describe '#find_inactive_by_token' do
     let(:administrateur) { create(:administration).invite_admin('paul@tps.fr') }
     let(:reset_password_token) { administrateur.invite!(administration.id) }

spec/views/admin/profile/show.html.haml_spec.rb

@@ -1,11 +0,0 @@
-require 'spec_helper'
-
-describe 'admin/profile/show.html.haml', type: :view do
-  let(:token) { 'super_token' }
-  let(:admin) { create(:administrateur, api_token: token) }
-  before do
-    assign(:administrateur, admin)
-    render
-  end
-  it { expect(rendered).to have_content(token) }
-end