From 5b3c2184da3a59e0d09be65d3a1b79f7dad04add Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 2 Aug 2024 13:05:52 -0600 Subject: [PATCH 01/27] 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 | 1 + 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 +++++++++++++------ spec/services/bgs_service_spec.rb | 39 +++++++++ spec/services/sensitivity_checker_spec.rb | 48 +++++++++++ 17 files changed, 363 insertions(+), 31 deletions(-) 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 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 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 42658082a..adae14727 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 641415651..0ede4b9bc 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/bgs_service_spec.rb b/spec/services/bgs_service_spec.rb index 3d9b09a61..cef54e327 100644 --- a/spec/services/bgs_service_spec.rb +++ b/spec/services/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 d9a2f77eff38303e0fdcb05c1091dcde6acbf684 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Tue, 13 Aug 2024 07:35:51 -0600 Subject: [PATCH 02/27] 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 9557efd28f365b8973649359f0d2336389add67e Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Tue, 13 Aug 2024 12:57:02 -0600 Subject: [PATCH 03/27] 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 08fe2d8c2..e8332c709 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -26,6 +26,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) @@ -44,6 +46,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) @@ -69,6 +73,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 @@ -110,4 +116,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 f1d396f54f32fe3b51ab5091c85a17b9a4d1ab24 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Wed, 14 Aug 2024 13:54:34 -0600 Subject: [PATCH 04/27] 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 af9d251fe67df989706588b52d6ae9f711235daa Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Thu, 15 Aug 2024 13:56:44 -0600 Subject: [PATCH 05/27] 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 e9fd8abb66bb4f386894423ec22acabce02bbd8d Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 15 Aug 2024 15:58:26 -0400 Subject: [PATCH 06/27] 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 923449742..4c2193c87 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 01576e477e290c196941efe7ec67f3adbe63982b Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Wed, 4 Sep 2024 09:32:36 -0600 Subject: [PATCH 07/27] 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 2f41563d9a4ea3a681ba85146fc6b40f075ec299 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 08:37:25 -0600 Subject: [PATCH 08/27] 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 183286bc82dca53829857d4ee9217018930145ab Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 09:10:54 -0600 Subject: [PATCH 09/27] 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 c3d84ced108fab992b5e66831c3b8ef0ad1ae310 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 11:28:36 -0600 Subject: [PATCH 10/27] 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 1a22df9dcf53b3304375f260772a2debf112adab Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 6 Sep 2024 11:53:07 -0600 Subject: [PATCH 11/27] Update CE API Gem (#1680) --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 20b39d724..e287a7cd3 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 4c2193c87..0a394953c 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 0555acfd1f30922fdfd31f37f678268fe511cc9b Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 9 Sep 2024 07:36:06 -0600 Subject: [PATCH 12/27] 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 cb430029848a1fe43210968e770c06697964a2bd Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Wed, 11 Sep 2024 16:49:23 -0600 Subject: [PATCH 13/27] 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 e8332c709..f89268450 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -32,7 +32,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] @@ -56,7 +56,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) @@ -126,4 +126,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 4a4f847ec01dd94fa81a91b8ebdbe044a3dc116e Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 12 Sep 2024 08:34:55 -0400 Subject: [PATCH 14/27] Updated ruby_claim_evidence_api --- Gemfile.lock | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 0a394953c..31fd4b743 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 9e2a9a44883ea650784aa4f1dad23ddb647f217a Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 13 Sep 2024 11:53:05 -0600 Subject: [PATCH 15/27] 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 f89268450..5215397f2 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -77,7 +77,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 6d184559962bafe588fa43b40b3d3d016e7dbe8c 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/27] 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 5215397f2..5d985330d 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -130,7 +130,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 1eaf33cbf14872bfbbf39e8707904b6ef95cfb51 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 13 Sep 2024 12:13:17 -0600 Subject: [PATCH 17/27] 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 9322a5034d4f8c6bac61fdc6cf3e8bb71dd3832c 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/27] 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 89a91f07c8c5fbcbf48f8898ac176494fce03116 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 16 Sep 2024 09:57:39 -0600 Subject: [PATCH 19/27] 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 455fef7635d94b38c84824229f8a929000b8e872 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 16 Sep 2024 10:05:54 -0600 Subject: [PATCH 20/27] 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 ae2f385a8b308107b345a6db6947330958a7062b Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Mon, 16 Sep 2024 11:24:10 -0600 Subject: [PATCH 21/27] 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 8b83f87325e1a5398a91bbaa763383c63c79f31c Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Sun, 22 Sep 2024 20:00:59 -0400 Subject: [PATCH 22/27] Updated ruby_claim_evidence_api gem with ref --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index e287a7cd3..c7f227b48 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 c13cb12a345ee7b201215d50245aa22bb90ae70f Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Wed, 25 Sep 2024 15:11:26 -0400 Subject: [PATCH 23/27] 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 5d985330d..f37d69c4c 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -117,7 +117,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 b701318e81c3e7120442734cdb2953aefac2f237 Mon Sep 17 00:00:00 2001 From: youfoundmanesh Date: Thu, 26 Sep 2024 12:08:09 -0400 Subject: [PATCH 24/27] 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 f37d69c4c..6eb5f4169 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -117,7 +117,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 919352c6d0957f452c4bd63ad3af02b43a42fc47 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/27] 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 6eb5f4169..35badc2b3 100644 --- a/app/services/external_api/vbms_service.rb +++ b/app/services/external_api/vbms_service.rb @@ -26,11 +26,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]) @@ -46,11 +45,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, @@ -73,9 +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) + 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 @@ -117,7 +114,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 50d2d0c33374d759f930b8c19a788577f42ea2e3 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/27] 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 05044a2a70343d946a651abab1c1c3e090247c55 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/27] 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 + } + } + } + ] +}