From 5da43b11b18501b6718db7e64b7f35fedeb34785 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 29 Aug 2023 18:08:12 +0100 Subject: [PATCH 1/3] Make UserLockingTest more robust I'm about to make some changes to EventLog.record_event and I don't want those changes to affect this test. Previously we were asserting that 8 failed sign-in attempts resulted in 10 enqueued jobs with a comment explaining that 9 of these jobs were the result of calls to EventLog.record_event. I've changed the test so that (a) it uses the value of User.maximum_attempts defined in the Devise configuration rather than the magic number 8 (which was actually incorrect; it should've been 6!); and (b) it ensures a single email (as opposed to any old job) is enqueued when the maximum number of attempts is reached. I did attempt to add a more specific assertion using ActionMailer::TestHelper#assert_enqueued_email_with, but I couldn't get it to work. --- test/integration/user_locking_test.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/integration/user_locking_test.rb b/test/integration/user_locking_test.rb index 7ff9a7724..325b85f56 100644 --- a/test/integration/user_locking_test.rb +++ b/test/integration/user_locking_test.rb @@ -25,10 +25,12 @@ class UserLockingTest < ActionDispatch::IntegrationTest user = create(:user) visit root_path - # One job is enqueued to send the email, 9 jobs are enqueued to stream log entries - # for each incorrect login attempt and the email sending - assert_enqueued_jobs(10) do - 8.times { signin_with(email: user.email, password: "wrong password") } + assert_no_enqueued_emails do + (User.maximum_attempts - 1).times { signin_with(email: user.email, password: "wrong password") } + end + + assert_enqueued_emails(1) do + signin_with(email: user.email, password: "wrong password") end end From 034c46d27f4058bd7ba0183d4501659a1732d7a1 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 29 Aug 2023 17:12:24 +0100 Subject: [PATCH 2/3] Extract EventLog.splunk_endpoint_enabled? method I'm planning to re-use this in EventLog.record_event in a subsequent commit. I've also added some test coverage for EventLog#send_to_splunk as well as the newly extract method. --- app/models/event_log.rb | 6 +++- test/factories/event_log.rb | 5 +++- test/models/event_log_test.rb | 53 +++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/app/models/event_log.rb b/app/models/event_log.rb index 4327a668d..4793f1844 100644 --- a/app/models/event_log.rb +++ b/app/models/event_log.rb @@ -86,7 +86,7 @@ def ip_address_string end def send_to_splunk(*) - return unless ENV["SPLUNK_EVENT_LOG_ENDPOINT_URL"] && ENV["SPLUNK_EVENT_LOG_ENDPOINT_HEC_TOKEN"] + return unless self.class.splunk_endpoint_enabled? event = { timestamp: created_at.utc, @@ -155,6 +155,10 @@ def self.convert_integer_to_ip_address(integer) end end + def self.splunk_endpoint_enabled? + ENV["SPLUNK_EVENT_LOG_ENDPOINT_URL"] && ENV["SPLUNK_EVENT_LOG_ENDPOINT_HEC_TOKEN"] + end + private def validate_event_mappable diff --git a/test/factories/event_log.rb b/test/factories/event_log.rb index 64f5ea17b..341646d00 100644 --- a/test/factories/event_log.rb +++ b/test/factories/event_log.rb @@ -1,3 +1,6 @@ FactoryBot.define do - factory :event_log + factory :event_log do + event_id { EventLog::NO_SUCH_ACCOUNT_LOGIN.id } + uid { create(:user).uid } + end end diff --git a/test/models/event_log_test.rb b/test/models/event_log_test.rb index 5720a15f7..2465cb54d 100644 --- a/test/models/event_log_test.rb +++ b/test/models/event_log_test.rb @@ -130,4 +130,57 @@ class EventLogTest < ActiveSupport::TestCase assert_equal admin, event_log.initiator assert_equal EventLog::ACCOUNT_INVITED, event_log.entry end + + context "when Splunk endpoint enabled" do + setup do + EventLog.stubs(:splunk_endpoint_enabled?).returns(true) + end + + should "send event to Splunk endpoint" do + ClimateControl.modify( + SPLUNK_EVENT_LOG_ENDPOINT_URL: "http://example.com/splunk", + SPLUNK_EVENT_LOG_ENDPOINT_HEC_TOKEN: "hec-token", + ) do + request = stub_request(:post, "http://example.com/splunk") + event_log = create(:event_log) + event_log.send_to_splunk + assert_requested request + end + end + end + + context "when Splunk endpoint disabled" do + setup do + EventLog.stubs(:splunk_endpoint_enabled?).returns(false) + end + + should "not send event to Splunk endpoint" do + request = stub_request(:post, "http://example.com/splunk") + event_log = create(:event_log) + event_log.send_to_splunk + assert_not_requested request + end + end + + context "when Splunk-related env vars are defined" do + should "return true for splunk_endpoint_enabled?" do + ClimateControl.modify( + SPLUNK_EVENT_LOG_ENDPOINT_URL: "url", + SPLUNK_EVENT_LOG_ENDPOINT_HEC_TOKEN: "hec-token", + ) do + assert EventLog.splunk_endpoint_enabled? + end + end + end + + context "when Splunk-related env vars are not defined" do + should "return false for splunk_endpoint_enabled?" do + ClimateControl.modify( + SPLUNK_EVENT_LOG_ENDPOINT_URL: nil, + SPLUNK_EVENT_LOG_ENDPOINT_HEC_TOKEN: nil, + ) do + assert_not EventLog.splunk_endpoint_enabled? + end + end + end end From c5af3679e1c06c0ed269fd984c5a2d1e4f92fba7 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 29 Aug 2023 16:40:58 +0100 Subject: [PATCH 3/3] Avoid queueing job if Splunk-related functionality disabled Trello: https://trello.com/c/q1B1HKBy Currently the Splunk-related functionality is effectively disabled, because neither of the relevant env vars are defined in any environment. See this Trello card [1] for more details. Prior to this commit, we were always queueing up a Sidekiq job whenever EventLog.record_event was called even though, as soon as that SplunkLogStreamingJob was executed, the first thing we did was give up if both the relevant env vars were not set. Now we only queue up the job in the first place if the relevant env vars are defined. I've left the original check in EventLog#send_to_splunk in place, because there's a small chance the state of the env vars could change between the job being queued and the job being executed. [1]: https://trello.com/c/Zp23Kd6c --- app/models/event_log.rb | 4 +++- test/models/event_log_test.rb | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/models/event_log.rb b/app/models/event_log.rb index 4793f1844..51a1e61e0 100644 --- a/app/models/event_log.rb +++ b/app/models/event_log.rb @@ -119,7 +119,9 @@ def self.record_event(user, event, options = {}) event_log_entry = EventLog.create!(attributes) - SplunkLogStreamingJob.perform_later(event_log_entry.id) + if splunk_endpoint_enabled? + SplunkLogStreamingJob.perform_later(event_log_entry.id) + end event_log_entry end diff --git a/test/models/event_log_test.rb b/test/models/event_log_test.rb index 2465cb54d..27562ce3e 100644 --- a/test/models/event_log_test.rb +++ b/test/models/event_log_test.rb @@ -131,6 +131,28 @@ class EventLogTest < ActiveSupport::TestCase assert_equal EventLog::ACCOUNT_INVITED, event_log.entry end + context "when Splunk endpoint enabled" do + setup do + EventLog.stubs(:splunk_endpoint_enabled?).returns(true) + end + + should "queue job to send event to Splunk endpoint" do + SplunkLogStreamingJob.expects(:perform_later) + EventLog.record_event(build(:user), EventLog::SUCCESSFUL_LOGIN) + end + end + + context "when Splunk endpoint disabled" do + setup do + EventLog.stubs(:splunk_endpoint_enabled?).returns(false) + end + + should "not queue job to send event to Splunk endpoint" do + SplunkLogStreamingJob.expects(:perform_later).never + EventLog.record_event(build(:user), EventLog::SUCCESSFUL_LOGIN) + end + end + context "when Splunk endpoint enabled" do setup do EventLog.stubs(:splunk_endpoint_enabled?).returns(true)