pundit: convert forum topics / forum posts to pundit.

Fix it being possible for users to delete or undelete their own forum
posts and topics, even if they were deleted by a mod.

app/controllers/forum_posts_controller.rb

@@ -1,25 +1,14 @@
 class ForumPostsController < ApplicationController
   respond_to :html, :xml, :json, :js
-  before_action :member_only, :except => [:index, :show, :search]
-  before_action :load_post, :only => [:edit, :show, :update, :destroy, :undelete]
-  before_action :check_min_level, :only => [:edit, :show, :update, :destroy, :undelete]
   skip_before_action :api_check
 
   def new
-    if params[:topic_id]
+    @forum_post = authorize ForumPost.new_reply(params)
-      @forum_topic = ForumTopic.find(params[:topic_id])
-      raise User::PrivilegeError.new unless @forum_topic.visible?(CurrentUser.user)
-    end
-    if params[:post_id]
-      quoted_post = ForumPost.find(params[:post_id])
-      raise User::PrivilegeError.new unless quoted_post.topic.visible?(CurrentUser.user)
-    end
-    @forum_post = ForumPost.new_reply(params)
     respond_with(@forum_post)
   end
 
   def edit
-    check_privilege(@forum_post)
+    @forum_post = authorize ForumPost.find(params[:id])
     respond_with(@forum_post)
   end
 
@@ -34,6 +23,8 @@ class ForumPostsController < ApplicationController
   end
 
   def show
+    @forum_post = authorize ForumPost.find(params[:id])
+
     respond_with(@forum_post) do |format|
       format.html do
         page = @forum_post.forum_topic_page
@@ -44,51 +35,29 @@ class ForumPostsController < ApplicationController
   end
 
   def create
