From e68aea5af52be618836e914b7f47f5986763cc51 Mon Sep 17 00:00:00 2001 From: Vraj Mohan Date: Sat, 15 Jun 2024 09:21:32 -0400 Subject: [PATCH] Remove inline invocation of RISC job **Why**: RISC notifications are delivered through an ActiveJob; inline invocation is no longer used. **How**: 1. Remove references to risc_notifications_active_job_enabled in code and assume it to be true 2. Refactor tests to assert that RiscDeliveryJob is scheduled with the correct arguments changelog: Internal, RISC, Remove obsolete code --- app/jobs/risc_delivery_job.rb | 11 +- app/services/analytics_events.rb | 3 - app/services/push_notification/http_push.rb | 10 +- config/application.yml.default | 1 - lib/identity_config.rb | 1 - spec/jobs/risc_delivery_job_spec.rb | 217 +++++------------- .../push_notification/http_push_spec.rb | 182 ++++----------- 7 files changed, 108 insertions(+), 317 deletions(-) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index 9bdbcb7cd53..c48afb8c243 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -44,7 +44,7 @@ def perform( 'Content-Type' => 'application/secevent+jwt', ) do |req| req.options.context = { - service_name: inline? ? 'risc_http_push_direct' : 'risc_http_push_async', + service_name: 'risc_http_push_async', } end end @@ -58,7 +58,7 @@ def perform( user:, ) rescue *NETWORK_ERRORS => err - raise err if self.executions < 2 && !inline? + raise err if self.executions < 2 track_event( error: err.message, @@ -68,7 +68,7 @@ def perform( user:, ) rescue RedisRateLimiter::LimitError => err - raise err if self.executions < 10 && !inline? + raise err if self.executions < 10 track_event( error: err.message, @@ -102,10 +102,6 @@ def faraday end end - def inline? - queue_adapter.is_a?(ActiveJob::QueueAdapters::InlineAdapter) - end - def track_event(event_type:, issuer:, success:, user:, error: nil, status: nil) analytics(user).risc_security_event_pushed( client_id: issuer, @@ -113,7 +109,6 @@ def track_event(event_type:, issuer:, success:, user:, error: nil, status: nil) event_type:, status:, success:, - transport: inline? ? 'direct' : 'async', ) end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index aa1aa55db71..3fc8a440dcd 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4919,14 +4919,12 @@ def return_to_sp_failure_to_proof(redirect_url:, flow: nil, step: nil, location: # @param [String] client_id # @param [String] event_type # @param [Boolean] success - # @param ['async'|'direct'] transport # @param [Integer] status # @param [String] error def risc_security_event_pushed( client_id:, event_type:, success:, - transport:, status: nil, error: nil, **extra @@ -4938,7 +4936,6 @@ def risc_security_event_pushed( event_type:, status:, success:, - transport:, **extra, ) end diff --git a/app/services/push_notification/http_push.rb b/app/services/push_notification/http_push.rb index 57a53dae816..f89d3b700bd 100644 --- a/app/services/push_notification/http_push.rb +++ b/app/services/push_notification/http_push.rb @@ -40,18 +40,12 @@ def url_options def deliver_one(service_provider) deliver_local(service_provider) if IdentityConfig.store.risc_notifications_local_enabled - job_arguments = { + RiscDeliveryJob.perform_later( push_notification_url: service_provider.push_notification_url, jwt: jwt(service_provider), event_type: event.event_type, issuer: service_provider.issuer, - } - - if IdentityConfig.store.risc_notifications_active_job_enabled - RiscDeliveryJob.perform_later(**job_arguments) - else - RiscDeliveryJob.perform_now(**job_arguments) - end + ) end def deliver_local(service_provider) diff --git a/config/application.yml.default b/config/application.yml.default index 490eac60801..def49b7f87b 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -276,7 +276,6 @@ reset_password_email_max_attempts: 20 reset_password_email_window_in_minutes: 60 reset_password_on_auth_fraud_event: true risc_notifications_local_enabled: false -risc_notifications_active_job_enabled: false risc_notifications_rate_limit_interval: 60 risc_notifications_rate_limit_max_requests: 1_000 risc_notifications_rate_limit_overrides: '{"https://example.com/push":{"interval":120,"max_requests":10000}}' diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 98314846d7b..cd9d75f75f9 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -334,7 +334,6 @@ def self.store config.add(:reset_password_email_max_attempts, type: :integer) config.add(:reset_password_email_window_in_minutes, type: :integer) config.add(:reset_password_on_auth_fraud_event, type: :boolean) - config.add(:risc_notifications_active_job_enabled, type: :boolean) config.add(:risc_notifications_local_enabled, type: :boolean) config.add(:risc_notifications_rate_limit_interval, type: :integer) config.add(:risc_notifications_rate_limit_max_requests, type: :integer) diff --git a/spec/jobs/risc_delivery_job_spec.rb b/spec/jobs/risc_delivery_job_spec.rb index 75b938183d8..ba832a7352d 100644 --- a/spec/jobs/risc_delivery_job_spec.rb +++ b/spec/jobs/risc_delivery_job_spec.rb @@ -23,7 +23,6 @@ event_type: event_type, status: nil, success: false, - transport: 'direct', } end @@ -39,6 +38,8 @@ before do allow(job).to receive(:analytics).and_return(job_analytics) + allow(job).to receive(:queue_adapter). + and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) end it 'POSTs the jwt to the given URL' do @@ -68,43 +69,24 @@ stub_request(:post, push_notification_url).to_raise(Faraday::SSLError) end - context 'when performed inline' do - it 'logs an event' do - expect { perform }.to_not raise_error - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge(error: 'Exception from WebMock'), - ) - end + it 'raises and retries via ActiveJob' do + expect { perform }.to raise_error(Faraday::SSLError) end - context 'when performed in a worker' do + context 'it has already failed twice' do before do - allow(job).to receive(:queue_adapter). - and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) + allow(job).to receive(:executions).and_return 2 end - it 'raises and retries via ActiveJob' do - expect { perform }.to raise_error(Faraday::SSLError) - end + it 'logs an event' do + expect { perform }.to_not raise_error - context 'it has already failed twice' do - before do - allow(job).to receive(:executions).and_return 2 - end - - it 'logs an event' do - expect { perform }.to_not raise_error - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'Exception from WebMock', - transport: 'async', - ), - ) - end + expect(job_analytics).to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'Exception from WebMock', + ), + ) end end end @@ -116,42 +98,24 @@ expect(job.faraday).to receive(:post).and_raise(Errno::ECONNREFUSED) end - context 'when performed inline' do - it 'logs an event' do - expect { perform }.to_not raise_error - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge(error: 'Connection refused'), - ) - end + it 'raises and retries via ActiveJob' do + expect { perform }.to raise_error(Errno::ECONNREFUSED) end - context 'when performed in a worker' do + context 'it has already failed twice' do before do - allow(job).to receive(:queue_adapter). - and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) + allow(job).to receive(:executions).and_return 2 end - it 'raises and retries via ActiveJob' do - expect { perform }.to raise_error(Errno::ECONNREFUSED) - end + it 'logs an event' do + expect { perform }.to_not raise_error - context 'it has already failed twice' do - before do - allow(job).to receive(:executions).and_return 2 - end - - it 'logs an event' do - expect { perform }.to_not raise_error - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'Connection refused', - transport: 'async', - ), - ) - end + expect(job_analytics).to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'Connection refused', + ), + ) end end end @@ -161,55 +125,33 @@ stub_request(:post, push_notification_url).to_return(status: 403) end - context 'when performed inline' do - it 'logs an event' do - expect { perform }.to_not raise_error - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'http_push_error', - status: 403, - ), - ) - end + it 'logs an event' do + expect { perform }.to_not raise_error + expect(job_analytics).to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'http_push_error', + status: 403, + ), + ) end - context 'when performed in a worker' do + context 'it has already failed twice' do before do - allow(job).to receive(:queue_adapter). - and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) + allow(job).to receive(:executions).and_return 2 end it 'logs an event' do expect { perform }.to_not raise_error + expect(job_analytics).to have_logged_event( :risc_security_event_pushed, risc_event_payload.merge( error: 'http_push_error', status: 403, - transport: 'async', ), ) end - - context 'it has already failed twice' do - before do - allow(job).to receive(:executions).and_return 2 - end - - it 'logs an event' do - expect { perform }.to_not raise_error - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'http_push_error', - status: 403, - transport: 'async', - ), - ) - end - end end end @@ -218,9 +160,18 @@ stub_request(:post, push_notification_url).to_timeout end - context 'when performed inline' do + it 'raises and retries via ActiveJob' do + expect { perform }.to raise_error(Faraday::ConnectionFailed) + end + + context 'it has already failed twice' do + before do + allow(job).to receive(:executions).and_return 2 + end + it 'logs an event' do expect { perform }.to_not raise_error + expect(job_analytics).to have_logged_event( :risc_security_event_pushed, risc_event_payload.merge( @@ -229,35 +180,6 @@ ) end end - - context 'when performed in a worker' do - before do - allow(job).to receive(:queue_adapter). - and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) - end - - it 'raises and retries via ActiveJob' do - expect { perform }.to raise_error(Faraday::ConnectionFailed) - end - - context 'it has already failed twice' do - before do - allow(job).to receive(:executions).and_return 2 - end - - it 'logs an event' do - expect { perform }.to_not raise_error - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'execution expired', - transport: 'async', - ), - ) - end - end - end end context 'rate limiting' do @@ -267,9 +189,18 @@ end end - context 'when performed inline' do - it 'logs an event on limit hit' do + it 'raises on rate limit errors (and retries via ActiveJob)' do + expect { perform }.to raise_error(RedisRateLimiter::LimitError) + end + + context 'it has already failed ten times' do + before do + allow(job).to receive(:executions).and_return 10 + end + + it 'logs an event' do expect { perform }.to_not raise_error + expect(job_analytics).to have_logged_event( :risc_security_event_pushed, risc_event_payload.merge( @@ -279,35 +210,6 @@ end end - context 'when performed in a worker' do - before do - allow(job).to receive(:queue_adapter). - and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) - end - - it 'raises on rate limit errors (and retries via ActiveJob)' do - expect { perform }.to raise_error(RedisRateLimiter::LimitError) - end - - context 'it has already failed ten times' do - before do - allow(job).to receive(:executions).and_return 10 - end - - it 'logs an event' do - expect { perform }.to_not raise_error - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'rate limit for push-notification-https://push.example.gov has maxed out', - transport: 'async', - ), - ) - end - end - end - context 'when the rate limit is overridden' do before do allow(IdentityConfig.store).to receive(:risc_notifications_rate_limit_overrides). @@ -324,7 +226,6 @@ risc_event_payload.merge( success: true, status: 200, - transport: 'direct', ), ) end diff --git a/spec/services/push_notification/http_push_spec.rb b/spec/services/push_notification/http_push_spec.rb index 6e3e08b2c8b..6a854c044b4 100644 --- a/spec/services/push_notification/http_push_spec.rb +++ b/spec/services/push_notification/http_push_spec.rb @@ -22,87 +22,55 @@ ) end let(:now) { Time.zone.now } - let(:risc_notifications_active_job_enabled) { false } let(:push_notifications_enabled) { true } - let(:job_analytics) { FakeAnalytics.new } - let(:risc_event_payload) do - { - client_id: sp_with_push_url.issuer, - error: nil, - event_type: event.event_type, - status: nil, - success: false, - transport: 'direct', - } - end subject(:http_push) { PushNotification::HttpPush.new(event, now: now) } before do - allow(IdentityConfig.store).to receive(:risc_notifications_active_job_enabled). - and_return(risc_notifications_active_job_enabled) + ActiveJob::Base.queue_adapter = :test allow(Identity::Hostdata).to receive(:env).and_return('dev') allow(IdentityConfig.store).to receive(:push_notifications_enabled). and_return(push_notifications_enabled) - allow(Analytics).to receive(:new).and_return(job_analytics) end describe '#deliver' do subject(:deliver) { http_push.deliver } - context 'when push_notifications_enabled is disabled' do - let(:push_notifications_enabled) { false } - - it 'does not deliver any notifications' do - expect(http_push).to_not receive(:deliver_one) - - deliver - end + it 'enqueues a background job to deliver a notification' do + expect { deliver }.to have_enqueued_job(RiscDeliveryJob).once end - context 'when risc_notifications_active_job_enabled is enabled' do - let(:risc_notifications_active_job_enabled) { true } + it 'enqueues a background job with the correct arguments' do + expect { deliver }.to have_enqueued_job(RiscDeliveryJob).with { |args| + expect(args[:push_notification_url]).to eq sp_with_push_url.push_notification_url + expect(args[:event_type]).to eq event.event_type + expect(args[:issuer]).to eq sp_with_push_url.issuer + + jwt_payload, headers = JWT.decode( + args[:jwt], + AppArtifacts.store.oidc_public_key, + true, + algorithm: 'RS256', + kid: JWT::JWK.new(AppArtifacts.store.oidc_private_key).kid, + ) - it 'delivers a notification via background job' do - expect(RiscDeliveryJob).to receive(:perform_later) + expect(headers['typ']).to eq('secevent+jwt') + expect(headers['kid']).to eq(JWT::JWK.new(AppArtifacts.store.oidc_private_key).kid) - deliver - end + expect(jwt_payload['iss']).to eq(root_url) + expect(jwt_payload['iat']).to eq(now.to_i) + expect(jwt_payload['exp']).to eq((now + 12.hours).to_i) + expect(jwt_payload['aud']).to eq(sp_with_push_url.push_notification_url) + expect(jwt_payload['events']).to eq(event.event_type => event.payload.as_json) + } end - it 'makes an HTTP post to service providers with a push_notification_url' do - stub_request(:post, sp_with_push_url.push_notification_url). - with do |request| - expect(request.headers['Content-Type']).to eq('application/secevent+jwt') - expect(request.headers['Accept']).to eq('application/json') - - payload, headers = JWT.decode( - request.body, - AppArtifacts.store.oidc_public_key, - true, - algorithm: 'RS256', - kid: JWT::JWK.new(AppArtifacts.store.oidc_private_key).kid, - ) + context 'when push_notifications_enabled is false' do + let(:push_notifications_enabled) { false } - expect(headers['typ']).to eq('secevent+jwt') - expect(headers['kid']).to eq(JWT::JWK.new(AppArtifacts.store.oidc_private_key).kid) - - expect(payload['iss']).to eq(root_url) - expect(payload['iat']).to eq(now.to_i) - expect(payload['exp']).to eq((now + 12.hours).to_i) - expect(payload['aud']).to eq(sp_with_push_url.push_notification_url) - expect(payload['events']).to eq(event.event_type => event.payload.as_json) - end - - deliver - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - status: 200, - success: true, - ), - ) + it 'does not enqueue a RISC notification' do + expect { deliver }.not_to have_enqueued_job(RiscDeliveryJob) + end end context 'with an event that sends agency-specific iss_sub' do @@ -111,97 +79,35 @@ let(:agency_uuid) { AgencyIdentityLinker.new(sp_with_push_url_identity).link_identity.uuid } it 'sends the agency-specific uuid' do - stub_request(:post, sp_with_push_url.push_notification_url). - with do |request| - payload, _headers = JWT.decode( - request.body, - AppArtifacts.store.oidc_public_key, - true, - algorithm: 'RS256', - ) - - expect(payload['events'][event.event_type]['subject']['sub']).to eq(agency_uuid) - end - - deliver - end - end - - context 'with a timeout when posting to one url' do - let(:third_sp) { create(:service_provider, active: true, push_notification_url: 'http://sp.url/push') } - - before do - IdentityLinker.new(user, third_sp).link_identity - - stub_request(:post, sp_with_push_url.push_notification_url).to_timeout - stub_request(:post, third_sp.push_notification_url).to_return(status: 200) - end - - it 'still posts to the others' do - deliver - - expect(WebMock).to have_requested(:post, third_sp.push_notification_url) - end - - it 'logs both events' do - deliver - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'execution expired', - ), - ) - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - client_id: third_sp.issuer, - status: 200, - success: true, - ), - ) - end - end - - context 'with a non-200 response from a push notification url' do - before do - stub_request(:post, sp_with_push_url.push_notification_url). - to_return(status: 500) - end - - it 'logs an event' do - deliver - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'http_push_error', - status: 500, - ), - ) + expect { deliver }.to have_enqueued_job(RiscDeliveryJob).with { |args| + jwt_payload, _headers = JWT.decode( + args[:jwt], + AppArtifacts.store.oidc_public_key, + true, + algorithm: 'RS256', + kid: JWT::JWK.new(AppArtifacts.store.oidc_private_key).kid, + ) + expect(jwt_payload['events'][event.event_type]['subject']['sub']).to eq(agency_uuid) + } end end context 'when a service provider is no longer active' do before { sp_with_push_url.update!(active: false) } - it 'does not notify that SP' do - deliver - - expect(WebMock).not_to have_requested(:get, sp_with_push_url.push_notification_url) + it 'does not enqueue a RISC notification' do + expect { deliver }.not_to have_enqueued_job(RiscDeliveryJob) end end - context 'when a user has revoked access to an SP' do + context 'when a user has revoked access to a service provider' do before do identity = user.identities.find_by(service_provider: sp_with_push_url.issuer) RevokeServiceProviderConsent.new(identity).call end - it 'does not notify that SP' do - deliver - - expect(WebMock).not_to have_requested(:get, sp_with_push_url.push_notification_url) + it 'does not enqueue a RISC notification' do + expect { deliver }.not_to have_enqueued_job(RiscDeliveryJob) end end end