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

dev
1

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:

  1. User identifies Mastodon domain their account is on.
  2. 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.
  3. 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
    • Use stored public_key to encrypt return payload
  • Otherwise follow the existing functionality.

cc @pmusaraj

2 Likes
2

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?

1 Like
4

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

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.

1 Like
6

Sorry for the delay, Angus.

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!

1 Like
7

I’ve published a draft PR implementing this

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(?)
1 Like