Allow opt-in into requiring gem owners to setup 2FA #2242
Conversation
app/models/rubygem.rb
Outdated
| end | ||
|
|
||
| def mfa_required? | ||
| versions.last&.rubygems_mfa_required? |
There was a problem hiding this comment.
I'm trying to understand the versions.last&.rubygems_mfa_required? check.
Can you clarify, when you push a new version, which is the first version to set rubygems_mfa_required=true, does that version itself require MFA? Or would it only be the next and subsequent versions?
E.g.
- v1.0 does not require MFA
- v1.1 sets rubygems_mfa_required=true.
- v1.2 will require MFA
Would v1.1 also require MFA? Assuming Rubygem#mfa_required? is checking against v1.0 which does not have rubygems_mfa_required=true.
There was a problem hiding this comment.
Thanks @jzajpt. So continuing the example above, when v1.1 is about to be pushed, the latest version is v1.0 (rubygems_mfa_required=nil). Does this mean v1.1 would not require MFA?
There was a problem hiding this comment.
@eliotsykes Sorry, I deleted my original comment as I realized I had not answered your question. When you're pushing the first version withrubygems_mfa_required=true, then the push itself doesn't require MFA yet as the check happens on the latest available version, not the one you're pushing. Only any subsequent action will require it. I will add a test case to cover this scenario.
|
Thanks for the great work! This is a nice feature, overall it looks good. But I'm doubting if checking latest version's metadata is a clean way. Can we add this into gem settings page instead of setting them in gemspec? |
|
@ecnelises I've got the idea to use metadata from the discussion on #2106. I quite like this approach, because there's no UI to edit any gem information on Rubygems.org at this time. But I am open to discussion. |
This idea is reasonable to me. Requiring mfa metadata to be set explicitly is fine, since currently we still see mfa disabled as default behavior. Now I have no idea how other pkg management systems do (npm, pypi, etc.), they might be good reference. |
80b25c2 to
d2d0c47
Compare
I am not sure I understand the requirement for this. Setting |
38b49e7 to
b6ddb47
Compare
Enforce MFA requirement when adding new owners When metadata "rubygems_mfa_required" is present on the last version, rubygems will make sure that new owners have enabled MFA before allowing to add them. Check for MFA requirement when pushing new gems or new gem versions Once the "rubygems_mfa_required" metadata flag is present, Rubygems.org will: * not allow to add/remove any owners unless the user has MFA enabled * not allow to add any owner who hasn't MFA enabled * not allow to push new gem to users without MFA enabled * not allow to push new versions to users without MFA enabled Enforce MFA requirement when yanking versions Fix Rubocop offenses Reuse check for MFA requirement Show information about MFA requirement on rubygems#show page Add DE translations Provide locale keys for all required translations Refactor checking for MFA requirement Use ActiveRecord's boolean cast to check for MFA opt-in Add a test case to cover pushing a version that requires MFA first time Fix Rubocop offense Show a version number that introduced MFA requirement Show only the latest version that introduced MFA requirement
Move mfa requirement check of current version to pusher checking this in rubygems model meant that it would have returned even when all versions of a gem was yanked.
651dab2 to
19767e5
Compare
We don't have much to gain by adding requirement of mfa being enabled for owner being added. The new owner won't be able to perform any action until they enable MFA anyway. Added a test to add the process of disabling mfa requirement. If mfa requirement is set, all versions (even with rubygems_mfa_required false) will require OTP. Versions following rubygems_mfa_required => false won't require OTP.
19767e5 to
844e3c3
Compare
|
I think this is a great feature, thank you! Are there plans to update the Gem Specification docs at https://guides.rubygems.org/specification-reference/ to explain how to use this gemspec metadata? |
This PR adds the ability to opt-in into requiring all gem owners to have enabled 2FA before performing the following actions:
Also, once enabled, Rubygems won't add any new owners to the gem who do not have 2FA already enabled.
The opt-in happens by adding gemspec's metadata option "rubygems_mfa_required" with any value ("true" used in the codebase here). It can be theoretically removed by yanking the version with the opt-in or pushing a new version without the option.