From 0824430da5e31b9f2b21af862c51640da5d328d5 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 24 Sep 2024 10:43:55 +1000 Subject: [PATCH 01/12] Add Vine connected app The connection/disconnection logic is yet to be implemented --- app/models/connected_app.rb | 3 ++- app/models/connected_apps/vine.rb | 15 +++++++++++ .../form/connected_apps/_vine.html.haml | 27 +++++++++++++++++++ app/webpacker/css/admin/connected_apps.scss | 23 +++++++++++++++- config/locales/en.yml | 17 ++++++++++++ 5 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 app/models/connected_apps/vine.rb create mode 100644 app/views/admin/enterprises/form/connected_apps/_vine.html.haml diff --git a/app/models/connected_app.rb b/app/models/connected_app.rb index 966df543b5..faf1b0abe7 100644 --- a/app/models/connected_app.rb +++ b/app/models/connected_app.rb @@ -4,13 +4,14 @@ # # Here we store keys and links to access the app. class ConnectedApp < ApplicationRecord - TYPES = ['discover_regen', 'affiliate_sales_data'].freeze + TYPES = ['discover_regen', 'affiliate_sales_data', 'vine'].freeze belongs_to :enterprise after_destroy :disconnect scope :discover_regen, -> { where(type: "ConnectedApp") } scope :affiliate_sales_data, -> { where(type: "ConnectedApps::AffiliateSalesData") } + scope :vine, -> { where(type: "ConnectedApps::Vine") } scope :connecting, -> { where(data: nil) } scope :ready, -> { where.not(data: nil) } diff --git a/app/models/connected_apps/vine.rb b/app/models/connected_apps/vine.rb new file mode 100644 index 0000000000..46fad0f1c1 --- /dev/null +++ b/app/models/connected_apps/vine.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# An enterprise can opt-in to use VINE API to manage vouchers +# +module ConnectedApps + class Vine < ConnectedApp + def connect(api_key: ) + # TODO + end + + def disconnect + # TODO + end + end +end diff --git a/app/views/admin/enterprises/form/connected_apps/_vine.html.haml b/app/views/admin/enterprises/form/connected_apps/_vine.html.haml new file mode 100644 index 0000000000..1281a73e84 --- /dev/null +++ b/app/views/admin/enterprises/form/connected_apps/_vine.html.haml @@ -0,0 +1,27 @@ +%section.connected_app + .connected-app__head + %div + %h3= t ".title" + %p= t ".tagline" + .connected-app__vine + - if connected_app.nil? + = form_with url: admin_enterprise_connected_apps_path(enterprise.id) do |f| + .connected-app__vine-content + .vine-api-key + = f.hidden_field :type, value: "ConnectedApps::Vine" + = f.label :vine_api_key, t(".vine_api_key") + %span.required * + = f.text_field :vine_api_key, { disabled: !managed_by_user?(enterprise) } + %div + - disabled = managed_by_user?(enterprise) ? {} : { disabled: true, "data-disable-with": false } + = f.submit t(".enable"), disabled + + -# This is only seen by super-admins: + %em= t(".need_to_be_manager") unless managed_by_user?(enterprise) + - else + .connected-app__vine-content + .vine-disable + = button_to t(".disable"), admin_enterprise_connected_app_path(connected_app.id, enterprise_id: enterprise.id), method: :delete + %hr + .connected-app__description + = t ".description_html" diff --git a/app/webpacker/css/admin/connected_apps.scss b/app/webpacker/css/admin/connected_apps.scss index be3356009d..f68d902ad0 100644 --- a/app/webpacker/css/admin/connected_apps.scss +++ b/app/webpacker/css/admin/connected_apps.scss @@ -34,7 +34,9 @@ border: none; border-left: $border-radius solid $color-3; border-radius: $border-radius; - box-shadow: 0px 1px 0px rgba(0, 0, 0, 0.05), 0px 2px 2px rgba(0, 0, 0, 0.07); + box-shadow: + 0px 1px 0px rgba(0, 0, 0, 0.05), + 0px 2px 2px rgba(0, 0, 0, 0.07); margin: 2em 0; padding: 0.5em 1em; @@ -47,3 +49,22 @@ flex-shrink: 1; } } + +.connected-app__vine { + margin: 1em 0; + + .connected-app__vine-content { + display: flex; + justify-content: space-between; + align-items: end; + + .vine-api-key { + width: 100%; + margin-right: 1em; + } + + .vine-disable { + margin-left: auto; + } + } +} diff --git a/config/locales/en.yml b/config/locales/en.yml index f1041c92bb..da1bbe8043 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -753,6 +753,7 @@ en: connected_apps_enabled: discover_regen: Discover Regenerative portal affiliate_sales_data: DFC anonymised orders API for research purposes + vine: Voucher Engine (VINE) update: resource: Connected app settings @@ -1418,6 +1419,22 @@ en: target="_blank">Learn more about Discover Regenerative

+ vine: + title: "Voucher Engine (VINE)" + tagline: "Enable the use of VINE api to handle vouchers" + enable: "Connect" + disable: "Disconnect" + need_to_be_manager: "Only managers can connect apps." + vine_api_key: "VINE API Key" + description_html: | +

+ To enable VINE for your enterprise, enter your API key. +

+

+ VINE + +

