I’m considering whether it’s feasible to allow user api key clients to register a valid auth_redirect for the purposes of key creation.
Current Functionality
Currently user api key redirects have to be registered by the host site in the site setting allowed_user_api_auth_redirects. Any auth_redirect sent with a key create request is checked against that site setting list (see here). This is a sensible protection to avoid an open redirect (which gives rise to various security risks).
Efficient Cause
The efficient cause for this consideration is the ActivityPub plugin’s user authorization feature. This allows a user to prove their ownership of an ActivityPub actor so that that actor’s activities can be associated with their account on a given instance. This is what that looks like
This currently works with Mastodon as every Mastodon server is an OAuth provider that allows the programmatic creation of OAuth clients. The flow works like this:
User identifies Mastodon domain their account is on.
If it’s the first time it’s authorized on that domain, the Plugin creates an OAuth App, on that Mastodon server, registering its redirects and obtaining client credentials for an OAuth Flow.
Plugin performs OAuth flow with user using the OAuth App it created (or previously created) in 2.
The goal is to have effectively the same functionality with Discourse using User Api Keys. This is already implemented in this PR
This flow requires the host site to have already added the client’s domain or redirect to allowed_user_api_auth_redirects.
Possible Change
What I’m currently thinking is making a PR to discourse/discourse that would do the following
Create a user_api_key_client table
Columns:
client_id and application_name migrated from user_api_key
redirect_uri (optional)
public_key(optional)
All existing functionality using client_id and application_name would work as before, through user_api_key_client.
Add a register client route
post "/user-api-key/register" => "user_api_keys#register"
Required params:
application_name
client_id
public_key
auth_redirect
Action:
Validate and save params to user_api_key_client
Return a success code if successful
Allow the use of registered clients on create actions
Change the UserApiKeyController create action as so
If client_id exists in user_api_key_client and has BOTH auth_redirect and public_key:
Validate use of auth_redirect with stored auth_redirect
Overall, I’m in favour of this because it would enable the user authorization workflow in the ActivityPub plugin and beyond without much friction for admins.
Some questions though:
I’m not a huge fan of the migration here, I would prefer to leave existing user api keys as they are because the id and name of individual user keys are independent from the id and name of the registered clients. Maybe instead we can add a column to user_api_key for user_api_key_client_id. The keys with associated clients can then use the redirect configured by the client (and existing keys, of course, keep the current behaviour).
Further, should admins be able to review somewhere the list of registered clients?
This is why I think the client_id should be migrated
Currently the user_api_keys table is a conflation of what perhaps should be two separate data models: user api keys and user api key clients. You can see the limitations of the current approach in the key creation method (here):
# destroy any old keys we had
UserApiKey.where(user_id: current_user.id, client_id: params[:client_id]).destroy_all
key =
UserApiKey.create!(
application_name: @application_name,
client_id: params[:client_id],
user_id: current_user.id,
push_url: params[:push_url],
scopes: scopes.map { |name| UserApiKeyScope.new(name: name) },
)
In order to create a new key you have to destroy all keys with the same client_id to keep the client_id unique. In other words, the current data model requires a 1:1 relationship between client and key. That 1:1 relationship is useful for a subset of use-cases, but limits others.
Indeed this 1:1 limitation is why the approach you’re suggesting probably wouldn’t work
In order for a client to register a redirect for multiple keys the uniqueness of the client_id has to be independent from the keys themselves. Otherwise the registration won’t work for multiple keys as the client_id in the registration action is what is used to identify the redirect.
If you move the client_id and application_name to the dedicated user_api_key_clients table you retain the 1:1 cases, but also allow for 1:many cases, and the features that opens up, such as redirect_uri registration for the purposes of ActivityPub.
I see what you are saying, but by migrating the existing client_id, we would be keeping some of this conflation in the new table because the migrated ids/names would be for keys that are 1-to-1 clients. Especially from an admin’s perspective down the road, it’s good to have a list of the registered multi-to-one clients.
But maybe addressing that isn’t appropriate at this time.
Let’s go ahead with the plan you’re proposing, looking forward to the PR. Thanks!
I’ve rebased this PR as I’m back to focusing on ActivityPub, and this is a potential framework for one of it’s features, as discussed in the OP.
While rebasing I noticed that separating keys from clients as this PR does would also solve issues like the one addressed recently by @nat
Namely, the need to make this change, to destroy all old keys associated with a client, regardless of user, arises because keys and clients are in the same table. Separating them means you can just register a new key for the alternate user of the client.
