Skip to content

Commit

Permalink
Validate document attachments at time of upload (#19532)
Browse files Browse the repository at this point in the history
* Validate document attachments at time of upload

* Added flipper for extra validation steps

---------

Co-authored-by: Wayne Weibel <[email protected]>
  • Loading branch information
danlim715 and wayne-weibel authored Dec 19, 2024
1 parent ad7406c commit ebf58a6
Show file tree
Hide file tree
Showing 10 changed files with 295 additions and 77 deletions.
3 changes: 3 additions & 0 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,7 @@ lib/caseflow @department-of-veterans-affairs/lighthouse-banana-peels @department
lib/central_mail @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
lib/chip @department-of-veterans-affairs/vsa-healthcare-health-quest-1-backend @department-of-veterans-affairs/patient-check-in @department-of-veterans-affairs/backend-review-group
lib/claim_letters @department-of-veterans-affairs/benefits-management-tools-be @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
lib/claim_documents/monitor.rb @department-of-veterans-affairs/pension-and-burials @department-of-veterans-affairs/backend-review-group
lib/clamav @department-of-veterans-affairs/backend-review-group
lib/common/client/base.rb @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
lib/common/client/concerns/mhv_fhir_session_client.rb @department-of-veterans-affairs/vfs-mhv-medical-records @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
Expand Down Expand Up @@ -1419,6 +1420,7 @@ spec/lib/carma @department-of-veterans-affairs/vfs-10-10 @department-of-veterans
spec/lib/caseflow @department-of-veterans-affairs/lighthouse-banana-peels @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/lib/central_mail @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/lib/chip @department-of-veterans-affairs/vsa-healthcare-health-quest-1-backend @department-of-veterans-affairs/patient-check-in @department-of-veterans-affairs/backend-review-group
spec/lib/claim_documents/monitor_spec.rb @department-of-veterans-affairs/pension-and-burials @department-of-veterans-affairs/backend-review-group
spec/lib/claim_status_tool @department-of-veterans-affairs/benefits-management-tools-be @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/lib/common/client/concerns/mhv_fhir_session_client_spec.rb @department-of-veterans-affairs/vfs-mhv-medical-records @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/lib/common/client/concerns/mhv_jwt_session_client_spec.rb @department-of-veterans-affairs/vfs-mhv-medical-records @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
Expand Down Expand Up @@ -2127,6 +2129,7 @@ spec/support/vcr_cassettes/spec/support @department-of-veterans-affairs/octo-ide
spec/support/vcr_cassettes/staccato @department-of-veterans-affairs/vfs-10-10 @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/support/vcr_cassettes/token_validation @department-of-veterans-affairs/lighthouse-banana-peels @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/support/vcr_cassettes/travel_pay @department-of-veterans-affairs/travel-pay-integration @department-of-veterans-affairs/backend-review-group
spec/support/vcr_cassettes/uploads/validate_document.yml @department-of-veterans-affairs/pension-and-burials @department-of-veterans-affairs/backend-review-group
spec/spupport/vcr_cassettes/user/get_facilities_empty.yml @department-of-veterans-affairs/vfs-facilities-frontend @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/support/vcr_cassettes/va_forms @department-of-veterans-affairs/platform-va-product-forms @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/support/vcr_cassettes/va_notify @department-of-veterans-affairs/va-notify-write @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
Expand Down
66 changes: 56 additions & 10 deletions app/controllers/v0/claim_documents_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,46 @@

require 'pension_burial/tag_sentry'
require 'lgy/tag_sentry'
require 'claim_documents/monitor'
require 'lighthouse/benefits_intake/service'
require 'pdf_utilities/datestamp_pdf'

module V0
class ClaimDocumentsController < ApplicationController
service_tag 'claims-shared'
skip_before_action(:authenticate)
before_action :load_user

def create
Rails.logger.info "Creating PersistentAttachment FormID=#{form_id}"
uploads_monitor.track_document_upload_attempt(form_id, current_user)

attachment = klass.new(form_id:)
@attachment = klass&.new(form_id:)
# add the file after so that we have a form_id and guid for the uploader to use
attachment.file = unlock_file(params['file'], params['password'])
@attachment.file = unlock_file(params['file'], params['password'])

raise Common::Exceptions::ValidationErrors, attachment unless attachment.valid?
if %w[21P-527EZ 21P-530 21P-530V2].include?(form_id) &&
Flipper.enabled?(:document_upload_validation_enabled) && !stamped_pdf_valid?

attachment.save
raise Common::Exceptions::ValidationErrors, @attachment
end

raise Common::Exceptions::ValidationErrors, @attachment unless @attachment.valid?

Rails.logger.info "Success creating PersistentAttachment FormID=#{form_id} AttachmentID=#{attachment.id}"
@attachment.save

render json: PersistentAttachmentSerializer.new(attachment)
uploads_monitor.track_document_upload_success(form_id, @attachment.id, current_user)

render json: PersistentAttachmentSerializer.new(@attachment)
rescue => e
Rails.logger.error "Error creating PersistentAttachment FormID=#{form_id} AttachmentID=#{attachment.id} #{e}"
uploads_monitor.track_document_upload_failed(form_id, @attachment&.id, current_user, e)
raise e
end

private

def klass
case form_id
when '21P-527EZ', '21P-530EZ'
when '21P-527EZ', '21P-530EZ', '21P-530V2'
PensionBurial::TagSentry.tag_sentry
PersistentAttachments::PensionBurial
when '21-686C', '686C-674'
Expand All @@ -47,7 +57,7 @@ def form_id
end

def unlock_file(file, file_password)
return file unless File.extname(file) == '.pdf' && file_password
return file unless File.extname(file) == '.pdf' && file_password.present?

pdftk = PdfForms.new(Settings.binaries.pdftk)
tmpf = Tempfile.new(['decrypted_form_attachment', '.pdf'])
Expand All @@ -69,5 +79,41 @@ def unlock_file(file, file_password)
file.tempfile = tmpf
file
end

# rubocop:disable Metrics/MethodLength
def stamped_pdf_valid?
extension = File.extname(@attachment&.file&.id)
allowed_types = PersistentAttachment::ALLOWED_DOCUMENT_TYPES

if allowed_types.exclude?(extension)
raise Common::Exceptions::UnprocessableEntity.new(
detail: I18n.t('errors.messages.extension_allowlist_error', extension:, allowed_types:),
source: 'PersistentAttachment.stamped_pdf_valid?'
)
elsif @attachment&.file&.size&.< PersistentAttachment::MINIMUM_FILE_SIZE
raise Common::Exceptions::UnprocessableEntity.new(
detail: 'File size must not be less than 1.0 KB',
source: 'PersistentAttachment.stamped_pdf_valid?'
)
end

document = PDFUtilities::DatestampPdf.new(@attachment.to_pdf).run(text: 'VA.GOV', x: 5, y: 5)
intake_service.valid_document?(document:)
rescue BenefitsIntake::Service::InvalidDocumentError => e
@attachment.errors.add(:attachment, e.message)
false
rescue PdfForms::PdftkError
@attachment.errors.add(:attachment, 'File is corrupt and cannot be uploaded')
false
end
# rubocop:enable Metrics/MethodLength

def intake_service
@intake_service ||= BenefitsIntake::Service.new
end

def uploads_monitor
@uploads_monitor ||= ClaimDocuments::Monitor.new
end
end
end
3 changes: 3 additions & 0 deletions app/models/persistent_attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
class PersistentAttachment < ApplicationRecord
include SetGuid

ALLOWED_DOCUMENT_TYPES = %w[.pdf .jpg .jpeg .png].freeze
MINIMUM_FILE_SIZE = 1.kilobyte.freeze

has_kms_key
has_encrypted :file_data, key: :kms_key, **lockbox_options
belongs_to :saved_claim, inverse_of: :persistent_attachments, optional: true
Expand Down
4 changes: 4 additions & 0 deletions config/features.yml
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ features:
disability_compensation_staging_lighthouse_brd:
actor_type: user
description: Switches to Lighthouse Staging BRD Service. NEVER ENABLE IN PRODUCTION.
document_upload_validation_enabled:
actor_type: user
description: Enables stamped PDF validation on document upload
enable_in_development: true
hca_browser_monitoring_enabled:
actor_type: user
description: Enables browser monitoring for the health care application.
Expand Down
48 changes: 48 additions & 0 deletions lib/claim_documents/monitor.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# frozen_string_literal: true

require 'logging/monitor'

module ClaimDocuments
##
# Monitor functions for Rails logging and StatsD
# @todo abstract, split logging for controller and sidekiq
#
class Monitor < ::Logging::Monitor
# statsd key for document uploads
DOCUMENT_STATS_KEY = 'api.claim_documents'

def initialize
super('claim_documents')
end

def track_document_upload_attempt(form_id, current_user)
additional_context = {
user_account_uuid: current_user&.user_account_uuid,
tags: ["form_id:#{form_id}"]
}
track_request('info', "Creating PersistentAttachment FormID=#{form_id}", "#{DOCUMENT_STATS_KEY}.attempt",
**additional_context)
end

def track_document_upload_success(form_id, attachment_id, current_user)
additional_context = {
attachment_id:,
user_account_uuid: current_user&.user_account_uuid,
tags: ["form_id:#{form_id}"]
}
track_request('info', "Success creating PersistentAttachment FormID=#{form_id} AttachmentID=#{attachment_id}",
"#{DOCUMENT_STATS_KEY}.success", **additional_context)
end

def track_document_upload_failed(form_id, attachment_id, current_user, e)
additional_context = {
attachment_id:,
user_account_uuid: current_user&.user_account_uuid,
tags: ["form_id:#{form_id}"],
message: e&.message
}
track_request('error', "Error creating PersistentAttachment FormID=#{form_id} AttachmentID=#{attachment_id} #{e}",
"#{DOCUMENT_STATS_KEY}.failure", **additional_context)
end
end
end
Binary file added spec/fixtures/files/tiny.pdf
Binary file not shown.
50 changes: 50 additions & 0 deletions spec/lib/claim_documents/monitor_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# frozen_string_literal: true

require 'rails_helper'
require 'claim_documents/monitor'

RSpec.describe ClaimDocuments::Monitor do
let(:service) { OpenStruct.new(uuid: 'uuid') }
let(:monitor) { described_class.new }
let(:document_stats_key) { described_class::DOCUMENT_STATS_KEY }
let(:form_id) { 'ABC123' }
let(:attachment_id) { '12345' }
let(:current_user) { create(:user) }
let(:error) { StandardError.new('An error occurred') }

describe '#track_document_upload_attempt' do
it 'logs an upload attempt' do
expect(StatsD).to receive(:increment).with("#{document_stats_key}.attempt", tags: ["form_id:#{form_id}"])
expect(Rails.logger).to receive(:info).with(
"Creating PersistentAttachment FormID=#{form_id}",
hash_including(user_account_uuid: current_user.user_account_uuid, statsd: "#{document_stats_key}.attempt")
)

monitor.track_document_upload_attempt(form_id, current_user)
end
end

describe '#track_document_upload_success' do
it 'logs a successful upload' do
expect(StatsD).to receive(:increment).with("#{document_stats_key}.success", tags: ["form_id:#{form_id}"])
expect(Rails.logger).to receive(:info).with(
"Success creating PersistentAttachment FormID=#{form_id} AttachmentID=#{attachment_id}",
hash_including(user_account_uuid: current_user.user_account_uuid, statsd: "#{document_stats_key}.success")
)

monitor.track_document_upload_success(form_id, attachment_id, current_user)
end
end

describe '#track_document_upload_failed' do
it 'logs a failed upload' do
expect(StatsD).to receive(:increment).with("#{document_stats_key}.failure", tags: ["form_id:#{form_id}"])
expect(Rails.logger).to receive(:error).with(
"Error creating PersistentAttachment FormID=#{form_id} AttachmentID=#{attachment_id} #{error}",
hash_including(user_account_uuid: current_user.user_account_uuid, statsd: "#{document_stats_key}.failure")
)

monitor.track_document_upload_failed(form_id, attachment_id, current_user, error)
end
end
end
38 changes: 20 additions & 18 deletions spec/requests/swagger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -296,25 +296,27 @@
end

it 'supports adding a claim document' do
expect(subject).to validate(
:post,
'/v0/claim_attachments',
200,
'_data' => {
'form_id' => '21P-530EZ',
file: fixture_file_upload('spec/fixtures/files/doctors-note.pdf')
}
)
VCR.use_cassette('uploads/validate_document') do
expect(subject).to validate(
:post,
'/v0/claim_attachments',
200,
'_data' => {
'form_id' => '21P-530EZ',
file: fixture_file_upload('spec/fixtures/files/doctors-note.pdf')
}
)

expect(subject).to validate(
:post,
'/v0/claim_attachments',
422,
'_data' => {
'form_id' => '21P-530EZ',
file: fixture_file_upload('spec/fixtures/files/empty_file.txt')
}
)
expect(subject).to validate(
:post,
'/v0/claim_attachments',
422,
'_data' => {
'form_id' => '21P-530EZ',
file: fixture_file_upload('spec/fixtures/files/empty_file.txt')
}
)
end
end

it 'supports checking stem_claim_status' do
Expand Down
Loading

0 comments on commit ebf58a6

Please sign in to comment.