+ actions: edit_profile: Settings properties: Properties From f7708d69a785516dd468099243413966dffb40dd Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 1 Oct 2024 15:58:09 +1000 Subject: [PATCH 02/12] Add VineJwtService Generate a JWT token to be used to connect to the VINE api --- app/services/vine_jwt_service.rb | 21 +++++++++++ spec/services/vine_jwt_service_spec.rb | 52 ++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) create mode 100644 app/services/vine_jwt_service.rb create mode 100644 spec/services/vine_jwt_service_spec.rb diff --git a/app/services/vine_jwt_service.rb b/app/services/vine_jwt_service.rb new file mode 100644 index 0000000000..f65f04a7f6 --- /dev/null +++ b/app/services/vine_jwt_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class VineJwtService + ALGORITHM = "HS256" + ISSUER = "openfoodnetwork" + + def initialize(secret: ) + @secret = secret + end + + def generate_token + generation_time = Time.zone.now + payload = { + iss: ISSUER, + iat: generation_time.to_i, + exp: (generation_time + 1.minute).to_i, + } + + JWT.encode(payload, @secret, ALGORITHM) + end +end diff --git a/spec/services/vine_jwt_service_spec.rb b/spec/services/vine_jwt_service_spec.rb new file mode 100644 index 0000000000..204dede96c --- /dev/null +++ b/spec/services/vine_jwt_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe VineJwtService do + describe "#generate_token" do + subject { described_class.new(secret: vine_secret) } + let(:vine_secret) { "some_secret" } + + it "generate a jwt token" do + expect(subject.generate_token).to be_a String + end + + it "includes issuing body" do + token = subject.generate_token + + payload = decode(token, vine_secret) + + expect(payload["iss"]).to eq("openfoodnetwork") + end + + it "includes issuing time" do + generate_time = Time.zone.now + Timecop.freeze(generate_time) do + token = subject.generate_token + + payload = decode(token, vine_secret) + + expect(payload["iat"].to_i).to eq(generate_time.to_i) + end + end + + it "includes expirations time" do + generate_time = Time.zone.now + Timecop.freeze(generate_time) do + token = subject.generate_token + + payload = decode(token, vine_secret) + + expect(payload["exp"].to_i).to eq((generate_time + 1.minute).to_i) + end + end + end + + def decode(token, secret) + JWT.decode( + token, + secret, + true, { algorithm: "HS256" } + ).first + end +end From 1a30cf649540f48a94c01db7b6c7c221f20ad674 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 1 Oct 2024 13:58:32 +1000 Subject: [PATCH 03/12] Hide VINE token --- spec/support/vcr_setup.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/support/vcr_setup.rb b/spec/support/vcr_setup.rb index bf2574a0b3..d9958751a6 100644 --- a/spec/support/vcr_setup.rb +++ b/spec/support/vcr_setup.rb @@ -45,4 +45,7 @@ VCR.configure do |config| config.filter_sensitive_data('') { |interaction| interaction.request.body.match(/"accessToken":"([^"]+)"/)&.public_send(:[], 1) } + config.filter_sensitive_data('') { |interaction| + interaction.request.headers["X-Authorization"]&.public_send(:[], 0) + } end From 097c6dee2f28bdc008dade05cc4e89f316727002 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 2 Oct 2024 10:42:11 +1000 Subject: [PATCH 04/12] Add VineApiService and specs It handles connection to the VINE API --- app/services/vine_api_service.rb | 39 +++++++++ .../returns_the_response_do.yml | 53 ++++++++++++ spec/services/vine_api_service_spec.rb | 83 +++++++++++++++++++ 3 files changed, 175 insertions(+) create mode 100644 app/services/vine_api_service.rb create mode 100644 spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response_do.yml create mode 100644 spec/services/vine_api_service_spec.rb diff --git a/app/services/vine_api_service.rb b/app/services/vine_api_service.rb new file mode 100644 index 0000000000..8fa34ce48a --- /dev/null +++ b/app/services/vine_api_service.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require "faraday" + +class VineApiService + attr_reader :api_key, :jwt_generator + + def initialize(api_key:, jwt_generator:) + @vine_api_url = ENV.fetch("VINE_API_URL") + @api_key = api_key + @jwt_generator = jwt_generator + end + + def my_team + my_team_url = "#{@vine_api_url}/my-team" + + jwt = jwt_generator.generate_token + connection = Faraday.new( + request: { timeout: 30 }, + headers: { + 'X-Authorization': "JWT #{jwt}", + Accept: "application/json" + } + ) do |f| + f.request :json + f.response :json + f.request :authorization, 'Bearer', api_key + end + + response = connection.get(my_team_url) + + if !response.success? + Rails.logger.error "VineApiService#my_team -- response_status: #{response.status}" + Rails.logger.error "VineApiService#my_team -- response: #{response.body}" + end + + response + end +end diff --git a/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response_do.yml b/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response_do.yml new file mode 100644 index 0000000000..136540eb6a --- /dev/null +++ b/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response_do.yml @@ -0,0 +1,53 @@ +--- +http_interactions: +- request: + method: get + uri: https://vine-staging.openfoodnetwork.org.au/api/v1/my-team + body: + encoding: US-ASCII + string: '' + headers: + X-Authorization: + - "" + User-Agent: + - Faraday v2.9.0 + Authorization: + - "" + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Accept: + - "*/*" + response: + status: + code: 200 + message: OK + headers: + Server: + - nginx + Content-Type: + - application/json + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Vary: + - Accept-Encoding + Cache-Control: + - no-cache, private + Date: + - Wed, 02 Oct 2024 00:27:30 GMT + Access-Control-Allow-Origin: + - "*" + X-Frame-Options: + - SAMEORIGIN + X-Xss-Protection: + - 1; mode=block + X-Content-Type-Options: + - nosniff + body: + encoding: ASCII-8BIT + string: '{"meta":{"responseCode":200,"limit":50,"offset":0,"message":"","cached":true,"cached_at":"2024-10-02 + 00:27:30","availableRelations":["teamUsers.user","country"]},"data":{"id":1,"name":"OK200 + Team","country_id":14,"created_at":"2024-09-12T06:39:07.000000Z","updated_at":"2024-09-12T06:39:07.000000Z","deleted_at":null}}' + recorded_at: Wed, 02 Oct 2024 00:27:30 GMT +recorded_with: VCR 6.2.0 diff --git a/spec/services/vine_api_service_spec.rb b/spec/services/vine_api_service_spec.rb new file mode 100644 index 0000000000..c0cb2953a4 --- /dev/null +++ b/spec/services/vine_api_service_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe VineApiService do + subject(:vine_api) { described_class.new(api_key: vine_api_key, jwt_generator: jwt_service) } + + let(:vine_api_url) { "https://vine-staging.openfoodnetwork.org.au/api/v1" } + let(:vine_api_key) { "12345" } + let(:jwt_service) { VineJwtService.new(secret:) } + let(:secret) { "my_secret" } + + before do + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("VINE_API_URL").and_return(vine_api_url) + end + + describe "#my_team" do + let(:my_team_url) { "#{vine_api_url}/my-team" } + + it "send a request to the team VINE api endpoint" do + stub_request(:get, my_team_url).to_return(status: 200) + + vine_api.my_team + + expect(a_request( + :get, "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team" + )).to have_been_made + end + + it "sends the VINE api key via a header" do + stub_request(:get, my_team_url).to_return(status: 200) + + vine_api.my_team + + expect(a_request(:get, "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team").with( + headers: { Authorization: "Bearer #{vine_api_key}" } + )).to have_been_made + end + + it "sends JWT token via a header" do + token = "some.jwt.token" + stub_request(:get, my_team_url).to_return(status: 200) + + allow(jwt_service).to receive(:generate_token).and_return(token) + + vine_api.my_team + + expect(a_request(:get, "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team").with( + headers: { 'X-Authorization': "JWT #{token}" } + )).to have_been_made + end + + context "when a request succeed", :vcr do + it "returns the response do" do + response = vine_api.my_team + + expect(response.success?).to be(true) + expect(response.body).not_to be_empty + end + end + + context "when a request fails" do + it "logs the error" do + stub_request(:get, my_team_url).to_return(body: "error", status: 401) + + expect(Rails.logger).to receive(:error).twice + + response = vine_api.my_team + + expect(response.success?).to be(false) + end + + it "return the response" do + stub_request(:get, my_team_url).to_return(body: "error", status: 401) + response = vine_api.my_team + + expect(response.success?).to be(false) + expect(response.body).not_to be_empty + end + end + end +end From f980cb45f61e3bab5055e94dbb2c5f0b7d62276e Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 2 Oct 2024 11:00:42 +1000 Subject: [PATCH 05/12] Add logic for ConnectedApps::Vine#connect and disconnect --- app/models/connected_apps/vine.rb | 16 ++++++--- config/locales/en.yml | 3 ++ spec/models/connected_apps/vine_spec.rb | 45 +++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 spec/models/connected_apps/vine_spec.rb diff --git a/app/models/connected_apps/vine.rb b/app/models/connected_apps/vine.rb index 46fad0f1c1..f32858d0b2 100644 --- a/app/models/connected_apps/vine.rb +++ b/app/models/connected_apps/vine.rb @@ -4,12 +4,18 @@ # module ConnectedApps class Vine < ConnectedApp - def connect(api_key: ) - # TODO + VINE_API_URL = "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team" + + def connect(api_key:, vine_api:, **_opts) + response = vine_api.my_team + + return update data: { api_key: } if response.success? + + errors.add(:base, I18n.t("activerecord.errors.models.connected_apps.vine.api_request_error")) + + false end - def disconnect - # TODO - end + def disconnect; end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index da1bbe8043..d0136b68ad 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -128,6 +128,9 @@ en: count_on_hand: using_producer_stock_settings_but_count_on_hand_set: "must be blank because using producer stock settings" limited_stock_but_no_count_on_hand: "must be specified because forcing limited stock" + connected_apps: + vine: + api_request_error: "An error occured when connecting to Vine API" messages: confirmation: "doesn't match %{attribute}" blank: "can't be blank" diff --git a/spec/models/connected_apps/vine_spec.rb b/spec/models/connected_apps/vine_spec.rb new file mode 100644 index 0000000000..7d94d3693e --- /dev/null +++ b/spec/models/connected_apps/vine_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe ConnectedApps::Vine do + subject(:connected_app) { ConnectedApps::Vine.new(enterprise: create(:enterprise)) } + + let(:vine_api_key) { "12345" } + let(:vine_api) { instance_double(VineApiService) } + + describe "#connect" do + it "send a request to VINE api" do + expect(vine_api).to receive(:my_team).and_return(mock_api_response(true)) + + connected_app.connect(api_key: vine_api_key, vine_api: ) + end + + context "when request succeed", :vcr do + it "store the vine api key" do + allow(vine_api).to receive(:my_team).and_return(mock_api_response(true)) + + expect(connected_app.connect(api_key: vine_api_key, vine_api:)).to eq(true) + expect(connected_app.data).to eql({ "api_key" => vine_api_key }) + end + end + + context "when request fails" do + it "doesn't store the vine api key" do + allow(vine_api).to receive(:my_team).and_return(mock_api_response(false)) + + expect(connected_app.connect(api_key: vine_api_key, vine_api:)).to eq(false) + expect(connected_app.data).to be_nil + expect(connected_app.errors[:base]).to include( + "An error occured when connecting to Vine API" + ) + end + end + end + + def mock_api_response(success) + mock_response = instance_double(Faraday::Response) + allow(mock_response).to receive(:success?).and_return(success) + mock_response + end +end From 22428fc78dd7621eecdbd1c794940fc5c3e4f37b Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 2 Oct 2024 15:40:27 +1000 Subject: [PATCH 06/12] ConnectedApps controller, handle ConnectedApps::Vine Add logiv to connect and disconnect VINE API plus spec --- .env | 4 + .../admin/connected_apps_controller.rb | 58 +++++- .../initializers/filter_parameter_logging.rb | 4 +- config/locales/en.yml | 5 +- .../admin/connected_apps_controller_spec.rb | 174 ++++++++++++++++++ 5 files changed, 236 insertions(+), 9 deletions(-) create mode 100644 spec/requests/admin/connected_apps_controller_spec.rb diff --git a/.env b/.env index fe9b06d4ff..b5bfa57c44 100644 --- a/.env +++ b/.env @@ -61,3 +61,7 @@ SMTP_PASSWORD="f00d" # NEW_RELIC_AGENT_ENABLED=true # NEW_RELIC_APP_NAME="Open Food Network" # NEW_RELIC_LICENSE_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + +# VINE API settings +# VINE_API_URL="https://vine-staging.openfoodnetwork.org.au/api/v1" +# VINE_API_SECRET="XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" diff --git a/app/controllers/admin/connected_apps_controller.rb b/app/controllers/admin/connected_apps_controller.rb index 3531c7371a..6cf7ff34ed 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -5,12 +5,7 @@ module Admin def create authorize! :admin, enterprise - attributes = {} - attributes[:type] = connected_app_params[:type] if connected_app_params[:type] - - app = ConnectedApp.create!(enterprise_id: enterprise.id, **attributes) - app.connect(api_key: spree_current_user.spree_api_key, - channel: SessionChannel.for_request(request)) + connect render_panel end @@ -26,6 +21,45 @@ module Admin private + def create_connected_app + attributes = {} + attributes[:type] = connected_app_params[:type] if connected_app_params[:type] + + @app = ConnectedApp.create!(enterprise_id: enterprise.id, **attributes) + end + + def connect + return connect_vine if connected_app_params[:type] == "ConnectedApps::Vine" + + create_connected_app + @app.connect(api_key: spree_current_user.spree_api_key, + channel: SessionChannel.for_request(request)) + end + + def connect_vine + if connected_app_params[:vine_api_key].empty? + return flash[:error] = I18n.t("admin.enterprises.form.connected_apps.vine.api_key_empty") + end + + create_connected_app + + jwt_service = VineJwtService.new(secret: ENV.fetch("VINE_API_SECRET")) + vine_api = VineApiService.new(api_key: connected_app_params[:vine_api_key], + jwt_generator: jwt_service) + + if !@app.connect(api_key: connected_app_params[:vine_api_key], vine_api:) + error_message = "#{@app.errors.full_messages.to_sentence}. \ + #{I18n.t('admin.enterprises.form.connected_apps.vine.api_key_error')}".squish + handle_error(error_message) + end + rescue Faraday::Error => e + log_and_notify_exception(e) + handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.connection_error")) + rescue KeyError => e + log_and_notify_exception(e) + handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.setup_error")) + end + def enterprise @enterprise ||= Enterprise.find(params.require(:enterprise_id)) end @@ -34,8 +68,18 @@ module Admin redirect_to "#{edit_admin_enterprise_path(enterprise)}#/connected_apps_panel" end + def handle_error(message) + flash[:error] = message + @app.destroy + end + + def log_and_notify_exception(exception) + Rails.logger.error exception.inspect + Bugsnag.notify(exception) + end + def connected_app_params - params.permit(:type) + params.permit(:type, :vine_api_key) end end end diff --git a/config/initializers/filter_parameter_logging.rb b/config/initializers/filter_parameter_logging.rb index e203fcee0a..42c801cbbd 100644 --- a/config/initializers/filter_parameter_logging.rb +++ b/config/initializers/filter_parameter_logging.rb @@ -1,2 +1,4 @@ +# frozen_string_literal: true + # Configure sensitive parameters which will be filtered from the log file. -Rails.application.config.filter_parameters += [:password] +Rails.application.config.filter_parameters += [:password, :vine_api_key] diff --git a/config/locales/en.yml b/config/locales/en.yml index d0136b68ad..fc1579deb8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1437,7 +1437,10 @@ en: VINE

