Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintainer Role #4883

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3ddaed4
Add Access Level field to Ownership table and update RubyGem policies…
colby-swandale Jul 17, 2024
bed0451
Add Ownership edit form to owners table
colby-swandale Sep 6, 2024
13dc4a5
Add role field to add Ownership form
colby-swandale Aug 2, 2024
ead2938
Replace Access Level field with Role
colby-swandale Aug 6, 2024
18d4f26
better validate the role in Owners API endpoint
colby-swandale Aug 7, 2024
10d3de0
Refactor Access Level field to be Role instead
colby-swandale Aug 7, 2024
361310d
Notify users when their Role was updated
colby-swandale Aug 14, 2024
75c540a
Don’t allow a user to update their own Ownership
colby-swandale Sep 3, 2024
c11b67e
prevent maintainers for accessing trusted publishing
colby-swandale Sep 4, 2024
3765cb2
Replace Role type object with an Enum
colby-swandale Sep 4, 2024
ca1ffb4
separate checking for onwnership and permissions into separate methods
colby-swandale Sep 5, 2024
4608d56
Add Minimum Role Scope to the Ownership model
colby-swandale Sep 6, 2024
6d1c10f
Add translations for Role field
colby-swandale Sep 6, 2024
20bc75a
Record event when a ownership role is updated
colby-swandale Sep 6, 2024
480db28
Indicate what role the user was updated to in the notification email
colby-swandale Sep 9, 2024
0bbbde0
Fix the wrong select values being passed to the edit/create ownership…
colby-swandale Sep 9, 2024
e37bcc3
Always email owners of any changes to their roles
colby-swandale Sep 13, 2024
2508f34
Fixup breaking test
colby-swandale Sep 17, 2024
05e3841
Remove tests that are no longer relevant
colby-swandale Sep 17, 2024
f766716
Fixup migration file name
colby-swandale Sep 17, 2024
1c66eca
add role to Membership
colby-swandale Sep 17, 2024
3374305
Refactor Ownership policies to cover updating user's own ownership
martinemde Sep 21, 2024
c513739
Improve readability of role access list
martinemde Sep 21, 2024
b51fd50
Add organization policy object
colby-swandale Sep 17, 2024
65788ad
record the previous value when recording the change role event
colby-swandale Sep 23, 2024
6f1e863
Move method to OrganizationPolicy
martinemde Sep 24, 2024
5599466
Fix new lint error
martinemde Sep 24, 2024
764b75c
Update app/models/events/rubygem_event.rb
colby-swandale Sep 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/avo/resources/ownership_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,5 @@ class ConfirmedFilter < ScopeBooleanFilter; end

field :authorizer, as: :belongs_to
field :confirmed_at, as: :date_time
field :role, as: :select, enum: Ownership.roles
end
25 changes: 24 additions & 1 deletion app/controllers/api/v1/owners_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ def show
def create
authorize @rubygem, :add_owner?
owner = User.find_by_name!(email_param)
ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user)
ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user, **ownership_params)

if ownership.save
OwnersMailer.ownership_confirmation(ownership).deliver_later
render plain: response_with_mfa_warning("#{owner.display_handle} was added as an unconfirmed owner. " \
Expand All @@ -26,10 +27,28 @@ def create
end
end

def update
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_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
end
end

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.")
Expand Down Expand Up @@ -60,4 +79,8 @@ def render_not_found
def email_param
params.permit(:email).require(:email)
end

def ownership_params
params.permit(:role)
end
end
42 changes: 37 additions & 5 deletions app/controllers/owners_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@ class OwnersController < ApplicationController
include SessionVerifiable

before_action :find_rubygem, except: :confirm
verify_session_before only: %i[index create destroy]
before_action :verify_mfa_requirement, only: %i[create destroy]
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)
Expand Down Expand Up @@ -32,10 +37,14 @@ def index
@ownerships = @rubygem.ownerships_including_unconfirmed.includes(:user, :authorizer)
end

def edit
end

def create
authorize @rubygem, :add_owner?
owner = User.find_by_name(handle_params)
ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user)

ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user, role: params[:role])
if ownership.save
OwnersMailer.ownership_confirmation(ownership).deliver_later
redirect_to rubygem_owners_path(@rubygem.slug), notice: t(".success_notice", handle: owner.name)
Expand All @@ -44,9 +53,19 @@ def create
end
end

