fixes for :read_forum and :read_announcements permissions

test plan:
* create a course role with the ability to
manage course content, but "View announcements" is disabled
* a user with that role should not see the Announcements tab

* having "Moderate forum" rights should not grant read
rights to read discussions and announcements (if those are
disabled)

* having "View announcements" or "View discussions" turned
 off should prevent new announcements and new discussions
 from showing in the dashboard or from sending
 notifications to the restricted users

closes #CNVS-28438 #CNVS-28469 #CNVS-28541

Change-Id: I9178d30edb33a6e83c492189f4a6fd1e496c42fa
Reviewed-on: https://gerrit.instructure.com/76294
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Tested-by: Jenkins
QA-Review: Jahnavi Yetukuri <jyetukuri@instructure.com>
Product-Review: Allison Weiss <allison@instructure.com>

Author: maneframe

app/models/announcement.rb

@@ -60,7 +60,7 @@ def self.lock_from_course(course)
 
   set_broadcast_policy! do
     dispatch :new_announcement
-    to { active_participants(true) - [user] }
+    to { users_with_permissions(active_participants(true) - [user]) }
     whenever { |record|
       record.send_notification_for_context? and
         ((record.just_created and !(record.post_delayed? || record.unpublished?)) || record.changed_state(:active, :unpublished) || record.changed_state(:active, :post_delayed))
@@ -100,7 +100,7 @@ def self.lock_from_course(course)
     given { |user, session| self.context.is_a?(Group) && self.context.grants_right?(user, session, :post_to_forum) }
     can :create
 
-    given { |user, session| self.context.grants_right?(user, session, :moderate_forum) } #admins.include?(user) }
+    given { |user, session| self.context.grants_all_rights?(user, session, :read_announcements, :moderate_forum) } #admins.include?(user) }
     can :update and can :delete and can :reply and can :create and can :read and can :attach
 
     given do |user, session|

app/models/course.rb

@@ -2418,16 +2418,16 @@ def uncached_tabs_available(user, opts)
         tabs.detect { |t| t[:id] == TAB_DISCUSSIONS }[:manageable] = true if self.grants_right?(user, opts[:session], :moderate_forum)
         tabs.delete_if { |t| t[:id] == TAB_SETTINGS } unless self.grants_right?(user, opts[:session], :read_as_admin)
 
+        unless announcements.temp_record.grants_right?(user, :read)
+          tabs.delete_if { |t| t[:id] == TAB_ANNOUNCEMENTS }
+        end
+
         if !user || !self.grants_right?(user, :manage_content)
           # remove some tabs for logged-out users or non-students
           unless grants_any_right?(user, :read_as_admin, :participate_as_student)
             tabs.delete_if {|t| [TAB_PEOPLE, TAB_OUTCOMES].include?(t[:id]) }
           end
 
-          unless announcements.temp_record.grants_right?(user, :read)
-            tabs.delete_if { |t| t[:id] == TAB_ANNOUNCEMENTS }
-          end
-
           # remove hidden tabs from students
           unless self.grants_right?(user, opts[:session], :read_as_admin)
             tabs.delete_if {|t| (t[:hidden] || (t[:hidden_unused] && !opts[:include_hidden_unused])) && !t[:manageable] }

app/models/discussion_topic.rb

@@ -183,7 +183,7 @@ def refresh_subtopics
         sub_topics << ensure_child_topic_for(group)
       end
     end
-    
+
     self.shard.activate do
       # delete any lingering child topics
       DiscussionTopic.where(:root_topic_id => self).where.not(:id => sub_topics).update_all(:workflow_state => "deleted")
@@ -861,11 +861,11 @@ def initialize_last_reply_at
     given { |user, session| context.respond_to?(:allow_student_forum_attachments) && context.allow_student_forum_attachments && context.grants_right?(user, session, :post_to_forum) }
     can :attach
 
-    given { |user, session| !self.root_topic_id && self.context.grants_right?(user, session, :moderate_forum) && self.available_for?(user) }
+    given { |user, session| !self.root_topic_id && self.context.grants_all_rights?(user, session, :read_forum, :moderate_forum) && self.available_for?(user) }
     can :update and can :delete and can :create and can :read and can :attach
 
     # Moderators can still modify content even in unavailable topics (*especially* unlocking them), but can't create new content
-    given { |user, session| !self.root_topic_id && self.context.grants_right?(user, session, :moderate_forum) }
+    given { |user, session| !self.root_topic_id && self.context.grants_all_rights?(user, session, :read_forum, :moderate_forum) }
     can :update and can :delete and can :read and can :attach
 
     given { |user, session| self.root_topic && self.root_topic.grants_right?(user, session, :update) }
@@ -961,7 +961,7 @@ def send_notification_for_context?
 
   set_broadcast_policy do |p|
     p.dispatch :new_discussion_topic
-    p.to { active_participants_with_visibility - [user] }
+    p.to { users_with_permissions(active_participants_with_visibility - [user]) }
     p.whenever { |record|
       record.send_notification_for_context? and
       ((record.just_created && record.active?) || record.changed_state(:active, !record.is_announcement ? :unpublished : :post_delayed))
@@ -986,6 +986,10 @@ def active_participants(include_observers=false)
     end
   end
 
+  def users_with_permissions(users)
+    users.select{|u| self.is_announcement ? self.context.grants_right?(u, :read_announcements) : self.context.grants_right?(u, :read_forum)}
+  end
+
   def course
     @course ||= context.is_a?(Group) ? context.context : context
   end
@@ -1066,6 +1070,8 @@ def visible_for?(user = nil)
       # user is the topic's author
       return true if user == self.user
 
+      return false if user && !(is_announcement ? context.grants_right?(user, :read_announcements) : context.grants_right?(user, :read_forum))
+
       # user is an admin in the context (teacher/ta/designer) OR
       # user is an account admin with appropriate permission
       return true if context.grants_any_right?(user, :manage, :read_course_content)

spec/models/announcement_spec.rb

@@ -92,6 +92,13 @@
       a = @course.announcements.create!(valid_announcement_attributes)
       expect(a.grants_right?(@user, :read)).to be(false)
     end
+
+    it 'does not allow announcements to be viewed without :read_announcements (even with moderate_forum)' do
+      course_with_teacher(active_all: true)
+      @course.account.role_overrides.create!(permission: 'read_announcements', role: teacher_role, enabled: false)
+      a = @course.announcements.create!(valid_announcement_attributes)
+      expect(a.grants_right?(@user, :read)).to be(false)
+    end
   end
 
   context "broadcast policy" do
@@ -147,5 +154,18 @@
       expect(to_users).to include(@observer)
       expect(@a.messages_sent["Announcement Created By You"].map(&:user)).to include(@teacher)
     end
+
+    it "should not broadcast if read_announcements is diabled" do
+      Account.default.role_overrides.create!(:role => student_role, :permission => 'read_announcements', :enabled => false)
+      course_with_student(:active_all => true)
+      notification_name = "New Announcement"
+      n = Notification.create(:name => notification_name, :category => "TestImmediately")
+      NotificationPolicy.create(:notification => n, :communication_channel => @student.communication_channel, :frequency => "immediately")
+
+      @context = @course
+      announcement_model(:user => @teacher)
+
+      expect(@a.messages_sent[notification_name]).to be_blank
+    end
   end
 end

spec/models/course_spec.rb

@@ -1505,6 +1505,12 @@ def setup_DA
       tab_ids = @course.tabs_available(@user).map{|t| t[:id] }
       expect(tab_ids).to eql(Course.default_tabs.map{|t| t[:id] })
     end
+
+    it "should not include Announcements without read_announcements rights" do
+      @course.account.role_overrides.create!(:role => teacher_role, :permission => 'read_announcements', :enabled => false)
+      tab_ids = @course.uncached_tabs_available(@teacher, {}).map{|t| t[:id] }
+      expect(tab_ids).to_not include(Course::TAB_ANNOUNCEMENTS)
+    end
   end
 
   context "students" do
@@ -1581,7 +1587,6 @@ def setup_DA
       Lti::MessageHandler.stubs(:lti_apps_tabs).returns([mock_tab])
       expect(@course.tabs_available(nil, :include_external => true)).to include(mock_tab)
     end
-
   end
 
   context "observers" do

spec/models/discussion_topic_spec.rb

@@ -92,6 +92,11 @@
       @relevant_permissions = [:read, :reply, :update, :delete]
     end
 
+    it "should not grant moderate permissions without read permissions" do
+      @course.account.role_overrides.create!(:role => teacher_role, :permission => 'read_forum', :enabled => false)
+      expect((@topic.check_policy(@teacher2) & @relevant_permissions)).to be_empty
+    end
+
     it "should grant permissions if it not locked" do
       @topic.publish!
       expect((@topic.check_policy(@teacher1) & @relevant_permissions).map(&:to_s).sort).to eq ['read', 'reply', 'update', 'delete'].sort
@@ -213,7 +218,7 @@
 
       account = @course.root_account
       nobody_role = custom_account_role('NobodyAdmin', account: account)
-      account_with_role_changes(account: account, role: nobody_role, role_changes: { read_course_content: true })
+      account_with_role_changes(account: account, role: nobody_role, role_changes: { read_course_content: true, read_forum: true })
       admin = account_admin_user(account: account, role: nobody_role, active_user: true)
       expect(@topic.visible_for?(admin)).to be_truthy
     end

spec/selenium/dashboard_spec.rb

@@ -81,6 +81,16 @@ def test_hiding(url)
       end
     end
 
+    it "should not show announcement stream items without permissions" do
+      @course.account.role_overrides.create!(:role => student_role, :permission => 'read_announcements', :enabled => false)
+
+      announcement = create_announcement
+      item_selector = '#announcement-details tbody tr'
+
+      get "/"
+      expect(f('.no_recent_messages')).to include_text('No Recent Messages')
+    end
+
     def click_recent_activity_header(type='announcement')
       f(".stream-#{type} .stream_header").click
     end