From db63b6d44fe893e29b5519e58ea110c847c23a42 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 16 Mar 2020 02:42:54 -0500 Subject: [PATCH] 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 | 53 ++------ app/controllers/forum_topics_controller.rb | 43 ++----- app/logical/forum_updater.rb | 2 +- app/models/bulk_update_request.rb | 4 +- app/models/forum_post.rb | 22 +--- app/models/forum_topic.rb | 16 +-- app/policies/forum_post_policy.rb | 33 +++++ app/policies/forum_topic_policy.rb | 36 ++++++ app/views/forum_posts/_forum_post.html.erb | 10 +- app/views/forum_posts/new.html.erb | 4 +- .../forum_posts/partials/new/_form.html.erb | 2 +- app/views/forum_topics/_form.html.erb | 2 +- .../forum_topics/_secondary_links.html.erb | 4 +- app/views/forum_topics/index.html.erb | 2 +- app/views/forum_topics/show.html.erb | 12 +- .../functional/forum_posts_controller_test.rb | 120 ++++++++++++++---- .../forum_topics_controller_test.rb | 48 ++++++- test/unit/forum_post_test.rb | 19 --- 18 files changed, 262 insertions(+), 170 deletions(-) create mode 100644 app/policies/forum_post_policy.rb create mode 100644 app/policies/forum_topic_policy.rb diff --git a/app/controllers/forum_posts_controller.rb b/app/controllers/forum_posts_controller.rb index c99148783..df230463d 100644 --- a/app/controllers/forum_posts_controller.rb +++ b/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_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) + @forum_post = authorize 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.update(forum_post_params(:update)) + @forum_post = authorize ForumPost.find(params[:id]) + @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 diff --git a/app/controllers/forum_topics_controller.rb b/app/controllers/forum_topics_controller.rb index 5c226d59d..82d66f722 100644 --- a/app/controllers/forum_topics_controller.rb +++ b/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.update(forum_topic_params(:update)) + @forum_topic = authorize ForumTopic.find(params[:id]) + @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 diff --git a/app/logical/forum_updater.rb b/app/logical/forum_updater.rb index b5d7bcd5b..6ea465183 100644 --- a/app/logical/forum_updater.rb +++ b/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) diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index c7d77d458..5e10284f0 100644 --- a/app/models/bulk_update_request.rb +++ b/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 diff --git a/app/models/forum_post.rb b/app/models/forum_post.rb index ac7e8dbd4..0bcb3397a 100644 --- a/app/models/forum_post.rb +++ b/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 diff --git a/app/models/forum_topic.rb b/app/models/forum_topic.rb index 4c7c054c4..22858c7f8 100644 --- a/app/models/forum_topic.rb +++ b/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_one :original_post, -> {order("forum_posts.id asc")}, class_name: "ForumPost", foreign_key: "topic_id", inverse_of: :topic + has_many :moderation_reports, through: :forum_posts + 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 diff --git a/app/policies/forum_post_policy.rb b/app/policies/forum_post_policy.rb new file mode 100644 index 000000000..34787cef3 --- /dev/null +++ b/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 diff --git a/app/policies/forum_topic_policy.rb b/app/policies/forum_topic_policy.rb new file mode 100644 index 000000000..43b96fdcb --- /dev/null +++ b/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 diff --git a/app/views/forum_posts/_forum_post.html.erb b/app/views/forum_posts/_forum_post.html.erb index 6b22ef4cf..617ddd952 100644 --- a/app/views/forum_posts/_forum_post.html.erb +++ b/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? %>
@@ -20,17 +20,17 @@ <%= render "application/update_notice", record: forum_post %> - <% if CurrentUser.is_member? && @forum_topic %> + <% if policy(forum_post).create? %>
  • <%= link_to "Reply", new_forum_post_path(:post_id => forum_post.id), :method => :get, :remote => true %>
    • <% 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 %>
  • <%= link_to "Undelete", undelete_forum_post_path(forum_post.id), :method => :post, :remote => true %>
    • <% else %>
  • <%= 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 %>
    • <% end %> <% end %> - <% if forum_post.editable_by?(CurrentUser.user) %> + <% if policy(forum_post).update? %> <% if forum_post.is_original_post?(original_forum_post_id) %>
  • <%= link_to "Edit", edit_forum_topic_path(forum_post.topic), :id => "edit_forum_topic_link_#{forum_post.topic.id}", :class => "edit_forum_topic_link" %>
    • <% else %> @@ -46,7 +46,7 @@ <% end %>     - <% 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 %> diff --git a/app/views/forum_posts/new.html.erb b/app/views/forum_posts/new.html.erb index 5cb4bc67a..02264b158 100644 --- a/app/views/forum_posts/new.html.erb +++ b/app/views/forum_posts/new.html.erb @@ -1,7 +1,7 @@
    - <% if @forum_topic %> -

    Reply to <%= @forum_topic.title %>

    + <% if @forum_post.topic.present? %> +

    Reply to <%= @forum_post.topic.title %>

    <% else %>

    New Forum Post

    <% end %> diff --git a/app/views/forum_posts/partials/new/_form.html.erb b/app/views/forum_posts/partials/new/_form.html.erb index 874910c4f..622d01056 100644 --- a/app/views/forum_posts/partials/new/_form.html.erb +++ b/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" %> diff --git a/app/views/forum_topics/_form.html.erb b/app/views/forum_topics/_form.html.erb index a2ec14166..f4e78c493 100644 --- a/app/views/forum_topics/_form.html.erb +++ b/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" %> diff --git a/app/views/forum_topics/_secondary_links.html.erb b/app/views/forum_topics/_secondary_links.html.erb index 135b9edf4..1e95d9bee 100644 --- a/app/views/forum_topics/_secondary_links.html.erb +++ b/app/views/forum_topics/_secondary_links.html.erb @@ -18,9 +18,9 @@ <% if CurrentUser.is_member? && @forum_topic && !@forum_topic.new_record? %>
  • |
    • <%= 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 %> diff --git a/app/views/forum_topics/index.html.erb b/app/views/forum_topics/index.html.erb index ba03d71f8..eeb50a9b8 100644 --- a/app/views/forum_topics/index.html.erb +++ b/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 %> diff --git a/app/views/forum_topics/show.html.erb b/app/views/forum_topics/show.html.erb index 001a39068..0ba9ca117 100644 --- a/app/views/forum_topics/show.html.erb +++ b/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 CurrentUser.is_moderator? || !@forum_topic.is_locked? %> -

    <%= link_to "Post reply", new_forum_post_path(topic_id: @forum_topic.id), id: "new-response-link" %>

    + <% if policy(ForumPost.new(topic: @forum_topic)).create? %> +

    <%= link_to "Post reply", new_forum_post_path(topic_id: @forum_topic.id), id: "new-response-link" %>

    - - <% end %> + <% end %> <%= numbered_paginator(@forum_posts) %> diff --git a/test/functional/forum_posts_controller_test.rb b/test/functional/forum_posts_controller_test.rb index 0dda30fda..c76c7c660 100644 --- a/test/functional/forum_posts_controller_test.rb +++ b/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 - @mod_topic = create(:mod_up_forum_topic, creator: @mod) - @mod_posts = create_list(:forum_post, 2, topic: @mod_topic, creator: @mod) - end - @mod_post_ids = ([@forum_post] + @mod_posts).map(&:id).reverse + @admin = create(:admin_user) + @mod_post = as(@mod) { create(:forum_post, creator: @mod, topic: build(:forum_topic, min_level: User::Levels::MODERATOR)) } + @admin_post = as(@admin) { create(:forum_post, creator: @admin, topic: build(:forum_topic, min_level: User::Levels::ADMIN)) } 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.is_deleted?) + assert_equal(true, @forum_post.reload.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 - as(@mod) do - @forum_post.update(is_deleted: true) - end - end - - should "restore the post" do + should "allow mods to undelete posts" do + as(@mod) { @forum_post.update!(is_deleted: true) } 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.is_deleted?) + assert_equal(false, @forum_post.reload.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 diff --git a/test/functional/forum_topics_controller_test.rb b/test/functional/forum_topics_controller_test.rb index 31c3534eb..5c9fc526d 100644 --- a/test/functional/forum_topics_controller_test.rb +++ b/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 diff --git a/test/unit/forum_post_test.rb b/test/unit/forum_post_test.rb index 15b048253..9e7e41c2f 100644 --- a/test/unit/forum_post_test.rb +++ b/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