From b7411f7d72529f8a016fa243200236680513b02d Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Wed, 4 Sep 2024 09:32:36 -0600 Subject: [PATCH] Fix Missing Manifest User Causing Sensitivity Check Failures (#1675) * Fix User Missing for BGS Sensitivity Check - Update the manifests_controller's refresh method to use the find_or_create_by_user method to find a manifest. This will ensure that the user is set correctly for BGS calls. - Move SensitivityLevelCheckFailure logic to the base_controller. * Fix Misc. Issues - Move rescue_from for BGS errors into the API V1 controller so its existing standard error rescue doesn't catch this exception. - Improve manifests_controller request spec with sensitivity check logic. --- .../api/v1/application_controller.rb | 9 +++ .../api/v2/manifests_controller.rb | 21 ++++--- app/controllers/base_controller.rb | 5 ++ app/models/manifest.rb | 2 +- app/services/sensitivity_checker.rb | 4 +- spec/requests/api/v2/manifests_spec.rb | 58 +++++++++++++++++++ 6 files changed, 88 insertions(+), 11 deletions(-) diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index 66385a934..726847927 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -13,6 +13,15 @@ class Api::V1::ApplicationController < BaseController }, status: 500 end + rescue_from BGS::SensitivityLevelCheckFailure do |e| + render json: { + status: e.message, + featureToggles: { + checkUserSensitivity: FeatureToggle.enabled?(:check_user_sensitivity) + } + }, status: :forbidden + end + rescue_from BGS::PublicError do |error| forbidden(error.public_message) end diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index ae70f0925..905ee47cb 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -1,24 +1,25 @@ +# frozen_string_literal: true + class Api::V2::ManifestsController < Api::V2::ApplicationController + # Need this as a before action since it gates access to these controller methods + before_action :veteran_file_number, only: [:start, :refresh] + def start - file_number = verify_veteran_file_number return if performed? - manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: file_number) + manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) manifest.start! render json: json_manifests(manifest) - rescue BGS::SensitivityLevelCheckFailure - forbidden("This user does not have permission to access this information") end def refresh - manifest = Manifest.find(params[:id]) - return record_not_found unless manifest + manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) + + return record_not_found if manifest.blank? return sensitive_record unless manifest.files_downloads.find_by(user: current_user) manifest.start! render json: json_manifests(manifest) - rescue BGS::SensitivityLevelCheckFailure - forbidden("This user does not have permission to access this information") end def progress @@ -37,6 +38,10 @@ def history private + def veteran_file_number + @veteran_file_number ||= verify_veteran_file_number + end + def json_manifests(manifest) ActiveModelSerializers::SerializableResource.new( manifest, diff --git a/app/controllers/base_controller.rb b/app/controllers/base_controller.rb index 5fbd3ee59..b4850af82 100644 --- a/app/controllers/base_controller.rb +++ b/app/controllers/base_controller.rb @@ -1,3 +1,8 @@ +# frozen_string_literal: true + +require "bgs" +require "bgs_errors" + class BaseController < ActionController::Base before_action :strict_transport_security before_action :current_user diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 8b974447c..ec715b6e8 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -38,7 +38,7 @@ def start! ) vbms_source.start! else - raise BGS::SensitivityLevelCheckFailure.new, "Unauthorized" + raise BGS::SensitivityLevelCheckFailure.new, "You are not authorized to access this manifest" end else vbms_source.start! diff --git a/app/services/sensitivity_checker.rb b/app/services/sensitivity_checker.rb index 8a7e33b17..cd7b5e8f1 100644 --- a/app/services/sensitivity_checker.rb +++ b/app/services/sensitivity_checker.rb @@ -4,8 +4,8 @@ class SensitivityChecker def sensitivity_levels_compatible?(user:, veteran_file_number:) bgs_service.sensitivity_level_for_user(user) >= bgs_service.sensitivity_level_for_veteran(veteran_file_number) - rescue StandardError => error - ExceptionLogger.capture(error) + rescue StandardError => e + ExceptionLogger.capture(e) false end diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index a4f197bb6..795950823 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -33,6 +33,64 @@ Timecop.freeze(Time.utc(2015, 1, 1, 17, 0, 0)) end + context "With sensitivity check failures" do + let(:mock_sensitivity_checker) { instance_double(SensitivityChecker) } + + before do + allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) + + FeatureToggle.enable!(:check_user_sensitivity) + FeatureToggle.enable!(:skip_vva) + end + + after do + FeatureToggle.disable!(:check_user_sensitivity) + FeatureToggle.disable!(:skip_vva) + end + + context "when the check succeeds" do + it "allows access to the start action" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "DEMO987").and_return(true) + + post "/api/v2/manifests", params: nil, headers: headers + + expect(response.code).to eq("200") + end + + it "allows access to the refresh action" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "DEMO987").and_return(true) + + post "/api/v2/manifests/#{manifest.id}", params: nil, headers: headers + + expect(response.code).to eq("200") + end + end + + context "when the check fails" do + it "gates access to the start action" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "DEMO987").and_return(false) + + post "/api/v2/manifests", params: nil, headers: headers + + expect(response.code).to eq("403") + expect(JSON.parse(response.body)["status"]).to eq("You are not authorized to access this manifest") + end + + it "gates access to the refresh action" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "DEMO987").and_return(false) + + post "/api/v2/manifests/#{manifest.id}", params: nil, headers: headers + + expect(response.code).to eq("403") + expect(JSON.parse(response.body)["status"]).to eq("You are not authorized to access this manifest") + end + end + end + context "View download history" do let(:manifest1) { Manifest.find_or_create_by!(file_number: "123C") } let(:manifest2) { Manifest.find_or_create_by!(file_number: "567C") }