[GSoC] Multi-factor feature for RubyGems. by ecnelises · Pull Request #2369 · ruby/rubygems

ecnelises · 2018-07-27T11:10:52Z

Description:

Hello. This is my GSoC project, dedicated to add multifactor authentication to both RubyGems command program and Gemcutter (RubyGems.org).

Work for the Gemcutter part has almost been finished (see PR #1753, PR #1729 and a series of my progress reports).

Content:

This PR will contain my changes to RubyGems client, adding multifactor auth for gem push, gem signin and gem owner commands. Since no command for editing profile, adding command for changing multifactor auth settings seems unnecessary.

Workflow:

User set up multifactor auth well in the site. (into mfa_login_and_write level)
When user does the actions requiring MFA, an OTP prompt is shown. Or user can add --otp option into command, like gem push mygem-0.0.0.gem --otp 123456.
If the OTP is incorrect, operation fails with failure text.

Tasks:

Add OTP requirement to push_command.
Add OTP requirement to owner_command.
Add OTP prompt to sign_in.
Support for yank_command.
Write related tests.

I will abide by the code of conduct.

hsbt · 2018-07-27T11:38:44Z

+        request.add_field 'Authorization', api_key
+      end
+
+      # For compatibility to Gemcutters without mfa support


Gemcutters is obsoleted name. Please use rubygems.org

Thanks. Here I originally wanted to include some private servers. So I'll change it to servers.

colby-swandale · 2018-07-28T11:14:01Z

Using both otp and mfa does not seem clean, the code would be clearer if you stick with just the one name.

colby-swandale · 2018-07-28T11:16:30Z

+  ##
+  # Require user for extra OTP code if multifactor authentication is enabled.
+
+  def run_mfa_check


run doesn't really mean anything. Maybe just mfa_check?

colby-swandale · 2018-07-28T11:19:54Z

Can you please explain how the user flows works with 2FA. Looking at the code, does the user need to sign in with 2FA first or an error will be given?

Can we just ask the user to give their MFA token whenever they sign in to an account which has 2FA?

ecnelises · 2018-07-28T15:48:04Z

@colby-swandale
Thanks. mfa is better known than otp, mixing them would confuse people not familiar with how mfa/2fa works.

Here we have a concept about level of mfa, inspired by npm. A user can set his level to login_only or login_and_write. login_only means when a user does login, a page requiring extra digit code will be shown. login_and_write also requires the digit code for some important operations besides login. Since I have implemented digit check for website login, add a check for MFA code to client seems more reasonable than not.

ecnelises · 2018-07-30T08:01:22Z

A lot of gem commands need to be sign in. But if we require user input code first, operations next may also require digit code. That will make operation fail. Or retieving API key does not need multifactor auth as long as we require digit code for all important operations.

ghost · 2018-07-30T22:46:14Z

+                   end
+    end
+
+    @mfa_level == 'mfa_login_and_write'


If there isn't a many different values for @mfa_level, shouldn't symbols be
favored over strings here?

Since text received is string and we turned # frozen_string_literal: true on, will that help (in performance) ?

No idea, I didn't even thought about performance when I commented to be honest.
It was more about "semantics", that it seemed used as an "identifier" more than
a string.
But on the other hand, as you said this data is received as string already, if
the spec pass in theory we shouldn't need more code :-)
Anyway it was a small detail I just asked about in case it would help, feel
free to ignore.

ghost · 2018-07-30T22:49:08Z

      end

-      # For compatibility to Gemcutters without mfa support
+      # For compatibility to servers without mfa support


I think this change belongs to another commit (the one adding this comment).

Yes, thanks.

segiddins · 2018-08-01T03:55:52Z

      options[:host] = value
    end

+    add_option('--mfa CODE', 'Digit code for multifactor authentication') do |value, options|


can this repeated code be factored into a module?

Yes. Thanks.

sonalkr132 · 2018-08-01T17:45:41Z

+  def need_mfa?
+    return false if options[:suppress_mfa]
+    unless instance_variable_defined? :@mfa_level
+      response = rubygems_api_request(:get, 'api/v1/multifactor_auth') do |request|


Won't this be adding additional api call to all requests? For default case, we should assume that mfa on api is disabled. mfa is an opt-in feature.
From server, we can return 403 or something more appropriate for cases where otp was required but not provided. Only then, client will prompt for otp and repeat the request.

Here options[:supress_mfa] is just an internal option for keep compatibility with older test cases, since many of them relies on last_request. And fail message the client received is plain/text, should I just match it literally?

for keep compatibility with older test cases, since many of them relies on last_request

Can you please explain more about this limitation? Are you sure we can't do away with options[:suppress_mfa]?

should I just match it literally?

Yeah, checking for response text would be good enough unless we come up with something more elegant.

If we want to remove options[:suppress_mfa], just @cmd.instance_variable_set :@mfa_level, 'no_mfa' works. This method should be not so intrusive. Just now I posted a much more complicated method, like here. Sorry for my carelessness.

The reason why we need to suppress mfa in original tests is they require last_request in Gem::FakeFetcher to determine method, content, headers of last request. One more fake request for mfa status will replace it.

sonalkr132 · 2018-08-11T13:55:25Z

+            request.set_form_data 'email' => owner
+            request.add_field "Authorization", api_key
+            request.add_field "OTP", options[:mfa]
+          end


Please don't duplicate code. Make a method out of common code where otp is optional argument.
Also, I agree with @colby-swandale

the code would be clearer if you stick with just the one name.

"OTP", options[:mfa] 🚫
Please use otp for everything.

sonalkr132 · 2018-08-11T13:59:41Z

    end

+    if need_mfa? response
+      check_mfa


check_mfa and need_mfa? always seem to go together. Can we please call check_mfa from need_mfa? instead?

Yes. Thanks, I changed it.

sonalkr132 · 2018-08-11T17:47:22Z

+  def need_otp? response
+    return unless response.kind_of?(Net::HTTPUnauthorized) &&
+        response.body.start_with?('You have enabled multifactor authentication')
+    unless options[:otp]


return true if options[:otp]

ecnelises · 2018-09-01T08:23:41Z

To push progress of this feature, I listed some implementation problems about current workflow. Can you please take a look and leave comments? Thanks!
See rubygems/rubygems.org#1725

ecnelises · 2018-09-23T16:37:09Z

😄 Is there any problem for current implementation status? I wrote in detail about what it will look like and how it's implemented in GSoC final report. Sorry for out of work these days, maybe I should add 2FA to yank now.

hsbt · 2018-10-09T11:53:55Z

@indirect @sonalkr132 Can you handle this issue with rubygems.org side. I'm okay to merge this pull-request targeted RubyGems 3.0.

hsbt · 2018-11-05T06:44:35Z

@ecnelises Do you still waiting to review this?

into ecnelises-multifactor-auth

hsbt · 2018-12-01T03:37:51Z

@bundlerbot r+

2369: [GSoC] Multi-factor feature for RubyGems. r=hsbt a=ecnelises # Description: Hello. This is my GSoC project, dedicated to add multifactor authentication to both RubyGems command program and Gemcutter ([RubyGems.org](https://rubygems.org)). Work for the Gemcutter part has almost been finished (see [PR #1753](rubygems/rubygems.org#1753), [PR #1729](rubygems/rubygems.org#1729) and a series of [my progress reports](https://ecnelises.github.io/)). ## Content: This PR will contain my changes to RubyGems client, adding multifactor auth for `gem push`, `gem signin` and `gem owner` commands. Since no command for editing profile, adding command for changing multifactor auth settings seems unnecessary. ## Workflow: - User set up multifactor auth well in the site. (into `mfa_login_and_write` level) - When user does the actions requiring MFA, an OTP prompt is shown. Or user can add `--otp` option into command, like `gem push mygem-0.0.0.gem --otp 123456`. - If the OTP is incorrect, operation fails with failure text. ______________ # Tasks: - [x] Add OTP requirement to `push_command`. - [x] Add OTP requirement to `owner_command`. - [x] Add OTP prompt to `sign_in`. - [ ] Support for `yank_command`. - [x] Write related tests. I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: Qiu Chaofan <fwage73@gmail.com> Co-authored-by: SHIBATA Hiroshi <hsbt@ruby-lang.org>

ghost · 2018-12-01T04:04:09Z

Build succeeded

continuous-integration/travis-ci/push

* [GSoC] Multi-factor feature for RubyGems ruby/rubygems#2369 git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@66118 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

hsbt · 2018-12-01T11:02:48Z

@ecnelises @sonalkr132 Thanks for your work. I'm going to release this feature with RG 3.0 at 25, Dec 2018.

sonalkr132 · 2018-12-01T15:08:35Z

Support for yank_command.

Not really sure why @ecnelises decided to keep it separate but PR for it is now merged on rubygems.org, I guess now he can add it here.

ecnelises · 2018-12-01T16:14:03Z

@sonalkr132 I thought that may delay the review process and I’ve waited for `yank` related API on rubygems.org for a while. I can add it in future PRs. :-) Thanks.