# This action is used to update a user's owenrship role. This endpoint currently asssumes
# 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
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
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)
Expand All @@ -61,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
Expand All @@ -69,6 +97,10 @@ def handle_params
params.permit(:handle).require(:handle)
end

def update_params
params.permit(:role)
end

def notify_owner_added(ownership)
ownership.rubygem.ownership_notifiable_owners.each do |notified_user|
OwnersMailer.owner_added(
Expand Down
11 changes: 11 additions & 0 deletions app/mailers/owners_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ def ownership_confirmation(ownership)
end
end

def owner_updated
@ownership = params[:ownership]
@user = @ownership.user
@rubygem = @ownership.rubygem

mail(
to: @user.email,
subject: t("mailer.owner_updated.subject", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY)
)
end

def owner_removed(user_id, remover_id, gem_id)
@user = User.find(user_id)
@remover = User.find(remover_id)
Expand Down
4 changes: 2 additions & 2 deletions app/models/api_key.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
class ApiKey < ApplicationRecord
class ScopeError < RuntimeError; end

API_SCOPES = %i[show_dashboard index_rubygems push_rubygem yank_rubygem add_owner remove_owner access_webhooks
API_SCOPES = %i[show_dashboard index_rubygems push_rubygem yank_rubygem add_owner update_owner remove_owner access_webhooks
configure_trusted_publishers].freeze
APPLICABLE_GEM_API_SCOPES = %i[push_rubygem yank_rubygem add_owner remove_owner configure_trusted_publishers].freeze
APPLICABLE_GEM_API_SCOPES = %i[push_rubygem yank_rubygem add_owner update_owner remove_owner configure_trusted_publishers].freeze
EXCLUSIVE_SCOPES = %i[show_dashboard].freeze

self.ignored_columns += API_SCOPES
Expand Down
11 changes: 11 additions & 0 deletions app/models/events/rubygem_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ class Events::RubygemEvent < ApplicationRecord
attribute :owner_gid, :global_id
end

OWNER_ROLE_UPDATED = define_event "rubygem:owner:updated" do
colby-swandale marked this conversation as resolved.
Show resolved Hide resolved
attribute :owner, :string
attribute :updated_by, :string

attribute :actor_gid, :global_id
attribute :owner_gid, :global_id

attribute :previous_role, :string
attribute :current_role, :string
end

OWNER_REMOVED = define_event "rubygem:owner:removed" do
attribute :owner, :string
attribute :removed_by, :string
Expand Down
2 changes: 2 additions & 0 deletions app/models/membership.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ class Membership < ApplicationRecord
scope :unconfirmed, -> { where(confirmed_at: nil) }
scope :confirmed, -> { where.not(confirmed_at: nil) }

enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER, admin: Access::ADMIN }, validate: true, default: :maintainer

def confirmed?
!confirmed_at.nil?
end
Expand Down
24 changes: 22 additions & 2 deletions app/models/ownership.rb
colby-swandale marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,17 @@ class Ownership < ApplicationRecord

after_create :record_create_event
after_update :record_confirmation_event, if: :saved_change_to_confirmed_at?
after_update :record_role_updated_event, if: :saved_change_to_role?
after_update :notify_user_role_of_role_change, if: :saved_change_to_role?
after_destroy :record_destroy_event

scope :confirmed, -> { where.not(confirmed_at: nil) }
scope :unconfirmed, -> { where(confirmed_at: nil) }

enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER }, validate: true, default: :owner

scope :user_with_minimum_role, ->(user, role) { where(user: user, role: Access.with_minimum_role(role)) }

def self.by_indexed_gem_name
select("ownerships.*, rubygems.name")
.left_joins(rubygem: :versions)
Expand All @@ -26,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)
Expand Down Expand Up @@ -107,11 +113,25 @@ def record_confirmation_event
actor_gid: Current.user&.to_gid)
end

def record_role_updated_event
rubygem.record_event!(Events::RubygemEvent::OWNER_ROLE_UPDATED,
owner: user.display_handle,
updated_by: Current.user&.display_handle,
owner_gid: user.to_gid,
actor_gid: Current.user&.to_gid,
previous_role: role_previously_was,
current_role: role)
end

