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 966df543b5f..faf1b0abe78 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 00000000000..46fad0f1c1b --- /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 00000000000..1281a73e847 --- /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 be3356009d7..f68d902ad03 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 f1041c92bbb..da1bbe8043a 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 00000000000..f65f04a7f60 --- /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 00000000000..204dede96c9 --- /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 bf2574a0b35..d9958751a67 100644 --- a/spec/support/vcr_setup.rb +++ b/spec/support/vcr_setup.rb @@ -45,4 +45,7 @@ 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 00000000000..8fa34ce48a9 --- /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 00000000000..136540eb6af --- /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 00000000000..c0cb2953a49 --- /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 46fad0f1c1b..f32858d0b29 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 - end + 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? - def disconnect - # TODO + errors.add(:base, I18n.t("activerecord.errors.models.connected_apps.vine.api_request_error")) + + false end + + def disconnect; end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index da1bbe8043a..d0136b68adc 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 00000000000..7d94d3693ea --- /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 fe9b06d4ff1..b5bfa57c44c 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 3531c7371ab..6cf7ff34ede 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -5,12 +5,7 @@ class ConnectedAppsController < ApplicationController 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 @@ def destroy 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 @@ def render_panel 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 e203fcee0a9..42c801cbbd9 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 d0136b68adc..fc1579deb83 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 00000000000..9b4caedca1c --- /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 fc1579deb83..5464660d6d4 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 f32858d0b29..704ae5c98b8 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 136540eb6af..99505aa934a 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 7d94d3693ea..e63ae42b07f 100644 --- a/spec/models/connected_apps/vine_spec.rb +++ b/spec/models/connected_apps/vine_spec.rb @@ -15,7 +15,7 @@ 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 9b4caedca1c..66b00a5985b 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 c0cb2953a49..0614931719c 100644 --- a/spec/services/vine_api_service_spec.rb +++ b/spec/services/vine_api_service_spec.rb @@ -52,7 +52,7 @@ 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 b5bfa57c44c..fe9b06d4ff1 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 6cf7ff34ede..ec4d2599454 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -37,27 +37,26 @@ def connect 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 @@ def log_and_notify_exception(exception) 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 704ae5c98b8..3e83ac5e64f 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 1281a73e847..1413e28676f 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 42c801cbbd9..b2319d12f3c 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 5464660d6d4..a193f820c88 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 e63ae42b07f..8b37afb0f5b 100644 --- a/spec/models/connected_apps/vine_spec.rb +++ b/spec/models/connected_apps/vine_spec.rb @@ -6,21 +6,22 @@ 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 @@ 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 66b00a5985b..3b9879b8c6d 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -8,9 +8,6 @@ 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 @@ 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 @@ 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 @@ 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 and a secret") + expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil + end + end + + 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") + 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 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 fe9b06d4ff1..14902e28e9e 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 68640acf212..94f1750d52f 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 c0097a0416f..d65627ce33b 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 3e83ac5e64f..350d8ac6ba9 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 a1bda0a7d5c..19cd559d977 100644 --- a/config/application.rb +++ b/config/application.rb @@ -255,5 +255,16 @@ module ::Reporting; end 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 14902e28e9e..3dc380b571c 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 ec4d2599454..a5308d4984b 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -57,6 +57,9 @@ def connect_vine 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 a193f820c88..a9354eda191 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 3b9879b8c6d..f1330ae7e2d 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -111,6 +111,38 @@ 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 f1330ae7e2d..05b7eefa0ce 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -113,8 +113,9 @@ 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