users: refactor login and authentication logic.

* Make authentication methods into User instance methods instead of
  class methods.
* Fix API key authentication to use a secure string comparison. Fixes a
  hypothetical (unlikely to be exploitable) timing attack.
* Move login logic from SessionCreator to SessionLoader.

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"

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

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"

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

app/logical/session_creator.rb

@@ -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

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)

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
 

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