🛂 Update Handling of Gem Permissions #5849
Conversation
Until now, we've allowed a `Rubygem` to be owned by users (via `Ownership`s) and an `Organization`. This has some downsides. For example, we've had to explicitly check both ownerships and organization membership in our gem-related policies. I've opted to push this logic into `Rubygem` and enforce the ownership check to be organization-specific if the gem belongs to an organization OR ownership-specific (i.e. user-specific) if it is not.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5849 +/- ##
==========================================
- Coverage 97.24% 97.22% -0.02%
==========================================
Files 476 477 +1
Lines 9785 9816 +31
==========================================
+ Hits 9515 9544 +29
- Misses 270 272 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
app/models/rubygem.rb
Outdated
| ownerships.user_with_minimum_role(user, minimum_required_role).exists? | ||
|
|
||
| if owned_by_organization? | ||
| organization.memberships.where(user: user).with_minimum_role(minimum_required_role).exists? |
There was a problem hiding this comment.
This will leave out the current implementation of Outside Contributors, which uses the ownerships table.
There's still work to be done, but this feels (to me) like an improvement over our access-means-ownership model.
landongrindheim
changed the title
This variant feels cleaner to me. Hopefully others will agree.
landongrindheim
force-pushed
the
landon/push-ownership-logic-to-rubygem-model
branch
from
618894b to
964234a
Compare
Ownership is proving to be trickier to keep consistent than I had hoped it would be. I've been migrating to a permissions-based approach, which has similar concerns beneath the covers, but is more explicit in its implementation.
Ownership has proven to be more complicated than expected, and these methods were primarily used by our policy code. The logic has been extracted into GemPermissions, where we've been able to get a bit more exact.
landongrindheim
force-pushed
the
landon/push-ownership-logic-to-rubygem-model
branch
from
bf3166e to
f155580
Compare
| 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 |
There was a problem hiding this comment.
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.
|
I feel that this Pull Request needs a bit more time to bake before I'm ready to accept it. This is a good step, but I also feel it's not complete either. With Organisation teams coming down the pipeline I feel it would be a good opportunity to spend a bit more time on this? |
@colby-swandale I think you're right to be cautious with core permission logic. The organization teams work will add complexity here. Would it be helpful if I walked through how this is designed to extend? I can show how the permission methods would handle the additional cases we'll need, or we can schedule a quick call to discuss the technical approach. Happy to adjust the design based on our conversations, or hold off if you'd prefer to finalize the broader requirements first 😄 |
landongrindheim
requested review from
colby-swandale and
jenshenny
and removed request for
colby-swandale and
martinemde
jenshenny
left a comment
jenshenny
left a comment
There was a problem hiding this comment.
I like that the gem permissions logic is more centralized with this PR.
| def can_delete_gem?(user) | ||
| GemPermissions.new(@rubygem, user).can_push? | ||
| end |
There was a problem hiding this comment.
I think delete_gem? would be good to have in GemPermissions
|
|
||
| def owns_gem?(rubygem) | ||
| rubygem.owned_by?(self) | ||
| def can_push?(rubygem) |
There was a problem hiding this comment.
Do these mean the same? I thought owns_gem? would be like an owner while can_push is like a maintainer with lower permissions
There was a problem hiding this comment.
As the code is currently written (i.e. in main, but not in this branch), Rubygem#owned_by? only considers whether there is an association. It does not consider role, which is why I've chosen to use a name reflecting lower-permissions.
I've lost track of what I was trying to test, and using a non-existing role results in an error. Removal seems appropriate.
3 participants
Until now, we've allowed a
Rubygemto be owned by users (viaOwnerships) and anOrganization. This has some downsides. For example, we've had to explicitly check both ownerships and organization membership in our gem-related policies.I've opted to push this logic into
Rubygemand enforce the ownership check to be organization-specific if the gem belongs to an organization OR ownership-specific (i.e. user-specific) if it is not.