def record_destroy_event
rubygem.record_event!(Events::RubygemEvent::OWNER_REMOVED,
owner: user.display_handle,
removed_by: Current.user&.display_handle,
owner_gid: user.to_gid,
actor_gid: Current.user&.to_gid)
end

def notify_user_role_of_role_change
OwnersMailer.with(ownership: self).owner_updated.deliver_later
end
end
7 changes: 7 additions & 0 deletions app/models/rubygem.rb
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ def owned_by?(user)
ownerships.exists?(user_id: user.id)
end

def owned_by_with_role?(user, minimum_required_role)
return false if user.blank?
ownerships.user_with_minimum_role(user, minimum_required_role).exists?
rescue KeyError
false
end

def unconfirmed_ownerships
ownerships_including_unconfirmed.unconfirmed
end
Expand Down
9 changes: 9 additions & 0 deletions app/policies/api/application_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 11 additions & 0 deletions app/policies/api/ownership_policy.rb
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions app/policies/api/rubygem_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ def add_owner?
user_authorized?(rubygem, :add_owner?)
end

def update_owner?
user_api_key? &&
mfa_requirement_satisfied?(rubygem) &&
api_key_scope?(:update_owner, rubygem) &&
user_authorized?(rubygem, :update_owner?)
end

def remove_owner?
user_api_key? &&
mfa_requirement_satisfied?(rubygem) &&
Expand Down
4 changes: 4 additions & 0 deletions app/policies/application_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ def rubygem_owned_by?(user)
rubygem.owned_by?(user) || deny(t(:forbidden))
end

def rubygem_owned_by_with_role?(user, minimum_required_role:)
rubygem.owned_by_with_role?(user, minimum_required_role) || deny(t(:forbidden))
end

def policy!(user, record) = Pundit.policy!(user, record)
def user_policy!(record) = policy!(user, record)

Expand Down
6 changes: 3 additions & 3 deletions app/policies/oidc/rubygem_trusted_publisher_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ class Scope < ApplicationPolicy::Scope
delegate :rubygem, to: :record

def show?
rubygem_owned_by?(user)
rubygem_owned_by_with_role?(user, minimum_required_role: :owner)
end

def create?
rubygem_owned_by?(user)
rubygem_owned_by_with_role?(user, minimum_required_role: :owner)
end

def destroy?
rubygem_owned_by?(user)
rubygem_owned_by_with_role?(user, minimum_required_role: :owner)
end
end
54 changes: 54 additions & 0 deletions app/policies/organization_policy.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
class OrganizationPolicy < ApplicationPolicy
class Scope < ApplicationPolicy::Scope
end

def show?
true

Check warning on line 6 in app/policies/organization_policy.rb

View check run for this annotation

Codecov / codecov/patch

app/policies/organization_policy.rb#L6

Added line #L6 was not covered by tests
end

def update?
organization_member_with_role?(user, minimum_required_role: Access::OWNER)
end

def create?
true

Check warning on line 14 in app/policies/organization_policy.rb

View check run for this annotation

Codecov / codecov/patch

app/policies/organization_policy.rb#L14

Added line #L14 was not covered by tests
end

def add_gem?
organization_member_with_role?(user, minimum_required_role: Access::OWNER)
end

def remove_gem?
organization_member_with_role?(user, minimum_required_role: Access::OWNER)
end

def add_membership?
organization_member_with_role?(user, minimum_required_role: Access::OWNER)
end

def update_membership?
organization_member_with_role?(user, minimum_required_role: Access::OWNER)
end

def remove_membership?
organization_member_with_role?(user, minimum_required_role: Access::OWNER)
end

def show_membership?
organization_member_with_role?(user, minimum_required_role: Access::MAINTAINER)
end

def list_memberships?
organization_member_with_role?(user, minimum_required_role: Access::MAINTAINER)
end

def destroy?
false # For now organizations cannot be deleted

Check warning on line 46 in app/policies/organization_policy.rb

View check run for this annotation

Codecov / codecov/patch

app/policies/organization_policy.rb#L46

Added line #L46 was not covered by tests
end

private

def organization_member_with_role?(user, minimum_required_role:)
record.memberships.exists?(["user_id = ? AND role >= ?", user.id, minimum_required_role])
end
colby-swandale marked this conversation as resolved.
Show resolved Hide resolved
end
Loading