-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add credential helper support for bundler #8501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e9cb2af
0e48def
1ffae05
972132a
5830288
a408b42
be95a01
c7431fa
259610f
db1e5ab
5b91e1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -166,6 +166,9 @@ learn more about their operation in [bundle install(1)](bundle-install.1.html). | |||||
| `bundle install`. | ||||||
| * `console` (`BUNDLE_CONSOLE`): | ||||||
| The console that `bundle console` starts. Defaults to `irb`. | ||||||
| * `credential.helper` (`BUNDLE_CREDENTIAL_HELPER`): | ||||||
| The path to a credential helper to use for fetching credentials from a | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| remote gem server. | ||||||
| * `default_install_uses_path` (`BUNDLE_DEFAULT_INSTALL_USES_PATH`): | ||||||
| Whether a `bundle install` without an explicit `--path` argument defaults | ||||||
| to installing gems in `.bundle`. | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,7 @@ class Settings | |
| system_bindir | ||
| trust-policy | ||
| version | ||
| credential.helper | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| ].freeze | ||
|
|
||
| DEFAULT_CONFIG = { | ||
|
|
@@ -197,7 +198,7 @@ def mirror_for(uri) | |
| end | ||
|
|
||
| def credentials_for(uri) | ||
| self[uri.to_s] || self[uri.host] | ||
| credentials_from_helper(uri) || self[uri.to_s] || self[uri.host] | ||
| end | ||
|
|
||
| def gem_mirrors | ||
|
|
@@ -595,5 +596,35 @@ def self.key_to_s(key) | |
| end | ||
| end | ||
| end | ||
|
|
||
| def credentials_from_helper(uri) | ||
| helper_key = "credential.helper.#{uri.host}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This key differs from the one written in the RFC: https://github.com/rubygems/rfcs/pull/59/changes#diff-30acc2f7e5e276ac49ae593cd2b11138320356f6ebee37c3ca655624e7f4ea4eR33 But I think both |
||
| helper_path = self[helper_key] | ||
| return unless helper_path | ||
|
|
||
| begin | ||
| require "shellwords" | ||
| command = Shellwords.shellsplit(helper_path) | ||
| command[0] = if command[0].start_with?("/", "~") | ||
| command[0] | ||
| else | ||
| "bundler-credential-#{command[0]}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why we need a constant prefix here. Isn't it enough to simply run it with the name as is even when the command is specified by a relative path? We can directly write |
||
| end | ||
|
|
||
| output = Bundler.with_unbundled_env { IO.popen(command, &:read) } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - lets check
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, db1e5ab fixed checking Process.last_status (use $? difficult to write specs) |
||
| unless Process.last_status.success? | ||
| Bundler.ui.warn "Credential helper failed with exit status #{$?.exitstatus}" | ||
| return nil | ||
| end | ||
| output = output.to_s.strip | ||
| output.empty? ? nil : output | ||
| rescue Errno::ENOENT, ArgumentError => e | ||
| Bundler.ui.warn "Credential helper #{helper_path} not available: #{e.message}" | ||
| nil | ||
| rescue StandardError => e | ||
| Bundler.ui.warn "Credential helper failed: #{e.message}" | ||
| nil | ||
|
Comment on lines
+624
to
+626
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO: I think this |
||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "stringio" | ||
| require "bundler/settings" | ||
|
|
||
| RSpec.describe Bundler::Settings do | ||
|
|
@@ -263,6 +264,47 @@ | |
| expect(settings.credentials_for(uri)).to eq(credentials) | ||
| end | ||
| end | ||
|
|
||
| context "with credential helper configured" do | ||
| let(:helper_path) { "/path/to/helper" } | ||
| let(:uri) { Gem::URI("https://gemserver.example.org") } | ||
|
|
||
| before do | ||
| settings.set_local "credential.helper.gemserver.example.org", helper_path | ||
| allow(Process).to receive(:last_status).and_return(double(success?: true, exitstatus: 0)) | ||
| end | ||
|
|
||
| it "uses the credential helper when configured" do | ||
| expect(IO).to receive(:popen).with([helper_path]).and_yield(StringIO.new("username:password\n")) | ||
| expect(settings.credentials_for(uri)).to eq("username:password") | ||
| end | ||
|
|
||
| it "fallback to config when helper fails" do | ||
| expect(IO).to receive(:popen).with([helper_path]).and_raise(StandardError, "Helper failed") | ||
| expect(Bundler.ui).to receive(:warn).with("Credential helper failed: Helper failed") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we want to have more info on which helper failed for which host? |
||
| settings.set_local "gemserver.example.org", "fallback:password" | ||
| expect(settings.credentials_for(uri)).to eq("fallback:password") | ||
| end | ||
|
|
||
| it "returns nil when helper fails and no fallback config exists" do | ||
| expect(IO).to receive(:popen).with([helper_path]).and_yield(StringIO.new("")) | ||
| expect(settings.credentials_for(uri)).to be_nil | ||
| end | ||
|
|
||
| context "with relative helper path and options" do | ||
| let(:helper_path) { "custom-helper --foo=bar" } | ||
|
|
||
| before do | ||
| settings.set_local "credential.helper.gemserver.example.org", helper_path | ||
| allow(Process).to receive(:last_status).and_return(double(success?: true, exitstatus: 0)) | ||
| end | ||
|
|
||
| it "prepends bundler-credential- to the helper name" do | ||
| expect(IO).to receive(:popen).with(["bundler-credential-custom-helper", "--foo=bar"]).and_yield(StringIO.new("username:password\n")) | ||
| expect(settings.credentials_for(uri)).to eq("username:password") | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe "URI normalization" do | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.