From 07eb3c57def1b6ff2164aa5226a7a8565d23356b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 10 May 2024 16:02:57 -0400 Subject: [PATCH 1/4] LG-12214: Refresh device cookie on every user action changelog: Bug Fixes, New Device Detection, Extend duration of permanent device cookie on every user event --- app/services/user_event_creator.rb | 10 +++------- spec/services/user_event_creator_spec.rb | 20 +++++++++++++++----- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/app/services/user_event_creator.rb b/app/services/user_event_creator.rb index 0db96a2d2b3..106cd0de154 100644 --- a/app/services/user_event_creator.rb +++ b/app/services/user_event_creator.rb @@ -102,19 +102,13 @@ def create_event_for_new_device(event_type:, user:, disavowal_token:) def create_device_for_user(user) cookie_uuid = cookies[:device].presence || SecureRandom.hex(COOKIE_LENGTH / 2) - device = Device.create!( + Device.create!( user: user, user_agent: request.user_agent.to_s, cookie_uuid: cookie_uuid, last_used_at: Time.zone.now, last_ip: request.remote_ip, ) - assign_device_cookie(device.cookie_uuid) - device - end - - def assign_device_cookie(device_cookie) - cookies.permanent[:device] = device_cookie unless device_cookie == cookies[:device] end def send_new_device_notification(event:, device:, disavowal_token:) @@ -134,6 +128,8 @@ def create_event_for_device(event_type:, user:, device:, disavowal_token: nil) disavowal_token_fingerprint: disavowal_token_fingerprint, ) + cookies.permanent[:device] = device.cookie_uuid if device + [event, disavowal_token] end diff --git a/spec/services/user_event_creator_spec.rb b/spec/services/user_event_creator_spec.rb index 7d9b6eb8f02..efac2e2e974 100644 --- a/spec/services/user_event_creator_spec.rb +++ b/spec/services/user_event_creator_spec.rb @@ -5,11 +5,9 @@ let(:ip_address) { '4.4.4.4' } let(:existing_device_cookie) { 'existing_device_cookie' } let(:cookie_jar) do - { - device: existing_device_cookie, - }.with_indifferent_access.tap do |cookie_jar| - allow(cookie_jar).to receive(:permanent).and_return({}) - end + cookie_jar = ActionDispatch::Cookies::CookieJar.new(Rails.configuration.action_dispatch) + cookie_jar.permanent[:device] = existing_device_cookie if existing_device_cookie + cookie_jar end let(:request) do double( @@ -41,6 +39,12 @@ expect(device.last_ip).to eq(ip_address) expect(device.last_used_at).to be_within(1).of(Time.zone.now) end + + it 'saves the cookie permanently' do + expect(cookie_jar.permanent).to receive(:[]=).with(:device, existing_device_cookie) + + subject.create_user_event(event_type, user) + end end context 'when a device exists that is not associated with the user' do @@ -95,6 +99,12 @@ expect(event.device.cookie_uuid.length).to eq(UserEventCreator::COOKIE_LENGTH) end + + it 'saves the cookie permanently' do + expect { subject.create_user_event(event_type, user) }.to change { cookie_jar[:device] }. + from(nil). + to(kind_of(String)) + end end it 'alerts the user if they have other devices' do From 78a7cbfe9292aa40f88d8e19f687720e6ad700f8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 10 May 2024 16:15:28 -0400 Subject: [PATCH 2/4] Improve wording of test description --- spec/services/user_event_creator_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/user_event_creator_spec.rb b/spec/services/user_event_creator_spec.rb index efac2e2e974..938d8af7dba 100644 --- a/spec/services/user_event_creator_spec.rb +++ b/spec/services/user_event_creator_spec.rb @@ -40,7 +40,7 @@ expect(device.last_used_at).to be_within(1).of(Time.zone.now) end - it 'saves the cookie permanently' do + it 'refreshes the permanent cookie' do expect(cookie_jar.permanent).to receive(:[]=).with(:device, existing_device_cookie) subject.create_user_event(event_type, user) From d8705cebb2437b0d81be078ad2370a0358b0ad21 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 10 May 2024 16:18:28 -0400 Subject: [PATCH 3/4] Assert specific device UUID --- spec/services/user_event_creator_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/user_event_creator_spec.rb b/spec/services/user_event_creator_spec.rb index 938d8af7dba..6f15de15628 100644 --- a/spec/services/user_event_creator_spec.rb +++ b/spec/services/user_event_creator_spec.rb @@ -103,7 +103,7 @@ it 'saves the cookie permanently' do expect { subject.create_user_event(event_type, user) }.to change { cookie_jar[:device] }. from(nil). - to(kind_of(String)) + to(lambda { |value| value == Device.last.cookie_uuid }) end end From fd9b104a058acf3ebb18c329b6e44f27f7e0bfe2 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 10 May 2024 16:30:48 -0400 Subject: [PATCH 4/4] Add spec for refreshing cookie on subsequent sign-in --- .../users/sessions_controller_spec.rb | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 26131776f4b..81ab41fc292 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -65,6 +65,31 @@ expect(subject.session[:sign_in_flow]).to eq(:sign_in) end + it 'saves and refreshes cookie for device for successful authentication' do + user = create(:user, :fully_registered) + + first_expires = nil + + freeze_time do + post :create, params: { user: { email: user.email, password: user.password } } + + device_cookie = response.headers['set-cookie'].find { |c| c.start_with?('device=') } + first_expires = CGI::Cookie.parse(device_cookie)['expires'].first + expect(Time.zone.parse(first_expires)).to be >= 20.years.from_now + end + + sign_out(user) + expect(cookies[:device]).to be_present + + travel_to 10.minutes.from_now do + post :create, params: { user: { email: user.email, password: user.password } } + + device_cookie = response.headers['set-cookie'].find { |c| c.start_with?('device=') } + second_expires = CGI::Cookie.parse(device_cookie)['expires'].first + expect(Time.zone.parse(second_expires)).to be >= Time.zone.parse(first_expires) + 10.minutes + end + end + it 'tracks the unsuccessful authentication for existing user' do user = create(:user, :fully_registered)