User API keys: duplicate client_id will lead to internal server error

Bug
1

When calling user-api-key/new with a client_id that is already used by another user the forum will throw a RecordNotUnique error and silently fail on an internal server error.

image
image1347×197 10.5 KB

ActiveRecord::RecordNotUnique (PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_user_api_keys_on_client_id"
DETAIL:  Key (client_id)=(893e0230d52455ea9b729334) already exists.
)
(eval):105:in `exec_params'
app/controllers/user_api_keys_controller.rb:66:in `create'

This might want to fail with something less silent informing the user that there already exists an API key with that client ID.

Though that brings me on the second question, are User API keys supposed to behave like that? Is the client ID supposed to be unique between all users?

5 Likes
2

Quick nudge for visibility. We’re still seeing these show up in our /logs endpoint fairly frequently:

image
image1359×70 9.59 KB

3 Likes
7

Thank you for reporting this, I have a couple of questions though in order to help me look into this.

Can you provide a basic repro for this so that I can debug this locally? What is your use case for user-api-keys? Are you using the discourse hub mobile app or something else?

1 Like
8

I’m not sure what you prefer as repro, but this bit of PHP code can repro it great.

Code 
<?php

// openssl genrsa -out keypair.pem 2048
$keypair = openssl_pkey_get_private(file_get_contents("keypair.pem"));

// Obtain the public key
$public = openssl_pkey_get_details($keypair)["key"];

// Build the query
$query = http_build_query([
    "auth_redirect" => "https://localhost/redirect",
    "application_name" => "Test repro",
    "client_id" => "7624a5376b7f52eb403a",
    "scopes" => "session_info",
    "nonce" => bin2hex(random_bytes(16)),
    "public_key" => $public
]);

$url = "https://forum.cfx.re/user-api-key/new?" . $query;
header("Location: " . $url);

The first and repeating authorization will succeed as the first user, when using it again for another user without changing the client_id it will fail.

image
image1252×63 8.55 KB

User API keys are used for allowing the user to use their forum account in the game client, so they’ll be able to post from ingame. We also have lot of users using them to authenticate with forum accounts to their own websites.

Whereas the client ID should be unique for the game clients, so each client is listed as seperate client in the apps screen. For website usecase you’d want to have one client ID so not each login is listed separately.

3 Likes
9

Was wondering if this was resolved

10

Has there been any update about this? It’s still unclear whether the client_id should be globally unique instead of per-user.

1 Like
11

It should be globally unique.

12

How should that be implemented for use cases where a website doesn’t have its own authentication system for users and shouldn’t be creating multiple user API applications?

2 Likes
13

Set a cookie? Or determine it by hashing something that identifies the user (plus something “secret” so external parties cannot replicate it)

14

If the application authenticating users should be used on multiple computers and does not have any user data prior to authentication, that is impossible.

15

I dont understand how this relates to the OP since that describes a case where a client ID is shared by multiple users where your case seems to describe where a user has multiple client ids.

It’s called a client id and not user id because a user can have multiple clients!

16

In most standards like OAuth the client id is described as “app identifier” and can be used for all users (not just one), for example your forum social logins always use the same client id.

However since user API keys seem to be designed primarily for clients such as Discourse apps they might have been designed to be unique, it would be nice to know whether they are.

Answering the above would make it clear whether there’s a check missing in user_api_keys.rb or a wrong index on the database. Because currently those requests currently throw a scary 500 error and show up in our /logs endpoint.

1 Like
17

Any update on this? We still see users running into this.

1 Like
18

The error should be better, yes, but client_id needs to be unique.

When you are sending users that way you must generate a unique id in your API call. The index is correct, 1 user may have N client ids.