- + api_key_empty: "Please enter an API key" + api_key_error: "Check you entered your API key correctly, contact your instance manager if the error persists" + connection_error: "API connection error, please try again" + setup_error: "VINE API is not configured, please contact your instance manager" actions: edit_profile: Settings properties: Properties diff --git a/spec/requests/admin/connected_apps_controller_spec.rb b/spec/requests/admin/connected_apps_controller_spec.rb new file mode 100644 index 0000000000..9b4caedca1 --- /dev/null +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -0,0 +1,174 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Admin::ConnectedAppsController do + let(:user) { create(:admin_user) } + let(:enterprise) { create(:enterprise, owner: user) } + let(:edit_enterprise_url) { "#{edit_admin_enterprise_url(enterprise)}#/connected_apps_panel" } + + before do + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("VINE_API_SECRET").and_return("my_secret") + + sign_in user + end + + describe "POST /admin/enterprises/:enterprise_id/connected_apps" do + context "with type ConnectedApps::Vine" do + let(:vine_api) { instance_double(VineApiService) } + + before do + allow(VineJwtService).to receive(:new).and_return(instance_double(VineJwtService)) + allow(VineApiService).to receive(:new).and_return(vine_api) + end + + it "creates a new connected app" do + allow(vine_api).to receive(:my_team).and_return(mock_api_response(true)) + + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).not_to be_nil + end + + it "redirects to enterprise edit page" do + allow(vine_api).to receive(:my_team).and_return(mock_api_response(true)) + + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + end + + context "when api key is empty" do + it "redirects to enterprise edit page, with an error" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + expect(flash[:error]).to eq("Please enter an API key") + expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil + end + end + + context "when api key is not valid" do + before do + allow(vine_api).to receive(:my_team).and_return(mock_api_response(false)) + end + + it "doesn't create a new connected app" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil + end + + it "redirects to enterprise edit page, with an error" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + expect(flash[:error]).to eq( + "An error occured when connecting to Vine API. Check you entered your API key \ + correctly, contact your instance manager if the error persists".squish + ) + end + end + + context "when there is a connection error" do + before do + allow(vine_api).to receive(:my_team).and_raise(Faraday::Error) + end + + it "redirects to enterprise edit page, with an error" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + expect(flash[:error]).to eq("API connection error, please try again") + end + + it "notifies Bugsnag" do + expect(Bugsnag).to receive(:notify) + + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + end + end + + context "when VINE API is not set up properly" do + before do + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("VINE_API_SECRET").and_raise(KeyError) + end + + it "redirects to enterprise edit page, with an error" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + expect(flash[:error]).to eq( + "VINE API is not configured, please contact your instance manager" + ) + end + + it "notifies Bugsnag" do + expect(Bugsnag).to receive(:notify) + + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + end + end + end + + describe "DELETE /admin/enterprises/:enterprise_id/connected_apps/:id" do + it "deletes the connected app" do + app = ConnectedApps::Vine.create!(enterprise:) + delete("/admin/enterprises/#{enterprise.id}/connected_apps/#{app.id}") + + expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil + end + + it "redirect to enterprise edit page" do + app = ConnectedApps::Vine.create!(enterprise:, data: { api_key: "12345" }) + delete("/admin/enterprises/#{enterprise.id}/connected_apps/#{app.id}") + + expect(response).to redirect_to(edit_enterprise_url) + end + end + end + + def mock_api_response(success) + mock_response = instance_double(Faraday::Response) + allow(mock_response).to receive(:success?).and_return(success) + mock_response + end +end From 10c3c53aad335a7b6a72cd6fecba4e0ac26626ee Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 7 Oct 2024 11:23:22 +1100 Subject: [PATCH 07/12] Fix translation per review. --- config/locales/en.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index fc1579deb8..5464660d6d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -756,7 +756,7 @@ en: connected_apps_enabled: discover_regen: Discover Regenerative portal affiliate_sales_data: DFC anonymised orders API for research purposes - vine: Voucher Engine (VINE) + vine: Voucher Integration Engine (VINE) update: resource: Connected app settings @@ -1423,8 +1423,8 @@ en:

