Fix #4555: Invalidate sessions for deleted users

Fix three exploits that allowed one to keep using their account after it was deleted:

* It was possible to use session cookies from another computer to login after you deleted your account.
* It was possible to use API keys to make API requests after you deleted your account.
* It was possible to request a password reset, delete your account, then use the password reset link
  to change your password and login to your deleted account.

app/logical/session_loader.rb

@@ -143,7 +143,13 @@ class SessionLoader
   # Set the current user based on the `user_id` session cookie.
   def load_session_user
     user = User.find_by_id(session[:user_id])
-    CurrentUser.user = user if user
+    return if user.nil?
+
+    if user.is_deleted?
+      logout(user)
+    else
+      CurrentUser.user = user
+    end
   end
 
   def update_last_logged_in_at

app/models/user.rb

@@ -150,10 +150,6 @@ class User < ApplicationRecord
 
   accepts_nested_attributes_for :email_address, reject_if: :all_blank, allow_destroy: true
 
-  # UserDeletion#rename renames deleted users to `user_<1234>~`. Tildes
-  # are appended if the username is taken.
-  scope :deleted, -> { where("name ~ 'user_[0-9]+~*'") }
-  scope :undeleted, -> { where("name !~ 'user_[0-9]+~*'") }
   scope :admins, -> { where(level: Levels::ADMIN) }
   scope :banned, -> { bit_prefs_match(:is_banned, true) }
 
@@ -161,6 +157,8 @@ class User < ApplicationRecord
   scope :has_private_favorites, -> { bit_prefs_match(:enable_private_favorites, true) }
   scope :has_public_favorites,  -> { bit_prefs_match(:enable_private_favorites, false) }
 
+  deletable
+
   module BanMethods
     def unban!
       self.is_banned = false
@@ -224,16 +222,22 @@ class User < ApplicationRecord
       self.bcrypt_password_hash = BCrypt::Password.create(hash_password(new_password))
     end
 
+    # @return [User, Boolean] Return the user if the signed user ID is correct, or false if it isn't.
     def authenticate_login_key(signed_user_id)
+      return false if is_deleted?
       signed_user_id.present? && id == Danbooru::MessageVerifier.new(:login).verify(signed_user_id) && self
     end
 
+    # @return [Array<(User, ApiKey)>, Boolean] Return a (User, ApiKey) pair if the API key is correct, or false if it isn't.
     def authenticate_api_key(key)
+      return false if is_deleted?
       api_key = api_keys.find_by(key: key)
       api_key.present? && ActiveSupport::SecurityUtils.secure_compare(api_key.key, key) && [self, api_key]
     end
 
+    # @return [User, Boolean] Return the user if the password is correct, or false if it isn't.
     def authenticate_password(password)
+      return false if is_deleted?
       BCrypt::Password.new(bcrypt_password_hash) == hash_password(password) && self
     end
 
@@ -300,10 +304,6 @@ class User < ApplicationRecord
       User.level_string(value || level)
     end
 
-    def is_deleted?
-      name.match?(/\Auser_[0-9]+~*\z/)
-    end
-
     def is_anonymous?
       level == Levels::ANONYMOUS
     end

test/functional/application_controller_test.rb

@@ -137,6 +137,14 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
           assert_response 401
         end
 
+        should "fail for a deleted user" do
+          @user.update!(is_deleted: true)
+          basic_auth_string = "Basic #{::Base64.encode64("#{@user.name}:#{@api_key.key}")}"
+          get profile_path, as: :json, headers: { HTTP_AUTHORIZATION: basic_auth_string }
+
+          assert_response 401
+        end
+
         should "succeed for non-GET requests without a CSRF token" do
           assert_changes -> { @user.reload.enable_safe_mode }, from: false, to: true do
             basic_auth_string = "Basic #{::Base64.encode64("#{@user.name}:#{@api_key.key}")}"
@@ -183,6 +191,13 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
           assert_response 401
         end
 
+        should "fail for a deleted user" do
+          @user.update!(is_deleted: true)
+          get edit_user_path(@user), params: { login: @user.name, api_key: @api_key.key }
+
+          assert_response 401
+        end
+
         should "succeed for non-GET requests without a CSRF token" do
           assert_changes -> { @user.reload.enable_safe_mode }, from: false, to: true do
             put user_path(@user, login: @user.name, api_key: @api_key.key), params: { user: { enable_safe_mode: "true" }}, as: :json
@@ -267,14 +282,26 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
     end
 
     context "on session cookie authentication" do
-      should "succeed" do
-        user = create(:user, password: "password")
+      setup do
+        @user = create(:user, password: "password")
+        post session_path, params: { name: @user.name, password: "password" }
+      end
 
-        post session_path, params: { name: user.name, password: "password" }
-        get edit_user_path(user)
+      should "succeed" do
+        get profile_path
 
         assert_response :success
       end
+
+      should "fail for a deleted user" do
+        @user.update!(is_deleted: true)
+
+        get profile_path
+
+        assert_redirected_to login_path(url: "/profile")
+        assert_nil(session[:user_id])
+        assert_equal(true, @user.user_events.exists?(category: :logout))
+      end
     end
 
     context "accessing an unauthorized page" do

test/functional/passwords_controller_test.rb

@@ -33,6 +33,18 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest
         assert_equal(true, @user.user_events.password_change.exists?)
       end
 
+      should "not update the password when a deleted user tries to reset their password with a valid login key" do
+        @user.update!(is_deleted: true)
+        old_password = @user.bcrypt_password_hash
+
+        signed_user_id = Danbooru::MessageVerifier.new(:login).generate(@user.id)
+        put_auth user_password_path(@user), @user, params: { user: { password: "abcde", password_confirmation: "abcde", signed_user_id: signed_user_id } }
+
+        assert_response 403
+        assert_equal(old_password, @user.reload.bcrypt_password_hash)
+        assert_equal(false, @user.user_events.password_change.exists?)
+      end
+
       should "allow the site owner to change the password of other users" do
         @owner = create(:owner_user)
         put_auth user_password_path(@user), @owner, params: { user: { password: "abcde", password_confirmation: "abcde" } }

test/functional/sessions_controller_test.rb

@@ -48,6 +48,15 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
         assert_response 403
       end
 
+      should "not allow deleted users to login" do
+        @user.update!(is_deleted: true)
+        post session_path, params: { name: @user.name, password: "password" }
+
+        assert_response 401
+        assert_nil(nil, session[:user_id])
+        assert_equal(true, @user.user_events.failed_login.exists?)
+      end
+
       should "not allow IP banned users to login" do
         @ip_ban = create(:ip_ban, category: :full, ip_addr: "1.2.3.4")
         post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" }