From bb477a3a0fe71f0e15b78b6e0fafb017065fba26 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sun, 13 Dec 2009 14:26:54 +0000 Subject: [PATCH] Make sure users don't get notified for thing they can not view (#3589). git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@3169 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/models/document.rb | 11 ++++++++ app/models/mailer.rb | 23 +++++++++-------- app/models/message.rb | 7 ++++++ app/models/message_observer.rb | 10 +------- app/models/news.rb | 11 ++++++++ app/models/wiki_content.rb | 11 ++++++++ test/unit/mailer_test.rb | 46 +++++++++++++++++++++++++++++++--- 7 files changed, 97 insertions(+), 22 deletions(-) diff --git a/app/models/document.rb b/app/models/document.rb index 1318e823d..d2d20d05d 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -29,6 +29,10 @@ class Document < ActiveRecord::Base validates_presence_of :project, :title, :category validates_length_of :title, :maximum => 60 + def visible?(user=User.current) + !user.nil? && user.allowed_to?(:view_documents, project) + end + def after_initialize if new_record? self.category ||= DocumentCategory.default @@ -42,4 +46,11 @@ class Document < ActiveRecord::Base end @updated_on end + + # Returns the mail adresses of users that should be notified + def recipients + notified = project.notified_users + notified.reject! {|user| !visible?(user)} + notified.collect(&:mail) + end end diff --git a/app/models/mailer.rb b/app/models/mailer.rb index 3d5231d36..dfd2737ab 100644 --- a/app/models/mailer.rb +++ b/app/models/mailer.rb @@ -94,7 +94,7 @@ class Mailer < ActionMailer::Base # Mailer.deliver_document_added(document) => sends an email to the document's project recipients def document_added(document) redmine_headers 'Project' => document.project.identifier - recipients document.project.recipients + recipients document.recipients subject "[#{document.project.name}] #{l(:label_document_new)}: #{document.title}" body :document => document, :document_url => url_for(:controller => 'documents', :action => 'show', :id => document) @@ -114,15 +114,17 @@ class Mailer < ActionMailer::Base when 'Project' added_to_url = url_for(:controller => 'projects', :action => 'list_files', :id => container) added_to = "#{l(:label_project)}: #{container}" + recipients container.project.notified_users.select {|user| user.allowed_to?(:view_files, container.project)} when 'Version' added_to_url = url_for(:controller => 'projects', :action => 'list_files', :id => container.project_id) added_to = "#{l(:label_version)}: #{container.name}" + recipients container.project.notified_users.select {|user| user.allowed_to?(:view_files, container.project)} when 'Document' added_to_url = url_for(:controller => 'documents', :action => 'show', :id => container.id) added_to = "#{l(:label_document)}: #{container.title}" + recipients container.recipients end redmine_headers 'Project' => container.project.identifier - recipients container.project.recipients subject "[#{container.project.name}] #{l(:label_attachment_new)}" body :attachments => attachments, :added_to => added_to, @@ -138,24 +140,25 @@ class Mailer < ActionMailer::Base def news_added(news) redmine_headers 'Project' => news.project.identifier message_id news - recipients news.project.recipients + recipients news.recipients subject "[#{news.project.name}] #{l(:label_news)}: #{news.title}" body :news => news, :news_url => url_for(:controller => 'news', :action => 'show', :id => news) render_multipart('news_added', body) end - # Builds a tmail object used to email the specified recipients of the specified message that was posted. + # Builds a tmail object used to email the recipients of the specified message that was posted. # # Example: - # message_posted(message, recipients) => tmail object - # Mailer.deliver_message_posted(message, recipients) => sends an email to the recipients - def message_posted(message, recipients) + # message_posted(message) => tmail object + # Mailer.deliver_message_posted(message) => sends an email to the recipients + def message_posted(message) redmine_headers 'Project' => message.project.identifier, 'Topic-Id' => (message.parent_id || message.id) message_id message references message.parent unless message.parent.nil? - recipients(recipients) + recipients(message.recipients) + cc((message.root.watcher_recipients + message.board.watcher_recipients).uniq - @recipients) subject "[#{message.board.project.name} - #{message.board.name} - msg#{message.root.id}] #{message.subject}" body :message => message, :message_url => url_for(:controller => 'messages', :action => 'show', :board_id => message.board_id, :id => message.root) @@ -171,7 +174,7 @@ class Mailer < ActionMailer::Base redmine_headers 'Project' => wiki_content.project.identifier, 'Wiki-Page-Id' => wiki_content.page.id message_id wiki_content - recipients wiki_content.project.recipients + recipients wiki_content.recipients cc(wiki_content.page.wiki.watcher_recipients - recipients) subject "[#{wiki_content.project.name}] #{l(:mail_subject_wiki_content_added, :page => wiki_content.page.pretty_title)}" body :wiki_content => wiki_content, @@ -188,7 +191,7 @@ class Mailer < ActionMailer::Base redmine_headers 'Project' => wiki_content.project.identifier, 'Wiki-Page-Id' => wiki_content.page.id message_id wiki_content - recipients wiki_content.project.recipients + recipients wiki_content.recipients cc(wiki_content.page.wiki.watcher_recipients + wiki_content.page.watcher_recipients - recipients) subject "[#{wiki_content.project.name}] #{l(:mail_subject_wiki_content_updated, :page => wiki_content.page.pretty_title)}" body :wiki_content => wiki_content, diff --git a/app/models/message.rb b/app/models/message.rb index 1e59719dd..535143775 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -90,6 +90,13 @@ class Message < ActiveRecord::Base usr && usr.logged? && (usr.allowed_to?(:delete_messages, project) || (self.author == usr && usr.allowed_to?(:delete_own_messages, project))) end + # Returns the mail adresses of users that should be notified + def recipients + notified = project.notified_users + notified.reject! {|user| !visible?(user)} + notified.collect(&:mail) + end + private def add_author_as_watcher diff --git a/app/models/message_observer.rb b/app/models/message_observer.rb index d37b53813..369ee8862 100644 --- a/app/models/message_observer.rb +++ b/app/models/message_observer.rb @@ -17,14 +17,6 @@ class MessageObserver < ActiveRecord::Observer def after_create(message) - recipients = [] - # send notification to the topic watchers - recipients += message.root.watcher_recipients - # send notification to the board watchers - recipients += message.board.watcher_recipients - # send notification to project members who want to be notified - recipients += message.board.project.recipients - recipients = recipients.compact.uniq - Mailer.deliver_message_posted(message, recipients) if !recipients.empty? && Setting.notified_events.include?('message_posted') + Mailer.deliver_message_posted(message) if Setting.notified_events.include?('message_posted') end end diff --git a/app/models/news.rb b/app/models/news.rb index c53fb05f9..a7b173439 100644 --- a/app/models/news.rb +++ b/app/models/news.rb @@ -29,6 +29,17 @@ class News < ActiveRecord::Base acts_as_activity_provider :find_options => {:include => [:project, :author]}, :author_key => :author_id + def visible?(user=User.current) + !user.nil? && user.allowed_to?(:view_news, project) + end + + # Returns the mail adresses of users that should be notified + def recipients + notified = project.notified_users + notified.reject! {|user| !visible?(user)} + notified.collect(&:mail) + end + # returns latest news for projects visible by user def self.latest(user = User.current, count = 5) find(:all, :limit => count, :conditions => Project.allowed_to_condition(user, :view_news), :include => [ :author, :project ], :order => "#{News.table_name}.created_on DESC") diff --git a/app/models/wiki_content.rb b/app/models/wiki_content.rb index 372ca8365..f81aa9e78 100644 --- a/app/models/wiki_content.rb +++ b/app/models/wiki_content.rb @@ -25,11 +25,22 @@ class WikiContent < ActiveRecord::Base validates_length_of :comments, :maximum => 255, :allow_nil => true acts_as_versioned + + def visible?(user=User.current) + page.visible?(user) + end def project page.project end + # Returns the mail adresses of users that should be notified + def recipients + notified = project.notified_users + notified.reject! {|user| !visible?(user)} + notified.collect(&:mail) + end + class Version belongs_to :page, :class_name => '::WikiPage', :foreign_key => 'page_id' belongs_to :author, :class_name => '::User', :foreign_key => 'author_id' diff --git a/test/unit/mailer_test.rb b/test/unit/mailer_test.rb index b8ff4f003..dfc6caf88 100644 --- a/test/unit/mailer_test.rb +++ b/test/unit/mailer_test.rb @@ -147,7 +147,7 @@ class MailerTest < ActiveSupport::TestCase def test_message_posted_message_id ActionMailer::Base.deliveries.clear message = Message.find(1) - Mailer.deliver_message_posted(message, message.author.mail) + Mailer.deliver_message_posted(message) mail = ActionMailer::Base.deliveries.last assert_not_nil mail assert_equal Mailer.message_id_for(message), mail.message_id @@ -157,13 +157,47 @@ class MailerTest < ActiveSupport::TestCase def test_reply_posted_message_id ActionMailer::Base.deliveries.clear message = Message.find(3) - Mailer.deliver_message_posted(message, message.author.mail) + Mailer.deliver_message_posted(message) mail = ActionMailer::Base.deliveries.last assert_not_nil mail assert_equal Mailer.message_id_for(message), mail.message_id assert_equal Mailer.message_id_for(message.parent), mail.references.first.to_s end + context("#issue_add") do + setup do + ActionMailer::Base.deliveries.clear + Setting.bcc_recipients = '1' + @issue = Issue.find(1) + end + + should "notify project members" do + assert Mailer.deliver_issue_add(@issue) + assert last_email.bcc.include?('dlopper@somenet.foo') + end + + should "not notify project members that are not allow to view the issue" do + Role.find(2).remove_permission!(:view_issues) + assert Mailer.deliver_issue_add(@issue) + assert !last_email.bcc.include?('dlopper@somenet.foo') + end + + should "notify issue watchers" do + user = User.find(9) + Watcher.create!(:watchable => @issue, :user => user) + assert Mailer.deliver_issue_add(@issue) + assert last_email.bcc.include?(user.mail) + end + + should "not notify watchers not allowed to view the issue" do + user = User.find(9) + Watcher.create!(:watchable => @issue, :user => user) + Role.non_member.remove_permission!(:view_issues) + assert Mailer.deliver_issue_add(@issue) + assert !last_email.bcc.include?(user.mail) + end + end + # test mailer methods for each language def test_issue_add issue = Issue.find(1) @@ -211,7 +245,7 @@ class MailerTest < ActiveSupport::TestCase recipients = recipients.compact.uniq valid_languages.each do |lang| Setting.default_language = lang.to_s - assert Mailer.deliver_message_posted(message, recipients) + assert Mailer.deliver_message_posted(message) end end @@ -256,4 +290,10 @@ class MailerTest < ActiveSupport::TestCase assert mail.bcc.include?('dlopper@somenet.foo') assert mail.body.include?('Bug #3: Error 281 when updating a recipe') end + + def last_email + mail = ActionMailer::Base.deliveries.last + assert_not_nil mail + mail + end end