-
-
Notifications
You must be signed in to change notification settings - Fork 974
🛂 Update Handling of Gem Permissions #5849
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
base: master
Are you sure you want to change the base?
Changes from 12 commits
3202054
4ad20fd
e5c7c3e
d70f77e
30706cf
964234a
c2596bb
16c0340
f9deac5
cffa861
f155580
9e868f7
6759a19
f404215
28ca093
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ def validate_gem_and_version | |
| if [email protected]? | ||
| render plain: response_with_mfa_warning(t(:this_rubygem_could_not_be_found)), | ||
| status: :not_found | ||
| elsif !@rubygem.owned_by?(@api_key.user) | ||
| elsif !can_delete_gem?(@api_key.user) | ||
| render_forbidden response_with_mfa_warning("You do not have permission to delete this gem.") | ||
| else | ||
| begin | ||
|
|
@@ -45,4 +45,8 @@ def validate_gem_and_version | |
| end | ||
| end | ||
| end | ||
|
|
||
| def can_delete_gem?(user) | ||
| GemPermissions.new(@rubygem, user).can_push? | ||
| end | ||
|
Comment on lines
+49
to
+51
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| class GemPermissions | ||
| def initialize(rubygem, user) | ||
| @rubygem = rubygem | ||
| @user = user | ||
| end | ||
|
|
||
| def can_push? | ||
| return false if user.blank? | ||
|
|
||
| if rubygem.owned_by_organization? | ||
| org_member_in_role?(:maintainer) || user_in_role?(:maintainer) | ||
| else | ||
| user_in_role?(:maintainer) | ||
| end | ||
| end | ||
|
|
||
| def can_manage_owners? | ||
| role_granted?(:owner) | ||
| end | ||
|
|
||
| def can_perform_gem_admin? | ||
| role_granted?(:admin) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :rubygem, :user | ||
|
|
||
| def role_granted?(minimum_role) | ||
| return false if @user.blank? | ||
|
|
||
| if rubygem.owned_by_organization? | ||
| org_member_in_role?(minimum_role) | ||
| else | ||
| user_in_role?(minimum_role) | ||
| end | ||
| end | ||
|
|
||
| def org_member_in_role?(minimum_role) | ||
| case minimum_role | ||
| when :maintainer | ||
| rubygem.organization.user_is_member?(user) | ||
| when :admin | ||
| rubygem.organization.administered_by?(user) | ||
| when :owner | ||
| rubygem.organization.owned_by?(user) | ||
| else | ||
| false | ||
| end | ||
| end | ||
|
|
||
| def user_in_role?(minimum_role) | ||
| rubygem.ownerships.user_with_minimum_role(user, minimum_role).exists? | ||
| end | ||
|
Comment on lines
+39
to
+54
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense to leave these here for now, but I foresee a future where org- (and team-) based permissions are checked in a separate class. |
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ def process | |
| end | ||
|
|
||
| def authorize | ||
| (rubygem.pushable? && (api_key.user? || find_pending_trusted_publisher)) || owner.owns_gem?(rubygem) || notify_unauthorized | ||
| (rubygem.pushable? && (api_key.user? || find_pending_trusted_publisher)) || owner.can_push?(rubygem) || notify_unauthorized | ||
| end | ||
|
|
||
| def verify_gem_scope | ||
|
|
@@ -260,7 +260,7 @@ def republish_notification(version) | |
| notify("It appears that #{version.full_name} did not finish pushing.\n" \ | ||
| "Please contact [email protected] for assistance if you pushed this gem more than a minute ago.", 409) | ||
| else | ||
| different_owner = "pushed by a previous owner of this gem " unless owner.owns_gem?(version.rubygem) | ||
| different_owner = "pushed by a previous owner of this gem " unless owner.can_push?(version.rubygem) | ||
| notify("A yanked version #{different_owner}already exists (#{version.full_name}).\n" \ | ||
| "Repushing of gem versions is not allowed. Please use a new version and retry", 409) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,8 +275,8 @@ def blocked? | |
| blocked_email.present? | ||
| end | ||
|
|
||
| def owns_gem?(rubygem) | ||
| rubygem.owned_by?(self) | ||
| def can_push?(rubygem) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these mean the same? I thought
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the code is currently written (i.e. in |
||
| GemPermissions.new(rubygem, self).can_push? | ||
| end | ||
|
|
||
| def acknowledge_policies! | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.