From a0d90874a9d5646e1dfd5c836825303d1381051f Mon Sep 17 00:00:00 2001
From: Ben Halpern <bendhalpern@gmail.com>
Date: Thu, 24 Jan 2019 13:35:39 -0500
Subject: [PATCH] Move org admin privilege check into controller (#1642)

---
 app/controllers/api/v0/articles_controller.rb |  2 +-
 app/controllers/articles_controller.rb        | 11 +++++++++-
 app/policies/article_policy.rb                | 20 +++----------------
 spec/policies/article_policy_spec.rb          | 10 +++++-----
 4 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/app/controllers/api/v0/articles_controller.rb b/app/controllers/api/v0/articles_controller.rb
index 556589e0a..a414f926f 100644
--- a/app/controllers/api/v0/articles_controller.rb
+++ b/app/controllers/api/v0/articles_controller.rb
@@ -88,7 +88,7 @@ module Api
           params["article"]["collection_id"] = nil
         end
         params.require(:article).permit(
-          :title, :body_markdown, :user_id, :main_image, :published, :description,
+          :title, :body_markdown, :main_image, :published, :description,
           :tag_list, :organization_id, :canonical_url, :series, :collection_id
         )
       end
diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb
index 0baac6a60..36f26956b 100644
--- a/app/controllers/articles_controller.rb
+++ b/app/controllers/articles_controller.rb
@@ -182,7 +182,9 @@ class ArticlesController < ApplicationController
 
   def article_params
     params[:article][:published] = true if params[:submit_button] == "PUBLISH"
-    params.require(:article).permit(policy(Article).permitted_attributes)
+    modified_params = policy(Article).permitted_attributes
+    modified_params << :user_id if org_admin_user_change_privilege
+    params.require(:article).permit(modified_params)
   end
 
   def job_opportunity_params
@@ -207,4 +209,11 @@ class ArticlesController < ApplicationController
       render :new
     end
   end
+
+  def org_admin_user_change_privilege
+    params[:article][:user_id] &&
+      current_user.org_admin &&
+      current_user.organization_id == @article.organization_id &&
+      User.find(params[:article][:user_id])&.organization_id == @article.organization_id
+  end
 end
diff --git a/app/policies/article_policy.rb b/app/policies/article_policy.rb
index 4fc0fa3a9..c99c77d7b 100644
--- a/app/policies/article_policy.rb
+++ b/app/policies/article_policy.rb
@@ -19,10 +19,6 @@ class ArticlePolicy < ApplicationPolicy
     update?
   end
 
-  def toggle_mute?
-    update?
-  end
-
   def preview?
     true
   end
@@ -32,15 +28,9 @@ class ArticlePolicy < ApplicationPolicy
   end
 
   def permitted_attributes
-    if user_org_admin? && author_org_member?
-      %i[title body_html user_id body_markdown main_image published canonical_url
-         description allow_small_edits allow_big_edits tag_list publish_under_org
-         video video_code video_source_url video_thumbnail_url]
-    else
-      %i[title body_html body_markdown main_image published canonical_url
-         description allow_small_edits allow_big_edits tag_list publish_under_org
-         video video_code video_source_url video_thumbnail_url]
-    end
+    %i[title body_html body_markdown main_image published canonical_url
+       description allow_small_edits allow_big_edits tag_list publish_under_org
+       video video_code video_source_url video_thumbnail_url]
   end
 
   private
@@ -57,10 +47,6 @@ class ArticlePolicy < ApplicationPolicy
     user.org_admin && user.organization_id == record.organization_id
   end
 
-  def author_org_member?
-    User.find(params[:user_id]).organization_id == record.organization_id
-  end
-
   def user_can_view_analytics?
     user.can_view_analytics?
   end
diff --git a/spec/policies/article_policy_spec.rb b/spec/policies/article_policy_spec.rb
index 3bbb86ea1..bccddb176 100644
--- a/spec/policies/article_policy_spec.rb
+++ b/spec/policies/article_policy_spec.rb
@@ -20,33 +20,33 @@ RSpec.describe ArticlePolicy 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 analytics_index toggle_mute]) }
+    it { is_expected.to forbid_actions(%i[update delete_confirm destroy analytics_index]) }
 
     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 analytics_index toggle_mute]) }
+      it { is_expected.to forbid_actions(%i[create update delete_confirm destroy analytics_index]) }
     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 toggle_mute]) }
+    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 toggle_mute]) }
+      it { is_expected.to permit_actions(%i[update new delete_confirm destroy preview]) }
     end
   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 toggle_mute]) }
+    it { is_expected.to permit_actions(%i[update new create delete_confirm destroy preview]) }
   end
 
   context "when a user with analytics tries to view someone else's article" do