diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index b57e8649a23..c7b6e6f885f 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -22,9 +22,13 @@ def show def device_and_events user_id = current_user.id - @events = DeviceTracking::ListDeviceEvents.call(user_id, device_id, 0, EVENTS_PAGE_SIZE). - map(&:decorate) - @device = Device.where(user_id: user_id).find(device_id) + device = Device.where(user_id: user_id).find(device_id) + return if !device + + @events = Event.where(user_id: user_id, device_id: device.id).order(created_at: :desc). + limit(EVENTS_PAGE_SIZE). + map(&:decorate) + @device = device.decorate end def device_id diff --git a/app/controllers/users/forget_all_browsers_controller.rb b/app/controllers/users/forget_all_browsers_controller.rb index ab8e41e12bf..544bfa3cfeb 100644 --- a/app/controllers/users/forget_all_browsers_controller.rb +++ b/app/controllers/users/forget_all_browsers_controller.rb @@ -7,7 +7,7 @@ def show end def destroy - DeviceTracking::ForgetAllBrowsers.new(current_user).call + ForgetAllBrowsers.new(current_user).call analytics.track_event(Analytics::FORGET_ALL_BROWSERS_SUBMITTED) diff --git a/app/decorators/device_decorator.rb b/app/decorators/device_decorator.rb index 491a4cd39b8..6b5b0bd56fa 100644 --- a/app/decorators/device_decorator.rb +++ b/app/decorators/device_decorator.rb @@ -1,5 +1,5 @@ DeviceDecorator = Struct.new(:device) do - delegate :nice_name, :last_used_at, :id, to: :device + delegate :last_used_at, :id, to: :device def last_sign_in_location_and_ip I18n.t('account.index.sign_in_location_and_ip', location: last_location, ip: device.last_ip) @@ -12,4 +12,13 @@ def last_location def happened_at device.last_used_at.utc end + + def nice_name + browser = BrowserCache.parse(device.user_agent) + I18n.t( + 'account.index.device', + browser: "#{browser.name} #{browser.version}", + os: "#{browser.platform.name} #{browser.platform.version.split('.').first}", + ) + end end diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index 6ec0cf2d5a8..e2d74f94588 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -143,7 +143,8 @@ def identity_events end def recent_devices - DeviceTracking::ListDevices.call(user.id, 0, MAX_RECENT_DEVICES).map(&:decorate) + @recent_devices ||= user.devices.order(last_used_at: :desc).limit(MAX_RECENT_DEVICES). + map(&:decorate) end def devices? diff --git a/app/models/device.rb b/app/models/device.rb index c3a07ea342c..89941e00b1a 100644 --- a/app/models/device.rb +++ b/app/models/device.rb @@ -13,7 +13,11 @@ def decorate DeviceDecorator.new(self) end - def device_name - DeviceTracking::DeviceName.call(self) + # @return [Device] + def update_last_used_ip(remote_ip, now: Time.zone.now) + self.last_used_at = now + self.last_ip = remote_ip + save + self end end diff --git a/app/services/device_tracking/create_device.rb b/app/services/device_tracking/create_device.rb deleted file mode 100644 index 4ae66eb4a8f..00000000000 --- a/app/services/device_tracking/create_device.rb +++ /dev/null @@ -1,15 +0,0 @@ -module DeviceTracking - class CreateDevice - COOKIE_LENGTH = 128 - - def self.call(user_id, remote_ip, user_agent, uuid) - Device.create!( - user_id: user_id, - user_agent: user_agent.to_s, - cookie_uuid: uuid.presence || SecureRandom.hex(COOKIE_LENGTH / 2), - last_used_at: Time.zone.now, - last_ip: remote_ip, - ) - end - end -end diff --git a/app/services/device_tracking/device_name.rb b/app/services/device_tracking/device_name.rb deleted file mode 100644 index 4e23d2ee108..00000000000 --- a/app/services/device_tracking/device_name.rb +++ /dev/null @@ -1,12 +0,0 @@ -module DeviceTracking - class DeviceName - def self.call(device) - browser = BrowserCache.parse(device.user_agent) - I18n.t( - 'account.index.device', - browser: "#{browser.name} #{browser.version}", - os: "#{browser.platform.name} #{browser.platform.version.split('.').first}", - ) - end - end -end diff --git a/app/services/device_tracking/forget_all_browsers.rb b/app/services/device_tracking/forget_all_browsers.rb deleted file mode 100644 index 2fb47b21a9e..00000000000 --- a/app/services/device_tracking/forget_all_browsers.rb +++ /dev/null @@ -1,19 +0,0 @@ -module DeviceTracking - class ForgetAllBrowsers - attr_reader :user, :remember_device_revoked_at - - def initialize(user, remember_device_revoked_at: nil) - @user = user - @remember_device_revoked_at = remember_device_revoked_at || Time.zone.now - end - - def call - UpdateUser.new( - user: user, - attributes: { - remember_device_revoked_at: remember_device_revoked_at, - }, - ).call - end - end -end diff --git a/app/services/device_tracking/list_device_events.rb b/app/services/device_tracking/list_device_events.rb deleted file mode 100644 index 3b1e6cc6b8a..00000000000 --- a/app/services/device_tracking/list_device_events.rb +++ /dev/null @@ -1,9 +0,0 @@ -module DeviceTracking - class ListDeviceEvents - def self.call(user_id, device_id, offset, limit) - return [] unless Device.where(user_id: user_id, id: device_id) - Event.where(user_id: user_id, device_id: device_id).order(created_at: :desc). - offset(offset).limit(limit) - end - end -end diff --git a/app/services/device_tracking/list_devices.rb b/app/services/device_tracking/list_devices.rb deleted file mode 100644 index 6df970c3c4f..00000000000 --- a/app/services/device_tracking/list_devices.rb +++ /dev/null @@ -1,9 +0,0 @@ -module DeviceTracking - class ListDevices - def self.call(user_id, offset, limit) - devices = Device.where(user_id: user_id).order(last_used_at: :desc).offset(offset). - limit(limit) - devices.each { |device| device.nice_name = DeviceName.call(device) } - end - end -end diff --git a/app/services/device_tracking/lookup_device_for_user.rb b/app/services/device_tracking/lookup_device_for_user.rb deleted file mode 100644 index 6c3d6554502..00000000000 --- a/app/services/device_tracking/lookup_device_for_user.rb +++ /dev/null @@ -1,7 +0,0 @@ -module DeviceTracking - class LookupDeviceForUser - def self.call(user_id, cookie_guid) - Device.find_by(user_id: user_id, cookie_uuid: cookie_guid) - end - end -end diff --git a/app/services/device_tracking/update_device.rb b/app/services/device_tracking/update_device.rb deleted file mode 100644 index 492a0ab684c..00000000000 --- a/app/services/device_tracking/update_device.rb +++ /dev/null @@ -1,10 +0,0 @@ -module DeviceTracking - class UpdateDevice - def self.call(device, remote_ip) - device.last_used_at = Time.zone.now - device.last_ip = remote_ip - device.save - device - end - end -end diff --git a/app/services/forget_all_browsers.rb b/app/services/forget_all_browsers.rb new file mode 100644 index 00000000000..2b992ea8a25 --- /dev/null +++ b/app/services/forget_all_browsers.rb @@ -0,0 +1,17 @@ +class ForgetAllBrowsers + attr_reader :user, :remember_device_revoked_at + + def initialize(user, remember_device_revoked_at: nil) + @user = user + @remember_device_revoked_at = remember_device_revoked_at || Time.zone.now + end + + def call + UpdateUser.new( + user: user, + attributes: { + remember_device_revoked_at: remember_device_revoked_at, + }, + ).call + end +end diff --git a/app/services/reset_user_password.rb b/app/services/reset_user_password.rb index 81522acc0db..0ef7277eb2d 100644 --- a/app/services/reset_user_password.rb +++ b/app/services/reset_user_password.rb @@ -20,7 +20,7 @@ def reset_user_password end def forget_all_browsers - DeviceTracking::ForgetAllBrowsers.new( + ForgetAllBrowsers.new( user, remember_device_revoked_at: remember_device_revoked_at, ).call diff --git a/app/services/user_event_creator.rb b/app/services/user_event_creator.rb index 78be4f358df..ac35072381f 100644 --- a/app/services/user_event_creator.rb +++ b/app/services/user_event_creator.rb @@ -1,4 +1,6 @@ class UserEventCreator + COOKIE_LENGTH = 128 + attr_reader :request, :current_user def initialize(current_user:, request: nil) @@ -8,7 +10,7 @@ def initialize(current_user:, request: nil) def create_user_event(event_type, user = current_user) return unless user&.id - existing_device = DeviceTracking::LookupDeviceForUser.call(user.id, cookies[:device]) + existing_device = Device.find_by(user_id: user.id, cookie_uuid: cookies[:device]) if existing_device.present? create_event_for_existing_device(event_type: event_type, user: user, device: existing_device) else @@ -30,7 +32,7 @@ def create_user_event_with_disavowal(event_type, user = current_user) private def create_event_for_existing_device(event_type:, user:, device:) - DeviceTracking::UpdateDevice.call(device, request.remote_ip) + device.update_last_used_ip(request.remote_ip) create_event_for_device(event_type: event_type, user: user, device: device) end @@ -47,8 +49,14 @@ def create_event_for_new_device(event_type:, user:) end def create_device_for_user(user) - device = DeviceTracking::CreateDevice.call( - user.id, request.remote_ip, request.user_agent, cookies[:device] + cookie_uuid = cookies[:device].presence || SecureRandom.hex(COOKIE_LENGTH / 2) + + device = 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 diff --git a/app/views/events/show.html.erb b/app/views/events/show.html.erb index b52a9eecf64..f2f12b2a75f 100644 --- a/app/views/events/show.html.erb +++ b/app/views/events/show.html.erb @@ -11,7 +11,7 @@

