From 2262e44fcdf9a01f0ab7cb37dcd665165f5676cf Mon Sep 17 00:00:00 2001 From: Rebecca Tolmach <10993987+rmtolmach@users.noreply.github.com> Date: Wed, 17 Apr 2024 16:27:11 -0400 Subject: [PATCH] Fix conflict (#16389) * Refactor uploads spec, simple forms (#16380) * Refactor uploads spec, simple forms * rubocop * BTSSS-77372 Add mocked responses for BTSSS (#16231) * Revert to original token URL in service * Add authorized ping mock, too * Handle Bearer Token parsing failures gracefully * Clean up services config * Revert some accidental deletions * Final bit of cleanup * Switch to correct file path * Move authorize method to a before_action * Update mockdata paths * Fix some linting errors * Add direct deposit email notification template (#16357) * Add template_type method to VANotifyEmailJob * Update direct deposit email job to accept a default parameter of nil for the direct deposit type. * Add method to request a token from STS (#16372) * Add method to request a token from STS * fix rubocop * Fix tests * Remove debugging line * change parameter to reflect actual value * Merge conflict fix * remove diff file from merge conflict * [80904] Creating a terms of use application check that occurs during secondary authentications with custom (#16371) --------- Co-authored-by: Eric Tillberg Co-authored-by: Dan Hinze Co-authored-by: Tom Harrison Co-authored-by: Kevin Duensing Co-authored-by: Trevor Bosaw --- .../v0/profile/direct_deposits_controller.rb | 2 +- app/sidekiq/va_notify_dd_email_job.rb | 13 +- config/settings.yml | 1 + lib/saml/post_url_service.rb | 29 +++- .../travel_pay/claims_controller.rb | 6 +- .../travel_pay/pings_controller.rb | 4 +- .../app/services/travel_pay/client.rb | 57 +++++++- .../controllers/claims_controller_spec.rb | 12 +- .../spec/controllers/pings_controller_spec.rb | 3 + .../direct_deposits_controller_spec.rb | 2 +- spec/lib/saml/post_url_service_spec.rb | 129 +++++++++++++----- spec/sidekiq/va_notify_dd_email_job_spec.rb | 71 ++++++++-- 12 files changed, 265 insertions(+), 64 deletions(-) diff --git a/app/controllers/v0/profile/direct_deposits_controller.rb b/app/controllers/v0/profile/direct_deposits_controller.rb index 4d4f6b92316..942a666f594 100644 --- a/app/controllers/v0/profile/direct_deposits_controller.rb +++ b/app/controllers/v0/profile/direct_deposits_controller.rb @@ -88,7 +88,7 @@ def control_info_params end def send_confirmation_email - VANotifyDdEmailJob.send_to_emails(current_user.all_emails, 'comp_and_pen') + VANotifyDdEmailJob.send_to_emails(current_user.all_emails) end end end diff --git a/app/sidekiq/va_notify_dd_email_job.rb b/app/sidekiq/va_notify_dd_email_job.rb index 6a1dcdc4121..1f8a81c0697 100644 --- a/app/sidekiq/va_notify_dd_email_job.rb +++ b/app/sidekiq/va_notify_dd_email_job.rb @@ -10,7 +10,7 @@ class VANotifyDdEmailJob STATSD_ERROR_NAME = 'worker.direct_deposit_confirmation_email.error' STATSD_SUCCESS_NAME = 'worker.direct_deposit_confirmation_email.success' - def self.send_to_emails(user_emails, dd_type) + def self.send_to_emails(user_emails, dd_type = nil) if user_emails.present? user_emails.each do |email| perform_async(email, dd_type) @@ -25,9 +25,9 @@ def self.send_to_emails(user_emails, dd_type) end end - def perform(email, dd_type) + def perform(email, dd_type = nil) notify_client = VaNotify::Service.new(Settings.vanotify.services.va_gov.api_key) - template_type = "direct_deposit_#{dd_type.to_sym == :ch33 ? 'edu' : 'comp_pen'}" + template_type = template_type(dd_type) template_id = Settings.vanotify.services.va_gov.template_id.public_send(template_type) notify_client.send_email( @@ -39,6 +39,13 @@ def perform(email, dd_type) handle_errors(e) end + def template_type(dd_type) + return 'direct_deposit_edu' if dd_type&.to_sym == :ch33 + return 'direct_deposit_comp_pen' if dd_type&.to_sym == :comp_pen + + 'direct_deposit' + end + def handle_errors(ex) VANotifyDdEmailJob.log_exception_to_sentry(ex) StatsD.increment(STATSD_ERROR_NAME) diff --git a/config/settings.yml b/config/settings.yml index bc29f317582..467b4ee40b2 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -1284,6 +1284,7 @@ vanotify: in_progress_reminder_email_generic: fake_template_id covid_vaccine_registration: fake_template_id covid_vaccine_expanded_registration: fake_template_id + direct_deposit: direct_deposit_template_id direct_deposit_edu: edu_template_id direct_deposit_comp_pen: comp_pen_template_id login_reactivation_email: reactivation_email_test_b diff --git a/lib/saml/post_url_service.rb b/lib/saml/post_url_service.rb index d16313784d4..b91b651d8e0 100644 --- a/lib/saml/post_url_service.rb +++ b/lib/saml/post_url_service.rb @@ -63,8 +63,7 @@ def tou_declined_logout_redirect_url end def terms_of_use_redirect_url - application = @tracker&.payload_attr(:application) || 'vaweb' - if enabled_tou_clients.include?(application) + if terms_of_use_enabled_application Rails.logger.info('Redirecting to /terms-of-use', type: :ssoe) add_query(terms_of_use_url, { redirect_url: login_redirect_url }) else @@ -79,6 +78,32 @@ def ssoe_slo_url private + def terms_of_use_enabled_application + cache_key = "terms_of_use_redirect_user_#{user.uuid}" + cached_application = retrieve_and_delete_terms_of_use_redirect_user(cache_key) + current_application = @tracker&.payload_attr(:application) + write_terms_of_use_redirect_user(cache_key, current_application) if should_cache_application?(current_application) + terms_of_use_redirect_enabled?(cached_application, current_application) + end + + def terms_of_use_redirect_enabled?(cached_application, current_application) + enabled_tou_clients.include?(cached_application || current_application || 'vaweb') + end + + def should_cache_application?(application) + enabled_tou_clients.include?(application) + end + + def retrieve_and_delete_terms_of_use_redirect_user(cache_key) + application = Rails.cache.read(cache_key) + Rails.cache.delete(cache_key) + application + end + + def write_terms_of_use_redirect_user(cache_key, application) + Rails.cache.write(cache_key, application, expires_in: 5.minutes) + end + def terms_of_use_url if Settings.review_instance_slug.present? "http://#{Settings.review_instance_slug}.review.vetsgov-internal/terms-of-use" diff --git a/modules/travel_pay/app/controllers/travel_pay/claims_controller.rb b/modules/travel_pay/app/controllers/travel_pay/claims_controller.rb index 18136f9f7e6..59cce20d36d 100644 --- a/modules/travel_pay/app/controllers/travel_pay/claims_controller.rb +++ b/modules/travel_pay/app/controllers/travel_pay/claims_controller.rb @@ -6,9 +6,9 @@ class ClaimsController < ApplicationController def index veis_token = client.request_veis_token - # Non-intuitive Ruby behavior: #split splits a string on space by default - vagov_token = request.headers['Authorization'].split[1] - btsss_token = client.request_btsss_token(veis_token, vagov_token) + + sts_token = client.request_sts_token(@current_user) + btsss_token = client.request_btsss_token(veis_token, sts_token) begin claims = client.get_claims(veis_token, btsss_token) diff --git a/modules/travel_pay/app/controllers/travel_pay/pings_controller.rb b/modules/travel_pay/app/controllers/travel_pay/pings_controller.rb index c0d12814307..6f8964a7fd2 100644 --- a/modules/travel_pay/app/controllers/travel_pay/pings_controller.rb +++ b/modules/travel_pay/app/controllers/travel_pay/pings_controller.rb @@ -13,9 +13,9 @@ def ping end def authorized_ping - vagov_token = request.headers['Authorization'].split[1] + sts_token = client.request_sts_token(@current_user) veis_token = client.request_veis_token - btsss_token = client.request_btsss_token(veis_token, vagov_token) + btsss_token = client.request_btsss_token(veis_token, sts_token) btsss_authorized_ping_response = client.authorized_ping(veis_token, btsss_token) render json: { diff --git a/modules/travel_pay/app/services/travel_pay/client.rb b/modules/travel_pay/app/services/travel_pay/client.rb index e48b5f52c3a..24ef261c2c9 100644 --- a/modules/travel_pay/app/services/travel_pay/client.rb +++ b/modules/travel_pay/app/services/travel_pay/client.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'securerandom' + module TravelPay class Client ## @@ -24,7 +26,7 @@ def request_veis_token # # @return [Faraday::Response] # - def request_btsss_token(veis_token, vagov_token) + def request_btsss_token(veis_token, sts_token) btsss_url = Settings.travel_pay.base_url api_key = Settings.travel_pay.subscription_key client_number = Settings.travel_pay.client_number @@ -33,7 +35,7 @@ def request_btsss_token(veis_token, vagov_token) req.headers['Authorization'] = "Bearer #{veis_token}" req.headers['Ocp-Apim-Subscription-Key'] = api_key req.headers['BTSSS-API-Client-Number'] = client_number.to_s - req.body = { authJwt: vagov_token } + req.body = { authJwt: sts_token } end response.body['access_token'] end @@ -90,8 +92,59 @@ def get_claims(veis_token, btsss_token) symbolized_body[:data].sort_by(&parse_claim_date).reverse! end + def request_sts_token(user) + host_baseurl = build_host_baseurl({ ip_form: false }) + private_key_file = Settings.sign_in.sts_client.key_path + private_key = OpenSSL::PKey::RSA.new(File.read(private_key_file)) + + assertion = build_sts_assertion(user) + jwt = JWT.encode(assertion, private_key, 'RS256') + + # send to sis + response = connection(server_url: host_baseurl).post('/v0/sign_in/token') do |req| + req.params['grant_type'] = 'urn:ietf:params:oauth:grant-type:jwt-bearer' + req.params['assertion'] = jwt + end + + response.body['data']['access_token'] + end + private + def build_sts_assertion(user) + service_account_id = Settings.travel_pay.sts.service_account_id + host_baseurl = build_host_baseurl({ ip_form: false }) + audience_baseurl = build_host_baseurl({ ip_form: true }) + + current_time = Time.now.to_i + jti = SecureRandom.uuid + + { + 'iss' => host_baseurl, + 'sub' => user.email, + 'aud' => "#{audience_baseurl}/v0/sign_in/token", + 'iat' => current_time, + 'exp' => current_time + 300, + 'scopes' => [], + 'service_account_id' => service_account_id, + 'jti' => jti, + 'user_attributes' => { 'icn' => user.icn } + } + end + + def build_host_baseurl(config) + env = Settings.vsp_environment + host = Settings.hostname + + if env == 'localhost' + return 'http://127.0.0.1:3000' if config[:ip_form] + + 'http://localhost:3000' + end + + "https://#{host}" + end + def veis_params { client_id: Settings.travel_pay.veis.client_id, diff --git a/modules/travel_pay/spec/controllers/claims_controller_spec.rb b/modules/travel_pay/spec/controllers/claims_controller_spec.rb index 0762820acf5..16c2ca24639 100644 --- a/modules/travel_pay/spec/controllers/claims_controller_spec.rb +++ b/modules/travel_pay/spec/controllers/claims_controller_spec.rb @@ -12,9 +12,13 @@ .to receive(:request_veis_token) .and_return('veis_token') + allow_any_instance_of(TravelPay::Client) + .to receive(:request_sts_token) + .and_return('sts_token') + allow_any_instance_of(TravelPay::Client) .to receive(:request_btsss_token) - .with('veis_token', 'vagov_token') + .with('veis_token', 'sts_token') .and_return('btsss_token') allow_any_instance_of(TravelPay::Client) @@ -34,9 +38,13 @@ .to receive(:request_veis_token) .and_return('veis_token') + allow_any_instance_of(TravelPay::Client) + .to receive(:request_sts_token) + .and_return('sts_token') + allow_any_instance_of(TravelPay::Client) .to receive(:request_btsss_token) - .with('veis_token', 'vagov_token') + .with('veis_token', 'sts_token') .and_return('btsss_token') allow_any_instance_of(TravelPay::Client) diff --git a/modules/travel_pay/spec/controllers/pings_controller_spec.rb b/modules/travel_pay/spec/controllers/pings_controller_spec.rb index 750a96600bf..93c43f00240 100644 --- a/modules/travel_pay/spec/controllers/pings_controller_spec.rb +++ b/modules/travel_pay/spec/controllers/pings_controller_spec.rb @@ -48,6 +48,9 @@ before do btsss_authorized_ping_response = double allow(btsss_authorized_ping_response).to receive(:status).and_return(200) + allow(client) + .to receive(:request_sts_token) + .and_return('sample_sts_token') allow(client) .to receive(:request_btsss_token) .and_return('sample_btsss_token') diff --git a/spec/controllers/v0/profile/direct_deposits_controller_spec.rb b/spec/controllers/v0/profile/direct_deposits_controller_spec.rb index 1e0af7acae2..3dd8abc3e91 100644 --- a/spec/controllers/v0/profile/direct_deposits_controller_spec.rb +++ b/spec/controllers/v0/profile/direct_deposits_controller_spec.rb @@ -181,7 +181,7 @@ context 'when the user does have an associated email address' do it 'sends an email through va notify' do expect(VANotifyDdEmailJob).to receive(:send_to_emails).with( - user.all_emails, 'comp_and_pen' + user.all_emails ) VCR.use_cassette('lighthouse/direct_deposit/update/200_valid') do diff --git a/spec/lib/saml/post_url_service_spec.rb b/spec/lib/saml/post_url_service_spec.rb index 692020efd7a..fb2c9df8172 100644 --- a/spec/lib/saml/post_url_service_spec.rb +++ b/spec/lib/saml/post_url_service_spec.rb @@ -613,29 +613,102 @@ let(:expected_log_message) { 'Redirecting to /terms-of-use' } let(:expected_log_payload) { { type: :ssoe } } - context 'when tracker application is within Settings.terms_of_use.enabled_clients' do + context 'when associated terms of use redirect user cache object exists' do + let(:cache_key) { "terms_of_use_redirect_user_#{user.uuid}" } + let(:enabled_clients) { application } + let(:cache_expiration) { 5.minutes } + before do - allow(Settings.terms_of_use).to receive(:enabled_clients).and_return(application) + allow(Settings.terms_of_use).to receive(:enabled_clients).and_return(enabled_clients) + allow(Rails.cache).to receive(:read).with(cache_key).and_return(application) end - context 'and authentication is occuring on a review instance' do - let(:review_instance_slug) { 'some-review-instance-slug' } - let(:review_instance_url) { "#{review_instance_slug}.review.vetsgov-internal" } + context 'and application is within Settings.terms_of_use.enabled_clients' do + let(:enabled_clients) { application } - before { allow(Settings).to receive(:review_instance_slug).and_return(review_instance_slug) } + context 'and authentication is occuring on a review instance' do + let(:review_instance_slug) { 'some-review-instance-slug' } + let(:review_instance_url) { "#{review_instance_slug}.review.vetsgov-internal" } - it 'has a login redirect url as a parameter embedded in review instance terms of use page' do - expect(subject.terms_of_use_redirect_url) - .to eq("http://#{review_instance_url}/terms-of-use?#{expected_redirect_url_param}") + before { allow(Settings).to receive(:review_instance_slug).and_return(review_instance_slug) } + + it 'has a login redirect url as a parameter embedded in review instance terms of use page' do + expect(subject.terms_of_use_redirect_url) + .to eq("http://#{review_instance_url}/terms-of-use?#{expected_redirect_url_param}") + end + + it 'logs expected message and payload' do + expect(Rails.logger).to receive(:info).with(expected_log_message, expected_log_payload) + subject.terms_of_use_redirect_url + end end - it 'logs expected message and payload' do - expect(Rails.logger).to receive(:info).with(expected_log_message, expected_log_payload) - subject.terms_of_use_redirect_url + context 'and authentication is not occurring on a review instance' do + it 'has a login redirect url as a parameter embedded in terms of use page with success' do + expect(subject.terms_of_use_redirect_url) + .to eq("#{values[:base_redirect]}/terms-of-use?#{expected_redirect_url_param}") + end + + it 'logs expected message and payload' do + expect(Rails.logger).to receive(:info).with(expected_log_message, expected_log_payload) + subject.terms_of_use_redirect_url + end end end - context 'and authentication is not occurring on a review instance' do + context 'and stored application is not within Settings.terms_of_use.enabled_clients' do + let(:enabled_clients) { '' } + + it 'has a login redirect url with success not embedded in a terms of use page' do + expect(subject.terms_of_use_redirect_url).to eq(expected_login_redirect_url) + end + end + + it 'deletes the cached terms of use redirect user object' do + expect(Rails.cache).to receive(:delete).with(cache_key) + subject.terms_of_use_redirect_url + end + end + + context 'when associated terms of use redirect user cache object does not exist' do + context 'when tracker application is within Settings.terms_of_use.enabled_clients' do + before do + allow(Settings.terms_of_use).to receive(:enabled_clients).and_return(application) + end + + context 'and authentication is occuring on a review instance' do + let(:review_instance_slug) { 'some-review-instance-slug' } + let(:review_instance_url) { "#{review_instance_slug}.review.vetsgov-internal" } + + before { allow(Settings).to receive(:review_instance_slug).and_return(review_instance_slug) } + + it 'has a login redirect url as a parameter embedded in review instance terms of use page' do + expect(subject.terms_of_use_redirect_url) + .to eq("http://#{review_instance_url}/terms-of-use?#{expected_redirect_url_param}") + end + + it 'logs expected message and payload' do + expect(Rails.logger).to receive(:info).with(expected_log_message, expected_log_payload) + subject.terms_of_use_redirect_url + end + end + + context 'and authentication is not occurring on a review instance' do + it 'has a login redirect url as a parameter embedded in terms of use page with success' do + expect(subject.terms_of_use_redirect_url) + .to eq("#{values[:base_redirect]}/terms-of-use?#{expected_redirect_url_param}") + end + + it 'logs expected message and payload' do + expect(Rails.logger).to receive(:info).with(expected_log_message, expected_log_payload) + subject.terms_of_use_redirect_url + end + end + end + + context 'when tracker application is nil' do + let(:application) { nil } + it 'has a login redirect url as a parameter embedded in terms of use page with success' do expect(subject.terms_of_use_redirect_url) .to eq("#{values[:base_redirect]}/terms-of-use?#{expected_redirect_url_param}") @@ -646,30 +719,16 @@ subject.terms_of_use_redirect_url end end - end - - context 'when tracker application is nil' do - let(:application) { nil } - - it 'has a login redirect url as a parameter embedded in terms of use page with success' do - expect(subject.terms_of_use_redirect_url) - .to eq("#{values[:base_redirect]}/terms-of-use?#{expected_redirect_url_param}") - end - - it 'logs expected message and payload' do - expect(Rails.logger).to receive(:info).with(expected_log_message, expected_log_payload) - subject.terms_of_use_redirect_url - end - end - context 'when tracker application is not within Settings.terms_of_use.enabled_clients' do - before do - allow(Settings.terms_of_use).to receive(:enabled_clients).and_return('') - end + context 'when tracker application is not within Settings.terms_of_use.enabled_clients' do + before do + allow(Settings.terms_of_use).to receive(:enabled_clients).and_return('') + end - it 'has a login redirect url with success not embedded in a terms of use page' do - expect(subject.terms_of_use_redirect_url) - .to eq(expected_login_redirect_url) + it 'has a login redirect url with success not embedded in a terms of use page' do + expect(subject.terms_of_use_redirect_url) + .to eq(expected_login_redirect_url) + end end end end diff --git a/spec/sidekiq/va_notify_dd_email_job_spec.rb b/spec/sidekiq/va_notify_dd_email_job_spec.rb index 10a5d528e74..406ee454aa7 100644 --- a/spec/sidekiq/va_notify_dd_email_job_spec.rb +++ b/spec/sidekiq/va_notify_dd_email_job_spec.rb @@ -38,19 +38,42 @@ describe '#perform' do let(:notification_client) { double('Notifications::Client') } - %w[ch33 comp_pen].each do |dd_type| - context "with a dd type of #{dd_type}" do - it 'sends a confirmation email' do - allow(VaNotify::Service).to receive(:new) - .with(Settings.vanotify.services.va_gov.api_key).and_return(notification_client) - - expect(notification_client).to receive(:send_email).with( - email_address: email, - template_id: dd_type == 'ch33' ? 'edu_template_id' : 'comp_pen_template_id' - ) - - described_class.new.perform(email, dd_type) - end + context 'with a dd type of ch33' do + it 'sends a confirmation email using the edu template' do + allow(VaNotify::Service).to receive(:new) + .with(Settings.vanotify.services.va_gov.api_key).and_return(notification_client) + + expect(notification_client).to receive(:send_email).with( + email_address: email, template_id: 'edu_template_id' + ) + + described_class.new.perform(email, 'ch33') + end + end + + context 'with a dd type of comp_pen' do + it 'sends a confirmation email using the comp and pen template' do + allow(VaNotify::Service).to receive(:new) + .with(Settings.vanotify.services.va_gov.api_key).and_return(notification_client) + + expect(notification_client).to receive(:send_email).with( + email_address: email, template_id: 'comp_pen_template_id' + ) + + described_class.new.perform(email, 'comp_pen') + end + end + + context 'without a dd type' do + it 'sends a confirmation email using the direct_deposit template' do + allow(VaNotify::Service).to receive(:new) + .with(Settings.vanotify.services.va_gov.api_key).and_return(notification_client) + + expect(notification_client).to receive(:send_email).with( + email_address: email, template_id: 'direct_deposit_template_id' + ) + + described_class.new.perform(email, nil) end end @@ -89,4 +112,26 @@ .and trigger_statsd_increment('worker.direct_deposit_confirmation_email.error') end end + + describe '#get_template' do + let(:job) { VANotifyDdEmailJob.new } + + context 'when dd_type is nil' do + it 'returns the direct_deposit template' do + expect(job.template_type(nil)).to eq('direct_deposit') + end + end + + context 'when dd_type is comp_pen' do + it 'returns the direct_deposit template' do + expect(job.template_type('comp_pen')).to eq('direct_deposit_comp_pen') + end + end + + context 'when dd_type is edu' do + it 'returns the direct_deposit template' do + expect(job.template_type('edu')).to eq('direct_deposit') + end + end + end end