User SqlBypass sessions to avoid ActiveRecord for session storage (#9142)

* Use SqlBypass sessions to avoid ActiveRecord for session storage

* Set default host if Capybara.app_host does not exist

app/models/sessions/active_record.rb

@@ -0,0 +1,55 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+##
+# An AR helper class to access sessions, but not create them.
+# You can still use AR methods to delete records however.
+module Sessions
+  class ActiveRecord < ::ApplicationRecord
+    self.table_name = 'sessions'
+
+    scope :for_user, ->(user) do
+      user_id = user.is_a?(User) ? user.id : user.to_i
+
+      where(user_id: user_id)
+    end
+
+    scope :non_user, -> do
+      where(user_id: nil)
+    end
+
+    ##
+    # Mark all records as readonly so they cannot
+    # modify the database
+    def readonly?
+      true
+    end
+  end
+end

app/models/sessions/sql_bypass.rb

@@ -0,0 +1,118 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+##
+# An extension to the SqlBypass class to store
+# sessions in database without going through ActiveRecord
+module Sessions
+  class SqlBypass < ::ActiveRecord::SessionStore::SqlBypass
+    class << self
+      ##
+      # Looks up session data for a given session ID.
+      #
+      # This is not specific to AR sessions which are stored as AR records.
+      # But this is the probably the first place one would search for session-related
+      # methods. I.e. this works just as well for cache- and file-based sessions.
+      #
+      # @param session_id [String] The session ID as found in the `_open_project_session` cookie
+      # @return [Hash] The saved session data (user_id, updated_at, etc.) or nil if no session was found.
+      def lookup_data(session_id)
+        if Rails.application.config.session_store == ActionDispatch::Session::ActiveRecordStore
+          find_by_session_id(session_id)&.data
+        else
+          session_store = Rails.application.config.session_store.new nil, {}
+          _id, data = session_store.instance_eval do
+            find_session({}, Rack::Session::SessionId.new(session_id))
+          end
+
+          data.presence
+        end
+      end
+    end
+
+    ##
+    # Save while updating the user_id reference and updated_at column
+    def save
+      return false unless loaded?
+
+      if @new_record
+        insert!
+      else
+        update!
+      end
+    end
+
+    ##
+    # Also destroy any other session when this one is actively destroyed
+    def destroy
+      delete_user_sessions
+      super
+    end
+
+    private
+
+    def user_id
+      id = data.with_indifferent_access['user_id'].to_i
+      id > 0 ? id : nil
+    end
+
+    def insert!
+      @new_record = false
+      connection.update <<~SQL, 'Create session'
+         INSERT INTO sessions (session_id, data, user_id, updated_at)
+         VALUES (
+           #{connection.quote(session_id)},
+           #{connection.quote(self.class.serialize(data))},
+           #{connection.quote(user_id)},
+           (now() at time zone 'utc')
+        )
+      SQL
+    end
+
+    def update!
+      connection.update <<~SQL, 'Update session'
+        UPDATE sessions
+        SET
+          data=#{connection.quote(self.class.serialize(data))},
+          session_id=#{connection.quote(session_id)},
+          user_id=#{connection.quote(user_id)},
+          updated_at=(now() at time zone 'utc')
+        WHERE session_id=#{connection.quote(@retrieved_by)}
+      SQL
+    end
+
+    def delete_user_sessions
+      uid = user_id
+      return unless uid && OpenProject::Configuration.drop_old_sessions_on_logout?
+
+      ::Sessions::ActiveRecord.for_user(uid).delete_all
+    end
+  end
+end

app/models/user_session.rb

@@ -1,71 +0,0 @@
-#-- encoding: UTF-8
-
-#-- copyright
-# OpenProject is an open source project management software.
-# Copyright (C) 2012-2021 the OpenProject GmbH
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License version 3.
-#
-# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
-# Copyright (C) 2006-2013 Jean-Philippe Lang
-# Copyright (C) 2010-2013 the ChiliProject Team
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License
-# as published by the Free Software Foundation; either version 2
-# of the License, or (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-#
-# See docs/COPYRIGHT.rdoc for more details.
-#++
-require "active_support/core_ext/module/attribute_accessors"
-
-class UserSession < ActiveRecord::SessionStore::Session
-  belongs_to :user
-
-  ##
-  # Keep an index on the current user for the given session hash
-  before_save :set_user_id
-
-  ##
-  # Delete related sessions when an active session is destroyed
-  after_destroy :delete_user_sessions
-
-  ##
-  # Looks up session data for a given session ID.
-  #
-  # This is not specific to AR sessions which are stored as `UserSession` records.
-  # But this is the probably the first place one would search for session-related
-  # methods. I.e. this works just as well for cache- and file-based sessions.
-  #
-  # @param session_id [String] The session ID as found in the `_open_project_session` cookie
-  # @return [Hash] The saved session data (user_id, updated_at, etc.) or nil if no session was found.
-  def self.lookup_data(session_id)
-    session_store = Rails.application.config.session_store.new nil, {}
-    _id, data = session_store.find_session({}, Rack::Session::SessionId.new(session_id))
-
-    data if data.present?
-  end
-
-  private
-
-  def set_user_id
-    write_attribute(:user_id, data['user_id'])
-  end
-
-  def delete_user_sessions
-    user_id = data['user_id']
-    return unless user_id && OpenProject::Configuration.drop_old_sessions_on_logout?
-
-    ::UserSession.where(user_id: user_id).delete_all
-  end
-end

app/services/sessions/drop_other_sessions_service.rb

@@ -38,9 +38,9 @@ module Sessions
       def call(user, session)
         return false unless active_record_sessions?
 
-        ::UserSession
-          .where(user_id: user.id)
-          .where.not(session_id: session.id.to_s)
+        ::Sessions::ActiveRecord
+          .for_user(user)
+          .where.not(session_id: session.id.private_id)
           .delete_all
 
         true

app/services/sessions/initialize_session_service.rb

@@ -41,7 +41,8 @@ module Sessions
         session[:updated_at] = Time.now
 
         if drop_old_sessions?
-          ::UserSession.where(user_id: user.id).delete_all
+          Rails.logger.info { "Deleting all other sessions for #{user}." }
+          ::Sessions::ActiveRecord.for_user(user).delete_all
         end
 
         ServiceResult.new(success: true, result: session)

config/initializers/session_store.rb

@@ -117,4 +117,7 @@ OpenProject::Application.config.session_store session_store, **session_options
 ##
 # We use our own decorated session model to note the user_id
 # for each session.
-ActionDispatch::Session::ActiveRecordStore.session_class = ::UserSession
+ActionDispatch::Session::ActiveRecordStore.session_class = ::Sessions::SqlBypass
+# Continue to use marshal serialization to retain symbols and whatnot
+ActiveRecord::SessionStore::Session.serializer = :marshal
+

db/migrate/20210331085058_migrate_sessions_unlogged.rb

@@ -0,0 +1,14 @@
+class MigrateSessionsUnlogged < ActiveRecord::Migration[6.1]
+  def change
+    truncate :sessions
+
+    # Set the table to unlogged
+    execute <<~SQL
+      ALTER TABLE "sessions" SET UNLOGGED
+    SQL
+
+    # We don't need the created at column
+    # that now no longer is set by rails
+    remove_column :sessions, :created_at, null: false
+  end
+end

spec/factories/user_session_factory.rb

@@ -27,13 +27,18 @@
 #++
 
 FactoryBot.define do
-  factory :user_session do
+  factory :user_session, class: ::Sessions::SqlBypass do
+    to_create { |instance| instance.save }
     sequence(:session_id) { |n| "session_#{n}" }
-    association :user
 
-    callback(:after_build) do |session|
-      session.data = {}
-      session.data['user_id'] = session.user.id if session.user.present?
+    transient do
+      user { nil }
+      data { {} }
+    end
+
+    callback(:after_build) do |session, evaluator|
+      session.data = evaluator.data
+      session.data['user_id'] = evaluator.user&.id
     end
   end
 end

spec/features/auth/omniauth_spec.rb

@@ -33,7 +33,14 @@ describe 'Omniauth authentication', type: :feature do
   # the Capybara app_host, however this change was not being reflected in the Rails host,
   # causing the redirect checks to fail below.
   def self.default_url_options
-    { host: Capybara.app_host.sub(/https?\/\//, "") }
+    host =
+      if Capybara.app_host
+        Capybara.app_host.sub(/https?\/\//, "")
+      else
+        'http://www.example.com'
+      end
+
+    { host: host }
   end
 
   let(:user) do

spec/features/security/expire_sessions_spec.rb

@@ -44,16 +44,16 @@ describe 'Expire old user sessions',
   describe 'logging in again' do
     context 'with drop_old_sessions enabled', with_config: { drop_old_sessions_on_login: true } do
       it 'destroys the old session' do
-        expect(UserSession.count).to eq(1)
+        expect(::Sessions::ActiveRecord.count).to eq(1)
 
-        first_session = UserSession.first
+        first_session = ::Sessions::ActiveRecord.first
         expect(first_session.user_id).to eq(admin.id)
 
         # Actually login now
         login_with(admin.login, admin_password)
 
-        expect(UserSession.count).to eq(1)
-        second_session = UserSession.first
+        expect(::Sessions::ActiveRecord.count).to eq(1)
+        second_session = ::Sessions::ActiveRecord.first
 
         expect(second_session.user_id).to eq(admin.id)
         expect(second_session.session_id).not_to eq(first_session.session_id)
@@ -65,7 +65,7 @@ describe 'Expire old user sessions',
         # Actually login now
         login_with(admin.login, admin_password)
 
-        expect(UserSession.where(user_id: admin.id).count).to eq(2)
+        expect(::Sessions::ActiveRecord.for_user(admin).count).to eq(2)
       end
     end
   end
@@ -74,23 +74,23 @@ describe 'Expire old user sessions',
     before do
       # Actually login now
       login_with(admin.login, admin_password)
-      expect(UserSession.where(user_id: admin.id).count).to eq(2)
+      expect(::Sessions::ActiveRecord.for_user(admin).count).to eq(2)
       visit '/logout'
     end
 
     context 'with drop_old_sessions enabled', with_config: { drop_old_sessions_on_logout: true } do
       it 'destroys the old session' do
         # A fresh session is opened due to reset_session
-        expect(UserSession.where(user_id: admin.id).count).to eq(0)
-        expect(UserSession.where(user_id: nil).count).to eq(1)
+        expect(::Sessions::ActiveRecord.for_user(admin).count).to eq(0)
+        expect(::Sessions::ActiveRecord.non_user.count).to eq(1)
       end
     end
 
     context 'with drop_old_sessions disabled',
             with_config: { drop_old_sessions_on_logout: false } do
       it 'keeps the old session' do
-        expect(UserSession.count).to eq(2)
-        expect(UserSession.where(user_id: admin.id).count).to eq(1)
+        expect(::Sessions::ActiveRecord.count).to eq(2)
+        expect(::Sessions::ActiveRecord.for_user(admin).count).to eq(1)
       end
     end
   end

spec/features/users/my_spec.rb

@@ -48,7 +48,7 @@ describe 'my',
     expect(page).to have_content I18n.t(:notice_account_other_session_expired)
 
     # expect session to be removed
-    expect(::UserSession.where(user_id: user.id, session_id: 'other').count).to eq 0
+    expect(::Sessions::ActiveRecord.for_user(user).where(session_id: 'other').count).to eq 0
 
     user.reload
     expect(user.mail).to eq 'foo@mail.com'
@@ -59,8 +59,10 @@ describe 'my',
     login_as(user)
 
     # Create dangling session
-    ::UserSession.create!(data: { 'user_id' => user.id }, session_id: 'other')
-    expect(::UserSession.where(user_id: user.id, session_id: 'other').count).to eq 1
+    session = ::Sessions::SqlBypass.new data: { user_id: user.id }, session_id: 'other'
+    session.save
+
+    expect(::Sessions::ActiveRecord.for_user(user).where(session_id: 'other').count).to eq 1
   end
 
   context 'user' do

spec/features/users/password_change_spec.rb

@@ -88,14 +88,16 @@ describe 'random password generation',
       fill_in 'new_password_confirmation', with: new_password
 
       # Expect other sessions to be deleted
-      ::UserSession.create!(data: { 'user_id' => user.id }, session_id: 'other')
-      expect(::UserSession.where(user_id: user.id).count).to be >= 1
+      session = ::Sessions::SqlBypass.new data: { user_id: user.id }, session_id: 'other'
+      session.save
+
+      expect(::Sessions::ActiveRecord.for_user(user.id).count).to be >= 1
 
       click_on 'Save'
       expect(page).to have_selector('.flash.notice', text: I18n.t(:notice_account_password_updated))
 
       # The old session is removed
-      expect(::UserSession.where(user_id: user.id, session_id: 'other').count).to eq 0
+      expect(::Sessions::ActiveRecord.find_by(session_id: 'other')).to be_nil
 
       # Logout and sign in with outdated password
       visit signout_path

spec/models/sessions/active_record_sessions_spec.rb

@@ -0,0 +1,89 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe ::Sessions::ActiveRecord do
+  subject { described_class.new session_id: 'foo' }
+
+  describe '#save' do
+    it 'can not save' do
+      expect { subject.save }.to raise_error(ActiveRecord::ReadOnlyRecord)
+      expect { subject.save! }.to raise_error(ActiveRecord::ReadOnlyRecord)
+    end
+  end
+
+  describe '#update' do
+    let(:session) { FactoryBot.create :user_session }
+    subject { described_class.find_by(session_id: session.session_id) }
+
+    it 'can not update' do
+      expect { subject.save }.to raise_error(ActiveRecord::ReadOnlyRecord)
+      expect { subject.save! }.to raise_error(ActiveRecord::ReadOnlyRecord)
+
+      expect { subject.update(session_id: 'foo') }.to raise_error(ActiveRecord::ReadOnlyRecord)
+      expect { subject.update!(session_id: 'foo') }.to raise_error(ActiveRecord::ReadOnlyRecord)
+    end
+  end
+
+  describe '#destroy' do
+    let(:sessions) { FactoryBot.create :user_session }
+
+    it 'can not destroy' do
+      expect { subject.destroy }.to raise_error(ActiveRecord::ReadOnlyRecord)
+      expect { subject.destroy! }.to raise_error(ActiveRecord::ReadOnlyRecord)
+    end
+  end
+
+  describe '.for_user' do
+    let(:user) { FactoryBot.create :user }
+    let!(:sessions) { FactoryBot.create_list :user_session, 2, user: user }
+
+    subject { described_class.for_user(user) }
+
+    it 'can find and delete, but not destroy those sessions' do
+      expect(subject.pluck(:session_id)).to match_array(sessions.map(&:session_id))
+
+      expect { subject.destroy_all }.to raise_error(ActiveRecord::ReadOnlyRecord)
+
+      expect { subject.delete_all }.not_to raise_error
+
+      expect(described_class.for_user(user).count).to eq 0
+    end
+  end
+
+  describe '.non_user' do
+    let!(:session) { FactoryBot.create :user_session, user: nil }
+
+    subject { described_class.non_user }
+
+    it 'can find those sessions' do
+      expect(subject.pluck(:session_id)).to contain_exactly(session.session_id)
+    end
+  end
+end

spec/models/sessions/sql_bypass_sessions_spec.rb

@@ -28,13 +28,13 @@
 
 require 'spec_helper'
 
-describe UserSession do
+describe ::Sessions::SqlBypass do
   subject { FactoryBot.build(:user_session, user: user) }
 
   shared_examples 'augments the user_id attribute' do
     it do
       subject.save
-      expect(subject.user_id).to eq(user_id)
+      expect(subject.data['user_id']).to eq(user_id)
     end
   end
 
@@ -57,21 +57,21 @@ describe UserSession do
     context 'when config is enabled',
             with_config: { drop_old_sessions_on_logout: true } do
       it 'destroys both sessions' do
-        expect(UserSession.where(user_id: user.id).count).to eq(2)
+        expect(::Sessions::ActiveRecord.for_user(user).count).to eq(2)
         sessions.first.destroy
 
-        expect(UserSession.count).to eq(0)
+        expect(::Sessions::ActiveRecord.count).to eq(0)
       end
     end
 
     context 'when config is disabled',
             with_config: { drop_old_sessions_on_logout: false } do
       it 'destroys only the one session' do
-        expect(UserSession.where(user_id: user.id).count).to eq(2)
+        expect(::Sessions::ActiveRecord.for_user(user).count).to eq(2)
         sessions.first.destroy
 
-        expect(UserSession.count).to eq(1)
-        expect(UserSession.first.session_id).to eq(sessions[1].session_id)
+        expect(::Sessions::ActiveRecord.count).to eq(1)
+        expect(::Sessions::ActiveRecord.first.session_id).to eq(sessions[1].session_id)
       end
     end
   end