From ff36271108517db5e540a2d23775b8fc188271bc Mon Sep 17 00:00:00 2001
From: Mac Siri <krairit.siri@gmail.com>
Date: Thu, 21 Jun 2018 14:18:32 -0400
Subject: [PATCH] Improve ArticlesController's authorization (#466)

* Fix lint

* Add pundit-matchers gem

* Strengthen ArticlePolicy & create spec

* Change ApplicationPolicy to a closed system

* Globally handle pundit's NotAuthorizedError

* Fix lint

* Adjust /new's authorization
---
 Gemfile                                      |  8 +--
 Gemfile.lock                                 |  3 +
 app/controllers/application_controller.rb    | 14 -----
 app/controllers/articles_controller.rb       | 60 +++++++++-----------
 app/policies/application_policy.rb           |  6 +-
 app/policies/article_policy.rb               | 22 ++++++-
 config/application.rb                        |  4 ++
 spec/controllers/articles_controller_spec.rb |  4 ++
 spec/factories/users.rb                      |  4 ++
 spec/policies/article_policy_spec.rb         | 57 +++++++++++++++++++
 spec/rails_helper.rb                         |  3 +
 spec/requests/chat_channels_spec.rb          | 19 ++++---
 spec/support/within_block_is_expected.rb     |  9 +++
 13 files changed, 149 insertions(+), 64 deletions(-)
 create mode 100644 spec/policies/article_policy_spec.rb
 create mode 100644 spec/support/within_block_is_expected.rb

diff --git a/Gemfile b/Gemfile
index dd74f1c2f..902508044 100644
--- a/Gemfile
+++ b/Gemfile
@@ -58,8 +58,8 @@ gem "pry", "~> 0.11"
 gem "pry-rails", "~> 0.3"
 gem "puma", "~> 3.11"
 gem "puma_worker_killer", "~> 0.1"
-gem "pusher", "~> 1.3"
 gem "pundit", "~> 1.1"
+gem "pusher", "~> 1.3"
 gem "rack-host-redirect", "~> 1.3"
 gem "rack-timeout", "~> 0.4"
 gem "rails", "~> 5.1"
@@ -85,13 +85,12 @@ gem "stream_rails", "~> 2.5"
 gem "stripe", "~> 3.9"
 gem "therubyracer", "~> 0.12", platforms: :ruby
 gem "timber", "~> 2.6"
-gem 'twilio-ruby', '~> 5.10.3'
+gem "twilio-ruby", "~> 5.10.3"
 gem "twitter", "~> 6.2"
 gem "uglifier", "~> 3.2"
 gem "validate_url", "~> 1.0"
 gem "webpacker", "~> 3.2"
-gem 'webpush', "~> 0.3"
-
+gem "webpush", "~> 0.3"
 
 group :development do
   gem "brakeman", "~> 3.7", require: false
@@ -129,6 +128,7 @@ group :test do
   gem "factory_bot_rails", "~> 4.8"
   gem "fake_stripe", "~> 0.1"
   gem "launchy", "~> 2.4"
+  gem "pundit-matchers", "~> 1.6"
   gem "rack_session_access", "~> 0.1"
   gem "rails-controller-testing", "~> 1.0"
   gem "ruby-prof", "~> 0.17", require: false
diff --git a/Gemfile.lock b/Gemfile.lock
index 21f5a3df6..7707f9f1e 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -618,6 +618,8 @@ GEM
       puma (>= 2.7, < 4)
     pundit (1.1.0)
       activesupport (>= 3.0.0)
+    pundit-matchers (1.6.0)
+      rspec-rails (>= 3.0.0)
     pusher (1.3.1)
       httpclient (~> 2.7)
       multi_json (~> 1.0)
@@ -979,6 +981,7 @@ DEPENDENCIES
   puma (~> 3.11)
   puma_worker_killer (~> 0.1)
   pundit (~> 1.1)
+  pundit-matchers (~> 1.6)
   pusher (~> 1.3)
   rack-host-redirect (~> 1.3)
   rack-timeout (~> 0.4)
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 9570bcdd3..7da5db2c4 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -4,7 +4,6 @@ class ApplicationController < ActionController::Base
   include EnforceAdmin
 
   include Pundit
-  rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized
 
   # before_action :require_http_auth if ENV["APP_NAME"] == "dev_stage"
 
@@ -87,17 +86,4 @@ class ApplicationController < ActionController::Base
   def touch_current_user
     current_user.touch
   end
