From 47085b09d4f9004296718592c65542f185fdcdc1 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 2 Aug 2024 13:05:52 -0600 Subject: [PATCH 01/36] Add Pre File Fetch Sensitivity Check (#1652) * Add Pre File Fetch Sensitivity Check - Verify user and veteran sensitivity levels are compatible. - Add specs and supporting services needed to perform sensitivity level checks. * Implement Banner for Unauthorized Vet Access * Restore Old Error Message Logic --- app/assets/stylesheets/_commons.scss | 10 +++ .../api/v2/manifests_controller.rb | 8 ++ app/exceptions/bgs_errors.rb | 9 +++ app/models/manifest.rb | 18 ++++- app/services/external_api/bgs_service.rb | 45 +++++++++++ app/services/sensitivity_checker.rb | 18 +++++ client/src/actionTypes.js | 1 + client/src/actions.jsx | 6 ++ client/src/apiActions.jsx | 9 ++- client/src/components/AlertBanner.jsx | 2 +- client/src/components/Icons.jsx | 58 +++++++++++++ client/src/containers/WelcomeContainer.jsx | 35 +++++++- client/src/reducer.js | 7 ++ lib/fakes/bgs_service.rb | 8 ++ spec/models/manifest_spec.rb | 81 +++++++++++++------ .../services/external_api/bgs_service_spec.rb | 39 +++++++++ spec/services/sensitivity_checker_spec.rb | 48 +++++++++++ 17 files changed, 371 insertions(+), 31 deletions(-) create mode 100644 app/exceptions/bgs_errors.rb create mode 100644 app/services/sensitivity_checker.rb create mode 100644 spec/services/sensitivity_checker_spec.rb diff --git a/app/assets/stylesheets/_commons.scss b/app/assets/stylesheets/_commons.scss index aaae47c18..3f7055900 100644 --- a/app/assets/stylesheets/_commons.scss +++ b/app/assets/stylesheets/_commons.scss @@ -256,6 +256,16 @@ dd { } } +.cf-icon-external-link { + vertical-align: -.45ex; + width: 1em; + height: 1em; + + path { + fill: $cf-link; + } +} + .cf-icon-close { display: block; margin: auto; diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index d7a40f3a9..544054e56 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -6,6 +6,8 @@ def start manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: file_number) manifest.start! render json: json_manifests(manifest) + rescue BGS::SensitivityLevelCheckFailure => e + sensitivity_check_failure_response end def refresh @@ -15,6 +17,8 @@ def refresh manifest.start! render json: json_manifests(manifest) + rescue BGS::SensitivityLevelCheckFailure => e + sensitivity_check_failure_response end def progress @@ -32,6 +36,10 @@ def history private + def sensitivity_check_failure_response + render json: { statusText: "This user does not have permission to access this information" }, status: :forbidden + end + def json_manifests(manifest) ActiveModelSerializers::SerializableResource.new( manifest, diff --git a/app/exceptions/bgs_errors.rb b/app/exceptions/bgs_errors.rb new file mode 100644 index 000000000..082d4637a --- /dev/null +++ b/app/exceptions/bgs_errors.rb @@ -0,0 +1,9 @@ +module BGS + class InvalidUsername < StandardError; end + class InvalidStation < StandardError; end + class InvalidApplication < StandardError; end + class NoActiveStations < StandardError; end + class NoCaseflowAccess < StandardError; end + class StationAssertionRequired < StandardError; end + class SensitivityLevelCheckFailure < StandardError; end +end diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 86f97247c..0fac038ad 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -31,7 +31,19 @@ def start! # Reset stale manifests. update!(fetched_files_status: :initialized) if ready_for_refresh? - vbms_source.start! + if FeatureToggle.enabled?(:check_user_sensitivity) + if sensitivity_checker.sensitivity_levels_compatible?( + user: user, + veteran_file_number: file_number + ) + vbms_source.start! + else + raise BGS::SensitivityLevelCheckFailure.new + end + else + vbms_source.start! + end + vva_source.start! unless FeatureToggle.enabled?(:skip_vva) end @@ -168,4 +180,8 @@ def update_veteran_info veteran_last_name: veteran.last_name || "", veteran_last_four_ssn: veteran.last_four_ssn || "") end + + def sensitivity_checker + @sensitivity_checker ||= SensitivityChecker.new + end end diff --git a/app/services/external_api/bgs_service.rb b/app/services/external_api/bgs_service.rb index e8dee9867..b52e72ac5 100644 --- a/app/services/external_api/bgs_service.rb +++ b/app/services/external_api/bgs_service.rb @@ -38,6 +38,51 @@ def initialize(client: nil) @client = client || self.class.init_client end + def sensitivity_level_for_user(user) + fail "Invalid user" if !user.instance_of?(User) + + participant_id = user.participant_id + + Rails.cache.fetch("sensitivity_level_for_user_id_#{user.id}", expires_in: 1.hour) do + MetricsService.record( + "Efolder BGS: sensitivity level for user #{user.id}", + service: :bgs, + name: "security.find_person_scrty_log_by_ptcpnt_id" + ) do + response = client.security.find_person_scrty_log_by_ptcpnt_id(participant_id) + + response.key?(:scrty_level_type_cd) ? Integer(response[:scrty_level_type_cd]) : 0 + rescue BGS::ShareError + 0 + end + end + end + + def sensitivity_level_for_veteran(veteran_file_number) + vet_info = fetch_veteran_info(veteran_file_number) + + participant_id = vet_info.present? ? vet_info[:participant_id] : nil + + fail "Invalid veteran" if participant_id.blank? + + Rails.cache.fetch("sensitivity_level_for_veteran_participant_id_#{participant_id}", expires_in: 1.hour) do + MetricsService.record( + "Efolder BGS: sensitivity level for veteran participant ID #{participant_id}", + service: :bgs, + name: "security.find_sensitivity_level_by_participant_id" + ) do + response = client.security.find_sensitivity_level_by_participant_id(participant_id) + + # guard clause for no response + return 0 if response.blank? + + response&.key?(:scrty_level_type_cd) ? Integer(response[:scrty_level_type_cd]) : 0 + rescue BGS::ShareError + 0 + end + end + end + def parse_veteran_info(veteran_data) ssn = veteran_data[:ssn] ? veteran_data[:ssn] : veteran_data[:soc_sec_number] last_four_ssn = ssn ? ssn[ssn.length - 4..ssn.length] : nil diff --git a/app/services/sensitivity_checker.rb b/app/services/sensitivity_checker.rb new file mode 100644 index 000000000..8a7e33b17 --- /dev/null +++ b/app/services/sensitivity_checker.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +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) + + false + end + + private + + def bgs_service + @bgs_service ||= BGSService.new + end +end diff --git a/client/src/actionTypes.js b/client/src/actionTypes.js index fa1e30206..4538727af 100644 --- a/client/src/actionTypes.js +++ b/client/src/actionTypes.js @@ -11,6 +11,7 @@ export const SET_ERROR_MESSAGE = 'SET_ERROR_MESSAGE'; export const SET_MANIFEST_ID = 'SET_MANIFEST_ID'; export const SET_RECENT_DOWNLOADS = 'SET_RECENT_DOWNLOADS'; export const SET_SEARCH_TEXT = 'SET_SEARCH_TEXT'; +export const SET_SHOW_UNAUTHORIZED_VETERAN_MESSAGE = 'SET_SHOW_UNAUTHORIZED_VETERAN_MESSAGE'; export const SET_VETERAN_ID = 'SET_VETERAN_ID'; export const SET_VETERAN_NAME = 'SET_VETERAN_NAME'; export const SHOW_CONFIRM_DOWNLOAD_MODAL = 'SHOW_CONFIRM_DOWNLOAD_MODAL'; diff --git a/client/src/actions.jsx b/client/src/actions.jsx index 874930b9a..75e75fd2f 100644 --- a/client/src/actions.jsx +++ b/client/src/actions.jsx @@ -12,6 +12,7 @@ import { SET_MANIFEST_ID, SET_RECENT_DOWNLOADS, SET_SEARCH_TEXT, + SET_SHOW_UNAUTHORIZED_VETERAN_MESSAGE, SET_VETERAN_ID, SET_VETERAN_NAME, SHOW_CONFIRM_DOWNLOAD_MODAL @@ -56,6 +57,11 @@ export const setDocumentSources = (sources) => ({ payload: sources }); +export const setShowUnauthorizedVeteranMessage = (showMessage) => ({ + type: SET_SHOW_UNAUTHORIZED_VETERAN_MESSAGE, + payload: showMessage +}); + export const setErrorMessage = (msg) => ({ type: SET_ERROR_MESSAGE, payload: msg diff --git a/client/src/apiActions.jsx b/client/src/apiActions.jsx index 50a1e4143..f136bc680 100644 --- a/client/src/apiActions.jsx +++ b/client/src/apiActions.jsx @@ -8,6 +8,7 @@ import { setDocumentsFetchStatus, setDocumentSources, setErrorMessage, + setShowUnauthorizedVeteranMessage, setManifestId, setRecentDownloads, setVeteranId, @@ -175,7 +176,13 @@ export const startManifestFetch = (veteranId, csrfToken, redirectFunction) => (d dispatch(setManifestId(manifestId)); redirectFunction(`/downloads/${manifestId}`); }, - (err) => dispatch(setErrorMessage(buildErrorMessageFromResponse(err.response))) + (err) => { + if (err.response.statusCode === 403) { + dispatch(setShowUnauthorizedVeteranMessage(true)); + } else { + dispatch(setErrorMessage(buildErrorMessageFromResponse(err.response))); + } + } ); }; diff --git a/client/src/components/AlertBanner.jsx b/client/src/components/AlertBanner.jsx index 2bef06677..4d7f06f00 100644 --- a/client/src/components/AlertBanner.jsx +++ b/client/src/components/AlertBanner.jsx @@ -25,7 +25,7 @@ export default class AlertBanner extends React.PureComponent { break; } - return
+ return

{this.props.title}

{this.props.children}
diff --git a/client/src/components/Icons.jsx b/client/src/components/Icons.jsx index 1e05d23d6..39a959dcb 100644 --- a/client/src/components/Icons.jsx +++ b/client/src/components/Icons.jsx @@ -148,3 +148,61 @@ export class SuccessIcon extends React.PureComponent { ); } } + +export class ExternalLinkIcon extends React.PureComponent { + render() { + return ( + + + + + + + + + + + ); + } +} diff --git a/client/src/containers/WelcomeContainer.jsx b/client/src/containers/WelcomeContainer.jsx index 793ac0cf0..c91f82013 100644 --- a/client/src/containers/WelcomeContainer.jsx +++ b/client/src/containers/WelcomeContainer.jsx @@ -10,8 +10,12 @@ import { clearErrorMessage, clearSearchInputText, setVeteranId, - setSearchInputText + setSearchInputText, + setShowUnauthorizedVeteranMessage } from '../actions'; +import { + ExternalLinkIcon +} from '../components/Icons'; import { startManifestFetch } from '../apiActions'; import AlertBanner from '../components/AlertBanner'; @@ -22,6 +26,7 @@ const searchBarNoteTextStyling = css({ class WelcomeContainer extends React.PureComponent { componentDidMount() { + this.props.setShowUnauthorizedVeteranMessage(false); this.props.clearErrorMessage(); this.props.clearSearchInputText(); } @@ -41,11 +46,33 @@ class WelcomeContainer extends React.PureComponent { render() { return { this.props.errorMessage.title && - +

{this.props.errorMessage.message}

 } + { this.props.showUnauthorizedVeteranMessage && + +

+ Please try searching for another veteran or  + request access  +  to search for this veteran ID. +

+ + } +

Welcome to eFolder Express

eFolder Express allows VA employees to bulk-download VBMS eFolders. @@ -86,6 +113,7 @@ Note: eFolder Express now includes Virtual VA documents from the Legacy Content const mapStateToProps = (state) => ({ csrfToken: state.csrfToken, errorMessage: state.errorMessage, + showUnauthorizedVeteranMessage: state.showUnauthorizedVeteranMessage, searchInputText: state.searchInputText }); @@ -94,7 +122,8 @@ const mapDispatchToProps = (dispatch) => bindActionCreators({ clearSearchInputText, setVeteranId, startManifestFetch, - setSearchInputText + setSearchInputText, + setShowUnauthorizedVeteranMessage }, dispatch); export default connect(mapStateToProps, mapDispatchToProps)(WelcomeContainer); diff --git a/client/src/reducer.js b/client/src/reducer.js index f93a21b1d..9651e0e72 100644 --- a/client/src/reducer.js +++ b/client/src/reducer.js @@ -16,6 +16,7 @@ export const initState = { errorMessage: { title: '', message: '' }, recentDownloads: [], searchInputText: '', + showUnauthorizedVeteranMessage: false, ...defaultManifestState }; @@ -58,6 +59,12 @@ export default function reducer(state = {}, action = {}) { return { ...state, documentsFetchStatus: action.payload }; + case Actions.SET_SHOW_UNAUTHORIZED_VETERAN_MESSAGE: + return { + ...state, + showUnauthorizedVeteranMessage: action.payload + }; + case Actions.SET_ERROR_MESSAGE: return { ...state, diff --git a/lib/fakes/bgs_service.rb b/lib/fakes/bgs_service.rb index 65de30350..d9d37b69f 100644 --- a/lib/fakes/bgs_service.rb +++ b/lib/fakes/bgs_service.rb @@ -120,6 +120,14 @@ def record_found?(veteran_info) ExternalApi::BGSService.new(client: true).record_found?(veteran_info) end + def sensitivity_level_for_user(user) + 0 + end + + def sensitivity_level_for_veteran(veteran_file_number) + 9 + end + # Methods to be stubbed out in tests: def veteran_info; end diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index aa115ef04..aa73aac28 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -1,38 +1,69 @@ describe Manifest do - context "#start!" do - before do - Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) - allow(V2::DownloadManifestJob).to receive(:perform_later) - end - - let(:manifest) { Manifest.create(file_number: "1234") } + describe "#start!" do + let(:user) { User.create(css_id: "Foo", station_id: "112") } + let(:manifest) { Manifest.create(file_number: "1234", user: user) } subject { manifest.start! } - context "when never fetched" do - it "starts all jobs" do - expect(manifest.sources.size).to eq 0 - subject - expect(manifest.sources.size).to eq 2 - expect(V2::DownloadManifestJob).to have_received(:perform_later).twice + context "without sensitivity level check" do + before do + Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) + allow(V2::DownloadManifestJob).to receive(:perform_later) + expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) end - end - context "when all manifests are current" do - it "does not start any jobs" do - manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) - manifest.sources.create(name: "VBMS", status: :success, fetched_at: 2.hours.ago) - subject - expect(V2::DownloadManifestJob).to_not have_received(:perform_later) + context "when never fetched" do + it "starts all jobs" do + expect(manifest.sources.size).to eq 0 + subject + expect(manifest.sources.size).to eq 2 + expect(V2::DownloadManifestJob).to have_received(:perform_later).twice + end + end + + context "when all manifests are current" do + it "does not start any jobs" do + manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) + manifest.sources.create(name: "VBMS", status: :success, fetched_at: 2.hours.ago) + subject + expect(V2::DownloadManifestJob).to_not have_received(:perform_later) + end + end + + context "when one manifest is expired" do + it "starts one job" do + manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) + manifest.sources.create(name: "VBMS", status: :success, fetched_at: 5.hours.ago) + subject + expect(V2::DownloadManifestJob).to have_received(:perform_later).once + end end end - context "when one manifest is expired" do - it "starts one job" do - manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) - manifest.sources.create(name: "VBMS", status: :success, fetched_at: 5.hours.ago) + context "with sensitivity level check" do + let(:mock_sensitivity_checker) { instance_double(SensitivityChecker) } + + before do + allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) + allow(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(true) + end + + it "enqueues a job if the sensitivity check passes" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "1234").and_return(true) + expect(V2::DownloadManifestJob).to receive(:perform_later).twice + subject - expect(V2::DownloadManifestJob).to have_received(:perform_later).once + end + + it "raises an exception if the sensitivity check fails" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "1234").and_return(false) + expect(V2::DownloadManifestJob).to_not receive(:perform_later) + + expect { subject }.to raise_error(BGS::SensitivityLevelCheckFailure) end end end diff --git a/spec/services/external_api/bgs_service_spec.rb b/spec/services/external_api/bgs_service_spec.rb index 3d9b09a61..cef54e327 100644 --- a/spec/services/external_api/bgs_service_spec.rb +++ b/spec/services/external_api/bgs_service_spec.rb @@ -121,6 +121,45 @@ .with(file_number) { bgs_benefit_claims_response } end + describe "#sensitivity_level_for_user" do + let(:user) { User.create(css_id: "Foo", station_id: "112", participant_id: "abc123") } + + it "validates the user param" do + expect { bgs_service.sensitivity_level_for_user(nil) }.to raise_error(RuntimeError, "Invalid user") + end + + it "calls the security service and caches the result" do + sensitivity_level = Random.new.rand(1..9) + + expect(bgs_client).to receive(:security).and_return(bgs_security_service) + expect(bgs_security_service).to receive(:find_person_scrty_log_by_ptcpnt_id) + .with(user.participant_id).and_return({ scrty_level_type_cd: sensitivity_level.to_s }) + + expect(bgs_service.sensitivity_level_for_user(user)).to eq(sensitivity_level) + + expect(Rails.cache.exist?("sensitivity_level_for_user_id_#{user.id}")).to be true + end + end + + describe "#sensitivity_level_for_veteran" do + it "validates the veteran_file_number param" do + expect(bgs_veteran_service).to receive(:find_by_file_number).with(nil).and_return(nil) + expect { bgs_service.sensitivity_level_for_veteran(nil) }.to raise_error(RuntimeError, "Invalid veteran") + end + + it "calls the security service and caches the result" do + sensitivity_level = Random.new.rand(1..9) + + expect(bgs_client).to receive(:security).and_return(bgs_security_service) + expect(bgs_security_service).to receive(:find_sensitivity_level_by_participant_id) + .with(participant_id).and_return({ scrty_level_type_cd: sensitivity_level.to_s }) + + expect(bgs_service.sensitivity_level_for_veteran(file_number)).to eq(sensitivity_level) + + expect(Rails.cache.exist?("sensitivity_level_for_veteran_participant_id_#{participant_id}")).to be true + end + end + context "#parse_veteran_info" do before do @veteran_data = { diff --git a/spec/services/sensitivity_checker_spec.rb b/spec/services/sensitivity_checker_spec.rb new file mode 100644 index 000000000..d4670ea36 --- /dev/null +++ b/spec/services/sensitivity_checker_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +describe SensitivityChecker do + subject(:described) { described_class.new } + + let(:user) { User.create(css_id: "Foo", station_id: "112") } + let(:mock_sensitivity_checker) { instance_double(BGSService) } + + before do + allow(BGSService).to receive(:new).and_return(mock_sensitivity_checker) + end + + describe "#sensitivity_levels_compatible?" do + context "when the sensitivity levels are compatible" do + it "returns true" do + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_return(Random.new.rand(4..9)) + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_veteran) + .with("1234").and_return(Random.new.rand(1..4)) + + expect(described.sensitivity_levels_compatible?(user: user, veteran_file_number: "1234")).to eq true + end + end + + context "when the sensitivity levels are NOT compatible" do + it "returns false" do + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_return(Random.new.rand(1..4)) + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_veteran) + .with("1234").and_return(Random.new.rand(4..9)) + + expect(described.sensitivity_levels_compatible?(user: user, veteran_file_number: "1234")).to eq false + end + end + + context "when the BGS call raises an exception" do + it "returns false" do + error = StandardError.new + + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_raise(error) + expect(ExceptionLogger).to receive(:capture).with(error) + + expect(described.sensitivity_levels_compatible?(user: user, veteran_file_number: "1234")).to eq false + end + end + end +end From be5a1f76160d73532f2ea7ac2afc7a66c88866e7 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Tue, 13 Aug 2024 07:35:51 -0600 Subject: [PATCH 02/36] Add Specs to Cover Additional Sensitivity Cases (#1662) --- spec/models/manifest_spec.rb | 38 ++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index aa73aac28..524fdd75b 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -46,24 +46,38 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) - allow(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) - expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(true) end - it "enqueues a job if the sensitivity check passes" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) - .with(user: user, veteran_file_number: "1234").and_return(true) - expect(V2::DownloadManifestJob).to receive(:perform_later).twice + context "when check_user_sensitivity feature toggle is enabled" do + before { FeatureToggle.enable!(:check_user_sensitivity) } + after { FeatureToggle.disable!(:check_user_sensitivity) } - subject + it "enqueues a job if the sensitivity check passes" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "1234").and_return(true) + expect(V2::DownloadManifestJob).to receive(:perform_later).twice + + subject + end + + it "raises an exception if the sensitivity check fails" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "1234").and_return(false) + expect(V2::DownloadManifestJob).to_not receive(:perform_later) + + expect { subject }.to raise_error(BGS::SensitivityLevelCheckFailure) + end end - it "raises an exception if the sensitivity check fails" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) - .with(user: user, veteran_file_number: "1234").and_return(false) - expect(V2::DownloadManifestJob).to_not receive(:perform_later) + context "when check_user_sensitivity feature toggle is disabled" do + before { FeatureToggle.disable!(:check_user_sensitivity) } + + it "enqueues a job to fetch the manifest" do + expect(mock_sensitivity_checker).to_not receive(:sensitivity_levels_compatible?) + expect(V2::DownloadManifestJob).to receive(:perform_later).twice - expect { subject }.to raise_error(BGS::SensitivityLevelCheckFailure) + subject + end end end end From d75b585465146ed7e67a8f192587c965f2289896 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Tue, 13 Aug 2024 12:57:02 -0600 Subject: [PATCH 03/36] Gate VBMS Methods with Sensitivity Checks (#1665) * Gate VBMS Methods with Sensitivity Checks Validate user access to veteran before allowing fetch of veteran data. * Remove Unneeded allow Directives in Spec --- app/services/external_api/vbms_service.rb | 16 +++++ .../external_api/vbms_service_spec.rb | 67 ++++++++++++++++++- 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 9a2038b0c..1d469fe35 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -24,6 +24,8 @@ def self.fetch_documents_for(download) end def self.v2_fetch_documents_for(veteran_file_number) + verify_user_veteran_access(veteran_file_number) + documents = [] if FeatureToggle.enabled?(:use_ce_api) @@ -42,6 +44,8 @@ def self.v2_fetch_documents_for(veteran_file_number) end def self.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_date_range = Time.zone.now) + verify_user_veteran_access(veteran_file_number) + documents = [] if FeatureToggle.enabled?(:use_ce_api) @@ -67,6 +71,8 @@ def self.fetch_document_file(document) end def self.v2_fetch_document_file(document) + verify_user_veteran_access(document.file_number) + if FeatureToggle.enabled?(:use_ce_api) # Not using #send_and_log_request because logging to MetricService implemeneted in CE API gem # Method call returns the response body, so no need to return response.content/body @@ -108,4 +114,14 @@ def self.vbms_client @vbms_client = init_client end + + def self.verify_user_veteran_access(veteran_file_number) + return if !FeatureToggle.enabled?(:check_user_sensitivity) + + raise "User does not have permission to access this information" unless + SensitivityChecker.new.sensitivity_levels_compatible?( + user: RequestStore[:current_user], + veteran_file_number: veteran_file_number + ) + end end diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 57c356575..99a009a00 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -3,13 +3,59 @@ describe ExternalApi::VBMSService do subject(:described) { described_class } + let(:mock_sensitivity_checker) { instance_double(SensitivityChecker, sensitivity_levels_compatible?: true) } + + before do + allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) + end + + describe ".verify_user_veteran_access" do + context "with check_user_sensitivity feature flag enabled" do + before { FeatureToggle.enable!(:check_user_sensitivity) } + after { FeatureToggle.disable!(:check_user_sensitivity) } + + let!(:user) do + user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") + RequestStore.store[:current_user] = user + end + + it "checks the user's sensitivity" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "123456789").and_return(true) + + described.verify_user_veteran_access("123456789") + end + + it "raises an exception when the sensitivity level is not compatible" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "123456789").and_return(false) + + expect { described.verify_user_veteran_access("123456789") } + .to raise_error(RuntimeError, "User does not have permission to access this information") + end + end + + context "with check_user_sensitivity feature flag disabled" do + before { FeatureToggle.disable!(:check_user_sensitivity) } + + it "does not check the user's sensitivity" do + expect(mock_sensitivity_checker).not_to receive(:sensitivity_levels_compatible?) + + described.verify_user_veteran_access("123456789") + end + end + end + describe ".v2_fetch_documents_for" do let(:mock_json_adapter) { instance_double(JsonApiResponseAdapter) } before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) + FeatureToggle.enable!(:check_user_sensitivity) end + after { FeatureToggle.disable!(:check_user_sensitivity) } + context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } after { FeatureToggle.disable!(:use_ce_api) } @@ -33,6 +79,7 @@ it "calls the PagedDocuments SOAP API endpoint" do veteran_id = "123456789" + expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(true) expect(described_class).to receive(:vbms_client) @@ -53,6 +100,7 @@ it "calls the FindDocumentVersionReference SOAP API endpoint" do veteran_id = "123456789" + expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(false) expect(VBMS::Requests::FindDocumentVersionReference).to receive(:new) @@ -70,8 +118,11 @@ before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) + FeatureToggle.enable!(:check_user_sensitivity) end + after { FeatureToggle.disable!(:check_user_sensitivity) } + context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } after { FeatureToggle.disable!(:use_ce_api) } @@ -94,8 +145,17 @@ describe ".v2_fetch_document_file" do context "with use_ce_api feature toggle enabled" do - before { FeatureToggle.enable!(:use_ce_api) } - after { FeatureToggle.disable!(:use_ce_api) } + before do + FeatureToggle.enable!(:use_ce_api) + FeatureToggle.disable!(:check_user_sensitivity) + end + + after do + FeatureToggle.disable!(:use_ce_api) + end + + let(:manifest) { Manifest.create(file_number: "1234") } + let(:source) { ManifestSource.create(name: %w[VBMS VVA].sample, manifest: manifest) } let(:fake_record) do Record.create( @@ -103,7 +163,8 @@ series_id: "{4444-4444}", received_at: Time.utc(2015, 9, 6, 1, 0, 0), type_id: "825", - mime_type: "application/pdf" + mime_type: "application/pdf", + manifest_source: source ) end From a724e36645bc33c0a7fd82e4cdf4dd6406fe7bcf Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Wed, 14 Aug 2024 13:54:34 -0600 Subject: [PATCH 04/36] Remove Unneeded Sensitivity Checks (#1666) * Remove Unneeded Sensitivity Checks Checks are already handled by the sensitive_record method. * Fix Non-Forbidden Banner Styling --- .../api/v2/manifests_controller.rb | 9 +- app/exceptions/bgs_errors.rb | 1 - app/models/manifest.rb | 19 +--- app/models/manifest_source.rb | 1 + client/src/components/AlertBanner.jsx | 2 +- client/src/containers/WelcomeContainer.jsx | 9 +- spec/models/manifest_spec.rb | 97 +++++-------------- 7 files changed, 38 insertions(+), 100 deletions(-) diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index 544054e56..76d5b36cb 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -6,8 +6,6 @@ def start manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: file_number) manifest.start! render json: json_manifests(manifest) - rescue BGS::SensitivityLevelCheckFailure => e - sensitivity_check_failure_response end def refresh @@ -17,8 +15,6 @@ def refresh manifest.start! render json: json_manifests(manifest) - rescue BGS::SensitivityLevelCheckFailure => e - sensitivity_check_failure_response end def progress @@ -27,6 +23,7 @@ def progress files_download ||= FilesDownload.find_with_manifest(manifest_id: params[:id], user_id: current_user.id) end return record_not_found unless files_download + render json: json_manifests(files_download.manifest) end @@ -36,10 +33,6 @@ def history private - def sensitivity_check_failure_response - render json: { statusText: "This user does not have permission to access this information" }, status: :forbidden - end - def json_manifests(manifest) ActiveModelSerializers::SerializableResource.new( manifest, diff --git a/app/exceptions/bgs_errors.rb b/app/exceptions/bgs_errors.rb index 082d4637a..a61d1a594 100644 --- a/app/exceptions/bgs_errors.rb +++ b/app/exceptions/bgs_errors.rb @@ -5,5 +5,4 @@ class InvalidApplication < StandardError; end class NoActiveStations < StandardError; end class NoCaseflowAccess < StandardError; end class StationAssertionRequired < StandardError; end - class SensitivityLevelCheckFailure < StandardError; end end diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 0fac038ad..25a4c35c0 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -31,19 +31,7 @@ def start! # Reset stale manifests. update!(fetched_files_status: :initialized) if ready_for_refresh? - if FeatureToggle.enabled?(:check_user_sensitivity) - if sensitivity_checker.sensitivity_levels_compatible?( - user: user, - veteran_file_number: file_number - ) - vbms_source.start! - else - raise BGS::SensitivityLevelCheckFailure.new - end - else - vbms_source.start! - end - + vbms_source.start! vva_source.start! unless FeatureToggle.enabled?(:skip_vva) end @@ -53,6 +41,7 @@ def download_and_package_files! expiration: SECONDS_TO_AUTO_UNLOCK) s.lock(SECONDS_TO_AUTO_UNLOCK) do return if pending? + update(fetched_files_status: :pending) end @@ -180,8 +169,4 @@ def update_veteran_info veteran_last_name: veteran.last_name || "", veteran_last_four_ssn: veteran.last_four_ssn || "") end - - def sensitivity_checker - @sensitivity_checker ||= SensitivityChecker.new - end end diff --git a/app/models/manifest_source.rb b/app/models/manifest_source.rb index 6a74f4be3..2a1ed1a68 100644 --- a/app/models/manifest_source.rb +++ b/app/models/manifest_source.rb @@ -19,6 +19,7 @@ class ManifestSource < ApplicationRecord def start! return if current? || processing? + V2::DownloadManifestJob.perform_later(self, RequestStore[:current_user]) rescue StandardError update(status: :initialized) diff --git a/client/src/components/AlertBanner.jsx b/client/src/components/AlertBanner.jsx index 4d7f06f00..5fb804108 100644 --- a/client/src/components/AlertBanner.jsx +++ b/client/src/components/AlertBanner.jsx @@ -25,7 +25,7 @@ export default class AlertBanner extends React.PureComponent { break; } - return

+ return

{this.props.title}

{this.props.children}
diff --git a/client/src/containers/WelcomeContainer.jsx b/client/src/containers/WelcomeContainer.jsx index c91f82013..84085c322 100644 --- a/client/src/containers/WelcomeContainer.jsx +++ b/client/src/containers/WelcomeContainer.jsx @@ -24,6 +24,11 @@ const searchBarNoteTextStyling = css({ textAlign: 'center' }); +const alertBannerStyling = { + marginTop: '0px', + marginBottom: '30px' +}; + class WelcomeContainer extends React.PureComponent { componentDidMount() { this.props.setShowUnauthorizedVeteranMessage(false); @@ -49,7 +54,7 @@ class WelcomeContainer extends React.PureComponent {

{this.props.errorMessage.message}

 @@ -59,7 +64,7 @@ class WelcomeContainer extends React.PureComponent {

Please try searching for another veteran or  diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index 524fdd75b..aa115ef04 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -1,83 +1,38 @@ describe Manifest do - describe "#start!" do - let(:user) { User.create(css_id: "Foo", station_id: "112") } - let(:manifest) { Manifest.create(file_number: "1234", user: user) } - - subject { manifest.start! } - - context "without sensitivity level check" do - before do - Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) - allow(V2::DownloadManifestJob).to receive(:perform_later) - expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) - expect(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) - end + context "#start!" do + before do + Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) + allow(V2::DownloadManifestJob).to receive(:perform_later) + end - context "when never fetched" do - it "starts all jobs" do - expect(manifest.sources.size).to eq 0 - subject - expect(manifest.sources.size).to eq 2 - expect(V2::DownloadManifestJob).to have_received(:perform_later).twice - end - end + let(:manifest) { Manifest.create(file_number: "1234") } - context "when all manifests are current" do - it "does not start any jobs" do - manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) - manifest.sources.create(name: "VBMS", status: :success, fetched_at: 2.hours.ago) - subject - expect(V2::DownloadManifestJob).to_not have_received(:perform_later) - end - end + subject { manifest.start! } - context "when one manifest is expired" do - it "starts one job" do - manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) - manifest.sources.create(name: "VBMS", status: :success, fetched_at: 5.hours.ago) - subject - expect(V2::DownloadManifestJob).to have_received(:perform_later).once - end + context "when never fetched" do + it "starts all jobs" do + expect(manifest.sources.size).to eq 0 + subject + expect(manifest.sources.size).to eq 2 + expect(V2::DownloadManifestJob).to have_received(:perform_later).twice end end - context "with sensitivity level check" do - let(:mock_sensitivity_checker) { instance_double(SensitivityChecker) } - - before do - allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) - end - - context "when check_user_sensitivity feature toggle is enabled" do - before { FeatureToggle.enable!(:check_user_sensitivity) } - after { FeatureToggle.disable!(:check_user_sensitivity) } - - it "enqueues a job if the sensitivity check passes" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) - .with(user: user, veteran_file_number: "1234").and_return(true) - expect(V2::DownloadManifestJob).to receive(:perform_later).twice - - subject - end - - it "raises an exception if the sensitivity check fails" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) - .with(user: user, veteran_file_number: "1234").and_return(false) - expect(V2::DownloadManifestJob).to_not receive(:perform_later) - - expect { subject }.to raise_error(BGS::SensitivityLevelCheckFailure) - end + context "when all manifests are current" do + it "does not start any jobs" do + manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) + manifest.sources.create(name: "VBMS", status: :success, fetched_at: 2.hours.ago) + subject + expect(V2::DownloadManifestJob).to_not have_received(:perform_later) end + end - context "when check_user_sensitivity feature toggle is disabled" do - before { FeatureToggle.disable!(:check_user_sensitivity) } - - it "enqueues a job to fetch the manifest" do - expect(mock_sensitivity_checker).to_not receive(:sensitivity_levels_compatible?) - expect(V2::DownloadManifestJob).to receive(:perform_later).twice - - subject - end + context "when one manifest is expired" do + it "starts one job" do + manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) + manifest.sources.create(name: "VBMS", status: :success, fetched_at: 5.hours.ago) + subject + expect(V2::DownloadManifestJob).to have_received(:perform_later).once end end end From fc27329b9387422b05cc8aa9e118a4d0435e0034 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Thu, 15 Aug 2024 13:56:44 -0600 Subject: [PATCH 05/36] Restore Pre-Fetch Sensitivity Check Logic & Add Frontend Feature Toggle (#1667) * Pass Sensitivity Check Feature Toggle to UI * Restore Manifest Sensitivity Logic --- .../api/v1/application_controller.rb | 7 +- .../api/v2/manifests_controller.rb | 4 + app/exceptions/bgs_errors.rb | 1 + app/models/manifest.rb | 17 +++- client/src/apiActions.jsx | 6 +- spec/models/manifest_spec.rb | 81 +++++++++++++------ 6 files changed, 88 insertions(+), 28 deletions(-) diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index 17581100f..66385a934 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -28,7 +28,12 @@ def sensitive_record end def forbidden(reason = "Forbidden: unspecified") - render json: { status: reason }, status: 403 + render json: { + status: reason, + featureToggles: { + checkUserSensitivity: FeatureToggle.enabled?(:check_user_sensitivity) + } + }, status: :forbidden end def missing_header(header) diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index 76d5b36cb..ae70f0925 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -6,6 +6,8 @@ def start manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: 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 @@ -15,6 +17,8 @@ def refresh manifest.start! render json: json_manifests(manifest) + rescue BGS::SensitivityLevelCheckFailure + forbidden("This user does not have permission to access this information") end def progress diff --git a/app/exceptions/bgs_errors.rb b/app/exceptions/bgs_errors.rb index a61d1a594..082d4637a 100644 --- a/app/exceptions/bgs_errors.rb +++ b/app/exceptions/bgs_errors.rb @@ -5,4 +5,5 @@ class InvalidApplication < StandardError; end class NoActiveStations < StandardError; end class NoCaseflowAccess < StandardError; end class StationAssertionRequired < StandardError; end + class SensitivityLevelCheckFailure < StandardError; end end diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 25a4c35c0..8b974447c 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -31,7 +31,18 @@ def start! # Reset stale manifests. update!(fetched_files_status: :initialized) if ready_for_refresh? - vbms_source.start! + if FeatureToggle.enabled?(:check_user_sensitivity) + if sensitivity_checker.sensitivity_levels_compatible?( + user: user, + veteran_file_number: file_number + ) + vbms_source.start! + else + raise BGS::SensitivityLevelCheckFailure.new, "Unauthorized" + end + else + vbms_source.start! + end vva_source.start! unless FeatureToggle.enabled?(:skip_vva) end @@ -169,4 +180,8 @@ def update_veteran_info veteran_last_name: veteran.last_name || "", veteran_last_four_ssn: veteran.last_four_ssn || "") end + + def sensitivity_checker + @sensitivity_checker ||= SensitivityChecker.new + end end diff --git a/client/src/apiActions.jsx b/client/src/apiActions.jsx index f136bc680..c4ad77905 100644 --- a/client/src/apiActions.jsx +++ b/client/src/apiActions.jsx @@ -166,6 +166,10 @@ export const restartManifestFetch = (manifestId, csrfToken) => (dispatch) => { }; export const startManifestFetch = (veteranId, csrfToken, redirectFunction) => (dispatch) => { + // Reset any error messages currently being displayed + dispatch(setShowUnauthorizedVeteranMessage(false)); + dispatch(setErrorMessage('')); + postRequest('/api/v2/manifests/', csrfToken, { 'FILE-NUMBER': veteranId }). then( (resp) => { @@ -177,7 +181,7 @@ export const startManifestFetch = (veteranId, csrfToken, redirectFunction) => (d redirectFunction(`/downloads/${manifestId}`); }, (err) => { - if (err.response.statusCode === 403) { + if (err.response.statusCode === 403 && err.response.body.featureToggles.checkUserSensitivity === true) { dispatch(setShowUnauthorizedVeteranMessage(true)); } else { dispatch(setErrorMessage(buildErrorMessageFromResponse(err.response))); diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index aa115ef04..aa73aac28 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -1,38 +1,69 @@ describe Manifest do - context "#start!" do - before do - Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) - allow(V2::DownloadManifestJob).to receive(:perform_later) - end - - let(:manifest) { Manifest.create(file_number: "1234") } + describe "#start!" do + let(:user) { User.create(css_id: "Foo", station_id: "112") } + let(:manifest) { Manifest.create(file_number: "1234", user: user) } subject { manifest.start! } - context "when never fetched" do - it "starts all jobs" do - expect(manifest.sources.size).to eq 0 - subject - expect(manifest.sources.size).to eq 2 - expect(V2::DownloadManifestJob).to have_received(:perform_later).twice + context "without sensitivity level check" do + before do + Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) + allow(V2::DownloadManifestJob).to receive(:perform_later) + expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) end - end - context "when all manifests are current" do - it "does not start any jobs" do - manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) - manifest.sources.create(name: "VBMS", status: :success, fetched_at: 2.hours.ago) - subject - expect(V2::DownloadManifestJob).to_not have_received(:perform_later) + context "when never fetched" do + it "starts all jobs" do + expect(manifest.sources.size).to eq 0 + subject + expect(manifest.sources.size).to eq 2 + expect(V2::DownloadManifestJob).to have_received(:perform_later).twice + end + end + + context "when all manifests are current" do + it "does not start any jobs" do + manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) + manifest.sources.create(name: "VBMS", status: :success, fetched_at: 2.hours.ago) + subject + expect(V2::DownloadManifestJob).to_not have_received(:perform_later) + end + end + + context "when one manifest is expired" do + it "starts one job" do + manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) + manifest.sources.create(name: "VBMS", status: :success, fetched_at: 5.hours.ago) + subject + expect(V2::DownloadManifestJob).to have_received(:perform_later).once + end end end - context "when one manifest is expired" do - it "starts one job" do - manifest.sources.create(name: "VVA", status: :success, fetched_at: 2.hours.ago) - manifest.sources.create(name: "VBMS", status: :success, fetched_at: 5.hours.ago) + context "with sensitivity level check" do + let(:mock_sensitivity_checker) { instance_double(SensitivityChecker) } + + before do + allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) + allow(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(true) + end + + it "enqueues a job if the sensitivity check passes" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "1234").and_return(true) + expect(V2::DownloadManifestJob).to receive(:perform_later).twice + subject - expect(V2::DownloadManifestJob).to have_received(:perform_later).once + end + + it "raises an exception if the sensitivity check fails" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "1234").and_return(false) + expect(V2::DownloadManifestJob).to_not receive(:perform_later) + + expect { subject }.to raise_error(BGS::SensitivityLevelCheckFailure) end end end From 507b9eec2f006499fb8d8d045769d3d1a7d5cd20 Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 15 Aug 2024 15:58:26 -0400 Subject: [PATCH 06/36] Updated ruby_claim_evidence_api gem --- Gemfile.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 01b1faf22..51056603e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -225,7 +225,7 @@ GEM coffee-script-source execjs coffee-script-source (1.12.2) - concurrent-ruby (1.3.3) + concurrent-ruby (1.3.4) crack (1.0.0) bigdecimal rexml @@ -261,7 +261,7 @@ GEM google-protobuf (>= 3.18, < 5.a) gyoku (1.3.1) builder (>= 2.1.2) - hashdiff (1.1.0) + hashdiff (1.1.1) hashie (4.1.0) httparty (0.18.1) mime-types (~> 3.0) @@ -309,7 +309,7 @@ GEM mini_magick (4.11.0) mini_mime (1.1.5) mini_portile2 (2.8.7) - minitest (5.24.1) + minitest (5.25.0) moment_timezone-rails (0.5.14) momentjs-rails (~> 2.15.1) momentjs-rails (2.15.1) @@ -440,7 +440,7 @@ GEM public_suffix (5.1.1) puma (5.6.4) nio4r (~> 2.0) - racc (1.8.0) + racc (1.8.1) rack (2.2.9) rack-cors (1.1.1) rack (>= 2.0.0) @@ -507,7 +507,7 @@ GEM regexp_parser (2.8.3) request_store (1.5.0) rack (>= 1.4) - rexml (3.3.1) + rexml (3.3.5) strscan rspec (3.10.0) rspec-core (~> 3.10.0) @@ -645,7 +645,7 @@ GEM xpath (3.2.0) nokogiri (~> 1.8) zaru (0.3.0) - zeitwerk (2.6.16) + zeitwerk (2.6.17) zero_downtime_migrations (0.0.7) activerecord From 7c242c915193694bb39be718d712c315d5ea3133 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Wed, 4 Sep 2024 09:32:36 -0600 Subject: [PATCH 07/36] 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") } From 7a02c1d0423ae064d315993b6e4c997fcc2a9016 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 08:37:25 -0600 Subject: [PATCH 08/36] Use New Sensitivity Check Method (#1676) - Use new method to check user/veteran sensitivity compatibility in the V2 ApplicationController. - This will prevent the old "use BGS error to verify access" logic from running. Co-authored-by: cacevesva <109166981+cacevesva@users.noreply.github.com> --- .../api/v2/application_controller.rb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/v2/application_controller.rb b/app/controllers/api/v2/application_controller.rb index f7f25cd24..d9929d69e 100644 --- a/app/controllers/api/v2/application_controller.rb +++ b/app/controllers/api/v2/application_controller.rb @@ -23,7 +23,18 @@ def verify_veteran_file_number return invalid_file_number unless bgs_service.valid_file_number?(file_number) - fetch_veteran_by_file_number(file_number) + if FeatureToggle.enabled?(:check_user_sensitivity) + if sensitivity_checker.sensitivity_levels_compatible?( + user: current_user, + veteran_file_number: file_number + ) + return file_number + else + raise BGS::SensitivityLevelCheckFailure.new, "You are not authorized to access this file number" + end + else + fetch_veteran_by_file_number(file_number) + end end def fetch_veteran_by_file_number(file_number) @@ -41,4 +52,8 @@ def fetch_veteran_by_file_number(file_number) raise BGS::ShareError, "Cannot access Veteran record" end end + + def sensitivity_checker + @sensitivity_checker ||= SensitivityChecker.new + end end From 5524ca9ca46464e459399bcf3948455d98d66fc1 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 09:10:54 -0600 Subject: [PATCH 09/36] Update Specs to Address Failures (#1677) --- spec/requests/api/v2/manifests_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index 795950823..acbbcdec0 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -50,7 +50,7 @@ context "when the check succeeds" do it "allows access to the start action" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests", params: nil, headers: headers @@ -59,7 +59,7 @@ end it "allows access to the refresh action" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests/#{manifest.id}", params: nil, headers: headers @@ -76,7 +76,7 @@ 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") + expect(JSON.parse(response.body)["status"]).to match(/You are not authorized to access this/) end it "gates access to the refresh action" do @@ -86,7 +86,7 @@ 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") + expect(JSON.parse(response.body)["status"]).to match(/You are not authorized to access this/) end end end From b9922cb3a3fcf4e8d5f4c7851d4728f4942faa50 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 11:28:36 -0600 Subject: [PATCH 10/36] Send Veteran Number in Restart Request (#1679) * Send Veteran Number in Restart Request * Update Link rel Param --- client/src/apiActions.jsx | 4 ++-- client/src/containers/DownloadContainer.jsx | 2 +- client/src/containers/WelcomeContainer.jsx | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/client/src/apiActions.jsx b/client/src/apiActions.jsx index c4ad77905..3e00388a5 100644 --- a/client/src/apiActions.jsx +++ b/client/src/apiActions.jsx @@ -157,8 +157,8 @@ export const startDocumentDownload = (manifestId, csrfToken) => (dispatch) => { ); }; -export const restartManifestFetch = (manifestId, csrfToken) => (dispatch) => { - postRequest(`/api/v2/manifests/${manifestId}`, csrfToken). +export const restartManifestFetch = (manifestId, veteranId, csrfToken) => (dispatch) => { + postRequest(`/api/v2/manifests/${manifestId}`, csrfToken, { 'FILE-NUMBER': veteranId }). then( (resp) => setStateFromResponse(dispatch, resp), (err) => dispatch(setErrorMessage(buildErrorMessageFromResponse(err.response))) diff --git a/client/src/containers/DownloadContainer.jsx b/client/src/containers/DownloadContainer.jsx index b1fc9576d..beda3930f 100644 --- a/client/src/containers/DownloadContainer.jsx +++ b/client/src/containers/DownloadContainer.jsx @@ -39,7 +39,7 @@ class DownloadContainer extends React.PureComponent { !manifestFetchComplete(this.props.documentSources) || this.props.documentsFetchStatus === MANIFEST_DOWNLOAD_STATE.IN_PROGRESS ) { - this.props.restartManifestFetch(manifestId, this.props.csrfToken); + this.props.restartManifestFetch(manifestId, this.props.veteranId, this.props.csrfToken); this.props.pollManifestFetchEndpoint(0, manifestId, this.props.csrfToken); } } diff --git a/client/src/containers/WelcomeContainer.jsx b/client/src/containers/WelcomeContainer.jsx index 84085c322..0f423ddb4 100644 --- a/client/src/containers/WelcomeContainer.jsx +++ b/client/src/containers/WelcomeContainer.jsx @@ -71,7 +71,7 @@ class WelcomeContainer extends React.PureComponent { request access   to search for this veteran ID.

From 48e3cdee10e276044015cf5a160ba452009bc4ef Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 11:53:07 -0600 Subject: [PATCH 11/36] Update CE API Gem (#1680) --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 8b456abf3..e040b953e 100644 --- a/Gemfile +++ b/Gemfile @@ -57,7 +57,7 @@ gem "redis-namespace" gem "redis-rails", "~> 5.0.2" gem "redis-semaphore" gem "request_store" -gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", ref: "095798918338650383b06ff535bc63fc5fbfc8dc" +gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", ref: "c56381d2bea2ffabe79bebbac6598f00450691ff" gem "rubyzip", ">= 1.3.0" gem "sass-rails", "~> 5.0" gem "sentry-raven" diff --git a/Gemfile.lock b/Gemfile.lock index 51056603e..177d89886 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -74,8 +74,8 @@ GIT GIT remote: https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git - revision: 095798918338650383b06ff535bc63fc5fbfc8dc - ref: 095798918338650383b06ff535bc63fc5fbfc8dc + revision: c56381d2bea2ffabe79bebbac6598f00450691ff + ref: c56381d2bea2ffabe79bebbac6598f00450691ff specs: ruby_claim_evidence_api (0.1.0) activesupport From d08736bcaee85ed5d6347adefabe6964f6feda8f Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 9 Sep 2024 07:36:06 -0600 Subject: [PATCH 12/36] Update Logic for Setting Veteran ID in Request (#1681) - Remove recently-added frontend logic for setting veteran ID in refresh request as it is unreliable in the way it sets the veteran ID. - Update manifests_controller to set the veteran ID using the manifest as this is much more reliable. --- app/controllers/api/v2/manifests_controller.rb | 13 +++++++++++++ client/src/apiActions.jsx | 4 ++-- client/src/containers/DownloadContainer.jsx | 2 +- spec/requests/api/v2/manifests_spec.rb | 4 ++-- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index 905ee47cb..ca586fd4d 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -42,6 +42,19 @@ def veteran_file_number @veteran_file_number ||= verify_veteran_file_number end + def verify_veteran_file_number + # The frontend may not have set this value (needed by parent's verify_veteran_file_number) + # but we are still able to determine it here in the child using the manifest + if request.headers["HTTP_FILE_NUMBER"].blank? + if params[:id].present? + manifest = Manifest.find(params[:id]) + request.headers["HTTP_FILE_NUMBER"] = manifest.file_number + end + end + + super + end + def json_manifests(manifest) ActiveModelSerializers::SerializableResource.new( manifest, diff --git a/client/src/apiActions.jsx b/client/src/apiActions.jsx index 3e00388a5..c4ad77905 100644 --- a/client/src/apiActions.jsx +++ b/client/src/apiActions.jsx @@ -157,8 +157,8 @@ export const startDocumentDownload = (manifestId, csrfToken) => (dispatch) => { ); }; -export const restartManifestFetch = (manifestId, veteranId, csrfToken) => (dispatch) => { - postRequest(`/api/v2/manifests/${manifestId}`, csrfToken, { 'FILE-NUMBER': veteranId }). +export const restartManifestFetch = (manifestId, csrfToken) => (dispatch) => { + postRequest(`/api/v2/manifests/${manifestId}`, csrfToken). then( (resp) => setStateFromResponse(dispatch, resp), (err) => dispatch(setErrorMessage(buildErrorMessageFromResponse(err.response))) diff --git a/client/src/containers/DownloadContainer.jsx b/client/src/containers/DownloadContainer.jsx index beda3930f..b1fc9576d 100644 --- a/client/src/containers/DownloadContainer.jsx +++ b/client/src/containers/DownloadContainer.jsx @@ -39,7 +39,7 @@ class DownloadContainer extends React.PureComponent { !manifestFetchComplete(this.props.documentSources) || this.props.documentsFetchStatus === MANIFEST_DOWNLOAD_STATE.IN_PROGRESS ) { - this.props.restartManifestFetch(manifestId, this.props.veteranId, this.props.csrfToken); + this.props.restartManifestFetch(manifestId, this.props.csrfToken); this.props.pollManifestFetchEndpoint(0, manifestId, this.props.csrfToken); } } diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index acbbcdec0..acd7622d0 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -50,7 +50,7 @@ context "when the check succeeds" do it "allows access to the start action" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).exactly(3).times .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests", params: nil, headers: headers @@ -59,7 +59,7 @@ end it "allows access to the refresh action" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).exactly(3).times .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests/#{manifest.id}", params: nil, headers: headers From b361e25e0788d292c983d3cb79893a5e01b29536 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Wed, 11 Sep 2024 16:49:23 -0600 Subject: [PATCH 13/36] Update JSON Parsing Logic to Handle CE API Gem Changes (#1686) - Gem now returns the JSON body of a HTTP response, so our response parsing code needed to be updated to handle the new format. - Update the VBMS service to alert us of any API responses that can't be parsed so we can troubleshoot them. --- app/services/external_api/vbms_service.rb | 16 ++++++++-- app/services/json_api_response_adapter.rb | 21 ++++++++---- .../external_api/vbms_service_spec.rb | 32 +++++++++++++++++++ .../json_api_response_adapter_spec.rb | 23 +++++-------- 4 files changed, 69 insertions(+), 23 deletions(-) diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 1d469fe35..2d709c91b 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -30,7 +30,7 @@ def self.v2_fetch_documents_for(veteran_file_number) if FeatureToggle.enabled?(:use_ce_api) response = VeteranFileFetcher.fetch_veteran_file_list(veteran_file_number: veteran_file_number) - documents = JsonApiResponseAdapter.new.adapt_v2_fetch_documents_for(response) + documents = process_fetch_veteran_file_list_response(response) elsif FeatureToggle.enabled?(:vbms_pagination, user: RequestStore[:current_user]) service = VBMS::Service::PagedDocuments.new(client: vbms_client) documents = call_and_log_service(service: service, vbms_id: veteran_file_number)[:documents] @@ -54,7 +54,7 @@ def self.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_da begin_date_range: begin_date_range, end_date_range: end_date_range ) - documents = JsonApiResponseAdapter.new.adapt_v2_fetch_documents_for(response) + documents = process_fetch_veteran_file_list_response(response) else request = VBMS::Requests::FindDocumentVersionReferenceByDateRange.new(veteran_file_number, begin_date_range, end_date_range) documents = send_and_log_request(veteran_file_number, request) @@ -124,4 +124,16 @@ def self.verify_user_veteran_access(veteran_file_number) veteran_file_number: veteran_file_number ) end + + def self.process_fetch_veteran_file_list_response(response) + documents = JsonApiResponseAdapter.new.adapt_v2_fetch_documents_for(response) + + # We want to be notified of any API responses that are not parsable + if documents.blank? + ex = RuntimeError.new("API response could not be parsed: #{response}") + ExceptionLogger.capture(ex) + end + + documents || [] + end end diff --git a/app/services/json_api_response_adapter.rb b/app/services/json_api_response_adapter.rb index 1cbd6d8f0..9356a2771 100644 --- a/app/services/json_api_response_adapter.rb +++ b/app/services/json_api_response_adapter.rb @@ -4,11 +4,12 @@ # by most of caseflow-efolder class JsonApiResponseAdapter def adapt_v2_fetch_documents_for(json_response) - documents = [] + json_response = normalize_json_response(json_response) - return documents unless valid_json_response?(json_response) + return nil unless valid_file_response?(json_response) - json_response.body["files"].each do |file_resp| + documents = [] + json_response["files"].each do |file_resp| documents.push(v2_fetch_documents_file_response(file_resp)) end @@ -17,10 +18,18 @@ def adapt_v2_fetch_documents_for(json_response) private - def valid_json_response?(json_response) - return false if json_response&.body.blank? + def normalize_json_response(json_response) + if json_response.blank? + {} + elsif json_response.instance_of?(Hash) + json_response.with_indifferent_access + elsif json_response.instance_of?(String) + JSON.parse(json_response) + end + end - json_response.body.key?("files") + def valid_file_response?(json_response) + json_response.key?("files") end def v2_fetch_documents_file_response(file_json) diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 99a009a00..1c0318c48 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -178,4 +178,36 @@ end end end + + describe ".process_fetch_veteran_file_list_response" do + let(:mock_json_adapter) { instance_double(JsonApiResponseAdapter) } + + before do + allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) + FeatureToggle.enable!(:use_ce_api) + end + + after { FeatureToggle.disable!(:use_ce_api) } + + context "when the adapted response is present" do + it "does not log any errors and returns the adapted response" do + expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).and_return(["foo"]) + expect(ExceptionLogger).not_to receive(:capture) + + result = described.process_fetch_veteran_file_list_response(nil) + expect(result.count).to eq 1 + end + end + + context "when the adapted response is nil" do + it "logs an exception and returns an empty array" do + expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).and_return(nil) + expect(RuntimeError).to receive(:new).with(/API response could not be parsed/).and_call_original + expect(ExceptionLogger).to receive(:capture).with(instance_of(RuntimeError)) + + result = described.process_fetch_veteran_file_list_response(nil) + expect(result).to eq [] + end + end + end end diff --git a/spec/services/json_api_response_adapter_spec.rb b/spec/services/json_api_response_adapter_spec.rb index 47346c02e..905bea885 100644 --- a/spec/services/json_api_response_adapter_spec.rb +++ b/spec/services/json_api_response_adapter_spec.rb @@ -5,28 +5,24 @@ describe JsonApiResponseAdapter do subject(:described) { described_class.new } - let(:api_response) { instance_double(ExternalApi::Response) } - describe "#adapt_v2_fetch_documents_for" do context "with invalid responses" do it "handles blank responses" do parsed = described.adapt_v2_fetch_documents_for(nil) - expect(parsed.length).to eq 0 + expect(parsed).to be_nil end - it "handles blank response bodies" do - response = instance_double(ExternalApi::Response, body: nil) - parsed = described.adapt_v2_fetch_documents_for(response) + it "handles empty response bodies" do + parsed = described.adapt_v2_fetch_documents_for({}) - expect(parsed.length).to eq 0 + expect(parsed).to be_nil end - it "handles response bodies with no files" do - response = instance_double(ExternalApi::Response, body: {}) - parsed = described.adapt_v2_fetch_documents_for(response) + it "handles blank response bodies" do + parsed = described.adapt_v2_fetch_documents_for("") - expect(parsed.length).to eq 0 + expect(parsed).to be_nil end end @@ -35,10 +31,7 @@ data_hash = JSON.parse(File.read(file)) file.close - expect(api_response).to receive(:body) - .exactly(3).times.and_return(data_hash) - - parsed = described.adapt_v2_fetch_documents_for(api_response) + parsed = described.adapt_v2_fetch_documents_for(data_hash) expect(parsed.length).to eq 2 From 23dca64717984109f8eeeeb43961fea429dd9c03 Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 12 Sep 2024 08:34:55 -0400 Subject: [PATCH 14/36] Updated ruby_claim_evidence_api --- Gemfile.lock | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 177d89886..947d7de76 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -309,7 +309,7 @@ GEM mini_magick (4.11.0) mini_mime (1.1.5) mini_portile2 (2.8.7) - minitest (5.25.0) + minitest (5.25.1) moment_timezone-rails (0.5.14) momentjs-rails (~> 2.15.1) momentjs-rails (2.15.1) @@ -507,8 +507,7 @@ GEM regexp_parser (2.8.3) request_store (1.5.0) rack (>= 1.4) - rexml (3.3.5) - strscan + rexml (3.3.7) rspec (3.10.0) rspec-core (~> 3.10.0) rspec-expectations (~> 3.10.0) @@ -600,7 +599,6 @@ GEM activesupport (>= 6.1) sprockets (>= 3.0.0) statsd-instrument (3.7.0) - strscan (3.1.0) systemu (2.6.5) thor (1.3.1) tilt (2.0.11) @@ -645,7 +643,7 @@ GEM xpath (3.2.0) nokogiri (~> 1.8) zaru (0.3.0) - zeitwerk (2.6.17) + zeitwerk (2.6.18) zero_downtime_migrations (0.0.7) activerecord From 200ea25bf151a34e1234828c334138ced107eaca Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 13 Sep 2024 11:53:05 -0600 Subject: [PATCH 15/36] Add current_user to SaveFilesInS3 Job (#1688) - Since the SaveFilesInS3 job is spawned by another job, it does not have access to RequestStore[:current_user] which is needed for verifying veteran/user sensitivity compatibility. - This PR also fixes several Rubocop violations in various files. --- app/helpers/application_helper.rb | 4 ++-- app/jobs/v2/download_manifest_job.rb | 10 +++++----- app/jobs/v2/save_files_in_s3_job.rb | 10 ++++++++-- app/services/external_api/vbms_service.rb | 1 - 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 9ed2d62a5..09d4f22ed 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true -# rubocop:disable Metrics/ModuleLength module ApplicationHelper def ui_user? return false unless RequestStore[:current_user] + (RequestStore[:current_user].roles || []).include?("Download eFolder") end @@ -13,6 +13,7 @@ def current_ga_path begin route = Rails.application.routes.recognize_path(full_path) return full_path unless route + ["", route[:controller], route[:action]].join("/") # no match in recognize_path @@ -21,4 +22,3 @@ def current_ga_path end end end -# rubocop:enable Metrics/ModuleLength diff --git a/app/jobs/v2/download_manifest_job.rb b/app/jobs/v2/download_manifest_job.rb index f6890e11a..ea71bcc0a 100644 --- a/app/jobs/v2/download_manifest_job.rb +++ b/app/jobs/v2/download_manifest_job.rb @@ -4,18 +4,18 @@ class V2::DownloadManifestJob < ApplicationJob def perform(manifest_source, user = nil) manifest_source.update(status: :pending) - RequestStore.store[:current_user] = user if user + RequestStore.store[:current_user] = user if user.present? Raven.extra_context(manifest_source: manifest_source.id) documents = ManifestFetcher.new(manifest_source: manifest_source).process log_info(manifest_source.manifest.file_number, documents, manifest_source.manifest.zipfile_size) - V2::SaveFilesInS3Job.perform_later(manifest_source) if documents.present? && !ApplicationController.helpers.ui_user? - rescue StandardError => error + V2::SaveFilesInS3Job.perform_later(manifest_source, current_user&.id) if documents.present? && !ApplicationController.helpers.ui_user? + rescue StandardError => e manifest_source.update!(status: :failed) - Rails.logger.error "DownloadManifestJob encountered error #{error.class.name} when fetching manifest for appeal #{manifest_source.manifest.file_number}" - raise error + Rails.logger.error "DownloadManifestJob encountered error #{e.class.name} when fetching manifest for appeal #{manifest_source.manifest.file_number}" + raise e end def max_attempts diff --git a/app/jobs/v2/save_files_in_s3_job.rb b/app/jobs/v2/save_files_in_s3_job.rb index 4f2d865a2..694b73543 100644 --- a/app/jobs/v2/save_files_in_s3_job.rb +++ b/app/jobs/v2/save_files_in_s3_job.rb @@ -1,8 +1,14 @@ class V2::SaveFilesInS3Job < ApplicationJob queue_as :low_priority - def perform(manifest_source) - Raven.extra_context(manifest_source: manifest_source.id) + def perform(manifest_source, user_id) + Raven.extra_context(manifest_source: manifest_source.id, user_id: user_id) + + # Set user for permission check if the user is blank + if FeatureToggle.enabled?(:check_user_sensitivity) && RequestStore[:current_user].blank? + user = User.find(user_id) + RequestStore.store[:current_user] = user + end manifest_source.records.each(&:fetch!) end diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 2d709c91b..d403d86d7 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -75,7 +75,6 @@ def self.v2_fetch_document_file(document) if FeatureToggle.enabled?(:use_ce_api) # Not using #send_and_log_request because logging to MetricService implemeneted in CE API gem - # Method call returns the response body, so no need to return response.content/body VeteranFileFetcher.get_document_content(doc_series_id: document.series_id) else request = VBMS::Requests::GetDocumentContent.new(document.document_id) From 44a88e520d27eade20223af0df8fcf6f5f9fdf6d Mon Sep 17 00:00:00 2001 From: SanthiParakal133 <132940479+SanthiParakal133@users.noreply.github.com> Date: Fri, 13 Sep 2024 13:53:49 -0400 Subject: [PATCH 16/36] add check for empty result (#1687) --- app/services/external_api/vbms_service.rb | 2 +- app/services/json_api_response_adapter.rb | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index d403d86d7..23d74899e 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -128,7 +128,7 @@ def self.process_fetch_veteran_file_list_response(response) documents = JsonApiResponseAdapter.new.adapt_v2_fetch_documents_for(response) # We want to be notified of any API responses that are not parsable - if documents.blank? + if documents.nil? ex = RuntimeError.new("API response could not be parsed: #{response}") ExceptionLogger.capture(ex) end diff --git a/app/services/json_api_response_adapter.rb b/app/services/json_api_response_adapter.rb index 9356a2771..122659070 100644 --- a/app/services/json_api_response_adapter.rb +++ b/app/services/json_api_response_adapter.rb @@ -6,6 +6,7 @@ class JsonApiResponseAdapter def adapt_v2_fetch_documents_for(json_response) json_response = normalize_json_response(json_response) + return [] if check_empty_result?(json_response) return nil unless valid_file_response?(json_response) documents = [] @@ -32,6 +33,10 @@ def valid_file_response?(json_response) json_response.key?("files") end + def check_empty_result?(json_response) + json_response.key?("page") && json_response['page']['totalResults'].to_i == 0 + end + def v2_fetch_documents_file_response(file_json) system_data = file_json["currentVersion"]["systemData"] provider_data = file_json["currentVersion"]["providerData"] From 76a5800debd876980a9cf3a6d3b4a63390a21c50 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 13 Sep 2024 12:13:17 -0600 Subject: [PATCH 17/36] Add Script for Finding Valid UAT Testing Users (#1690) --- config/initializers/deploy_env.rb | 4 ++ .../find_valid_veteran_file_numbers.rb | 66 +++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 lib/scripts/find_valid_veteran_file_numbers.rb diff --git a/config/initializers/deploy_env.rb b/config/initializers/deploy_env.rb index f797035c6..76d8a87dd 100644 --- a/config/initializers/deploy_env.rb +++ b/config/initializers/deploy_env.rb @@ -23,4 +23,8 @@ def self.current_env def self.deploy_env current_env end + + def self.non_production_env? + deploy_env == :uat || deploy_env == :test || deploy_env == :development + end end diff --git a/lib/scripts/find_valid_veteran_file_numbers.rb b/lib/scripts/find_valid_veteran_file_numbers.rb new file mode 100644 index 000000000..10ed086bf --- /dev/null +++ b/lib/scripts/find_valid_veteran_file_numbers.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +class FindValidVeteranFileNumbers + def initialize(user: nil) + fail "Non-prod only" unless Rails.non_production_env? + + self.current_user = user || User.system_user + + RequestStore.store[:current_user] = user + end + + def run + valid_file_numbers = {} + invalid_file_numbers = [] + + Manifest.all.find_each do |manifest| + file_number = manifest.file_number + + begin + level = bgs_service.sensitivity_level_for_veteran(file_number) + + key = "sensitivity_level_#{level}" + if !valid_file_numbers.key?(key) + valid_file_numbers[key] = [] + end + + valid_file_numbers[key].push(file_number) + rescue RuntimeError => e + invalid_file_numbers.push(manifest.file_number) + continue + end + end + + display_final_result(valid_file_numbers, invalid_file_numbers) + end + + private + + attr_accessor :current_user + + def display_final_result(valid_file_numbers, invalid_file_numbers) + puts "" + puts "================================================================================" + + puts "VALID FILE NUMBERS:" + valid_file_numbers.each do |k, v| + puts "#{k.upcase}" + + valid_file_numbers[k].each do |number| + puts "- #{number}" + end + end + + puts "\nINVALID FILE NUMBERS:" + invalid_file_numbers.each do |number| + puts "- #{number}" + end + + puts "================================================================================" + puts "" + end + + def bgs_service + @bgs_service ||= BGSService.new + end +end From b7bd88e2e99ddee7444e542b4e1240aa68514c9b Mon Sep 17 00:00:00 2001 From: Kevma50287 <104021955+Kevma50287@users.noreply.github.com> Date: Mon, 16 Sep 2024 11:40:35 -0400 Subject: [PATCH 18/36] Kev ma/appeals 58216 time adjust (#1692) * Adjusted UI_EXPIRY_HOURS based on deploy environment * Adjust API HOURS --- app/models/manifest.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/manifest.rb b/app/models/manifest.rb index ec715b6e8..93b9f0dc8 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -19,8 +19,8 @@ class Manifest < ApplicationRecord failed: 3 } - UI_HOURS_UNTIL_EXPIRY = 72 - API_HOURS_UNTIL_EXPIRY = 3 + UI_HOURS_UNTIL_EXPIRY = Rails.non_production_env? ? 0.25 : 72 + API_HOURS_UNTIL_EXPIRY = Rails.non_production_env? ? 0.25 : 3 SECONDS_TO_AUTO_UNLOCK = 5 From e0fff1f7be1e596f318a9571571c37091a30300b Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 16 Sep 2024 09:57:39 -0600 Subject: [PATCH 19/36] Fix Incorrect Keyword in Script (#1693) --- lib/scripts/find_valid_veteran_file_numbers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/scripts/find_valid_veteran_file_numbers.rb b/lib/scripts/find_valid_veteran_file_numbers.rb index 10ed086bf..59315ae0e 100644 --- a/lib/scripts/find_valid_veteran_file_numbers.rb +++ b/lib/scripts/find_valid_veteran_file_numbers.rb @@ -27,7 +27,7 @@ def run valid_file_numbers[key].push(file_number) rescue RuntimeError => e invalid_file_numbers.push(manifest.file_number) - continue + next end end From 668e5bba95c16c28ced7d96d4fd783c09c66e85a Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 16 Sep 2024 10:05:54 -0600 Subject: [PATCH 20/36] Handle All Possible Errors in Script (#1694) --- lib/scripts/find_valid_veteran_file_numbers.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/scripts/find_valid_veteran_file_numbers.rb b/lib/scripts/find_valid_veteran_file_numbers.rb index 59315ae0e..e5d9c5507 100644 --- a/lib/scripts/find_valid_veteran_file_numbers.rb +++ b/lib/scripts/find_valid_veteran_file_numbers.rb @@ -25,8 +25,8 @@ def run end valid_file_numbers[key].push(file_number) - rescue RuntimeError => e - invalid_file_numbers.push(manifest.file_number) + rescue => e + invalid_file_numbers.push("#{manifest.file_number} (#{e.message})") next end end From c6f26f923ccb2cf5f47588d10303cad4db832877 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 16 Sep 2024 11:24:10 -0600 Subject: [PATCH 21/36] Improve Script Output (#1695) - Sort output by sensitivity level. - Display total result count for each level. --- lib/scripts/find_valid_veteran_file_numbers.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/scripts/find_valid_veteran_file_numbers.rb b/lib/scripts/find_valid_veteran_file_numbers.rb index e5d9c5507..0a47a463a 100644 --- a/lib/scripts/find_valid_veteran_file_numbers.rb +++ b/lib/scripts/find_valid_veteran_file_numbers.rb @@ -43,17 +43,21 @@ def display_final_result(valid_file_numbers, invalid_file_numbers) puts "================================================================================" puts "VALID FILE NUMBERS:" - valid_file_numbers.each do |k, v| - puts "#{k.upcase}" + valid_file_numbers.sort.to_h.each do |k, v| + puts "#{k.upcase} (#{valid_file_numbers[k].count} total numbers)" valid_file_numbers[k].each do |number| - puts "- #{number}" + puts "#{number}" end + + puts "" end - puts "\nINVALID FILE NUMBERS:" + puts "================================================================================" + + puts "\nINVALID FILE NUMBERS (#{invalid_file_numbers.count} total numbers):" invalid_file_numbers.each do |number| - puts "- #{number}" + puts "#{number}" end puts "================================================================================" From 24f9c9cdd2c2289bfbdc289dc056e9bd332aa66e Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Sun, 22 Sep 2024 20:00:59 -0400 Subject: [PATCH 22/36] Updated ruby_claim_evidence_api gem with ref --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index e040b953e..9531074f3 100644 --- a/Gemfile +++ b/Gemfile @@ -57,8 +57,8 @@ gem "redis-namespace" gem "redis-rails", "~> 5.0.2" gem "redis-semaphore" gem "request_store" -gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", ref: "c56381d2bea2ffabe79bebbac6598f00450691ff" gem "rubyzip", ">= 1.3.0" +gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", ref: "c56381d2bea2ffabe79bebbac6598f00450691ff" gem "sass-rails", "~> 5.0" gem "sentry-raven" gem "shoryuken", "3.1.11" From d1c8f12b249e98b38610c5392ae201ecd71e9067 Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Wed, 25 Sep 2024 15:11:26 -0400 Subject: [PATCH 23/36] Update feature toogle to send_current_user_cred --- .../api/v1/application_controller.rb | 4 ++-- .../api/v2/application_controller.rb | 2 +- app/jobs/v2/save_files_in_s3_job.rb | 2 +- app/models/manifest.rb | 2 +- app/services/external_api/vbms_service.rb | 2 +- spec/models/manifest_spec.rb | 4 ++-- spec/requests/api/v2/manifests_spec.rb | 4 ++-- .../external_api/vbms_service_spec.rb | 24 +++++++++---------- 8 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index 726847927..b0e4eaea9 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -17,7 +17,7 @@ class Api::V1::ApplicationController < BaseController render json: { status: e.message, featureToggles: { - checkUserSensitivity: FeatureToggle.enabled?(:check_user_sensitivity) + checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred) } }, status: :forbidden end @@ -40,7 +40,7 @@ def forbidden(reason = "Forbidden: unspecified") render json: { status: reason, featureToggles: { - checkUserSensitivity: FeatureToggle.enabled?(:check_user_sensitivity) + checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred) } }, status: :forbidden end diff --git a/app/controllers/api/v2/application_controller.rb b/app/controllers/api/v2/application_controller.rb index d9929d69e..d420a4d9c 100644 --- a/app/controllers/api/v2/application_controller.rb +++ b/app/controllers/api/v2/application_controller.rb @@ -23,7 +23,7 @@ def verify_veteran_file_number return invalid_file_number unless bgs_service.valid_file_number?(file_number) - if FeatureToggle.enabled?(:check_user_sensitivity) + if FeatureToggle.enabled?(:send_current_user_cred) if sensitivity_checker.sensitivity_levels_compatible?( user: current_user, veteran_file_number: file_number diff --git a/app/jobs/v2/save_files_in_s3_job.rb b/app/jobs/v2/save_files_in_s3_job.rb index 694b73543..9fb854fe3 100644 --- a/app/jobs/v2/save_files_in_s3_job.rb +++ b/app/jobs/v2/save_files_in_s3_job.rb @@ -5,7 +5,7 @@ def perform(manifest_source, user_id) Raven.extra_context(manifest_source: manifest_source.id, user_id: user_id) # Set user for permission check if the user is blank - if FeatureToggle.enabled?(:check_user_sensitivity) && RequestStore[:current_user].blank? + if FeatureToggle.enabled?(:send_current_user_cred) && RequestStore[:current_user].blank? user = User.find(user_id) RequestStore.store[:current_user] = user end diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 93b9f0dc8..90bdc1902 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -31,7 +31,7 @@ def start! # Reset stale manifests. update!(fetched_files_status: :initialized) if ready_for_refresh? - if FeatureToggle.enabled?(:check_user_sensitivity) + if FeatureToggle.enabled?(:send_current_user_cred) if sensitivity_checker.sensitivity_levels_compatible?( user: user, veteran_file_number: file_number diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 23d74899e..2836fac8f 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -115,7 +115,7 @@ def self.vbms_client end def self.verify_user_veteran_access(veteran_file_number) - return if !FeatureToggle.enabled?(:check_user_sensitivity) + return if !FeatureToggle.enabled?(:send_current_user_cred) raise "User does not have permission to access this information" unless SensitivityChecker.new.sensitivity_levels_compatible?( diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index aa73aac28..f7f04cda9 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -9,7 +9,7 @@ before do Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) allow(V2::DownloadManifestJob).to receive(:perform_later) - expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) end @@ -47,7 +47,7 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) allow(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) - expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(true) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(true) end it "enqueues a job if the sensitivity check passes" do diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index acd7622d0..c71bf46a3 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -39,12 +39,12 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) - FeatureToggle.enable!(:check_user_sensitivity) + FeatureToggle.enable!(:send_current_user_cred) FeatureToggle.enable!(:skip_vva) end after do - FeatureToggle.disable!(:check_user_sensitivity) + FeatureToggle.disable!(:send_current_user_cred) FeatureToggle.disable!(:skip_vva) end diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 1c0318c48..067bab19a 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -10,9 +10,9 @@ end describe ".verify_user_veteran_access" do - context "with check_user_sensitivity feature flag enabled" do - before { FeatureToggle.enable!(:check_user_sensitivity) } - after { FeatureToggle.disable!(:check_user_sensitivity) } + context "with send_current_user_cred feature flag enabled" do + before { FeatureToggle.enable!(:send_current_user_cred) } + after { FeatureToggle.disable!(:send_current_user_cred) } let!(:user) do user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") @@ -35,8 +35,8 @@ end end - context "with check_user_sensitivity feature flag disabled" do - before { FeatureToggle.disable!(:check_user_sensitivity) } + context "with send_current_user_cred feature flag disabled" do + before { FeatureToggle.disable!(:send_current_user_cred) } it "does not check the user's sensitivity" do expect(mock_sensitivity_checker).not_to receive(:sensitivity_levels_compatible?) @@ -51,10 +51,10 @@ before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) - FeatureToggle.enable!(:check_user_sensitivity) + FeatureToggle.enable!(:send_current_user_cred) end - after { FeatureToggle.disable!(:check_user_sensitivity) } + after { FeatureToggle.disable!(:send_current_user_cred) } context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } @@ -79,7 +79,7 @@ it "calls the PagedDocuments SOAP API endpoint" do veteran_id = "123456789" - expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(true) expect(described_class).to receive(:vbms_client) @@ -100,7 +100,7 @@ it "calls the FindDocumentVersionReference SOAP API endpoint" do veteran_id = "123456789" - expect(FeatureToggle).to receive(:enabled?).with(:check_user_sensitivity).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(false) expect(VBMS::Requests::FindDocumentVersionReference).to receive(:new) @@ -118,10 +118,10 @@ before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) - FeatureToggle.enable!(:check_user_sensitivity) + FeatureToggle.enable!(:send_current_user_cred) end - after { FeatureToggle.disable!(:check_user_sensitivity) } + after { FeatureToggle.disable!(:send_current_user_cred) } context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } @@ -147,7 +147,7 @@ context "with use_ce_api feature toggle enabled" do before do FeatureToggle.enable!(:use_ce_api) - FeatureToggle.disable!(:check_user_sensitivity) + FeatureToggle.disable!(:send_current_user_cred) end after do From a6267cb70c3d3945c9ae69d6d2e18fa6201e34d0 Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 26 Sep 2024 12:08:09 -0400 Subject: [PATCH 24/36] Update feature toogle to send_current_user_cred_to_ce_api --- .../api/v1/application_controller.rb | 4 ++-- .../api/v2/application_controller.rb | 2 +- app/jobs/v2/save_files_in_s3_job.rb | 2 +- app/models/manifest.rb | 2 +- app/services/external_api/vbms_service.rb | 2 +- spec/models/manifest_spec.rb | 4 ++-- spec/requests/api/v2/manifests_spec.rb | 4 ++-- .../external_api/vbms_service_spec.rb | 24 +++++++++---------- 8 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index b0e4eaea9..6339fe239 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -17,7 +17,7 @@ class Api::V1::ApplicationController < BaseController render json: { status: e.message, featureToggles: { - checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred) + checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) } }, status: :forbidden end @@ -40,7 +40,7 @@ def forbidden(reason = "Forbidden: unspecified") render json: { status: reason, featureToggles: { - checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred) + checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) } }, status: :forbidden end diff --git a/app/controllers/api/v2/application_controller.rb b/app/controllers/api/v2/application_controller.rb index d420a4d9c..dc0b54fe1 100644 --- a/app/controllers/api/v2/application_controller.rb +++ b/app/controllers/api/v2/application_controller.rb @@ -23,7 +23,7 @@ def verify_veteran_file_number return invalid_file_number unless bgs_service.valid_file_number?(file_number) - if FeatureToggle.enabled?(:send_current_user_cred) + if FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) if sensitivity_checker.sensitivity_levels_compatible?( user: current_user, veteran_file_number: file_number diff --git a/app/jobs/v2/save_files_in_s3_job.rb b/app/jobs/v2/save_files_in_s3_job.rb index 9fb854fe3..1ac7a7629 100644 --- a/app/jobs/v2/save_files_in_s3_job.rb +++ b/app/jobs/v2/save_files_in_s3_job.rb @@ -5,7 +5,7 @@ def perform(manifest_source, user_id) Raven.extra_context(manifest_source: manifest_source.id, user_id: user_id) # Set user for permission check if the user is blank - if FeatureToggle.enabled?(:send_current_user_cred) && RequestStore[:current_user].blank? + if FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) && RequestStore[:current_user].blank? user = User.find(user_id) RequestStore.store[:current_user] = user end diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 90bdc1902..f103261e4 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -31,7 +31,7 @@ def start! # Reset stale manifests. update!(fetched_files_status: :initialized) if ready_for_refresh? - if FeatureToggle.enabled?(:send_current_user_cred) + if FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) if sensitivity_checker.sensitivity_levels_compatible?( user: user, veteran_file_number: file_number diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 2836fac8f..6b5f472f4 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -115,7 +115,7 @@ def self.vbms_client end def self.verify_user_veteran_access(veteran_file_number) - return if !FeatureToggle.enabled?(:send_current_user_cred) + return if !FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) raise "User does not have permission to access this information" unless SensitivityChecker.new.sensitivity_levels_compatible?( diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index f7f04cda9..5d61fb042 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -9,7 +9,7 @@ before do Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) allow(V2::DownloadManifestJob).to receive(:perform_later) - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) end @@ -47,7 +47,7 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) allow(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(true) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(true) end it "enqueues a job if the sensitivity check passes" do diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index c71bf46a3..7e302ecbe 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -39,12 +39,12 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) - FeatureToggle.enable!(:send_current_user_cred) + FeatureToggle.enable!(:send_current_user_cred_to_ce_api) FeatureToggle.enable!(:skip_vva) end after do - FeatureToggle.disable!(:send_current_user_cred) + FeatureToggle.disable!(:send_current_user_cred_to_ce_api) FeatureToggle.disable!(:skip_vva) end diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 067bab19a..d30de7131 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -10,9 +10,9 @@ end describe ".verify_user_veteran_access" do - context "with send_current_user_cred feature flag enabled" do - before { FeatureToggle.enable!(:send_current_user_cred) } - after { FeatureToggle.disable!(:send_current_user_cred) } + context "with send_current_user_cred_to_ce_api feature flag enabled" do + before { FeatureToggle.enable!(:send_current_user_cred_to_ce_api) } + after { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } let!(:user) do user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") @@ -35,8 +35,8 @@ end end - context "with send_current_user_cred feature flag disabled" do - before { FeatureToggle.disable!(:send_current_user_cred) } + context "with send_current_user_cred_to_ce_api feature flag disabled" do + before { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } it "does not check the user's sensitivity" do expect(mock_sensitivity_checker).not_to receive(:sensitivity_levels_compatible?) @@ -51,10 +51,10 @@ before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) - FeatureToggle.enable!(:send_current_user_cred) + FeatureToggle.enable!(:send_current_user_cred_to_ce_api) end - after { FeatureToggle.disable!(:send_current_user_cred) } + after { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } @@ -79,7 +79,7 @@ it "calls the PagedDocuments SOAP API endpoint" do veteran_id = "123456789" - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(true) expect(described_class).to receive(:vbms_client) @@ -100,7 +100,7 @@ it "calls the FindDocumentVersionReference SOAP API endpoint" do veteran_id = "123456789" - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(false) expect(VBMS::Requests::FindDocumentVersionReference).to receive(:new) @@ -118,10 +118,10 @@ before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) - FeatureToggle.enable!(:send_current_user_cred) + FeatureToggle.enable!(:send_current_user_cred_to_ce_api) end - after { FeatureToggle.disable!(:send_current_user_cred) } + after { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } @@ -147,7 +147,7 @@ context "with use_ce_api feature toggle enabled" do before do FeatureToggle.enable!(:use_ce_api) - FeatureToggle.disable!(:send_current_user_cred) + FeatureToggle.disable!(:send_current_user_cred_to_ce_api) end after do From 2dc2210665e5bcd9962167116918a735c90135b5 Mon Sep 17 00:00:00 2001 From: Kevma50287 <104021955+Kevma50287@users.noreply.github.com> Date: Fri, 27 Sep 2024 13:36:20 -0400 Subject: [PATCH 25/36] Kev ma/appeals 59461 (#1701) * Removed send_user feature flag, combined with use_ce_api * Updated vbms service spec * Updated manifest spec for uat expiration hours * send user feature toggle combined with use_ce_api * Combined with ce_api feature toggle * Update failing rspecs * Remove feature flag from method, wrapped method with all ce api calls * wrap ce_api related sensitivity changes * Fix failing specs, reverted to prior code outside of feature flag * update manifest expiry hours to not change in test + non prod * remove pry, reverted to previous test case * If user is blank, return * update front end error handling * Linting * Revert changes --- .../api/v1/application_controller.rb | 9 +-- .../api/v2/application_controller.rb | 2 +- .../api/v2/manifests_controller.rb | 19 ++++++- app/jobs/v2/save_files_in_s3_job.rb | 2 +- app/models/manifest.rb | 6 +- app/services/external_api/vbms_service.rb | 11 ++-- client/src/apiActions.jsx | 2 +- config/initializers/deploy_env.rb | 4 ++ spec/models/manifest_spec.rb | 4 +- spec/requests/api/v2/manifests_spec.rb | 8 +-- .../external_api/vbms_service_spec.rb | 56 +++++++------------ 11 files changed, 57 insertions(+), 66 deletions(-) diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index 6339fe239..b060275e8 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -17,7 +17,7 @@ class Api::V1::ApplicationController < BaseController render json: { status: e.message, featureToggles: { - checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) + use_ce_api: FeatureToggle.enabled?(:use_ce_api) } }, status: :forbidden end @@ -37,12 +37,7 @@ def sensitive_record end def forbidden(reason = "Forbidden: unspecified") - render json: { - status: reason, - featureToggles: { - checkUserSensitivity: FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) - } - }, status: :forbidden + render json: { status: reason }, status: :forbidden end def missing_header(header) diff --git a/app/controllers/api/v2/application_controller.rb b/app/controllers/api/v2/application_controller.rb index dc0b54fe1..2ec75dd8b 100644 --- a/app/controllers/api/v2/application_controller.rb +++ b/app/controllers/api/v2/application_controller.rb @@ -23,7 +23,7 @@ def verify_veteran_file_number return invalid_file_number unless bgs_service.valid_file_number?(file_number) - if FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) + if FeatureToggle.enabled?(:use_ce_api) if sensitivity_checker.sensitivity_levels_compatible?( user: current_user, veteran_file_number: file_number diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index ca586fd4d..aee1dd063 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -5,15 +5,27 @@ class Api::V2::ManifestsController < Api::V2::ApplicationController before_action :veteran_file_number, only: [:start, :refresh] def start - return if performed? + if FeatureToggle.enabled?(:use_ce_api) + return if performed? + + manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) + else + 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) + end - 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) end def refresh - manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) + manifest = if FeatureToggle.enabled?(:use_ce_api) + Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) + else + Manifest.find(params[:id]) + end return record_not_found if manifest.blank? return sensitive_record unless manifest.files_downloads.find_by(user: current_user) @@ -27,6 +39,7 @@ def progress distribute_reads do files_download ||= FilesDownload.find_with_manifest(manifest_id: params[:id], user_id: current_user.id) end + return record_not_found unless files_download render json: json_manifests(files_download.manifest) diff --git a/app/jobs/v2/save_files_in_s3_job.rb b/app/jobs/v2/save_files_in_s3_job.rb index 1ac7a7629..91c603c8a 100644 --- a/app/jobs/v2/save_files_in_s3_job.rb +++ b/app/jobs/v2/save_files_in_s3_job.rb @@ -5,7 +5,7 @@ def perform(manifest_source, user_id) Raven.extra_context(manifest_source: manifest_source.id, user_id: user_id) # Set user for permission check if the user is blank - if FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) && RequestStore[:current_user].blank? + if FeatureToggle.enabled?(:use_ce_api) && RequestStore[:current_user].blank? user = User.find(user_id) RequestStore.store[:current_user] = user end diff --git a/app/models/manifest.rb b/app/models/manifest.rb index f103261e4..5755f2916 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -19,8 +19,8 @@ class Manifest < ApplicationRecord failed: 3 } - UI_HOURS_UNTIL_EXPIRY = Rails.non_production_env? ? 0.25 : 72 - API_HOURS_UNTIL_EXPIRY = Rails.non_production_env? ? 0.25 : 3 + UI_HOURS_UNTIL_EXPIRY = Rails.non_production_env? && Rails.non_test_env? ? 0.25 : 72 + API_HOURS_UNTIL_EXPIRY = Rails.non_production_env? && Rails.non_test_env? ? 0.25 : 3 SECONDS_TO_AUTO_UNLOCK = 5 @@ -31,7 +31,7 @@ def start! # Reset stale manifests. update!(fetched_files_status: :initialized) if ready_for_refresh? - if FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) + if FeatureToggle.enabled?(:use_ce_api) if sensitivity_checker.sensitivity_levels_compatible?( user: user, veteran_file_number: file_number diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 6b5f472f4..53742d4b5 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -24,11 +24,10 @@ def self.fetch_documents_for(download) end def self.v2_fetch_documents_for(veteran_file_number) - verify_user_veteran_access(veteran_file_number) - documents = [] if FeatureToggle.enabled?(:use_ce_api) + verify_user_veteran_access(veteran_file_number) response = VeteranFileFetcher.fetch_veteran_file_list(veteran_file_number: veteran_file_number) documents = process_fetch_veteran_file_list_response(response) elsif FeatureToggle.enabled?(:vbms_pagination, user: RequestStore[:current_user]) @@ -44,11 +43,10 @@ def self.v2_fetch_documents_for(veteran_file_number) end def self.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_date_range = Time.zone.now) - verify_user_veteran_access(veteran_file_number) - documents = [] if FeatureToggle.enabled?(:use_ce_api) + verify_user_veteran_access(veteran_file_number) response = VeteranFileFetcher.fetch_veteran_file_list_by_date_range( veteran_file_number: veteran_file_number, begin_date_range: begin_date_range, @@ -71,9 +69,8 @@ def self.fetch_document_file(document) end def self.v2_fetch_document_file(document) - verify_user_veteran_access(document.file_number) - if FeatureToggle.enabled?(:use_ce_api) + verify_user_veteran_access(document.file_number) # Not using #send_and_log_request because logging to MetricService implemeneted in CE API gem VeteranFileFetcher.get_document_content(doc_series_id: document.series_id) else @@ -115,7 +112,7 @@ def self.vbms_client end def self.verify_user_veteran_access(veteran_file_number) - return if !FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) + return if RequestStore[:current_user].blank? raise "User does not have permission to access this information" unless SensitivityChecker.new.sensitivity_levels_compatible?( diff --git a/client/src/apiActions.jsx b/client/src/apiActions.jsx index c4ad77905..b522ef656 100644 --- a/client/src/apiActions.jsx +++ b/client/src/apiActions.jsx @@ -181,7 +181,7 @@ export const startManifestFetch = (veteranId, csrfToken, redirectFunction) => (d redirectFunction(`/downloads/${manifestId}`); }, (err) => { - if (err.response.statusCode === 403 && err.response.body.featureToggles.checkUserSensitivity === true) { + if (err.response.statusCode === 403 && err.response.body.featureToggles?.use_ce_api === true) { dispatch(setShowUnauthorizedVeteranMessage(true)); } else { dispatch(setErrorMessage(buildErrorMessageFromResponse(err.response))); diff --git a/config/initializers/deploy_env.rb b/config/initializers/deploy_env.rb index 76d8a87dd..7df5ee382 100644 --- a/config/initializers/deploy_env.rb +++ b/config/initializers/deploy_env.rb @@ -27,4 +27,8 @@ def self.deploy_env def self.non_production_env? deploy_env == :uat || deploy_env == :test || deploy_env == :development end + + def self.non_test_env? + deploy_env =! :test + end end diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index 5d61fb042..dd4c2ecc6 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -9,7 +9,7 @@ before do Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) allow(V2::DownloadManifestJob).to receive(:perform_later) - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) end @@ -47,7 +47,7 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) allow(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(true) + expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(true) end it "enqueues a job if the sensitivity check passes" do diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index 7e302ecbe..73d2e7a83 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -39,18 +39,18 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) - FeatureToggle.enable!(:send_current_user_cred_to_ce_api) + FeatureToggle.enable!(:use_ce_api) FeatureToggle.enable!(:skip_vva) end after do - FeatureToggle.disable!(:send_current_user_cred_to_ce_api) + FeatureToggle.disable!(:use_ce_api) 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?).exactly(3).times + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests", params: nil, headers: headers @@ -59,7 +59,7 @@ end it "allows access to the refresh action" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).exactly(3).times + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests/#{manifest.id}", params: nil, headers: headers diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index d30de7131..46031d4a9 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -10,39 +10,24 @@ end describe ".verify_user_veteran_access" do - context "with send_current_user_cred_to_ce_api feature flag enabled" do - before { FeatureToggle.enable!(:send_current_user_cred_to_ce_api) } - after { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } - - let!(:user) do - user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") - RequestStore.store[:current_user] = user - end - - it "checks the user's sensitivity" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) - .with(user: user, veteran_file_number: "123456789").and_return(true) - - described.verify_user_veteran_access("123456789") - end + let!(:user) do + user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") + RequestStore.store[:current_user] = user + end - it "raises an exception when the sensitivity level is not compatible" do - expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) - .with(user: user, veteran_file_number: "123456789").and_return(false) + it "checks the user's sensitivity" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "123456789").and_return(true) - expect { described.verify_user_veteran_access("123456789") } - .to raise_error(RuntimeError, "User does not have permission to access this information") - end + described.verify_user_veteran_access("123456789") end - context "with send_current_user_cred_to_ce_api feature flag disabled" do - before { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } + it "raises an exception when the sensitivity level is not compatible" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: "123456789").and_return(false) - it "does not check the user's sensitivity" do - expect(mock_sensitivity_checker).not_to receive(:sensitivity_levels_compatible?) - - described.verify_user_veteran_access("123456789") - end + expect { described.verify_user_veteran_access("123456789") } + .to raise_error(RuntimeError, "User does not have permission to access this information") end end @@ -51,10 +36,10 @@ before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) - FeatureToggle.enable!(:send_current_user_cred_to_ce_api) + FeatureToggle.enable!(:use_ce_api) end - after { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } + after { FeatureToggle.disable!(:use_ce_api) } context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } @@ -79,7 +64,6 @@ it "calls the PagedDocuments SOAP API endpoint" do veteran_id = "123456789" - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(true) expect(described_class).to receive(:vbms_client) @@ -100,7 +84,6 @@ it "calls the FindDocumentVersionReference SOAP API endpoint" do veteran_id = "123456789" - expect(FeatureToggle).to receive(:enabled?).with(:send_current_user_cred_to_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(false) expect(VBMS::Requests::FindDocumentVersionReference).to receive(:new) @@ -118,10 +101,10 @@ before do allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) - FeatureToggle.enable!(:send_current_user_cred_to_ce_api) + FeatureToggle.enable!(:use_ce_api) end - after { FeatureToggle.disable!(:send_current_user_cred_to_ce_api) } + after { FeatureToggle.disable!(:use_ce_api) } context "with use_ce_api feature toggle enabled" do before { FeatureToggle.enable!(:use_ce_api) } @@ -147,7 +130,6 @@ context "with use_ce_api feature toggle enabled" do before do FeatureToggle.enable!(:use_ce_api) - FeatureToggle.disable!(:send_current_user_cred_to_ce_api) end after do @@ -195,7 +177,7 @@ expect(ExceptionLogger).not_to receive(:capture) result = described.process_fetch_veteran_file_list_response(nil) - expect(result.count).to eq 1 + expect(result.count).to eq 1 end end @@ -206,7 +188,7 @@ expect(ExceptionLogger).to receive(:capture).with(instance_of(RuntimeError)) result = described.process_fetch_veteran_file_list_response(nil) - expect(result).to eq [] + expect(result).to eq [] end end end From 04a2b05f2c286697e66845539d6fe6bc3ebf066d Mon Sep 17 00:00:00 2001 From: Kevma50287 <104021955+Kevma50287@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:59:02 -0400 Subject: [PATCH 26/36] typo fix (#1702) --- config/initializers/deploy_env.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/deploy_env.rb b/config/initializers/deploy_env.rb index 7df5ee382..7e8d86196 100644 --- a/config/initializers/deploy_env.rb +++ b/config/initializers/deploy_env.rb @@ -29,6 +29,6 @@ def self.non_production_env? end def self.non_test_env? - deploy_env =! :test + deploy_env != :test end end From bf72460bf28c165fdc6e201279e374295f0e08e1 Mon Sep 17 00:00:00 2001 From: Kevma50287 <104021955+Kevma50287@users.noreply.github.com> Date: Mon, 30 Sep 2024 16:41:39 -0400 Subject: [PATCH 27/36] Kev ma/appeals 58827 v2 (#1703) * Change type_description to mapping * Updated rspecs to handle edge case --- app/services/json_api_response_adapter.rb | 6 ++- .../json_api_response_adapter_spec.rb | 12 +++++ .../ce_api_folders_files_search_unknown.json | 47 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 spec/support/api_responses/ce_api_folders_files_search_unknown.json diff --git a/app/services/json_api_response_adapter.rb b/app/services/json_api_response_adapter.rb index 122659070..0907ca133 100644 --- a/app/services/json_api_response_adapter.rb +++ b/app/services/json_api_response_adapter.rb @@ -3,6 +3,8 @@ # Translates JSON API responses into a format that's compatible with the legacy SOAP responses expected # by most of caseflow-efolder class JsonApiResponseAdapter + include Caseflow::DocumentTypes + def adapt_v2_fetch_documents_for(json_response) json_response = normalize_json_response(json_response) @@ -45,7 +47,9 @@ def v2_fetch_documents_file_response(file_json) document_id: "{#{file_json['currentVersionUuid'].upcase}}", series_id: "{#{file_json['uuid'].upcase}}", version: "1", - type_description: provider_data["subject"], + # CE Api doesn't provide document category type desciption. + # Use Caseflow document type mapping instead, and if not found then "Unknown" + type_description: TYPES[provider_data["documentTypeId"]] || TYPES[10], type_id: provider_data["documentTypeId"], doc_type: provider_data["documentTypeId"], subject: provider_data["subject"], diff --git a/spec/services/json_api_response_adapter_spec.rb b/spec/services/json_api_response_adapter_spec.rb index 905bea885..e8e60977e 100644 --- a/spec/services/json_api_response_adapter_spec.rb +++ b/spec/services/json_api_response_adapter_spec.rb @@ -36,12 +36,24 @@ expect(parsed.length).to eq 2 expect(parsed[0].document_id).to eq "{03223945-468B-4E8A-B79B-82FA73C2D2D9}" + expect(parsed[0].type_description).to eq "Rating Decision - Codesheet" expect(parsed[0].received_at).to eq "2018/03/08" expect(parsed[0].mime_type).to eq "application/pdf" expect(parsed[1].document_id).to eq "{7D6AFD8C-3BF7-4224-93AE-E1F07AC43C71}" + expect(parsed[1].type_description).to eq "Rating Decision - Narrative" expect(parsed[1].received_at).to eq "2018/12/08" expect(parsed[1].mime_type).to eq "application/pdf" end + + it "returns type description 'Unknown' if mapping is not found in Caseflow" do + file = File.open(Rails.root.join("spec/support/api_responses/ce_api_folders_files_search_unknown.json")) + data_hash = JSON.parse(File.read(file)) + file.close + + parsed = described.adapt_v2_fetch_documents_for(data_hash) + + expect(parsed[0].type_description).to eq "UNKNOWN" + end end end diff --git a/spec/support/api_responses/ce_api_folders_files_search_unknown.json b/spec/support/api_responses/ce_api_folders_files_search_unknown.json new file mode 100644 index 000000000..cfb706a48 --- /dev/null +++ b/spec/support/api_responses/ce_api_folders_files_search_unknown.json @@ -0,0 +1,47 @@ +{ + "page": { + "totalPages": 1, + "requestedResultsPerPage": 20, + "currentPage": 1, + "totalResults": 1 + }, + "files": [ + { + "owner": { + "type": "VETERAN", + "id": "123456789" + }, + "uuid": "23e7ae3b-5489-4fa9-b6e3-b1f953287138", + "currentVersionUuid": "03223945-468b-4e8a-b79b-82fa73c2d2d9", + "currentVersion": { + "systemData": { + "uploadedDateTime": "2018-03-08T14:21:40", + "contentName": "20180308091849 - codesheet.pdf", + "mimeType": "application/pdf", + "uploadSource": "RATING" + }, + "providerData": { + "subject": "Rating Decision - Codesheet", + "documentTypeId": 999111, + "ocrStatus": "Failure to Process", + "newMail": false, + "bookmarks": { + "VBA": { + "isDefaultRealm": true + } + }, + "systemSource": "RATING", + "isAnnotated": false, + "modifiedDateTime": "2018-03-08T14:21:40", + "numberOfContentions": 0, + "readByCurrentUser": false, + "dateVaReceivedDocument": "2018-03-08", + "hasContentionAnnotations": false, + "contentSource": "VBMS-R", + "actionable": false, + "lastOpenedDocument": false + } + } + } + ] +} From ca2f5254a85e0da604b9e0dbd0f06f1f4ffec1bf Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 15 Aug 2024 15:58:26 -0400 Subject: [PATCH 28/36] Updated ruby_claim_evidence_api gem --- Gemfile.lock | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 947d7de76..5e4046556 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -309,7 +309,7 @@ GEM mini_magick (4.11.0) mini_mime (1.1.5) mini_portile2 (2.8.7) - minitest (5.25.1) + minitest (5.25.0) moment_timezone-rails (0.5.14) momentjs-rails (~> 2.15.1) momentjs-rails (2.15.1) @@ -508,6 +508,7 @@ GEM request_store (1.5.0) rack (>= 1.4) rexml (3.3.7) + strscan rspec (3.10.0) rspec-core (~> 3.10.0) rspec-expectations (~> 3.10.0) From fc9afd9faa0a49fcb43566731a3399db745435c7 Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 12 Sep 2024 08:34:55 -0400 Subject: [PATCH 29/36] Updated ruby_claim_evidence_api --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5e4046556..f0295aac4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -309,7 +309,7 @@ GEM mini_magick (4.11.0) mini_mime (1.1.5) mini_portile2 (2.8.7) - minitest (5.25.0) + minitest (5.25.1) moment_timezone-rails (0.5.14) momentjs-rails (~> 2.15.1) momentjs-rails (2.15.1) From 8ac2bfac3bd7f00f5a5f9a09b11dbf26c195eaa2 Mon Sep 17 00:00:00 2001 From: SanthiParakal133 <132940479+SanthiParakal133@users.noreply.github.com> Date: Wed, 9 Oct 2024 09:24:45 -0400 Subject: [PATCH 30/36] Deepak/appeals 59642 v1 (#1714) * pass user info to ceapi * Update specs * update branch name * fix rspec * update claim_evidence_request method * change ref to branch * revert x86 --------- Co-authored-by: youfoundmanesh --- Gemfile | 2 +- Gemfile.lock | 9 +-- app/jobs/v2/save_files_in_s3_job.rb | 2 +- app/services/external_api/vbms_service.rb | 19 ++++- .../external_api/vbms_service_spec.rb | 70 ++++++++++++++++++- 5 files changed, 90 insertions(+), 12 deletions(-) diff --git a/Gemfile b/Gemfile index 9531074f3..1d39039fc 100644 --- a/Gemfile +++ b/Gemfile @@ -58,7 +58,7 @@ gem "redis-rails", "~> 5.0.2" gem "redis-semaphore" gem "request_store" gem "rubyzip", ">= 1.3.0" -gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", ref: "c56381d2bea2ffabe79bebbac6598f00450691ff" +gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", branch: "current-user-mgmt" gem "sass-rails", "~> 5.0" gem "sentry-raven" gem "shoryuken", "3.1.11" diff --git a/Gemfile.lock b/Gemfile.lock index f0295aac4..31ff489b0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -74,8 +74,8 @@ GIT GIT remote: https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git - revision: c56381d2bea2ffabe79bebbac6598f00450691ff - ref: c56381d2bea2ffabe79bebbac6598f00450691ff + revision: 9e667a1e626cbaf29f3f759d93e93e1afb13ab16 + branch: current-user-mgmt specs: ruby_claim_evidence_api (0.1.0) activesupport @@ -308,7 +308,6 @@ GEM mime-types-data (3.2020.1104) mini_magick (4.11.0) mini_mime (1.1.5) - mini_portile2 (2.8.7) minitest (5.25.1) moment_timezone-rails (0.5.14) momentjs-rails (~> 2.15.1) @@ -330,7 +329,6 @@ GEM net-protocol nio4r (2.7.3) nokogiri (1.15.6) - mini_portile2 (~> 2.8.2) racc (~> 1.4) nori (2.6.0) omniauth (1.9.1) @@ -507,8 +505,7 @@ GEM regexp_parser (2.8.3) request_store (1.5.0) rack (>= 1.4) - rexml (3.3.7) - strscan + rexml (3.3.8) rspec (3.10.0) rspec-core (~> 3.10.0) rspec-expectations (~> 3.10.0) diff --git a/app/jobs/v2/save_files_in_s3_job.rb b/app/jobs/v2/save_files_in_s3_job.rb index 91c603c8a..b63a49246 100644 --- a/app/jobs/v2/save_files_in_s3_job.rb +++ b/app/jobs/v2/save_files_in_s3_job.rb @@ -1,7 +1,7 @@ class V2::SaveFilesInS3Job < ApplicationJob queue_as :low_priority - def perform(manifest_source, user_id) + def perform(manifest_source, user_id = nil) Raven.extra_context(manifest_source: manifest_source.id, user_id: user_id) # Set user for permission check if the user is blank diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 53742d4b5..62efe5ce8 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -28,7 +28,10 @@ def self.v2_fetch_documents_for(veteran_file_number) if FeatureToggle.enabled?(:use_ce_api) verify_user_veteran_access(veteran_file_number) - response = VeteranFileFetcher.fetch_veteran_file_list(veteran_file_number: veteran_file_number) + response = VeteranFileFetcher.fetch_veteran_file_list( + veteran_file_number: veteran_file_number, + claim_evidence_request: claim_evidence_request + ) documents = process_fetch_veteran_file_list_response(response) elsif FeatureToggle.enabled?(:vbms_pagination, user: RequestStore[:current_user]) service = VBMS::Service::PagedDocuments.new(client: vbms_client) @@ -49,6 +52,7 @@ def self.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_da verify_user_veteran_access(veteran_file_number) response = VeteranFileFetcher.fetch_veteran_file_list_by_date_range( veteran_file_number: veteran_file_number, + claim_evidence_request: claim_evidence_request, begin_date_range: begin_date_range, end_date_range: end_date_range ) @@ -72,7 +76,7 @@ def self.v2_fetch_document_file(document) if FeatureToggle.enabled?(:use_ce_api) verify_user_veteran_access(document.file_number) # Not using #send_and_log_request because logging to MetricService implemeneted in CE API gem - VeteranFileFetcher.get_document_content(doc_series_id: document.series_id) + VeteranFileFetcher.get_document_content(doc_series_id: document.series_id, claim_evidence_request: claim_evidence_request) else request = VBMS::Requests::GetDocumentContent.new(document.document_id) result = send_and_log_request(document.document_id, request) @@ -132,4 +136,15 @@ def self.process_fetch_veteran_file_list_response(response) documents || [] end + + def self.claim_evidence_request + ClaimEvidenceRequest.new( + user_css_id: allow_user_info? ? RequestStore[:current_user].css_id : ENV['CLAIM_EVIDENCE_VBMS_USER'], + station_id: allow_user_info? ? RequestStore[:current_user].station_id : ENV['CLAIM_EVIDENCE_STATION_ID'] + ) + end + + def self.allow_user_info? + RequestStore[:current_user].present? && FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) + end end diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 46031d4a9..d00a25625 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -48,7 +48,8 @@ it "calls the CE API" do veteran_id = "123456789" - expect(VeteranFileFetcher).to receive(:fetch_veteran_file_list).with(veteran_file_number: veteran_id) + expect(VeteranFileFetcher).to receive(:fetch_veteran_file_list) + .with(veteran_file_number: veteran_id, claim_evidence_request: instance_of(ClaimEvidenceRequest)) expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).and_return([]) described.v2_fetch_documents_for(veteran_id) @@ -117,6 +118,7 @@ expect(VeteranFileFetcher).to receive(:fetch_veteran_file_list_by_date_range) .with( veteran_file_number: veteran_file_number, + claim_evidence_request: instance_of(ClaimEvidenceRequest), begin_date_range: begin_date_range, end_date_range: end_date_range ) @@ -153,7 +155,7 @@ it "calls the CE API" do expect(VeteranFileFetcher) .to receive(:get_document_content) - .with(doc_series_id: fake_record.series_id) + .with(doc_series_id: fake_record.series_id, claim_evidence_request: instance_of(ClaimEvidenceRequest)) .and_return("Pdf Byte String") described.v2_fetch_document_file(fake_record) @@ -192,4 +194,68 @@ end end end + + describe ".claim_evidence_request" do + context "with send_current_user_cred_to_ce_api feature toggle enabled" do + before { FeatureToggle.enable!(:send_current_user_cred_to_ce_api) } + + context "when current_user is set in the RequestStore" do + let!(:user) do + user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") + RequestStore[:current_user] = user + end + + it "returns user credentials" do + result = described.claim_evidence_request + + expect(result.user_css_id).to eq user.css_id + expect(result.station_id).to eq user.station_id + end + end + + context "when current_user is NOT set in the RequestStore" do + before do + RequestStore[:current_user] = nil + ENV["CLAIM_EVIDENCE_VBMS_USER"] = "CSS_123" + ENV["CLAIM_EVIDENCE_STATION_ID"] = "123" + end + it "returns system credentials" do + result = described.claim_evidence_request + expect(result.user_css_id).to eq "CSS_123" + expect(result.station_id).to eq "123" + end + end + end + + context "with send_current_user_cred_to_ce_api feature toggle disabled" do + before do + FeatureToggle.disable!(:send_current_user_cred_to_ce_api) + ENV["CLAIM_EVIDENCE_VBMS_USER"] = "CSS_999" + ENV["CLAIM_EVIDENCE_STATION_ID"] = "999" + end + + context "when current_user is set in the RequestStore" do + let(:user) do + user = create(:user) + RequestStore[:current_user] = user + end + + it "returns system credentials" do + result = described.claim_evidence_request + + expect(result.user_css_id).to eq "CSS_999" + expect(result.station_id).to eq "999" + end + end + + context "when current_user is NOT set in the RequestStore" do + before { RequestStore[:current_user] = nil } + it "returns system credentials" do + result = described.claim_evidence_request + expect(result.user_css_id).to eq "CSS_999" + expect(result.station_id).to eq "999" + end + end + end + end end From d88b8ab1493d080ecdc1fb8a1d484ef41b31547c Mon Sep 17 00:00:00 2001 From: Alex Smith <2421172+tradesmanhelix@users.noreply.github.com> Date: Tue, 22 Oct 2024 14:17:35 -0600 Subject: [PATCH 31/36] Improve Claim Evidence API Error Logging (#1723) * Add CE API Error Handler Class * Update VBMSService and SensitivityChecker Classes with Error Handling * Update VBMSService with CE API Error Handling * Fix Bad Method Signature * Fix More Spec Failures --- .../api/v1/application_controller.rb | 19 ++- .../claim_evidence_api_error_handler.rb | 31 +++++ app/services/external_api/vbms_service.rb | 58 +++++++-- app/services/sensitivity_checker.rb | 23 +++- spec/requests/api/v2/manifests_spec.rb | 4 + .../claim_evidence_api_error_handler_spec.rb | 46 +++++++ .../external_api/vbms_service_spec.rb | 114 +++++++++++++++--- spec/services/sensitivity_checker_spec.rb | 43 ++++++- 8 files changed, 305 insertions(+), 33 deletions(-) create mode 100644 app/services/error_handlers/claim_evidence_api_error_handler.rb create mode 100644 spec/services/error_handlers/claim_evidence_api_error_handler_spec.rb diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index b060275e8..600c26546 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -14,11 +14,22 @@ class Api::V1::ApplicationController < BaseController end rescue_from BGS::SensitivityLevelCheckFailure do |e| + current_user = RequestStore[:current_user] + user_sensitivity_level = if current_user.present? + SensitivityChecker.new.sensitivity_level_for_user(current_user) + else + "User is not set in the RequestStore" + end + + error_details = { + user_css_id: current_user&.css_id || "User is not set in the RequestStore", + user_sensitivity_level: user_sensitivity_level, + error_uuid: SecureRandom.uuid + } + ErrorHandlers::ClaimEvidenceApiErrorHandler.new.handle_error(error: e, error_details: error_details) + render json: { - status: e.message, - featureToggles: { - use_ce_api: FeatureToggle.enabled?(:use_ce_api) - } + status: e.message }, status: :forbidden end diff --git a/app/services/error_handlers/claim_evidence_api_error_handler.rb b/app/services/error_handlers/claim_evidence_api_error_handler.rb new file mode 100644 index 000000000..f71d07c0b --- /dev/null +++ b/app/services/error_handlers/claim_evidence_api_error_handler.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class ErrorHandlers::ClaimEvidenceApiErrorHandler + def handle_error(error:, error_details:) + report_error_to_sentry(error: error, error_details: error_details) + end + + private + + def report_error_to_sentry(error:, error_details:) + Raven.capture_exception( + error, + tags: { feature: "claim_evidence_api" }, + extra: { + feature_toggle_enabled_use_ce_api: use_ce_api?, + feature_toggle_enabled_send_current_user_cred_to_ce_api: send_current_user_cred_to_ce_api?, + user_css_id: error_details[:user_css_id], + user_sensitivity_level: error_details[:user_sensitivity_level], + error_uuid: error_details[:error_uuid] + } + ) + end + + def send_current_user_cred_to_ce_api? + FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) + end + + def use_ce_api? + FeatureToggle.enabled?(:use_ce_api) + end +end diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index 62efe5ce8..a02631e70 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -28,9 +28,10 @@ def self.v2_fetch_documents_for(veteran_file_number) if FeatureToggle.enabled?(:use_ce_api) verify_user_veteran_access(veteran_file_number) - response = VeteranFileFetcher.fetch_veteran_file_list( - veteran_file_number: veteran_file_number, - claim_evidence_request: claim_evidence_request + response = send_claim_evidence_request( + class_name: VeteranFileFetcher, + class_method: :fetch_veteran_file_list, + method_args: { veteran_file_number: veteran_file_number, claim_evidence_request: claim_evidence_request } ) documents = process_fetch_veteran_file_list_response(response) elsif FeatureToggle.enabled?(:vbms_pagination, user: RequestStore[:current_user]) @@ -50,11 +51,15 @@ def self.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_da if FeatureToggle.enabled?(:use_ce_api) verify_user_veteran_access(veteran_file_number) - response = VeteranFileFetcher.fetch_veteran_file_list_by_date_range( - veteran_file_number: veteran_file_number, - claim_evidence_request: claim_evidence_request, - begin_date_range: begin_date_range, - end_date_range: end_date_range + response = send_claim_evidence_request( + class_name: VeteranFileFetcher, + class_method: :fetch_veteran_file_list_by_date_range, + method_args: { + veteran_file_number: veteran_file_number, + claim_evidence_request: claim_evidence_request, + begin_date_range: begin_date_range, + end_date_range: end_date_range + } ) documents = process_fetch_veteran_file_list_response(response) else @@ -76,7 +81,11 @@ def self.v2_fetch_document_file(document) if FeatureToggle.enabled?(:use_ce_api) verify_user_veteran_access(document.file_number) # Not using #send_and_log_request because logging to MetricService implemeneted in CE API gem - VeteranFileFetcher.get_document_content(doc_series_id: document.series_id, claim_evidence_request: claim_evidence_request) + send_claim_evidence_request( + class_name: VeteranFileFetcher, + class_method: :get_document_content, + method_args: { doc_series_id: document.series_id, claim_evidence_request: claim_evidence_request } + ) else request = VBMS::Requests::GetDocumentContent.new(document.document_id) result = send_and_log_request(document.document_id, request) @@ -131,7 +140,7 @@ def self.process_fetch_veteran_file_list_response(response) # We want to be notified of any API responses that are not parsable if documents.nil? ex = RuntimeError.new("API response could not be parsed: #{response}") - ExceptionLogger.capture(ex) + log_claim_evidence_error(ex) end documents || [] @@ -143,8 +152,35 @@ def self.claim_evidence_request station_id: allow_user_info? ? RequestStore[:current_user].station_id : ENV['CLAIM_EVIDENCE_STATION_ID'] ) end - + def self.allow_user_info? RequestStore[:current_user].present? && FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) end + + class << self + private + + def send_claim_evidence_request(class_name:, class_method:, method_args:) + class_name.public_send(class_method, **method_args) + rescue StandardError => e + log_claim_evidence_error(e) + + nil + end + + def log_claim_evidence_error(error) + current_user = RequestStore[:current_user] + user_sensitivity_level = if current_user.present? + SensitivityChecker.new.sensitivity_level_for_user(current_user) + else + "User is not set in the RequestStore" + end + error_details = { + user_css_id: current_user&.css_id || "User is not set in the RequestStore", + user_sensitivity_level: user_sensitivity_level, + error_uuid: SecureRandom.uuid + } + ErrorHandlers::ClaimEvidenceApiErrorHandler.new.handle_error(error: error, error_details: error_details) + end + end end diff --git a/app/services/sensitivity_checker.rb b/app/services/sensitivity_checker.rb index cd7b5e8f1..9ae583621 100644 --- a/app/services/sensitivity_checker.rb +++ b/app/services/sensitivity_checker.rb @@ -5,14 +5,35 @@ 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 => e - ExceptionLogger.capture(e) + report_error(e) false end + def sensitivity_level_for_user(user) + bgs_service.sensitivity_level_for_user(user) + rescue StandardError => e + report_error(e) + + nil + end + private def bgs_service @bgs_service ||= BGSService.new end + + def error_handler + @error_handler ||= ErrorHandlers::ClaimEvidenceApiErrorHandler.new + end + + def report_error(error) + error_details = { + user_css_id: RequestStore[:current_user]&.css_id || "User is not set in RequestStore", + user_sensitivity_level: "Error occurred in SensitivityChecker", + error_uuid: SecureRandom.uuid + } + error_handler.handle_error(error: error, error_details: error_details) + end end diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index 73d2e7a83..791b1399c 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -72,6 +72,8 @@ 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) + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_return(4) post "/api/v2/manifests", params: nil, headers: headers @@ -82,6 +84,8 @@ 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) + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_return(4) post "/api/v2/manifests/#{manifest.id}", params: nil, headers: headers diff --git a/spec/services/error_handlers/claim_evidence_api_error_handler_spec.rb b/spec/services/error_handlers/claim_evidence_api_error_handler_spec.rb new file mode 100644 index 000000000..73a3144f1 --- /dev/null +++ b/spec/services/error_handlers/claim_evidence_api_error_handler_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe ErrorHandlers::ClaimEvidenceApiErrorHandler do + subject(:described) { described_class.new } + + describe "#handle_error" do + let(:mock_sentry_client) { class_double(Raven) } + + before do + FeatureToggle.enable!(:use_ce_api) + FeatureToggle.enable!(:send_current_user_cred_to_ce_api) + end + + after do + FeatureToggle.disable!(:use_ce_api) + FeatureToggle.disable!(:send_current_user_cred_to_ce_api) + end + + it "sends the error to its registered clients" do + error = StandardError.new("Example CE API failure") + + expect(Raven).to receive(:capture_exception) + .with( + error, + tags: { feature: "claim_evidence_api" }, + extra: { + feature_toggle_enabled_use_ce_api: true, + feature_toggle_enabled_send_current_user_cred_to_ce_api: true, + user_css_id: "USER_12345", + user_sensitivity_level: 4, + error_uuid: "1234-1234-1234" + } + ) + + error_details = { + user_css_id: "USER_12345", + user_sensitivity_level: 4, + error_uuid: "1234-1234-1234" + } + + described.handle_error(error: error, error_details: error_details) + end + end +end diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index d00a25625..5d24a2574 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -3,9 +3,13 @@ describe ExternalApi::VBMSService do subject(:described) { described_class } + let(:mock_error_handler) { instance_double(ErrorHandlers::ClaimEvidenceApiErrorHandler) } + let(:mock_json_adapter) { instance_double(JsonApiResponseAdapter) } let(:mock_sensitivity_checker) { instance_double(SensitivityChecker, sensitivity_levels_compatible?: true) } before do + allow(ErrorHandlers::ClaimEvidenceApiErrorHandler).to receive(:new).and_return(mock_error_handler) + allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) end @@ -32,10 +36,7 @@ end describe ".v2_fetch_documents_for" do - let(:mock_json_adapter) { instance_double(JsonApiResponseAdapter) } - before do - allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) FeatureToggle.enable!(:use_ce_api) end @@ -98,10 +99,7 @@ end describe ".fetch_delta_documents_for" do - let(:mock_json_adapter) { instance_double(JsonApiResponseAdapter) } - before do - allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) FeatureToggle.enable!(:use_ce_api) end @@ -116,12 +114,12 @@ begin_date_range = "2024-05-10" end_date_range = "2024-05-10" expect(VeteranFileFetcher).to receive(:fetch_veteran_file_list_by_date_range) - .with( - veteran_file_number: veteran_file_number, - claim_evidence_request: instance_of(ClaimEvidenceRequest), - begin_date_range: begin_date_range, - end_date_range: end_date_range - ) + .with( + veteran_file_number: veteran_file_number, + claim_evidence_request: instance_of(ClaimEvidenceRequest), + begin_date_range: begin_date_range, + end_date_range: end_date_range + ) expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).and_return([]) described.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_date_range) end @@ -164,10 +162,7 @@ end describe ".process_fetch_veteran_file_list_response" do - let(:mock_json_adapter) { instance_double(JsonApiResponseAdapter) } - before do - allow(JsonApiResponseAdapter).to receive(:new).and_return(mock_json_adapter) FeatureToggle.enable!(:use_ce_api) end @@ -184,10 +179,26 @@ end context "when the adapted response is nil" do + let!(:user) do + user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") + RequestStore.store[:current_user] = user + end + it "logs an exception and returns an empty array" do expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).and_return(nil) expect(RuntimeError).to receive(:new).with(/API response could not be parsed/).and_call_original - expect(ExceptionLogger).to receive(:capture).with(instance_of(RuntimeError)) + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_return(4) + expect(SecureRandom).to receive(:uuid).and_return("1234") + expect(mock_error_handler).to receive(:handle_error) + .with( + error: instance_of(RuntimeError), + error_details: { + user_css_id: user.css_id, + user_sensitivity_level: 4, + error_uuid: "1234" + } + ) result = described.process_fetch_veteran_file_list_response(nil) expect(result).to eq [] @@ -258,4 +269,75 @@ end end end + + describe "error handling" do + let(:example_error) { StandardError.new("My test error") } + let(:veteran_file_number) { "123456789" } + + before do + FeatureToggle.enable!(:use_ce_api) + end + after { FeatureToggle.disable!(:use_ce_api) } + + context "when the current_user is set in the RequestStore" do + let!(:user) do + user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") + RequestStore.store[:current_user] = user + end + + it "reports errors to the ClaimEvidenceApiErrorHandler and includes the current_user details" do + expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?) + .with(user: user, veteran_file_number: veteran_file_number).and_return(true) + expect(VeteranFileFetcher).to receive(:fetch_veteran_file_list) + .with( + veteran_file_number: veteran_file_number, + claim_evidence_request: instance_of(ClaimEvidenceRequest) + ).and_raise(example_error) + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_return(4) + expect(SecureRandom).to receive(:uuid).and_return("1234") + expect(mock_error_handler).to receive(:handle_error) + .with( + error: example_error, + error_details: { + user_css_id: user.css_id, + user_sensitivity_level: 4, + error_uuid: "1234" + } + ) + expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).with(nil).and_return([]) + + response = described.v2_fetch_documents_for(veteran_file_number) + expect(response).to eq([]) + end + end + + context "when the current_user is NOT set in the RequestStore" do + before { RequestStore.store[:current_user] = nil } + + it "reports errors to the ClaimEvidenceApiErrorHandler" do + expect(mock_sensitivity_checker).not_to receive(:sensitivity_levels_compatible?) + expect(VeteranFileFetcher).to receive(:fetch_veteran_file_list) + .with( + veteran_file_number: veteran_file_number, + claim_evidence_request: instance_of(ClaimEvidenceRequest) + ).and_raise(example_error) + expect(mock_sensitivity_checker).not_to receive(:sensitivity_level_for_user) + expect(SecureRandom).to receive(:uuid).and_return("1234") + expect(mock_error_handler).to receive(:handle_error) + .with( + error: example_error, + error_details: { + user_css_id: "User is not set in the RequestStore", + user_sensitivity_level: "User is not set in the RequestStore", + error_uuid: "1234" + } + ) + expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).with(nil).and_return([]) + + response = described.v2_fetch_documents_for(veteran_file_number) + expect(response).to eq([]) + end + end + end end diff --git a/spec/services/sensitivity_checker_spec.rb b/spec/services/sensitivity_checker_spec.rb index d4670ea36..350c731e5 100644 --- a/spec/services/sensitivity_checker_spec.rb +++ b/spec/services/sensitivity_checker_spec.rb @@ -5,11 +5,43 @@ let(:user) { User.create(css_id: "Foo", station_id: "112") } let(:mock_sensitivity_checker) { instance_double(BGSService) } + let(:mock_error_handler) { instance_double(ErrorHandlers::ClaimEvidenceApiErrorHandler) } before do + allow(ErrorHandlers::ClaimEvidenceApiErrorHandler).to receive(:new).and_return(mock_error_handler) allow(BGSService).to receive(:new).and_return(mock_sensitivity_checker) end + describe "#sensitivity_level_for_user" do + it "returns the sensitivity level" do + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_return(5) + + expect(described.sensitivity_level_for_user(user)).to eq 5 + end + + context "when an exception is raised" do + it "reports the exception and returns nil" do + error = StandardError.new + + expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) + .with(user).and_raise(error) + expect(SecureRandom).to receive(:uuid).and_return("1234") + expect(mock_error_handler).to receive(:handle_error) + .with( + error: error, + error_details: { + user_css_id: "User is not set in RequestStore", + user_sensitivity_level: "Error occurred in SensitivityChecker", + error_uuid: "1234" + } + ) + + expect(described.sensitivity_level_for_user(user)).to eq nil + end + end + end + describe "#sensitivity_levels_compatible?" do context "when the sensitivity levels are compatible" do it "returns true" do @@ -39,7 +71,16 @@ expect(mock_sensitivity_checker).to receive(:sensitivity_level_for_user) .with(user).and_raise(error) - expect(ExceptionLogger).to receive(:capture).with(error) + expect(SecureRandom).to receive(:uuid).and_return("1234") + expect(mock_error_handler).to receive(:handle_error) + .with( + error: error, + error_details: { + user_css_id: "User is not set in RequestStore", + user_sensitivity_level: "Error occurred in SensitivityChecker", + error_uuid: "1234" + } + ) expect(described.sensitivity_levels_compatible?(user: user, veteran_file_number: "1234")).to eq false end From 98ba09a2b9cf4d1e36c5d4f9410f6e22441bdebb Mon Sep 17 00:00:00 2001 From: Alex Smith <2421172+tradesmanhelix@users.noreply.github.com> Date: Tue, 29 Oct 2024 08:00:04 -0600 Subject: [PATCH 32/36] Fix Logging Logic Errors (#1731) - Restore code we know is good. - Wrap in begin/rescue blocks and handle errors there. - Add feature toggle to ApplicationController response so frontend will display banners correctly. --- .../api/v1/application_controller.rb | 5 +- app/services/external_api/vbms_service.rb | 57 +++++++++---------- .../external_api/vbms_service_spec.rb | 2 - 3 files changed, 31 insertions(+), 33 deletions(-) diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index 600c26546..b8b082be2 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -29,7 +29,10 @@ class Api::V1::ApplicationController < BaseController ErrorHandlers::ClaimEvidenceApiErrorHandler.new.handle_error(error: e, error_details: error_details) render json: { - status: e.message + status: e.message, + featureToggles: { + use_ce_api: FeatureToggle.enabled?(:use_ce_api) + } }, status: :forbidden end diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index a02631e70..ca523154c 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -27,13 +27,16 @@ def self.v2_fetch_documents_for(veteran_file_number) documents = [] if FeatureToggle.enabled?(:use_ce_api) - verify_user_veteran_access(veteran_file_number) - response = send_claim_evidence_request( - class_name: VeteranFileFetcher, - class_method: :fetch_veteran_file_list, - method_args: { veteran_file_number: veteran_file_number, claim_evidence_request: claim_evidence_request } - ) - documents = process_fetch_veteran_file_list_response(response) + begin + verify_user_veteran_access(veteran_file_number) + response = VeteranFileFetcher.fetch_veteran_file_list( + veteran_file_number: veteran_file_number, + claim_evidence_request: claim_evidence_request + ) + documents = process_fetch_veteran_file_list_response(response) + rescue StandardError => e + log_claim_evidence_error(e) + end elsif FeatureToggle.enabled?(:vbms_pagination, user: RequestStore[:current_user]) service = VBMS::Service::PagedDocuments.new(client: vbms_client) documents = call_and_log_service(service: service, vbms_id: veteran_file_number)[:documents] @@ -50,18 +53,18 @@ def self.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_da documents = [] if FeatureToggle.enabled?(:use_ce_api) - verify_user_veteran_access(veteran_file_number) - response = send_claim_evidence_request( - class_name: VeteranFileFetcher, - class_method: :fetch_veteran_file_list_by_date_range, - method_args: { + begin + verify_user_veteran_access(veteran_file_number) + response = VeteranFileFetcher.fetch_veteran_file_list_by_date_range( veteran_file_number: veteran_file_number, claim_evidence_request: claim_evidence_request, begin_date_range: begin_date_range, end_date_range: end_date_range - } - ) - documents = process_fetch_veteran_file_list_response(response) + ) + documents = process_fetch_veteran_file_list_response(response) + rescue StandardError => e + log_claim_evidence_error(e) + end else request = VBMS::Requests::FindDocumentVersionReferenceByDateRange.new(veteran_file_number, begin_date_range, end_date_range) documents = send_and_log_request(veteran_file_number, request) @@ -79,13 +82,15 @@ def self.fetch_document_file(document) def self.v2_fetch_document_file(document) if FeatureToggle.enabled?(:use_ce_api) - verify_user_veteran_access(document.file_number) - # Not using #send_and_log_request because logging to MetricService implemeneted in CE API gem - send_claim_evidence_request( - class_name: VeteranFileFetcher, - class_method: :get_document_content, - method_args: { doc_series_id: document.series_id, claim_evidence_request: claim_evidence_request } - ) + begin + verify_user_veteran_access(document.file_number) + VeteranFileFetcher.get_document_content( + doc_series_id: document.series_id, + claim_evidence_request: claim_evidence_request + ) + rescue StandardError => e + log_claim_evidence_error(e) + end else request = VBMS::Requests::GetDocumentContent.new(document.document_id) result = send_and_log_request(document.document_id, request) @@ -160,14 +165,6 @@ def self.allow_user_info? class << self private - def send_claim_evidence_request(class_name:, class_method:, method_args:) - class_name.public_send(class_method, **method_args) - rescue StandardError => e - log_claim_evidence_error(e) - - nil - end - def log_claim_evidence_error(error) current_user = RequestStore[:current_user] user_sensitivity_level = if current_user.present? diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 5d24a2574..1fd3c2bc2 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -305,7 +305,6 @@ error_uuid: "1234" } ) - expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).with(nil).and_return([]) response = described.v2_fetch_documents_for(veteran_file_number) expect(response).to eq([]) @@ -333,7 +332,6 @@ error_uuid: "1234" } ) - expect(mock_json_adapter).to receive(:adapt_v2_fetch_documents_for).with(nil).and_return([]) response = described.v2_fetch_documents_for(veteran_file_number) expect(response).to eq([]) From 7013b9b697cc9ecff168909596abf180de84be27 Mon Sep 17 00:00:00 2001 From: youfoundmanesh <129548081+youfoundmanesh@users.noreply.github.com> Date: Wed, 13 Nov 2024 16:02:50 -0500 Subject: [PATCH 33/36] Update ruby_claim_evidence_api gem --- Gemfile | 2 +- Gemfile.lock | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 1d39039fc..d162b4d79 100644 --- a/Gemfile +++ b/Gemfile @@ -58,7 +58,7 @@ gem "redis-rails", "~> 5.0.2" gem "redis-semaphore" gem "request_store" gem "rubyzip", ">= 1.3.0" -gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", branch: "current-user-mgmt" +gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", ref: "4ed23adddb3f584d3a530ef64dcb9ea0cf0eb2b3" gem "sass-rails", "~> 5.0" gem "sentry-raven" gem "shoryuken", "3.1.11" diff --git a/Gemfile.lock b/Gemfile.lock index 31ff489b0..54272cf5b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -74,8 +74,8 @@ GIT GIT remote: https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git - revision: 9e667a1e626cbaf29f3f759d93e93e1afb13ab16 - branch: current-user-mgmt + revision: 4ed23adddb3f584d3a530ef64dcb9ea0cf0eb2b3 + ref: 4ed23adddb3f584d3a530ef64dcb9ea0cf0eb2b3 specs: ruby_claim_evidence_api (0.1.0) activesupport @@ -738,4 +738,4 @@ DEPENDENCIES zero_downtime_migrations BUNDLED WITH - 2.4.17 + 2.4.22 From 05a41c04ef994a77d2a398d87b9cd1055d0f9a18 Mon Sep 17 00:00:00 2001 From: youfoundmanesh <129548081+youfoundmanesh@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:41:26 -0500 Subject: [PATCH 34/36] Compatible with Zeitwrek autoloader --- app/controllers/base_controller.rb | 5 ----- app/exceptions/bgs_errors.rb | 9 --------- config/initializers/bgs.rb | 1 + 3 files changed, 1 insertion(+), 14 deletions(-) delete mode 100644 app/exceptions/bgs_errors.rb diff --git a/app/controllers/base_controller.rb b/app/controllers/base_controller.rb index b4850af82..5fbd3ee59 100644 --- a/app/controllers/base_controller.rb +++ b/app/controllers/base_controller.rb @@ -1,8 +1,3 @@ -# 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/exceptions/bgs_errors.rb b/app/exceptions/bgs_errors.rb deleted file mode 100644 index 082d4637a..000000000 --- a/app/exceptions/bgs_errors.rb +++ /dev/null @@ -1,9 +0,0 @@ -module BGS - class InvalidUsername < StandardError; end - class InvalidStation < StandardError; end - class InvalidApplication < StandardError; end - class NoActiveStations < StandardError; end - class NoCaseflowAccess < StandardError; end - class StationAssertionRequired < StandardError; end - class SensitivityLevelCheckFailure < StandardError; end -end diff --git a/config/initializers/bgs.rb b/config/initializers/bgs.rb index ddb1ef447..baee988a0 100644 --- a/config/initializers/bgs.rb +++ b/config/initializers/bgs.rb @@ -8,6 +8,7 @@ class InvalidApplication < StandardError; end class NoActiveStations < StandardError; end class NoCaseflowAccess < StandardError; end class StationAssertionRequired < StandardError; end + class SensitivityLevelCheckFailure < StandardError; end end end From 3146da8270babaca5ffccfec4cc28d157d70b744 Mon Sep 17 00:00:00 2001 From: SanthiParakal133 <132940479+SanthiParakal133@users.noreply.github.com> Date: Wed, 20 Nov 2024 10:42:46 -0500 Subject: [PATCH 35/36] Deepak/appeals 65021 (#1744) * update feature toggle for user specific * update specs --- app/controllers/api/v1/application_controller.rb | 2 +- app/controllers/api/v2/application_controller.rb | 2 +- app/controllers/api/v2/manifests_controller.rb | 8 ++++++-- app/jobs/v2/save_files_in_s3_job.rb | 2 +- app/models/manifest.rb | 2 +- .../claim_evidence_api_error_handler.rb | 2 +- app/services/external_api/vbms_service.rb | 15 +++++++++------ spec/models/manifest_spec.rb | 9 ++++++--- spec/services/external_api/vbms_service_spec.rb | 8 ++++---- 9 files changed, 30 insertions(+), 20 deletions(-) diff --git a/app/controllers/api/v1/application_controller.rb b/app/controllers/api/v1/application_controller.rb index b8b082be2..f737ac8ab 100644 --- a/app/controllers/api/v1/application_controller.rb +++ b/app/controllers/api/v1/application_controller.rb @@ -31,7 +31,7 @@ class Api::V1::ApplicationController < BaseController render json: { status: e.message, featureToggles: { - use_ce_api: FeatureToggle.enabled?(:use_ce_api) + use_ce_api: FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) } }, status: :forbidden end diff --git a/app/controllers/api/v2/application_controller.rb b/app/controllers/api/v2/application_controller.rb index 2ec75dd8b..09cb8ebe8 100644 --- a/app/controllers/api/v2/application_controller.rb +++ b/app/controllers/api/v2/application_controller.rb @@ -23,7 +23,7 @@ def verify_veteran_file_number return invalid_file_number unless bgs_service.valid_file_number?(file_number) - if FeatureToggle.enabled?(:use_ce_api) + if FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) if sensitivity_checker.sensitivity_levels_compatible?( user: current_user, veteran_file_number: file_number diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index aee1dd063..f3a98c0af 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -5,7 +5,7 @@ class Api::V2::ManifestsController < Api::V2::ApplicationController before_action :veteran_file_number, only: [:start, :refresh] def start - if FeatureToggle.enabled?(:use_ce_api) + if use_ce_api? return if performed? manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) @@ -21,7 +21,7 @@ def start end def refresh - manifest = if FeatureToggle.enabled?(:use_ce_api) + manifest = if use_ce_api? Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) else Manifest.find(params[:id]) @@ -51,6 +51,10 @@ def history private + def use_ce_api? + FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) + end + def veteran_file_number @veteran_file_number ||= verify_veteran_file_number end diff --git a/app/jobs/v2/save_files_in_s3_job.rb b/app/jobs/v2/save_files_in_s3_job.rb index b63a49246..0baaaf59a 100644 --- a/app/jobs/v2/save_files_in_s3_job.rb +++ b/app/jobs/v2/save_files_in_s3_job.rb @@ -5,7 +5,7 @@ def perform(manifest_source, user_id = nil) Raven.extra_context(manifest_source: manifest_source.id, user_id: user_id) # Set user for permission check if the user is blank - if FeatureToggle.enabled?(:use_ce_api) && RequestStore[:current_user].blank? + if FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) && RequestStore[:current_user].blank? user = User.find(user_id) RequestStore.store[:current_user] = user end diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 5755f2916..924fc1951 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -31,7 +31,7 @@ def start! # Reset stale manifests. update!(fetched_files_status: :initialized) if ready_for_refresh? - if FeatureToggle.enabled?(:use_ce_api) + if FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) if sensitivity_checker.sensitivity_levels_compatible?( user: user, veteran_file_number: file_number diff --git a/app/services/error_handlers/claim_evidence_api_error_handler.rb b/app/services/error_handlers/claim_evidence_api_error_handler.rb index f71d07c0b..20fbd4c6d 100644 --- a/app/services/error_handlers/claim_evidence_api_error_handler.rb +++ b/app/services/error_handlers/claim_evidence_api_error_handler.rb @@ -26,6 +26,6 @@ def send_current_user_cred_to_ce_api? end def use_ce_api? - FeatureToggle.enabled?(:use_ce_api) + FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) end end diff --git a/app/services/external_api/vbms_service.rb b/app/services/external_api/vbms_service.rb index ca523154c..7f41350e6 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -25,8 +25,7 @@ def self.fetch_documents_for(download) def self.v2_fetch_documents_for(veteran_file_number) documents = [] - - if FeatureToggle.enabled?(:use_ce_api) + if use_ce_api? begin verify_user_veteran_access(veteran_file_number) response = VeteranFileFetcher.fetch_veteran_file_list( @@ -52,7 +51,7 @@ def self.v2_fetch_documents_for(veteran_file_number) def self.fetch_delta_documents_for(veteran_file_number, begin_date_range, end_date_range = Time.zone.now) documents = [] - if FeatureToggle.enabled?(:use_ce_api) + if use_ce_api? begin verify_user_veteran_access(veteran_file_number) response = VeteranFileFetcher.fetch_veteran_file_list_by_date_range( @@ -81,7 +80,7 @@ def self.fetch_document_file(document) end def self.v2_fetch_document_file(document) - if FeatureToggle.enabled?(:use_ce_api) + if use_ce_api? begin verify_user_veteran_access(document.file_number) VeteranFileFetcher.get_document_content( @@ -153,8 +152,8 @@ def self.process_fetch_veteran_file_list_response(response) def self.claim_evidence_request ClaimEvidenceRequest.new( - user_css_id: allow_user_info? ? RequestStore[:current_user].css_id : ENV['CLAIM_EVIDENCE_VBMS_USER'], - station_id: allow_user_info? ? RequestStore[:current_user].station_id : ENV['CLAIM_EVIDENCE_STATION_ID'] + user_css_id: allow_user_info? ? RequestStore[:current_user].css_id : ENV["CLAIM_EVIDENCE_VBMS_USER"], + station_id: allow_user_info? ? RequestStore[:current_user].station_id : ENV["CLAIM_EVIDENCE_STATION_ID"] ) end @@ -162,6 +161,10 @@ def self.allow_user_info? RequestStore[:current_user].present? && FeatureToggle.enabled?(:send_current_user_cred_to_ce_api) end + def self.use_ce_api? + FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) + end + class << self private diff --git a/spec/models/manifest_spec.rb b/spec/models/manifest_spec.rb index dd4c2ecc6..6148e2ef7 100644 --- a/spec/models/manifest_spec.rb +++ b/spec/models/manifest_spec.rb @@ -1,6 +1,9 @@ describe Manifest do describe "#start!" do - let(:user) { User.create(css_id: "Foo", station_id: "112") } + let(:user) do + user = User.create(css_id: "VSO", station_id: "283") + RequestStore.store[:current_user] = user + end let(:manifest) { Manifest.create(file_number: "1234", user: user) } subject { manifest.start! } @@ -9,7 +12,7 @@ before do Timecop.freeze(Time.utc(2015, 12, 2, 17, 0, 0)) allow(V2::DownloadManifestJob).to receive(:perform_later) - expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api, user: user).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) end @@ -47,7 +50,7 @@ before do allow(SensitivityChecker).to receive(:new).and_return(mock_sensitivity_checker) allow(FeatureToggle).to receive(:enabled?).with(:skip_vva).and_return(false) - expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(true) + expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api, user: user).and_return(true) end it "enqueues a job if the sensitivity check passes" do diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 1fd3c2bc2..9bc931dd7 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true - + describe ExternalApi::VBMSService do subject(:described) { described_class } @@ -66,7 +66,7 @@ it "calls the PagedDocuments SOAP API endpoint" do veteran_id = "123456789" - expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api, user: user).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(true) expect(described_class).to receive(:vbms_client) expect(VBMS::Service::PagedDocuments).to receive(:new).and_return(:test_service) @@ -78,6 +78,7 @@ end context "with no feature toggles enabled" do + let!(:user) do user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") RequestStore.store[:current_user] = user @@ -85,8 +86,7 @@ it "calls the FindDocumentVersionReference SOAP API endpoint" do veteran_id = "123456789" - - expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api).and_return(false) + expect(FeatureToggle).to receive(:enabled?).with(:use_ce_api, user: user).and_return(false) expect(FeatureToggle).to receive(:enabled?).with(:vbms_pagination, user: user).and_return(false) expect(VBMS::Requests::FindDocumentVersionReference).to receive(:new) .with(veteran_id).and_return(:test_service) From 253c22c180dd7093ad3313b60edbdb262fb9f919 Mon Sep 17 00:00:00 2001 From: youfoundmanesh <129548081+youfoundmanesh@users.noreply.github.com> Date: Mon, 2 Dec 2024 09:25:24 -0500 Subject: [PATCH 36/36] Fixed the code climate issues --- Gemfile | 2 +- .../api/v2/application_controller.rb | 42 ++++--- .../api/v2/manifests_controller.rb | 15 +-- app/models/manifest.rb | 43 +++++--- app/services/external_api/bgs_service.rb | 103 ++++++++++++------ app/services/json_api_response_adapter.rb | 2 +- .../find_valid_veteran_file_numbers.rb | 14 +-- spec/requests/api/v2/manifests_spec.rb | 4 +- .../external_api/vbms_service_spec.rb | 3 +- 9 files changed, 138 insertions(+), 90 deletions(-) diff --git a/Gemfile b/Gemfile index d162b4d79..7ebbf209a 100644 --- a/Gemfile +++ b/Gemfile @@ -57,8 +57,8 @@ gem "redis-namespace" gem "redis-rails", "~> 5.0.2" gem "redis-semaphore" gem "request_store" -gem "rubyzip", ">= 1.3.0" gem "ruby_claim_evidence_api", git: "https://github.com/department-of-veterans-affairs/ruby_claim_evidence_api.git", ref: "4ed23adddb3f584d3a530ef64dcb9ea0cf0eb2b3" +gem "rubyzip", ">= 1.3.0" gem "sass-rails", "~> 5.0" gem "sentry-raven" gem "shoryuken", "3.1.11" diff --git a/app/controllers/api/v2/application_controller.rb b/app/controllers/api/v2/application_controller.rb index 09cb8ebe8..e9638b53c 100644 --- a/app/controllers/api/v2/application_controller.rb +++ b/app/controllers/api/v2/application_controller.rb @@ -18,22 +18,36 @@ def invalid_file_number end def verify_veteran_file_number + file_number = fetch_file_number_from_headers + validate_file_number(file_number) + + return file_number if use_ce_api? && sensitivity_check_passed?(file_number) + + fetch_veteran_by_file_number(file_number) + end + + def fetch_file_number_from_headers file_number = request.headers["HTTP_FILE_NUMBER"] - return missing_header("File Number") unless file_number - - return invalid_file_number unless bgs_service.valid_file_number?(file_number) - - if FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) - if sensitivity_checker.sensitivity_levels_compatible?( - user: current_user, - veteran_file_number: file_number - ) - return file_number - else - raise BGS::SensitivityLevelCheckFailure.new, "You are not authorized to access this file number" - end + missing_header("File Number") unless file_number + file_number + end + + def validate_file_number(file_number) + invalid_file_number unless bgs_service.valid_file_number?(file_number) + end + + def use_ce_api? + FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) + end + + def sensitivity_check_passed?(file_number) + if sensitivity_checker.sensitivity_levels_compatible?( + user: current_user, + veteran_file_number: file_number + ) + true else - fetch_veteran_by_file_number(file_number) + raise BGS::SensitivityLevelCheckFailure.new, "You are not authorized to access this file number" end end diff --git a/app/controllers/api/v2/manifests_controller.rb b/app/controllers/api/v2/manifests_controller.rb index f3a98c0af..6c85a223a 100644 --- a/app/controllers/api/v2/manifests_controller.rb +++ b/app/controllers/api/v2/manifests_controller.rb @@ -5,23 +5,16 @@ class Api::V2::ManifestsController < Api::V2::ApplicationController before_action :veteran_file_number, only: [:start, :refresh] def start - if use_ce_api? - return if performed? - - manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) - else - 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) - end + return if performed? + file_number = use_ce_api? ? veteran_file_number : verify_veteran_file_number + manifest = Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: file_number) manifest.start! render json: json_manifests(manifest) end def refresh - manifest = if use_ce_api? + manifest = if use_ce_api? Manifest.includes(:sources, :records).find_or_create_by_user(user: current_user, file_number: veteran_file_number) else Manifest.find(params[:id]) diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 924fc1951..152b80389 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -28,22 +28,9 @@ class Manifest < ApplicationRecord attr_accessor :user def start! - # Reset stale manifests. - update!(fetched_files_status: :initialized) if ready_for_refresh? - - if FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) - if sensitivity_checker.sensitivity_levels_compatible?( - user: user, - veteran_file_number: file_number - ) - vbms_source.start! - else - raise BGS::SensitivityLevelCheckFailure.new, "You are not authorized to access this manifest" - end - else - vbms_source.start! - end - vva_source.start! unless FeatureToggle.enabled?(:skip_vva) + reset_stale_manifests if ready_for_refresh? + process_vbms_source + start_vva_source end def download_and_package_files! @@ -184,4 +171,28 @@ def update_veteran_info def sensitivity_checker @sensitivity_checker ||= SensitivityChecker.new end + + def reset_stale_manifests + update!(fetched_files_status: :initialized) + end + + def process_vbms_source + if FeatureToggle.enabled?(:use_ce_api, user: RequestStore[:current_user]) + check_sensitivity_and_start_vbms + else + vbms_source.start! + end + end + + def check_sensitivity_and_start_vbms + if sensitivity_checker.sensitivity_levels_compatible?(user: user, veteran_file_number: file_number) + vbms_source.start! + else + raise BGS::SensitivityLevelCheckFailure.new, "You are not authorized to access this manifest" + end + end + + def start_vva_source + vva_source.start! unless FeatureToggle.enabled?(:skip_vva) + end end diff --git a/app/services/external_api/bgs_service.rb b/app/services/external_api/bgs_service.rb index b52e72ac5..a1e5bdfe4 100644 --- a/app/services/external_api/bgs_service.rb +++ b/app/services/external_api/bgs_service.rb @@ -39,47 +39,19 @@ def initialize(client: nil) end def sensitivity_level_for_user(user) - fail "Invalid user" if !user.instance_of?(User) + validate_user(user) - participant_id = user.participant_id - - Rails.cache.fetch("sensitivity_level_for_user_id_#{user.id}", expires_in: 1.hour) do - MetricsService.record( - "Efolder BGS: sensitivity level for user #{user.id}", - service: :bgs, - name: "security.find_person_scrty_log_by_ptcpnt_id" - ) do - response = client.security.find_person_scrty_log_by_ptcpnt_id(participant_id) - - response.key?(:scrty_level_type_cd) ? Integer(response[:scrty_level_type_cd]) : 0 - rescue BGS::ShareError - 0 - end + Rails.cache.fetch(cache_key_for_user(user), expires_in: 1.hour) do + fetch_sensitivity_level(user) end end def sensitivity_level_for_veteran(veteran_file_number) - vet_info = fetch_veteran_info(veteran_file_number) - - participant_id = vet_info.present? ? vet_info[:participant_id] : nil + participant_id = fetch_participant_id(veteran_file_number) + validate_participant_id(participant_id) - fail "Invalid veteran" if participant_id.blank? - - Rails.cache.fetch("sensitivity_level_for_veteran_participant_id_#{participant_id}", expires_in: 1.hour) do - MetricsService.record( - "Efolder BGS: sensitivity level for veteran participant ID #{participant_id}", - service: :bgs, - name: "security.find_sensitivity_level_by_participant_id" - ) do - response = client.security.find_sensitivity_level_by_participant_id(participant_id) - - # guard clause for no response - return 0 if response.blank? - - response&.key?(:scrty_level_type_cd) ? Integer(response[:scrty_level_type_cd]) : 0 - rescue BGS::ShareError - 0 - end + Rails.cache.fetch(cache_key_for_veteran(participant_id), expires_in: 1.hour) do + record_sensitivity_level_metric(participant_id) end end @@ -304,4 +276,65 @@ def bust_fetch_veteran_info_cache(file_number) def fetch_veteran_info_cache_key(file_number) "bgs_veteran_info_#{client.client_username}_#{client.client_station_id}_#{file_number}" end + + def validate_user(user) + raise "Invalid user" unless user.instance_of?(User) + end + + def cache_key_for_user(user) + "sensitivity_level_for_user_id_#{user.id}" + end + + def fetch_sensitivity_level(user) + participant_id = user.participant_id + + MetricsService.record( + "Efolder BGS: sensitivity level for user #{user.id}", + service: :bgs, + name: "security.find_person_scrty_log_by_ptcpnt_id" + ) do + response = client.security.find_person_scrty_log_by_ptcpnt_id(participant_id) + + parse_sensitivity_level(response) + rescue BGS::ShareError + 0 + end + end + + def fetch_participant_id(veteran_file_number) + vet_info = fetch_veteran_info(veteran_file_number) + vet_info.present? ? vet_info[:participant_id] : nil + end + + def validate_participant_id(participant_id) + raise "Invalid veteran" if participant_id.blank? + end + + def cache_key_for_veteran(participant_id) + "sensitivity_level_for_veteran_participant_id_#{participant_id}" + end + + def record_sensitivity_level_metric(participant_id) + MetricsService.record( + "Efolder BGS: sensitivity level for veteran participant ID #{participant_id}", + service: :bgs, + name: "security.find_sensitivity_level_by_participant_id" + ) do + fetch_sensitivity_level(participant_id) + end + end + + def fetch_sensitivity_level(participant_id) + response = client.security.find_sensitivity_level_by_participant_id(participant_id) + + return 0 if response.blank? # Guard clause for no response + + parse_sensitivity_level(response) + rescue BGS::ShareError + 0 + end + + def parse_sensitivity_level(response) + response.key?(:scrty_level_type_cd) ? Integer(response[:scrty_level_type_cd]) : 0 + end end diff --git a/app/services/json_api_response_adapter.rb b/app/services/json_api_response_adapter.rb index 0907ca133..45defc097 100644 --- a/app/services/json_api_response_adapter.rb +++ b/app/services/json_api_response_adapter.rb @@ -36,7 +36,7 @@ def valid_file_response?(json_response) end def check_empty_result?(json_response) - json_response.key?("page") && json_response['page']['totalResults'].to_i == 0 + json_response.key?("page") && json_response["page"]["totalResults"].to_i == 0 end def v2_fetch_documents_file_response(file_json) diff --git a/lib/scripts/find_valid_veteran_file_numbers.rb b/lib/scripts/find_valid_veteran_file_numbers.rb index 0a47a463a..b75fbc43c 100644 --- a/lib/scripts/find_valid_veteran_file_numbers.rb +++ b/lib/scripts/find_valid_veteran_file_numbers.rb @@ -2,7 +2,7 @@ class FindValidVeteranFileNumbers def initialize(user: nil) - fail "Non-prod only" unless Rails.non_production_env? + raise "Non-prod only" unless Rails.non_production_env? self.current_user = user || User.system_user @@ -20,12 +20,10 @@ def run level = bgs_service.sensitivity_level_for_veteran(file_number) key = "sensitivity_level_#{level}" - if !valid_file_numbers.key?(key) - valid_file_numbers[key] = [] - end + valid_file_numbers[key] = [] if !valid_file_numbers.key?(key) valid_file_numbers[key].push(file_number) - rescue => e + rescue StandardError => e invalid_file_numbers.push("#{manifest.file_number} (#{e.message})") next end @@ -43,11 +41,11 @@ def display_final_result(valid_file_numbers, invalid_file_numbers) puts "================================================================================" puts "VALID FILE NUMBERS:" - valid_file_numbers.sort.to_h.each do |k, v| + valid_file_numbers.sort.to_h.each do |k, _v| puts "#{k.upcase} (#{valid_file_numbers[k].count} total numbers)" valid_file_numbers[k].each do |number| - puts "#{number}" + puts number.to_s end puts "" @@ -57,7 +55,7 @@ def display_final_result(valid_file_numbers, invalid_file_numbers) puts "\nINVALID FILE NUMBERS (#{invalid_file_numbers.count} total numbers):" invalid_file_numbers.each do |number| - puts "#{number}" + puts number.to_s end puts "================================================================================" diff --git a/spec/requests/api/v2/manifests_spec.rb b/spec/requests/api/v2/manifests_spec.rb index 791b1399c..1eaa4d079 100644 --- a/spec/requests/api/v2/manifests_spec.rb +++ b/spec/requests/api/v2/manifests_spec.rb @@ -51,7 +51,7 @@ context "when the check succeeds" do it "allows access to the start action" do expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice - .with(user: user, veteran_file_number: "DEMO987").and_return(true) + .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests", params: nil, headers: headers @@ -60,7 +60,7 @@ it "allows access to the refresh action" do expect(mock_sensitivity_checker).to receive(:sensitivity_levels_compatible?).twice - .with(user: user, veteran_file_number: "DEMO987").and_return(true) + .with(user: user, veteran_file_number: "DEMO987").and_return(true) post "/api/v2/manifests/#{manifest.id}", params: nil, headers: headers diff --git a/spec/services/external_api/vbms_service_spec.rb b/spec/services/external_api/vbms_service_spec.rb index 9bc931dd7..ee631a3f0 100644 --- a/spec/services/external_api/vbms_service_spec.rb +++ b/spec/services/external_api/vbms_service_spec.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true - + describe ExternalApi::VBMSService do subject(:described) { described_class } @@ -78,7 +78,6 @@ end context "with no feature toggles enabled" do - let!(:user) do user = User.create(css_id: "VSO", station_id: "283", participant_id: "1234") RequestStore.store[:current_user] = user