From a0c4617057695d6a212e50d1d6b22427014056a0 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 15 Mar 2020 15:37:34 -0500 Subject: [PATCH] pundit: convert comments to pundit. --- app/controllers/comments_controller.rb | 44 +++++++------------ app/models/comment.rb | 4 -- app/policies/comment_policy.rb | 19 ++++++++ app/views/comments/_form.html.erb | 6 ++- .../comments/partials/index/_list.html.erb | 2 +- .../comments/partials/show/_comment.html.erb | 6 +-- test/functional/comments_controller_test.rb | 22 +++++++--- 7 files changed, 58 insertions(+), 45 deletions(-) create mode 100644 app/policies/comment_policy.rb diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 6831cf2fa..5bd8ca612 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -1,7 +1,6 @@ class CommentsController < ApplicationController respond_to :html, :xml, :json, :atom respond_to :js, only: [:new, :destroy, :undelete] - before_action :member_only, :except => [:index, :search, :show] skip_before_action :api_check def index @@ -20,20 +19,25 @@ class CommentsController < ApplicationController end def new - @comment = Comment.new(comment_params(:create)) - @comment.body = Comment.find(params[:id]).quoted_response if params[:id] + if params[:id] + quoted_comment = Comment.find(params[:id]) + @comment = authorize Comment.new(post_id: quoted_comment.post_id, body: quoted_comment.quoted_response) + else + @comment = authorize Comment.new(permitted_attributes(Comment)) + end + respond_with(@comment) end def update - @comment = Comment.find(params[:id]) - check_privilege(@comment) - @comment.update(comment_params(:update)) + @comment = authorize Comment.find(params[:id]) + @comment.update(permitted_attributes(@comment)) respond_with(@comment, :location => post_path(@comment.post_id)) end def create - @comment = Comment.create(comment_params(:create).merge(creator: CurrentUser.user, creator_ip_addr: CurrentUser.ip_addr)) + @comment = authorize Comment.new(creator: CurrentUser.user, creator_ip_addr: CurrentUser.ip_addr) + @comment.update(permitted_attributes(@comment)) flash[:notice] = @comment.valid? ? "Comment posted" : @comment.errors.full_messages.join("; ") respond_with(@comment) do |format| format.html do @@ -43,13 +47,12 @@ class CommentsController < ApplicationController end def edit - @comment = Comment.find(params[:id]) - check_privilege(@comment) + @comment = authorize Comment.find(params[:id]) respond_with(@comment) end def show - @comment = Comment.find(params[:id]) + @comment = authorize Comment.find(params[:id]) respond_with(@comment) do |format| format.html do @@ -59,15 +62,13 @@ class CommentsController < ApplicationController end def destroy - @comment = Comment.find(params[:id]) - check_privilege(@comment) + @comment = authorize Comment.find(params[:id]) @comment.update(is_deleted: true) respond_with(@comment) end def undelete - @comment = Comment.find(params[:id]) - check_privilege(@comment) + @comment = authorize Comment.find(params[:id]) @comment.update(is_deleted: false) respond_with(@comment) end @@ -103,19 +104,4 @@ class CommentsController < ApplicationController respond_with(@comments) end - - def check_privilege(comment) - if !comment.editable_by?(CurrentUser.user) - raise User::PrivilegeError - end - end - - def comment_params(context) - permitted_params = %i[body post_id] - permitted_params += %i[do_not_bump_post] if context == :create - permitted_params += %i[is_deleted] if context == :update - permitted_params += %i[is_sticky] if CurrentUser.is_moderator? - - params.fetch(:comment, {}).permit(permitted_params) - end end diff --git a/app/models/comment.rb b/app/models/comment.rb index 013ecd7f1..ea4b36e9e 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -117,10 +117,6 @@ class Comment < ApplicationRecord end end - def editable_by?(user) - updater_id == user.id || user.is_moderator? - end - def reportable_by?(user) creator_id != user.id && !creator.is_moderator? end diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb new file mode 100644 index 000000000..cfa1f55ed --- /dev/null +++ b/app/policies/comment_policy.rb @@ -0,0 +1,19 @@ +class CommentPolicy < ApplicationPolicy + def update? + unbanned? && (user.is_moderator? || record.updater_id == user.id) + end + + def can_sticky_comment? + user.is_moderator? + end + + def permitted_attributes_for_create + [:body, :post_id, :do_not_bump_post, (:is_sticky if can_sticky_comment?)].compact + end + + def permitted_attributes_for_update + [:body, :is_deleted, (:is_sticky if can_sticky_comment?)].compact + end + + alias_method :undelete?, :update? +end diff --git a/app/views/comments/_form.html.erb b/app/views/comments/_form.html.erb index 36ddea864..300795e97 100644 --- a/app/views/comments/_form.html.erb +++ b/app/views/comments/_form.html.erb @@ -1,14 +1,16 @@ <%= error_messages_for :comment %> <%= edit_form_for(comment, html: { style: ("display: none;" if local_assigns[:hidden]), class: "edit_comment" }) do |f| %> - <%= f.hidden_field :post_id %> + <% if comment.new_record? %> + <%= f.hidden_field :post_id %> + <% end %> <%= dtext_field "comment", "body", :classes => "autocomplete-mentions", :value => comment.body, :input_id => "comment_body_for_#{comment.id}", :preview_id => "dtext-preview-for-#{comment.id}" %> <%= f.button :submit, "Submit" %> <%= dtext_preview_button "comment", "body", :input_id => "comment_body_for_#{comment.id}", :preview_id => "dtext-preview-for-#{comment.id}" %> <% if comment.new_record? %> <%= f.input :do_not_bump_post, :label => "No bump" %> <% end %> - <% if CurrentUser.is_moderator? %> + <% if policy(comment).can_sticky_comment? %> <%= f.input :is_sticky, :label => "Post as moderator", :for => "comment_is_sticky" %> <% end %> <% end %> diff --git a/app/views/comments/partials/index/_list.html.erb b/app/views/comments/partials/index/_list.html.erb index 78a9d7d45..a098389a8 100644 --- a/app/views/comments/partials/index/_list.html.erb +++ b/app/views/comments/partials/index/_list.html.erb @@ -28,7 +28,7 @@ <% end %> - <% if CurrentUser.is_member? %> + <% if policy(Comment).create? %>

