From 93e5e10283fe9c15555ed29b39c579ecf3a99dec Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Thu, 18 Jul 2024 20:05:14 -0700 Subject: [PATCH] Replace ownership checks in views with policy checks --- app/controllers/adoptions_controller.rb | 2 +- .../ownership_requests_controller.rb | 4 +-- app/policies/ownership_call_policy.rb | 4 +-- app/policies/rubygem_policy.rb | 32 +++++++++++-------- app/views/adoptions/index.html.erb | 6 ++-- app/views/ownership_calls/_close.html.erb | 2 +- app/views/ownership_requests/_list.html.erb | 2 +- app/views/rubygems/_aside.html.erb | 8 ++--- test/policies/ownership_call_policy_test.rb | 2 +- test/policies/rubygem_policy_test.rb | 32 ++++++++++++++----- 10 files changed, 57 insertions(+), 37 deletions(-) diff --git a/app/controllers/adoptions_controller.rb b/app/controllers/adoptions_controller.rb index 77f4d09aebf..8ee0d1cbdf1 100644 --- a/app/controllers/adoptions_controller.rb +++ b/app/controllers/adoptions_controller.rb @@ -2,7 +2,7 @@ class AdoptionsController < ApplicationController include SessionVerifiable before_action :find_rubygem - before_action :redirect_to_verify, if: -> { @rubygem.owned_by?(current_user) && !verified_session_active? } + before_action :redirect_to_verify, if: -> { policy(@rubygem).manage_adoption? && !verified_session_active? } def index @ownership_call = @rubygem.ownership_call diff --git a/app/controllers/ownership_requests_controller.rb b/app/controllers/ownership_requests_controller.rb index 71f9c9109d8..48d8f887e05 100644 --- a/app/controllers/ownership_requests_controller.rb +++ b/app/controllers/ownership_requests_controller.rb @@ -5,7 +5,7 @@ class OwnershipRequestsController < ApplicationController before_action :redirect_to_signin, unless: :signed_in? before_action :redirect_to_new_mfa, if: :mfa_required_not_yet_enabled? before_action :redirect_to_settings_strong_mfa_required, if: :mfa_required_weak_level_enabled? - before_action :redirect_to_verify, only: %i[update close_all], if: -> { @rubygem.owned_by?(current_user) && !verified_session_active? } + before_action :redirect_to_verify, only: %i[update close_all], if: -> { policy(@rubygem).manage_adoption? && !verified_session_active? } rescue_from ActiveRecord::RecordInvalid, with: :redirect_try_again rescue_from ActiveRecord::RecordNotSaved, with: :redirect_try_again @@ -34,7 +34,7 @@ def update end def close_all - authorize(@rubygem, :close_ownership_requests?).ownership_requests.each(&:close!) + authorize(@rubygem, :manage_adoption?).ownership_requests.each(&:close!) redirect_to rubygem_adoptions_path(@rubygem.slug), notice: t("ownership_requests.close.success_notice", gem: @rubygem.name) end diff --git a/app/policies/ownership_call_policy.rb b/app/policies/ownership_call_policy.rb index eae5d07be29..dafeff928cc 100644 --- a/app/policies/ownership_call_policy.rb +++ b/app/policies/ownership_call_policy.rb @@ -5,10 +5,10 @@ class Scope < ApplicationPolicy::Scope delegate :rubygem, to: :record def create? - rubygem_owned_by?(user) && current_user?(record.user) + user_authorized?(rubygem, :manage_adoption?) end def close? - rubygem_owned_by?(user) + user_authorized?(rubygem, :manage_adoption?) end end diff --git a/app/policies/rubygem_policy.rb b/app/policies/rubygem_policy.rb index 221c6d8d048..56a5c02ec58 100644 --- a/app/policies/rubygem_policy.rb +++ b/app/policies/rubygem_policy.rb @@ -23,38 +23,42 @@ def destroy? false end - def show_adoption? - rubygem_owned_by?(user) || request_ownership? + def add_owner? + rubygem_owned_by?(user) end - def show_events? + def configure_oidc? rubygem_owned_by?(user) end - def request_ownership? - return allow if rubygem.ownership_calls.any? - return false if rubygem.downloads >= ABANDONED_DOWNLOADS_MAX - return false if rubygem.latest_version.nil? || rubygem.latest_version.created_at.after?(ABANDONED_RELEASE_AGE.ago) - allow + def configure_trusted_publishers? + rubygem_owned_by?(user) end - def close_ownership_requests? + def manage_adoption? rubygem_owned_by?(user) end - def configure_trusted_publishers? + def remove_owner? rubygem_owned_by?(user) end - def show_unconfirmed_ownerships? - rubygem_owned_by?(user) + def request_ownership? + return allow if rubygem.ownership_calls.any? + return false if rubygem.downloads >= ABANDONED_DOWNLOADS_MAX + return false if rubygem.latest_version.nil? || rubygem.latest_version.created_at.after?(ABANDONED_RELEASE_AGE.ago) + allow end - def add_owner? + def show_adoption? + manage_adoption? || request_ownership? + end + + def show_events? rubygem_owned_by?(user) end - def remove_owner? + def show_unconfirmed_ownerships? rubygem_owned_by?(user) end end diff --git a/app/views/adoptions/index.html.erb b/app/views/adoptions/index.html.erb index 48afcc14296..7d6ae93012f 100644 --- a/app/views/adoptions/index.html.erb +++ b/app/views/adoptions/index.html.erb @@ -3,7 +3,7 @@ <% content_for :title do %>

