commit 46d3e49ff9b9784e5e95875a7db1e39a5255a9cb
parent 929085d4489f0b15b2ca4337870e9e3e6b7c0571
Author: Christian Grothoff <christian@grothoff.org>
Date: Thu, 6 Aug 2026 15:01:49 +0200
distinguish URI collision from case where client is unknown
Diffstat:
5 files changed, 179 insertions(+), 14 deletions(-)
diff --git a/src/challenger/challenger-admin.c b/src/challenger/challenger-admin.c
@@ -160,6 +160,7 @@ run (void *cls,
{
enum GNUNET_DB_QueryStatus qs;
unsigned long long row_id;
+ bool uri_conflict = false;
char dummy;
if ( (! isdigit ((unsigned char) client_id[0])) ||
@@ -180,7 +181,8 @@ run (void *cls,
qs = CHALLENGERDB_update_client (db,
row_id,
redirect_uri,
- client_secret);
+ client_secret,
+ &uri_conflict);
switch (qs)
{
case GNUNET_DB_STATUS_SOFT_ERROR:
@@ -189,9 +191,14 @@ run (void *cls,
global_ret = EXIT_FAILURE;
goto cleanup;
case GNUNET_DB_STATUS_SUCCESS_NO_RESULTS:
- GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
- "Client %llu not found.\n",
- row_id);
+ if (uri_conflict)
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "CLIENT_REDIRECT_URI is already in use by another client. Client %llu was not modified.\n",
+ row_id);
+ else
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Client %llu not found.\n",
+ row_id);
global_ret = EXIT_FAILURE;
goto cleanup;
case GNUNET_DB_STATUS_SUCCESS_ONE_RESULT:
diff --git a/src/challenger/meson.build b/src/challenger/meson.build
@@ -9,6 +9,7 @@ install_data('challenger.conf', install_dir: pkgcfgdir)
check_SCRIPTS = [
'test-challenger',
+ 'test-challenger-admin',
'test-challenger-dbconfig',
'test-challenger-pkce',
'test-challenger-pkce-downgrade',
diff --git a/src/challenger/test-challenger-admin.sh b/src/challenger/test-challenger-admin.sh
@@ -0,0 +1,98 @@
+#!/usr/bin/env bash
+# This file is in the public domain.
+#
+# Tests the diagnostics of challenger-admin. Needs a database, but no daemon.
+#
+# The interesting case is '-m' with a CLIENT_REDIRECT_URI that another client
+# already uses: that violates clients_uri_key, which GNUnet reports as
+# SUCCESS_NO_RESULTS -- the very same status as "no such client". Reporting
+# it as "Client not found" tells the operator (and any deployment script) the
+# wrong thing.
+
+set -eu
+
+# Exit, with status code "skip" (no 'real' failure)
+function exit_skip() {
+ echo " SKIP: $1"
+ exit 77
+}
+
+# Exit, with error message (hard failure)
+function exit_fail() {
+ echo " FAIL: $@"
+ exit 1
+}
+
+# Cleanup to run whenever we exit
+function cleanup()
+{
+ rm -f "$LAST_OUTPUT"
+}
+
+LAST_OUTPUT=$(mktemp adminXXXXXX.log)
+
+# Install cleanup handler (except for kill -9)
+trap cleanup EXIT
+
+export PATH="$PATH:."
+
+echo -n "Testing for challenger-admin ..."
+challenger-admin -h > /dev/null || exit_skip "challenger-admin required"
+echo " FOUND"
+
+CONF="test-challenger.conf"
+URI_A="http://client-a.example.com/"
+URI_B="http://client-b.example.com/"
+URI_C="http://client-c.example.com/"
+CLIENT_SECRET="secret-token:secret"
+
+echo -n "Initialize challenger database ..."
+challenger-dbinit -r -c "${CONF}" &> dbinit.log
+echo " OK"
+
+echo -n "Add two challenger clients ..."
+ID_A=$(challenger-admin -c "${CONF}" -q -a "${CLIENT_SECRET}" "${URI_A}")
+ID_B=$(challenger-admin -c "${CONF}" -q -a "${CLIENT_SECRET}" "${URI_B}")
+if [ "${ID_A}" = "${ID_B}" ]
+then
+ exit_fail "Expected two distinct client IDs, got ${ID_A} twice"
+fi
+echo " OK (${ID_A}, ${ID_B})"
+
+echo -n "Modifying client ${ID_B} to the URI of client ${ID_A} ..."
+if challenger-admin -c "${CONF}" -m "${ID_B}" "${URI_A}" &> "$LAST_OUTPUT"
+then
+ exit_fail "Expected failure, but the modification succeeded: $(cat "$LAST_OUTPUT")"
+fi
+if grep -q "not found" "$LAST_OUTPUT"
+then
+ exit_fail "URI collision was misreported as a missing client: $(cat "$LAST_OUTPUT")"
+fi
+if ! grep -qi "already in use" "$LAST_OUTPUT"
+then
+ exit_fail "Expected an error about the URI being in use, got: $(cat "$LAST_OUTPUT")"
+fi
+echo " OK"
+
+echo -n "Modifying a client that does not exist ..."
+if challenger-admin -c "${CONF}" -m 999999 "${URI_C}" &> "$LAST_OUTPUT"
+then
+ exit_fail "Expected failure, but the modification succeeded: $(cat "$LAST_OUTPUT")"
+fi
+if ! grep -q "not found" "$LAST_OUTPUT"
+then
+ exit_fail "Expected 'not found', got: $(cat "$LAST_OUTPUT")"
+fi
+echo " OK"
+
+echo -n "Modifying client ${ID_B} to an unused URI ..."
+challenger-admin -c "${CONF}" -m "${ID_B}" "${URI_C}" &> "$LAST_OUTPUT" \
+ || exit_fail "Expected success, got: $(cat "$LAST_OUTPUT")"
+echo " OK"
+
+echo -n "Re-using the now free URI for client ${ID_A} ..."
+challenger-admin -c "${CONF}" -m "${ID_A}" "${URI_B}" &> "$LAST_OUTPUT" \
+ || exit_fail "Expected success, got: $(cat "$LAST_OUTPUT")"
+echo " OK"
+
+exit 0
diff --git a/src/challengerdb/update_client.c b/src/challengerdb/update_client.c
@@ -29,7 +29,8 @@ enum GNUNET_DB_QueryStatus
CHALLENGERDB_update_client (struct CHALLENGERDB_PostgresContext *ctx,
uint64_t client_id,
const char *client_redirect_uri,
- const char *client_secret)
+ const char *client_secret,
+ bool *uri_conflict)
{
struct GNUNET_PQ_QueryParam params[] = {
GNUNET_PQ_query_param_uint64 (&client_id),
@@ -39,14 +40,62 @@ CHALLENGERDB_update_client (struct CHALLENGERDB_PostgresContext *ctx,
: GNUNET_PQ_query_param_string (client_secret),
GNUNET_PQ_query_param_end
};
+ bool updated;
+ bool conflict;
+ struct GNUNET_PQ_ResultSpec rs[] = {
+ GNUNET_PQ_result_spec_bool ("updated",
+ &updated),
+ GNUNET_PQ_result_spec_bool ("uri_conflict",
+ &conflict),
+ GNUNET_PQ_result_spec_end
+ };
+ enum GNUNET_DB_QueryStatus qs;
+ *uri_conflict = false;
+ /* "No rows updated" has two very different causes here: the client does
+ not exist, or another client already uses @a client_redirect_uri (which
+ violates clients_uri_key, and GNUnet maps a unique violation to
+ NO_RESULTS -- indistinguishable from "not found" at the API). So detect
+ the collision ourselves and refuse the UPDATE in that case, and report
+ which of the two happened. */
PREPARE (ctx,
"update_client",
- "UPDATE clients"
- " SET uri=$2"
- " ,client_secret=COALESCE($3,client_secret)"
- " WHERE client_serial_id=$1");
- return GNUNET_PQ_eval_prepared_non_select (ctx->conn,
- "update_client",
- params);
+ "WITH target AS ("
+ " SELECT client_serial_id"
+ " FROM clients"
+ " WHERE client_serial_id=$1"
+ "), taken AS ("
+ " SELECT client_serial_id"
+ " FROM clients"
+ " WHERE uri=$2"
+ " AND client_serial_id<>$1"
+ "), upd AS ("
+ " UPDATE clients"
+ " SET uri=$2"
+ " ,client_secret=COALESCE($3,client_secret)"
+ " WHERE client_serial_id=$1"
+ " AND NOT EXISTS (SELECT 1 FROM taken)"
+ " RETURNING client_serial_id"
+ ") SELECT"
+ " EXISTS (SELECT 1 FROM upd) AS updated"
+ " ,( EXISTS (SELECT 1 FROM target)"
+ " AND EXISTS (SELECT 1 FROM taken) ) AS uri_conflict;");
+ qs = GNUNET_PQ_eval_prepared_singleton_select (ctx->conn,
+ "update_client",
+ params,
+ rs);
+ if (0 > qs)
+ return qs;
+ if (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS == qs)
+ {
+ /* Our SELECT always yields exactly one row, so zero rows can only be a
+ unique violation on clients.uri: another transaction inserted the URI
+ after our snapshot was taken. */
+ *uri_conflict = true;
+ return qs;
+ }
+ *uri_conflict = conflict;
+ return updated
+ ? GNUNET_DB_STATUS_SUCCESS_ONE_RESULT
+ : GNUNET_DB_STATUS_SUCCESS_NO_RESULTS;
}
diff --git a/src/include/challenger-database/update_client.h b/src/include/challenger-database/update_client.h
@@ -32,13 +32,23 @@ struct CHALLENGERDB_PostgresContext;
* @param client_id the client ID on success
* @param client_url URL of the client
* @param client_secret authorization secret for the client, NULL to not modify the secret
- * @return transaction status
+ * @param[out] uri_conflict set to true if @a client_url is already used by
+ * another client; only meaningful if the return value is
+ * #GNUNET_DB_STATUS_SUCCESS_NO_RESULTS, where it tells the two causes
+ * apart: true means the URI is taken, false that there is no client
+ * with row @a client_id
+ * @return transaction status:
+ * #GNUNET_DB_STATUS_SUCCESS_ONE_RESULT if the client was modified
+ * #GNUNET_DB_STATUS_SUCCESS_NO_RESULTS if the client does not exist or
+ * @a client_url is already used by another client (see @a uri_conflict)
+ * #GNUNET_DB_STATUS_HARD_ERROR on failure
*/
enum GNUNET_DB_QueryStatus
CHALLENGERDB_update_client (struct CHALLENGERDB_PostgresContext *ctx,
uint64_t client_id,
const char *client_url,
- const char *client_secret);
+ const char *client_secret,
+ bool *uri_conflict);
#endif
