From d976eb7ab60e6705b303a269dfd55c42d72cdb8f Mon Sep 17 00:00:00 2001
From: Mac Siri <krairit.siri@gmail.com>
Date: Thu, 17 Oct 2019 14:21:43 -0400
Subject: [PATCH] Create InternalPolicy (#4380)

* Ensure verify_authorized in internal

* Create internal_policy

* Replace ArticlePolicy usage with InternalPolicy

* Replace BufferUpdate&CommentPolicy with InternalPolicy

* Remove verbose authorize_admin

* Forgot to remove coresponding specs

* Create specs for InternalPolicy

* Expand policy access on internal's broadcast

* Refactor

* Create shared spec

* Create request specs

* Add additional test cases
---
 .../internal/application_controller.rb        |  1 +
 .../internal/articles_controller.rb           |  8 ++---
 .../internal/broadcasts_controller.rb         |  4 +++
 .../internal/buffer_updates_controller.rb     | 10 +++---
 .../internal/comments_controller.rb           |  8 +++--
 app/models/broadcast.rb                       |  2 ++
 app/policies/internal/article_policy.rb       | 19 -----------
 app/policies/internal/buffer_update_policy.rb | 15 ---------
 app/policies/internal/comment_policy.rb       | 11 -------
 app/policies/internal_policy.rb               |  9 +++++
 spec/policies/internal/article_policy_spec.rb | 18 ----------
 .../internal/buffer_update_policy_spec.rb     | 29 ----------------
 spec/policies/internal/comment_policy_spec.rb | 18 ----------
 spec/policies/internal_policy_spec.rb         | 33 +++++++++++++++++++
 spec/requests/internal/articles_spec.rb       |  8 +++++
 spec/requests/internal/broadcasts_spec.rb     |  8 +++++
 spec/requests/internal/buffer_updates_spec.rb |  8 +++++
 spec/requests/internal/comments_spec.rb       |  8 +++++
 .../internal_policy_dependant_request.rb      | 30 +++++++++++++++++
 19 files changed, 127 insertions(+), 120 deletions(-)
 delete mode 100644 app/policies/internal/article_policy.rb
 delete mode 100644 app/policies/internal/buffer_update_policy.rb
 delete mode 100644 app/policies/internal/comment_policy.rb
 create mode 100644 app/policies/internal_policy.rb
 delete mode 100644 spec/policies/internal/article_policy_spec.rb
 delete mode 100644 spec/policies/internal/buffer_update_policy_spec.rb
 delete mode 100644 spec/policies/internal/comment_policy_spec.rb
 create mode 100644 spec/policies/internal_policy_spec.rb
 create mode 100644 spec/requests/internal/articles_spec.rb
 create mode 100644 spec/requests/internal/broadcasts_spec.rb
 create mode 100644 spec/requests/internal/buffer_updates_spec.rb
 create mode 100644 spec/requests/internal/comments_spec.rb
 create mode 100644 spec/requests/shared_examples/internal_policy_dependant_request.rb

diff --git a/app/controllers/internal/application_controller.rb b/app/controllers/internal/application_controller.rb
index 24408735d..e86a98dfa 100644
--- a/app/controllers/internal/application_controller.rb
+++ b/app/controllers/internal/application_controller.rb
@@ -1,5 +1,6 @@
 class Internal::ApplicationController < ApplicationController
   before_action :authorize_admin
+  after_action :verify_authorized
 
   private
 
diff --git a/app/controllers/internal/articles_controller.rb b/app/controllers/internal/articles_controller.rb
index 83aea89b7..a271749bc 100644
--- a/app/controllers/internal/articles_controller.rb
+++ b/app/controllers/internal/articles_controller.rb
@@ -1,9 +1,7 @@
 class Internal::ArticlesController < Internal::ApplicationController
   layout "internal"
-  skip_before_action :authorize_admin # Instead, specific admin via authorize([:internal, Article])
 
   def index
-    authorize([:internal, Article])
     @pending_buffer_updates = BufferUpdate.where(status: "pending").includes(:article)
 
     case params[:state]
@@ -26,12 +24,10 @@ class Internal::ArticlesController < Internal::ApplicationController
   end
 
   def show
-    authorize([:internal, Article])
     @article = Article.find(params[:id])
   end
 
   def update
-    authorize([:internal, Article])
     article = Article.find(params[:id])
     article.featured = article_params[:featured].to_s == "true"
     article.approved = article_params[:approved].to_s == "true"
@@ -130,4 +126,8 @@ class Internal::ArticlesController < Internal::ApplicationController
                         last_buffered]
     params.require(:article).permit(allowed_params)
   end