vine: - title: "Voucher Engine (VINE)" - tagline: "Enable the use of VINE api to handle vouchers" + title: "Voucher Integration Engine (VINE)" + tagline: "Allow redemption of VINE vouchers in your shopfront." enable: "Connect" disable: "Disconnect" need_to_be_manager: "Only managers can connect apps." From 224738e0a135141732ac9579cbad67a9db5e3cbb Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 7 Oct 2024 11:29:10 +1100 Subject: [PATCH 08/12] Per review, clean up code --- app/models/connected_apps/vine.rb | 2 -- ...he_response_do.yml => returns_the_response.yml} | 14 +++++++------- spec/models/connected_apps/vine_spec.rb | 2 +- .../admin/connected_apps_controller_spec.rb | 2 +- spec/services/vine_api_service_spec.rb | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) rename spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/{returns_the_response_do.yml => returns_the_response.yml} (72%) diff --git a/app/models/connected_apps/vine.rb b/app/models/connected_apps/vine.rb index f32858d0b2..704ae5c98b 100644 --- a/app/models/connected_apps/vine.rb +++ b/app/models/connected_apps/vine.rb @@ -4,8 +4,6 @@ # module ConnectedApps class Vine < ConnectedApp - VINE_API_URL = "https://vine-staging.openfoodnetwork.org.au/api/v1/my-team" - def connect(api_key:, vine_api:, **_opts) response = vine_api.my_team diff --git a/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response_do.yml b/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response.yml similarity index 72% rename from spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response_do.yml rename to spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response.yml index 136540eb6a..99505aa934 100644 --- a/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response_do.yml +++ b/spec/fixtures/vcr_cassettes/VineApiService/_my_team/when_a_request_succeed/returns_the_response.yml @@ -9,14 +9,14 @@ http_interactions: headers: X-Authorization: - "" + Accept: + - application/json User-Agent: - Faraday v2.9.0 Authorization: - "" Accept-Encoding: - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 - Accept: - - "*/*" response: status: code: 200 @@ -35,7 +35,7 @@ http_interactions: Cache-Control: - no-cache, private Date: - - Wed, 02 Oct 2024 00:27:30 GMT + - Mon, 07 Oct 2024 04:07:27 GMT Access-Control-Allow-Origin: - "*" X-Frame-Options: @@ -46,8 +46,8 @@ http_interactions: - nosniff body: encoding: ASCII-8BIT - string: '{"meta":{"responseCode":200,"limit":50,"offset":0,"message":"","cached":true,"cached_at":"2024-10-02 - 00:27:30","availableRelations":["teamUsers.user","country"]},"data":{"id":1,"name":"OK200 - Team","country_id":14,"created_at":"2024-09-12T06:39:07.000000Z","updated_at":"2024-09-12T06:39:07.000000Z","deleted_at":null}}' - recorded_at: Wed, 02 Oct 2024 00:27:30 GMT + string: '{"meta":{"responseCode":200,"limit":50,"offset":0,"message":"","cached":true,"cached_at":"2024-10-07 + 04:07:27","availableRelations":["teamUsers.user","country"]},"data":{"id":9,"name":"Open + Food Network TEST","country_id":14,"created_at":"2024-10-05T03:39:50.000000Z","updated_at":"2024-10-05T03:39:50.000000Z","deleted_at":null}}' + recorded_at: Mon, 07 Oct 2024 04:07:27 GMT recorded_with: VCR 6.2.0 diff --git a/spec/models/connected_apps/vine_spec.rb b/spec/models/connected_apps/vine_spec.rb index 7d94d3693e..e63ae42b07 100644 --- a/spec/models/connected_apps/vine_spec.rb +++ b/spec/models/connected_apps/vine_spec.rb @@ -15,7 +15,7 @@ RSpec.describe ConnectedApps::Vine do connected_app.connect(api_key: vine_api_key, vine_api: ) end - context "when request succeed", :vcr do + context "when request succeed" do it "store the vine api key" do allow(vine_api).to receive(:my_team).and_return(mock_api_response(true)) diff --git a/spec/requests/admin/connected_apps_controller_spec.rb b/spec/requests/admin/connected_apps_controller_spec.rb index 9b4caedca1..66b00a5985 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe Admin::ConnectedAppsController do +RSpec.describe "Admin ConnectedApp" do let(:user) { create(:admin_user) } let(:enterprise) { create(:enterprise, owner: user) } let(:edit_enterprise_url) { "#{edit_admin_enterprise_url(enterprise)}#/connected_apps_panel" } diff --git a/spec/services/vine_api_service_spec.rb b/spec/services/vine_api_service_spec.rb index c0cb2953a4..0614931719 100644 --- a/spec/services/vine_api_service_spec.rb +++ b/spec/services/vine_api_service_spec.rb @@ -52,7 +52,7 @@ RSpec.describe VineApiService do end context "when a request succeed", :vcr do - it "returns the response do" do + it "returns the response" do response = vine_api.my_team expect(response.success?).to be(true) From b14a1e72f33350855e8b314fa0dfc4e29b22639f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 7 Oct 2024 13:52:27 +1100 Subject: [PATCH 09/12] Handle api secret The VINE Api require a secret and an API key to be used. The secret is used to sign the request. The secret is linked to the API key so we need to store it along side the key. --- .env | 4 - .../admin/connected_apps_controller.rb | 21 +++-- app/models/connected_apps/vine.rb | 4 +- .../form/connected_apps/_vine.html.haml | 3 + .../initializers/filter_parameter_logging.rb | 2 +- config/locales/en.yml | 8 +- spec/models/connected_apps/vine_spec.rb | 11 +-- .../admin/connected_apps_controller_spec.rb | 85 +++++++++---------- 8 files changed, 68 insertions(+), 70 deletions(-) diff --git a/.env b/.env index b5bfa57c44..fe9b06d4ff 100644 --- a/.env +++ b/.env @@ -61,7 +61,3 @@ SMTP_PASSWORD="f00d" # NEW_RELIC_AGENT_ENABLED=true # NEW_RELIC_APP_NAME="Open Food Network" # NEW_RELIC_LICENSE_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" - -# VINE API settings -# VINE_API_URL="https://vine-staging.openfoodnetwork.org.au/api/v1" -# VINE_API_SECRET="XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" diff --git a/app/controllers/admin/connected_apps_controller.rb b/app/controllers/admin/connected_apps_controller.rb index 6cf7ff34ed..ec4d259945 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -37,27 +37,26 @@ module Admin end def connect_vine - if connected_app_params[:vine_api_key].empty? - return flash[:error] = I18n.t("admin.enterprises.form.connected_apps.vine.api_key_empty") + if vine_params_empty? + return flash[:error] = + I18n.t("admin.enterprises.form.connected_apps.vine.api_parameters_empty") end create_connected_app - jwt_service = VineJwtService.new(secret: ENV.fetch("VINE_API_SECRET")) + jwt_service = VineJwtService.new(secret: connected_app_params[:vine_secret]) vine_api = VineApiService.new(api_key: connected_app_params[:vine_api_key], jwt_generator: jwt_service) - if !@app.connect(api_key: connected_app_params[:vine_api_key], vine_api:) + if !@app.connect(api_key: connected_app_params[:vine_api_key], + secret: connected_app_params[:vine_secret], vine_api:) error_message = "#{@app.errors.full_messages.to_sentence}. \ - #{I18n.t('admin.enterprises.form.connected_apps.vine.api_key_error')}".squish + #{I18n.t('admin.enterprises.form.connected_apps.vine.api_parameters_error')}".squish handle_error(error_message) end rescue Faraday::Error => e log_and_notify_exception(e) handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.connection_error")) - rescue KeyError => e - log_and_notify_exception(e) - handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.setup_error")) end def enterprise @@ -78,8 +77,12 @@ module Admin Bugsnag.notify(exception) end + def vine_params_empty? + connected_app_params[:vine_api_key].empty? || connected_app_params[:vine_secret].empty? + end + def connected_app_params - params.permit(:type, :vine_api_key) + params.permit(:type, :vine_api_key, :vine_secret) end end end diff --git a/app/models/connected_apps/vine.rb b/app/models/connected_apps/vine.rb index 704ae5c98b..3e83ac5e64 100644 --- a/app/models/connected_apps/vine.rb +++ b/app/models/connected_apps/vine.rb @@ -4,10 +4,10 @@ # module ConnectedApps class Vine < ConnectedApp - def connect(api_key:, vine_api:, **_opts) + def connect(api_key:, secret:, vine_api:, **_opts) response = vine_api.my_team - return update data: { api_key: } if response.success? + return update data: { api_key:, secret: } if response.success? errors.add(:base, I18n.t("activerecord.errors.models.connected_apps.vine.api_request_error")) diff --git a/app/views/admin/enterprises/form/connected_apps/_vine.html.haml b/app/views/admin/enterprises/form/connected_apps/_vine.html.haml index 1281a73e84..1413e28676 100644 --- a/app/views/admin/enterprises/form/connected_apps/_vine.html.haml +++ b/app/views/admin/enterprises/form/connected_apps/_vine.html.haml @@ -12,6 +12,9 @@ = f.label :vine_api_key, t(".vine_api_key") %span.required * = f.text_field :vine_api_key, { disabled: !managed_by_user?(enterprise) } + = f.label :vine_secret, t(".vine_secret") + %span.required * + = f.text_field :vine_secret, { disabled: !managed_by_user?(enterprise) } %div - disabled = managed_by_user?(enterprise) ? {} : { disabled: true, "data-disable-with": false } = f.submit t(".enable"), disabled diff --git a/config/initializers/filter_parameter_logging.rb b/config/initializers/filter_parameter_logging.rb index 42c801cbbd..b2319d12f3 100644 --- a/config/initializers/filter_parameter_logging.rb +++ b/config/initializers/filter_parameter_logging.rb @@ -1,4 +1,4 @@ # frozen_string_literal: true # Configure sensitive parameters which will be filtered from the log file. -Rails.application.config.filter_parameters += [:password, :vine_api_key] +Rails.application.config.filter_parameters += [:password, :vine_api_key, :vine_secret] diff --git a/config/locales/en.yml b/config/locales/en.yml index 5464660d6d..a193f820c8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1429,18 +1429,18 @@ en: disable: "Disconnect" need_to_be_manager: "Only managers can connect apps." vine_api_key: "VINE API Key" + vine_secret: "VINE secret" description_html: |