-
-  private
-
-  def user_not_authorized
-    respond_to do |format|
-      format.html do
-        flash[:alert] = "You are not authorized to perform this action."
-        redirect_to(request.referrer || root_path)
-      end
-
-      format.json { render json: { error: "Not authorized" }, status: 401 }
-    end
-  end
 end
diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb
index b5851834f..630f3b272 100644
--- a/app/controllers/articles_controller.rb
+++ b/app/controllers/articles_controller.rb
@@ -1,24 +1,26 @@
 class ArticlesController < ApplicationController
   include ApplicationHelper
-  before_action :authenticate_user!, except: [:feed, :new]
-  before_action :set_article, only: [:edit, :update, :destroy]
-  before_action :raise_banned, only: [:new, :create, :update]
-  before_action :set_cache_control_headers, only: [:feed]
+  before_action :authenticate_user!, except: %i[feed new]
+  before_action :set_article, only: %i[edit update destroy]
+  before_action :raise_banned, only: %i[new create update]
+  before_action :set_cache_control_headers, only: %i[feed]
+  after_action :verify_authorized
 
   def feed
+    skip_authorization
     @page = params[:page].to_i
     if params[:username]
       if @user = User.find_by_username(params[:username])
         @articles = Article.where(published: true, user_id: @user.id).
           includes(:user).
           select(:published_at, :slug, :processed_html, :user_id, :organization_id, :title).
-          order('published_at DESC').
+          order("published_at DESC").
           page(@page).per(15)
       elsif @user = Organization.find_by_slug(params[:username])
         @articles = Article.where(published: true, organization_id: @user.id).
           includes(:user).
           select(:published_at, :slug, :processed_html, :user_id, :organization_id, :title).
-          order('published_at DESC').
+          order("published_at DESC").
           page(@page).per(15)
       else
         render body: nil
@@ -28,7 +30,7 @@ class ArticlesController < ApplicationController
       @articles = Article.where(published: true, featured: true).
         includes(:user).
         select(:published_at, :slug, :processed_html, :user_id, :organization_id, :title).
-        order('published_at DESC').
+        order("published_at DESC").
         page(@page).per(15)
     end
     set_surrogate_key_header "feed", @articles.map(&:record_key)
@@ -37,14 +39,19 @@ class ArticlesController < ApplicationController
   end
 
   def new
-    authorize Article
     @user = current_user
     @tag = Tag.find_by_name(params[:template])
-    if @tag && @tag.submission_template.present? && @user
-      @article = Article.new(body_markdown:@tag.submission_template_customized(@user.name),processed_html:"")
-    else
-      @article = Article.new(body_markdown:"---\ntitle: \npublished: false\ndescription: \ntags: \n---\n\n",processed_html:"")
-    end
+    @article = if @tag&.submission_template.present? && @user
+                 authorize Article
+                 Article.new(body_markdown: @tag.submission_template_customized(@user.name),
+                             processed_html: "")
+               else
+                 skip_authorization
+                 Article.new(
+                   body_markdown: "---\ntitle: \npublished: false\ndescription: \ntags: \n---\n\n",
+                   processed_html: "",
+                 )
+               end
   end
 
   def edit
@@ -53,6 +60,7 @@ class ArticlesController < ApplicationController
   end
 
   def preview
+    authorize Article
     begin
       fixed_body_markdown = MarkdownFixer.fix_for_preview(params[:article_body])
       parsed = FrontMatterParser::Parser.new(:md).call(fixed_body_markdown)
@@ -108,29 +116,20 @@ class ArticlesController < ApplicationController
 
   def delete_confirm
     @article = current_user.articles.find_by_slug(params[:slug])
+    authorize @article
   end
 
   def destroy
-    if current_user == @article.user
-      @article.destroy
-      respond_to do |format|
-        format.html { redirect_to "/dashboard", notice: "Article was successfully deleted." }
-        format.json { head :no_content }
-      end
-    else
-      respond_to do |format|
-        format.html { redirect_to "/dashboard", notice: "Unauthorized attempt" }
-        format.json { head :no_content }
-      end
+    authorize @article
+    @article.destroy!
+    respond_to do |format|
+      format.html { redirect_to "/dashboard", notice: "Article was successfully deleted." }
+      format.json { head :no_content }
     end
   end
 
   private
 
-  def user_not_authorized
-    redirect_to @article ? @article.path : "/"
-  end
-
   def handle_org_assignment
     if @user.organization_id.present? && article_params[:publish_under_org].to_i == 1
       @article.organization_id = @user.organization_id