<%= link_to "Post comment", new_comment_path(comment: { post_id: post.id }), :class => "expand-comment-response" %>

<%= render "comments/form", comment: post.comments.new, hidden: true %> diff --git a/app/views/comments/partials/show/_comment.html.erb b/app/views/comments/partials/show/_comment.html.erb index a0ff5f2eb..fcaec4e76 100644 --- a/app/views/comments/partials/show/_comment.html.erb +++ b/app/views/comments/partials/show/_comment.html.erb @@ -31,7 +31,7 @@
<%= render "application/update_notice", record: comment %> - <% if CurrentUser.is_member? %> + <% if policy(comment).create? %> <% if context == :index_by_comment %>
  • <%= link_to "Reply", new_comment_path(id: comment, comment: { post_id: comment.post_id }), class: "reply-link" %>
    • @@ -39,7 +39,7 @@
  • <%= link_to "Reply", new_comment_path(id: comment, comment: { post_id: comment.post_id }), class: "reply-link", remote: true %>
    • <% end %> - <% if comment.editable_by?(CurrentUser.user) %> + <% if policy(comment).update? %> <% if comment.is_deleted? %>
  • <%= link_to "Undelete", undelete_comment_path(comment.id), method: :post, remote: true %>
    • <% else %> @@ -60,7 +60,7 @@
  • <%= link_to "Report", new_moderation_report_path(moderation_report: { model_type: "Comment", model_id: comment.id }), remote: true %>
    • <% end %>     - <% if comment.editable_by?(CurrentUser.user) %> + <% if policy(comment).update? %> <%= render "comments/form", comment: comment, hidden: true %> <% end %> <% end %> diff --git a/test/functional/comments_controller_test.rb b/test/functional/comments_controller_test.rb index 4040a4088..a72ffe353 100644 --- a/test/functional/comments_controller_test.rb +++ b/test/functional/comments_controller_test.rb @@ -151,6 +151,7 @@ class CommentsControllerTest < ActionDispatch::IntegrationTest should "fail if updater is not a moderator" do put_auth comment_path(@comment.id), @user, params: {comment: {is_sticky: true}} + assert_response 403 assert_equal(false, @comment.reload.is_sticky) end end @@ -169,20 +170,29 @@ class CommentsControllerTest < ActionDispatch::IntegrationTest end should "not allow changing do_not_bump_post or post_id" do - as_user do - @another_post = create(:post) - end - put_auth comment_path(@comment.id), @comment.creator, params: {do_not_bump_post: true, post_id: @another_post.id} + @another_post = as(@user) { create(:post) } + + put_auth comment_path(@comment.id), @comment.creator, params: { do_not_bump_post: true } + assert_response 403 assert_equal(false, @comment.reload.do_not_bump_post) - assert_equal(@post.id, @comment.post_id) + + put_auth comment_path(@comment.id), @comment.creator, params: { post_id: @another_post.id } + assert_response 403 + assert_equal(@post.id, @comment.reload.post_id) end end context "new action" do - should "redirect" do + should "work" do get_auth new_comment_path, @user assert_response :success end + + should "work when quoting a post" do + @comment = create(:comment) + get_auth new_comment_path(id: @comment.id), @user, as: :javascript + assert_response :success + end end context "create action" do