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)