Refactor access to rubygem in Policies, cleanup policies. (#4833)

Clean up how the forbidden message is sent from pundit
Remove unused methods

app/controllers/application_controller.rb

@@ -9,7 +9,7 @@ class ApplicationController < ActionController::Base
   rescue_from ActiveRecord::RecordNotFound, with: :render_not_found
   rescue_from ActionController::InvalidAuthenticityToken, with: :render_forbidden
   rescue_from ActionController::UnpermittedParameters, with: :render_bad_request
-  rescue_from Pundit::NotAuthorizedError, with: :render_forbidden
+  rescue_from(Pundit::NotAuthorizedError) { |_| render_forbidden } # don't pass pundit error message
 
   before_action :set_locale
   before_action :reject_null_char_param
@@ -139,7 +139,6 @@ def render_not_found
   end
 
   def render_forbidden(error = "forbidden")
-    error = "forbidden" if error.is_a?(Pundit::NotAuthorizedError)
     render plain: error, status: :forbidden
   end
 

app/policies/application_policy.rb

@@ -56,4 +56,10 @@ def destroy?
   def search?
     index?
   end
+
+  private
+
+  def current_user?(record_user)
+    user && user == record_user
+  end
 end

app/policies/events/rubygem_event_policy.rb

@@ -1,12 +1,11 @@
 class Events::RubygemEventPolicy < ApplicationPolicy
   class Scope < ApplicationPolicy::Scope
-    def resolve
-      scope.none
-    end
   end
 
+  delegate :rubygem, to: :record
+
   def show?
-    record.rubygem.owned_by?(user)
+    rubygem.owned_by?(user)
   end
 
   def create?

app/policies/oidc/pending_trusted_publisher_policy.rb

@@ -6,14 +6,14 @@ def resolve
   end
 
   def show?
-    record.user == user
+    current_user?(record.user)
   end
 
   def create?
-    record.user == user
+    current_user?(record.user)
   end
 
   def destroy?
-    record.user == user
+    current_user?(record.user)
   end
 end

app/policies/oidc/rubygem_trusted_publisher_policy.rb

@@ -1,19 +1,18 @@
 class OIDC::RubygemTrustedPublisherPolicy < ApplicationPolicy
   class Scope < ApplicationPolicy::Scope
-    def resolve
-      scope.none
-    end
   end
 
+  delegate :rubygem, to: :record
+
   def show?
-    record.rubygem.owned_by?(user)
+    rubygem.owned_by?(user)
   end
 
   def create?
-    record.rubygem.owned_by?(user)
+    rubygem.owned_by?(user)
   end
 
   def destroy?
-    record.rubygem.owned_by?(user)
+    rubygem.owned_by?(user)
   end
 end

app/policies/ownership_call_policy.rb

@@ -1,15 +1,14 @@
 class OwnershipCallPolicy < ApplicationPolicy
   class Scope < ApplicationPolicy::Scope
-    def resolve
-      scope.none
-    end
   end
 
+  delegate :rubygem, to: :record
+
   def create?
-    record.rubygem.owned_by?(user) && record.user == user
+    rubygem.owned_by?(user) && current_user?(record.user)
   end
 
   def close?
-    record.rubygem.owned_by?(user)
+    rubygem.owned_by?(user)
   end
 end

app/policies/ownership_policy.rb

@@ -1,15 +1,14 @@
 class OwnershipPolicy < ApplicationPolicy
   class Scope < ApplicationPolicy::Scope
-    def resolve
-      scope.none # unused
-    end
   end
 
+  delegate :rubygem, to: :record
+
   def create?
-    record.rubygem.owned_by?(user) && record.authorizer == user
+    rubygem.owned_by?(user) && current_user?(record.authorizer)
   end
 
   def destroy?
-    record.rubygem.owned_by?(user)
+    rubygem.owned_by?(user)
   end
 end

app/policies/ownership_request_policy.rb

@@ -1,20 +1,18 @@
 class OwnershipRequestPolicy < ApplicationPolicy
   class Scope < ApplicationPolicy::Scope
-    def resolve
-      scope.none
-    end
   end
 
+  delegate :rubygem, to: :record
+
   def create?
-    return false unless record.user == user
-    Pundit.policy!(user, record.rubygem).request_ownership?
+    current_user?(record.user) && Pundit.policy!(user, rubygem).request_ownership?
   end
 
   def approve?
-    record.rubygem.owned_by?(user)
+    rubygem.owned_by?(user)
   end
 
   def close?
-    (user.present? && record.user == user) || record.rubygem.owned_by?(user)
+    current_user?(record.user) || rubygem.owned_by?(user)
   end
 end

app/policies/rubygem_policy.rb

@@ -1,8 +1,5 @@
 class RubygemPolicy < ApplicationPolicy
   class Scope < ApplicationPolicy::Scope
-    def resolve
-      scope.none # unused
-    end
   end
 
   ABANDONED_RELEASE_AGE = 1.year

test/policies/events/rubygem_event_policy_test.rb

@@ -8,11 +8,6 @@ class Events::RubygemEventPolicyTest < ActiveSupport::TestCase
     @user = FactoryBot.create(:user)
   end
 
-  def test_scope
-    assert_empty Pundit.policy_scope!(@owner, Events::RubygemEvent).to_a
-    assert_empty Pundit.policy_scope!(@owner, @rubygem.events).to_a
-  end
-
   def test_show
     assert_predicate Pundit.policy!(@owner, @event), :show?
     refute_predicate Pundit.policy!(@user, @event), :show?

test/policies/oidc/rubygem_trusted_publisher_policy_test.rb

@@ -8,11 +8,6 @@ class OIDC::RubygemTrustedPublisherPolicyTest < ActiveSupport::TestCase
     @user = FactoryBot.create(:user)
   end
 
-  def test_scope
-    assert_empty Pundit.policy_scope!(@owner, OIDC::RubygemTrustedPublisher).to_a
-    assert_empty Pundit.policy_scope!(@owner, @rubygem.oidc_rubygem_trusted_publishers).to_a
-  end
-
   def test_show
     assert_predicate Pundit.policy!(@owner, @trusted_publisher), :show?
     refute_predicate Pundit.policy!(@user, @trusted_publisher), :show?

test/policies/ownership_call_policy_test.rb

@@ -9,13 +9,6 @@ class OwnershipCallPolicyTest < ActiveSupport::TestCase
     @user = FactoryBot.create(:user)
   end
 
-  def test_scope
-    # Tests that nothing is returned currently because scope is unused
-    assert_empty Pundit.policy_scope!(@authorizer, OwnershipCall).to_a
-    assert_empty Pundit.policy_scope!(@invited, OwnershipCall).to_a
-    assert_empty Pundit.policy_scope!(@user, OwnershipCall).to_a
-  end
-
   def test_create
     assert_predicate Pundit.policy!(@owner, @ownership_call), :create?
     refute_predicate Pundit.policy!(@user, @ownership_call), :create?

test/policies/ownership_policy_test.rb

@@ -11,13 +11,6 @@ class OwnershipPolicyTest < ActiveSupport::TestCase
     @user = FactoryBot.create(:user)
   end
 
-  def test_scope
-    # Tests that nothing is returned currently because scope is unused
-    assert_empty Pundit.policy_scope!(@authorizer, Ownership).to_a
-    assert_empty Pundit.policy_scope!(@invited, Ownership).to_a
-    assert_empty Pundit.policy_scope!(@user, Ownership).to_a
-  end
-
   def test_create
     assert_predicate Pundit.policy!(@authorizer, @unconfirmed_ownership), :create?
     refute_predicate Pundit.policy!(@invited, @unconfirmed_ownership), :create?

test/policies/ownership_request_policy_test.rb

@@ -14,35 +14,42 @@ class OwnershipRequestPolicyTest < ActiveSupport::TestCase
     @ownership_request = create(:ownership_request, rubygem: @rubygem, user: @requester)
   end
 
-  def test_scope
-    assert_empty Pundit.policy_scope!(@owner, OwnershipCall).to_a
-    assert_empty Pundit.policy_scope!(@owner, @rubygem.ownership_requests).to_a
-    assert_empty Pundit.policy_scope!(@requester, OwnershipCall).to_a
-    assert_empty Pundit.policy_scope!(@user, OwnershipCall).to_a
+  context "#create?" do
+    should "allow the requester to create when the gem is considered abandoned" do
+      assert_predicate Pundit.policy!(@requester, @ownership_request), :create?
+      refute_predicate Pundit.policy!(@owner, @ownership_request), :create?
+      refute_predicate Pundit.policy!(@user, @ownership_request), :create?
+    end
+
+    should "not allow the requester to create when the gem is not considered abandoned" do
+      newgem = create(:rubygem, number: "1.0", owners: [@owner])
+      newgem_request = build(:ownership_request, rubygem: newgem, user: @requester)
+
+      refute_predicate Pundit.policy!(@requester, newgem_request), :create?
+      refute_predicate Pundit.policy!(@owner, newgem_request), :create?
+      refute_predicate Pundit.policy!(@user, newgem_request), :create?
+    end
   end
 
-  def test_create
-    assert_predicate Pundit.policy!(@requester, @ownership_request), :create?
-    refute_predicate Pundit.policy!(@owner, @ownership_request), :create?
-    refute_predicate Pundit.policy!(@user, @ownership_request), :create?
-
-    newgem = create(:rubygem, number: "1.0", owners: [@owner])
-    newgem_request = build(:ownership_request, rubygem: newgem, user: @requester)
-
-    refute_predicate Pundit.policy!(@requester, newgem_request), :create?
-    refute_predicate Pundit.policy!(@owner, newgem_request), :create?
-    refute_predicate Pundit.policy!(@user, newgem_request), :create?
+  context "#approve?" do
+    should "only allow the owner to approve" do
+      refute_predicate Pundit.policy!(@requester, @ownership_request), :approve?
+      assert_predicate Pundit.policy!(@owner, @ownership_request), :approve?
+      refute_predicate Pundit.policy!(@user, @ownership_request), :approve?
+    end
   end
 
-  def test_approve
-    refute_predicate Pundit.policy!(@requester, @ownership_request), :approve?
-    assert_predicate Pundit.policy!(@owner, @ownership_request), :approve?
-    refute_predicate Pundit.policy!(@user, @ownership_request), :approve?
-  end
+  context "#close?" do
+    should "allow the requester to close" do
+      assert_predicate Pundit.policy!(@requester, @ownership_request), :close?
+    end
+
+    should "allow the owner to close" do
+      assert_predicate Pundit.policy!(@owner, @ownership_request), :close?
+    end
 
-  def test_close
-    assert_predicate Pundit.policy!(@requester, @ownership_request), :close?
-    assert_predicate Pundit.policy!(@owner, @ownership_request), :close?
-    refute_predicate Pundit.policy!(@user, @ownership_request), :close?
+    should "not allow other users to close" do
+      refute_predicate Pundit.policy!(@user, @ownership_request), :close?
+    end
   end
 end

test/policies/rubygem_policy_test.rb

@@ -2,20 +2,9 @@
 
 class RubygemPolicyTest < ActiveSupport::TestCase
   setup do
-    @owner = create(:user)
+    @owner = create(:user, handle: "owner")
     @rubygem = create(:rubygem, owners: [@owner])
-    @user = create(:user)
-  end
-
-  def test_scope
-    # Tests that nothing is returned currently because scope is unused
-    assert_empty Pundit.policy_scope!(@owner, Rubygem).to_a
-    assert_empty Pundit.policy_scope!(@user, Rubygem).to_a
-  end
-
-  def test_show
-    assert_predicate Pundit.policy!(@owner, @rubygem), :show?
-    assert_predicate Pundit.policy!(nil, @rubygem), :show?
+    @user = create(:user, handle: "user")
   end
 
   context "#request_ownership?" do