diff --git a/app/logical/session_loader.rb b/app/logical/session_loader.rb index 703ba595e..9999950c3 100644 --- a/app/logical/session_loader.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 57ce3c3ab..e11e2b033 100644 --- a/app/models/user.rb +++ b/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 diff --git a/test/functional/application_controller_test.rb b/test/functional/application_controller_test.rb index ee302d2dc..92b9b03eb 100644 --- a/test/functional/application_controller_test.rb +++ b/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 diff --git a/test/functional/passwords_controller_test.rb b/test/functional/passwords_controller_test.rb index e67822180..3c1d04630 100644 --- a/test/functional/passwords_controller_test.rb +++ b/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" } } diff --git a/test/functional/sessions_controller_test.rb b/test/functional/sessions_controller_test.rb index 3cac418cd..b63dfe453 100644 --- a/test/functional/sessions_controller_test.rb +++ b/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" }