From b14a1e72f33350855e8b314fa0dfc4e29b22639f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 7 Oct 2024 13:52:27 +1100 Subject: [PATCH] Handle api secret The VINE Api require a secret and an API key to be used. The secret is used to sign the request. The secret is linked to the API key so we need to store it along side the key. --- .env | 4 - .../admin/connected_apps_controller.rb | 21 +++-- app/models/connected_apps/vine.rb | 4 +- .../form/connected_apps/_vine.html.haml | 3 + .../initializers/filter_parameter_logging.rb | 2 +- config/locales/en.yml | 8 +- spec/models/connected_apps/vine_spec.rb | 11 +-- .../admin/connected_apps_controller_spec.rb | 85 +++++++++---------- 8 files changed, 68 insertions(+), 70 deletions(-) diff --git a/.env b/.env index b5bfa57c44..fe9b06d4ff 100644 --- a/.env +++ b/.env @@ -61,7 +61,3 @@ SMTP_PASSWORD="f00d" # NEW_RELIC_AGENT_ENABLED=true # NEW_RELIC_APP_NAME="Open Food Network" # NEW_RELIC_LICENSE_KEY="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" - -# VINE API settings -# VINE_API_URL="https://vine-staging.openfoodnetwork.org.au/api/v1" -# VINE_API_SECRET="XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" diff --git a/app/controllers/admin/connected_apps_controller.rb b/app/controllers/admin/connected_apps_controller.rb index 6cf7ff34ed..ec4d259945 100644 --- a/app/controllers/admin/connected_apps_controller.rb +++ b/app/controllers/admin/connected_apps_controller.rb @@ -37,27 +37,26 @@ module Admin end def connect_vine - if connected_app_params[:vine_api_key].empty? - return flash[:error] = I18n.t("admin.enterprises.form.connected_apps.vine.api_key_empty") + if vine_params_empty? + return flash[:error] = + I18n.t("admin.enterprises.form.connected_apps.vine.api_parameters_empty") end create_connected_app - jwt_service = VineJwtService.new(secret: ENV.fetch("VINE_API_SECRET")) + jwt_service = VineJwtService.new(secret: connected_app_params[:vine_secret]) vine_api = VineApiService.new(api_key: connected_app_params[:vine_api_key], jwt_generator: jwt_service) - if !@app.connect(api_key: connected_app_params[:vine_api_key], vine_api:) + if !@app.connect(api_key: connected_app_params[:vine_api_key], + secret: connected_app_params[:vine_secret], vine_api:) error_message = "#{@app.errors.full_messages.to_sentence}. \ - #{I18n.t('admin.enterprises.form.connected_apps.vine.api_key_error')}".squish + #{I18n.t('admin.enterprises.form.connected_apps.vine.api_parameters_error')}".squish handle_error(error_message) end rescue Faraday::Error => e log_and_notify_exception(e) handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.connection_error")) - rescue KeyError => e - log_and_notify_exception(e) - handle_error(I18n.t("admin.enterprises.form.connected_apps.vine.setup_error")) end def enterprise @@ -78,8 +77,12 @@ module Admin Bugsnag.notify(exception) end + def vine_params_empty? + connected_app_params[:vine_api_key].empty? || connected_app_params[:vine_secret].empty? + end + def connected_app_params - params.permit(:type, :vine_api_key) + params.permit(:type, :vine_api_key, :vine_secret) end end end diff --git a/app/models/connected_apps/vine.rb b/app/models/connected_apps/vine.rb index 704ae5c98b..3e83ac5e64 100644 --- a/app/models/connected_apps/vine.rb +++ b/app/models/connected_apps/vine.rb @@ -4,10 +4,10 @@ # module ConnectedApps class Vine < ConnectedApp - def connect(api_key:, vine_api:, **_opts) + def connect(api_key:, secret:, vine_api:, **_opts) response = vine_api.my_team - return update data: { api_key: } if response.success? + return update data: { api_key:, secret: } if response.success? errors.add(:base, I18n.t("activerecord.errors.models.connected_apps.vine.api_request_error")) diff --git a/app/views/admin/enterprises/form/connected_apps/_vine.html.haml b/app/views/admin/enterprises/form/connected_apps/_vine.html.haml index 1281a73e84..1413e28676 100644 --- a/app/views/admin/enterprises/form/connected_apps/_vine.html.haml +++ b/app/views/admin/enterprises/form/connected_apps/_vine.html.haml @@ -12,6 +12,9 @@ = f.label :vine_api_key, t(".vine_api_key") %span.required * = f.text_field :vine_api_key, { disabled: !managed_by_user?(enterprise) } + = f.label :vine_secret, t(".vine_secret") + %span.required * + = f.text_field :vine_secret, { disabled: !managed_by_user?(enterprise) } %div - disabled = managed_by_user?(enterprise) ? {} : { disabled: true, "data-disable-with": false } = f.submit t(".enable"), disabled diff --git a/config/initializers/filter_parameter_logging.rb b/config/initializers/filter_parameter_logging.rb index 42c801cbbd..b2319d12f3 100644 --- a/config/initializers/filter_parameter_logging.rb +++ b/config/initializers/filter_parameter_logging.rb @@ -1,4 +1,4 @@ # frozen_string_literal: true # Configure sensitive parameters which will be filtered from the log file. -Rails.application.config.filter_parameters += [:password, :vine_api_key] +Rails.application.config.filter_parameters += [:password, :vine_api_key, :vine_secret] diff --git a/config/locales/en.yml b/config/locales/en.yml index 5464660d6d..a193f820c8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1429,18 +1429,18 @@ en: disable: "Disconnect" need_to_be_manager: "Only managers can connect apps." vine_api_key: "VINE API Key" + vine_secret: "VINE secret" description_html: |

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