+
+  def authorize_admin
+    authorize Article, :access?, policy_class: InternalPolicy
+  end
 end
diff --git a/app/controllers/internal/broadcasts_controller.rb b/app/controllers/internal/broadcasts_controller.rb
index 10d331d03..ece319f45 100644
--- a/app/controllers/internal/broadcasts_controller.rb
+++ b/app/controllers/internal/broadcasts_controller.rb
@@ -31,4 +31,8 @@ class Internal::BroadcastsController < Internal::ApplicationController
     # left out body_markdown and processed_html attributes
     #   until we decide we're using them
   end
+
+  def authorize_admin
+    authorize Broadcast, :access?, policy_class: InternalPolicy
+  end
 end
diff --git a/app/controllers/internal/buffer_updates_controller.rb b/app/controllers/internal/buffer_updates_controller.rb
index 368cfd95f..196744b60 100644
--- a/app/controllers/internal/buffer_updates_controller.rb
+++ b/app/controllers/internal/buffer_updates_controller.rb
@@ -1,8 +1,5 @@
 class Internal::BufferUpdatesController < Internal::ApplicationController
-  skip_before_action :authorize_admin # Instead, specific admin via authorize([:internal, Article])
-
   def create
-    authorize([:internal, BufferUpdate])
     article_id = params[:article_id]
     article = Article.find(article_id) if article_id.present?
     fb_post = params[:fb_post]
@@ -26,8 +23,13 @@ class Internal::BufferUpdatesController < Internal::ApplicationController
   end
 
   def update
-    authorize([:internal, BufferUpdate])
     BufferUpdate.upbuff!(params[:id], current_user.id, params[:body_text], params[:status])
     render body: nil
   end
+
+  private
+
+  def authorize_admin
+    authorize BufferUpdate, :access?, policy_class: InternalPolicy
+  end
 end
diff --git a/app/controllers/internal/comments_controller.rb b/app/controllers/internal/comments_controller.rb
index 4011dd02f..12cbba481 100644
--- a/app/controllers/internal/comments_controller.rb
+++ b/app/controllers/internal/comments_controller.rb
@@ -1,9 +1,7 @@
 class Internal::CommentsController < Internal::ApplicationController
   layout "internal"
-  skip_before_action :authorize_admin
 
   def index
-    authorize([:internal, Comment])
     @comments = if params[:state]&.start_with?("toplast-")
                   Comment.
                     includes(:user).
@@ -21,4 +19,10 @@ class Internal::CommentsController < Internal::ApplicationController
                     page(params[:page] || 1).per(50)
                 end
   end
+
+  private
+
+  def authorize_admin
+    authorize Comment, :access?, policy_class: InternalPolicy
+  end
 end
diff --git a/app/models/broadcast.rb b/app/models/broadcast.rb
index ee2d7952a..871f89c76 100644
--- a/app/models/broadcast.rb
+++ b/app/models/broadcast.rb
@@ -1,4 +1,6 @@
 class Broadcast < ApplicationRecord
+  resourcify
+
   has_many :notifications, as: :notifiable, inverse_of: :notifiable
 
   validates :title, :type_of, :processed_html, presence: true