- To enable VINE for your enterprise, enter your API key. + To enable VINE for your enterprise, enter your API key and secret.

VINE

- api_key_empty: "Please enter an API key" - api_key_error: "Check you entered your API key correctly, contact your instance manager if the error persists" + api_parameters_empty: "Please enter an API key and a secret" + api_parameters_error: "Check you entered your API key and secret correctly, contact your instance manager if the error persists" connection_error: "API connection error, please try again" - setup_error: "VINE API is not configured, please contact your instance manager" actions: edit_profile: Settings properties: Properties diff --git a/spec/models/connected_apps/vine_spec.rb b/spec/models/connected_apps/vine_spec.rb index e63ae42b07..8b37afb0f5 100644 --- a/spec/models/connected_apps/vine_spec.rb +++ b/spec/models/connected_apps/vine_spec.rb @@ -6,21 +6,22 @@ RSpec.describe ConnectedApps::Vine do subject(:connected_app) { ConnectedApps::Vine.new(enterprise: create(:enterprise)) } let(:vine_api_key) { "12345" } + let(:secret) { "my_secret" } let(:vine_api) { instance_double(VineApiService) } describe "#connect" do it "send a request to VINE api" do expect(vine_api).to receive(:my_team).and_return(mock_api_response(true)) - connected_app.connect(api_key: vine_api_key, vine_api: ) + connected_app.connect(api_key: vine_api_key, secret:, vine_api: ) end context "when request succeed" do - it "store the vine api key" do + it "store the vine api key and secret" do allow(vine_api).to receive(:my_team).and_return(mock_api_response(true)) - expect(connected_app.connect(api_key: vine_api_key, vine_api:)).to eq(true) - expect(connected_app.data).to eql({ "api_key" => vine_api_key }) + expect(connected_app.connect(api_key: vine_api_key, secret:, vine_api:)).to eq(true) + expect(connected_app.data).to eql({ "api_key" => vine_api_key, "secret" => secret }) end end @@ -28,7 +29,7 @@ RSpec.describe ConnectedApps::Vine do it "doesn't store the vine api key" do allow(vine_api).to receive(:my_team).and_return(mock_api_response(false)) - expect(connected_app.connect(api_key: vine_api_key, vine_api:)).to eq(false) + expect(connected_app.connect(api_key: vine_api_key, secret:, vine_api:)).to eq(false) expect(connected_app.data).to be_nil expect(connected_app.errors[:base]).to include( "An error occured when connecting to Vine API" diff --git a/spec/requests/admin/connected_apps_controller_spec.rb b/spec/requests/admin/connected_apps_controller_spec.rb index 66b00a5985..3b9879b8c6 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -8,9 +8,6 @@ RSpec.describe "Admin ConnectedApp" do let(:edit_enterprise_url) { "#{edit_admin_enterprise_url(enterprise)}#/connected_apps_panel" } before do - allow(ENV).to receive(:fetch).and_call_original - allow(ENV).to receive(:fetch).with("VINE_API_SECRET").and_return("my_secret") - sign_in user end @@ -28,11 +25,15 @@ RSpec.describe "Admin ConnectedApp" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) - expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).not_to be_nil + vine_app = ConnectedApps::Vine.find_by(enterprise_id: enterprise.id) + expect(vine_app).not_to be_nil + expect(vine_app.data["api_key"]).to eq("12345678") + expect(vine_app.data["secret"]).to eq("my_secret") end it "redirects to enterprise edit page" do @@ -40,7 +41,8 @@ RSpec.describe "Admin ConnectedApp" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) @@ -51,17 +53,33 @@ RSpec.describe "Admin ConnectedApp" do it "redirects to enterprise edit page, with an error" do params = { type: ConnectedApps::Vine, - vine_api_key: "" + vine_api_key: "", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) expect(response).to redirect_to(edit_enterprise_url) - expect(flash[:error]).to eq("Please enter an API key") + expect(flash[:error]).to eq("Please enter an API key and a secret") expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil end end - context "when api key is not valid" do + context "when api secret is empty" do + it "redirects to enterprise edit page, with an error" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678", + vine_secret: "" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + expect(flash[:error]).to eq("Please enter an API key and a secret") + expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil + end + end + + context "when api key or secret is not valid" do before do allow(vine_api).to receive(:my_team).and_return(mock_api_response(false)) end @@ -69,7 +87,8 @@ RSpec.describe "Admin ConnectedApp" do it "doesn't create a new connected app" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) @@ -79,14 +98,15 @@ RSpec.describe "Admin ConnectedApp" do it "redirects to enterprise edit page, with an error" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) expect(response).to redirect_to(edit_enterprise_url) expect(flash[:error]).to eq( "An error occured when connecting to Vine API. Check you entered your API key \ - correctly, contact your instance manager if the error persists".squish + and secret correctly, contact your instance manager if the error persists".squish ) end end @@ -99,7 +119,8 @@ RSpec.describe "Admin ConnectedApp" do it "redirects to enterprise edit page, with an error" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) @@ -112,37 +133,8 @@ RSpec.describe "Admin ConnectedApp" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" - } - post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) - end - end - - context "when VINE API is not set up properly" do - before do - allow(ENV).to receive(:fetch).and_call_original - allow(ENV).to receive(:fetch).with("VINE_API_SECRET").and_raise(KeyError) - end - - it "redirects to enterprise edit page, with an error" do - params = { - type: ConnectedApps::Vine, - vine_api_key: "12345678" - } - post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) - - expect(response).to redirect_to(edit_enterprise_url) - expect(flash[:error]).to eq( - "VINE API is not configured, please contact your instance manager" - ) - end - - it "notifies Bugsnag" do - expect(Bugsnag).to receive(:notify) - - params = { - type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) end @@ -158,7 +150,10 @@ RSpec.describe "Admin ConnectedApp" do end it "redirect to enterprise edit page" do - app = ConnectedApps::Vine.create!(enterprise:, data: { api_key: "12345" }) + app = ConnectedApps::Vine.create!(enterprise:, + data: { + api_key: "12345", secret: "my_secret" + }) delete("/admin/enterprises/#{enterprise.id}/connected_apps/#{app.id}") expect(response).to redirect_to(edit_enterprise_url) From a3d8ae693d5d65889eb799ef73583d9d74ea9029 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 7 Oct 2024 14:53:35 +1100 Subject: [PATCH 10/12] Add encryption for ConnectedApps::Vine#data Added layer of security, we encrypt the API key and related secret. It requires setting up some encryption keys that can be generated wiht `bin/rails db:encryption:init` --- .env | 6 ++++++ .env.development | 5 +++++ .env.test | 4 ++++ app/models/connected_apps/vine.rb | 2 ++ config/application.rb | 11 +++++++++++ 5 files changed, 28 insertions(+) diff --git a/.env b/.env index fe9b06d4ff..14902e28e9 100644 --- a/.env +++ b/.env @@ -61,3 +61,9 @@ SMTP_PASSWORD="f00d" # NEW_RELIC_AGENT_ENABLED=true # NEW_RELIC_APP_NAME="Open Food Network" # NEW_RELIC_LICENSE_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + +# Database encryption configuration, required for VINE connected app +# Generate with bin/rails db:encryption:init +# ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" +# ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" +# ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" diff --git a/.env.development b/.env.development index 68640acf21..94f1750d52 100644 --- a/.env.development +++ b/.env.development @@ -24,3 +24,8 @@ SITE_URL="0.0.0.0:3000" RACK_TIMEOUT_SERVICE_TIMEOUT="0" RACK_TIMEOUT_WAIT_TIMEOUT="0" RACK_TIMEOUT_WAIT_OVERTIME="0" + +# Database encryption configuration, required for VINE connected app +ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY="dev_primary_key" +ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY="dev_determinnistic_key" +ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT="dev_derivation_salt" diff --git a/.env.test b/.env.test index c0097a0416..d65627ce33 100644 --- a/.env.test +++ b/.env.test @@ -18,3 +18,7 @@ SITE_URL="test.host" OPENID_APP_ID="test-provider" OPENID_APP_SECRET="12345" OPENID_REFRESH_TOKEN="dummy-refresh-token" + +ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY="test_primary_key" +ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY="test_deterministic_key" +ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT="test_derivation_salt" diff --git a/app/models/connected_apps/vine.rb b/app/models/connected_apps/vine.rb index 3e83ac5e64..350d8ac6ba 100644 --- a/app/models/connected_apps/vine.rb +++ b/app/models/connected_apps/vine.rb @@ -4,6 +4,8 @@ # module ConnectedApps class Vine < ConnectedApp + encrypts :data + def connect(api_key:, secret:, vine_api:, **_opts) response = vine_api.my_team diff --git a/config/application.rb b/config/application.rb index a1bda0a7d5..19cd559d97 100644 --- a/config/application.rb +++ b/config/application.rb @@ -255,5 +255,16 @@ module Openfoodnetwork config.exceptions_app = self.routes config.view_component.generate.sidecar = true # Always generate components in subfolders + + # Database encryption configuration, required for VINE connected app + config.active_record.encryption.primary_key = ENV.fetch( + "ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY", nil + ) + config.active_record.encryption.deterministic_key = ENV.fetch( + "ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY", nil + ) + config.active_record.encryption.key_derivation_salt = ENV.fetch( + "ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT", nil + ) end end From df67b53971a33eb89e36b66cf411258acfef52c6 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 8 Oct 2024 13:26:57 +1100 Subject: [PATCH 11/12] Re add VINE_API_URL env variable And add error handling if the variable is not set --- .env | 3 ++ .../admin/connected_apps_controller.rb | 3 ++ config/locales/en.yml | 1 + .../admin/connected_apps_controller_spec.rb | 32 +++++++++++++++++++ 4 files changed, 39 insertions(+) diff --git a/.env b/.env index 14902e28e9..3dc380b571 100644 --- a/.env +++ b/.env @@ -67,3 +67,6 @@ SMTP_PASSWORD="f00d" # ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" # ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" # ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + +# VINE API settings +# VINE_API_URL="https://vine-staging.openfoodnetwork.org.au/api/v1" diff --git a/app/controllers/admin/connected_apps_controller.rb b/app/controllers/admin/connected_apps_controller.rb index ec4d259945..a5308d4984 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -57,6 +57,9 @@ module Admin rescue Faraday::Error => e log_and_notify_exception(e) handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.connection_error")) + rescue KeyError => e + log_and_notify_exception(e) + handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.setup_error")) end def enterprise diff --git a/config/locales/en.yml b/config/locales/en.yml index a193f820c8..a9354eda19 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1441,6 +1441,7 @@ en: api_parameters_empty: "Please enter an API key and a secret" api_parameters_error: "Check you entered your API key and secret correctly, contact your instance manager if the error persists" connection_error: "API connection error, please try again" + setup_error: "VINE API is not configured, please contact your instance manager" actions: edit_profile: Settings properties: Properties diff --git a/spec/requests/admin/connected_apps_controller_spec.rb b/spec/requests/admin/connected_apps_controller_spec.rb index 3b9879b8c6..f1330ae7e2 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -111,6 +111,38 @@ RSpec.describe "Admin ConnectedApp" do end end + context "when VINE API is not set up properly" do + before do + # VineApiService will raise a KeyError if VINE_API_URL is not set + allow(VineApiService).to receive(:new).and_raise(KeyError) + end + + it "redirects to enterprise edit page, with an error" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678", + vine_secret: "my_secret" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + expect(flash[:error]).to eq( + "VINE API is not configured, please contact your instance manager" + ) + end + + it "notifies Bugsnag" do + expect(Bugsnag).to receive(:notify) + + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678", + vine_secret: "my_secret" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + end + end + context "when there is a connection error" do before do allow(vine_api).to receive(:my_team).and_raise(Faraday::Error) From 08308ba08e2638873b6a6910f1e30700f6b2ca31 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 8 Oct 2024 16:15:35 +1100 Subject: [PATCH 12/12] Fix spec checking if VINE api is set up The condition for checking the error now match a real scenario --- spec/requests/admin/connected_apps_controller_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/requests/admin/connected_apps_controller_spec.rb b/spec/requests/admin/connected_apps_controller_spec.rb index f1330ae7e2..05b7eefa0c 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -113,8 +113,9 @@ RSpec.describe "Admin ConnectedApp" do context "when VINE API is not set up properly" do before do - # VineApiService will raise a KeyError if VINE_API_URL is not set - allow(VineApiService).to receive(:new).and_raise(KeyError) + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("VINE_API_URL").and_raise(KeyError) + allow(VineApiService).to receive(:new).and_call_original end it "redirects to enterprise edit page, with an error" do