Scope API keys to a gem

[jenshenny]

Background

This PR allows users to select a gem scope an API key when creating and editing. Keys with this scope can only push, yank, add/remove owners with only the selected gem. All other behaviour would remain the same.

Data model additions

New join table ApiKeyRubygemScope that belong to a gem ownership and an API key. An API key can up to one ApiKeyRubygemScope, ownerships can have many ApiKeyRubygemScope.

There's also a soft_deleted column on ApiKeys, which will be explained more in the What happens when an ownership is deleted? section.

Creating/Editing a Key with a Gem scope

If user finds themselves submitting an invalid rubygem id, an error will be displayed "Selected gem cannot be scoped to this key"

Screen.Recording.2022-04-04.at.11.48.36.PM.mov

CLI

Screen.Recording.2022-04-05.at.12.06.22.AM.mov

What happens when an ownership is deleted?

When an ownership is deleted, it dependently destroys all associated ApiKeyRubygemScopes. In turn, in a before_destroy callback, the ApiKeyRubygemScope updates the soft_deleted_at associated ApiKey. Soft deleted API keys cannot be edited or used. The user can only delete it. Thought about invalid being the name but that can be confusing will Rails' validations

Screen.Recording.2022-04-05.at.12.09.32.AM.mov

[sonalkr132]

I want to highlight that rubygems.org sorely needs an authorization plugin (ex: pundit). A lot of tech debt has been accrued with checks like these all over the codebase.

[jenshenny]

I felt a little guilty writing this line of code. Maybe delegating this logic to the model with something like def authorized_for_gem?(gem) would be an improvement?

I want to highlight that rubygems.org sorely needs an authorization plugin (ex: pundit). A lot of tech debt has been accrued with checks like these all over the codebase.

Would it be worth creating an issue for this? I bet someone would be interested in looking into this (myself included).

[sonalkr132]

see #2967

[sonalkr132]

should a join table be created between API keys and gems instead of a column for API keys?

IMO, we should create a join table. On the related note, I don't think we need rubygems has_many api_keys relation.

just a textbox. Prone to typos

I think we should start with this. We would show validation errors for typos.

[simi]

