Refactor Ownership policies to cover updating user's own ownership

Make authorization failures more helpful on ownership changes.
We were sometimes returning just the text 'Forbidden' when
really we need to say that you can't edit your own role or
explain the authorization problem, if possible, then redirect
back to the rubygem page.

app/controllers/api/v1/owners_controller.rb

@@ -15,9 +15,7 @@ def show
   def create
     authorize @rubygem, :add_owner?
     owner = User.find_by_name!(email_param)
-    role = params[:role]
-    ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user)
-    ownership.role = role if role.present?
+    ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user, **ownership_params)
 
     if ownership.save
       OwnersMailer.ownership_confirmation(ownership).deliver_later
@@ -30,11 +28,16 @@ def create
   end
 
   def update
-    authorize @rubygem, :update_owner?
-    owner = User.find_by_name!(email_param)
-    ownership = @rubygem.ownerships.find_by!(user: owner)
+    owner = User.find_by_name(email_param)
+    ownership = @rubygem.ownerships.find_by(user: owner) if owner
+    if ownership
+      authorize(ownership)
+    else
+      authorize(@rubygem, :update_owner?) # don't leak presence of an email unless authorized
+      return render_not_found
+    end
 
-    if ownership.update(ownership_update_params)
+    if ownership.update(ownership_params)
       render plain: response_with_mfa_warning("Owner updated successfully.")
     else
       render plain: response_with_mfa_warning(ownership.errors.full_messages.to_sentence), status: :unprocessable_entity
@@ -45,6 +48,7 @@ def destroy
     authorize @rubygem, :remove_owner?
     owner = User.find_by_name!(email_param)
     ownership = @rubygem.ownerships_including_unconfirmed.find_by!(user: owner)
+
     if ownership.safe_destroy
       OwnersMailer.owner_removed(ownership.user_id, @api_key.user.id, ownership.rubygem_id).deliver_later
       render plain: response_with_mfa_warning("Owner removed successfully.")
@@ -76,7 +80,7 @@ def email_param
     params.permit(:email).require(:email)
   end
 
-  def ownership_update_params
+  def ownership_params
     params.permit(:role)
   end
 end

app/controllers/owners_controller.rb

@@ -4,6 +4,11 @@ class OwnersController < ApplicationController
   before_action :find_rubygem, except: :confirm
   verify_session_before only: %i[index edit update create destroy]
   before_action :verify_mfa_requirement, only: %i[create edit update destroy]
+  before_action :find_ownership, only: %i[edit update destroy]
+
+  rescue_from(Pundit::NotAuthorizedError) do |e|
+    redirect_to rubygem_path(@rubygem.slug), alert: e.policy.error
+  end
 
   def confirm
     ownership = Ownership.find_by!(token: token_params)
@@ -33,8 +38,6 @@ def index
   end
 
   def edit
-    authorize @rubygem, :update_owner?
-    @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params)
   end
 
   def create
@@ -54,25 +57,15 @@ def create
   # the role is the only thing that can be updated. If more fields are added to the ownership
   # this action will need to be tweaked a bit
   def update
-    authorize @rubygem, :update_owner?
-    ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params)
-
-    if ownership.user == current_user && update_params[:role] != ownership.role
-      # Don't allow the owner to change the access level of their own ownership
-      return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".update_current_user_role")
-    end
-
-    if ownership.update(update_params)
-      OwnersMailer.with(ownership: ownership, authorizer: current_user).owner_updated.deliver_later
-      redirect_to rubygem_owners_path(ownership.rubygem.slug), notice: t(".success_notice", handle: ownership.user.name)
+    if @ownership.update(update_params)
+      OwnersMailer.with(ownership: @ownership, authorizer: current_user).owner_updated.deliver_later
+      redirect_to rubygem_owners_path(@ownership.rubygem.slug), notice: t(".success_notice", handle: @ownership.user.name)
     else
-      index_with_error ownership.errors.full_messages.to_sentence, :unprocessable_entity
+      index_with_error @ownership.errors.full_messages.to_sentence, :unprocessable_entity
     end
   end
 
   def destroy
-    authorize @rubygem, :remove_owner?
-    @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params)
     if @ownership.safe_destroy
       OwnersMailer.owner_removed(@ownership.user_id, current_user.id, @ownership.rubygem_id).deliver_later
       redirect_to rubygem_owners_path(@ownership.rubygem.slug), notice: t(".removed_notice", owner_name: @ownership.owner_name)
@@ -87,6 +80,15 @@ def verify_session_redirect_path
     rubygem_owners_url(params[:rubygem_id])
   end
 
+  def find_ownership
+    @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle(handle_params)
+    return authorize(@ownership) if @ownership
+
+    predicate = params[:action] == "destroy" ? :remove_owner? : :update_owner?
+    authorize(@rubygem, predicate)
+    render_not_found
+  end
+
   def token_params
     params.permit(:token).require(:token)
   end

app/models/ownership.rb

@@ -32,8 +32,8 @@ def self.by_indexed_gem_name
       .order("rubygems.name ASC")
   end
 
-  def self.find_by_owner_handle!(handle)
-    joins(:user).find_by(users: { handle: handle }) || joins(:user).find_by!(users: { id: handle })
+  def self.find_by_owner_handle(handle)
+    joins(:user).find_by(users: { handle: handle }) || joins(:user).find_by(users: { id: handle })
   end
 
   def self.create_confirmed(rubygem, user, approver)

app/policies/api/application_policy.rb

@@ -42,10 +42,19 @@ def deny(error)
     false
   end
 
+  def api_policy!(record)
+    Pundit.policy!(api_key, [:api, record])
+  end
+
   def user_policy!(record)
     Pundit.policy!(api_key.user, record)
   end
 
