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]:
33e9bac0f7/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]:33e9bac0f7/app/controllers/application_controller.rb (L32)
[3]:33e9bac0f7/config/application.rb (L71-L81)

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

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

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

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

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