From 0b03ba4d68b51e95d40c9c4d6dee893869635f06 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 2 Aug 2023 19:33:42 +0200 Subject: [PATCH 01/18] remove v1/v2 api token logic --- app/controllers/api/v2/base_controller.rb | 7 ---- app/controllers/api_controller.rb | 2 +- app/graphql/api/v2/context.rb | 15 +------- app/models/api_token.rb | 46 ++--------------------- 4 files changed, 5 insertions(+), 65 deletions(-) diff --git a/app/controllers/api/v2/base_controller.rb b/app/controllers/api/v2/base_controller.rb index 1deaa29cb..342de720e 100644 --- a/app/controllers/api/v2/base_controller.rb +++ b/app/controllers/api/v2/base_controller.rb @@ -8,7 +8,6 @@ class API::V2::BaseController < ApplicationController private def context - # new token if api_token.present? api_token.context # web interface (/graphql) give current_administrateur @@ -18,12 +17,6 @@ class API::V2::BaseController < ApplicationController procedure_ids: current_administrateur.procedure_ids, write_access: true } - # old token - else - { - token: authorization_bearer_token, - write_access: true - } end end diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 3a3adf757..55eedd8a1 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -4,7 +4,7 @@ class APIController < ApplicationController protected def find_administrateur_for_token(procedure) - api_token = APIToken.find_and_verify(authorization_bearer_token, procedure.administrateurs) + api_token = APIToken.find_and_verify(authorization_bearer_token) if api_token.present? && api_token.context.fetch(:procedure_ids).include?(procedure.id) api_token.touch(:last_v1_authenticated_at) api_token.administrateur diff --git a/app/graphql/api/v2/context.rb b/app/graphql/api/v2/context.rb index 22ccb3162..824c9cd30 100644 --- a/app/graphql/api/v2/context.rb +++ b/app/graphql/api/v2/context.rb @@ -75,20 +75,7 @@ class API::V2::Context < GraphQL::Query::Context def compute_demarche_authorization(demarche) # procedure_ids and token are passed from graphql controller - if self[:procedure_ids].present? - self[:procedure_ids].include?(demarche.id) - elsif self[:token].present? - token = APIToken.find_and_verify(self[:token], demarche.administrateurs) - if token.present? - token.touch(:last_v2_authenticated_at) - Current.user = token.administrateur.user - true - else - false - end - else - false - end + (self[:procedure_ids] || []).include?(demarche.id) end # This is a query AST visitor that we use to check diff --git a/app/models/api_token.rb b/app/models/api_token.rb index 4616969b8..7e5ded57a 100644 --- a/app/models/api_token.rb +++ b/app/models/api_token.rb @@ -55,53 +55,13 @@ class APIToken < ApplicationRecord [api_token, packed_token] end - def find_and_verify(maybe_packed_token, administrateurs = []) - token = case unpack(maybe_packed_token) - in { plain_token:, id: } # token v3 - find_by(id:, version: 3)&.then(&ensure_valid_token(plain_token)) - in { plain_token:, administrateur_id: } # token v2 - # the migration to the APIToken model set `version: 1` for all the v1 and v2 token - # this is the only place where we can fix the version - where(administrateur_id:, version: 1).update_all(version: 2) # update to v2 - find_by(administrateur_id:, version: 2)&.then(&ensure_valid_token(plain_token)) - in { plain_token: } # token v1 - where(administrateur: administrateurs, version: 1).find(&ensure_valid_token(plain_token)) - end - - # TODO: - # remove all the not v3 version code - # when everyone has migrated - # it should also be a good place in case we need to feature flag old token use - if token&.version == 3 || Rails.env.test? - token - else - nil - end + def find_and_verify(base64_packed_token) + id, plain_token = Base64.urlsafe_decode64(base64_packed_token).split(';') + find_by(id:, version: 3)&.then(&ensure_valid_token(plain_token)) end private - UUID_SIZE = SecureRandom.uuid.size - def unpack(maybe_packed_token) - case message_verifier.verified(maybe_packed_token) - in [administrateur_id, plain_token] - { plain_token:, administrateur_id: } - else - case Base64.urlsafe_decode64(maybe_packed_token).split(';') - in [id, plain_token] if id.size == UUID_SIZE # valid format ";" - { plain_token:, id: } - else - { plain_token: maybe_packed_token } - end - end - rescue - { plain_token: maybe_packed_token } - end - - def message_verifier - Rails.application.message_verifier('api_v2_token') - end - def ensure_valid_token(plain_token) -> (api_token) { api_token if BCrypt::Password.new(api_token.encrypted_token) == plain_token } end From c7afad2a88795325e133340ab46ef41d72447e94 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 3 Aug 2023 10:23:05 +0200 Subject: [PATCH 02/18] refacto: spec --- .../api/v2/graphql_controller_spec.rb | 33 +---- spec/models/api_token_spec.rb | 138 +++--------------- 2 files changed, 22 insertions(+), 149 deletions(-) diff --git a/spec/controllers/api/v2/graphql_controller_spec.rb b/spec/controllers/api/v2/graphql_controller_spec.rb index 1ffeedf5e..3447beaec 100644 --- a/spec/controllers/api/v2/graphql_controller_spec.rb +++ b/spec/controllers/api/v2/graphql_controller_spec.rb @@ -3,7 +3,6 @@ describe API::V2::GraphqlController do let(:generated_token) { APIToken.generate(admin) } let(:api_token) { generated_token.first } let(:token) { generated_token.second } - let(:legacy_token) { APIToken.send(:unpack, token)[:plain_token] } let(:procedure) { create(:procedure, :published, :for_individual, :with_service, administrateurs: [admin]) } let(:dossier) { create(:dossier, :en_construction, :with_individual, procedure: procedure) } let(:dossier1) { create(:dossier, :en_construction, :with_individual, procedure: procedure, en_construction_at: 1.day.ago) } @@ -113,20 +112,6 @@ describe API::V2::GraphqlController do subject { post :execute, params: { query: query, variables: variables, operationName: operation_name, queryId: query_id }.compact, as: :json } - context "when authenticated with legacy token" do - let(:authorization_header) { ActionController::HttpAuthentication::Token.encode_credentials(legacy_token) } - - before do - request.env['HTTP_AUTHORIZATION'] = authorization_header - admin.api_tokens.first.update(version: 1) - end - - it "returns the demarche" do - expect(gql_errors).to eq(nil) - expect(gql_data[:demarche][:id]).to eq(procedure.to_typed_id) - end - end - context "when authenticated" do let(:authorization_header) { ActionController::HttpAuthentication::Token.encode_credentials(token) } @@ -164,18 +149,6 @@ describe API::V2::GraphqlController do expect(gql_errors.first[:message]).to eq("An object of type Demarche was hidden due to permissions") } end - context 'v2' do - let(:token) { APIToken.send(:message_verifier).generate([another_administrateur.id, plain_token]) } - it { - expect(gql_errors.first[:message]).to eq("An object of type Demarche was hidden due to permissions") - } - end - context 'v1' do - let(:token) { plain_token } - it { - expect(gql_errors.first[:message]).to eq("An object of type Demarche was hidden due to permissions") - } - end end context "when the token is revoked" do @@ -1499,13 +1472,15 @@ describe API::V2::GraphqlController do message { body } + errors { + message + } } }" end it "should return error" do - expect(gql_data[:dossierEnvoyerMessage]).to eq(nil) - expect(gql_errors).not_to eq(nil) + expect(gql_data[:dossierEnvoyerMessage][:errors].first[:message]).to eq("Le jeton utilisé est configuré seulement en lecture") end end end diff --git a/spec/models/api_token_spec.rb b/spec/models/api_token_spec.rb index ce07a1808..714ebc99e 100644 --- a/spec/models/api_token_spec.rb +++ b/spec/models/api_token_spec.rb @@ -1,12 +1,11 @@ describe APIToken, type: :model do let(:administrateur) { create(:administrateur) } - let(:api_token_and_packed_token) { APIToken.generate(administrateur) } - let(:api_token) { api_token_and_packed_token.first } - let(:packed_token) { api_token_and_packed_token.second } - let(:plain_token) { APIToken.send(:unpack, packed_token)[:plain_token] } - let(:packed_token_v2) { APIToken.send(:message_verifier).generate([administrateur.id, plain_token]) } describe '#generate' do + let(:api_token_and_packed_token) { APIToken.generate(administrateur) } + let(:api_token) { api_token_and_packed_token.first } + let(:packed_token) { api_token_and_packed_token.second } + it do expect(api_token.administrateur).to eq(administrateur) expect(api_token.prefix).to eq(packed_token.slice(0, 5)) @@ -109,137 +108,36 @@ describe APIToken, type: :model do end describe '#find_and_verify' do - let(:result) { APIToken.find_and_verify(token, administrateurs) } - let(:token) { packed_token } - let(:administrateurs) { [administrateur] } + let(:api_token_and_packed_token) { APIToken.generate(administrateur) } + let(:api_token) { api_token_and_packed_token.first } + let(:packed_token) { api_token_and_packed_token.second } + let(:bearer_token) { packed_token } - context 'without administrateur' do - let(:administrateurs) { [] } + subject { APIToken.find_and_verify(bearer_token) } - context 'with packed token' do - it { expect(result).to be_truthy } - end - - context 'with packed token v2' do - before { api_token.update(version: 2) } - let(:token) { packed_token_v2 } - it { expect(result).to be_truthy } - end - - context 'with plain token' do - before { api_token.update(version: 1) } - let(:token) { plain_token } - it { expect(result).to be_falsey } - end + context 'with the legit packed token' do + it { is_expected.to eq(api_token) } end context 'with destroyed token' do before { api_token.destroy } - context 'with packed token' do - it { expect(result).to be_falsey } - end - - context 'with packed token v2' do - let(:token) { packed_token_v2 } - it { expect(result).to be_falsey } - end - - context 'with plain token' do - let(:token) { plain_token } - it { expect(result).to be_falsey } - end + it { is_expected.to be_nil } end context 'with destroyed administrateur' do before { api_token.administrateur.destroy } - let(:administrateurs) { [] } - context 'with packed token' do - it { expect(result).to be_falsey } - end - - context 'with packed token v2' do - let(:token) { packed_token_v2 } - it { expect(result).to be_falsey } - end - - context 'with plain token' do - let(:token) { plain_token } - it { expect(result).to be_falsey } - end + it { is_expected.to be_nil } end - context 'with other administrateur' do - let(:other_administrateur) { create(:administrateur, :with_api_token) } - let(:administrateurs) { [other_administrateur] } - - context 'with packed token' do - it { expect(result).to be_truthy } + context "with a bearer token with the wrong plain_token" do + let(:bearer_token) do + clear_packed = [api_token.id, 'wrong'].join(';') + Base64.urlsafe_encode64(clear_packed) end - context 'with packed token v2' do - before { api_token.update(version: 2) } - - let(:token) { packed_token_v2 } - it { expect(result).to be_truthy } - end - - context 'with plain token' do - before { api_token.update(version: 1) } - - let(:token) { plain_token } - it { expect(result).to be_falsey } - end - end - - context 'with many administrateurs' do - let(:other_administrateur) { create(:administrateur, :with_api_token) } - let(:other_api_token_and_packed_token) { APIToken.generate(other_administrateur) } - let(:other_api_token) { other_api_token_and_packed_token.first } - let(:other_packed_token) { other_api_token_and_packed_token.second } - let(:other_plain_token) { APIToken.send(:unpack, other_packed_token)[:plain_token] } - let(:administrateurs) { [administrateur, other_administrateur] } - - context 'with plain token' do - before do - api_token.update(version: 1) - other_api_token.update(version: 1) - end - - let(:token) { plain_token } - it { expect(result).to be_truthy } - - context 'with other plain token' do - let(:token) { other_plain_token } - it { expect(result).to be_truthy } - end - end - end - - context 'with packed token' do - it { expect(result).to be_truthy } - end - - context 'with packed token v2' do - before { api_token.update(version: 2) } - - let(:token) { packed_token_v2 } - it { expect(result).to be_truthy } - end - - context 'with plain token' do - before { api_token.update(version: 1) } - - let(:token) { plain_token } - it { expect(result).to be_truthy } - end - - context "with valid garbage base64" do - before { api_token.update(version: 1, encrypted_token: BCrypt::Password.create(token)) } - - let(:token) { "R5dAqE7nMxfMp93PcuuevDtn" } - it { expect(result).to be_truthy } + it { is_expected.to be_nil } end end end From 87933d3567e1fa4d37cf0c1f0d72b6910100bfb6 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 3 Aug 2023 12:04:08 +0200 Subject: [PATCH 03/18] introduce token bearer --- app/models/api_token.rb | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/app/models/api_token.rb b/app/models/api_token.rb index 7e5ded57a..c33af4840 100644 --- a/app/models/api_token.rb +++ b/app/models/api_token.rb @@ -51,19 +51,20 @@ class APIToken < ApplicationRecord plain_token = generate_unique_secure_token encrypted_token = BCrypt::Password.create(plain_token) api_token = create!(administrateur:, encrypted_token:, name: Date.today.strftime('Jeton d’API généré le %d/%m/%Y')) - packed_token = Base64.urlsafe_encode64([api_token.id, plain_token].join(';')) - [api_token, packed_token] + bearer = BearerToken.new(api_token.id, plain_token) + [api_token, bearer.to_string] end - def find_and_verify(base64_packed_token) - id, plain_token = Base64.urlsafe_decode64(base64_packed_token).split(';') - find_by(id:, version: 3)&.then(&ensure_valid_token(plain_token)) - end + def find_and_verify(bearer_string) + bearer = BearerToken.from_string(bearer_string) - private + return if bearer.nil? - def ensure_valid_token(plain_token) - -> (api_token) { api_token if BCrypt::Password.new(api_token.encrypted_token) == plain_token } + api_token = find_by(id: bearer.api_token_id, version: 3) + + return if api_token.nil? + + BCrypt::Password.new(api_token.encrypted_token) == bearer.plain_token ? api_token : nil end end @@ -74,4 +75,18 @@ class APIToken < ApplicationRecord self.allowed_procedure_ids = allowed_procedures.map(&:id) end end + + class BearerToken < Data.define(:api_token_id, :plain_token) + def to_string + Base64.urlsafe_encode64([api_token_id, plain_token].join(';')) + end + + def self.from_string(bearer_token) + return if bearer_token.nil? + + api_token_id, plain_token = Base64.urlsafe_decode64(bearer_token).split(';') + BearerToken.new(api_token_id, plain_token) + rescue ArgumentError + end + end end From 40ed59a231a98f0257ea39065a55a7272da3c7c7 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 3 Aug 2023 15:27:33 +0200 Subject: [PATCH 04/18] rename find_and_verify => authenticate --- app/controllers/api/v2/base_controller.rb | 2 +- app/controllers/api_controller.rb | 2 +- app/models/api_token.rb | 2 +- spec/models/api_token_spec.rb | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/v2/base_controller.rb b/app/controllers/api/v2/base_controller.rb index 342de720e..0c43862ed 100644 --- a/app/controllers/api/v2/base_controller.rb +++ b/app/controllers/api/v2/base_controller.rb @@ -33,7 +33,7 @@ class API::V2::BaseController < ApplicationController def api_token if @api_token.nil? @api_token = APIToken - .find_and_verify(authorization_bearer_token) + .authenticate(authorization_bearer_token) &.tap { _1.touch(:last_v2_authenticated_at) } || false end @api_token diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 55eedd8a1..8090f8e59 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -4,7 +4,7 @@ class APIController < ApplicationController protected def find_administrateur_for_token(procedure) - api_token = APIToken.find_and_verify(authorization_bearer_token) + api_token = APIToken.authenticate(authorization_bearer_token) if api_token.present? && api_token.context.fetch(:procedure_ids).include?(procedure.id) api_token.touch(:last_v1_authenticated_at) api_token.administrateur diff --git a/app/models/api_token.rb b/app/models/api_token.rb index c33af4840..f3fd499e2 100644 --- a/app/models/api_token.rb +++ b/app/models/api_token.rb @@ -55,7 +55,7 @@ class APIToken < ApplicationRecord [api_token, bearer.to_string] end - def find_and_verify(bearer_string) + def authenticate(bearer_string) bearer = BearerToken.from_string(bearer_string) return if bearer.nil? diff --git a/spec/models/api_token_spec.rb b/spec/models/api_token_spec.rb index 714ebc99e..ebcceb357 100644 --- a/spec/models/api_token_spec.rb +++ b/spec/models/api_token_spec.rb @@ -107,13 +107,13 @@ describe APIToken, type: :model do end end - describe '#find_and_verify' do + describe '#authenticate' do let(:api_token_and_packed_token) { APIToken.generate(administrateur) } let(:api_token) { api_token_and_packed_token.first } let(:packed_token) { api_token_and_packed_token.second } let(:bearer_token) { packed_token } - subject { APIToken.find_and_verify(bearer_token) } + subject { APIToken.authenticate(bearer_token) } context 'with the legit packed token' do it { is_expected.to eq(api_token) } From f434c6a6ad5f0dbb70e4546773fd33319c31728d Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 3 Aug 2023 15:38:51 +0200 Subject: [PATCH 05/18] refactor: try base controller --- app/controllers/api/v2/base_controller.rb | 38 ++++++----------------- 1 file changed, 10 insertions(+), 28 deletions(-) diff --git a/app/controllers/api/v2/base_controller.rb b/app/controllers/api/v2/base_controller.rb index 0c43862ed..8a8ff9509 100644 --- a/app/controllers/api/v2/base_controller.rb +++ b/app/controllers/api/v2/base_controller.rb @@ -1,15 +1,15 @@ class API::V2::BaseController < ApplicationController # Disable forgery protection for API controllers when the request is authenticated # with a bearer token. Otherwise the session will be nullified and we'll lose curent_user - protect_from_forgery with: :null_session, unless: :token? + skip_forgery_protection if: -> { request.headers.key?('HTTP_AUTHORIZATION') } skip_before_action :setup_tracking - prepend_before_action :authenticate_administrateur_from_token + before_action :authenticate_from_token private def context - if api_token.present? - api_token.context + if @api_token.present? + @api_token.context # web interface (/graphql) give current_administrateur elsif current_administrateur.present? { @@ -20,32 +20,14 @@ class API::V2::BaseController < ApplicationController end end - def token? - authorization_bearer_token.present? - end + private - def authenticate_administrateur_from_token - if api_token.present? - @current_user = api_token.administrateur.user - end - end + def authenticate_from_token + @api_token = authenticate_with_http_token { |t, _o| APIToken.authenticate(t) } - def api_token - if @api_token.nil? - @api_token = APIToken - .authenticate(authorization_bearer_token) - &.tap { _1.touch(:last_v2_authenticated_at) } || false - end - @api_token - end - - def authorization_bearer_token - @authorization_bearer_token ||= begin - received_token = nil - authenticate_with_http_token do |token, _options| - received_token = token - end - received_token + if @api_token.present? + @api_token.touch(:last_v2_authenticated_at) + @current_user = @api_token.administrateur.user end end end From 24fd12ed700e7878e5600e6e90f81afe3ec6d61c Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 3 Aug 2023 16:33:30 +0200 Subject: [PATCH 06/18] explicit about different contexts --- app/controllers/api/v2/base_controller.rb | 24 ++++++++++++++++++----- app/graphql/api/v2/context.rb | 2 +- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/v2/base_controller.rb b/app/controllers/api/v2/base_controller.rb index 8a8ff9509..f8a640482 100644 --- a/app/controllers/api/v2/base_controller.rb +++ b/app/controllers/api/v2/base_controller.rb @@ -12,16 +12,30 @@ class API::V2::BaseController < ApplicationController @api_token.context # web interface (/graphql) give current_administrateur elsif current_administrateur.present? - { - administrateur_id: current_administrateur.id, - procedure_ids: current_administrateur.procedure_ids, - write_access: true - } + graphql_web_interface_context + else + unauthenticated_request_context end end private + def graphql_web_interface_context + { + administrateur_id: current_administrateur.id, + procedure_ids: current_administrateur.procedure_ids, + write_access: true + } + end + + def unauthenticated_request_context + { + administrateur_id: nil, + procedure_ids: [], + write_access: false + } + end + def authenticate_from_token @api_token = authenticate_with_http_token { |t, _o| APIToken.authenticate(t) } diff --git a/app/graphql/api/v2/context.rb b/app/graphql/api/v2/context.rb index 824c9cd30..e8496535a 100644 --- a/app/graphql/api/v2/context.rb +++ b/app/graphql/api/v2/context.rb @@ -75,7 +75,7 @@ class API::V2::Context < GraphQL::Query::Context def compute_demarche_authorization(demarche) # procedure_ids and token are passed from graphql controller - (self[:procedure_ids] || []).include?(demarche.id) + self[:procedure_ids].include?(demarche.id) end # This is a query AST visitor that we use to check From 05a8fd8ee18c63519fd484c3e0721febefb63c1b Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Thu, 3 Aug 2023 17:53:34 +0200 Subject: [PATCH 07/18] refactor of api_token --- app/models/api_token.rb | 61 +++++++++++-------- spec/models/api_token_spec.rb | 107 ++++++++++++++++++++-------------- 2 files changed, 100 insertions(+), 68 deletions(-) diff --git a/app/models/api_token.rb b/app/models/api_token.rb index f3fd499e2..bac2cb53a 100644 --- a/app/models/api_token.rb +++ b/app/models/api_token.rb @@ -2,42 +2,53 @@ class APIToken < ApplicationRecord include ActiveRecord::SecureToken belongs_to :administrateur, inverse_of: :api_tokens - has_many :procedures, through: :administrateur - before_save :check_allowed_procedure_ids_ownership + before_save :sanitize_targeted_procedure_ids def context - context = { administrateur_id: administrateur_id, write_access: write_access? } + { + administrateur_id:, + procedure_ids:, + write_access: + } + end + def procedure_ids if full_access? - context.merge procedure_ids: + administrateur.procedures.ids else - context.merge procedure_ids: procedure_ids & allowed_procedure_ids + sanitized_targeted_procedure_ids end end + def procedures + Procedure.where(id: procedure_ids) + end + def full_access? - allowed_procedure_ids.nil? + targeted_procedure_ids.nil? end - def procedures_to_allow - procedures.select(:id, :libelle, :path).where.not(id: allowed_procedure_ids || []).order(:libelle) + def targetable_procedures + administrateur + .procedures + .where.not(id: targeted_procedure_ids) + .select(:id, :libelle, :path) + .order(:libelle) end - def allowed_procedures - if allowed_procedure_ids.present? - procedures.select(:id, :libelle, :path).where(id: allowed_procedure_ids).order(:libelle) - else - [] - end + def untarget_procedure(procedure_id) + new_target_ids = targeted_procedure_ids - [procedure_id] + + update!(allowed_procedure_ids: new_target_ids) end - def disallow_procedure(procedure_id) - allowed_procedure_ids = allowed_procedures.map(&:id) - [procedure_id] - if allowed_procedure_ids.empty? - allowed_procedure_ids = nil - end - update!(allowed_procedure_ids:) + def sanitized_targeted_procedure_ids + administrateur.procedures.ids.intersection(targeted_procedure_ids || []) + end + + def become_full_access! + update_column(:allowed_procedure_ids, nil) end # Prefix is made of the first 6 characters of the uuid base64 encoded @@ -70,12 +81,16 @@ class APIToken < ApplicationRecord private - def check_allowed_procedure_ids_ownership - if allowed_procedure_ids.present? - self.allowed_procedure_ids = allowed_procedures.map(&:id) + def sanitize_targeted_procedure_ids + if targeted_procedure_ids.present? + write_attribute(:allowed_procedure_ids, sanitized_targeted_procedure_ids) end end + def targeted_procedure_ids + read_attribute(:allowed_procedure_ids) + end + class BearerToken < Data.define(:api_token_id, :plain_token) def to_string Base64.urlsafe_encode64([api_token_id, plain_token].join(';')) diff --git a/spec/models/api_token_spec.rb b/spec/models/api_token_spec.rb index ebcceb357..aaad7202d 100644 --- a/spec/models/api_token_spec.rb +++ b/spec/models/api_token_spec.rb @@ -6,105 +6,123 @@ describe APIToken, type: :model do let(:api_token) { api_token_and_packed_token.first } let(:packed_token) { api_token_and_packed_token.second } - it do + before { api_token_and_packed_token } + + it 'with a full access token' do expect(api_token.administrateur).to eq(administrateur) expect(api_token.prefix).to eq(packed_token.slice(0, 5)) expect(api_token.version).to eq(3) expect(api_token.write_access?).to eq(true) expect(api_token.procedure_ids).to eq([]) - expect(api_token.allowed_procedure_ids).to eq(nil) expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [], write_access: true) expect(api_token.full_access?).to be_truthy end - context 'with read_only' do + context 'updated read_only' do before { api_token.update(write_access: false) } + it do - expect(api_token.full_access?).to be_truthy expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [], write_access: false) end end - context 'with procedure' do + context 'with a new added procedure' do let(:procedure) { create(:procedure, administrateurs: [administrateur]) } - before { procedure } + + before do + procedure + api_token.reload + end it do + expect(api_token.full_access?).to be_truthy expect(api_token.procedure_ids).to eq([procedure.id]) - expect(api_token.procedures_to_allow).to eq([procedure]) - expect(api_token.allowed_procedure_ids).to eq(nil) + expect(api_token.procedures).to eq([procedure]) expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [procedure.id], write_access: true) end - context 'update with procedure_id' do - let(:procedure) { create(:procedure, administrateurs: [administrateur]) } + context 'and another procedure, but access only to the first one' do let(:other_procedure) { create(:procedure, administrateurs: [administrateur]) } - before { api_token.update(allowed_procedure_ids: [procedure.id]); other_procedure } + + before do + other_procedure + api_token.update(allowed_procedure_ids: [procedure.id]) + api_token.reload + end it do - expect(api_token.procedure_ids).to match_array([procedure.id, other_procedure.id]) - expect(api_token.procedures_to_allow).to eq([other_procedure]) - expect(api_token.allowed_procedure_ids).to eq([procedure.id]) + expect(api_token.full_access?).to be_falsey + expect(api_token.procedure_ids).to match_array([procedure.id]) + expect(api_token.targetable_procedures).to eq([other_procedure]) expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [procedure.id], write_access: true) end + + context 'and then gain full access' do + before do + api_token.become_full_access! + api_token.reload + end + + it do + expect(api_token.full_access?).to be(true) + expect(api_token.procedure_ids).to match_array([procedure.id, other_procedure.id]) + expect(api_token.targetable_procedures).to eq([procedure, other_procedure]) + end + end end - context 'update with wrong procedure_id' do - let(:other_administrateur) { create(:administrateur) } - let(:procedure) { create(:procedure, administrateurs: [other_administrateur]) } - before { api_token.update(allowed_procedure_ids: [procedure.id]) } + context 'but acces to a wrong procedure_id' do + let(:forbidden_procedure) { create(:procedure) } + + before do + api_token.update(allowed_procedure_ids: [forbidden_procedure.id]) + api_token.reload + end it do expect(api_token.full_access?).to be_falsey expect(api_token.procedure_ids).to eq([]) - expect(api_token.procedures_to_allow).to eq([]) - expect(api_token.allowed_procedure_ids).to eq([]) + expect(api_token.targetable_procedures).to eq([procedure]) expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [], write_access: true) end end context 'update with destroyed procedure_id' do let(:procedure) { create(:procedure, administrateurs: [administrateur]) } - before { api_token.update(allowed_procedure_ids: [procedure.id]); procedure.destroy } + + before do + api_token.update(allowed_procedure_ids: [procedure.id]) + procedure.destroy + api_token.reload + end it do expect(api_token.full_access?).to be_falsey expect(api_token.procedure_ids).to eq([]) - expect(api_token.procedures_to_allow).to eq([]) - expect(api_token.allowed_procedure_ids).to eq([procedure.id]) + expect(api_token.targetable_procedures).to eq([]) expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [], write_access: true) end end context 'update with detached procedure_id' do - let(:other_procedure) { create(:procedure, administrateurs: [administrateur]) } let(:procedure) { create(:procedure, administrateurs: [administrateur]) } - before { api_token.update(allowed_procedure_ids: [procedure.id]); other_procedure; administrateur.procedures.delete(procedure) } + let(:other_procedure) { create(:procedure, administrateurs: [administrateur]) } + + before do + api_token.update(allowed_procedure_ids: [procedure.id]) + other_procedure + administrateur.procedures.delete(procedure) + api_token.reload + end it do expect(api_token.full_access?).to be_falsey - expect(api_token.procedure_ids).to eq([other_procedure.id]) - expect(api_token.allowed_procedure_ids).to eq([procedure.id]) + expect(api_token.procedure_ids).to eq([]) + expect(api_token.targetable_procedures).to eq([other_procedure]) expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [], write_access: true) end end end - - context 'with procedure and allowed_procedure_ids' do - let(:procedure) { create(:procedure, administrateurs: [administrateur]) } - let(:other_procedure) { create(:procedure, administrateurs: [administrateur]) } - - before do - api_token.update(allowed_procedure_ids: [procedure.id]) - other_procedure - end - - it do - expect(api_token.procedure_ids).to match_array([procedure.id, other_procedure.id]) - expect(api_token.allowed_procedure_ids).to eq([procedure.id]) - expect(api_token.context).to eq(administrateur_id: administrateur.id, procedure_ids: [procedure.id], write_access: true) - end - end end describe '#authenticate' do @@ -133,8 +151,7 @@ describe APIToken, type: :model do context "with a bearer token with the wrong plain_token" do let(:bearer_token) do - clear_packed = [api_token.id, 'wrong'].join(';') - Base64.urlsafe_encode64(clear_packed) + APIToken::BearerToken.new(api_token.id, 'wrong').to_string end it { is_expected.to be_nil } From 01efae960b48af934d69aaeea9038f12d4d91adf Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 12 Sep 2023 17:06:55 +0200 Subject: [PATCH 08/18] fix component --- app/components/profile/api_token_component.rb | 4 ++-- .../api_token_component/api_token_component.html.haml | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/components/profile/api_token_component.rb b/app/components/profile/api_token_component.rb index 7b91244eb..9ff127222 100644 --- a/app/components/profile/api_token_component.rb +++ b/app/components/profile/api_token_component.rb @@ -7,10 +7,10 @@ class Profile::APITokenComponent < ApplicationComponent private def procedures_to_allow_options - @api_token.procedures_to_allow.map { ["#{_1.id} – #{_1.libelle}", _1.id] } + @api_token.targetable_procedures.map { ["#{_1.id} – #{_1.libelle}", _1.id] } end def procedures_to_allow_select_options - { selected: @api_token.procedures_to_allow.first&.id } + { selected: @api_token.targetable_procedures.first&.id } end end diff --git a/app/components/profile/api_token_component/api_token_component.html.haml b/app/components/profile/api_token_component/api_token_component.html.haml index c4d064fc0..f27e2ba8b 100644 --- a/app/components/profile/api_token_component/api_token_component.html.haml +++ b/app/components/profile/api_token_component/api_token_component.html.haml @@ -40,13 +40,13 @@ = button_to t('.delete'), @api_token, method: :patch, params: { api_token: { disallow_procedure_id: procedure.id } }, class: "fr-btn fr-btn--secondary" .fr-card__end - = form_for @api_token, namespace: dom_id(@api_token, :allowed_procedures), html: { class: 'form form-ds-fr-white mb-3', data: { turbo: true } } do |f| - = f.label :allowed_procedure_ids do + = form_for @api_token, namespace: dom_id(@api_token, :allowed_procedures), html: { class: 'mb-3', data: { turbo: true } } do |f| + = f.label :allowed_procedure_ids, class: 'fr-label' do = t('.action_choice') - @api_token.allowed_procedures.each do |procedure| = f.hidden_field :allowed_procedure_ids, value: procedure.id, multiple: true, id: dom_id(procedure, :allowed_procedure) .flex.justify-between.align-center{ 'data-turbo-force': :server } - = f.select :allowed_procedure_ids, procedures_to_allow_options, {prompt: t('.prompt_choose_procedure')}, { class: 'no-margin width-66 small', name: "api_token[allowed_procedure_ids][]" } + = f.select :allowed_procedure_ids, procedures_to_allow_options, {prompt: t('.prompt_choose_procedure')}, { class: 'fr-select ', name: "api_token[allowed_procedure_ids][]" } = f.button type: :submit, class: "fr-btn fr-btn--secondary" do = t('.add') From 2a109d3aa4e81688b06ba4073afc375b62ddfb40 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 12 Sep 2023 17:20:00 +0200 Subject: [PATCH 09/18] api_tokens_controller: use before_action set_api_token --- app/controllers/api_tokens_controller.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/controllers/api_tokens_controller.rb b/app/controllers/api_tokens_controller.rb index 7cadd42b5..92f503cbd 100644 --- a/app/controllers/api_tokens_controller.rb +++ b/app/controllers/api_tokens_controller.rb @@ -1,5 +1,6 @@ class APITokensController < ApplicationController before_action :authenticate_administrateur! + before_action :set_api_token, only: [:update, :destroy] def create @api_token, @packed_token = APIToken.generate(current_administrateur) @@ -11,8 +12,6 @@ class APITokensController < ApplicationController end def update - @api_token = current_administrateur.api_tokens.find(params[:id]) - disallow_procedure_id = api_token_params.fetch(:disallow_procedure_id, nil) if disallow_procedure_id.present? @api_token.disallow_procedure(disallow_procedure_id.to_i) @@ -27,7 +26,6 @@ class APITokensController < ApplicationController end def destroy - @api_token = current_administrateur.api_tokens.find(params[:id]) @api_token.destroy respond_to do |format| @@ -38,6 +36,10 @@ class APITokensController < ApplicationController private + def set_api_token + @api_token = current_administrateur.api_tokens.find(params[:id]) + end + def api_token_params params.require(:api_token).permit(:name, :write_access, :disallow_procedure_id, allowed_procedure_ids: []) end From 9b440b6c44918f3ca6e050f3bbfaf7a1147bcc41 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 13 Sep 2023 10:30:47 +0200 Subject: [PATCH 10/18] api_tokens_controller: extract disallow_procedure --- app/controllers/api_tokens_controller.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/api_tokens_controller.rb b/app/controllers/api_tokens_controller.rb index 92f503cbd..119cde0df 100644 --- a/app/controllers/api_tokens_controller.rb +++ b/app/controllers/api_tokens_controller.rb @@ -12,9 +12,8 @@ class APITokensController < ApplicationController end def update - disallow_procedure_id = api_token_params.fetch(:disallow_procedure_id, nil) if disallow_procedure_id.present? - @api_token.disallow_procedure(disallow_procedure_id.to_i) + @api_token.untarget_procedure(disallow_procedure_id.to_i) else @api_token.update!(api_token_params) end @@ -40,6 +39,10 @@ class APITokensController < ApplicationController @api_token = current_administrateur.api_tokens.find(params[:id]) end + def disallow_procedure_id + api_token_params[:disallow_procedure_id] + end + def api_token_params params.require(:api_token).permit(:name, :write_access, :disallow_procedure_id, allowed_procedure_ids: []) end From 9047c2a7defd144db420f4976da7d33d2c3c8d02 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 13 Sep 2023 10:29:49 +0200 Subject: [PATCH 11/18] api_tokens_controller: extract become full_access --- .../api_token_component/api_token_component.html.haml | 4 ++-- app/controllers/api_tokens_controller.rb | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/components/profile/api_token_component/api_token_component.html.haml b/app/components/profile/api_token_component/api_token_component.html.haml index f27e2ba8b..364ddfa8e 100644 --- a/app/components/profile/api_token_component/api_token_component.html.haml +++ b/app/components/profile/api_token_component/api_token_component.html.haml @@ -29,8 +29,8 @@ %p.fr-text--lg = t('.allowed_procedures_html', count: @api_token.allowed_procedures.size) - - if @api_token.allowed_procedures.empty? - = button_to t('.action_all'), @api_token, method: :patch, params: { api_token: { disallow_procedure_id: '0' } }, class: "fr-btn fr-btn--secondary" + - if @api_token.procedures.empty? + = button_to t('.action_all'), @api_token, method: :patch, params: { api_token: { become_full_access: '1' } }, class: "fr-btn fr-btn--secondary" - else %ul - @api_token.allowed_procedures.each do |procedure| diff --git a/app/controllers/api_tokens_controller.rb b/app/controllers/api_tokens_controller.rb index 119cde0df..a4c0348d2 100644 --- a/app/controllers/api_tokens_controller.rb +++ b/app/controllers/api_tokens_controller.rb @@ -12,7 +12,9 @@ class APITokensController < ApplicationController end def update - if disallow_procedure_id.present? + if become_full_access? + @api_token.become_full_access! + elsif disallow_procedure_id.present? @api_token.untarget_procedure(disallow_procedure_id.to_i) else @api_token.update!(api_token_params) @@ -39,11 +41,15 @@ class APITokensController < ApplicationController @api_token = current_administrateur.api_tokens.find(params[:id]) end + def become_full_access? + api_token_params[:become_full_access].present? + end + def disallow_procedure_id api_token_params[:disallow_procedure_id] end def api_token_params - params.require(:api_token).permit(:name, :write_access, :disallow_procedure_id, allowed_procedure_ids: []) + params.require(:api_token).permit(:name, :write_access, :become_full_access, :disallow_procedure_id, allowed_procedure_ids: []) end end From 954c5334efd922f21e98eb4b36494d7449413856 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 13 Sep 2023 09:56:25 +0200 Subject: [PATCH 12/18] use new procedure in view --- .../api_token_component/api_token_component.html.haml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/components/profile/api_token_component/api_token_component.html.haml b/app/components/profile/api_token_component/api_token_component.html.haml index 364ddfa8e..d187510a0 100644 --- a/app/components/profile/api_token_component/api_token_component.html.haml +++ b/app/components/profile/api_token_component/api_token_component.html.haml @@ -27,13 +27,13 @@ = t('.allowed_full_access_html') - else %p.fr-text--lg - = t('.allowed_procedures_html', count: @api_token.allowed_procedures.size) + = t('.allowed_procedures_html', count: @api_token.procedures.size) - if @api_token.procedures.empty? = button_to t('.action_all'), @api_token, method: :patch, params: { api_token: { become_full_access: '1' } }, class: "fr-btn fr-btn--secondary" - else %ul - - @api_token.allowed_procedures.each do |procedure| + - @api_token.procedures.each do |procedure| %li.flex.justify-between.align-center .truncate-80 = "#{procedure.id} – #{procedure.libelle}" @@ -43,8 +43,9 @@ = form_for @api_token, namespace: dom_id(@api_token, :allowed_procedures), html: { class: 'mb-3', data: { turbo: true } } do |f| = f.label :allowed_procedure_ids, class: 'fr-label' do = t('.action_choice') - - @api_token.allowed_procedures.each do |procedure| - = f.hidden_field :allowed_procedure_ids, value: procedure.id, multiple: true, id: dom_id(procedure, :allowed_procedure) + - if !@api_token.full_access? + - @api_token.procedures.each do |procedure| + = f.hidden_field :allowed_procedure_ids, value: procedure.id, multiple: true, id: dom_id(procedure, :allowed_procedure) .flex.justify-between.align-center{ 'data-turbo-force': :server } = f.select :allowed_procedure_ids, procedures_to_allow_options, {prompt: t('.prompt_choose_procedure')}, { class: 'fr-select ', name: "api_token[allowed_procedure_ids][]" } = f.button type: :submit, class: "fr-btn fr-btn--secondary" do From 2664c3671f3e781400fb4701cd22f5fa6c44c3e3 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 13 Sep 2023 12:05:40 +0200 Subject: [PATCH 13/18] api_tokens_controller: only use turbo --- app/controllers/api_tokens_controller.rb | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/app/controllers/api_tokens_controller.rb b/app/controllers/api_tokens_controller.rb index a4c0348d2..bf51f65d6 100644 --- a/app/controllers/api_tokens_controller.rb +++ b/app/controllers/api_tokens_controller.rb @@ -5,10 +5,7 @@ class APITokensController < ApplicationController def create @api_token, @packed_token = APIToken.generate(current_administrateur) - respond_to do |format| - format.turbo_stream { render :index } - format.html { redirect_back(fallback_location: profil_path) } - end + render :index end def update @@ -20,19 +17,13 @@ class APITokensController < ApplicationController @api_token.update!(api_token_params) end - respond_to do |format| - format.turbo_stream { render :index } - format.html { redirect_back(fallback_location: profil_path) } - end + render :index end def destroy @api_token.destroy - respond_to do |format| - format.turbo_stream { render :index } - format.html { redirect_back(fallback_location: profil_path) } - end + render :index end private From 118242dbd2a3a9c49cf7193432753c5f7335b57a Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 20 Sep 2023 09:28:03 +0200 Subject: [PATCH 14/18] refactor api_controller --- app/controllers/api_controller.rb | 38 +++++++++---------- .../api/v1/procedures_controller_spec.rb | 7 ++++ spec/controllers/api_controller_spec.rb | 23 ++++++----- 3 files changed, 38 insertions(+), 30 deletions(-) diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 8090f8e59..928ad5106 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -1,15 +1,6 @@ class APIController < ApplicationController before_action :default_format_json - - protected - - def find_administrateur_for_token(procedure) - api_token = APIToken.authenticate(authorization_bearer_token) - if api_token.present? && api_token.context.fetch(:procedure_ids).include?(procedure.id) - api_token.touch(:last_v1_authenticated_at) - api_token.administrateur - end - end + before_action :authenticate_from_token private @@ -17,19 +8,24 @@ class APIController < ApplicationController request.format = "json" if !request.params[:format] end - def authorization_bearer_token - params_token.presence || header_token - end - - def header_token - received_token = nil - authenticate_with_http_token do |token, _options| - received_token = token + def check_api_token + if @api_token.nil? + render json: {}, status: :unauthorized end - received_token end - def params_token - params[:token] + def authenticate_from_token + @api_token = authenticate_with_http_token { |t, _o| APIToken.authenticate(t) } + + # legacy way of sending the token by url + # not available in api v2 + if @api_token.nil? + @api_token = APIToken.authenticate(params[:token]) + end + + if @api_token.present? + @api_token.touch(:last_v1_authenticated_at) + @current_user = @api_token.administrateur.user + end end end diff --git a/spec/controllers/api/v1/procedures_controller_spec.rb b/spec/controllers/api/v1/procedures_controller_spec.rb index 88d8676ae..091f5c314 100644 --- a/spec/controllers/api/v1/procedures_controller_spec.rb +++ b/spec/controllers/api/v1/procedures_controller_spec.rb @@ -19,6 +19,13 @@ describe API::V1::ProceduresController, type: :controller do it { is_expected.to have_http_status(401) } end + context 'when procedure exist but bad token' do + let(:token) { 'bad' } + let(:procedure_id) { create(:procedure, administrateur: admin).id } + + it { is_expected.to have_http_status(401) } + end + context 'when procedure exist' do let(:procedure_id) { create(:procedure, administrateur: admin).id } diff --git a/spec/controllers/api_controller_spec.rb b/spec/controllers/api_controller_spec.rb index fb839d925..8f63a138b 100644 --- a/spec/controllers/api_controller_spec.rb +++ b/spec/controllers/api_controller_spec.rb @@ -1,36 +1,41 @@ describe APIController, type: :controller do - describe 'valid_token_for_procedure?' do + describe 'authenticate_from_token' do let(:procedure) { create(:procedure) } let(:admin) { procedure.administrateurs.first } - subject { !!controller.send(:find_administrateur_for_token, procedure) } + subject do + controller.send(:authenticate_from_token) + assigns(:api_token) + end context 'when the admin has not any token' do context 'and the token is not given' do - it { is_expected.to be false } + it { is_expected.to be nil } end end context 'when the admin has a token' do - let!(:token) { APIToken.generate(admin)[1] } + let(:token_bearer_couple) { APIToken.generate(admin) } + let(:token) { token_bearer_couple[0] } + let(:bearer) { token_bearer_couple[1] } context 'and the token is given by params' do - before { controller.params[:token] = token } + before { controller.params[:token] = bearer } - it { is_expected.to be true } + it { is_expected.to eq(token) } end context 'and the token is given by header' do before do - valid_headers = { 'Authorization' => "Bearer token=#{token}" } + valid_headers = { 'Authorization' => "Bearer token=#{bearer}" } request.headers.merge!(valid_headers) end - it { is_expected.to be true } + it { is_expected.to eq(token) } end context 'and the token is not given' do - it { is_expected.to be false } + it { is_expected.to be nil } end end end From c248f96f31dee76045f8b633fbca1b1d40292d84 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 20 Sep 2023 09:28:19 +0200 Subject: [PATCH 15/18] fixup base controller --- app/controllers/api/v2/base_controller.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/controllers/api/v2/base_controller.rb b/app/controllers/api/v2/base_controller.rb index f8a640482..29ed95a57 100644 --- a/app/controllers/api/v2/base_controller.rb +++ b/app/controllers/api/v2/base_controller.rb @@ -1,6 +1,4 @@ class API::V2::BaseController < ApplicationController - # Disable forgery protection for API controllers when the request is authenticated - # with a bearer token. Otherwise the session will be nullified and we'll lose curent_user skip_forgery_protection if: -> { request.headers.key?('HTTP_AUTHORIZATION') } skip_before_action :setup_tracking before_action :authenticate_from_token From 40a15b9be45bf33bd20f10c2868bb29348b6e207 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 20 Sep 2023 09:30:14 +0200 Subject: [PATCH 16/18] refactor dossier_controller --- app/controllers/api/v1/dossiers_controller.rb | 36 +++++++------------ .../api/v1/dossiers_controller_spec.rb | 4 +-- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/app/controllers/api/v1/dossiers_controller.rb b/app/controllers/api/v1/dossiers_controller.rb index 81133222f..c42739d1c 100644 --- a/app/controllers/api/v1/dossiers_controller.rb +++ b/app/controllers/api/v1/dossiers_controller.rb @@ -1,5 +1,6 @@ class API::V1::DossiersController < APIController - before_action :fetch_procedure_and_check_token + before_action :check_api_token + before_action :fetch_dossiers DEFAULT_PAGE_SIZE = 100 MAX_PAGE_SIZE = 1000 @@ -8,19 +9,17 @@ class API::V1::DossiersController < APIController def index dossiers = @dossiers.page(params[:page]).per(per_page) - render json: { dossiers: dossiers.map { |dossier| DossiersSerializer.new(dossier) }, pagination: pagination(dossiers) }, status: 200 + render json: { dossiers: dossiers.map { |dossier| DossiersSerializer.new(dossier) }, pagination: pagination(dossiers) } rescue ActiveRecord::RecordNotFound - render json: {}, status: 404 + render json: {}, status: :not_found end def show dossier = @dossiers.for_api.find(params[:id]) - respond_to do |format| - format.json { render json: { dossier: DossierSerializer.new(dossier).as_json }, status: 200 } - end + render json: { dossier: DossierSerializer.new(dossier).as_json } rescue ActiveRecord::RecordNotFound - render json: {}, status: 404 + render json: {}, status: :not_found end private @@ -42,24 +41,15 @@ class API::V1::DossiersController < APIController end end - def fetch_procedure_and_check_token - @procedure = Procedure.for_api.find(params[:procedure_id]) + def fetch_dossiers + procedure = @api_token.procedures.find(params[:procedure_id]) - administrateur = find_administrateur_for_token(@procedure) - if administrateur.nil? - render json: {}, status: :unauthorized - else - # allow BaseController append_info_to_payload - # to log info on current_user - @current_user = administrateur.user + order = ORDER_DIRECTIONS.fetch(params[:order], :asc) + @dossiers = procedure + .dossiers + .visible_by_administration + .order_by_created_at(order) - order = ORDER_DIRECTIONS.fetch(params[:order], :asc) - @dossiers = @procedure - .dossiers - .visible_by_administration - .order_by_created_at(order) - - end rescue ActiveRecord::RecordNotFound render json: {}, status: :not_found end diff --git a/spec/controllers/api/v1/dossiers_controller_spec.rb b/spec/controllers/api/v1/dossiers_controller_spec.rb index 23ba99781..f4a972fb6 100644 --- a/spec/controllers/api/v1/dossiers_controller_spec.rb +++ b/spec/controllers/api/v1/dossiers_controller_spec.rb @@ -34,7 +34,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('401') } + it { expect(subject.code).to eq('404') } end context 'when procedure is found and belongs to admin' do @@ -136,7 +136,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('401') } + it { expect(subject.code).to eq('404') } end context 'when procedure is found and belongs to current admin' do From 4a17dec87b73b094b911ba2c80094c91b3723793 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Wed, 20 Sep 2023 09:31:14 +0200 Subject: [PATCH 17/18] refactor procedure_controller --- app/controllers/api/v1/procedures_controller.rb | 15 ++++----------- .../api/v1/procedures_controller_spec.rb | 2 +- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/app/controllers/api/v1/procedures_controller.rb b/app/controllers/api/v1/procedures_controller.rb index 50c208c39..23f635f8e 100644 --- a/app/controllers/api/v1/procedures_controller.rb +++ b/app/controllers/api/v1/procedures_controller.rb @@ -1,5 +1,6 @@ class API::V1::ProceduresController < APIController - before_action :fetch_procedure_and_check_token + before_action :check_api_token + before_action :fetch_procedure def show render json: { procedure: ProcedureSerializer.new(@procedure).as_json } @@ -7,17 +8,9 @@ class API::V1::ProceduresController < APIController private - def fetch_procedure_and_check_token - @procedure = Procedure.for_api.find(params[:id]) + def fetch_procedure + @procedure = @api_token.procedures.for_api.find(params[:id]) - administrateur = find_administrateur_for_token(@procedure) - if administrateur.nil? - render json: {}, status: :unauthorized - else - # allow BaseController append_info_to_payload - # to log info on current_user - @current_user = administrateur.user - end rescue ActiveRecord::RecordNotFound render json: {}, status: :not_found end diff --git a/spec/controllers/api/v1/procedures_controller_spec.rb b/spec/controllers/api/v1/procedures_controller_spec.rb index 091f5c314..ec96d4619 100644 --- a/spec/controllers/api/v1/procedures_controller_spec.rb +++ b/spec/controllers/api/v1/procedures_controller_spec.rb @@ -16,7 +16,7 @@ describe API::V1::ProceduresController, type: :controller do context 'when procedure belongs to administrateur without token' do let(:procedure_id) { create(:procedure).id } - it { is_expected.to have_http_status(401) } + it { is_expected.to have_http_status(404) } end context 'when procedure exist but bad token' do From 4be682145ca589bea51d785f7c03a0732eb7f082 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 19 Sep 2023 23:05:52 +0200 Subject: [PATCH 18/18] spec: speed up by 3 --- .../api/v1/dossiers_controller_spec.rb | 137 ++++++++++-------- .../api/v1/procedures_controller_spec.rb | 48 +++--- 2 files changed, 103 insertions(+), 82 deletions(-) diff --git a/spec/controllers/api/v1/dossiers_controller_spec.rb b/spec/controllers/api/v1/dossiers_controller_spec.rb index f4a972fb6..eb11f4b49 100644 --- a/spec/controllers/api/v1/dossiers_controller_spec.rb +++ b/spec/controllers/api/v1/dossiers_controller_spec.rb @@ -43,28 +43,30 @@ describe API::V1::DossiersController do let!(:dossier) { Timecop.freeze(date_creation) { create(:dossier, :with_entreprise, :en_construction, procedure: procedure) } } let(:body) { JSON.parse(retour.body, symbolize_names: true) } - it 'return REST code 200', :show_in_doc do + it do expect(retour.code).to eq('200') + expect(body).to have_key :pagination + expect(body).to have_key :dossiers end - it { expect(body).to have_key :pagination } + context 'but the token is invalid' do + let(:token) { 'bad' } - it { expect(body).to have_key :dossiers } + it { expect(subject.code).to eq('401') } + end describe 'pagination' do subject { body[:pagination] } - it { is_expected.to have_key(:page) } - it { expect(subject[:page]).to eq(1) } - it { is_expected.to have_key(:resultats_par_page) } - it { expect(subject[:resultats_par_page]).to eq(described_class.const_get(:DEFAULT_PAGE_SIZE)) } - it { is_expected.to have_key(:nombre_de_page) } - it { expect(subject[:nombre_de_page]).to eq(1) } + it do + expect(subject[:page]).to eq(1) + expect(subject[:resultats_par_page]).to eq(described_class.const_get(:DEFAULT_PAGE_SIZE)) + expect(subject[:nombre_de_page]).to eq(1) + end end describe 'with custom resultats_par_page' do 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) } end @@ -73,11 +75,14 @@ describe API::V1::DossiersController do it { expect(subject).to be_an(Array) } describe 'dossier' do subject { super().first } - it { expect(subject[:id]).to eq(dossier.id) } - it { expect(subject[:updated_at]).to eq("2008-09-01T08:05:00.000Z") } - it { expect(subject[:initiated_at]).to eq("2008-09-01T08:06:00.000Z") } - it { expect(subject[:state]).to eq("initiated") } - it { expect(subject.keys.size).to eq(4) } + + it do + expect(subject[:id]).to eq(dossier.id) + expect(subject[:updated_at]).to eq("2008-09-01T08:05:00.000Z") + expect(subject[:initiated_at]).to eq("2008-09-01T08:06:00.000Z") + expect(subject[:state]).to eq("initiated") + expect(subject.keys.size).to eq(4) + end end describe 'order' do @@ -181,13 +186,14 @@ describe API::V1::DossiersController do expect(retour.code).to eq('200') end - it { expect(subject[:id]).to eq(dossier.id) } - it { expect(subject[:state]).to eq('closed') } - it { expect(subject[:created_at]).to eq('2008-09-01T08:05:00.000Z') } - it { expect(subject[:updated_at]).to eq('2008-09-01T08:05:00.000Z') } - it { expect(subject[:archived]).to eq(dossier.archived) } - - it { expect(subject.keys).to match_array(field_list) } + it do + expect(subject[:id]).to eq(dossier.id) + expect(subject[:state]).to eq('closed') + expect(subject[:created_at]).to eq('2008-09-01T08:05:00.000Z') + expect(subject[:updated_at]).to eq('2008-09-01T08:05:00.000Z') + expect(subject[:archived]).to eq(dossier.archived) + expect(subject.keys).to match_array(field_list) + end describe 'entreprise' do let(:field_list) { @@ -213,17 +219,19 @@ describe API::V1::DossiersController do } subject { super()[:entreprise] } - it { expect(subject[:siren]).to eq('440117620') } - it { expect(subject[:capital_social]).to eq(537_100_000) } - it { expect(subject[:numero_tva_intracommunautaire]).to eq('FR27440117620') } - it { expect(subject[:forme_juridique]).to eq('SA à conseil d\'administration (s.a.i.)') } - it { expect(subject[:forme_juridique_code]).to eq('5599') } - it { expect(subject[:nom_commercial]).to eq('GRTGAZ') } - it { expect(subject[:raison_sociale]).to eq('GRTGAZ') } - it { expect(subject[:siret_siege_social]).to eq('44011762001530') } - it { expect(subject[:code_effectif_entreprise]).to eq('51') } - it { expect(subject[:date_creation]).to eq('1990-04-24T00:00:00.000+00:00') } - it { expect(subject.keys).to match_array(field_list) } + it do + expect(subject[:siren]).to eq('440117620') + expect(subject[:capital_social]).to eq(537_100_000) + expect(subject[:numero_tva_intracommunautaire]).to eq('FR27440117620') + expect(subject[:forme_juridique]).to eq('SA à conseil d\'administration (s.a.i.)') + expect(subject[:forme_juridique_code]).to eq('5599') + expect(subject[:nom_commercial]).to eq('GRTGAZ') + expect(subject[:raison_sociale]).to eq('GRTGAZ') + expect(subject[:siret_siege_social]).to eq('44011762001530') + expect(subject[:code_effectif_entreprise]).to eq('51') + expect(subject[:date_creation]).to eq('1990-04-24T00:00:00.000+00:00') + expect(subject.keys).to match_array(field_list) + end end describe 'champs' do @@ -250,11 +258,13 @@ describe API::V1::DossiersController do } subject { super()[:type_de_champ] } - it { expect(subject.key?(:id)).to be_truthy } - it { expect(subject[:libelle]).to include('Libelle du champ') } - it { expect(subject[:description]).to include('description du champ') } - it { expect(subject.key?(:order_place)).to be_truthy } - it { expect(subject[:type_champ]).to eq('text') } + it do + expect(subject.key?(:id)).to be_truthy + expect(subject[:libelle]).to include('Libelle du champ') + expect(subject[:description]).to include('description du champ') + expect(subject.key?(:order_place)).to be_truthy + expect(subject[:type_champ]).to eq('text') + end end end @@ -310,11 +320,13 @@ describe API::V1::DossiersController do } subject { super()[:type_de_champ] } - it { expect(subject.key?(:id)).to be_truthy } - it { expect(subject[:libelle]).to include('Libelle champ privé') } - it { expect(subject[:description]).to include('description du champ privé') } - it { expect(subject.key?(:order_place)).to be_truthy } - it { expect(subject[:type_champ]).to eq('text') } + it do + expect(subject.key?(:id)).to be_truthy + expect(subject[:libelle]).to include('Libelle champ privé') + expect(subject[:description]).to include('description du champ privé') + expect(subject.key?(:order_place)).to be_truthy + expect(subject[:type_champ]).to eq('text') + end end end end @@ -325,11 +337,12 @@ describe API::V1::DossiersController do subject { super()[:commentaires] } - it { expect(subject.size).to eq 2 } - - it { expect(subject.first[:body]).to eq 'plop' } - it { expect(subject.first[:created_at]).to eq '2016-03-14T13:00:00.000Z' } - it { expect(subject.first[:email]).to eq 'plop@plip.com' } + it do + expect(subject.size).to eq 2 + expect(subject.first[:body]).to eq 'plop' + expect(subject.first[:created_at]).to eq '2016-03-14T13:00:00.000Z' + expect(subject.first[:email]).to eq 'plop@plip.com' + end end describe 'avis' do @@ -359,19 +372,21 @@ describe API::V1::DossiersController do } subject { super()[:etablissement] } - it { expect(subject[:siret]).to eq('44011762001530') } - it { expect(subject[:siege_social]).to eq(true) } - it { expect(subject[:naf]).to eq('4950Z') } - it { expect(subject[:libelle_naf]).to eq('Transports par conduites') } - it { expect(subject[:adresse]).to eq("GRTGAZ\r IMMEUBLE BORA\r 6 RUE RAOUL NORDLING\r 92270 BOIS COLOMBES\r") } - it { expect(subject[:numero_voie]).to eq('6') } - it { expect(subject[:type_voie]).to eq('RUE') } - it { expect(subject[:nom_voie]).to eq('RAOUL NORDLING') } - it { expect(subject[:complement_adresse]).to eq('IMMEUBLE BORA') } - it { expect(subject[:code_postal]).to eq('92270') } - it { expect(subject[:localite]).to eq('BOIS COLOMBES') } - it { expect(subject[:code_insee_localite]).to eq('92009') } - it { expect(subject.keys).to match_array(field_list) } + it do + expect(subject[:siret]).to eq('44011762001530') + expect(subject[:siege_social]).to eq(true) + expect(subject[:naf]).to eq('4950Z') + expect(subject[:libelle_naf]).to eq('Transports par conduites') + expect(subject[:adresse]).to eq("GRTGAZ\r IMMEUBLE BORA\r 6 RUE RAOUL NORDLING\r 92270 BOIS COLOMBES\r") + expect(subject[:numero_voie]).to eq('6') + expect(subject[:type_voie]).to eq('RUE') + expect(subject[:nom_voie]).to eq('RAOUL NORDLING') + expect(subject[:complement_adresse]).to eq('IMMEUBLE BORA') + expect(subject[:code_postal]).to eq('92270') + expect(subject[:localite]).to eq('BOIS COLOMBES') + expect(subject[:code_insee_localite]).to eq('92009') + expect(subject.keys).to match_array(field_list) + end end end end diff --git a/spec/controllers/api/v1/procedures_controller_spec.rb b/spec/controllers/api/v1/procedures_controller_spec.rb index ec96d4619..c0023618d 100644 --- a/spec/controllers/api/v1/procedures_controller_spec.rb +++ b/spec/controllers/api/v1/procedures_controller_spec.rb @@ -37,25 +37,29 @@ describe API::V1::ProceduresController, type: :controller do subject { JSON.parse(response.body, symbolize_names: true)[:procedure] } - it { expect(subject[:id]).to eq(procedure.id) } - it { expect(subject[:label]).to eq(procedure.libelle) } - it { expect(subject[:description]).to eq(procedure.description) } - it { expect(subject[:organisation]).to eq(procedure.organisation) } - it { expect(subject[:archived_at]).to eq(procedure.closed_at) } - it { expect(subject[:direction]).to eq("") } - 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) } + it do + expect(subject[:id]).to eq(procedure.id) + expect(subject[:label]).to eq(procedure.libelle) + expect(subject[:description]).to eq(procedure.description) + expect(subject[:organisation]).to eq(procedure.organisation) + expect(subject[:archived_at]).to eq(procedure.closed_at) + expect(subject[:direction]).to eq("") + expect(subject[:total_dossier]).to eq(procedure.total_dossier) + is_expected.to have_key(:types_de_champ) + expect(subject[:types_de_champ]).to be_an(Array) + end describe 'type_de_champ' do subject { super()[:types_de_champ][0] } let(:champ) { procedure.active_revision.types_de_champ_public.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) } - it { expect(subject[:description]).to eq(champ.description) } + it do + expect(subject[:id]).to eq(champ.id) + expect(subject[:libelle]).to eq(champ.libelle) + expect(subject[:type_champ]).to eq(champ.type_champ) + expect(subject[:description]).to eq(champ.description) + end end describe 'service' do @@ -63,14 +67,16 @@ describe API::V1::ProceduresController, type: :controller do let(:service) { procedure.service } - it { expect(subject[:id]).to eq(service.id) } - it { expect(subject[:email]).to eq(service.email) } - it { expect(subject[:name]).to eq(service.nom) } - it { expect(subject[:type_organization]).to eq(service.type_organisme) } - it { expect(subject[:organization]).to eq(service.organisme) } - it { expect(subject[:phone]).to eq(service.telephone) } - it { expect(subject[:schedule]).to eq(service.horaires) } - it { expect(subject[:address]).to eq(service.adresse) } + it do + expect(subject[:id]).to eq(service.id) + expect(subject[:email]).to eq(service.email) + expect(subject[:name]).to eq(service.nom) + expect(subject[:type_organization]).to eq(service.type_organisme) + expect(subject[:organization]).to eq(service.organisme) + expect(subject[:phone]).to eq(service.telephone) + expect(subject[:schedule]).to eq(service.horaires) + expect(subject[:address]).to eq(service.adresse) + end end end end