-    @forum_post = ForumPost.create(forum_post_params(:create).merge(creator: CurrentUser.user))
+    @forum_post = authorize ForumPost.new(creator: CurrentUser.user, 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
     respond_with(@forum_post, :location => forum_topic_path(@forum_post.topic, :page => page))
   end
 
   def update
-    check_privilege(@forum_post)
+    @forum_post = authorize ForumPost.find(params[:id])
-    @forum_post.update(forum_post_params(:update))
+    @forum_post.update(permitted_attributes(@forum_post))
     page = @forum_post.forum_topic_page if @forum_post.forum_topic_page > 1
     respond_with(@forum_post, :location => forum_topic_path(@forum_post.topic, :page => page, :anchor => "forum_post_#{@forum_post.id}"))
   end
 
   def destroy
-    check_privilege(@forum_post)
+    @forum_post = authorize ForumPost.find(params[:id])
     @forum_post.delete!
     respond_with(@forum_post)
   end
 
   def undelete
-    check_privilege(@forum_post)
+    @forum_post = authorize ForumPost.find(params[:id])
     @forum_post.undelete!
     respond_with(@forum_post)
   end
-
-  private
-
-  def load_post
-    @forum_post = ForumPost.find(params[:id])
-    @forum_topic = @forum_post.topic
-  end
-
-  def check_min_level
-    raise User::PrivilegeError if CurrentUser.user.level < @forum_topic.min_level
-  end
-
-  def check_privilege(forum_post)
-    if !forum_post.editable_by?(CurrentUser.user)
-      raise User::PrivilegeError
-    end
-  end
-
-  def forum_post_params(context)
-    permitted_params = [:body]
-    permitted_params += [:topic_id] if context == :create
-
-    params.require(:forum_post).permit(permitted_params)
-  end
 end

app/controllers/forum_topics_controller.rb

@@ -1,20 +1,17 @@
 class ForumTopicsController < ApplicationController
   respond_to :html, :xml, :json
   respond_to :atom, only: [:index, :show]
-  before_action :member_only, :except => [:index, :show]
   before_action :normalize_search, :only => :index
-  before_action :load_topic, :only => [:edit, :show, :update, :destroy, :undelete]
-  before_action :check_min_level, :only => [:show, :edit, :update, :destroy, :undelete]
   skip_before_action :api_check
 
   def new
-    @forum_topic = ForumTopic.new
+    @forum_topic = authorize ForumTopic.new
     @forum_topic.original_post = ForumPost.new
     respond_with(@forum_topic)
   end
 
   def edit
-    check_privilege(@forum_topic)
+    @forum_topic = authorize ForumTopic.find(params[:id])
     respond_with(@forum_topic)
   end
 
@@ -35,11 +32,13 @@ class ForumTopicsController < ApplicationController
   end
 
   def show
+    @forum_topic = authorize ForumTopic.find(params[:id])
+
     if request.format.html?
       @forum_topic.mark_as_read!(CurrentUser.user)
     end
 
-    @forum_posts = ForumPost.search(:topic_id => @forum_topic.id).reorder("forum_posts.id").paginate(params[:page])
+    @forum_posts = @forum_topic.forum_posts.order(id: :asc).paginate(params[:page], limit: params[:limit])
 
     if request.format.atom?
       @forum_posts = @forum_posts.reverse_order.load
@@ -51,7 +50,7 @@ class ForumTopicsController < ApplicationController
   end
 
   def create
-    @forum_topic = ForumTopic.new(forum_topic_params(:create))
+    @forum_topic = authorize ForumTopic.new(permitted_attributes(ForumTopic))
     @forum_topic.creator = CurrentUser.user
     @forum_topic.original_post.creator = CurrentUser.user
     @forum_topic.save
@@ -60,13 +59,13 @@ class ForumTopicsController < ApplicationController
   end
 
   def update
-    check_privilege(@forum_topic)
+    @forum_topic = authorize ForumTopic.find(params[:id])
-    @forum_topic.update(forum_topic_params(:update))
+    @forum_topic.update(permitted_attributes(@forum_topic))
     respond_with(@forum_topic)
   end
 
   def destroy
-    check_privilege(@forum_topic)
+    @forum_topic = authorize ForumTopic.find(params[:id])
     @forum_topic.update(is_deleted: true)
     @forum_topic.create_mod_action_for_delete
     flash[:notice] = "Topic deleted"
@@ -74,7 +73,7 @@ class ForumTopicsController < ApplicationController
   end
 
   def undelete
-    check_privilege(@forum_topic)
+    @forum_topic = authorize ForumTopic.find(params[:id])
     @forum_topic.update(is_deleted: false)
     @forum_topic.create_mod_action_for_undelete
     flash[:notice] = "Topic undeleted"
@@ -82,6 +81,7 @@ class ForumTopicsController < ApplicationController
   end
 
   def mark_all_as_read
+    authorize ForumTopic
     CurrentUser.user.update_attribute(:last_forum_read_at, Time.now)
     ForumTopicVisit.prune!(CurrentUser.user)
     redirect_to forum_topics_path, :notice => "All topics marked as read"
@@ -100,25 +100,4 @@ class ForumTopicsController < ApplicationController
       params[:search][:title] = params.delete(:title)
     end
   end
-
-  def check_privilege(forum_topic)
-    if !forum_topic.editable_by?(CurrentUser.user)
-      raise User::PrivilegeError
-    end
-  end
-
-  def load_topic
-    @forum_topic = ForumTopic.find(params[:id])
-  end
-
-  def check_min_level
-    raise User::PrivilegeError if CurrentUser.user.level < @forum_topic.min_level
-  end
-
-  def forum_topic_params(context)
-    permitted_params = [:title, :category_id, { original_post_attributes: %i[id body] }]
-    permitted_params += %i[is_sticky is_locked min_level] if CurrentUser.is_moderator?
-
-    params.require(:forum_topic).permit(permitted_params)
-  end
 end

app/logical/forum_updater.rb

@@ -19,7 +19,7 @@ class ForumUpdater
   end
 
   def create_response(body)
-    forum_topic.posts.create(body: body, skip_mention_notifications: true, creator: User.system)
+    forum_topic.forum_posts.create(body: body, skip_mention_notifications: true, creator: User.system)
   end
 
   def update_post(body)

app/models/bulk_update_request.rb

@@ -63,7 +63,7 @@ class BulkUpdateRequest < ApplicationRecord
     def forum_updater
       @forum_updater ||= begin
         post = if forum_topic
-          forum_post || forum_topic.posts.first
+          forum_post || forum_topic.forum_posts.first
         else
           nil
         end
@@ -89,7 +89,7 @@ class BulkUpdateRequest < ApplicationRecord
     def create_forum_topic
       CurrentUser.as(user) do
         self.forum_topic = ForumTopic.create(title: title, category_id: 1, creator: user) unless forum_topic.present?
-        self.forum_post = forum_topic.posts.create(body: reason_with_link, creator: user) unless forum_post.present?
+        self.forum_post = forum_topic.forum_posts.create(body: reason_with_link, creator: user) unless forum_post.present?
         save
       end
     end

app/models/forum_post.rb

@@ -2,7 +2,7 @@ class ForumPost < ApplicationRecord
   attr_readonly :topic_id
   belongs_to :creator, class_name: "User"
   belongs_to_updater
-  belongs_to :topic, :class_name => "ForumTopic"
+  belongs_to :topic, class_name: "ForumTopic", inverse_of: :forum_posts
   has_many :dtext_links, as: :model, dependent: :destroy
   has_many :moderation_reports, as: :model
   has_many :votes, class_name: "ForumPostVote"
@@ -16,8 +16,6 @@ class ForumPost < ApplicationRecord
   after_update :update_topic_updated_at_on_update_for_original_posts
   after_destroy :update_topic_updated_at_on_destroy
   validates_presence_of :body
-  validate :validate_topic_is_unlocked
-  validate :topic_is_not_restricted, :on => :create
   after_save :delete_topic_if_original_post
   after_update(:if => ->(rec) {rec.updater_id != rec.creator_id}) do |rec|
     ModAction.log("#{CurrentUser.name} updated forum ##{rec.id}", :forum_post_update)
@@ -101,24 +99,8 @@ class ForumPost < ApplicationRecord
     end
   end
 
-  def validate_topic_is_unlocked
-    if topic.is_locked? && !updater.is_moderator?
-      errors[:topic] << "is locked"
-    end
-  end
-
-  def topic_is_not_restricted
-    if topic && !topic.visible?(creator)
-      errors[:topic] << "is restricted"
-    end
-  end
-
-  def editable_by?(user)
-    (creator_id == user.id || user.is_moderator?) && visible?(user)
-  end
-
   def visible?(user, show_deleted_posts = false)
-    user.is_moderator? || (topic.visible?(user) && (show_deleted_posts || !is_deleted?))
+    user.is_moderator? || (user.level >= topic.min_level && (show_deleted_posts || !is_deleted?))
   end
 
   def update_topic_updated_at_on_create

app/models/forum_topic.rb

@@ -13,11 +13,11 @@ class ForumTopic < ApplicationRecord
 
   belongs_to :creator, class_name: "User"
   belongs_to_updater
-  has_many :posts, -> {order("forum_posts.id asc")}, :class_name => "ForumPost", :foreign_key => "topic_id", :dependent => :destroy
+  has_many :forum_posts, foreign_key: "topic_id", dependent: :destroy, inverse_of: :topic
   has_many :forum_topic_visits
   has_one :forum_topic_visit_by_current_user, -> { where(user_id: CurrentUser.id) }, class_name: "ForumTopicVisit"
-  has_many :moderation_reports, through: :posts
+  has_many :moderation_reports, through: :forum_posts
-  has_one :original_post, -> {order("forum_posts.id asc")}, class_name: "ForumPost", foreign_key: "topic_id", inverse_of: :topic
+  has_one :original_post, -> { order(id: :asc) }, class_name: "ForumPost", foreign_key: "topic_id", inverse_of: :topic
   has_many :bulk_update_requests, :foreign_key => "forum_topic_id"
 
   validates_presence_of :title
@@ -147,14 +147,6 @@ class ForumTopic < ApplicationRecord
   include CategoryMethods
   include VisitMethods
 
-  def editable_by?(user)
-    (creator_id == user.id || user.is_moderator?) && visible?(user)
-  end
-
-  def visible?(user)
-    user.level >= min_level
-  end
-
   # XXX forum_topic_visit_by_current_user is a hack to reduce queries on the forum index.
   def is_read?
     return true if CurrentUser.is_anonymous?
@@ -179,7 +171,7 @@ class ForumTopic < ApplicationRecord
   end
 
   def page_for(post_id)
-    (posts.where("id < ?", post_id).count / Danbooru.config.posts_per_page.to_f).ceil
+    (forum_posts.where("id < ?", post_id).count / Danbooru.config.posts_per_page.to_f).ceil
   end
 
   def last_page

app/policies/forum_post_policy.rb

@@ -0,0 +1,33 @@
+class ForumPostPolicy < ApplicationPolicy
+  def show?
+    user.level >= record.topic.min_level
+  end
+
+  def create?
+    unbanned? && policy(record.topic).reply?
+  end
+
+  def update?
+    unbanned? && show? && (user.is_moderator? || (record.creator_id == user.id && !record.topic.is_locked?))
+  end
+
+  def destroy?
+    unbanned? && show? && user.is_moderator?
+  end
+
+  def undelete?
+    unbanned? && show? && user.is_moderator?
+  end
+
+  def show_deleted?
+    !record.is_deleted? || user.is_moderator?
+  end
+
+  def permitted_attributes_for_create
+    [:body, :topic_id]
+  end
+
+  def permitted_attributes_for_update
+    [:body]
+  end
+end

app/policies/forum_topic_policy.rb

@@ -0,0 +1,36 @@
+class ForumTopicPolicy < ApplicationPolicy
+  def show?
+    user.level >= record.min_level
+  end
+
+  def update?
+    unbanned? && show? && (user.is_moderator? || (record.creator_id == user.id && !record.is_locked?))
+  end
+
+  def destroy?
+    unbanned? && show? && user.is_moderator?
+  end
+
+  def undelete?
+    unbanned? && show? && user.is_moderator?
+  end
+
+  def mark_all_as_read?
+    user.is_member?
+  end
+
+  def reply?
+    unbanned? && show? && (user.is_moderator? || !record.is_locked?)
+  end
+
+  def moderate?
+    user.is_moderator?
+  end
+
+  def permitted_attributes
+    [
+      :title, :category_id, { original_post_attributes: [:id, :body] },
+      ([:is_sticky, :is_locked, :min_level] if moderate?)
+    ].compact.flatten
+  end
+end

app/views/forum_posts/_forum_post.html.erb

@@ -1,4 +1,4 @@
-<% if forum_post.visible?(CurrentUser.user, ActiveModel::Type::Boolean.new.cast(params.dig(:search, :is_deleted))) %>
+<% if policy(forum_post).show_deleted? %>
   <article class="forum-post message" id="forum_post_<%= forum_post.id %>" 
            data-forum-post-id="<%= forum_post.id %>"
            <% if CurrentUser.is_moderator? %>
@@ -20,17 +20,17 @@
       </div>
       <%= render "application/update_notice", record: forum_post %>
       <menu>
-        <% if CurrentUser.is_member? && @forum_topic %>
+        <% if policy(forum_post).create? %>
           <li><%= link_to "Reply", new_forum_post_path(:post_id => forum_post.id), :method => :get, :remote => true %></li>
         <% end %>
-        <% if CurrentUser.is_moderator? && !forum_post.is_original_post?(original_forum_post_id) %>
+        <% if policy(forum_post).destroy? && !forum_post.is_original_post?(original_forum_post_id) %>
           <% if forum_post.is_deleted %>
             <li><%= link_to "Undelete", undelete_forum_post_path(forum_post.id), :method => :post, :remote => true %></li>
           <% else %>
             <li><%= link_to "Delete", forum_post_path(forum_post.id), :data => {:confirm => "Are you sure you want to delete this forum post?"}, :method => :delete, :remote => true %></li>
           <% end %>
         <% end %>
-        <% if forum_post.editable_by?(CurrentUser.user) %>
+        <% if policy(forum_post).update? %>
           <% if forum_post.is_original_post?(original_forum_post_id) %>
             <li><%= link_to "Edit", edit_forum_topic_path(forum_post.topic), :id => "edit_forum_topic_link_#{forum_post.topic.id}", :class => "edit_forum_topic_link" %></li>
           <% else %>
@@ -46,7 +46,7 @@
           </ul>
         <% end %>
       </menu>
-      <% if forum_post.editable_by?(CurrentUser.user) %>
+      <% if policy(forum_post).update? %>
         <% if forum_post.is_original_post?(original_forum_post_id) %>
           <%= render "forum_topics/form", :forum_topic => forum_post.topic %>
         <% else %>

app/views/forum_posts/new.html.erb

@@ -1,7 +1,7 @@
 <div id="c-forum-posts">
   <div id="a-new">
-    <% if @forum_topic %>
+    <% if @forum_post.topic.present? %>
-      <h1>Reply to <%= @forum_topic.title %></h1>
+      <h1>Reply to <%= @forum_post.topic.title %></h1>
     <% else %>
       <h1>New Forum Post</h1>
     <% end %>

app/views/forum_posts/partials/new/_form.html.erb

@@ -1,7 +1,7 @@
 <%= error_messages_for("forum_post") %>
 
 <%= edit_form_for(forum_post) do |f| %>
-  <% if @forum_topic %>
+  <% if forum_post.topic_id.present? %>
     <%= f.input :topic_id, :as => :hidden %>
   <% else %>
     <%= f.input :topic_id, :label => "Topic ID" %>

app/views/forum_topics/_form.html.erb

@@ -13,7 +13,7 @@
       <%= dtext_field "forum_post", "body", :classes => "autocomplete-mentions", :input_name => "forum_topic[original_post_attributes][body]", :value => forum_topic.original_post.body, :input_id => "forum_post_body_for_#{forum_topic.original_post.id}", :preview_id => "dtext-preview-for-#{forum_topic.original_post.id}" %>
     <% end %>
 
-    <% if CurrentUser.is_moderator? %>
+    <% if policy(forum_topic).moderate? %>
       <%= f.input :min_level, :include_blank => false, :collection => available_min_user_levels %>
       <%= f.input :is_sticky, :label => "Sticky" %>
       <%= f.input :is_locked, :label => "Locked" %>

app/views/forum_topics/_secondary_links.html.erb

@@ -18,9 +18,9 @@
   <% if CurrentUser.is_member? && @forum_topic && !@forum_topic.new_record? %>
     <li>|</li>
     <%= subnav_link_to "Reply", new_forum_post_path(:topic_id => @forum_topic.id) %>
-    <% if !@forum_topic.new_record? && @forum_topic.editable_by?(CurrentUser.user) %>
+    <% if !@forum_topic.new_record? && policy(@forum_topic).update? %>
       <%= subnav_link_to "Edit", edit_forum_topic_path(@forum_topic), "data-shortcut": "e" %>
-      <% if CurrentUser.is_moderator? %>
+      <% if policy(@forum_topic).destroy? # XXX %>
         <% if @forum_topic.is_deleted? %>
           <%= subnav_link_to "Undelete", undelete_forum_topic_path(@forum_topic), :method => :post %>
         <% else %>

app/views/forum_topics/index.html.erb

@@ -11,7 +11,7 @@
       Categories:
       <%= link_to "All", forum_topics_path %>, 
       <%= link_to "New", forum_topics_path(search: { is_read: false }) %>,
-      <% if CurrentUser.is_moderator? %>
+      <% if policy(ForumTopic).moderate? %>
         <%= link_to "Private", forum_topics_path(search: { is_private: true }) %>,
       <% end %>
       <%= ForumTopic::CATEGORIES.map {|id, name| link_to_unless_current(name, forum_topics_path(:search => {:category_id => id}))}.join(", ").html_safe %>

app/views/forum_topics/show.html.erb

@@ -28,14 +28,12 @@
 
     <%= render "forum_posts/listing", forum_posts: @forum_posts, original_forum_post_id: @forum_topic.original_post&.id, dtext_data: DText.preprocess(@forum_posts.map(&:body)), moderation_reports: @forum_topic.moderation_reports.visible.recent %>
 
-    <% if CurrentUser.is_member? %>
+    <% if policy(ForumPost.new(topic: @forum_topic)).create? %>
-      <% if CurrentUser.is_moderator? || !@forum_topic.is_locked? %>
+      <p><%= link_to "Post reply", new_forum_post_path(topic_id: @forum_topic.id), id: "new-response-link" %></p>
-        <p><%= link_to "Post reply", new_forum_post_path(topic_id: @forum_topic.id), id: "new-response-link" %></p>
 
-        <div style="display: none;" id="topic-response">
+      <div style="display: none;" id="topic-response">
-          <%= render "forum_posts/partials/new/form", :forum_post => ForumPost.new(:topic_id => @forum_topic.id) %>
+        <%= render "forum_posts/partials/new/form", forum_post: ForumPost.new(topic_id: @forum_topic.id) %>
-        </div>
+      </div>
-      <% end %>
     <% end %>
 
     <%= numbered_paginator(@forum_posts) %>

test/functional/forum_posts_controller_test.rb

@@ -21,11 +21,13 @@ class ForumPostsControllerTest < ActionDispatch::IntegrationTest
 
       should "render the vote links" do
         get_auth forum_topic_path(@forum_topic), @mod
+        assert_response :success
         assert_select "a[title='Vote up']"
       end
 
       should "render existing votes" do
         get_auth forum_topic_path(@forum_topic), @mod
+        assert_response :success
         assert_select "li.vote-score-up"
       end
 
@@ -39,10 +41,12 @@ class ForumPostsControllerTest < ActionDispatch::IntegrationTest
 
         should "hide the vote links" do
           assert_select "a[title='Vote up']", false
+          assert_response :success
         end
 
         should "still render existing votes" do
           assert_select "li.vote-score-up"
+          assert_response :success
         end
       end
     end
@@ -73,28 +77,38 @@ class ForumPostsControllerTest < ActionDispatch::IntegrationTest
         end
       end
 
-      context "with private topics" do
+      context "for posts in private topics" do
         setup do
-          as(@mod) do
+          @admin = create(:admin_user)
-            @mod_topic = create(:mod_up_forum_topic, creator: @mod)
+          @mod_post = as(@mod) { create(:forum_post, creator: @mod, topic: build(:forum_topic, min_level: User::Levels::MODERATOR)) }
-            @mod_posts = create_list(:forum_post, 2, topic: @mod_topic, creator: @mod)
+          @admin_post = as(@admin) { create(:forum_post, creator: @admin, topic: build(:forum_topic, min_level: User::Levels::ADMIN)) }
-          end
-          @mod_post_ids = ([@forum_post] + @mod_posts).map(&:id).reverse
         end
 
-        should "list only permitted posts for members" do
+        should "list only permitted posts for anons" do
           get forum_posts_path
 
           assert_response :success
           assert_select "#forum-post-#{@forum_post.id}"
-          assert_select "#forum-post-#{@mod_posts[0].id}", false
+          assert_select "#forum-post-#{@mod_post.id}", false
+          assert_select "#forum-post-#{@admin_post.id}", false
         end
 
         should "list only permitted posts for mods" do
           get_auth forum_posts_path, @mod
 
           assert_response :success
-          assert_select "#forum-post-#{@mod_posts[0].id}"
+          assert_select "#forum-post-#{@forum_post.id}"
+          assert_select "#forum-post-#{@mod_post.id}"
+          assert_select "#forum-post-#{@admin_post.id}", false
+        end
+
+        should "list only permitted posts for admins" do
+          get_auth forum_posts_path, @admin
+
+          assert_response :success
+          assert_select "#forum-post-#{@forum_post.id}"
+          assert_select "#forum-post-#{@mod_post.id}"
+          assert_select "#forum-post-#{@admin_post.id}"
         end
       end
     end
@@ -128,6 +142,12 @@ class ForumPostsControllerTest < ActionDispatch::IntegrationTest
         get_auth edit_forum_post_path(@forum_post), @other_user
         assert_response(403)
       end
+
+      should "fail if the topic is private and the editor is unauthorized" do
+        as(@user) { @forum_post.topic.update(min_level: User::Levels::ADMIN) }
+        get_auth edit_forum_post_path(@forum_post), @user
+        assert_response(403)
+      end
     end
 
     context "new action" do
@@ -135,40 +155,96 @@ class ForumPostsControllerTest < ActionDispatch::IntegrationTest
         get_auth new_forum_post_path, @user, params: {:topic_id => @forum_topic.id}
         assert_response :success
       end
+
+      should "not allow unauthorized users to quote posts in private forum topics" do
+        as(@user) { @forum_post.topic.update(min_level: User::Levels::ADMIN) }
+        get_auth new_forum_post_path, @user, params: { post_id: @forum_post.id }
+
+        assert_response 403
+      end
     end
 
     context "create action" do
       should "create a new forum post" do
         assert_difference("ForumPost.count", 1) do
           post_auth forum_posts_path, @user, params: {:forum_post => {:body => "xaxaxa", :topic_id => @forum_topic.id}}
+          assert_redirected_to(forum_topic_path(@forum_topic))
         end
+      end
 
-        forum_post = ForumPost.last
+      should "not allow unauthorized users to create posts in private topics" do
+        as(@user) { @forum_post.topic.update!(min_level: User::Levels::ADMIN) }
+
+        post_auth forum_posts_path, @user, params: { forum_post: { body: "xaxaxa", topic_id: @forum_topic.id }}
+        assert_response 403
+      end
+
+      should "not allow non-moderators to create posts in locked topics" do
+        as(@user) { @forum_post.topic.update!(is_locked: true) }
+
+        post_auth forum_posts_path, @user, params: { forum_post: { body: "xaxaxa", topic_id: @forum_topic.id }}
+        assert_response 403
+      end
+
+      should "allow moderators to create posts in locked topics" do
+        as(@user) { @forum_post.topic.update!(is_locked: true) }
+
+        post_auth forum_posts_path, @mod, params: { forum_post: { body: "xaxaxa", topic_id: @forum_topic.id }}
         assert_redirected_to(forum_topic_path(@forum_topic))
       end
     end
 
+    context "update action" do
+      should "allow users to update their own posts" do
+        put_auth forum_post_path(@forum_post), @user, params: { forum_post: { body: "test" }}
+        assert_redirected_to(forum_topic_path(@forum_topic, anchor: "forum_post_#{@forum_post.id}"))
+      end
+
+      should "not allow users to update their own posts in locked topics" do
+        as(@user) { @forum_post.topic.update!(is_locked: true) }
+
+        put_auth forum_post_path(@forum_post), @user, params: { forum_post: { body: "test" }}
+        assert_response 403
+      end
+
+      should "not allow users to update other people's posts" do
+        put_auth forum_post_path(@forum_post), @other_user, params: { forum_post: { body: "test" }}
+        assert_response 403
+      end
+
+      should "allow moderators to update other people's posts" do
+        put_auth forum_post_path(@forum_post), @mod, params: { forum_post: { body: "test" }}
+        assert_redirected_to(forum_topic_path(@forum_topic, anchor: "forum_post_#{@forum_post.id}"))
+      end
+    end
+
     context "destroy action" do
-      should "destroy the posts" do
+      should "allow mods to delete posts" do
         delete_auth forum_post_path(@forum_post), @mod
         assert_redirected_to(forum_post_path(@forum_post))
-        @forum_post.reload
+        assert_equal(true, @forum_post.reload.is_deleted?)
-        assert_equal(true, @forum_post.is_deleted?)
+      end
+
+      should "not allow users to delete their own posts" do
+        delete_auth forum_post_path(@forum_post), @user
+        assert_response 403
+        assert_equal(false, @forum_post.reload.is_deleted?)
       end
     end
 
     context "undelete action" do
-      setup do
+      should "allow mods to undelete posts" do
-        as(@mod) do
+        as(@mod) { @forum_post.update!(is_deleted: true) }
-          @forum_post.update(is_deleted: true)
-        end
-      end
-
-      should "restore the post" do
         post_auth undelete_forum_post_path(@forum_post), @mod
         assert_redirected_to(forum_post_path(@forum_post))
-        @forum_post.reload
+        assert_equal(false, @forum_post.reload.is_deleted?)
-        assert_equal(false, @forum_post.is_deleted?)
+      end
+
+      should "not allow users to undelete their own posts" do
+        as(@mod) { @forum_post.update!(is_deleted: true) }
+        post_auth undelete_forum_post_path(@forum_post), @user
+        assert_response 403
+        assert_equal(true, @forum_post.reload.is_deleted?)
       end
     end
   end

test/functional/forum_topics_controller_test.rb

@@ -95,9 +95,12 @@ class ForumTopicsControllerTest < ActionDispatch::IntegrationTest
         end
       end
 
-      should "list all forum topics" do
+      should "list public forum topics for members" do
         get forum_topics_path
+
         assert_response :success
+        assert_select "a.forum-post-link", count: 1, text: @topic1.title
+        assert_select "a.forum-post-link", count: 1, text: @topic2.title
       end
 
       should "not list stickied topics first for JSON responses" do
@@ -111,6 +114,26 @@ class ForumTopicsControllerTest < ActionDispatch::IntegrationTest
         assert_response :success
       end
 
+      context "with private topics" do
+        should "not show private topics to unprivileged users" do
+          as(@user) { @topic2.update!(min_level: User::Levels::MODERATOR) }
+          get forum_topics_path
+
+          assert_response :success
+          assert_select "a.forum-post-link", count: 1, text: @topic1.title
+          assert_select "a.forum-post-link", count: 0, text: @topic2.title
+        end
+
+        should "show private topics to privileged users" do
+          as(@user) { @topic2.update!(min_level: User::Levels::MODERATOR) }
+          get_auth forum_topics_path, @mod
+
+          assert_response :success
+          assert_select "a.forum-post-link", count: 1, text: @topic1.title
+          assert_select "a.forum-post-link", count: 1, text: @topic2.title
+        end
+      end
+
       context "with search conditions" do
         should "list all matching forum topics" do
           get forum_topics_path, params: {:search => {:title_matches => "forum"}}
@@ -217,6 +240,29 @@ class ForumTopicsControllerTest < ActionDispatch::IntegrationTest
         assert_redirected_to forum_topic_path(@forum_topic)
         assert_equal(true, @forum_topic.reload.is_locked)
       end
+
+      should "allow users to update their own topics" do
+        put_auth forum_topic_path(@forum_topic), @user, params: { forum_topic: { title: "test" }}
+
+        assert_redirected_to forum_topic_path(@forum_topic)
+        assert_equal("test", @forum_topic.reload.title)
+      end
+
+      should "not allow users to update locked topics" do
+        as(@mod) { @forum_topic.update!(is_locked: true) }
+        put_auth forum_topic_path(@forum_topic), @user, params: { forum_topic: { title: "test" }}
+
+        assert_response 403
+        assert_not_equal("test", @forum_topic.reload.title)
+      end
+
+      should "allow mods to update locked topics" do
+        as(@mod) { @forum_topic.update!(is_locked: true) }
+        put_auth forum_topic_path(@forum_topic), @mod, params: { forum_topic: { title: "test" }}
+
+        assert_redirected_to forum_topic_path(@forum_topic)
+        assert_equal("test", @forum_topic.reload.title)
+      end
     end
 
     context "destroy action" do

test/unit/forum_post_test.rb

@@ -102,25 +102,6 @@ class ForumPostTest < ActiveSupport::TestCase
       end
     end
 
-    context "belonging to a locked topic" do
-      setup do
-        @post = create(:forum_post, topic: @topic, body: "zzz")
-        @topic.update(is_locked: true)
-      end
-
-      should "not be updateable" do
-        @post.update(body: "xxx")
-        assert_equal(true, @post.invalid?)
-        assert_equal("zzz", @post.reload.body)
-      end
-
-      should "not be deletable" do
-        @post.delete!
-        assert_equal(true, @post.invalid?)
-        assert_equal(false, @post.reload.is_deleted)
-      end
-    end
-
     should "update the topic when created" do
       @original_topic_updated_at = @topic.updated_at
       travel(1.second) do