Maintainer Role

[colby-swandale]

What's this about?

RubyGems.org currently has a single implicit role assigned to everyone who is an owner of a given gem. This role lets any gem owner do any, and all, things to a gem without restriction, such as adding & removing other owners, publishing new versions, yanking versions etc. In recent years, a strong desire in the community as emerged for a role system to be introduced for groups & businesses to better align their organizational structure and reduce the impact blast of any potential account take over.

What this Pull Request introduces

• This Pull Request adds the formal concept of a Owner & Maintainer role and what permissions are granted.
• Adds a new field to Ownership that assigns a particular role for the that gem + owner relationship
• Adds a new form to edit an existing ownership to allow user's roles to be updated (Owners or Maintainers cannot update their own Role).

What this Pull Request is not introducing

• Fine grained access controls. Permissions & Roles will remain specified in source code and cannot be modified by users.

Technical Details

We've added a new field to the Ownership table that records the user's current role. Inside Rails this is mapped from a string, either maintainer or owner to a new mapped integer in Access which is the thing that's stored in Postgres.

This integer is then used to determine what level of access is granted inside the Rubygem policy.

Decisions made for this Pull Request

The blast radius of this change if things goes wrong is very big, worst case is we stop anyone from publishing new versions. So a priority on maintaining the status quo was put in place to guide this PR.

• All existing ownerships will remain untouched with the same permissions they currently have.
• The default role offered to users in our UI will remain as "Owner" the same role that was implicitly assigned.

Owner vs Maintainer

	Owner	Maintainer
Manage Owners	✅	❌
Publish Version	✅	✅
Yank Version	✅	✅
Manage Adoptions	✅	❌
Manage Trusted Publishing	✅	❌

Screenshots

Viewing current owners and their role

Updating a user's role

User notification of their role being updated

[codecov]

Codecov Report

Attention: Patch coverage is 97.79412% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.04%. Comparing base (b678655) to head (5599466).
Report is 47 commits behind head on master.

Files with missing lines	Patch %	Lines
app/policies/organization_policy.rb	88.88%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4883      +/-   ##
==========================================
+ Coverage   97.02%   97.04%   +0.02%     
==========================================
  Files         420      424       +4     
  Lines        8726     8866     +140     
==========================================
+ Hits         8466     8604     +138     
- Misses        260      262       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[martinemde]

I think it looks good, I need to finish reading through the tests so I'll approve when I'm done, but wanted to get these comments in.

[martinemde]

should this have the new role or old role so we can print "from admin to maintainer".

[martinemde]

I wish we didn't have to have brakeman.ignore. They often need to be fixed and I only notice the code changed when the build fails.

I hesitate to suggest... but changing the param and attribute to access would prevent the warnings. It pairs the name with lib/access.rb. What do you think? Too much?

[colby-swandale]

I did an experiment and found Brakeman is okay with using params[:role]. The solution here I think is to move the action of updating a user's role to its own Rails Controller

[colby-swandale]

As discussed, we're going to leave this for now. If we're making lots of changes and this turns into a pain we will investigate moving the update role action into its own controller