Skip to content
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

Fix api key loading if RubyGems host is development #6683

Merged
merged 2 commits into from May 23, 2023

Conversation

jenshenny
Copy link
Member

@jenshenny jenshenny commented May 16, 2023

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

From: #6624 (comment)

The gem signin fails with a warning if the host is localhost:3000

RUBYGEMS_HOST=http://localhost:3000 ruby -Ilib bin/gem signin
Failed to load /Users/jennyshen/.local/share/gem/credentials because it doesn't contain valid YAML hash
Failed to load /Users/jennyshen/.local/share/gem/credentials because it doesn't contain valid YAML hash
Enter your http://localhost:3000 credentials.
Don't have an account yet? Create one at http://localhost:3000/sign_up
   Email:   test2@example.com
Password:   

API Key name [Jennys-MacBook-Pro-2.local-jennyshen-20230516143333]:   
Please select scopes you want to enable for the API key (y/n)
index_rubygems [yN]  
...
Signed in with API key: Jennys-MacBook-Pro-2.local-jennyshen-20230516143333.
Failed to load /Users/jennyshen/.local/share/gem/credentials because it doesn't contain valid YAML hash
Failed to load /Users/jennyshen/.local/share/gem/credentials because it doesn't contain valid YAML hash

The original PR checks for invalid yaml like invalid: foo: bar by checking if there is : in the key. It is possible to have : as a part of the key like http://localhost:3000.

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

Change the includes?(":") to includes?(": ") as that would prompt invalid yaml.

Furthermore, after the warning was fixed, keys with http://localhost:3000 still aren't successfully loaded. This is because

When the api keys gets loaded, the localhost key has a trailing / causing Gem.configuration.api_keys.key?(host) to fail.

[41, 50] in /Users/jennyshen/src/github.com/Shopify/rubygems/lib/rubygems/gemcutter_utilities.rb
   41:   ##
   42:   # The API key from the command options or from the user's configuration.
   43: 
   44:   def api_key
   45:     byebug
=> 46:     if ENV["GEM_HOST_API_KEY"]
   47:       ENV["GEM_HOST_API_KEY"]
   48:     elsif options[:key]
   49:       verify_api_key options[:key]
   50:     elsif Gem.configuration.api_keys.key?(host)
(byebug) Gem.configuration.api_keys
{"http://localhost:3000/"=>"rubygems_"}
(byebug) Gem.configuration.api_keys.key?(host)
false

The serialized key gets converted back here

  • If the key contains __, it gets replaced with . and removes the trailing forward slash.

In the case of localhost:3000, since there aren't any __ because the original host doesn't contain any . , the forward slash never gets removed. I changed the elsif to remove any trailing forward slash.

cc: @hsbt

Make sure the following tasks are checked

Co-authored-by: Eric Herscovich <eric.herscovich@shopify.com>
@jenshenny jenshenny marked this pull request as ready for review May 16, 2023 19:15
Copy link
Contributor

@ericherscovich ericherscovich left a comment

Choose a reason for hiding this comment

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

Looks good thank you!

Copy link
Member

@indirect indirect left a comment

Choose a reason for hiding this comment

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

looks good to me, let's get a review from @hsbt or @deivid-rodriguez to double check 👍🏻

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

It looks good to me!

@jenshenny
Copy link
Member Author

Would anyone be able to merge this in? I don't have merge access for this repo 😄

@indirect indirect merged commit 73df193 into rubygems:master May 23, 2023
83 checks passed
@indirect
Copy link
Member

done!

deivid-rodriguez pushed a commit that referenced this pull request Oct 13, 2023
Fix api key loading if RubyGems host is development

(cherry picked from commit 73df193)
deivid-rodriguez pushed a commit that referenced this pull request Oct 16, 2023
Fix api key loading if RubyGems host is development

(cherry picked from commit 73df193)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants