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

Add rubygem scope to API key create API endpoint #3136

Merged
merged 5 commits into from
Apr 28, 2023

Conversation

jenshenny
Copy link
Member

@jenshenny jenshenny commented Jul 7, 2022

Adds onto: #2944

Adds support to scope an API key to a gem when creating an API key through the API. This will be used when the user runs gem signin.

How is this implemented?

Added a rubygem_name param to API key create. During gem signin, the client asks for the name of the gem.

...
Enter the name of the gem you want to scope this key to (strongly recommended).
Gem Scope [All gems]: rails
...

CLI PR: rubygems/rubygems#5710

Alternatives

I thought about having the user select from a list of their gems but it's difficult to display correctly if the user has many gems. Furthermore, the only endpoint that returns the gems someone owns requires an API key with the index_rubygems scope which can't work because the command is creating an API key. So if we were going to go down this path, we would need to allow for basic auth to the endpoint instead.

Other Notes

I omitted adding the rubygem_name param to update for now as assign_attributes autosaves associations and "half saves" the api key if validations fail. I have a branch with a monkey patch for it but I want to spend more time figuring out if there's a better way. Nonetheless, the update endpoint won't be used for gem signin.

Testing

  1. Create or have a user on hand
  2. Have the user own a gem (Rubygem.create(name: "a-gem"), Ownership.create_confirmed(Rubygem.find_by_name("a-gem", User.first, User.first)
  3. Run RUBYGEMS_HOST=http://localhost:3000 ruby -Ilib bin/gem signin in the rubygems directory
  4. Enable one or more of the applicable scopes (add/yank rubygems, add/remove owner) and you will see a gem scope prompt. Enter the name of the gem that the owner owns and the created API key should have the scope
  5. Do step 4 again (should gem signout) and type something else, an error will return as the owner doesn't own a gem named that and an API key would not be created.
  6. Do step 4 again and enable one of the other scopes (eg. index_rubygems) and you will not see a gem scope prompt.

@jenshenny jenshenny force-pushed the gem-scope-api branch 5 times, most recently from db9e565 to 80dcd3f Compare July 11, 2022 01:38
@jenshenny jenshenny changed the title Add rubygem scope to API key create and update API endpoints Add rubygem scope to API key create API endpoint Jul 11, 2022
@jenshenny jenshenny marked this pull request as ready for review July 11, 2022 18:39
@simi
Copy link
Member

simi commented Mar 17, 2023

@jenshenny any idea how to move this forward? 🤔

@jchestershopify
Copy link
Contributor

@jenshenny is out for the next week. I'm not sure where we got to on this but it looks like we'd need to do a fair amount of rebasing.

@jenshenny
Copy link
Member Author

Thanks for pinging me on this, it fell off my radar. At the point in time in which the PR was opened, it was ready for review. I'll rebase this PR and the RubyGems one rubygems/rubygems#5710 to get it up to date and make sure the changes make sense. Afterwards, I'll request some reviews!

@jenshenny jenshenny force-pushed the gem-scope-api branch 2 times, most recently from 9c7a8d7 to 7d1e3db Compare April 24, 2023 03:41
@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #3136 (c51be12) into master (d601df0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3136   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files         209      209           
  Lines        5135     5139    +4     
=======================================
+ Hits         5072     5076    +4     
  Misses         63       63           
Impacted Files Coverage Δ
app/controllers/api/v1/api_keys_controller.rb 100.00% <100.00%> (ø)
app/models/api_key.rb 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -277,7 +277,7 @@ def self.should_expect_otp_for_update
authorize_with("#{@user.email}:#{@user.password}")
end

context "oh successful save" do
context "on successful save" do
Copy link
Member

Choose a reason for hiding this comment

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

:)

@simi simi merged commit 5409ec6 into rubygems:master Apr 28, 2023
@simi
Copy link
Member

simi commented Apr 28, 2023

I'm sorry for such a long time without review.

@kevinlinxc
Copy link
Contributor

We did it! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants