Skip to content

Commit

Permalink
Move BsRequest is_target_maintainer? authorization to pundit
Browse files Browse the repository at this point in the history
  • Loading branch information
krauselukas committed Mar 14, 2024
1 parent f37741f commit ecdc872
Show file tree
Hide file tree
Showing 8 changed files with 13 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
= form_tag({ action: 'changerequest' }, id: 'request_handle_form') do
= hidden_field_tag(:number, @bs_request.number)
= text_area_tag(:reason, nil, placeholder: 'Please explain your decision...', rows: 4, class: 'w-100 form-control mb-2')
- if single_action_request && @is_target_maintainer && @bs_request.state.in?([:new, :review])
- if single_action_request && policy(@bs_request).target_maintainer? && @bs_request.state.in?([:new, :review])
- if show_add_submitter_as_maintainer_option?
.form-check.mb-2
= check_box_tag('add_submitter_as_maintainer_0', "#{@action[:tprj]}_#_#{@action[:tpkg]}", false, class: 'form-check-input')
Expand Down
5 changes: 2 additions & 3 deletions src/api/app/components/request_decision_component.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
class RequestDecisionComponent < ApplicationComponent
def initialize(bs_request:, action:, is_target_maintainer:)
def initialize(bs_request:, action:)
super

@bs_request = bs_request
@is_target_maintainer = is_target_maintainer
@action = action
end

Expand All @@ -29,7 +28,7 @@ def show_add_submitter_as_maintainer_option?

# TODO: Move all those "can_*" checks to a pundit policy
def can_accept_request?
@bs_request.state.in?(%i[new review]) && @is_target_maintainer
@bs_request.state.in?(%i[new review]) && policy(@bs_request).target_maintainer?
end

def can_reopen_request?
Expand Down
3 changes: 1 addition & 2 deletions src/api/app/controllers/webui/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def show
@diff_to_superseded_id = params[:diff_to_superseded]
@is_author = @bs_request.creator == User.possibly_nobody.login

@is_target_maintainer = @bs_request.is_target_maintainer?(User.session)
@is_target_maintainer = BsRequestPolicy.new(User.session, @bs_request).target_maintainer?
@can_handle_request = @bs_request.state.in?(%i[new review declined]) && (@is_target_maintainer || @is_author)

@history = @bs_request.history_elements.includes(:user)
Expand Down Expand Up @@ -534,7 +534,6 @@ def handle_notification
end

def prepare_request_data
@is_target_maintainer = @bs_request.is_target_maintainer?(User.session)
@my_open_reviews = ReviewsFinder.new(@bs_request.reviews).open_reviews_for_user(User.session).reject(&:staging_project?)

@diff_limit = params[:full_diff] ? 0 : nil
Expand Down
5 changes: 0 additions & 5 deletions src/api/app/models/bs_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -907,11 +907,6 @@ def auto_accept
end
end

# Check if 'user' is maintainer in _all_ request targets:
def is_target_maintainer?(user)
bs_request_actions.all? { |action| action.is_target_maintainer?(user) }
end

def sanitize!
# apply default values, expand and do permission checks
self.creator ||= User.session!.login
Expand Down
10 changes: 6 additions & 4 deletions src/api/app/policies/bs_request_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ def create?
end

def handle_request?
is_target_maintainer = record.is_target_maintainer?(user)
record.state.in?(%i[new review declined]) && (is_target_maintainer || author?)
record.state.in?(%i[new review declined]) && (target_maintainer? || author?)
end

def add_reviews?
is_target_maintainer = record.is_target_maintainer?(user)
has_open_reviews = record.reviews.where(state: 'new').any? { |review| review.matches_user?(user) }
record.state.in?(%i[new review]) && (author? || is_target_maintainer || has_open_reviews.present?)
record.state.in?(%i[new review]) && (author? || target_maintainer? || has_open_reviews.present?)
end

def revoke_request?
Expand All @@ -31,6 +29,10 @@ def decline_request?
!author?
end

def target_maintainer?
record.bs_request_actions.all? { |action| action.is_target_maintainer?(user) }
end

private

def author?
Expand Down
2 changes: 1 addition & 1 deletion src/api/app/policies/comment_lock_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def create?
return record.maintainers.include?(user)
# Request receivers (maintainers of target package) can also lock comments
when BsRequest
return record.is_target_maintainer?(user)
return BsRequestPolicy.new(user, record).target_maintainer?
when BsRequestAction
return record.bs_request.is_target_maintainer?(user)
end
Expand Down
2 changes: 1 addition & 1 deletion src/api/app/policies/comment_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def maintainer?
when 'Project'
user.has_local_permission?('change_project', record.commentable)
when 'BsRequest'
record.commentable.is_target_maintainer?(user)
BsRequestPolicy.new(user, record.commentable).target_maintainer?
end
end

Expand Down
2 changes: 1 addition & 1 deletion src/api/app/views/webui/request/beta_show.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@
setInterval(updateChartBuildResults, 60000);

= render AccordionReviewsComponent.new(@request_reviews, @bs_request)
= render RequestDecisionComponent.new(bs_request: @bs_request, action: @action, is_target_maintainer: @is_target_maintainer)
= render RequestDecisionComponent.new(bs_request: @bs_request, action: @action)
= render DeleteConfirmationDialogComponent.new(modal_id: 'delete-comment-modal',
method: :delete,
options: { modal_title: 'Delete comment?', remote: true })
Expand Down

0 comments on commit ecdc872

Please sign in to comment.