diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 13e0a93c5..191bc1fa0 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -16,12 +16,6 @@ def new sig { void } def create - begin - clear_expired_sessions - rescue StandardError - # Cleanup is best effort; it must not block logging in. - end - user, status = User.authenticate_with_status(params[:session][:login], params[:session][:password]) case status @@ -68,9 +62,4 @@ def try_to_redirect_incorrect_login(redirect_params = {}) redirect_to login_path, redirect_params end end - - sig { void } - def clear_expired_sessions - Session.delete_expired - end end diff --git a/app/models/session.rb b/app/models/session.rb index f7f359385..31a1907d0 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -3,10 +3,6 @@ class Session < ActiveRecord::SessionStore::Session extend T::Sig - sig { returns(Integer) } - def self.delete_expired - where(updated_at: ...1.month.ago).delete_all - end sig { params(user: User).returns(T::Boolean) } def belongs_to?(user) diff --git a/app/models/user.rb b/app/models/user.rb index e72c5da07..e8430f1d5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,8 +39,9 @@ class User < ApplicationRecord has_many :organization_memberships, dependent: :destroy has_many :memberships, through: :organization_memberships, source: :organization + # Case-sensitive so it uses index_users_on_login; a case-insensitive check scans every user on each signup. validates :login, presence: true, - uniqueness: { case_sensitive: false }, + uniqueness: true, length: { within: 2..50 } validates :email, presence: true, diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 6f5196b98..4577d340d 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -77,6 +77,17 @@ def post_create end end + describe 'with stale sessions in the store' do + it 'does not delete them while logging in' do + stale = Session.create!(session_id: 'stale', data: {}) + stale.update_column(:updated_at, 2.months.ago) + + post_create + + expect(Session.exists?(stale.id)).to be(true) + end + end + describe 'when authentication fails' do before :each do allow(User).to receive(:authenticate_with_status).and_return([nil, :rejected]) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4f7c9b3a6..3f4f70e9e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -112,6 +112,16 @@ expect(user.errors[:login].size).to eq(1) end + it 'checks login uniqueness without scanning LOWER(login)' do + user = User.new(@params) + queries = [] + callback = ->(*, payload) { queries << payload[:sql] } + ActiveSupport::Notifications.subscribed(callback, 'sql.active_record') { user.valid? } + + expect(queries.grep(/"users"\."login"/)).not_to be_empty + expect(queries.grep(/LOWER\("users"\."login"\)/i)).to be_empty + end + it 'should fail without email' do @params.delete(:email)