Should we really report back which gem is assigned to the key? I would expect same message with wrong (non-existing key) as for key with different scope. It is just invalid for this request (and it doesn't matter why). Exposing this info can make it it easier when you find out a key without having knowledge which gem is assigned to it.

Yes, in theory you can try to find all possible gems to the key and try one by one, but what to make it easier?

[jenshenny]

My intent was to display why the request was invalid as much as possible, but it's true that we are making it easier for someone malicious to use this key as you mentioned. I think it's sufficient to say that the key isn't scoped to this gem, and the user can view the key on the UI to figure out which gem the key belongs to 👍

[simi]

I think "invalid key" message and "key belongs to different gem" messages should be equal.

[jenshenny]

I see what you mean. However, the behaviour between these two cases has a slight difference. For the invalid case, the key cannot be used in any api request while the key belongs to a different gem cannot be used in some owner and gem specific commands, so I'm having a bit of trouble crafting a consistent message.

For now, I have it as "Rubygem <name> cannot be scoped this API key". but that does not sounds entirely correct for either cases. Instead I was thinking of changing in the "key belongs to a different gem" message to be something like this:

This API key does not have permission to modify this gem

while in the "invalid key case" have something more generic since it is set up so that there could be multiple reasons why something is invalid (eg. implementing the concept of expiries on keys).

This API key is invalid

wdyt?

[tomstuart]

Do we want to support selection of a subset of the user’s gems instead of just one (now or in the future)? If so, should a join table be created between API keys and gems instead of a column for API keys?

IMO, we should create a join table.

@sonalkr132 Can you say anything about why you’d prefer a join table? Are you saying that we do want to support multiple gems per API key? If not (or not yet) then IMO it may be clearer and simpler to stick with a belongs_to :rubygem for now, and migrate to a join table if it becomes necessary in future.

[sonalkr132]

I am primarily concerned about api_table.rubygems_id pointing to gems that have changed ownership.

• user1 creates key1 for gem1
• user1 yanks gem1 (all versions)
• user2 claims gem1 after 100 days lock-in
• key1 of user1 still gives user1 access to gem1 (should not happen)

If we keep the association separate, we can delete the record (with ownership). I understand we have an authorization check in code (return true if rubygem.owned_by?(user)) that will prevent this without having to delete records, However, I much rather keep table data consistent as well.

Are you saying that we do want to support multiple gems per API key? If not (or not yet)

I am no opinion about support for multiple gems support, if we receive a feature request with this, we will think about it. I do want to support limiting the scope to IP CIDR(s). Again, technically it can be done without a separate table but I would prefer keeping authorization data separate from data about api key.

[tomstuart]

Thank you @sonalkr132, that’s a very helpful answer.

If I understand correctly, you’d like to ensure that we clean up the table data when gem ownership changes (i.e. an Ownership is deleted) so that we can trust the correctness of the data and aren’t completely reliant upon authorization checks in code. So, for example, we could introduce an ApiKeyAuthorization model which joins an API key to an ownership (rather than to a user), and say that Ownership has_many :api_key_authorizations, dependent: :destroy so that the authorizations get deleted along with the ownership. Is that roughly what you had in mind?

We need to figure out how this should interact with the lifetime of an API key itself. @jenshenny pointed out that, if a key is created to be scoped to a single gem, it would be dangerous for it to then revert to an unscoped key when the ownership (and perhaps join record) is deleted. Does that mean we should still retain some data about key scope when an ownership is deleted, or alternatively that the key itself should be deleted at the same time? If a person loses ownership of a gem and later regains it, should their old API key start working again? Storing the rubygem_id on the API key itself means that we automatically remember the intent to scope the key to a specific gem; if we delete the join record then we need some other mechanism to ensure that the key doesn’t become unscoped.

I do want to support limiting the scope to IP CIDR(s). Again, technically it can be done without a separate table but I would prefer keeping authorization data separate from data about api key.

Okay, so you’re suggesting the IP CIDR scope could also live on the join model (e.g. ApiKeyAuthorization above) and be deleted when the ownership is deleted?

[sonalkr132]

Does that mean we should still retain some data about key scope when an ownership is deleted, or alternatively that the key itself should be deleted at the same time?

Yes, I would prefer dependent destroy but you are right it would mean that api_key reverts to unscoped key. Technically, we can use api_key.fk_id pointing to non-existent record as check, however, I don't want to keep dangling pointers in api_key either. I think we don't want to delete the API key because it may not be as intuitive for the user when they are debugging why their key is not working (given that they don't remember that the key was scoped). I guess metadata about the key being invalid would be the way unless someone has a better idea.

Storing the rubygem_id on the API key itself means that we automatically remember the intent to scope the key to a specific gem;

I understand. I would still prefer a more verbose schema over this.

so you’re suggesting the IP CIDR scope could also live on the join model

yes

[jenshenny]

Sorry for the delay... I updated the PR to reflect the suggestions raised. Updated the PR description with the changes (1. being adding a join model between api keys and ownership and 2. falling back to a dropdown).

[jenshenny]

setting rubygem_id is dependant on having user available, calling build on current_user doesn't set the user until the rest of the params have been set.

[sonalkr132]

A few suggestions on UI:

• if api_key.rubygem is nil, the api keys table gem column should show "all gems" (empty as of now)
• when key is invalid in addition to grey out, we should also explain why is it greyed out. perhaps with tooltip.
• figure out a better way to pairing auth scopes with gem scope. for example, user wants to create a key with access_webhook scope, they can still specify gem scope(which doesn't mean anything). This may be confusing. We can grey out the gem scope area until user selects scopes where gem scope is applicable. Perhaps we can add validation checks as well in model.

[jenshenny]

I'm not much of a UX expert, but I've done what I can to make the UX better as suggested

• if api_key.rubygem is nil, the api keys table gem column should show "all gems" (empty as of now) (50aa4be)
• when key is invalid in addition to grey out, we should also explain why is it greyed out. perhaps with tooltip. (803437f). I also decided to save the removed rubygem name to provide more information (228b722).
• figure out a better way to pairing auth scopes with gem scope. for example, user wants to create a key with access_webhook scope, they can still specify gem scope(which doesn't mean anything). This may be confusing. We can grey out the gem scope area until user selects scopes where gem scope is applicable. Perhaps we can add validation checks as well in model. (33d2f3b for grey out selector, b9c7476 for api key validations)

Screen.Recording.2022-05-06.at.10.55.30.PM.mov

[sonalkr132]

Thank you so much for working on this 💯 You can reach out in rubygems-org slack if you would like to test this in staging.