+  def api_authorized?(record, action)
+    policy = api_policy!(record)
+    policy.send(action) || deny(policy.error)
+  end
+
   def user_authorized?(record, action)
     policy = user_policy!(record)
     policy.send(action) || deny(policy.error)

app/policies/api/ownership_policy.rb

@@ -0,0 +1,11 @@
+class Api::OwnershipPolicy < Api::ApplicationPolicy
+  class Scope < Api::ApplicationPolicy::Scope
+  end
+
+  delegate :rubygem, to: :record
+
+  def update?
+    api_authorized?(rubygem, :update_owner?) &&
+      user_authorized?(record, :update?)
+  end
+end

app/policies/api/rubygem_policy.rb

@@ -26,18 +26,18 @@ def add_owner?
       user_authorized?(rubygem, :add_owner?)
   end
 
-  def remove_owner?
+  def update_owner?
     user_api_key? &&
       mfa_requirement_satisfied?(rubygem) &&
-      api_key_scope?(:remove_owner, rubygem) &&
-      user_authorized?(rubygem, :remove_owner?)
+      api_key_scope?(:update_owner, rubygem) &&
+      user_authorized?(rubygem, :update_owner?)
   end
 
-  def update_owner?
+  def remove_owner?
     user_api_key? &&
       mfa_requirement_satisfied?(rubygem) &&
-      api_key_scope?(:update_owner, rubygem) &&
-      user_authorized?(rubygem, :update_owner?)
+      api_key_scope?(:remove_owner, rubygem) &&
+      user_authorized?(rubygem, :remove_owner?)
   end
 
   def configure_trusted_publishers?

app/policies/ownership_policy.rb

@@ -9,8 +9,10 @@ def create?
   end
 
   def update?
+    return deny(t("owners.update.update_current_user_role")) if current_user?(record.user)
     policy!(user, rubygem).update_owner?
   end
+  alias edit? update?
 
   def destroy?
     policy!(user, rubygem).remove_owner?

app/views/owners_mailer/ownership_confirmation.html.erb

@@ -29,7 +29,7 @@
                       </td>
                       <td bgcolor="#e9573f">
                         <div class="text-btn" style="color:#ffffff; font-family:Arial, sans-serif; min-width:auto !important; font-size:16px; line-height:20px; text-align:center">
-                          <a href="<%= rubygem_owners_url(@rubygem.slug, token: @ownership.token) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">VERIFY</span></a>
+                          <a href="<%= confirm_rubygem_owners_url(@rubygem.slug, token: @ownership.token) %>" target="_blank" rel="noopener" class="link-white" style="color:#ffffff; text-decoration:none"><span class="link-white" style="color:#ffffff; text-decoration:none">VERIFY</span></a>
                         </div>
                       </td>
                       <td class="img" style="font-size:0pt; line-height:0pt; text-align:left" width="15"></td>

test/functional/api/v1/owners_controller_test.rb

@@ -975,43 +975,65 @@ def self.should_respond_to(format)
     setup do
       @owner = create(:user)
       @maintainer = create(:user)
-      @rubygem = create(:rubygem)
-
-      @owner_gem_ownership = create(:ownership, user: @owner, rubygem: @rubygem, role: :owner)
-      @maintainer_gem_ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, role: :maintainer)
+      @rubygem = create(:rubygem, owners: [@owner])
 
       @api_key = create(:api_key, key: "12223", scopes: %i[update_owner], owner: @owner, rubygem: @rubygem)
       @request.env["HTTP_AUTHORIZATION"] = "12223"
     end
 
     should "set the maintainer to a lower access level" do
+      ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, role: :owner)
+
       patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :maintainer }
 
       assert_response :success
-      assert_predicate @maintainer_gem_ownership.reload, :maintainer?
+      assert_predicate ownership.reload, :maintainer?
+      assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership }
     end
 
-    should "schedule an email for the updated user" do
-      patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :owner }
+    context "when the current user is changing their own role" do
+      should "forbid changing the role" do
+        patch :update, params: { rubygem_id: @rubygem.slug, email: @owner.email, role: :maintainer }
 
-      ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer)
+        ownership = @rubygem.ownerships.find_by(user: @owner)
 
-      assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership }
+        assert_response :forbidden
+        assert_predicate ownership.reload, :owner?
+      end
     end
 
     context "when the role is invalid" do
       should "return a bad request response with the error message" do
-        patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid }
+        ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, role: :maintainer)
 
-        respond_with :bad_request
+        patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid }
 
+        assert_response :unprocessable_entity
         assert_equal "Role is not included in the list", @response.body
+        assert_predicate ownership.reload, :maintainer?
       end
+    end
 
-      should "not update the user with the new role" do
-        patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid }
+    context "when the owner is not found" do
+      context "when the update is authorized" do
+        should "return a not found response" do
+          patch :update, params: { rubygem_id: @rubygem.slug, email: "notauser", role: :owner }
+
+          assert_response :not_found
+          assert_equal "Owner could not be found.", @response.body
+        end
+      end
 
-        assert_predicate @maintainer_gem_ownership.reload, :maintainer?
+      context "when the update is not authorized" do
+        should "return a forbidden response" do
+          @api_key = create(:api_key, key: "99999", scopes: %i[push_rubygem], owner: @owner)
+          @request.env["HTTP_AUTHORIZATION"] = "99999"
+
+          patch :update, params: { rubygem_id: @rubygem.slug, email: "notauser", role: :owner }
+
+          assert_response :forbidden
+          assert_equal "This API key cannot perform the specified action on this gem.", @response.body
+        end
       end
     end
   end