From d1069af35c90b2fbe3dcf7a74740b790c724f761 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 7 Mar 2022 15:27:43 -0500 Subject: [PATCH] Adding ArticlePolicy#moderate? (#16786) This commit does three things: 1) Adds and tests the ArticlePolicy#moderate? feature 2) Refactors the spec helpers to be a bit more flexible 3) Adds a few more edge case tests around ArticlePolicy My apologies for conflating these changes, as it makes this commit more challenging to review. The goal is to adequate demonstrate the logic around users who can or cannot moderate. At present, the rules for who can moderate is here: https://github.com/forem/forem/blob/841491c6ee7f9a46d8033b4b55052316db251863/app/javascript/packs/articleModerationTools.js#L17-L27 ```js if (user?.trusted) { if (user?.id !== articleAuthorId && !isModerationPage()) { initializeActionsPanel(user, path); initializeFlagUserModal(articleAuthorId); // "/mod" page } else if (isModerationPage()) { initializeActionsPanel(user, path); initializeFlagUserModal(articleAuthorId); } } ``` - Related to forem/forem#16783 - Closes forem/forem#16784 --- app/policies/article_policy.rb | 18 +++++ spec/policies/article_policy_spec.rb | 112 ++++++++++++++------------- 2 files changed, 78 insertions(+), 52 deletions(-) diff --git a/app/policies/article_policy.rb b/app/policies/article_policy.rb index 7806d5535..43571d9ed 100644 --- a/app/policies/article_policy.rb +++ b/app/policies/article_policy.rb @@ -112,6 +112,24 @@ class ArticlePolicy < ApplicationPolicy user_author? || user_super_admin? || user_org_admin? || user_any_admin? end + # @see https://github.com/forem/forem/blob/841491c6ee7f9a46d8033b4b55052316db251863/app/javascript/packs/articleModerationTools.js#L17-L27 + # for details regarding original authorization logic for this method. + def moderate? + # Technically, we could check the limit_post_creation_to_admins? first, but [@jeremyf]'s + # operating on a "trying to maintain consistency" approach. + require_user_in_good_standing! + + return false if self.class.limit_post_creation_to_admins? + + # Don't let a user moderate their own article. See for prior UI logic reinforcing this + # https://github.com/forem/forem/blob/841491c6ee7f9a46d8033b4b55052316db251863/app/javascript/packs/articleModerationTools.js#L17-L27 + return false if user_author? + + # Beware a trusted user does not guarantee that they are an admin. And more specifically, being + # an admin does not guarantee being trusted. + user.trusted? + end + alias admin_featured_toggle? admin_unpublish? alias new? create? diff --git a/spec/policies/article_policy_spec.rb b/spec/policies/article_policy_spec.rb index ec5df6a7d..36aec1cb3 100644 --- a/spec/policies/article_policy_spec.rb +++ b/spec/policies/article_policy_spec.rb @@ -4,83 +4,77 @@ require "rails_helper" # And to extract would require more explict interface definition. RSpec.shared_examples "it requires an authenticated user" do let(:user) { nil } - specify "otherwise it raises ApplicationPolicy::UserRequiredError" do + + it "otherwise raises ApplicationPolicy::UserRequiredError" do expect { subject }.to raise_error(ApplicationPolicy::UserRequiredError) end end RSpec.shared_examples "it requires a user in good standing" do - let(:author) { create(:user, :suspended) } - specify "otherwise it raises ApplicationPolicy::UserSuspendedError" do + let(:user) { create(:user, :suspended) } + + it "otherwise raises ApplicationPolicy::UserSuspendedError" do expect { subject }.to raise_error(ApplicationPolicy::UserSuspendedError) end end -RSpec.shared_examples "permitted roles" do |kwargs| - Array(kwargs.fetch(:to)).each do |role| - context "#{role.inspect} authorization" do - case role - when :suspended_author - let(:author) { create(:user, :suspended) } - when :org_admin - let(:organization) { user.organizations.first } - let(:user) { create(:user, role, :org_admin) } - when :anyone - let(:user) { create(:user) } +RSpec.shared_examples "permitted roles" do |**kwargs| + to = kwargs.delete(:to) + label = kwargs.except(:to).map { |key, value| "#{key} is #{value}" }.join(" AND ") + label = "when #{label} " if label.present? + + Array(to).each do |role| + context "#{label}#{role.inspect} authorization" do + before { kwargs.each { |k, v| allow(described_class).to receive(k).and_return(v) } } + + if role == :suspended_author + let(:author) { suspended_user } + let(:user) { author } else - let(:user) { create(:user, role) } + let(:user) { public_send(role) } end + it { is_expected.to be_truthy } end end end -RSpec.shared_examples "disallowed roles" do |kwargs| - Array(kwargs.fetch(:to)).each do |role| - context "#{role.inspect} authorization" do - case role - when :org_admin - let(:organization) { user.organizations.first } - let(:user) { create(:user, role, :org_admin) } - when :author + +RSpec.shared_examples "disallowed roles" do |**kwargs| + to = kwargs.delete(:to) + label = kwargs.map { |key, value| "#{key} is #{value}" }.join(" AND ") + label = "when #{label} " if label.present? + + Array(to).each do |role| + context "#{label}#{role.inspect} authorization" do + before { kwargs.each { |k, v| allow(described_class).to receive(k).and_return(v) } } + + if role == :suspended_author + let(:author) { suspended_user } let(:user) { author } - when :other_users - let(:user) { create(:user) } else - let(:user) { create(:user, role) } + let(:user) { public_send(role) } end + it { is_expected.to be_falsey } end end end -RSpec.shared_examples "it is otherwise unavailable" do - let(:user) { create(:user) } - let(:author) { create(:user) } - it { is_expected.to be_falsey } -end - -RSpec.shared_examples "when limit_post_creation_to_admins is enabled" do |kwargs| - before { allow(described_class).to receive(:limit_post_creation_to_admins?).and_return(true) } - - Array(kwargs.fetch(:authorizes)).each do |role| - context "when user is #{role.inspect}" do - let(:user) { create(:user, role) } - - it { is_expected.to be_truthy } - end - end - - context "with the \"default\" user" do - it { is_expected.to be_falsey } - end -end - RSpec.describe ArticlePolicy do subject(:method_call) { policy.public_send(policy_method) } + let(:organization) { org_admin&.organizations&.first } + + # The named "folks" with one or more roles + let(:org_admin) { create(:user, :org_admin) } + let(:anyone) { create(:user) } + let(:super_admin) { create(:user, :super_admin) } + let(:suspended_user) { create(:user, :suspended) } + let(:admin) { create(:user, :admin) } + let(:trusted) { create(:user, :trusted) } + let(:other_users) { create(:user) } let(:author) { create(:user) } - let(:organization) { nil } - let(:user) { author } + let(:resource) { build(:article, user: author, organization: organization) } let(:policy) { described_class.new(user, resource) } @@ -111,6 +105,7 @@ RSpec.describe ArticlePolicy do describe "#feed?" do let(:policy_method) { :feed? } + let(:user) { build(:user) } it { is_expected.to be_truthy } end @@ -121,11 +116,24 @@ RSpec.describe ArticlePolicy do it_behaves_like "it requires an authenticated user" it_behaves_like "it requires a user in good standing" - it_behaves_like "permitted roles", to: [:anyone] - it_behaves_like "when limit_post_creation_to_admins is enabled", authorizes: %i[super_admin admin] + it_behaves_like "permitted roles", to: %i[anyone], limit_post_creation_to_admins?: false + it_behaves_like "permitted roles", to: %i[super_admin admin], limit_post_creation_to_admins?: true + it_behaves_like "disallowed roles", to: %i[anyone], limit_post_creation_to_admins?: true end end + describe "#moderate?" do + let(:policy_method) { :moderate? } + + it_behaves_like "it requires a user in good standing" + it_behaves_like "it requires an authenticated user" + + it_behaves_like "permitted roles", to: %i[trusted], limit_post_creation_to_admins?: false + + it_behaves_like "disallowed roles", to: %i[super_admin admin author], limit_post_creation_to_admins?: false + it_behaves_like "disallowed roles", to: %i[trusted], limit_post_creation_to_admins?: true + end + %i[update? edit?].each do |method_name| describe "##{method_name}" do let(:policy_method) { method_name }