diff --git a/app/controllers/emails_controller.rb b/app/controllers/emails_controller.rb index 949b83616..5418289a8 100644 --- a/app/controllers/emails_controller.rb +++ b/app/controllers/emails_controller.rb @@ -14,7 +14,7 @@ class EmailsController < ApplicationController def update @user = authorize User.find(params[:user_id]), policy_class: EmailAddressPolicy - if User.authenticate(@user.name, params[:user][:password]) + if @user.authenticate_password(params[:user][:password]) @user.update(email_address_attributes: { address: params[:user][:email] }) else @user.errors[:base] << "Password was incorrect" diff --git a/app/controllers/maintenance/user/api_keys_controller.rb b/app/controllers/maintenance/user/api_keys_controller.rb index 2b2b64064..bb90d1c42 100644 --- a/app/controllers/maintenance/user/api_keys_controller.rb +++ b/app/controllers/maintenance/user/api_keys_controller.rb @@ -27,7 +27,7 @@ module Maintenance end def authenticate! - if ::User.authenticate(CurrentUser.user.name, params[:user][:password]) == CurrentUser.user + if CurrentUser.user.authenticate_password(params[:user][:password]) @api_key = CurrentUser.user.api_key || ApiKey.generate!(CurrentUser.user) @password = params[:user][:password] else diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index d1b16b1f0..1a63c5b8a 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -9,9 +9,7 @@ class PasswordsController < ApplicationController def update @user = authorize User.find(params[:user_id]), policy_class: PasswordPolicy - if User.authenticate(@user.name, params[:user][:old_password]) - @user.update(password: params[:user][:password], password_confirmation: params[:user][:password_confirmation]) - elsif @user.authenticate_login_key(params[:user][:signed_user_id]) + if @user.authenticate_password(params[:user][:old_password]) || @user.authenticate_login_key(params[:user][:signed_user_id]) @user.update(password: params[:user][:password], password_confirmation: params[:user][:password_confirmation]) else @user.errors[:base] << "Incorrect password" diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index d9ba48578..610e00150 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -7,13 +7,12 @@ class SessionsController < ApplicationController end def create - session_params = params[:session].presence || params - session_creator = SessionCreator.new(session, session_params[:name], session_params[:password], request.remote_ip) + name, password, url = params.fetch(:session, params).slice(:name, :password, :url).values + user = SessionLoader.new(request).login(name, password) - if session_creator.authenticate - url = session_params[:url] - url = posts_path if !url&.start_with?("/") - respond_with(session_creator.user, location: url, methods: [:api_token]) + if user + url = posts_path unless url&.start_with?("/") + respond_with(user, location: url, methods: [:api_token]) else flash.now[:notice] = "Password was incorrect" raise SessionLoader::AuthenticationFailure diff --git a/app/logical/session_creator.rb b/app/logical/session_creator.rb deleted file mode 100644 index 16bb57e8d..000000000 --- a/app/logical/session_creator.rb +++ /dev/null @@ -1,23 +0,0 @@ -class SessionCreator - attr_reader :session, :name, :password, :ip_addr - attr_reader :user - - def initialize(session, name, password, ip_addr) - @session = session - @name = name - @password = password - @ip_addr = ip_addr - end - - def authenticate - if User.authenticate(name, password) - @user = User.find_by_name(name) - - session[:user_id] = @user.id - @user.update_column(:last_ip_addr, ip_addr) - return true - else - return false - end - end -end diff --git a/app/logical/session_loader.rb b/app/logical/session_loader.rb index 4499c0564..ba6263dbc 100644 --- a/app/logical/session_loader.rb +++ b/app/logical/session_loader.rb @@ -9,6 +9,15 @@ class SessionLoader @params = request.parameters end + def login(name, password) + user = User.find_by_name(name)&.authenticate_password(password) + return nil unless user + + session[:user_id] = user.id + user.update_column(:last_ip_addr, request.remote_ip) + user + end + def load CurrentUser.user = User.anonymous CurrentUser.ip_addr = request.remote_ip @@ -50,8 +59,6 @@ class SessionLoader authenticate_api_key(params[:login], params[:api_key]) elsif params[:login].present? && params[:password_hash].present? authenticate_legacy_api_key(params[:login], params[:password_hash]) - else - raise AuthenticationFailure end end @@ -63,11 +70,8 @@ class SessionLoader end def authenticate_api_key(name, api_key) - CurrentUser.user = User.authenticate_api_key(name, api_key) - - if CurrentUser.user.nil? - raise AuthenticationFailure.new - end + CurrentUser.user = User.find_by_name(name)&.authenticate_api_key(api_key) + raise AuthenticationFailure unless Currentuser.user.present? end def authenticate_legacy_api_key(name, password_hash) diff --git a/app/logical/user_deletion.rb b/app/logical/user_deletion.rb index 4fa4b06a5..1d3b96cba 100644 --- a/app/logical/user_deletion.rb +++ b/app/logical/user_deletion.rb @@ -62,7 +62,7 @@ class UserDeletion end def validate - if !User.authenticate(user.name, password) + if !user.authenticate_password(password) raise ValidationError.new("Password is incorrect") end diff --git a/app/models/user.rb b/app/models/user.rb index 770f3795e..d3036424b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -160,38 +160,29 @@ class User < ApplicationRecord end end - module PasswordMethods - def bcrypt_password - BCrypt::Password.new(bcrypt_password_hash) - end - + concerning :AuthenticationMethods do def password=(new_password) @password = new_password - self.bcrypt_password_hash = User.bcrypt(new_password) + self.bcrypt_password_hash = BCrypt::Password.create(hash_password(new_password)) end - end - - module AuthenticationMethods - extend ActiveSupport::Concern def authenticate_login_key(signed_user_id) - signed_user_id.present? && id == Danbooru::MessageVerifier.new(:login).verify(signed_user_id) + signed_user_id.present? && id == Danbooru::MessageVerifier.new(:login).verify(signed_user_id) && self + end + + def authenticate_api_key(key) + api_key.present? && ActiveSupport::SecurityUtils.secure_compare(api_key.key, key) && self + end + + def authenticate_password(password) + BCrypt::Password.new(bcrypt_password_hash) == hash_password(password) && self + end + + def hash_password(password) + Digest::SHA1.hexdigest("choujin-steiner--#{password}--") end module ClassMethods - def authenticate(name, pass) - authenticate_hash(name, sha1(pass)) - end - - def authenticate_api_key(name, api_key) - key = ApiKey.where(:key => api_key).first - return nil if key.nil? - user = find_by_name(name) - return nil if user.nil? - return user if key.user_id == user.id - nil - end - def authenticate_hash(name, hash) user = find_by_name(name) if user && user.bcrypt_password == hash @@ -200,15 +191,6 @@ class User < ApplicationRecord nil end end - - def bcrypt(pass) - BCrypt::Password.create(sha1(pass)) - end - - def sha1(pass) - salt = "choujin-steiner" - Digest::SHA1.hexdigest("#{salt}--#{pass}--") - end end end @@ -610,8 +592,6 @@ class User < ApplicationRecord end include BanMethods - include PasswordMethods - include AuthenticationMethods include LevelMethods include EmailMethods include BlacklistMethods diff --git a/test/functional/passwords_controller_test.rb b/test/functional/passwords_controller_test.rb index 83057663d..aae7c9acb 100644 --- a/test/functional/passwords_controller_test.rb +++ b/test/functional/passwords_controller_test.rb @@ -18,8 +18,8 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest put_auth user_password_path(@user), @user, params: { user: { old_password: "12345", password: "abcde", password_confirmation: "abcde" } } assert_redirected_to @user - assert_equal(nil, User.authenticate(@user.name, "12345")) - assert_equal(@user, User.authenticate(@user.name, "abcde")) + assert_equal(false, @user.reload.authenticate_password("12345")) + assert_equal(@user, @user.authenticate_password("abcde")) end should "update the password when given a valid login key" do @@ -27,24 +27,24 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest put_auth user_password_path(@user), @user, params: { user: { password: "abcde", password_confirmation: "abcde", signed_user_id: signed_user_id } } assert_redirected_to @user - assert_equal(nil, User.authenticate(@user.name, "12345")) - assert_equal(@user, User.authenticate(@user.name, "abcde")) + assert_equal(false, @user.reload.authenticate_password("12345")) + assert_equal(@user, @user.authenticate_password("abcde")) end should "not update the password when given an invalid old password" do put_auth user_password_path(@user), @user, params: { user: { old_password: "3qoirjqe", password: "abcde", password_confirmation: "abcde" } } assert_response :success - assert_equal(@user, User.authenticate(@user.name, "12345")) - assert_equal(nil, User.authenticate(@user.name, "abcde")) + assert_equal(@user, @user.reload.authenticate_password("12345")) + assert_equal(false, @user.authenticate_password("abcde")) end should "not update the password when password confirmation fails for the new password" do put_auth user_password_path(@user), @user, params: { user: { old_password: "12345", password: "abcde", password_confirmation: "qerogijqe" } } assert_response :success - assert_equal(@user, User.authenticate(@user.name, "12345")) - assert_equal(nil, User.authenticate(@user.name, "abcde")) + assert_equal(@user, @user.reload.authenticate_password("12345")) + assert_equal(false, @user.authenticate_password("abcde")) end end end diff --git a/test/functional/sessions_controller_test.rb b/test/functional/sessions_controller_test.rb index 566950e39..d78ea4b16 100644 --- a/test/functional/sessions_controller_test.rb +++ b/test/functional/sessions_controller_test.rb @@ -3,7 +3,7 @@ require 'test_helper' class SessionsControllerTest < ActionDispatch::IntegrationTest context "the sessions controller" do setup do - @user = create(:user) + @user = create(:user, password: "password") end context "new action" do @@ -14,15 +14,27 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest end context "create action" do - should "create a new session" do - post session_path, params: {:name => @user.name, :password => "password"} + should "log the user in when given the correct password" do + post session_path, params: { name: @user.name, password: "password" } assert_redirected_to posts_path assert_equal(@user.id, session[:user_id]) assert_not_nil(@user.reload.last_ip_addr) end - should "not allow IP banned users to create a new session" do + should "not log the user in when given an incorrect password" do + post session_path, params: { name: @user.name, password: "wrong"} + + assert_response 401 + assert_nil(nil, session[:user_id]) + end + + should "redirect the user when given an url param" do + post session_path, params: { name: @user.name, password: "password", url: tags_path } + assert_redirected_to tags_path + end + + should "not allow IP banned users to login" do create(:ip_ban, ip_addr: "1.2.3.4") post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" } diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index a7adcaffc..ed03c3838 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -118,7 +118,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_redirected_to User.last assert_equal("xxx", User.last.name) - assert_equal(User.last, User.authenticate("xxx", "xxxxx1")) + assert_equal(User.last, User.last.authenticate_password("xxxxx1")) assert_equal(nil, User.last.email_address) assert_no_enqueued_emails end @@ -128,7 +128,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_redirected_to User.last assert_equal("xxx", User.last.name) - assert_equal(User.last, User.authenticate("xxx", "xxxxx1")) + assert_equal(User.last, User.last.authenticate_password("xxxxx1")) assert_equal("webmaster@danbooru.donmai.us", User.last.email_address.address) assert_enqueued_email_with UserMailer, :welcome_user, args: [User.last] end diff --git a/test/unit/api_key_test.rb b/test/unit/api_key_test.rb index 09812dfd2..cdc246e68 100644 --- a/test/unit/api_key_test.rb +++ b/test/unit/api_key_test.rb @@ -18,15 +18,15 @@ class ApiKeyTest < ActiveSupport::TestCase end should "authenticate via api key" do - assert_not_nil(User.authenticate_api_key(@user.name, @api_key.key)) + assert_equal(@user, @user.authenticate_api_key(@api_key.key)) end should "not authenticate with the wrong api key" do - assert_nil(User.authenticate_api_key(@user.name, "xxx")) + assert_equal(false, @user.authenticate_api_key("xxx")) end should "not authenticate with the wrong name" do - assert_nil(User.authenticate_api_key("xxx", @api_key.key)) + assert_equal(false, create(:user).authenticate_api_key(@api_key.key)) end should "have the same limits whether or not they have an api key" do diff --git a/test/unit/user_deletion_test.rb b/test/unit/user_deletion_test.rb index f63d987b1..dca99dca9 100644 --- a/test/unit/user_deletion_test.rb +++ b/test/unit/user_deletion_test.rb @@ -43,7 +43,7 @@ class UserDeletionTest < ActiveSupport::TestCase should "reset the password" do @deletion.delete! - assert_nil(User.authenticate(@user.name, "password")) + assert_equal(false, @user.authenticate_password("password")) end should "remove any favorites" do diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 6aa81b3a5..cde212ac4 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -39,11 +39,9 @@ class UserTest < ActiveSupport::TestCase end end - should "authenticate" do - assert(User.authenticate(@user.name, "password"), "Authentication should have succeeded") - assert(!User.authenticate(@user.name, "password2"), "Authentication should not have succeeded") - assert(User.authenticate_hash(@user.name, User.sha1("password")), "Authentication should have succeeded") - assert(!User.authenticate_hash(@user.name, User.sha1("xxx")), "Authentication should not have succeeded") + should "authenticate password" do + assert_equal(@user, @user.authenticate_password("password")) + assert_equal(false, @user.authenticate_password("password2")) end should "normalize its level" do