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