- <%= @device.device_name %> + <%= @device.nice_name %>

<% @events.each do |event| %> diff --git a/spec/decorators/device_decorator_spec.rb b/spec/decorators/device_decorator_spec.rb new file mode 100644 index 00000000000..81ad17e80e6 --- /dev/null +++ b/spec/decorators/device_decorator_spec.rb @@ -0,0 +1,12 @@ +require 'rails_helper' + +RSpec.describe DeviceDecorator do + let(:device) { create(:device) } + subject(:decorator) { DeviceDecorator.new(device) } + + describe '#nice_name' do + it 'gives a shortened os and browser name' do + expect(decorator.nice_name).to eq('Chrome 58 on Windows 10') + end + end +end diff --git a/spec/models/device_spec.rb b/spec/models/device_spec.rb index 8b20994e2ce..2e040326538 100644 --- a/spec/models/device_spec.rb +++ b/spec/models/device_spec.rb @@ -15,4 +15,20 @@ expect(device).to be_valid end end + + describe '#update_last_used_ip' do + let(:user) { create(:user) } + let(:remote_ip) { '1.2.3.4' } + let(:user_agent) { 'Chrome/58.0.3029.110 Safari/537.36' } + let(:uuid) { 'abc123' } + let(:now) { Time.zone.now } + let(:old_timestamp) { now - 1.hour } + let(:device) { create(:device, last_used_at: old_timestamp) } + + it 'updates the last ip and last_used_at' do + expect { device.update_last_used_ip(remote_ip) }. + to(change { device.reload.last_used_at.to_i }.from(old_timestamp.to_i).to(now.to_i). + and(change { device.reload.last_ip }.to(remote_ip))) + end + end end diff --git a/spec/services/device_tracking/create_device_spec.rb b/spec/services/device_tracking/create_device_spec.rb deleted file mode 100644 index 20151fde42f..00000000000 --- a/spec/services/device_tracking/create_device_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'rails_helper' - -describe DeviceTracking::CreateDevice do - subject { described_class } - let(:user) { create(:user) } - let(:remote_ip) { '1.2.3.4' } - let(:user_agent) { 'Chrome/58.0.3029.110 Safari/537.36' } - let(:uuid) { 'abc123' } - - it 'creates a new device' do - device = subject.call(user.id, remote_ip, user_agent, uuid) - expect(device.cookie_uuid).to eq uuid - expect(device.user_agent).to eq user_agent - expect(device.last_ip).to eq remote_ip - expect(device.last_used_at).to be_present - end - - it 'uuid defaults to new random string if no cookie uuid is supplied' do - device = subject.call(user.id, remote_ip, user_agent, nil) - - expect(device.cookie_uuid.length).to eq(DeviceTracking::CreateDevice::COOKIE_LENGTH) - end -end diff --git a/spec/services/device_tracking/device_name_spec.rb b/spec/services/device_tracking/device_name_spec.rb deleted file mode 100644 index 050e21f4d6e..00000000000 --- a/spec/services/device_tracking/device_name_spec.rb +++ /dev/null @@ -1,12 +0,0 @@ -require 'rails_helper' - -describe DeviceTracking::DeviceName do - subject { described_class } - let(:device) { create(:device) } - - it 'gives a shortened os and browser name' do - result = subject.call(device) - - expect(result).to eq('Chrome 58 on Windows 10') - end -end diff --git a/spec/services/device_tracking/list_device_events_spec.rb b/spec/services/device_tracking/list_device_events_spec.rb deleted file mode 100644 index 6ea72aa1d14..00000000000 --- a/spec/services/device_tracking/list_device_events_spec.rb +++ /dev/null @@ -1,55 +0,0 @@ -require 'rails_helper' - -describe DeviceTracking::ListDeviceEvents do - subject { described_class } - - let(:current_user) { create(:user) } - let(:other_user) { create(:user) } - - let(:current_device) { create(:device, user: current_user) } - let(:other_device) { create(:device) } - - let(:events) do - [ - create(:event, user: current_user, device: current_device, created_at: 2.hours.ago), - create(:event, user: current_user, device: current_device, created_at: 3.hours.ago), - create(:event, user: current_user, device: current_device, created_at: 1.hour.ago), - ] - end - - before do - # Memoize records to run creates for specs that query the table - current_user - other_user - current_device - other_device - events - end - - it 'returns an empty list if the device is not found' do - result = subject.call(current_user.id, other_device.id, 0, 1) - - expect(result).to eq([]) - end - - it 'returns an empty list if the device is found but the user is not' do - result = subject.call(other_user.id, current_device.id, 0, 1) - - expect(result).to eq([]) - end - - it 'returns a list containing a single most recent event' do - result = subject.call(current_user.id, current_device.id, 0, 1) - - expect(result.size).to eq(1) - expect(result[0].id).to eq(events[2].id) - end - - it 'returns a list containing multiple most recent events in order and using an offset' do - result = subject.call(current_user.id, current_device.id, 1, 2) - - expect(result.size).to eq(2) - expect(result[0].id).to eq(events[0].id) - expect(result[1].id).to eq(events[1].id) - end -end diff --git a/spec/services/device_tracking/list_devices_spec.rb b/spec/services/device_tracking/list_devices_spec.rb deleted file mode 100644 index 20539645d18..00000000000 --- a/spec/services/device_tracking/list_devices_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -require 'rails_helper' - -describe DeviceTracking::ListDevices do - subject { described_class } - - let(:current_user) { create(:user) } - let(:other_user) { create(:user) } - - let(:devices) do - [ - create(:device, user: current_user, last_used_at: 2.hours.ago), - create(:device, user: current_user, last_used_at: 3.hours.ago), - create(:device, user: current_user, last_used_at: 1.hour.ago), - ] - end - - before do - # Memoize records to run creates for specs that query the table - current_user - other_user - devices - end - - it 'returns an empty list if the device is not found' do - result = subject.call(other_user.id, 0, 1) - - expect(result).to eq([]) - end - - it 'returns a list containing a single most recent device' do - result = subject.call(current_user.id, 0, 1) - - expect(result.size).to eq(1) - expect(result[0].id).to eq(devices[2].id) - end - - it 'returns a list containing multiple most recent devices in order and using an offset' do - result = subject.call(current_user.id, 1, 2) - - expect(result.size).to eq(2) - expect(result[0].id).to eq(devices[0].id) - expect(result[1].id).to eq(devices[1].id) - end -end diff --git a/spec/services/device_tracking/lookup_device_for_user_spec.rb b/spec/services/device_tracking/lookup_device_for_user_spec.rb deleted file mode 100644 index 316dc110eb2..00000000000 --- a/spec/services/device_tracking/lookup_device_for_user_spec.rb +++ /dev/null @@ -1,40 +0,0 @@ -require 'rails_helper' - -describe DeviceTracking::LookupDeviceForUser do - subject { described_class } - let(:current_user) { create(:user) } - let(:other_user) { create(:user) } - - let(:device) { create(:device, user: current_user, cookie_uuid: device_uuid) } - - let(:device_uuid) { 'foo' } - let(:other_uuid) { 'bar' } - - before do - current_user - other_user - device - end - - it 'returns nil if the user is found but the cookie uuid is not' do - expect(Device.find_by(user_id: current_user.id)).to be_present - - result = subject.call(current_user.id, other_uuid) - - expect(result).to be_nil - end - - it 'returns nil if the user is not found and the cookie uuid is found' do - expect(Device.find_by(cookie_uuid: device_uuid)).to be_present - - result = subject.call(other_user.id, device_uuid) - - expect(result).to be_nil - end - - it 'returns the device' do - result = subject.call(current_user.id, device_uuid) - - expect(result).to be_present - end -end diff --git a/spec/services/device_tracking/update_device_spec.rb b/spec/services/device_tracking/update_device_spec.rb deleted file mode 100644 index dcd5eca3dc7..00000000000 --- a/spec/services/device_tracking/update_device_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'rails_helper' - -describe DeviceTracking::UpdateDevice do - subject { described_class } - let(:user) { create(:user) } - let(:remote_ip) { '1.2.3.4' } - let(:user_agent) { 'Chrome/58.0.3029.110 Safari/537.36' } - let(:uuid) { 'abc123' } - let(:old_timestamp) { Time.zone.now - 1.hour } - let(:device) { create(:device, last_used_at: old_timestamp) } - - it 'updates the device' do - expect(device.last_used_at).to eq(old_timestamp) - subject.call(device, remote_ip) - - expect(device.last_ip).to eq(remote_ip) - expect(device.last_used_at).to_not eq(old_timestamp) - end -end diff --git a/spec/services/device_tracking/forget_all_browsers_spec.rb b/spec/services/forget_all_browsers_spec.rb similarity index 77% rename from spec/services/device_tracking/forget_all_browsers_spec.rb rename to spec/services/forget_all_browsers_spec.rb index 0aee972e488..b37827e19ce 100644 --- a/spec/services/device_tracking/forget_all_browsers_spec.rb +++ b/spec/services/forget_all_browsers_spec.rb @@ -1,12 +1,12 @@ require 'rails_helper' -RSpec.describe DeviceTracking::ForgetAllBrowsers do +RSpec.describe ForgetAllBrowsers do let(:user) { build(:user, remember_device_revoked_at: original_revoked_at) } let(:now) { Time.zone.now } let(:original_revoked_at) { 30.days.from_now } subject(:service) do - DeviceTracking::ForgetAllBrowsers.new(user, remember_device_revoked_at: now) + ForgetAllBrowsers.new(user, remember_device_revoked_at: now) end describe '#call' do diff --git a/spec/services/idv/gpo_mail_spec.rb b/spec/services/idv/gpo_mail_spec.rb index 5a558e77c59..557633a60ae 100644 --- a/spec/services/idv/gpo_mail_spec.rb +++ b/spec/services/idv/gpo_mail_spec.rb @@ -52,7 +52,7 @@ def event_create(hash) event = hash[:event_type] now = Time.zone.now updated_at = hash[:updated_at] || now - device = DeviceTracking::LookupDeviceForUser.call(user.id, uuid) + device = Device.find_by(user_id: user.id, cookie_uuid: uuid) if device device.last_used_at = now device.last_ip = remote_ip diff --git a/spec/services/user_event_creator_spec.rb b/spec/services/user_event_creator_spec.rb index 6a84ae94491..5e37bff8a3e 100644 --- a/spec/services/user_event_creator_spec.rb +++ b/spec/services/user_event_creator_spec.rb @@ -4,16 +4,19 @@ let(:user_agent) { 'A computer on the internet' } 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 + end let(:request) do - request = double - - allow(request).to receive(:remote_ip).and_return(ip_address) - allow(request).to receive(:user_agent).and_return(user_agent) - - cookie_jar = { device: existing_device_cookie }.with_indifferent_access - allow(cookie_jar).to receive(:permanent).and_return({}) - allow(request).to receive(:cookie_jar).and_return(cookie_jar) - request + double( + remote_ip: ip_address, + user_agent: user_agent, + cookie_jar: cookie_jar, + ) end let(:user) { create(:user) } let(:device) { create(:device, user: user, cookie_uuid: existing_device_cookie) } @@ -81,6 +84,16 @@ expect(event.device.last_used_at).to be_within(1).of(Time.zone.now) end + context 'when there is no device cookie' do + let(:existing_device_cookie) { nil } + + it 'assigns one to the device' do + event = subject.create_user_event(event_type, user) + + expect(event.device.cookie_uuid.length).to eq(UserEventCreator::COOKIE_LENGTH) + end + end + it 'alerts the user if they have other devices' do allow(UserAlerts::AlertUserAboutNewDevice).to receive(:call) create(:device, user: user)