Skip to content

Commit

Permalink
Replace ownership checks in views with policy checks
Browse files Browse the repository at this point in the history
  • Loading branch information
martinemde authored and simi committed Sep 1, 2024
1 parent e0f3df2 commit 93e5e10
Show file tree
Hide file tree
Showing 10 changed files with 57 additions and 37 deletions.
2 changes: 1 addition & 1 deletion app/controllers/adoptions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/ownership_requests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions app/policies/ownership_call_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
32 changes: 18 additions & 14 deletions app/policies/rubygem_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 3 additions & 3 deletions app/views/adoptions/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<% content_for :title do %>
<h1 class="t-display page__heading page__heading--small">
<%= t('.title') %>
<% if @rubygem.owned_by?(current_user) %>
<% if policy(@rubygem).manage_adoption? %>
<i class="page__subheading page__subheading--block"><%= t(".subtitle_owner_html", gem: @rubygem.name) %></i>
<% else %>
<i class="page__subheading page__subheading--block"><%= t(".subtitle_user_html", gem: @rubygem.name) %></i>
Expand All @@ -24,12 +24,12 @@
<strong><%= t("ownership_calls.created_by") %>:</strong>
<%= link_to @ownership_call.user_display_handle, profile_path(@ownership_call.user), class: "t-text t-link" %>
</p>
<% 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 %>
</div>

<% elsif @rubygem.owned_by?(current_user) %>
<% elsif policy(@rubygem).manage_adoption? %>
<%= render partial: "ownership_calls/form", locals: { gem: @rubygem.name } %>
<% else %>
<div class="t-list__item">
Expand Down
2 changes: 1 addition & 1 deletion app/views/ownership_calls/_close.html.erb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<% if ownership_call.rubygem.owned_by?(current_user) %>
<% if policy(ownership_call).close? %>
<div><%= button_to t("ownership_calls.close"), close_rubygem_ownership_calls_path(ownership_call.rubygem.slug), method: :patch, class: "form__submit form__submit--medium" %></div>
<% end %>
2 changes: 1 addition & 1 deletion app/views/ownership_requests/_list.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
</h2>
</div>

<% if @rubygem.owned_by?(current_user) %>
<% if policy(@rubygem).manage_adoption? %>
<div class="t-list__items">
<%= 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) %>
</div>
Expand Down
8 changes: 4 additions & 4 deletions app/views/rubygems/_aside.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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? %>
</div>
</div>
2 changes: 1 addition & 1 deletion test/policies/ownership_call_policy_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 24 additions & 8 deletions test/policies/rubygem_policy_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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?
Expand Down

0 comments on commit 93e5e10

Please sign in to comment.