diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index eb0e2f7b4..f28cd9390 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -59,7 +59,7 @@ class UsersController < ApplicationController end def create - requires_verification = IpLookup.new(CurrentUser.ip_addr).is_proxy? || IpBan.hit!(:partial, CurrentUser.ip_addr) + requires_verification = UserVerifier.new(CurrentUser.user, request).requires_verification? @user = authorize User.new( last_ip_addr: CurrentUser.ip_addr, diff --git a/app/logical/danbooru/http.rb b/app/logical/danbooru/http.rb index e1b76b16b..bbf30cafd 100644 --- a/app/logical/danbooru/http.rb +++ b/app/logical/danbooru/http.rb @@ -1,3 +1,4 @@ +require "danbooru/http/application_client" require "danbooru/http/html_adapter" require "danbooru/http/xml_adapter" require "danbooru/http/cache" diff --git a/app/logical/user_verifier.rb b/app/logical/user_verifier.rb new file mode 100644 index 000000000..e225924ae --- /dev/null +++ b/app/logical/user_verifier.rb @@ -0,0 +1,46 @@ +# Checks whether a new account seems suspicious and should require email verification. + +class UserVerifier + attr_reader :current_user, :request + + # current_user is the user creating the new account, not the new account itself. + def initialize(current_user, request) + @current_user, @request = current_user, request + end + + def requires_verification? + return false if is_local_ip? + + # we check for IP bans first to make sure we bump the IP ban hit count + is_ip_banned? || is_logged_in? || is_recent_signup? || is_proxy? + end + + private + + def ip_address + @ip_address ||= IPAddress.parse(request.remote_ip) + end + + def is_local_ip? + ip_address.loopback? || ip_address.link_local? || ip_address.private? || ip_address.try(:unique_local?) + end + + def is_logged_in? + !current_user.is_anonymous? + end + + def is_recent_signup?(age: 24.hours) + subnet_len = ip_address.ipv4? ? 24 : 64 + subnet = "#{ip_address}/#{subnet_len}" + + User.where("last_ip_addr <<= ?", subnet).where("created_at > ?", age.ago).exists? + end + + def is_ip_banned? + IpBan.hit!(:partial, ip_address.to_s) + end + + def is_proxy? + IpLookup.new(ip_address).is_proxy? + end +end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index fa1dff0f4..1aaaaa84f 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -1,6 +1,10 @@ class UserPolicy < ApplicationPolicy def create? - user.is_anonymous? && !sockpuppet? + true + end + + def new? + true end def update? @@ -31,10 +35,6 @@ class UserPolicy < ApplicationPolicy user.is_admin? || record.id == user.id || !record.enable_private_favorites? end - def sockpuppet? - User.where(last_ip_addr: request.remote_ip).where("created_at > ?", 1.day.ago).exists? - end - def permitted_attributes_for_create [:name, :password, :password_confirmation, { email_address_attributes: [:address] }] end diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index fe4353683..38216a1a6 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -243,6 +243,11 @@ class UsersControllerTest < ActionDispatch::IntegrationTest get new_user_path assert_response :success end + + should "render for a logged in user" do + get_auth new_user_path, @user + assert_response :success + end end context "create action" do @@ -256,11 +261,6 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_no_enqueued_emails end - should "not allow logged in users to create a new account" do - post_auth users_path, @user, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} - assert_response 403 - end - should "create a user with a valid email" do post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email: "webmaster@danbooru.donmai.us" }} @@ -289,45 +289,71 @@ class UsersControllerTest < ActionDispatch::IntegrationTest end end - should "mark users signing up from proxies as requiring verification" do - skip unless IpLookup.enabled? + context "sockpuppet detection" do + setup do + @private_ip = "192.168.0.1" + @valid_ip = "187.37.226.17" # a random valid, non-proxy public IP + @proxy_ip = "51.15.128.1" + end - self.remote_addr = "51.15.128.1" - post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + should "mark accounts created by already logged in users as requiring verification" do + self.remote_addr = @valid_ip - assert_redirected_to User.last - assert_equal(true, User.last.requires_verification) - end + post_auth users_path, @user, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} - should "mark users signing up from a partial banned IP as requiring verification" do - skip unless IpLookup.enabled? - self.remote_addr = "187.37.226.17" + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + end - @ip_ban = create(:ip_ban, ip_addr: self.remote_addr, category: :partial) - post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + should "mark users signing up from proxies as requiring verification" do + skip unless IpLookup.enabled? + self.remote_addr = @proxy_ip - assert_redirected_to User.last - assert_equal(true, User.last.requires_verification) - assert_equal(1, @ip_ban.reload.hit_count) - assert(@ip_ban.last_hit_at > 1.minute.ago) - end + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} - should "not mark users signing up from non-proxies as requiring verification" do - skip unless IpLookup.enabled? - self.remote_addr = "187.37.226.17" - post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + end - assert_redirected_to User.last - assert_equal(false, User.last.requires_verification) - end + should "mark users signing up from a partial banned IP as requiring verification" do + self.remote_addr = @valid_ip - context "with sockpuppet validation enabled" do - should "not allow registering multiple accounts with the same IP" do - assert_difference("User.count", 0) do - @user.update(last_ip_addr: "127.0.0.1") - post users_path, params: {:user => {:name => "dupe", :password => "xxxxx1", :password_confirmation => "xxxxx1"}} - assert_response 403 - end + @ip_ban = create(:ip_ban, ip_addr: self.remote_addr, category: :partial) + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + assert_equal(1, @ip_ban.reload.hit_count) + assert(@ip_ban.last_hit_at > 1.minute.ago) + end + + should "not mark users signing up from non-proxies as requiring verification" do + skip unless IpLookup.enabled? + self.remote_addr = @valid_ip + + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(false, User.last.requires_verification) + end + + should "mark accounts registered from an IPv4 address recently used for another account as requiring verification" do + @user.update!(last_ip_addr: @valid_ip) + self.remote_addr = @valid_ip + + post users_path, params: { user: { name: "dupe", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + end + + should "not mark users signing up from localhost as requiring verification" do + self.remote_addr = "127.0.0.1" + + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(false, User.last.requires_verification) end end end