jaculabilis/danbooru
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

users: refactor login and authentication logic.

Browse Source 
* 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.
This commit is contained in:
evazion
2020-03-25 03:41:27 -05:00
parent 64af957031
commit b2cf765d6d
14 changed files with 68 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/emails_controller.rb
View File

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

2
app/controllers/maintenance/user/api_keys_controller.rb
View File

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

4
app/controllers/passwords_controller.rb
View File

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

11
app/controllers/sessions_controller.rb
View File

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

23
app/logical/session_creator.rb
View File

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

18
app/logical/session_loader.rb
View File

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

2
app/logical/user_deletion.rb
View File

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

50
app/models/user.rb
View File

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

16
test/functional/passwords_controller_test.rb
View File

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

20
test/functional/sessions_controller_test.rb
View File

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

4
test/functional/users_controller_test.rb
View File

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

6
test/unit/api_key_test.rb
View File

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

2
test/unit/user_deletion_test.rb
View File

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

8
test/unit/user_test.rb
View File

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