From 3a018ee9f771430bf1d3888e8cc17c355a409205 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 23 Feb 2020 01:41:54 -0600 Subject: [PATCH] dmails: set sender name and ip address explicitly. Set the sender name and IP addresses explicitly in the controller rather than implicitly in the model. Fixes cases where automated dmails from DanbooruBot had their IP addresses set to the person who triggered the dmail, even though they didn't actually send the dmail themselves. --- app/controllers/dmails_controller.rb | 2 +- app/models/dmail.rb | 16 ++++------------ test/functional/dmails_controller_test.rb | 12 +++++------- test/unit/dmail_test.rb | 22 +++++++--------------- 4 files changed, 17 insertions(+), 35 deletions(-) diff --git a/app/controllers/dmails_controller.rb b/app/controllers/dmails_controller.rb index 960503dbf..869238bec 100644 --- a/app/controllers/dmails_controller.rb +++ b/app/controllers/dmails_controller.rb @@ -33,7 +33,7 @@ class DmailsController < ApplicationController end def create - @dmail = Dmail.create_split(dmail_params(:create)) + @dmail = Dmail.create_split(from: CurrentUser.user, creator_ip_addr: CurrentUser.ip_addr, **dmail_params(:create)) respond_with(@dmail) end diff --git a/app/models/dmail.rb b/app/models/dmail.rb index 882ad7f83..491c06d72 100644 --- a/app/models/dmail.rb +++ b/app/models/dmail.rb @@ -10,7 +10,6 @@ class Dmail < ApplicationRecord belongs_to :from, :class_name => "User" has_many :moderation_reports, as: :model - after_initialize :initialize_attributes, if: :new_record? before_create :autoreport_spam after_save :update_unread_dmail_count after_commit :send_email, on: :create @@ -28,11 +27,6 @@ class Dmail < ApplicationRecord def to_name=(name) self.to = User.find_by_name(name) end - - def initialize_attributes - self.from_id ||= CurrentUser.id - self.creator_ip_addr ||= CurrentUser.ip_addr - end end module FactoryMethods @@ -59,12 +53,10 @@ class Dmail < ApplicationRecord end def create_automated(params) - CurrentUser.as_system do - dmail = Dmail.new(from: User.system, **params) - dmail.owner = dmail.to - dmail.save - dmail - end + dmail = Dmail.new(from: User.system, creator_ip_addr: "127.0.0.1", **params) + dmail.owner = dmail.to + dmail.save + dmail end end diff --git a/test/functional/dmails_controller_test.rb b/test/functional/dmails_controller_test.rb index 607b8aa46..f62d01c1c 100644 --- a/test/functional/dmails_controller_test.rb +++ b/test/functional/dmails_controller_test.rb @@ -166,12 +166,10 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest @sender = create(:user) @recipient = create(:user) - as(@sender) do - @dmail1 = Dmail.create_split(title: "test1", body: "test", to: @recipient) - @dmail2 = Dmail.create_split(title: "test2", body: "test", to: @recipient) - @dmail3 = Dmail.create_split(title: "test3", body: "test", to: @recipient, is_read: true) - @dmail4 = Dmail.create_split(title: "test4", body: "test", to: @recipient, is_deleted: true) - end + @dmail1 = create(:dmail, from: @sender, owner: @recipient, to: @recipient) + @dmail2 = create(:dmail, from: @sender, owner: @recipient, to: @recipient) + @dmail3 = create(:dmail, from: @sender, owner: @recipient, to: @recipient, is_read: true) + @dmail4 = create(:dmail, from: @sender, owner: @recipient, to: @recipient, is_deleted: true) end should "mark all unread, undeleted dmails as read" do @@ -182,7 +180,7 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_equal(0, @recipient.reload.unread_dmail_count) - assert_equal(true, [@dmail1, @dmail2, @dmail3, @dmail4].all?(&:is_read)) + assert_equal(true, [@dmail1, @dmail2, @dmail3, @dmail4].map(&:reload).all?(&:is_read)) end end diff --git a/test/unit/dmail_test.rb b/test/unit/dmail_test.rb index 9a85b8509..0814c2181 100644 --- a/test/unit/dmail_test.rb +++ b/test/unit/dmail_test.rb @@ -29,14 +29,11 @@ class DmailTest < ActiveSupport::TestCase end context "from a banned user" do - setup do - @user.update_attribute(:is_banned, true) - end - should "not validate" do - dmail = FactoryBot.build(:dmail, :title => "xxx", :owner => @user) - dmail.save - assert_equal(1, dmail.errors.size) + user = create(:banned_user) + dmail = build(:dmail, owner: user, from: user) + + assert_equal(false, dmail.valid?) assert_equal(["Sender is banned and cannot send messages"], dmail.errors.full_messages) end end @@ -85,15 +82,10 @@ 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(:to_id => @new_user.id, :title => "foo", :body => "foo") + Dmail.create_split(from: CurrentUser.user, creator_ip_addr: "127.0.0.1", to_id: @new_user.id, title: "foo", body: "foo") end end - should "record the creator's ip addr" do - dmail = FactoryBot.create(:dmail, owner: @user) - assert_equal(CurrentUser.ip_addr, dmail.creator_ip_addr.to_s) - end - should "send an email if the user wants it" do user = create(:user, receive_email_notifications: true) assert_difference("ActionMailer::Base.deliveries.size", 1) do @@ -104,14 +96,14 @@ class DmailTest < ActiveSupport::TestCase should "create only one message for a split response" do user = FactoryBot.create(:user, :receive_email_notifications => true) assert_difference("ActionMailer::Base.deliveries.size", 1) do - Dmail.create_split(:to_id => user.id, :title => "foo", :body => "foo") + Dmail.create_split(from: CurrentUser.user, creator_ip_addr: "127.0.0.1", to_id: user.id, title: "foo", body: "foo") end end should "notify the recipient he has mail" do recipient = create(:user) - Dmail.create_split(title: "hello", body: "hello", to_id: recipient.id) + create(:dmail, owner: recipient, to: recipient) assert_equal(1, recipient.reload.unread_dmail_count) recipient.dmails.unread.last.update!(is_read: true)