users: improve sockpuppet detection on signup.

Require new accounts to verify their email address if any of the
following conditions are true:

* Their IP is a proxy.
* Their IP is under a partial IP ban.
* They're creating a new account while logged in to another account.
* Somebody recently created an account from the same IP in the last week.

Changes from before:

* Allow logged in users to view the signup page and create new accounts.
  Creating a new account while logged in to your old account is now
  allowed, but it requires email verification. This is a honeypot.
* Creating multiple accounts from the same IP is now allowed, but they
  require email verification. Previously the same IP check was only for
  the last day (now it's the last week), and only for an exact IP match
  (now it's a subnet match, /24 for IPv4 or /64 for IPv6).
* New account verification is disabled for private IPs (e.g. 127.0.0.1,
  192.168.0.1), to make development or running personal boorus easier
  (fixes #4618).

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,

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"

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

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

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