From 129646e76077d565347f945e90e4dfccb00439d6 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Fri, 11 Jun 2021 12:17:25 +0000 Subject: [PATCH] Fixes #3605 - session timeout does regulary logout active users before timeout because of dead old sessions. --- app/jobs/session_timeout_job.rb | 58 ++++++++----------------- app/jobs/session_timeout_job/session.rb | 53 ++++++++++++++++++++++ spec/jobs/session_timeout_job_spec.rb | 33 ++++++++++++++ spec/system/dashboard_spec.rb | 6 +-- 4 files changed, 107 insertions(+), 43 deletions(-) create mode 100644 app/jobs/session_timeout_job/session.rb diff --git a/app/jobs/session_timeout_job.rb b/app/jobs/session_timeout_job.rb index da8f8f267..403c8453d 100644 --- a/app/jobs/session_timeout_job.rb +++ b/app/jobs/session_timeout_job.rb @@ -2,58 +2,36 @@ class SessionTimeoutJob < ApplicationJob def perform - sessions.find_each do |session| + sessions.each do |session| perform_session(session) end end def perform_session(session) - return if !session.data['user_id'] # user is optional because it can be deleted already - user = User.find_by(id: session.data['user_id']) - if user - timeout = get_timeout(user) - return if timeout < 1 - return if session.data['ping'] > timeout.seconds.ago + if session.user? + return if session.active? + + # if the user has no active sessions then we + # make sure to definitely log him out if there + # is any frontends opened + if !active_session(session.user) + session.frontend_timeout + end end - self.class.destroy_session(user, session) - end - - def self.destroy_session(user, session) - - # user is optional because it can be deleted already - if user - PushMessages.send_to(user.id, { event: 'session_timeout' }) - end session.destroy end + def active_session(user) + @active_session ||= {} + return @active_session[user.id] if @active_session[user.id].present? + + @active_session[user.id] = sessions.detect { |session| session.active? && session.user.id == user.id } + end + def sessions - ActiveRecord::SessionStore::Session.where('updated_at < ?', config.values.map(&:to_i).min.seconds.ago) - end - - def config - Setting.get('session_timeout') - end - - def get_timeout(user) - permissions = Permission.where(id: user.permissions_with_child_ids).pluck(:name) - - timeout = -1 - config.each do |key, value| - next if key == 'default' - next if permissions.exclude?(key) - next if value.to_i < timeout - - timeout = value.to_i - end - - if timeout < 1 - timeout = config['default'].to_i - end - - timeout + @sessions ||= ActiveRecord::SessionStore::Session.order(updated_at: :desc).limit(10_000).map { |session| SessionTimeoutJob::Session.new(session) } end end diff --git a/app/jobs/session_timeout_job/session.rb b/app/jobs/session_timeout_job/session.rb new file mode 100644 index 000000000..79f53048d --- /dev/null +++ b/app/jobs/session_timeout_job/session.rb @@ -0,0 +1,53 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +class SessionTimeoutJob::Session + attr_accessor :session, :user + + delegate :destroy, to: :session + + def initialize(session) + @session = session + @user = User.find_by(id: session.data['user_id']) + end + + def user? + user.present? + end + + def active? + return true if !user? + return true if timeout < 1 + return true if session.data['ping'] > timeout.seconds.ago + end + + def frontend_timeout + return if !user? + + PushMessages.send_to(user.id, { event: 'session_timeout' }) + end + + def timeout + @timeout ||= begin + permissions = Permission.where(id: user.permissions_with_child_ids).pluck(:name) + + result = -1 + config.each do |key, value| + next if key == 'default' + next if permissions.exclude?(key) + next if value.to_i < result + + result = value.to_i + end + + if result < 1 + result = config['default'].to_i + end + + result + end + end + + def config + Setting.get('session_timeout') + end +end diff --git a/spec/jobs/session_timeout_job_spec.rb b/spec/jobs/session_timeout_job_spec.rb index ae42f8367..ed943f29b 100644 --- a/spec/jobs/session_timeout_job_spec.rb +++ b/spec/jobs/session_timeout_job_spec.rb @@ -150,4 +150,37 @@ RSpec.describe SessionTimeoutJob, type: :job do expect { described_class.perform_now }.to change(ActiveRecord::SessionStore::Session, :count).by(0) end end + + context 'with timeout and a dead session in the past' do + let(:user) { create(:admin) } + + before do + Setting.set('session_timeout', { admin: 30.minutes.to_s }) + travel_to 10.hours.ago + create(:active_session, user: user) + travel_to 10.hours.from_now + end + + it 'does a frontend logout for the user' do + allow(PushMessages).to receive(:send_to) + travel_to 1.hour.from_now + described_class.perform_now + expect(PushMessages).to have_received(:send_to).with(user.id, { event: 'session_timeout' }).twice + end + + it 'does not init a frontend logout for the user because he does not exist anymore' do + allow(PushMessages).to receive(:send_to) + user.destroy + travel_to 1.hour.from_now + described_class.perform_now + expect(PushMessages).not_to have_received(:send_to).with(user.id, { event: 'session_timeout' }) + end + + it 'does not init a frontend logout for the user because of an active session' do + allow(PushMessages).to receive(:send_to) + travel_to 1.minute.from_now + described_class.perform_now + expect(PushMessages).not_to have_received(:send_to).with(user.id, { event: 'session_timeout' }) + end + end end diff --git a/spec/system/dashboard_spec.rb b/spec/system/dashboard_spec.rb index 0e5a50c45..5eaab2f39 100644 --- a/spec/system/dashboard_spec.rb +++ b/spec/system/dashboard_spec.rb @@ -102,16 +102,16 @@ RSpec.describe 'Dashboard', type: :system, authenticated_as: true do end end - context 'Logout by SessionTimeoutJob - destroy_session' do + context 'Logout by SessionTimeoutJob - frontend_timeout' do it 'does logout user', authenticated_as: :admin do # because of the websocket server running in the same # process and the checks in the frontend it is really # hard test the SessionTimeoutJob.perform_now here # so we only check the session killing code and use - # backend tests for the rest + # backend tests for the reset session = ActiveRecord::SessionStore::Session.all.detect { |s| s.data['user_id'] == admin.id } - SessionTimeoutJob.destroy_session(admin, session) + SessionTimeoutJob::Session.new(session).frontend_timeout expect(page).to have_text('Due to inactivity you are automatically logged out.', wait: 20) end end