From 61554af5e667d457275ee120047b832d6f21a583 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 7 Mar 2022 15:27:18 -0500 Subject: [PATCH] Ensuring the VideoPolicy adhears to article creation (#16762) Videos are a conceptual subset of Articles (see [VideosController#create][1] for supporting "evidence"). As such, we want to ensure they conform to the expectations of the ArticlePolicy (as described in forem/forem#16483). This feature enhancement does a few things: 1. Renames swaps the `VideoPolicy#new?` and `VideoPolicy#create?` declarations (reversing the alias direction) 2. Moves methods out of the concrete ArticlesPolicy into the ApplicationPolicy. 3. Exposes these user oriented methods for external consumers (e.g. making future refactoring work easier). The happy benefit is that this already removes a bit of duplicated logic. 4. Highlights that while yes the Policy is correct, we also may be flirting with the concept of some sort of User "properties as it relates to authorization" logic (good standing users, established users, etc.) 5. Reworked the timing of a guard clause and setting of instance variables. [1]: https://github.com/forem/forem/blob/33e9bac0f74afb4fee0e015ee72887bd076525a6/app/controllers/videos_controller.rb#L18-L22) Relates to #16483 Closes forem/forem#16728 To test this, I'm relying on the existing test suite. I envision that shifting from returning false for suspended users to raising an exception might cause some false positive errors. Note, in the [AppliciationController][2] we handle the responses. Paired with [config/appliciation.rb][3], we handle the policy exceptions. [2]:https://github.com/forem/forem/blob/33e9bac0f74afb4fee0e015ee72887bd076525a6/app/controllers/application_controller.rb#L32 [3]:https://github.com/forem/forem/blob/33e9bac0f74afb4fee0e015ee72887bd076525a6/config/application.rb#L71-L81 --- app/policies/application_policy.rb | 50 +++++++++++++++++++++++- app/policies/article_policy.rb | 14 ------- app/policies/video_policy.rb | 18 +++++++-- spec/policies/application_policy_spec.rb | 38 ++++++++++++++++++ spec/policies/video_policy_spec.rb | 32 ++++++++++++--- 5 files changed, 128 insertions(+), 24 deletions(-) diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 3db502ec7..6ea638297 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -55,15 +55,51 @@ class ApplicationPolicy class UserRequiredError < NotAuthorizedError end + # @param user [Object] the "user" (person?) we're attempting to authorize. + # @return [TrueClass] if we have a "user" (whatever that object might be, we'll assume the callers + # know what they're doing) and that user is not suspended + # + # @raise [ApplicationPolicy::UserSuspendedError] if our user suspended + # @raise [ApplicationPolicy::UserRequiredError] if our given user was "falsey" + # + # @see {ApplicationPolicy.require_user!} + # @note [@jeremyf] I'm choosing to make this a class method (even though later I define an + # instance method) because this question is something that we often ask outside of our + # current policy implementation. By making this class method, I can begin to factor those + # policy implementations to at least a common thing that we use within our policies. + def self.require_user_in_good_standing!(user:) + require_user!(user: user) + + return true unless user.suspended? + + raise ApplicationPolicy::UserSuspendedError, I18n.t("policies.application_policy.your_account_is_suspended") + end + + # @param user [Object] the "user" (person?) we're attempting to authorize. + # @return [TrueClass] if we have a "user" (whatever that object might be, we'll assume the callers + # know what they're doing) + # + # @raise [ApplicationPolicy::UserRequiredError] if our user is "falsey" + # + # @note [@jeremyf] I'm choosing to make this a class method (even though later I define an + # instance method) because this question is something that we often ask outside of our + # current policy implementation. By making this class method, I can begin to factor those + # policy implementations to at least a common thing that we use within our policies. + def self.require_user!(user:) + return true if user + + raise ApplicationPolicy::UserRequiredError, I18n.t("policies.application_policy.you_must_be_logged_in") + end + # @param user [User] who's the one taking the action? # # @param record [Class, Object] what is the user acting on? This could be a model (e.g. Article) # or an instance of a model (e.g. Article.new) or any Plain Old Ruby Object [PORO]. def initialize(user, record) - raise UserRequiredError, I18n.t("policies.application_policy.you_must_be_logged_in") unless user - @user = user @record = record + + require_user! end def index? @@ -130,4 +166,14 @@ class ApplicationPolicy def user_trusted? user.has_trusted_role? end + + protected + + def require_user! + self.class.require_user!(user: user) + end + + def require_user_in_good_standing! + self.class.require_user_in_good_standing!(user: user) + end end diff --git a/app/policies/article_policy.rb b/app/policies/article_policy.rb index 264a8dfe5..7806d5535 100644 --- a/app/policies/article_policy.rb +++ b/app/policies/article_policy.rb @@ -138,20 +138,6 @@ class ArticlePolicy < ApplicationPolicy private - def require_user_in_good_standing! - require_user! - - return true unless user.suspended? - - raise ApplicationPolicy::UserSuspendedError, I18n.t("policies.application_policy.your_account_is_suspended") - end - - def require_user! - return true if user - - raise ApplicationPolicy::UserRequiredError, I18n.t("policies.application_policy.you_must_be_logged_in") - end - def user_author? if record.instance_of?(Article) record.user_id == user.id diff --git a/app/policies/video_policy.rb b/app/policies/video_policy.rb index 5b9ec4ccc..4b20094a7 100644 --- a/app/policies/video_policy.rb +++ b/app/policies/video_policy.rb @@ -1,11 +1,23 @@ class VideoPolicy < ApplicationPolicy - def new? + # @return [Boolean] if the user can :create a video. + # @raise [ApplicationPolicy::UserSuspendedError] if the user's suspended. + # @raise [ApplicationPolicy::UserRequiredError] if the caller did not provide a user. + # + # @todo Consider extracting a user_is_established_in_community as this `user.created_at.before?` + # question rattles around in the code-base. But I think we'd want guidance on configuring + # what that means. [@jeremyf] envisions that we could use an admin setting to allow the + # administrators to set the number of days to consider a "new user". + def create? return false unless Settings::General.enable_video_upload - return false if user.suspended? + + require_user_in_good_standing! return false unless user.created_at + # Newly granted admins get to "short-circuit" the business logic of "were you created too + # recently?" + return user_any_admin? if ArticlePolicy.limit_post_creation_to_admins? user.created_at.before?(2.weeks.ago) end - alias create? new? + alias new? create? end diff --git a/spec/policies/application_policy_spec.rb b/spec/policies/application_policy_spec.rb index 32630fc02..4974b32a6 100644 --- a/spec/policies/application_policy_spec.rb +++ b/spec/policies/application_policy_spec.rb @@ -26,4 +26,42 @@ RSpec.describe ApplicationPolicy do it { is_expected.to be_a(ApplicationPolicy::NotAuthorizedError) } end # rubocop:enable RSpec/DescribedClass + + describe "require_user_in_good_standing!" do + subject(:method_call) { described_class.require_user_in_good_standing!(user: user) } + + context "when no user given" do + let(:user) { nil } + + it { within_block_is_expected.to raise_error ApplicationPolicy::UserRequiredError } + end + + context "when given a user who is not suspended" do + let(:user) { User.new } + + it { is_expected.to be_truthy } + end + + context "when given a user who suspended" do + let(:user) { build(:user, :suspended) } + + it { within_block_is_expected.to raise_error ApplicationPolicy::UserSuspendedError } + end + end + + describe "require_user!" do + subject(:method_call) { described_class.require_user!(user: user) } + + context "when no user given" do + let(:user) { nil } + + it { within_block_is_expected.to raise_error ApplicationPolicy::UserRequiredError } + end + + context "when given a user" do + let(:user) { User.new } + + it { is_expected.to be_truthy } + end + end end diff --git a/spec/policies/video_policy_spec.rb b/spec/policies/video_policy_spec.rb index 97c244176..ddc823196 100644 --- a/spec/policies/video_policy_spec.rb +++ b/spec/policies/video_policy_spec.rb @@ -4,24 +4,28 @@ RSpec.describe VideoPolicy do let(:user) { User.new } let(:policy) { described_class.new(user, nil) } let(:enabled) { true } + let(:limit_post_creation_to_admins) { false } - before { allow(Settings::General).to receive(:enable_video_upload).and_return(enabled) } + before do + allow(Settings::General).to receive(:enable_video_upload).and_return(enabled) + allow(ArticlePolicy).to receive(:limit_post_creation_to_admins?).and_return(limit_post_creation_to_admins) + end - describe "#new?" do - subject(:predicate) { policy.new? } + describe "#create?" do + subject(:predicate) { policy.create? } context "when user is suspended" do let(:enabled) { true } let(:user) { create(:user, :suspended) } - it { is_expected.to be_falsey } + it { within_block_is_expected.to raise_error(ApplicationPolicy::UserSuspendedError) } end context "when user is not signed-in" do let(:enabled) { true } let(:user) { nil } - it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) } + it { within_block_is_expected.to raise_error(ApplicationPolicy::UserRequiredError) } end context "when user has been registered for awhile" do @@ -49,5 +53,23 @@ RSpec.describe VideoPolicy do it { is_expected.to be_falsey } end + + context "when limit_post_creation_to_admins and user not admin" do + let(:limit_post_creation_to_admins) { true } + let(:user) { build(:user) } + + before { user.created_at = 3.weeks.ago } + + it { is_expected.to be_falsey } + end + + context "when limit_post_creation_to_admins and user is a new admin" do + let(:limit_post_creation_to_admins) { true } + let(:user) { build(:user, :admin) } + + before { user.created_at = 1.week.ago } + + it { is_expected.to be_truthy } + end end end