From d4da8499ce4e420f9e4ada7105dda5b96de26d05 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 18 Sep 2022 03:02:30 -0500 Subject: [PATCH] models: stop saving IP addresses in version tables. Mark various `creator_ip_addr` and `updater_ip_addr` columns as ignored and stop updating them in preparation for dropping them. --- app/controllers/forum_posts_controller.rb | 2 +- app/controllers/forum_topics_controller.rb | 4 ++-- app/controllers/uploads_controller.rb | 4 ++-- app/logical/spam_detector.rb | 6 +++--- app/models/application_record.rb | 1 - app/models/artist.rb | 1 - app/models/artist_commentary_version.rb | 2 ++ app/models/artist_version.rb | 2 ++ app/models/comment.rb | 6 +++++- app/models/dmail.rb | 8 ++++++-- app/models/forum_post.rb | 3 ++- app/models/note.rb | 7 +++---- app/models/note_version.rb | 2 ++ app/models/pool.rb | 4 ++-- app/models/pool_version.rb | 5 +++-- app/models/post.rb | 5 +++-- app/models/post_version.rb | 3 ++- app/models/upload.rb | 2 ++ app/models/wiki_page.rb | 1 - app/models/wiki_page_version.rb | 2 ++ db/populate.rb | 6 ++---- test/factories/comment.rb | 1 - test/factories/dmail.rb | 1 - test/factories/post.rb | 1 - test/factories/upload.rb | 1 - test/unit/concerns/searchable.rb | 17 ++++++++++------- test/unit/dmail_test.rb | 9 ++++----- test/unit/note_test.rb | 4 ---- 28 files changed, 60 insertions(+), 50 deletions(-) diff --git a/app/controllers/forum_posts_controller.rb b/app/controllers/forum_posts_controller.rb index 0ab945b58..e293b01c6 100644 --- a/app/controllers/forum_posts_controller.rb +++ b/app/controllers/forum_posts_controller.rb @@ -38,7 +38,7 @@ class ForumPostsController < ApplicationController end def create - @forum_post = authorize ForumPost.new(creator: CurrentUser.user, topic_id: params.dig(:forum_post, :topic_id)) + @forum_post = authorize ForumPost.new(creator: CurrentUser.user, creator_ip_addr: CurrentUser.ip_addr, topic_id: params.dig(:forum_post, :topic_id)) @forum_post.update(permitted_attributes(@forum_post)) page = @forum_post.topic.last_page if @forum_post.topic.last_page > 1 diff --git a/app/controllers/forum_topics_controller.rb b/app/controllers/forum_topics_controller.rb index b83b04957..d6aa47816 100644 --- a/app/controllers/forum_topics_controller.rb +++ b/app/controllers/forum_topics_controller.rb @@ -56,9 +56,9 @@ class ForumTopicsController < ApplicationController end def create - @forum_topic = authorize ForumTopic.new(permitted_attributes(ForumTopic)) - @forum_topic.creator = CurrentUser.user + @forum_topic = authorize ForumTopic.new(creator: CurrentUser.user, **permitted_attributes(ForumTopic)) @forum_topic.original_post.creator = CurrentUser.user + @forum_topic.original_post.creator_ip_addr = CurrentUser.ip_addr @forum_topic.save respond_with(@forum_topic) diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 1d3015850..66a10140c 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -5,12 +5,12 @@ class UploadsController < ApplicationController skip_before_action :verify_authenticity_token, only: [:create], if: -> { request.xhr? } def new - @upload = authorize Upload.new(uploader: CurrentUser.user, uploader_ip_addr: CurrentUser.ip_addr, source: params[:url], referer_url: params[:ref], **permitted_attributes(Upload)) + @upload = authorize Upload.new(uploader: CurrentUser.user, source: params[:url], referer_url: params[:ref], **permitted_attributes(Upload)) respond_with(@upload) end def create - @upload = authorize Upload.new(uploader: CurrentUser.user, uploader_ip_addr: CurrentUser.ip_addr, **permitted_attributes(Upload)) + @upload = authorize Upload.new(uploader: CurrentUser.user, **permitted_attributes(Upload)) @upload.save respond_with(@upload) end diff --git a/app/logical/spam_detector.rb b/app/logical/spam_detector.rb index 286c5d222..eba3b03a8 100644 --- a/app/logical/spam_detector.rb +++ b/app/logical/spam_detector.rb @@ -65,14 +65,14 @@ class SpamDetector # Initialize a spam check for a message. # @param record [Dmail, ForumPost, Comment] the message to spam check # @param user_ip [String] the IP address of the user who posted the message - def initialize(record, user_ip: nil) + def initialize(record, user_ip:) case record when Dmail @record = record @user = record.from @content = record.body @comment_type = "message" - @user_ip = user_ip || record.creator_ip_addr.to_s + @user_ip = user_ip when ForumPost @record = record @user = record.creator @@ -84,7 +84,7 @@ class SpamDetector @user = record.creator @content = record.body @comment_type = "comment" - @user_ip = user_ip || record.creator_ip_addr.to_s + @user_ip = user_ip else raise ArgumentError end diff --git a/app/models/application_record.rb b/app/models/application_record.rb index faca00400..a1996b067 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -191,7 +191,6 @@ class ApplicationRecord < ActiveRecord::Base belongs_to :updater, class_name: "User", **options before_validation do |rec| rec.updater_id = CurrentUser.id - rec.updater_ip_addr = CurrentUser.ip_addr if rec.respond_to?(:updater_ip_addr=) end end end diff --git a/app/models/artist.rb b/app/models/artist.rb index 8090bf0cf..257cd6c17 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -111,7 +111,6 @@ class Artist < ApplicationRecord :artist_id => id, :name => name, :updater_id => CurrentUser.id, - :updater_ip_addr => CurrentUser.ip_addr, :urls => url_array, :is_deleted => is_deleted, :is_banned => is_banned, diff --git a/app/models/artist_commentary_version.rb b/app/models/artist_commentary_version.rb index ec63655b1..d87c7810a 100644 --- a/app/models/artist_commentary_version.rb +++ b/app/models/artist_commentary_version.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class ArtistCommentaryVersion < ApplicationRecord + self.ignored_columns = [:updater_ip_addr] + belongs_to :post belongs_to_updater diff --git a/app/models/artist_version.rb b/app/models/artist_version.rb index 58e09a639..352172efb 100644 --- a/app/models/artist_version.rb +++ b/app/models/artist_version.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class ArtistVersion < ApplicationRecord + self.ignored_columns = [:updater_ip_addr] + array_attribute :urls array_attribute :other_names diff --git a/app/models/comment.rb b/app/models/comment.rb index cd24a5c0b..679eeda4d 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true class Comment < ApplicationRecord + self.ignored_columns = [:creator_ip_addr, :updater_ip_addr] + + attr_accessor :creator_ip_addr + belongs_to :post belongs_to :creator, class_name: "User" belongs_to_updater @@ -52,7 +56,7 @@ class Comment < ApplicationRecord extend SearchMethods def autoreport_spam - if SpamDetector.new(self).spam? + if SpamDetector.new(self, user_ip: creator_ip_addr).spam? moderation_reports << ModerationReport.new(creator: User.system, reason: "Spam.") end end diff --git a/app/models/dmail.rb b/app/models/dmail.rb index b48afc2d2..add6e06b6 100644 --- a/app/models/dmail.rb +++ b/app/models/dmail.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true class Dmail < ApplicationRecord + self.ignored_columns = [:creator_ip_addr] + + attr_accessor :creator_ip_addr + validate :validate_sender_is_not_limited, on: :create validates :title, presence: true, length: { maximum: 200 }, if: :title_changed? validates :body, presence: true, length: { maximum: 50_000 }, if: :body_changed? @@ -52,7 +56,7 @@ class Dmail < ApplicationRecord end def create_automated(params) - dmail = Dmail.new(from: User.system, creator_ip_addr: "127.0.0.1", **params) + dmail = Dmail.new(from: User.system, **params) dmail.owner = dmail.to dmail.save dmail @@ -171,7 +175,7 @@ class Dmail < ApplicationRecord end def autoreport_spam - if is_recipient? && !is_sender? && SpamDetector.new(self).spam? + if is_recipient? && !is_sender? && SpamDetector.new(self, user_ip: creator_ip_addr).spam? self.is_deleted = true moderation_reports << ModerationReport.new(creator: User.system, reason: "Spam.") end diff --git a/app/models/forum_post.rb b/app/models/forum_post.rb index 3c2aa39ee..c176a491c 100644 --- a/app/models/forum_post.rb +++ b/app/models/forum_post.rb @@ -2,6 +2,7 @@ class ForumPost < ApplicationRecord attr_readonly :topic_id + attr_accessor :creator_ip_addr belongs_to :creator, class_name: "User" belongs_to_updater @@ -97,7 +98,7 @@ class ForumPost < ApplicationRecord end def autoreport_spam - if SpamDetector.new(self, user_ip: CurrentUser.ip_addr).spam? + if SpamDetector.new(self, user_ip: creator_ip_addr).spam? moderation_reports << ModerationReport.new(creator: User.system, reason: "Spam.") end end diff --git a/app/models/note.rb b/app/models/note.rb index 04dcab700..ceb8a9b13 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -54,7 +54,7 @@ class Note < ApplicationRecord end end - def create_version(updater: CurrentUser.user, updater_ip_addr: CurrentUser.ip_addr) + def create_version(updater: CurrentUser.user) return unless saved_change_to_versioned_attributes? if merge_version?(updater.id) @@ -62,7 +62,7 @@ class Note < ApplicationRecord else Note.where(:id => id).update_all("version = coalesce(version, 0) + 1") reload - create_new_version(updater.id, updater_ip_addr) + create_new_version(updater.id) end end @@ -70,10 +70,9 @@ class Note < ApplicationRecord new_record? || saved_change_to_x? || saved_change_to_y? || saved_change_to_width? || saved_change_to_height? || saved_change_to_is_active? || saved_change_to_body? end - def create_new_version(updater_id, updater_ip_addr) + def create_new_version(updater_id) versions.create( :updater_id => updater_id, - :updater_ip_addr => updater_ip_addr, :post_id => post_id, :x => x, :y => y, diff --git a/app/models/note_version.rb b/app/models/note_version.rb index 94d1366cf..1e3bfb6f6 100644 --- a/app/models/note_version.rb +++ b/app/models/note_version.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class NoteVersion < ApplicationRecord + self.ignored_columns = [:updater_ip_addr] + belongs_to :post belongs_to :note belongs_to_updater :counter_cache => "note_update_count" diff --git a/app/models/pool.rb b/app/models/pool.rb index 6b51bb5f2..da44b48de 100644 --- a/app/models/pool.rb +++ b/app/models/pool.rb @@ -220,9 +220,9 @@ class Pool < ApplicationRecord post_count > 0 ? Post.find(post_ids.first) : nil end - def create_version(updater: CurrentUser.user, updater_ip_addr: CurrentUser.ip_addr) + def create_version(updater: CurrentUser.user) if PoolVersion.enabled? - PoolVersion.queue(self, updater, updater_ip_addr) + PoolVersion.queue(self, updater) else Rails.logger.warn("Archive service is not configured. Pool versions will not be saved.") end diff --git a/app/models/pool_version.rb b/app/models/pool_version.rb index 5efc6747c..8ceeac4de 100644 --- a/app/models/pool_version.rb +++ b/app/models/pool_version.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class PoolVersion < ApplicationRecord + self.ignored_columns = [:updater_ip_addr] + belongs_to :updater, :class_name => "User" belongs_to :pool @@ -64,7 +66,7 @@ class PoolVersion < ApplicationRecord SqsService.new(Danbooru.config.aws_sqs_archives_url) end - def self.queue(pool, updater, updater_ip_addr) + def self.queue(pool, updater) # queue updates to sqs so that if archives goes down for whatever reason it won't # block pool updates raise NotImplementedError, "Archive service is not configured." if !enabled? @@ -73,7 +75,6 @@ class PoolVersion < ApplicationRecord pool_id: pool.id, post_ids: pool.post_ids, updater_id: updater.id, - updater_ip_addr: updater_ip_addr.to_s, created_at: pool.created_at.try(:iso8601), updated_at: pool.updated_at.try(:iso8601), description: pool.description, diff --git a/app/models/post.rb b/app/models/post.rb index 1a67d3d9c..1ed6a9d0a 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Post < ApplicationRecord + self.ignored_columns = [:uploader_ip_addr] + class RevertError < StandardError; end class DeletionError < StandardError; end @@ -109,7 +111,6 @@ class Post < ApplicationRecord post = Post.new( uploader: upload.uploader, - uploader_ip_addr: upload.uploader_ip_addr, md5: media_asset&.md5, file_ext: media_asset&.file_ext, file_size: media_asset&.file_size, @@ -1420,7 +1421,7 @@ class Post < ApplicationRecord :image_height, :tag_count, :has_children, :has_active_children, :is_pending, :is_flagged, :is_deleted, :is_banned, :last_comment_bumped_at, :last_commented_at, :last_noted_at, - :uploader_ip_addr, :uploader, :approver, :parent, + :uploader, :approver, :parent, :artist_commentary, :flags, :appeals, :notes, :comments, :children, :approvals, :replacements, :pixiv_ugoira_frame_data ) diff --git a/app/models/post_version.rb b/app/models/post_version.rb index 34d35cac0..93f4bae70 100644 --- a/app/models/post_version.rb +++ b/app/models/post_version.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class PostVersion < ApplicationRecord + self.ignored_columns = [:updater_ip_addr] + class RevertError < StandardError; end extend Memoist @@ -91,7 +93,6 @@ class PostVersion < ApplicationRecord "parent_id" => post.parent_id, "source" => post.source, "updater_id" => CurrentUser.id, - "updater_ip_addr" => CurrentUser.ip_addr.to_s, "updated_at" => post.updated_at.try(:iso8601), "created_at" => post.created_at.try(:iso8601), "tags" => post.tag_string, diff --git a/app/models/upload.rb b/app/models/upload.rb index 09f2f7bb3..bceabe7d7 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Upload < ApplicationRecord + self.ignored_columns = [:uploader_ip_addr] + extend Memoist class Error < StandardError; end diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index dec26e130..25d51f55f 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -185,7 +185,6 @@ class WikiPage < ApplicationRecord def create_new_version versions.create( :updater_id => CurrentUser.id, - :updater_ip_addr => CurrentUser.ip_addr, :title => title, :body => body, :is_locked => is_locked, diff --git a/app/models/wiki_page_version.rb b/app/models/wiki_page_version.rb index 64d9ca5f5..f84c59b5d 100644 --- a/app/models/wiki_page_version.rb +++ b/app/models/wiki_page_version.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class WikiPageVersion < ApplicationRecord + self.ignored_columns = [:updater_ip_addr] + array_attribute :other_names belongs_to :wiki_page belongs_to_updater diff --git a/db/populate.rb b/db/populate.rb index 812436bec..a1c6bd67e 100644 --- a/db/populate.rb +++ b/db/populate.rb @@ -56,8 +56,7 @@ def populate_posts(n, search: "rating:s", batch_size: 200, timeout: 30.seconds) posts.each do |danbooru_post| Timeout.timeout(timeout) do user = User.order("random()").first - ip_addr = FFaker::Internet.ip_v4_address - upload = Upload.create(uploader: user, uploader_ip_addr: ip_addr, source: danbooru_post["file_url"]) + upload = Upload.create(uploader: user, source: danbooru_post["file_url"]) sleep 1 until upload.reload.is_finished? # wait for the job worker to process the upload in the background post = Post.new_from_upload(upload.upload_media_assets.first, tag_string: danbooru_post["tag_string"], source: danbooru_post["source"], rating: danbooru_post["rating"]) @@ -77,8 +76,7 @@ def populate_comments(n) n.times do |i| user = User.order("random()").first post = Post.order("random()").first - ip_addr = FFaker::Internet.ip_v4_address - comment = CurrentUser.scoped(user) { Comment.create(creator: user, creator_ip_addr: ip_addr, post: post, body: FFaker::Lorem.paragraph) } + comment = CurrentUser.scoped(user) { Comment.create(creator: user, post: post, body: FFaker::Lorem.paragraph) } puts "Created comment ##{comment.id}" end diff --git a/test/factories/comment.rb b/test/factories/comment.rb index f9e7d6764..665fac853 100644 --- a/test/factories/comment.rb +++ b/test/factories/comment.rb @@ -2,7 +2,6 @@ FactoryBot.define do factory(:comment) do |f| creator post - creator_ip_addr { FFaker::Internet.ip_v4_address } body {FFaker::Lorem.sentences.join(" ")} end end diff --git a/test/factories/dmail.rb b/test/factories/dmail.rb index 1cf559318..5e851552a 100644 --- a/test/factories/dmail.rb +++ b/test/factories/dmail.rb @@ -3,7 +3,6 @@ FactoryBot.define do owner factory: :user from factory: :user to factory: :user - creator_ip_addr { FFaker::Internet.ip_v4_address } title {FFaker::Lorem.words.join(" ")} body {FFaker::Lorem.sentences.join(" ")} end diff --git a/test/factories/post.rb b/test/factories/post.rb index 31c1b94f1..0782e0722 100644 --- a/test/factories/post.rb +++ b/test/factories/post.rb @@ -2,7 +2,6 @@ FactoryBot.define do factory(:post) do md5 { SecureRandom.hex(32) } uploader - uploader_ip_addr {"127.0.0.1"} tag_string {"tag1 tag2"} tag_count {2} tag_count_general {2} diff --git a/test/factories/upload.rb b/test/factories/upload.rb index e2c040c14..21823b810 100644 --- a/test/factories/upload.rb +++ b/test/factories/upload.rb @@ -1,7 +1,6 @@ FactoryBot.define do factory(:upload) do uploader factory: :user - uploader_ip_addr { "127.0.0.1" } status { "pending" } source { "https://cdn.donmai.us/original/d3/4e/d34e4cf0a437a5d65f8e82b7bcd02606.jpg" } diff --git a/test/unit/concerns/searchable.rb b/test/unit/concerns/searchable.rb index d640e5f6a..4b0d49a9e 100644 --- a/test/unit/concerns/searchable.rb +++ b/test/unit/concerns/searchable.rb @@ -11,7 +11,7 @@ class SearchableTest < ActiveSupport::TestCase subject { Post } setup do - @p1 = create(:post, source: "a1", score: 1, is_deleted: true, uploader_ip_addr: "10.0.0.1") + @p1 = create(:post, source: "a1", score: 1, is_deleted: true) @p2 = create(:post, source: "b2", score: 2, is_deleted: false) @p3 = create(:post, source: "c3", score: 3, is_deleted: false) end @@ -108,11 +108,14 @@ class SearchableTest < ActiveSupport::TestCase end context "for an inet attribute" do + subject { UserSession } + should "work" do - assert_search_equals(@p1, uploader_ip_addr: "10.0.0.1") - assert_search_equals(@p1, uploader_ip_addr: "10.0.0.1/24") - assert_search_equals(@p1, uploader_ip_addr: "10.0.0.1,1.1.1.1") - assert_search_equals(@p1, uploader_ip_addr: "10.0.0.1 1.1.1.1") + @us = create(:user_session, ip_addr: "10.0.0.1") + assert_search_equals(@us, ip_addr: "10.0.0.1") + assert_search_equals(@us, ip_addr: "10.0.0.1/24") + assert_search_equals(@us, ip_addr: "10.0.0.1,1.1.1.1") + assert_search_equals(@us, ip_addr: "10.0.0.1 1.1.1.1") end end @@ -258,8 +261,8 @@ class SearchableTest < ActiveSupport::TestCase should "work" do @media_asset = create(:media_asset) - @upload1 = create(:upload, media_assets: [@media_asset]) - @upload2 = create(:upload, media_assets: [@media_asset]) + @upload1 = create(:upload, upload_media_assets: [build(:upload_media_asset, media_asset: @media_asset)]) + @upload2 = create(:upload, upload_media_assets: [build(:upload_media_asset, media_asset: @media_asset)]) assert_search_equals([@upload2, @upload1], media_asset: { md5: @media_asset.md5 }) end diff --git a/test/unit/dmail_test.rb b/test/unit/dmail_test.rb index 156564d32..62ce43600 100644 --- a/test/unit/dmail_test.rb +++ b/test/unit/dmail_test.rb @@ -5,7 +5,6 @@ class DmailTest < ActiveSupport::TestCase setup do @user = FactoryBot.create(:user) CurrentUser.user = @user - CurrentUser.ip_addr = "1.2.3.4" end teardown do @@ -80,7 +79,7 @@ class DmailTest < ActiveSupport::TestCase should "create a copy for each user" do @new_user = FactoryBot.create(:user) assert_difference("Dmail.count", 2) do - Dmail.create_split(from: CurrentUser.user, creator_ip_addr: "127.0.0.1", to: @new_user, title: "foo", body: "foo") + Dmail.create_split(from: CurrentUser.user, to: @new_user, title: "foo", body: "foo") end end @@ -118,11 +117,11 @@ class DmailTest < ActiveSupport::TestCase context "sending a dmail" do should "fail if the user has sent too many dmails recently" do 10.times do - Dmail.create_split(from: @user, to: create(:user), title: "blah", body: "blah", creator_ip_addr: "127.0.0.1") + Dmail.create_split(from: @user, to: create(:user), title: "blah", body: "blah") end assert_no_difference("Dmail.count") do - @dmail = Dmail.create_split(from: @user, to: create(:user), title: "blah", body: "blah", creator_ip_addr: "127.0.0.1") + @dmail = Dmail.create_split(from: @user, to: create(:user), title: "blah", body: "blah") assert_equal(false, @dmail.valid?) assert_equal(["You can't send dmails to more than 10 users per hour"], @dmail.errors[:base]) @@ -133,7 +132,7 @@ class DmailTest < ActiveSupport::TestCase context "destroying a dmail" do setup do @recipient = create(:user) - @dmail = Dmail.create_split(from: @user, to: @recipient, creator_ip_addr: "127.0.0.1", title: "foo", body: "foo") + @dmail = Dmail.create_split(from: @user, to: @recipient, title: "foo", body: "foo") @modreport = create(:moderation_report, model: @dmail) end diff --git a/test/unit/note_test.rb b/test/unit/note_test.rb index f62e89293..4981ff66a 100644 --- a/test/unit/note_test.rb +++ b/test/unit/note_test.rb @@ -5,12 +5,10 @@ class NoteTest < ActiveSupport::TestCase setup do @user = FactoryBot.create(:user) CurrentUser.user = @user - CurrentUser.ip_addr = "127.0.0.1" end teardown do CurrentUser.user = nil - CurrentUser.ip_addr = nil end context "#merge_version" do @@ -77,7 +75,6 @@ class NoteTest < ActiveSupport::TestCase assert_equal(1, @note.version) assert_equal(1, @note.versions.first.version) assert_equal(@user.id, @note.versions.first.updater_id) - assert_equal(CurrentUser.ip_addr, @note.versions.first.updater_ip_addr.to_s) end should "update the post's last_noted_at field" do @@ -120,7 +117,6 @@ class NoteTest < ActiveSupport::TestCase assert_equal("fafafa", @note.versions.last.body) assert_equal(2, @note.version) assert_equal(@user.id, @note.versions.last.updater_id) - assert_equal(CurrentUser.ip_addr, @note.versions.last.updater_ip_addr.to_s) end context "without making any changes" do