…

________________________________ 寄件者: Aditya Prakash <notifications@github.com> 寄件日期: 星期六, 12月 1, 2018 11:08 下午收件者: rubygems/rubygems 副本: Qiu Chaofan; Mention 主旨: Re: [rubygems/rubygems] [GSoC] Multi-factor feature for RubyGems. (#2369) * Support for yank_command. Not really sure why @ecnelises<https://github.com/ecnelises> decided to keep it separate but PR for it is now merged on rubygems.org, I guess now he can add it here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2369 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJ2ePzy7oXpghjbwp3C2-inZytYKai3aks5u0pt2gaJpZM4VjU1G>.

sonalkr132 · 2018-12-01T16:19:12Z

yeah, its cool. yank is not as often used (although very important for mfa) command.
If @hsbt really wants to see make it in one go, I hope he will be kind enough to merge it again 🙏

* [GSoC] Multi-factor feature for RubyGems ruby/rubygems#2369 git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@66118 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

bronzdoc added the status: triage label Jul 27, 2018

hsbt reviewed Jul 27, 2018

View reviewed changes

colby-swandale reviewed Jul 28, 2018

View reviewed changes

ghost reviewed Jul 30, 2018

View reviewed changes

ecnelises force-pushed the multifactor-auth branch from 71e4429 to 6c1996b Compare July 31, 2018 07:42

segiddins reviewed Aug 1, 2018

View reviewed changes

sonalkr132 reviewed Aug 1, 2018

View reviewed changes

ecnelises force-pushed the multifactor-auth branch 4 times, most recently from d92565d to b02ae88 Compare August 10, 2018 12:38

ecnelises closed this Aug 10, 2018

bronzdoc removed the status: triage label Aug 10, 2018

ecnelises reopened this Aug 10, 2018

ecnelises changed the title ~~[GSoC][WIP] Multi-factor feature for RubyGems.~~ [GSoC] Multi-factor feature for RubyGems. Aug 10, 2018

sonalkr132 reviewed Aug 11, 2018

View reviewed changes

ecnelises force-pushed the multifactor-auth branch from e8445e7 to 2295519 Compare August 11, 2018 18:10

ecnelises force-pushed the multifactor-auth branch from b70e0cf to c8f5fef Compare September 1, 2018 07:40

ecnelises force-pushed the multifactor-auth branch from c8f5fef to 67ffcf6 Compare September 21, 2018 16:13

sonalkr132 mentioned this pull request Oct 23, 2018

Two-factor authentication feature in API part. rubygems/rubygems.org#1753

Merged

8 tasks

ecnelises added 15 commits November 18, 2018 16:01

Rename run_mfa_check to check_mfa.

17824f6

Rename otp in code to mfa.

ec1b878

Add a method for --mfa command options.

5fcf469

Remove suppress_mfa option for testing.

09c8b9a

Check response text to determine if mfa needed.

2a85f4a

Add support to callable objects to data of FakeFetcher#request.

f606f78

Add multifactor auth related test to gemcutter utilities.

c5c4cc2

Add multifactor auth related test to push and owner command.

03636c3

Add --mfa option to signin command.

bba2a3f

Wrap api request needing mfa into single methods.

73b6ad2

Unify all multi-factor auth related names to otp.

3b789c4

Move otp asking logic into need_otp?.

1c232f8

Add test for OTP fields in request header.

89f6c2b

Keep OTP after signed in.

c45f786

Tweak MFA prompt text.

f24d666

ecnelises force-pushed the multifactor-auth branch from ba17286 to f24d666 Compare November 18, 2018 08:04

hsbt added 3 commits December 1, 2018 08:35

Merge branch 'master' into multifactor-auth

4726d54

Merge branch 'multifactor-auth' of https://github.com/ecnelises/rubygems

6a625d5

into ecnelises-multifactor-auth

rubocop -a

ee0cbc9

ghost merged commit ee0cbc9 into ruby:master Dec 1, 2018

matzbot pushed a commit to ruby/ruby that referenced this pull request Dec 1, 2018

Merge rubygems-3.0.0.beta3.

7a46a3b

* [GSoC] Multi-factor feature for RubyGems ruby/rubygems#2369 git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@66118 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

bannable pushed a commit to bannable/ruby that referenced this pull request Dec 10, 2018

Merge rubygems-3.0.0.beta3.

f793685

* [GSoC] Multi-factor feature for RubyGems ruby/rubygems#2369 git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@66118 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

cben mentioned this pull request Oct 2, 2019

Minimum gem command version for MFA support? rubygems/guides#245

Closed

This pull request was closed.

Uh oh!

Conversation

ecnelises commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Content:

Workflow:

Tasks:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colby-swandale commented Jul 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colby-swandale commented Jul 28, 2018

Uh oh!

ecnelises commented Jul 28, 2018

Uh oh!

ecnelises commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecnelises Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecnelises commented Sep 1, 2018

Uh oh!

ecnelises commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsbt commented Oct 9, 2018

Uh oh!

hsbt commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsbt commented Dec 1, 2018

Uh oh!

ghost commented Dec 1, 2018

Build succeeded

Uh oh!

hsbt commented Dec 1, 2018

Uh oh!

sonalkr132 commented Dec 1, 2018

Uh oh!

ecnelises commented Dec 1, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonalkr132 commented Dec 1, 2018

Uh oh!

Reviewers

ecnelises commented Jul 27, 2018 •

edited

Loading

ecnelises Aug 3, 2018 •

edited

Loading

ecnelises commented Sep 23, 2018 •

edited

Loading

hsbt commented Nov 5, 2018 •

edited

Loading

ecnelises commented Dec 1, 2018 via email •

edited

Loading