Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

Allow OTP code to be passed through bundler#7005

Closed
kddnewton wants to merge 1 commit intorubygems:masterfrom
kddnewton:allow-otp
Closed

Allow OTP code to be passed through bundler#7005
kddnewton wants to merge 1 commit intorubygems:masterfrom
kddnewton:allow-otp

Conversation

@kddnewton
Copy link
Copy Markdown
Contributor

@kddnewton kddnewton commented Mar 1, 2019

Hi bundler friends!

What was the end-user problem that led to this PR?

Currently, you can't pass your OTP code for 2FA through the bundle exec rake release command because it just sits and hangs.

What was your diagnosis of the problem?

The subshell used to spawn the gem command that does the push is hanging because it's waiting for the user to enter their OTP code.

What is your fix for the problem, implemented in this PR?

I switched the spawning from calling read immediately (which hangs, because it's waiting for EOF) to instead hook together stdins and call gets in a separate thread.

Why did you choose this fix out of the possible options?

It works!

I have no idea how to actually test this with existing testing infra. Could use some guidance on that. Thanks!

@welcome
Copy link
Copy Markdown

welcome bot commented Mar 1, 2019

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

deivid-rodriguez commented Mar 1, 2019

Thanks for fixing this @kddeisz! Tests are failing, can you have a look?

As per how to test this, I haven't looked at it in detail, but some initial advice I could give is to have a look at the bundle gem specs, because that command picks up user input, so they might help here.

colby-swandale
colby-swandale reviewed Mar 1, 2019
View reviewed changes
lib/bundler/gem_helper.rb
unless allowed_push_host || Bundler.user_home.join(".gem/credentials").file?
raise "Your rubygems.org credentials aren't set. Run `gem push` to set them."
end
sh(gem_command)
Copy link
Copy Markdown
Member

@colby-swandale colby-swandale Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good if we could fix this problem in the sh method rather then just have the release task do its own output/input.

Copy link
Copy Markdown
Contributor Author

@kddnewton kddnewton Mar 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to push this into sh, I just wasn't sure if you would want to switch all of these functions over to that. Not sure of any other implications it may have.

Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colby-swandale @kddeisz I had a look at this PR and the related one (#7108), and I tend to think that @kddeisz's approach is better. The fix in #7108 changes all the methods using the sh helper to print all of there stdout and stderr to the screen. That's quite verbose, specially in the case of rake build. We might still want to change that and print all those messages to the screen everytime, but for the shake of fixing this particular issue, I would only modify this specific method, which is also the only case where we should actually expect input from the user, I think.

What about extracting a sh_with_input method with the modified logic, and use it only here, for now?

Copy link
Copy Markdown
Contributor Author

@kddnewton kddnewton Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gosh yeah sorry this has languished for a while. I can take a look at this again later this week.

Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, let's wait for @colby-swandale's input first so that you don't do unnecessary work.

@deivid-rodriguez deivid-rodriguez mentioned this pull request Mar 3, 2019
Closed
@colby-swandale colby-swandale added this to the 2.0.2 milestone Apr 6, 2019
@colby-swandale colby-swandale mentioned this pull request Apr 8, 2019
Closed
@colby-swandale colby-swandale removed this from the 2.0.2 milestone Apr 8, 2019
@grosser grosser mentioned this pull request Apr 18, 2019
Closed
@dentarg dentarg mentioned this pull request May 6, 2019
Closed
@deivid-rodriguez deivid-rodriguez mentioned this pull request Jun 11, 2019
Merged
@deivid-rodriguez
Copy link
Copy Markdown
Contributor

deivid-rodriguez commented Jun 11, 2019

@kddeisz I created #7199 to try and finally fix this. I started with your code though, so I kept your attribution in there. I'm closing this to reduce the number of PRs fixing the same thing, thanks so much for getting this fix started!!

@kddnewton
Copy link
Copy Markdown
Contributor Author

kddnewton commented Jun 11, 2019

No worries sounds good

ghost pushed a commit that referenced this pull request Jun 12, 2019
@bundlerbot @deivid-rodriguez
Merge #7199
cd05f13 
7199: Allow `rake release` to ask for input (3rd take) r=colby-swandale a=deivid-rodriguez

This PR supersedes #7108 and #7005.

It fixes #6854.

### What was the end-user problem that led to this PR?

The problem was that if users has 2FA authentication on their rubygems account, `rake release` doesn't really work at the moment, since it hangs asking for the OTP code, but without letting the user know.

### What was your diagnosis of the problem?

My diagnosis was that we need to allow the `rake release` helper that shells out to `gem push` to read `gem push` output and show it to the user, so that she can introduce the requested information.

### What is your fix for the problem, implemented in this PR?

My fix is inspired by @segiddins's comment in #6854 (comment). `Kernel#system` works like we would expect in this situation. 

### Why did you choose this fix out of the possible options?

I chose this fix because #7108 had a few problems:

* It would update the `sh` helper, which is used in many different places. This was unnecessary since most of the times we shell out to the `gem` CLI we don't need to ask for input, and it also produced a very verbose output in those cases, since everything the `gem` CLI prints to the screen would be printed by the bundler helpers too. This PR does not change the current output, other than for `rake release`.

* It would print _duplicate_ output. This is a `rake release` test using #7108:

    ```
    $ RUBYOPT="-I../bundler/lib" ../bundler/exe/bundle exec rake release
      Successfully built RubyGem
      Name: rake_release_tester
      Version: 0.1.0
      File: rake_release_tester-0.1.0.gem
    rake_release_tester 0.1.0 built to pkg/rake_release_tester-0.1.0.gem.
    v0.1.0
    Tag v0.1.0 has already been created.
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   asd
    Your OTP code is incorrect. Please check it and retry.
    rake aborted!
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   Your OTP code is incorrect. Please check it and retry.
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:187:in `sh'
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:109:in `rubygem_push'
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:70:in `block in install'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:69:in `load'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:69:in `kernel_load'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:28:in `run'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:468:in `exec'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:26:in `dispatch'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:17:in `start'
    ../bundler/exe/bundle:30:in `block in <main>'
    /home/deivid/Code/bundler/lib/bundler/friendly_errors.rb:123:in `with_friendly_errors'
    ../bundler/exe/bundle:22:in `<main>'
    Tasks: TOP => release => release:rubygem_push
    (See full trace by running task with --trace)
    ```

    This is the same test using this PR:

    ```
    $ RUBYOPT="-I../bundler/lib" ../bundler/exe/bundle exec rake release
    rake_release_tester 0.1.0 built to pkg/rake_release_tester-0.1.0.gem.
    Tag v0.1.0 has already been created.
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   asdfasdf
    Your OTP code is incorrect. Please check it and retry.
    ```

* Previous approach was hard to test. The test included here might not be great but it's something...

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
colby-swandale pushed a commit that referenced this pull request Jun 12, 2019
@bundlerbot @deivid-rodriguez
Merge #7199
6705960 
7199: Allow `rake release` to ask for input (3rd take) r=colby-swandale a=deivid-rodriguez

This PR supersedes #7108 and #7005.

It fixes #6854.

### What was the end-user problem that led to this PR?

The problem was that if users has 2FA authentication on their rubygems account, `rake release` doesn't really work at the moment, since it hangs asking for the OTP code, but without letting the user know.

### What was your diagnosis of the problem?

My diagnosis was that we need to allow the `rake release` helper that shells out to `gem push` to read `gem push` output and show it to the user, so that she can introduce the requested information.

### What is your fix for the problem, implemented in this PR?

My fix is inspired by @segiddins's comment in #6854 (comment). `Kernel#system` works like we would expect in this situation.

### Why did you choose this fix out of the possible options?

I chose this fix because #7108 had a few problems:

* It would update the `sh` helper, which is used in many different places. This was unnecessary since most of the times we shell out to the `gem` CLI we don't need to ask for input, and it also produced a very verbose output in those cases, since everything the `gem` CLI prints to the screen would be printed by the bundler helpers too. This PR does not change the current output, other than for `rake release`.

* It would print _duplicate_ output. This is a `rake release` test using #7108:

    ```
    $ RUBYOPT="-I../bundler/lib" ../bundler/exe/bundle exec rake release
      Successfully built RubyGem
      Name: rake_release_tester
      Version: 0.1.0
      File: rake_release_tester-0.1.0.gem
    rake_release_tester 0.1.0 built to pkg/rake_release_tester-0.1.0.gem.
    v0.1.0
    Tag v0.1.0 has already been created.
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   asd
    Your OTP code is incorrect. Please check it and retry.
    rake aborted!
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   Your OTP code is incorrect. Please check it and retry.
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:187:in `sh'
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:109:in `rubygem_push'
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:70:in `block in install'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:69:in `load'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:69:in `kernel_load'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:28:in `run'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:468:in `exec'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:26:in `dispatch'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:17:in `start'
    ../bundler/exe/bundle:30:in `block in <main>'
    /home/deivid/Code/bundler/lib/bundler/friendly_errors.rb:123:in `with_friendly_errors'
    ../bundler/exe/bundle:22:in `<main>'
    Tasks: TOP => release => release:rubygem_push
    (See full trace by running task with --trace)
    ```

    This is the same test using this PR:

    ```
    $ RUBYOPT="-I../bundler/lib" ../bundler/exe/bundle exec rake release
    rake_release_tester 0.1.0 built to pkg/rake_release_tester-0.1.0.gem.
    Tag v0.1.0 has already been created.
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   asdfasdf
    Your OTP code is incorrect. Please check it and retry.
    ```

* Previous approach was hard to test. The test included here might not be great but it's something...

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
(cherry picked from commit cd05f13)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@colby-swandale colby-swandale colby-swandale left review comments

@deivid-rodriguez deivid-rodriguez deivid-rodriguez left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kddnewton @deivid-rodriguez @colby-swandale