<%= t('.title') %> - <% if @rubygem.owned_by?(current_user) %> + <% if policy(@rubygem).manage_adoption? %> <%= t(".subtitle_owner_html", gem: @rubygem.name) %> <% else %> <%= t(".subtitle_user_html", gem: @rubygem.name) %> @@ -24,12 +24,12 @@ <%= t("ownership_calls.created_by") %>: <%= link_to @ownership_call.user_display_handle, profile_path(@ownership_call.user), class: "t-text t-link" %>

- <% if @ownership_call.rubygem.owned_by?(current_user) %> + <% if policy(@ownership_call).close? %> <%= button_to t("ownership_calls.close"), close_rubygem_ownership_calls_path(@ownership_call.rubygem.slug), method: :patch, class: "form__submit form__submit--medium" %> <% end %> -<% elsif @rubygem.owned_by?(current_user) %> +<% elsif policy(@rubygem).manage_adoption? %> <%= render partial: "ownership_calls/form", locals: { gem: @rubygem.name } %> <% else %>
diff --git a/app/views/ownership_calls/_close.html.erb b/app/views/ownership_calls/_close.html.erb index af7ddc1001f..1d9b5e119a4 100644 --- a/app/views/ownership_calls/_close.html.erb +++ b/app/views/ownership_calls/_close.html.erb @@ -1,3 +1,3 @@ -<% if ownership_call.rubygem.owned_by?(current_user) %> +<% if policy(ownership_call).close? %>
<%= button_to t("ownership_calls.close"), close_rubygem_ownership_calls_path(ownership_call.rubygem.slug), method: :patch, class: "form__submit form__submit--medium" %>
<% end %> diff --git a/app/views/ownership_requests/_list.html.erb b/app/views/ownership_requests/_list.html.erb index a0cdd088204..4481cf4ec4a 100644 --- a/app/views/ownership_requests/_list.html.erb +++ b/app/views/ownership_requests/_list.html.erb @@ -4,7 +4,7 @@

-<% if @rubygem.owned_by?(current_user) %> +<% if policy(@rubygem).manage_adoption? %>
<%= render(partial: "ownership_requests/owner", layout: "ownership_requests/ownership_request", collection: @ownership_requests, as: :ownership_request, locals: { show_user: true, show_gem: false }) || t("ownership_requests.no_ownership_requests", gem: @rubygem.name) %>
diff --git a/app/views/rubygems/_aside.html.erb b/app/views/rubygems/_aside.html.erb index 9073ea0e1ee..9e011629aaf 100644 --- a/app/views/rubygems/_aside.html.erb +++ b/app/views/rubygems/_aside.html.erb @@ -74,11 +74,11 @@ <%= atom_link(@rubygem) %> <%= report_abuse_link(@rubygem) %> <%= reverse_dependencies_link(@rubygem) %> - <%= ownership_link(@rubygem) if @rubygem.owned_by?(current_user) %> - <%= rubygem_trusted_publishers_link(@rubygem) if @rubygem.owned_by?(current_user) %> - <%= oidc_api_key_role_links(@rubygem) if @rubygem.owned_by?(current_user) %> + <%= ownership_link(@rubygem) if policy(@rubygem).show_unconfirmed_ownerships? %> + <%= rubygem_trusted_publishers_link(@rubygem) if policy(@rubygem).configure_trusted_publishers? %> + <%= oidc_api_key_role_links(@rubygem) if policy(@rubygem).configure_oidc? %> <%= resend_owner_confirmation_link(@rubygem) if @rubygem.unconfirmed_ownership?(current_user) %> <%= rubygem_adoptions_link(@rubygem) if policy(@rubygem).show_adoption? %> - <%= rubygem_security_events_link(@rubygem) if @rubygem.owned_by?(current_user) %> + <%= rubygem_security_events_link(@rubygem) if policy(@rubygem).show_events? %> diff --git a/test/policies/ownership_call_policy_test.rb b/test/policies/ownership_call_policy_test.rb index 33138e98acf..9bd3c3c93a9 100644 --- a/test/policies/ownership_call_policy_test.rb +++ b/test/policies/ownership_call_policy_test.rb @@ -18,7 +18,7 @@ def test_create refute_authorized @user, :create? end - def test_destroy + def test_close assert_authorized @owner, :close? refute_authorized @user, :close? end diff --git a/test/policies/rubygem_policy_test.rb b/test/policies/rubygem_policy_test.rb index 25b4d473d55..c5cefe7a214 100644 --- a/test/policies/rubygem_policy_test.rb +++ b/test/policies/rubygem_policy_test.rb @@ -11,6 +11,30 @@ def policy!(user) Pundit.policy!(user, @rubygem) end + context "#configure_oidc?" do + should "only allow the owner" do + assert_authorized @owner, :configure_oidc? + refute_authorized @user, :configure_oidc? + refute_authorized nil, :configure_oidc? + end + end + + context "#configure_trusted_publishers?" do + should "only allow the owner" do + assert_authorized @owner, :configure_trusted_publishers? + refute_authorized @user, :configure_trusted_publishers? + refute_authorized nil, :configure_trusted_publishers? + end + end + + context "#manage_adoption?" do + should "only allow the owner" do + assert_authorized @owner, :manage_adoption? + refute_authorized @user, :manage_adoption? + refute_authorized nil, :manage_adoption? + end + end + context "#request_ownership?" do should "be true if the gem has ownership calls" do create(:ownership_call, rubygem: @rubygem, user: @owner) @@ -64,14 +88,6 @@ def policy!(user) end end - context "#configure_trusted_publishers?" do - should "only allow the owner" do - assert_authorized @owner, :configure_trusted_publishers? - refute_authorized @user, :configure_trusted_publishers? - refute_authorized nil, :configure_trusted_publishers? - end - end - context "#show_unconfirmed_ownerships?" do should "only allow the owner" do assert_authorized @owner, :show_unconfirmed_ownerships?