commit bff7981e354ff85aa587f59a8ee2af450b7fe472
parent 893fbfee7b0f9ef770a7b784360ad1e69e6d5f19
Author: Christian Grothoff <christian@grothoff.org>
Date: Thu, 6 Aug 2026 15:07:54 +0200
be precise as to the cause of why the insert did not succeed and report back to the user accordingly
Diffstat:
5 files changed, 199 insertions(+), 22 deletions(-)
diff --git a/src/challenger/challenger-admin.c b/src/challenger/challenger-admin.c
@@ -213,6 +213,7 @@ run (void *cls,
{
enum GNUNET_DB_QueryStatus qs;
uint64_t row_id;
+ bool uri_taken = false;
qs = CHALLENGERDB_get_client_by_uri (db,
redirect_uri,
@@ -241,7 +242,8 @@ run (void *cls,
qs = CHALLENGERDB_insert_client (db,
redirect_uri,
client_secret,
- &row_id);
+ &row_id,
+ &uri_taken);
switch (qs)
{
case GNUNET_DB_STATUS_SOFT_ERROR:
@@ -250,8 +252,12 @@ run (void *cls,
global_ret = EXIT_FAILURE;
goto cleanup;
case GNUNET_DB_STATUS_SUCCESS_NO_RESULTS:
- GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
- "Client with this CLIENT_REDIRECT_URI and different secret already exists.\n");
+ if (uri_taken)
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Client with this CLIENT_REDIRECT_URI and different secret already exists.\n");
+ else
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Failed to add client even though this CLIENT_REDIRECT_URI is not in use. Check that the identity sequence of the 'clients' table is not behind max(client_serial_id); this happens after restoring a dump without its sequences.\n");
global_ret = EXIT_FAILURE;
goto cleanup;
case GNUNET_DB_STATUS_SUCCESS_ONE_RESULT:
diff --git a/src/challengerdb/insert_client.c b/src/challengerdb/insert_client.c
@@ -30,31 +30,66 @@ enum GNUNET_DB_QueryStatus
CHALLENGERDB_insert_client (struct CHALLENGERDB_PostgresContext *ctx,
const char *client_redirect_uri,
const char *client_secret,
- uint64_t *client_id)
+ uint64_t *client_id,
+ bool *uri_taken)
{
struct GNUNET_PQ_QueryParam params[] = {
GNUNET_PQ_query_param_string (client_redirect_uri),
GNUNET_PQ_query_param_string (client_secret),
GNUNET_PQ_query_param_end
};
+ bool inserted;
+ bool taken;
struct GNUNET_PQ_ResultSpec rs[] = {
- GNUNET_PQ_result_spec_uint64 ("client_serial_id",
- client_id),
+ GNUNET_PQ_result_spec_allow_null (
+ GNUNET_PQ_result_spec_uint64 ("client_serial_id",
+ client_id),
+ NULL),
+ GNUNET_PQ_result_spec_bool ("inserted",
+ &inserted),
+ GNUNET_PQ_result_spec_bool ("uri_taken",
+ &taken),
GNUNET_PQ_result_spec_end
};
+ enum GNUNET_DB_QueryStatus qs;
*client_id = 0;
+ *uri_taken = false;
+ /* The ON CONFLICT arbiter must be spelled out: a bare 'ON CONFLICT DO
+ NOTHING' swallows *any* unique violation, including one on the primary
+ key, which happens when the identity sequence of 'clients' lags behind
+ max(client_serial_id) (e.g. after a restore that did not restore the
+ sequence). As GNUnet maps every unique violation to NO_RESULTS, the
+ caller could not tell that apart from "this URI is already registered"
+ either, so report whether the URI was taken; if it was not, the INSERT
+ failed for a different reason and the caller must say so. */
PREPARE (ctx,
"insert_client",
- "INSERT INTO clients"
- " (uri"
- " ,client_secret"
- ") VALUES "
- "($1, $2)"
- " ON CONFLICT DO NOTHING" /* CONFLICT on (uri) */
- " RETURNING client_serial_id");
- return GNUNET_PQ_eval_prepared_singleton_select (ctx->conn,
- "insert_client",
- params,
- rs);
+ "WITH ins AS ("
+ " INSERT INTO clients"
+ " (uri"
+ " ,client_secret"
+ " ) VALUES "
+ " ($1, $2)"
+ " ON CONFLICT (uri) DO NOTHING"
+ " RETURNING client_serial_id"
+ ") SELECT"
+ " (SELECT client_serial_id FROM ins) AS client_serial_id"
+ " ,EXISTS (SELECT 1 FROM ins) AS inserted"
+ " ,EXISTS (SELECT 1 FROM clients WHERE uri=$1) AS uri_taken;");
+ qs = GNUNET_PQ_eval_prepared_singleton_select (ctx->conn,
+ "insert_client",
+ params,
+ rs);
+ if (0 > qs)
+ return qs;
+ if (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS == qs)
+ return qs;
+ *uri_taken = taken;
+ if (! inserted)
+ {
+ *client_id = 0;
+ return GNUNET_DB_STATUS_SUCCESS_NO_RESULTS;
+ }
+ return GNUNET_DB_STATUS_SUCCESS_ONE_RESULT;
}
diff --git a/src/challengerdb/meson.build b/src/challengerdb/meson.build
@@ -108,6 +108,7 @@ test_challenger_db_postgres = executable(
libchallengerutil_dep,
libchallengerdb_dep,
gnunetpq_dep,
+ pq_dep,
talerutil_dep,
pq_dep,
json_dep,
diff --git a/src/challengerdb/test_challenger_db.c b/src/challengerdb/test_challenger_db.c
@@ -80,11 +80,14 @@ static uint64_t client_id;
static enum GNUNET_GenericReturnValue
setup_client (void)
{
+ bool uri_taken = false;
+
if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT !=
CHALLENGERDB_insert_client (pg,
CLIENT_URI,
CLIENT_SECRET,
- &client_id))
+ &client_id,
+ &uri_taken))
{
GNUNET_break (0);
return GNUNET_SYSERR;
@@ -502,6 +505,130 @@ test_foreign_client_not_redeemable (void)
/**
+ * Run @a sql directly on the database, bypassing the CHALLENGERDB API.
+ * Used to create states (and to make assertions about the schema) that
+ * the API deliberately cannot produce.
+ *
+ * @param sql SQL statement to execute
+ * @return #GNUNET_OK on success
+ */
+static enum GNUNET_GenericReturnValue
+exec_sql (const char *sql)
+{
+ struct GNUNET_PQ_ExecuteStatement es[] = {
+ GNUNET_PQ_make_execute (sql),
+ GNUNET_PQ_EXECUTE_STATEMENT_END
+ };
+
+ return GNUNET_PQ_exec_statements (pg->conn,
+ es);
+}
+
+
+/**
+ * Test that a client cannot be inserted silently: a URI that is already
+ * registered must be reported as such, and any *other* unique violation
+ * (notably one on the primary key, which happens once the identity sequence
+ * of 'clients' lags behind max(client_serial_id)) must not be misreported as
+ * a duplicate URI.
+ *
+ * @return #GNUNET_OK on success
+ */
+static enum GNUNET_GenericReturnValue
+test_insert_client (void)
+{
+ uint64_t client_id;
+ bool uri_taken;
+
+ if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT !=
+ CHALLENGERDB_insert_client (pg,
+ "https://example.com/a",
+ "secret-token:a",
+ &client_id,
+ &uri_taken))
+ {
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ if (uri_taken)
+ {
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ /* Same URI again: this one really is a duplicate. */
+ if ( (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS !=
+ CHALLENGERDB_insert_client (pg,
+ "https://example.com/a",
+ "secret-token:b",
+ &client_id,
+ &uri_taken)) ||
+ (! uri_taken) )
+ {
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ /* Rewind the identity sequence, as a restore that did not restore the
+ sequences leaves it: the next INSERT then collides on the primary key,
+ not on the URI. */
+ if (GNUNET_OK !=
+ exec_sql ("DO $$ BEGIN"
+ " PERFORM setval("
+ " pg_get_serial_sequence('challenger.clients',"
+ " 'client_serial_id'),"
+ " (SELECT MIN(client_serial_id) FROM challenger.clients),"
+ " false);"
+ "END $$;"))
+ {
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT ==
+ CHALLENGERDB_insert_client (pg,
+ "https://example.com/b",
+ "secret-token:b",
+ &client_id,
+ &uri_taken))
+ {
+ /* The PK collision must not go unnoticed. */
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ if (uri_taken)
+ {
+ /* This is the regression: the failure has nothing to do with the URI,
+ and the operator must not be told that the URI already exists. */
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ /* Repair the sequence, then the very same insert must work. */
+ if (GNUNET_OK !=
+ exec_sql ("DO $$ BEGIN"
+ " PERFORM setval("
+ " pg_get_serial_sequence('challenger.clients',"
+ " 'client_serial_id'),"
+ " (SELECT MAX(client_serial_id) FROM challenger.clients),"
+ " true);"
+ "END $$;"))
+ {
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ if ( (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT !=
+ CHALLENGERDB_insert_client (pg,
+ "https://example.com/b",
+ "secret-token:b",
+ &client_id,
+ &uri_taken)) ||
+ (uri_taken) )
+ {
+ GNUNET_break (0);
+ return GNUNET_SYSERR;
+ }
+ return GNUNET_OK;
+}
+
+
+/**
* Main function that will be run by the scheduler.
*
* @param cls closure with config
@@ -554,6 +681,8 @@ run (void *cls)
test_expired_not_redeemable ());
FAILIF (GNUNET_OK !=
test_foreign_client_not_redeemable ());
+ FAILIF (GNUNET_OK !=
+ test_insert_client ());
result = 0;
drop:
GNUNET_break (GNUNET_OK ==
diff --git a/src/include/challenger-database/insert_client.h b/src/include/challenger-database/insert_client.h
@@ -32,18 +32,24 @@ struct CHALLENGERDB_PostgresContext;
* @param client_url URL of the client
* @param client_secret authorization secret for the client
* @param[out] client_id set to the client ID on success; left at 0 if the
- * client already exists (duplicate URI: ON CONFLICT DO NOTHING)
+ * client already exists (duplicate URI: ON CONFLICT (uri) DO NOTHING)
+ * @param[out] uri_taken set to true if @a client_url is already registered;
+ * only meaningful if the return value is
+ * #GNUNET_DB_STATUS_SUCCESS_NO_RESULTS, where false means the
+ * INSERT failed for another reason (typically the identity
+ * sequence of 'clients' lagging behind max(client_serial_id))
* @return transaction status:
* #GNUNET_DB_STATUS_SUCCESS_ONE_RESULT if the client was added
- * #GNUNET_DB_STATUS_SUCCESS_NO_RESULTS if a client with this URI already
- * exists (nothing inserted, @a client_id left 0)
+ * #GNUNET_DB_STATUS_SUCCESS_NO_RESULTS if nothing was inserted (@a client_id
+ * left 0), see @a uri_taken for why
* #GNUNET_DB_STATUS_HARD_ERROR on failure
*/
enum GNUNET_DB_QueryStatus
CHALLENGERDB_insert_client (struct CHALLENGERDB_PostgresContext *ctx,
const char *client_url,
const char *client_secret,
- uint64_t *client_id);
+ uint64_t *client_id,
+ bool *uri_taken);
#endif
