From b8338fb9af6f36919e6dee91f547176a9fde71ce Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Mar 2023 13:49:21 +1100 Subject: [PATCH 1/4] Removing engine namespace from services Services are usually not namespaced because they are part of the app. This engine has an isolated namespace which means that we don't need to separate with out own namespacing here. --- .../dfc_provider/base_controller.rb | 2 +- .../dfc_provider/catalog_items_controller.rb | 2 +- .../supplied_products_controller.rb | 2 +- .../dfc_provider/enterprise_serializer.rb | 4 +- .../app/services/authorization_control.rb | 54 ++++++++++++++++++ .../dfc_provider/authorization_control.rb | 56 ------------------- .../services/dfc_provider/variant_fetcher.rb | 18 ------ .../app/services/variant_fetcher.rb | 16 ++++++ .../dfc_provider/enterprises_spec.rb | 2 +- .../dfc_provider/persons_controller_spec.rb | 2 +- .../supplied_products_controller_spec.rb | 2 +- .../services/authorization_control_spec.rb | 2 +- .../spec/support/authorization_helper.rb | 2 +- 13 files changed, 80 insertions(+), 84 deletions(-) create mode 100644 engines/dfc_provider/app/services/authorization_control.rb delete mode 100644 engines/dfc_provider/app/services/dfc_provider/authorization_control.rb delete mode 100644 engines/dfc_provider/app/services/dfc_provider/variant_fetcher.rb create mode 100644 engines/dfc_provider/app/services/variant_fetcher.rb diff --git a/engines/dfc_provider/app/controllers/dfc_provider/base_controller.rb b/engines/dfc_provider/app/controllers/dfc_provider/base_controller.rb index bcee581836..389882a078 100644 --- a/engines/dfc_provider/app/controllers/dfc_provider/base_controller.rb +++ b/engines/dfc_provider/app/controllers/dfc_provider/base_controller.rb @@ -42,7 +42,7 @@ module DfcProvider end def authorization_control - DfcProvider::AuthorizationControl.new(request) + AuthorizationControl.new(request) end def not_found diff --git a/engines/dfc_provider/app/controllers/dfc_provider/catalog_items_controller.rb b/engines/dfc_provider/app/controllers/dfc_provider/catalog_items_controller.rb index 6851e7dd93..378e904d2f 100644 --- a/engines/dfc_provider/app/controllers/dfc_provider/catalog_items_controller.rb +++ b/engines/dfc_provider/app/controllers/dfc_provider/catalog_items_controller.rb @@ -22,7 +22,7 @@ module DfcProvider def variant @variant ||= - DfcProvider::VariantFetcher.new(current_enterprise).scope.find(params[:id]) + VariantFetcher.new(current_enterprise).scope.find(params[:id]) end end end diff --git a/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb b/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb index f1ac6c08c0..ec9a7767d5 100644 --- a/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb +++ b/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb @@ -21,7 +21,7 @@ module DfcProvider def variant @variant ||= - DfcProvider::VariantFetcher.new(current_enterprise).scope.find(params[:id]) + VariantFetcher.new(current_enterprise).scope.find(params[:id]) end end end diff --git a/engines/dfc_provider/app/serializers/dfc_provider/enterprise_serializer.rb b/engines/dfc_provider/app/serializers/dfc_provider/enterprise_serializer.rb index b45b52250e..77754602a2 100644 --- a/engines/dfc_provider/app/serializers/dfc_provider/enterprise_serializer.rb +++ b/engines/dfc_provider/app/serializers/dfc_provider/enterprise_serializer.rb @@ -32,11 +32,11 @@ module DfcProvider end def supplies - DfcProvider::VariantFetcher.new(object).scope + VariantFetcher.new(object).scope end def manages - DfcProvider::VariantFetcher.new(object).scope + VariantFetcher.new(object).scope end end end diff --git a/engines/dfc_provider/app/services/authorization_control.rb b/engines/dfc_provider/app/services/authorization_control.rb new file mode 100644 index 0000000000..dd9f3b045c --- /dev/null +++ b/engines/dfc_provider/app/services/authorization_control.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +# Service used to authorize the user on DCF Provider API +# It controls an OICD Access token and an enterprise. +class AuthorizationControl + # Copied from: https://login.lescommuns.org/auth/realms/data-food-consortium/ + LES_COMMUNES_PUBLIC_KEY = <<~KEY + -----BEGIN PUBLIC KEY----- + MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAl68JGqAILFzoi/1+6siXXp2vylu+7mPjYKjKelTtHFYXWVkbmVptCsamHlY3jRhqSQYe6M1SKfw8D+uXrrWsWficYvpdlV44Vm7uETZOr1/XBOjpWOi1vLmBVtX6jFeqN1BxfE1PxLROAiGn+MeMg90AJKShD2c5RoNv26e20dgPhshRVFPUGru+0T1RoKyIa64z/qcTcTVD2V7KX+ANMweRODdoPAzQFGGjTnL1uUqIdUwSfHSpXYnKxXOsnPC3Mowkv8UIGWWDxS/yzhWc7sOk1NmC7pb+Cg7G8NKj+Pp9qQZnXF39Dg95ZsxJrl6fyPFvTo3zf9CPG/fUM1CkkwIDAQAB + -----END PUBLIC KEY----- + KEY + + def self.public_key + OpenSSL::PKey::RSA.new(LES_COMMUNES_PUBLIC_KEY) + end + + def initialize(request) + @request = request + end + + def user + oidc_user || ofn_user + rescue JWT::ExpiredSignature + nil + end + + private + + def oidc_user + find_ofn_user(decode_token) if access_token + end + + def ofn_user + @request.env['warden']&.user + end + + def decode_token + JWT.decode( + access_token, + self.class.public_key, + true, { algorithm: "RS256" } + ).first + end + + def access_token + @request.headers['Authorization'].to_s.split(' ').last + end + + def find_ofn_user(payload) + return if payload["email"].blank? + + Spree::User.find_by(uid: payload["email"]) + end +end diff --git a/engines/dfc_provider/app/services/dfc_provider/authorization_control.rb b/engines/dfc_provider/app/services/dfc_provider/authorization_control.rb deleted file mode 100644 index dcdbd8ad5f..0000000000 --- a/engines/dfc_provider/app/services/dfc_provider/authorization_control.rb +++ /dev/null @@ -1,56 +0,0 @@ -# frozen_string_literal: true - -# Service used to authorize the user on DCF Provider API -# It controls an OICD Access token and an enterprise. -module DfcProvider - class AuthorizationControl - # Copied from: https://login.lescommuns.org/auth/realms/data-food-consortium/ - LES_COMMUNES_PUBLIC_KEY = <<~KEY - -----BEGIN PUBLIC KEY----- - MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAl68JGqAILFzoi/1+6siXXp2vylu+7mPjYKjKelTtHFYXWVkbmVptCsamHlY3jRhqSQYe6M1SKfw8D+uXrrWsWficYvpdlV44Vm7uETZOr1/XBOjpWOi1vLmBVtX6jFeqN1BxfE1PxLROAiGn+MeMg90AJKShD2c5RoNv26e20dgPhshRVFPUGru+0T1RoKyIa64z/qcTcTVD2V7KX+ANMweRODdoPAzQFGGjTnL1uUqIdUwSfHSpXYnKxXOsnPC3Mowkv8UIGWWDxS/yzhWc7sOk1NmC7pb+Cg7G8NKj+Pp9qQZnXF39Dg95ZsxJrl6fyPFvTo3zf9CPG/fUM1CkkwIDAQAB - -----END PUBLIC KEY----- - KEY - - def self.public_key - OpenSSL::PKey::RSA.new(LES_COMMUNES_PUBLIC_KEY) - end - - def initialize(request) - @request = request - end - - def user - oidc_user || ofn_user - rescue JWT::ExpiredSignature - nil - end - - private - - def oidc_user - find_ofn_user(decode_token) if access_token - end - - def ofn_user - @request.env['warden']&.user - end - - def decode_token - JWT.decode( - access_token, - self.class.public_key, - true, { algorithm: "RS256" } - ).first - end - - def access_token - @request.headers['Authorization'].to_s.split(' ').last - end - - def find_ofn_user(payload) - return if payload["email"].blank? - - Spree::User.find_by(uid: payload["email"]) - end - end -end diff --git a/engines/dfc_provider/app/services/dfc_provider/variant_fetcher.rb b/engines/dfc_provider/app/services/dfc_provider/variant_fetcher.rb deleted file mode 100644 index 5ccb95b17a..0000000000 --- a/engines/dfc_provider/app/services/dfc_provider/variant_fetcher.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -# Service used to fetch variants related to an entreprise. -# It improves maintenance as it is the central point requesting -# Spree::Varaint inside the DfcProvider engine. -module DfcProvider - class VariantFetcher - def initialize(enterprise) - @enterprise = enterprise - end - - def scope - Spree::Variant. - joins(product: :supplier). - where('enterprises.id' => @enterprise.id) - end - end -end diff --git a/engines/dfc_provider/app/services/variant_fetcher.rb b/engines/dfc_provider/app/services/variant_fetcher.rb new file mode 100644 index 0000000000..3947a12b65 --- /dev/null +++ b/engines/dfc_provider/app/services/variant_fetcher.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +# Service used to fetch variants related to an entreprise. +# It improves maintenance as it is the central point requesting +# Spree::Variant inside the DfcProvider engine. +class VariantFetcher + def initialize(enterprise) + @enterprise = enterprise + end + + def scope + Spree::Variant. + joins(product: :supplier). + where('enterprises.id' => @enterprise.id) + end +end diff --git a/engines/dfc_provider/spec/controllers/dfc_provider/enterprises_spec.rb b/engines/dfc_provider/spec/controllers/dfc_provider/enterprises_spec.rb index b97e2d54f9..e56cafc54b 100644 --- a/engines/dfc_provider/spec/controllers/dfc_provider/enterprises_spec.rb +++ b/engines/dfc_provider/spec/controllers/dfc_provider/enterprises_spec.rb @@ -17,7 +17,7 @@ describe DfcProvider::EnterprisesController, type: :controller do context 'with an authenticated user' do before do - allow_any_instance_of(DfcProvider::AuthorizationControl) + allow_any_instance_of(AuthorizationControl) .to receive(:user) .and_return(user) end diff --git a/engines/dfc_provider/spec/controllers/dfc_provider/persons_controller_spec.rb b/engines/dfc_provider/spec/controllers/dfc_provider/persons_controller_spec.rb index 241bbd8d51..680c4dea0f 100644 --- a/engines/dfc_provider/spec/controllers/dfc_provider/persons_controller_spec.rb +++ b/engines/dfc_provider/spec/controllers/dfc_provider/persons_controller_spec.rb @@ -15,7 +15,7 @@ describe DfcProvider::PersonsController, type: :controller do context 'with an authenticated user' do before do - allow_any_instance_of(DfcProvider::AuthorizationControl) + allow_any_instance_of(AuthorizationControl) .to receive(:user) .and_return(user) end diff --git a/engines/dfc_provider/spec/controllers/dfc_provider/supplied_products_controller_spec.rb b/engines/dfc_provider/spec/controllers/dfc_provider/supplied_products_controller_spec.rb index dd555262a2..ca04ec9b91 100644 --- a/engines/dfc_provider/spec/controllers/dfc_provider/supplied_products_controller_spec.rb +++ b/engines/dfc_provider/spec/controllers/dfc_provider/supplied_products_controller_spec.rb @@ -20,7 +20,7 @@ describe DfcProvider::SuppliedProductsController, type: :controller do context 'with an authenticated user' do before do - allow_any_instance_of(DfcProvider::AuthorizationControl) + allow_any_instance_of(AuthorizationControl) .to receive(:user) .and_return(user) end diff --git a/engines/dfc_provider/spec/services/authorization_control_spec.rb b/engines/dfc_provider/spec/services/authorization_control_spec.rb index ea52c5806e..be6e3af441 100644 --- a/engines/dfc_provider/spec/services/authorization_control_spec.rb +++ b/engines/dfc_provider/spec/services/authorization_control_spec.rb @@ -2,7 +2,7 @@ require DfcProvider::Engine.root.join("spec/spec_helper") -describe DfcProvider::AuthorizationControl do +describe AuthorizationControl do include AuthorizationHelper let(:user) { create(:oidc_user) } diff --git a/engines/dfc_provider/spec/support/authorization_helper.rb b/engines/dfc_provider/spec/support/authorization_helper.rb index f35e028f1a..4d8a42fdad 100644 --- a/engines/dfc_provider/spec/support/authorization_helper.rb +++ b/engines/dfc_provider/spec/support/authorization_helper.rb @@ -8,7 +8,7 @@ module AuthorizationHelper def allow_token_for(payload) private_key = OpenSSL::PKey::RSA.generate 2048 - allow(DfcProvider::AuthorizationControl).to receive(:public_key). + allow(AuthorizationControl).to receive(:public_key). and_return(private_key.public_key) JWT.encode(payload, private_key, "RS256") From 6e514acc77a1f0a237dbf38304904db49702c99e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Mar 2023 14:13:39 +1100 Subject: [PATCH 2/4] Spec too many variants exported to DFC --- .../spec/services/variant_fetcher_spec.rb | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 engines/dfc_provider/spec/services/variant_fetcher_spec.rb diff --git a/engines/dfc_provider/spec/services/variant_fetcher_spec.rb b/engines/dfc_provider/spec/services/variant_fetcher_spec.rb new file mode 100644 index 0000000000..8a7adebc15 --- /dev/null +++ b/engines/dfc_provider/spec/services/variant_fetcher_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require DfcProvider::Engine.root.join("spec/spec_helper") + +describe VariantFetcher do + subject { VariantFetcher.new(enterprise) } + let(:enterprise) { build(:enterprise) } + let(:other_enterprise) { build(:enterprise) } + + it "returns an empty set" do + expect(subject.scope).to eq [] + end + + it "returns the variants of a supplier" do + product = create(:product, supplier: enterprise) + + pending "ignoring of the master variant" + # it currently returns two variants instead of one + expect(subject.scope.count).to eq 1 + expect(subject.scope).to eq product.variants + end + + it "ignores the variants of another enterprise" do + create(:product, supplier: other_enterprise) + + expect(subject.scope).to eq [] + end +end From 2105c0d0eadbe170b8cea91c95fb49fe2db4f943 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Mar 2023 14:21:10 +1100 Subject: [PATCH 3/4] Ignore master variants exporting to DFC --- engines/dfc_provider/app/services/variant_fetcher.rb | 2 +- engines/dfc_provider/spec/services/variant_fetcher_spec.rb | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/engines/dfc_provider/app/services/variant_fetcher.rb b/engines/dfc_provider/app/services/variant_fetcher.rb index 3947a12b65..e99eb8dd02 100644 --- a/engines/dfc_provider/app/services/variant_fetcher.rb +++ b/engines/dfc_provider/app/services/variant_fetcher.rb @@ -9,7 +9,7 @@ class VariantFetcher end def scope - Spree::Variant. + Spree::Variant.not_master. joins(product: :supplier). where('enterprises.id' => @enterprise.id) end diff --git a/engines/dfc_provider/spec/services/variant_fetcher_spec.rb b/engines/dfc_provider/spec/services/variant_fetcher_spec.rb index 8a7adebc15..84a916c43f 100644 --- a/engines/dfc_provider/spec/services/variant_fetcher_spec.rb +++ b/engines/dfc_provider/spec/services/variant_fetcher_spec.rb @@ -14,8 +14,6 @@ describe VariantFetcher do it "returns the variants of a supplier" do product = create(:product, supplier: enterprise) - pending "ignoring of the master variant" - # it currently returns two variants instead of one expect(subject.scope.count).to eq 1 expect(subject.scope).to eq product.variants end From 9c3bdc6b9b37d7ca6c1e4df535df88bb216d2ff8 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Mar 2023 14:24:04 +1100 Subject: [PATCH 4/4] Remove unnecessary table join And use Rails syntax for clarity and future extensions. --- engines/dfc_provider/app/services/variant_fetcher.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/engines/dfc_provider/app/services/variant_fetcher.rb b/engines/dfc_provider/app/services/variant_fetcher.rb index e99eb8dd02..df3fd86f82 100644 --- a/engines/dfc_provider/app/services/variant_fetcher.rb +++ b/engines/dfc_provider/app/services/variant_fetcher.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Service used to fetch variants related to an entreprise. +# Service used to fetch variants related to an enterprise. # It improves maintenance as it is the central point requesting # Spree::Variant inside the DfcProvider engine. class VariantFetcher @@ -10,7 +10,7 @@ class VariantFetcher def scope Spree::Variant.not_master. - joins(product: :supplier). - where('enterprises.id' => @enterprise.id) + joins(:product). + where(spree_products: { supplier: @enterprise }) end end