@@ -175,10 +174,7 @@ class ArticlesController < ApplicationController
 
   def article_params
     params[:article][:published] = true if params[:submit_button] == "PUBLISH"
-    params.require(:article).
-      permit(:title, :body_html, :body_markdown, :user_id, :main_image, :published,
-          :description, :allow_small_edits, :allow_big_edits, :tag_list, :publish_under_org,
-          :video, :video_code, :video_source_url, :video_thumbnail_url)
+    params.require(:article).permit(policy(Article).permitted_attributes)
   end
 
   def job_opportunity_params
diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb
index 12357d3c9..2cc6e0854 100644
--- a/app/policies/application_policy.rb
+++ b/app/policies/application_policy.rb
@@ -2,6 +2,7 @@ class ApplicationPolicy
   attr_reader :user, :record
 
   def initialize(user, record)
+    raise Pundit::NotAuthorizedError, "You must be logged in" unless user
     @user = user
     @record = record
   end
@@ -11,7 +12,7 @@ class ApplicationPolicy
   end
 
   def show?
-    scope.where(:id => record.id).exists?
+    scope.where(id: record.id).exists?
   end
 
   def create?
@@ -42,6 +43,7 @@ class ApplicationPolicy
     attr_reader :user, :scope
 
     def initialize(user, scope)
+      raise Pundit::NotAuthorizedError, "must be logged in" unless user
       @user = user
       @scope = scope
     end
@@ -51,8 +53,6 @@ class ApplicationPolicy
     end
   end
 
-  private
-
   def user_is_admin?
     user.has_any_role?(:super_admin, :admin)
   end
diff --git a/app/policies/article_policy.rb b/app/policies/article_policy.rb
index fa5faadab..3e1df7bf8 100644
--- a/app/policies/article_policy.rb
+++ b/app/policies/article_policy.rb
@@ -4,13 +4,31 @@ class ArticlePolicy < ApplicationPolicy
   end
 
   def new?
-    !user_is_banned?
+    true
   end
 
   def create?
     !user_is_banned?
   end
 
+  def delete_confirm?
+    update?
+  end
+
+  def destroy?
+    update?
+  end
+
+  def preview?
+    true
+  end
+
+  def permitted_attributes
+    %i[title body_html body_markdown user_id main_image published
+       description allow_small_edits allow_big_edits tag_list publish_under_org
+       video video_code video_source_url video_thumbnail_url]
+  end
+
   private
 
   def user_is_author?
@@ -22,6 +40,6 @@ class ArticlePolicy < ApplicationPolicy
   end
 
   def user_is_banned?
-    user && user.has_role?(:banned)
+    user.has_role?(:banned)
   end
 end
diff --git a/config/application.rb b/config/application.rb
index 6c1e37619..32ca1b7ce 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -44,6 +44,10 @@ module PracticalDeveloper
     # to implement access control for the Flipflop dashboard.
     config.flipflop.dashboard_access_filter = -> { head :forbidden unless current_user.has_any_role?(:super_admin) }
 
+    # Globally handle Pundit::NotAuthorizedError by serving 404
+    config.action_dispatch.rescue_responses["Pundit::NotAuthorizedError"] = :not_found
+
+    # After-initialize checker to add routes to reserved words
     config.after_initialize do
       Rails.application.reload_routes!
       top_routes = []
diff --git a/spec/controllers/articles_controller_spec.rb b/spec/controllers/articles_controller_spec.rb
index abf5cc6f1..7a60e717d 100644
--- a/spec/controllers/articles_controller_spec.rb
+++ b/spec/controllers/articles_controller_spec.rb
@@ -3,6 +3,8 @@
 require "rails_helper"
 
 RSpec.describe ArticlesController, type: :controller do
+  let(:user) { create(:user) }
+
   describe "GET #feed" do
     it "works" do
       get :feed, format: :rss
@@ -11,6 +13,8 @@ RSpec.describe ArticlesController, type: :controller do
   end
 
   describe "GET #new" do
+    before { sign_in user }
+
     context "with authorized user" do
       it "returns a new article" do
         get :new
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index b5166f923..b3b4f1e5e 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -22,6 +22,10 @@ FactoryBot.define do
       after(:build) { |user| user.add_role(:super_admin) }
     end
 
+    trait :admin do
+      after(:build) { |user| user.add_role(:admin) }
+    end
+
     trait :banned do
       after(:build) { |user| user.add_role(:banned) }
     end