diff --git a/app/policies/internal/article_policy.rb b/app/policies/internal/article_policy.rb
deleted file mode 100644
index b0202f714..000000000
--- a/app/policies/internal/article_policy.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-class Internal::ArticlePolicy < ApplicationPolicy
-  def index?
-    article_admin?
-  end
-
-  def show?
-    article_admin?
-  end
-
-  def update?
-    article_admin?
-  end
-
-  private
-
-  def article_admin?
-    user.has_role?(:single_resource_admin, Article) || user.has_role?(:super_admin) || user.has_role?(:admin)
-  end
-end
diff --git a/app/policies/internal/buffer_update_policy.rb b/app/policies/internal/buffer_update_policy.rb
deleted file mode 100644
index c355a4dc5..000000000
--- a/app/policies/internal/buffer_update_policy.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-class Internal::BufferUpdatePolicy < ApplicationPolicy
-  def create?
-    buffer_admin? || minimal_admin?
-  end
-
-  def update?
-    buffer_admin? || minimal_admin?
-  end
-
-  private
-
-  def buffer_admin?
-    user.has_role?(:single_resource_admin, BufferUpdate)
-  end
-end
diff --git a/app/policies/internal/comment_policy.rb b/app/policies/internal/comment_policy.rb
deleted file mode 100644
index 15800b353..000000000
--- a/app/policies/internal/comment_policy.rb
+++ /dev/null
@@ -1,11 +0,0 @@
-class Internal::CommentPolicy < ApplicationPolicy
-  def index?
-    comment_admin?
-  end
-
-  private
-
-  def comment_admin?
-    user.has_role?(:single_resource_admin, Comment) || user.has_role?(:super_admin) || user.has_role?(:admin)
-  end
-end
diff --git a/app/policies/internal_policy.rb b/app/policies/internal_policy.rb
new file mode 100644
index 000000000..04751d370
--- /dev/null
+++ b/app/policies/internal_policy.rb
@@ -0,0 +1,9 @@
+class InternalPolicy < ApplicationPolicy
+  def access?
+    user.has_any_role?(
+      { name: :single_resource_admin, resource: record },
+      :super_admin,
+      :admin,
+    )
+  end
+end
diff --git a/spec/policies/internal/article_policy_spec.rb b/spec/policies/internal/article_policy_spec.rb
deleted file mode 100644
index a554019d3..000000000
--- a/spec/policies/internal/article_policy_spec.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Internal::ArticlePolicy do
-  subject { described_class.new(user, article) }
-
-  let(:article) { build(:article) }
-  let(:user) { build_stubbed(:user) }
-
-  context "when regular user" do
-    it { is_expected.to forbid_actions(%i[index show update]) }
-  end
-
-  context "when user has a scoped article admin role" do
-    before { allow(user).to receive(:has_role?).with(:single_resource_admin, Article).and_return(true) }
-
-    it { is_expected.to permit_actions(%i[index show update]) }
-  end
-end
diff --git a/spec/policies/internal/buffer_update_policy_spec.rb b/spec/policies/internal/buffer_update_policy_spec.rb
deleted file mode 100644
index 05485a1d1..000000000
--- a/spec/policies/internal/buffer_update_policy_spec.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Internal::BufferUpdatePolicy do
-  subject { described_class.new(user, BufferUpdate) }
-
-  let(:user) { build_stubbed(:user) }
-
-  context "when regular user" do
-    it { is_expected.to forbid_actions(%i[create update]) }
-  end
-
-  context "when user is permission to update buffers" do
-    before { user.add_role(:single_resource_admin, BufferUpdate) }
-
-    it { is_expected.to permit_actions(%i[create update]) }
-  end
-
-  context "when user is an admin" do
-    before { user.add_role(:admin) }
-
-    it { is_expected.to permit_actions(%i[create update]) }
-  end
-
-  context "when user is a super_admin" do
-    before { user.add_role(:super_admin) }
-
-    it { is_expected.to permit_actions(%i[create update]) }
-  end
-end
diff --git a/spec/policies/internal/comment_policy_spec.rb b/spec/policies/internal/comment_policy_spec.rb
deleted file mode 100644
index 36a28aab6..000000000
--- a/spec/policies/internal/comment_policy_spec.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-require "rails_helper"
-
-RSpec.describe Internal::CommentPolicy do
-  subject { described_class.new(user, comment) }
-
-  let(:comment) { Comment }
-  let(:user) { build_stubbed(:user) }
-
-  context "when regular user" do
-    it { is_expected.to forbid_actions(%i[index]) }
-  end
-
-  context "when user has a scoped article admin role" do
-    before { user.add_role(:single_resource_admin, Comment) }
-
-    it { is_expected.to permit_actions(%i[index]) }
-  end
-end
diff --git a/spec/policies/internal_policy_spec.rb b/spec/policies/internal_policy_spec.rb
new file mode 100644
index 000000000..249524ec4
--- /dev/null
+++ b/spec/policies/internal_policy_spec.rb
@@ -0,0 +1,33 @@
+require "rails_helper"
+
+RSpec.describe InternalPolicy, type: :policy do
+  let(:internal_policy) { described_class }
+
+  permissions :access? do
+    it "does not allow someone without admin privileges to do continue" do
+      expect(internal_policy).not_to permit(build(:user))
+    end
+
+    it "allow someone with admin privileges to continue" do
+      expect(internal_policy).to permit(build(:user, :admin))
+    end
+
+    it "allow someone with super_admin privileges to continue" do
+      expect(internal_policy).to permit(build(:user, :super_admin))
+    end
+
+    context "when tied to a resource" do
+      let(:user) { create(:user) }
+
+      it "grant access based on permitted resource" do
+        user.add_role(:single_resource_admin, Article)
+        expect(internal_policy).to permit(user, Article)
+      end
+
+      it "does not grant cross resource access" do
+        user.add_role(:single_resource_admin, Article)
+        expect(internal_policy).not_to permit(user, Comment)
+      end
+    end
+  end
+end
diff --git a/spec/requests/internal/articles_spec.rb b/spec/requests/internal/articles_spec.rb
new file mode 100644
index 000000000..75448134c
--- /dev/null
+++ b/spec/requests/internal/articles_spec.rb
@@ -0,0 +1,8 @@
+require "rails_helper"
+require "requests/shared_examples/internal_policy_dependant_request"
+
+RSpec.describe "/internal/articles", type: :request do
+  it_behaves_like "an InternalPolicy dependant request", Article do
+    let(:request) { get "/internal/articles" }
+  end
+end
diff --git a/spec/requests/internal/broadcasts_spec.rb b/spec/requests/internal/broadcasts_spec.rb
new file mode 100644
index 000000000..56498daf4
--- /dev/null
+++ b/spec/requests/internal/broadcasts_spec.rb
@@ -0,0 +1,8 @@
+require "rails_helper"
+require "requests/shared_examples/internal_policy_dependant_request"
+
+RSpec.describe "/internal/Broadcasts", type: :request do
+  it_behaves_like "an InternalPolicy dependant request", Broadcast do
+    let(:request) { get "/internal/broadcasts" }
+  end
+end
diff --git a/spec/requests/internal/buffer_updates_spec.rb b/spec/requests/internal/buffer_updates_spec.rb
new file mode 100644
index 000000000..0ba5f46bb
--- /dev/null
+++ b/spec/requests/internal/buffer_updates_spec.rb
@@ -0,0 +1,8 @@
+require "rails_helper"
+require "requests/shared_examples/internal_policy_dependant_request"
+
+RSpec.describe "/internal/buffer_updates", type: :request do
+  it_behaves_like "an InternalPolicy dependant request", BufferUpdate do
+    let(:request) { post "/internal/buffer_updates" }
+  end
+end
diff --git a/spec/requests/internal/comments_spec.rb b/spec/requests/internal/comments_spec.rb
new file mode 100644
index 000000000..517060c23
--- /dev/null
+++ b/spec/requests/internal/comments_spec.rb
@@ -0,0 +1,8 @@
+require "rails_helper"
+require "requests/shared_examples/internal_policy_dependant_request"
+
+RSpec.describe "/internal/comments", type: :request do
+  it_behaves_like "an InternalPolicy dependant request", Comment do
+    let(:request) { get "/internal/comments" }
+  end
+end
diff --git a/spec/requests/shared_examples/internal_policy_dependant_request.rb b/spec/requests/shared_examples/internal_policy_dependant_request.rb
new file mode 100644
index 000000000..0cb3f9410
--- /dev/null
+++ b/spec/requests/shared_examples/internal_policy_dependant_request.rb
@@ -0,0 +1,30 @@
+RSpec.shared_examples "an InternalPolicy dependant request" do |resource|
+  let(:user) { create(:user) }
+
+  context "when user is a single_resource_admin" do
+    before do
+      user.add_role(:single_resource_admin, resource)
+      sign_in user
+      allow(InternalPolicy).to receive(:new).and_call_original
+    end
+
+    it "responds with 200 OK" do
+      request
+      expect(response).to have_http_status(:success)
+      expect(InternalPolicy).to have_received(:new).with(user, resource)
+    end
+  end
+
+  context "when user is not an admin", proper_status: true do
+    before do
+      sign_in user
+      allow(InternalPolicy).to receive(:new).and_call_original
+    end
+
+    it "responds with 404 not_found" do
+      request
+      expect(response).to have_http_status(:not_found)
+      expect(InternalPolicy).to have_received(:new).with(user, resource)
+    end
+  end
+end