VINE

- api_key_empty: "Please enter an API key" - api_key_error: "Check you entered your API key correctly, contact your instance manager if the error persists" + api_parameters_empty: "Please enter an API key and a secret" + api_parameters_error: "Check you entered your API key and secret correctly, contact your instance manager if the error persists" connection_error: "API connection error, please try again" - setup_error: "VINE API is not configured, please contact your instance manager" actions: edit_profile: Settings properties: Properties diff --git a/spec/models/connected_apps/vine_spec.rb b/spec/models/connected_apps/vine_spec.rb index e63ae42b07..8b37afb0f5 100644 --- a/spec/models/connected_apps/vine_spec.rb +++ b/spec/models/connected_apps/vine_spec.rb @@ -6,21 +6,22 @@ RSpec.describe ConnectedApps::Vine do subject(:connected_app) { ConnectedApps::Vine.new(enterprise: create(:enterprise)) } let(:vine_api_key) { "12345" } + let(:secret) { "my_secret" } let(:vine_api) { instance_double(VineApiService) } describe "#connect" do it "send a request to VINE api" do expect(vine_api).to receive(:my_team).and_return(mock_api_response(true)) - connected_app.connect(api_key: vine_api_key, vine_api: ) + connected_app.connect(api_key: vine_api_key, secret:, vine_api: ) end context "when request succeed" do - it "store the vine api key" do + it "store the vine api key and secret" do allow(vine_api).to receive(:my_team).and_return(mock_api_response(true)) - expect(connected_app.connect(api_key: vine_api_key, vine_api:)).to eq(true) - expect(connected_app.data).to eql({ "api_key" => vine_api_key }) + expect(connected_app.connect(api_key: vine_api_key, secret:, vine_api:)).to eq(true) + expect(connected_app.data).to eql({ "api_key" => vine_api_key, "secret" => secret }) end end @@ -28,7 +29,7 @@ RSpec.describe ConnectedApps::Vine do it "doesn't store the vine api key" do allow(vine_api).to receive(:my_team).and_return(mock_api_response(false)) - expect(connected_app.connect(api_key: vine_api_key, vine_api:)).to eq(false) + expect(connected_app.connect(api_key: vine_api_key, secret:, vine_api:)).to eq(false) expect(connected_app.data).to be_nil expect(connected_app.errors[:base]).to include( "An error occured when connecting to Vine API" diff --git a/spec/requests/admin/connected_apps_controller_spec.rb b/spec/requests/admin/connected_apps_controller_spec.rb index 66b00a5985..3b9879b8c6 100644 --- a/spec/requests/admin/connected_apps_controller_spec.rb +++ b/spec/requests/admin/connected_apps_controller_spec.rb @@ -8,9 +8,6 @@ RSpec.describe "Admin ConnectedApp" do let(:edit_enterprise_url) { "#{edit_admin_enterprise_url(enterprise)}#/connected_apps_panel" } before do - allow(ENV).to receive(:fetch).and_call_original - allow(ENV).to receive(:fetch).with("VINE_API_SECRET").and_return("my_secret") - sign_in user end @@ -28,11 +25,15 @@ RSpec.describe "Admin ConnectedApp" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) - expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).not_to be_nil + vine_app = ConnectedApps::Vine.find_by(enterprise_id: enterprise.id) + expect(vine_app).not_to be_nil + expect(vine_app.data["api_key"]).to eq("12345678") + expect(vine_app.data["secret"]).to eq("my_secret") end it "redirects to enterprise edit page" do @@ -40,7 +41,8 @@ RSpec.describe "Admin ConnectedApp" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) @@ -51,17 +53,33 @@ RSpec.describe "Admin ConnectedApp" do it "redirects to enterprise edit page, with an error" do params = { type: ConnectedApps::Vine, - vine_api_key: "" + vine_api_key: "", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) expect(response).to redirect_to(edit_enterprise_url) - expect(flash[:error]).to eq("Please enter an API key") + expect(flash[:error]).to eq("Please enter an API key and a secret") expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil end end - context "when api key is not valid" do + context "when api secret is empty" do + it "redirects to enterprise edit page, with an error" do + params = { + type: ConnectedApps::Vine, + vine_api_key: "12345678", + vine_secret: "" + } + post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) + + expect(response).to redirect_to(edit_enterprise_url) + expect(flash[:error]).to eq("Please enter an API key and a secret") + expect(ConnectedApps::Vine.find_by(enterprise_id: enterprise.id)).to be_nil + end + end + + context "when api key or secret is not valid" do before do allow(vine_api).to receive(:my_team).and_return(mock_api_response(false)) end @@ -69,7 +87,8 @@ RSpec.describe "Admin ConnectedApp" do it "doesn't create a new connected app" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) @@ -79,14 +98,15 @@ RSpec.describe "Admin ConnectedApp" do it "redirects to enterprise edit page, with an error" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) expect(response).to redirect_to(edit_enterprise_url) expect(flash[:error]).to eq( "An error occured when connecting to Vine API. Check you entered your API key \ - correctly, contact your instance manager if the error persists".squish + and secret correctly, contact your instance manager if the error persists".squish ) end end @@ -99,7 +119,8 @@ RSpec.describe "Admin ConnectedApp" do it "redirects to enterprise edit page, with an error" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) @@ -112,37 +133,8 @@ RSpec.describe "Admin ConnectedApp" do params = { type: ConnectedApps::Vine, - vine_api_key: "12345678" - } - post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) - end - end - - context "when VINE API is not set up properly" do - before do - allow(ENV).to receive(:fetch).and_call_original - allow(ENV).to receive(:fetch).with("VINE_API_SECRET").and_raise(KeyError) - end - - it "redirects to enterprise edit page, with an error" do - params = { - type: ConnectedApps::Vine, - vine_api_key: "12345678" - } - post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) - - expect(response).to redirect_to(edit_enterprise_url) - expect(flash[:error]).to eq( - "VINE API is not configured, please contact your instance manager" - ) - end - - it "notifies Bugsnag" do - expect(Bugsnag).to receive(:notify) - - params = { - type: ConnectedApps::Vine, - vine_api_key: "12345678" + vine_api_key: "12345678", + vine_secret: "my_secret" } post("/admin/enterprises/#{enterprise.id}/connected_apps", params: ) end @@ -158,7 +150,10 @@ RSpec.describe "Admin ConnectedApp" do end it "redirect to enterprise edit page" do - app = ConnectedApps::Vine.create!(enterprise:, data: { api_key: "12345" }) + app = ConnectedApps::Vine.create!(enterprise:, + data: { + api_key: "12345", secret: "my_secret" + }) delete("/admin/enterprises/#{enterprise.id}/connected_apps/#{app.id}") expect(response).to redirect_to(edit_enterprise_url)