diff --git a/spec/policies/article_policy_spec.rb b/spec/policies/article_policy_spec.rb
new file mode 100644
index 000000000..22510eacc
--- /dev/null
+++ b/spec/policies/article_policy_spec.rb
@@ -0,0 +1,57 @@
+require "rails_helper"
+
+RSpec.describe ArticlePolicy do
+  subject { described_class.new(user, article) }
+
+  let(:article) { build(:article) }
+  let(:valid_attributes) do
+    %i[title body_html body_markdown user_id main_image published
+       description allow_small_edits allow_big_edits tag_list publish_under_org
+       video video_code video_source_url video_thumbnail_url]
+  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[new create preview]) }
+    it { is_expected.to forbid_actions(%i[update delete_confirm destroy]) }
+
+    context "with banned status" do
+      before { user.add_role :banned }
+
+      it { is_expected.to permit_actions(%i[new preview]) }
+      it { is_expected.to forbid_actions(%i[create update delete_confirm destroy]) }
+    end
+  end
+
+  context "when user is the author" do
+    let(:user) { article.user }
+
+    it { is_expected.to permit_actions(%i[update new create delete_confirm destroy preview]) }
+    it { is_expected.to permit_mass_assignment_of(valid_attributes) }
+
+    context "with banned status" do
+      before { user.add_role :banned }
+
+      it { is_expected.to permit_actions(%i[update new delete_confirm destroy preview]) }
+    end
+  end
+
+  context "when user is an admin" do
+    let(:user) { build(:user, :admin) }
+
+    it { is_expected.to permit_actions(%i[update new create delete_confirm destroy preview]) }
+  end
+
+  context "when user is a super_admin" do
+    let(:user) { build(:user, :super_admin) }
+
+    it { is_expected.to permit_actions(%i[update new create delete_confirm destroy preview]) }
+  end
+end
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index 8c14dd90e..831a6d6b1 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -17,6 +17,9 @@ require "rspec/retry"
 require "algolia/webmock"
 require "approvals/rspec"
 require "shoulda/matchers"
+require "pundit/rspec"
+require "pundit/matchers"
+
 
 WebMock.disable_net_connect!(allow_localhost: true)
 
diff --git a/spec/requests/chat_channels_spec.rb b/spec/requests/chat_channels_spec.rb
index 52d963d32..32cc10583 100644
--- a/spec/requests/chat_channels_spec.rb
+++ b/spec/requests/chat_channels_spec.rb
@@ -10,7 +10,6 @@ RSpec.describe "ChatChannels", type: :request do
     chat_channel.add_users([user])
   end
 
-
   describe "GET /connect" do
     context "logged in" do
       before do
@@ -52,19 +51,21 @@ RSpec.describe "ChatChannels", type: :request do
   end
 
   describe "POST /chat_channels/:id/moderate" do
-    it "returns 401 unless user is logged in" do
-      post "/chat_channels/#{chat_channel.id}/moderate",
+    it "raises NotAuthorizedError if user is not logged in" do
+      expect do
+        post "/chat_channels/#{chat_channel.id}/moderate",
         params: { chat_channel: { command: "/ban huh" } },
         headers: { HTTP_ACCEPT: "application/json" }
-      expect(response.status).to eq(401)
+      end.to raise_error(Pundit::NotAuthorizedError)
     end
 
-    it "returns 401 if user is logged in but not authorized" do
+    it "raises NotAuthorizedError if user is logged in but not authorized" do
       sign_in user
-      post "/chat_channels/#{chat_channel.id}/moderate",
-        params: { chat_channel: { command: "/ban huh" } },
-        headers: { HTTP_ACCEPT: "application/json" }
-      expect(response.status).to eq(401)
+      expect do
+        post "/chat_channels/#{chat_channel.id}/moderate",
+          params: { chat_channel: { command: "/ban huh" } },
+          headers: { HTTP_ACCEPT: "application/json" }
+      end.to raise_error(Pundit::NotAuthorizedError)
     end
 
     context "when user is logged-in and authorized" do
diff --git a/spec/support/within_block_is_expected.rb b/spec/support/within_block_is_expected.rb
new file mode 100644
index 000000000..e535ff7a1
--- /dev/null
+++ b/spec/support/within_block_is_expected.rb
@@ -0,0 +1,9 @@
+module WithinBlockIsExpected
+  def within_block_is_expected
+    expect { subject }
+  end
+end
+
+RSpec.configure do |config|
+  config.include WithinBlockIsExpected
+end