From 0785720908502dc8c26bd09740a4db93585f54f9 Mon Sep 17 00:00:00 2001 From: Andy Zhao Date: Thu, 21 Jun 2018 17:20:34 -0400 Subject: [PATCH] Add comment policy and specs (#475) * Fix edge case with apostrophes * Add comment policy and specs * Add login for deleting comment spec * Change test to raise pundit error instead of 404 * Clean up comment destroy request specs * Remove redundant raise --- app/controllers/comments_controller.rb | 38 ++++++---------- app/policies/comment_policy.rb | 47 +++++++++++++++++++ spec/policies/comment_policy_spec.rb | 54 ++++++++++++++++++++++ spec/requests/badges_spec.rb | 2 +- spec/requests/comments_destroy_spec.rb | 63 +++++++++++++++++--------- spec/requests/comments_spec.rb | 4 +- 6 files changed, 159 insertions(+), 49 deletions(-) create mode 100644 app/policies/comment_policy.rb create mode 100644 spec/policies/comment_policy_spec.rb diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index a8b0219a5..bc216374a 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -1,12 +1,13 @@ class CommentsController < ApplicationController before_action :set_comment, only: %i[update destroy] before_action :set_cache_control_headers, only: [:index] - before_action :raise_banned, only: %i[create update] before_action :authenticate_user!, only: %i[preview create] + after_action :verify_authorized # GET /comments # GET /comments.json def index + skip_authorization @on_comments_page = true @comment = Comment.new @podcast = Podcast.find_by_slug(params[:username]) @@ -44,7 +45,7 @@ class CommentsController < ApplicationController # GET /comments/1/edit def edit @comment = Comment.find(params[:id_code].to_i(26)) - not_found unless current_user && current_user.id == @comment.user_id + authorize @comment @parent_comment = @comment.parent @commentable = @comment.commentable end @@ -52,8 +53,9 @@ class CommentsController < ApplicationController # POST /comments # POST /comments.json def create + authorize Comment raise if RateLimitChecker.new(current_user).limit_by_situation("comment_creation") - @comment = Comment.new(comment_params) + @comment = Comment.new(permitted_attributes(Comment)) @comment.user_id = current_user.id if @comment.save if params[:checked_code_of_conduct].present? && !current_user.checked_code_of_conduct @@ -97,8 +99,8 @@ class CommentsController < ApplicationController # PATCH/PUT /comments/1 # PATCH/PUT /comments/1.json def update - raise unless @comment.user_id == current_user.id - if @comment.update(comment_update_params.merge(edited_at: DateTime.now)) + authorize @comment + if @comment.update(permitted_attributes(@comment).merge(edited_at: DateTime.now)) Mention.create_all(@comment) redirect_to "#{@comment.commentable.path}/comments/#{@comment.id_code_generated}", notice: "Comment was successfully updated." else @@ -110,7 +112,7 @@ class CommentsController < ApplicationController # DELETE /comments/1 # DELETE /comments/1.json def destroy - raise unless @comment.user_id == current_user.id + authorize @comment @commentable_path = @comment.commentable.path if @comment.is_childless? @comment.destroy @@ -122,17 +124,15 @@ class CommentsController < ApplicationController end def delete_confirm - unless current_user && Comment.where(id: params[:id_code].to_i(26), user_id: current_user.id).first - @comment = Comment.find(params[:id_code].to_i(26)) - redirect_to @comment.path - return - end - @comment = Comment.where(id: params[:id_code].to_i(26), user_id: current_user.id).first + @comment = Comment.find(params[:id_code].to_i(26)) + authorize @comment end def preview + skip_authorization begin - fixed_body_markdown = MarkdownFixer.fix_for_preview(comment_params[:body_markdown]) + permitted_body_markdown = permitted_attributes(Comment)[:body_markdown] + fixed_body_markdown = MarkdownFixer.fix_for_preview(permitted_body_markdown) parsed_markdown = MarkdownParser.new(fixed_body_markdown) processed_html = parsed_markdown.finalize rescue StandardError => e @@ -149,16 +149,4 @@ class CommentsController < ApplicationController def set_comment @comment = Comment.find(params[:id]) end - - # Never trust parameters from the scary internet, only allow the white list through. - def comment_params - params.require(:comment).permit(:body_markdown, - :commentable_id, - :commentable_type, - :parent_id) - end - - def comment_update_params - params.require(:comment).permit(:body_markdown) - end end diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb new file mode 100644 index 000000000..13a10a0ad --- /dev/null +++ b/app/policies/comment_policy.rb @@ -0,0 +1,47 @@ +class CommentPolicy < ApplicationPolicy + def edit? + user_is_author? + end + + def create? + !user_is_banned? + end + + def update? + edit? + end + + def destroy? + edit? + end + + def delete_confirm? + edit? + end + + def preview? + true + end + + def permitted_attributes_for_update + %i[body_markdown] + end + + def permitted_attributes_for_preview + %i[body_markdown] + end + + def permitted_attributes_for_create + %i[body_markdown commentable_id commentable_type parent_id] + end + + private + + def user_is_author? + record.user_id == user.id + end + + def user_is_banned? + user.has_role?(:banned) + end +end diff --git a/spec/policies/comment_policy_spec.rb b/spec/policies/comment_policy_spec.rb new file mode 100644 index 000000000..3ce76db76 --- /dev/null +++ b/spec/policies/comment_policy_spec.rb @@ -0,0 +1,54 @@ +require "rails_helper" + +RSpec.describe CommentPolicy do + subject { described_class.new(user, comment) } + + let(:comment) { build(:comment) } + + let(:valid_attributes_for_create) do + %i[body_markdown commentable_id commentable_type parent_id] + end + + let(:valid_attributes_for_update) do + %i[body_markdown] + end + + context "when user is not signed-in" do + let(:user) { nil } + + it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) } + end + + context "when user is not the author" do + let(:user) { build(:user) } + + it { is_expected.to permit_actions(%i[create]) } + it { is_expected.to forbid_actions(%i[edit update destroy delete_confirm]) } + + it { is_expected.to permit_mass_assignment_of(valid_attributes_for_create).for_action(:create) } + + context "with banned status" do + before { user.add_role :banned } + + it { is_expected.to forbid_actions(%i[create edit update destroy delete_confirm]) } + end + end + + context "when user is the author" do + let(:user) { comment.user } + + it { is_expected.to permit_actions(%i[edit update new create delete_confirm destroy]) } + + it { is_expected.to permit_mass_assignment_of(valid_attributes_for_create).for_action(:create) } + it { is_expected.to permit_mass_assignment_of(valid_attributes_for_update).for_action(:update) } + + context "with banned status" do + before { user.add_role :banned } + + it { is_expected.to permit_actions(%i[edit update destroy delete_confirm]) } + it { is_expected.to forbid_actions(%i[create]) } + + it { is_expected.to permit_mass_assignment_of(valid_attributes_for_update).for_action(:update) } + end + end +end diff --git a/spec/requests/badges_spec.rb b/spec/requests/badges_spec.rb index 8fdd5871e..2f7314054 100644 --- a/spec/requests/badges_spec.rb +++ b/spec/requests/badges_spec.rb @@ -7,7 +7,7 @@ RSpec.describe "Badges", type: :request do describe "GET /badge/:slug" do it "shows the badge" do get "/badge/#{badge.slug}" - expect(response.body).to include badge.title + expect(response.body).to include CGI.escapeHTML(badge.title) end end end diff --git a/spec/requests/comments_destroy_spec.rb b/spec/requests/comments_destroy_spec.rb index 7a83bace1..a2eb00761 100644 --- a/spec/requests/comments_destroy_spec.rb +++ b/spec/requests/comments_destroy_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe "CommentsUpdate", type: :request do +RSpec.describe "CommentsDestroy", type: :request do let(:user) { create(:user) } let(:article) { create(:article, user_id: user.id) } @@ -8,27 +8,48 @@ RSpec.describe "CommentsUpdate", type: :request do sign_in user end - it "deletes childless article" do - new_body = "NEW TITLE #{rand(100)}" - comment = create(:comment, user_id: user.id, commentable_id: article.id) - delete "/comments/#{comment.id}" - expect(Comment.all.size).to eq(0) + describe "GET /:username/comment/:id_code/delete_confirm" do + it "renders the confirmation message" do + comment = create(:comment, user_id: user.id, commentable_id: article.id) + get comment.path + "/delete_confirm" + expect(response.body).to include("Are you sure you want to delete this comment") + end end - it "deletes childless article" do - new_body = "NEW TITLE #{rand(100)}" - comment = create(:comment, user_id: user.id, commentable_id: article.id) - comment_2 = create(:comment, user_id: user.id, commentable_id: article.id, parent_id: comment.id) - delete "/comments/#{comment.id}" - expect(Comment.first.deleted).to eq(true) + describe "DELETE /comments/:id" do + context "when comment has no children" do + it "destroys the comment" do + comment = create(:comment, user_id: user.id, commentable_id: article.id) + delete "/comments/#{comment.id}" + expect(Comment.all.size).to eq(0) + end + end + + context "when comment has children" do + let(:parent_comment) { create(:comment, user_id: user.id, commentable_id: article.id) } + let(:child_comment) do + create( + :comment, + user_id: user.id, + commentable_id: article.id, + parent_id: parent_comment.id, + ) + end + + before do + parent_comment + child_comment + delete "/comments/#{parent_comment.id}" + end + + it "marks the comment as deleted" do + expect(Comment.first.deleted).to eq(true) + end + + it "renders [deleted]" do + get parent_comment.path + expect(response.body).to include "[deleted]" + end + end end - - - it "visits delete confirm" do - new_body = "NEW TITLE #{rand(100)}" - comment = create(:comment, user_id: user.id, commentable_id: article.id) - get comment.path + "/delete_confirm" - expect(response.body).to include("Are you sure you want to delete this comment") - end - end diff --git a/spec/requests/comments_spec.rb b/spec/requests/comments_spec.rb index 97b9e82c7..15519fc31 100644 --- a/spec/requests/comments_spec.rb +++ b/spec/requests/comments_spec.rb @@ -39,10 +39,10 @@ RSpec.describe "Comments", type: :request do describe "GET /:username/:slug/comments/:id_code/edit" do context "when not logged-in" do - it "returns not_found " do + it "returns unauthorized error" do expect do get "/#{user.username}/#{article.slug}/comments/#{comment.id_code_generated}/edit" - end.to raise_error(ActionController::RoutingError) + end.to raise_error(Pundit::NotAuthorizedError) end end