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 https://github.com/discourse/discourse-activity-pub/pull/93 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 Use stored public_key to encrypt return payload Otherwise follow the existing functionality. cc @pmusaraj

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: [image] Angus McLeod: client_id and application_name migrated from user_api_key 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?

[image] Penar Musaraj: the id and name of individual user keys are independent from the id and name of the registered clients This is why I think the client_id should be migrated :slight_smile: 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 [image] Penar Musaraj: 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). 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.

Sorry for the delay, Angus. [image] Angus McLeod: 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. 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 published a draft PR implementing this https://github.com/discourse/discourse/pull/28119 Note that: All existing functionality works as before. The migration is reversible. To do: Fix failing tests. Currently any user can register a client. Query whether this should be restricted in some way(?)

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. https://github.com/discourse/discourse/pull/28119 While rebasing I noticed that separating keys from clients as this PR does would also solve issues like the one addressed recently by @nat https://github.com/discourse/discourse/commit/ede06ffd439263c36ce7fad125efe556a1e8524f 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.

Many thanks! It has (unblocked me) :slight_smile: There will be an implementation in an ActivityPub Discourse-to-Discourse auth PR soon.

Feasibility of allowing a User Api Key client to register a valid auth_redirect

Development

pmusaraj (Penar Musaraj) November 11, 2024, 8:29pm 9

This is at last merged, thanks so much @angus, hopefully this will unblock you now.

Topic		Replies	Views
User API keys: duplicate client_id will lead to internal server error Bug rest-api	13	1235	September 6, 2024
Generating User Api Keys with REST API Development rest-api	20	9096	April 8, 2018
User API keys specification Integrations rest-api , reference	69	38621	September 22, 2025
Can non-admin user issue their own API key? Development rest-api	13	3069	September 2, 2021
Allow API use by regular users, not just admins Development rest-api	6	1657	February 14, 2023

Feasibility of allowing a User Api Key client to register a valid auth_redirect

Related topics