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

@@ -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

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

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

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

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

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