From 6f8c2241fcd836bad836861c4b3eb7d43504a780 Mon Sep 17 00:00:00 2001 From: Christian Grothoff Date: Sat, 5 Sep 2020 13:13:19 +0200 Subject: fix idempotency issue for POST /private/orders (#6445) --- src/backend/taler-merchant-httpd_get-orders-ID.c | 2 + .../taler-merchant-httpd_post-orders-ID-claim.c | 3 + ...taler-merchant-httpd_private-delete-orders-ID.c | 15 ++- .../taler-merchant-httpd_private-get-orders-ID.c | 3 + .../taler-merchant-httpd_private-get-orders.c | 3 + .../taler-merchant-httpd_private-post-orders.c | 125 +++++++++++++++------ src/backenddb/merchant-0001.sql | 3 + src/backenddb/plugin_merchantdb_postgres.c | 13 ++- src/backenddb/test_merchantdb.c | 46 +++++--- src/include/taler_merchantdb_plugin.h | 4 + 10 files changed, 161 insertions(+), 56 deletions(-) (limited to 'src') diff --git a/src/backend/taler-merchant-httpd_get-orders-ID.c b/src/backend/taler-merchant-httpd_get-orders-ID.c index 92c86c10..5ed92085 100644 --- a/src/backend/taler-merchant-httpd_get-orders-ID.c +++ b/src/backend/taler-merchant-httpd_get-orders-ID.c @@ -845,11 +845,13 @@ TMH_get_orders_ID (const struct TMH_RequestHandler *rh, if (! contract_match) { struct TALER_ClaimTokenP db_claim_token; + struct GNUNET_HashCode unused; qs = TMH_db->lookup_order (TMH_db->cls, hc->instance->settings.id, order_id, &db_claim_token, + &unused, (NULL == god->contract_terms) ? &god->contract_terms : NULL); diff --git a/src/backend/taler-merchant-httpd_post-orders-ID-claim.c b/src/backend/taler-merchant-httpd_post-orders-ID-claim.c index d63766cd..beef21ba 100644 --- a/src/backend/taler-merchant-httpd_post-orders-ID-claim.c +++ b/src/backend/taler-merchant-httpd_post-orders-ID-claim.c @@ -85,11 +85,14 @@ claim_order (const char *instance_id, if (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS == qs) { + struct GNUNET_HashCode unused; + /* see if we have this order in our table of unclaimed orders */ qs = TMH_db->lookup_order (TMH_db->cls, instance_id, order_id, &order_ct, + &unused, contract_terms); if (0 >= qs) { diff --git a/src/backend/taler-merchant-httpd_private-delete-orders-ID.c b/src/backend/taler-merchant-httpd_private-delete-orders-ID.c index ef272a6e..5cf091ba 100644 --- a/src/backend/taler-merchant-httpd_private-delete-orders-ID.c +++ b/src/backend/taler-merchant-httpd_private-delete-orders-ID.c @@ -62,11 +62,16 @@ TMH_private_delete_orders_ID (const struct TMH_RequestHandler *rh, TALER_EC_INTERNAL_INVARIANT_FAILURE, NULL); case GNUNET_DB_STATUS_SUCCESS_NO_RESULTS: - qs = TMH_db->lookup_order (TMH_db->cls, - mi->settings.id, - hc->infix, - NULL, - NULL); + { + struct GNUNET_HashCode unused; + + qs = TMH_db->lookup_order (TMH_db->cls, + mi->settings.id, + hc->infix, + NULL, + &unused, + NULL); + } if (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS == qs) { uint64_t order_serial; diff --git a/src/backend/taler-merchant-httpd_private-get-orders-ID.c b/src/backend/taler-merchant-httpd_private-get-orders-ID.c index 09f9dc51..e6825c5b 100644 --- a/src/backend/taler-merchant-httpd_private-get-orders-ID.c +++ b/src/backend/taler-merchant-httpd_private-get-orders-ID.c @@ -825,11 +825,14 @@ TMH_private_get_orders_ID (const struct TMH_RequestHandler *rh, &gorc->order_serial); if (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS == qs) { + struct GNUNET_HashCode unused; + /* We don't have contract terms, but the order may still exist. */ qs = TMH_db->lookup_order (TMH_db->cls, hc->instance->settings.id, hc->infix, &claim_token, + &unused, &gorc->contract_terms); order_only = true; } diff --git a/src/backend/taler-merchant-httpd_private-get-orders.c b/src/backend/taler-merchant-httpd_private-get-orders.c index c8a13382..3d4c1c5b 100644 --- a/src/backend/taler-merchant-httpd_private-get-orders.c +++ b/src/backend/taler-merchant-httpd_private-get-orders.c @@ -290,10 +290,13 @@ add_order (void *cls, } else { + struct GNUNET_HashCode unused; + qs = TMH_db->lookup_order (TMH_db->cls, aos->instance_id, order_id, NULL, + &unused, &contract_terms); } diff --git a/src/backend/taler-merchant-httpd_private-post-orders.c b/src/backend/taler-merchant-httpd_private-post-orders.c index 05bbadec..a491a9e0 100644 --- a/src/backend/taler-merchant-httpd_private-post-orders.c +++ b/src/backend/taler-merchant-httpd_private-post-orders.c @@ -185,6 +185,7 @@ struct InventoryProduct * * @param hc handler context for the request * @param order_id unique ID for the order + * @param h_post_data hash of the client's POST request, for idempotency checks * @param pay_deadline until when does the order have to be paid * @param[in] order order to process (not modified) * @param claim_token token to use for access control @@ -197,6 +198,7 @@ struct InventoryProduct static enum GNUNET_DB_QueryStatus execute_transaction (struct TMH_HandlerContext *hc, const char *order_id, + const struct GNUNET_HashCode *h_post_data, struct GNUNET_TIME_Absolute pay_deadline, json_t *order, const struct TALER_ClaimTokenP *claim_token, @@ -220,6 +222,7 @@ execute_transaction (struct TMH_HandlerContext *hc, qs = TMH_db->insert_order (TMH_db->cls, hc->instance->settings.id, order_id, + h_post_data, pay_deadline, claim_token, order); @@ -305,6 +308,7 @@ execute_transaction (struct TMH_HandlerContext *hc, * * @param connection connection to write the result or error to * @param hc handler context for the request + * @param h_post_data hash of the client's POST request, for idempotency checks * @param[in,out] order order to process (can be modified) * @param claim_token token to use for access control * @param inventory_products_length length of the @a inventory_products array @@ -316,6 +320,7 @@ execute_transaction (struct TMH_HandlerContext *hc, static MHD_RESULT execute_order (struct MHD_Connection *connection, struct TMH_HandlerContext *hc, + const struct GNUNET_HashCode *h_post_data, json_t *order, const struct TALER_ClaimTokenP *claim_token, unsigned int inventory_products_length, @@ -415,11 +420,14 @@ execute_order (struct MHD_Connection *connection, { struct TALER_ClaimTokenP token; json_t *contract_terms; + struct GNUNET_HashCode orig_post; + TMH_db->preflight (TMH_db->cls); qs = TMH_db->lookup_order (TMH_db->cls, hc->instance->settings.id, order_id, &token, + &orig_post, &contract_terms); /* If yes, check for idempotency */ if (0 > qs) @@ -432,8 +440,8 @@ execute_order (struct MHD_Connection *connection, { /* Comparing the contract terms is sufficient because all the other params get added to it at some point. */ - if (1 == json_equal (order, - contract_terms)) + if (0 == GNUNET_memcmp (&orig_post, + h_post_data)) { MHD_RESULT ret; @@ -473,6 +481,7 @@ execute_order (struct MHD_Connection *connection, TMH_db->preflight (TMH_db->cls); qs = execute_transaction (hc, order_id, + h_post_data, pay_deadline, order, claim_token, @@ -559,6 +568,7 @@ execute_order (struct MHD_Connection *connection, * * @param connection connection to write the result or error to * @param hc handler context for the request + * @param h_post_data hash of the client's POST request, for idempotency checks * @param[in,out] order order to process (can be modified) * @param claim_token token to use for access control * @param inventory_products_length length of the @a inventory_products array @@ -570,6 +580,7 @@ execute_order (struct MHD_Connection *connection, static MHD_RESULT patch_order (struct MHD_Connection *connection, struct TMH_HandlerContext *hc, + const struct GNUNET_HashCode *h_post_data, json_t *order, const struct TALER_ClaimTokenP *claim_token, struct GNUNET_TIME_Relative refund_delay, @@ -837,6 +848,7 @@ patch_order (struct MHD_Connection *connection, &hc->instance->merchant_pub))); return execute_order (connection, hc, + h_post_data, order, claim_token, inventory_products_length, @@ -853,6 +865,7 @@ patch_order (struct MHD_Connection *connection, * * @param connection connection to write the result or error to * @param hc handler context for the request + * @param h_post_data hash of the client's POST request, for idempotency checks * @param[in,out] order order to process (can be modified) * @param claim_token token to use for access control * @param payment_target desired wire method, NULL for no preference @@ -865,6 +878,7 @@ patch_order (struct MHD_Connection *connection, static MHD_RESULT add_payment_details (struct MHD_Connection *connection, struct TMH_HandlerContext *hc, + const struct GNUNET_HashCode *h_post_data, json_t *order, const struct TALER_ClaimTokenP *claim_token, struct GNUNET_TIME_Relative refund_delay, @@ -910,6 +924,7 @@ add_payment_details (struct MHD_Connection *connection, json_string (wm->wire_method))); return patch_order (connection, hc, + h_post_data, order, claim_token, refund_delay, @@ -927,6 +942,7 @@ add_payment_details (struct MHD_Connection *connection, * * @param connection connection to write the result or error to * @param hc handler context for the request + * @param h_post_data hash of the client's POST request, for idempotency checks * @param[in,out] order order to process (can be modified) * @param claim_token token to use for access control * @param inventory_products_length length of the @a inventory_products array @@ -938,6 +954,7 @@ add_payment_details (struct MHD_Connection *connection, static MHD_RESULT merge_inventory (struct MHD_Connection *connection, struct TMH_HandlerContext *hc, + const struct GNUNET_HashCode *h_post_data, json_t *order, const struct TALER_ClaimTokenP *claim_token, struct GNUNET_TIME_Relative refund_delay, @@ -1036,6 +1053,7 @@ merge_inventory (struct MHD_Connection *connection, } return add_payment_details (connection, hc, + h_post_data, order, claim_token, refund_delay, @@ -1076,6 +1094,8 @@ TMH_private_post_orders (const struct TMH_RequestHandler *rh, GNUNET_JSON_spec_end () }; enum GNUNET_GenericReturnValue ret; + struct GNUNET_HashCode h_post_data; + bool create_token = true; /* default */ (void) rh; ret = TALER_MHD_parse_json_data (connection, @@ -1087,45 +1107,41 @@ TMH_private_post_orders (const struct TMH_RequestHandler *rh, : MHD_NO; /* parse and handle the create_token (optionally given) */ + if (NULL != json_object_get (hc->request_body, + "create_token")) { - bool create_token = true; /* default */ + struct GNUNET_JSON_Specification ispec[] = { + GNUNET_JSON_spec_bool ("create_token", + &create_token), + GNUNET_JSON_spec_end () + }; + enum GNUNET_GenericReturnValue ret; - if (NULL != json_object_get (hc->request_body, - "create_token")) - { - struct GNUNET_JSON_Specification ispec[] = { - GNUNET_JSON_spec_bool ("create_token", - &create_token), - GNUNET_JSON_spec_end () - }; - enum GNUNET_GenericReturnValue ret; - - (void) rh; - ret = TALER_MHD_parse_json_data (connection, - hc->request_body, - ispec); - if (GNUNET_OK != ret) - { - GNUNET_JSON_parse_free (spec); - return (GNUNET_NO == ret) - ? MHD_YES - : MHD_NO; - } - } - if (create_token) - { - GNUNET_CRYPTO_random_block (GNUNET_CRYPTO_QUALITY_NONCE, - &claim_token, - sizeof (claim_token)); - } - else + (void) rh; + ret = TALER_MHD_parse_json_data (connection, + hc->request_body, + ispec); + if (GNUNET_OK != ret) { - /* we use all-zeros for 'no token' */ - memset (&claim_token, - 0, - sizeof (claim_token)); + GNUNET_JSON_parse_free (spec); + return (GNUNET_NO == ret) + ? MHD_YES + : MHD_NO; } } + if (create_token) + { + GNUNET_CRYPTO_random_block (GNUNET_CRYPTO_QUALITY_NONCE, + &claim_token, + sizeof (claim_token)); + } + else + { + /* we use all-zeros for 'no token' */ + memset (&claim_token, + 0, + sizeof (claim_token)); + } /* parse the refund_delay (optionally given) */ if (NULL != json_object_get (hc->request_body, "refund_delay")) @@ -1151,6 +1167,38 @@ TMH_private_post_orders (const struct TMH_RequestHandler *rh, { refund_delay.rel_value_us = 0; } + + /* Compute h_post_data (for idempotency check) */ + { + char *wire_enc; + char *input; + + if (NULL == (wire_enc = json_dumps (hc->request_body, + JSON_ENCODE_ANY + | JSON_COMPACT + | JSON_SORT_KEYS))) + { + GNUNET_break (0); + GNUNET_JSON_parse_free (spec); + return TALER_MHD_reply_with_error (connection, + MHD_HTTP_BAD_REQUEST, + TALER_EC_ALLOCATION_FAILURE, + "request body normalization for hashing"); + } + /* We include the full request: JSON body and the create_token and + refund_delay arguments */ + GNUNET_asprintf (&input, + "%s-%llu-%d\n", + wire_enc, + (unsigned long long) refund_delay.rel_value_us, + create_token ? 1 : 0); + free (wire_enc); + GNUNET_CRYPTO_hash (input, + strlen (input), + &h_post_data); + GNUNET_free (input); + } + /* parse the payment_target (optionally given) */ { const json_t *pt; @@ -1170,6 +1218,8 @@ TMH_private_post_orders (const struct TMH_RequestHandler *rh, payment_target = json_string_value (pt); } } + + /* parse the inventory_products (optionally given) */ { const json_t *ip; @@ -1226,6 +1276,7 @@ TMH_private_post_orders (const struct TMH_RequestHandler *rh, } } } + /* parse the lock_uuids (optionally given) */ { const json_t *uuid; @@ -1286,12 +1337,14 @@ TMH_private_post_orders (const struct TMH_RequestHandler *rh, } } } + /* Finally, start by completing the order */ { MHD_RESULT res; res = merge_inventory (connection, hc, + &h_post_data, order, &claim_token, refund_delay, diff --git a/src/backenddb/merchant-0001.sql b/src/backenddb/merchant-0001.sql index 0adc495b..f75cbb28 100644 --- a/src/backenddb/merchant-0001.sql +++ b/src/backenddb/merchant-0001.sql @@ -190,6 +190,7 @@ CREATE TABLE IF NOT EXISTS merchant_orders REFERENCES merchant_instances (merchant_serial) ON DELETE CASCADE ,order_id VARCHAR NOT NULL ,claim_token BYTEA NOT NULL CHECK (LENGTH(claim_token)=16) + ,h_post_data BYTEA NOT NULL CHECK (LENGTH(h_post_data)=64) ,pay_deadline INT8 NOT NULL ,creation_time INT8 NOT NULL ,contract_terms BYTEA NOT NULL @@ -199,6 +200,8 @@ COMMENT ON TABLE merchant_orders IS 'Orders we offered to a customer, but that have not yet been claimed'; COMMENT ON COLUMN merchant_orders.contract_terms IS 'Claiming changes the contract_terms, hence we have no hash of the terms in this table'; +COMMENT ON COLUMN merchant_orders.h_post_data + IS 'Hash of the POST request that created this order, for idempotency checks'; COMMENT ON COLUMN merchant_orders.claim_token IS 'Token optionally used to authorize the wallet to claim the order. All zeros (not NULL) if not used'; COMMENT ON COLUMN merchant_orders.merchant_serial diff --git a/src/backenddb/plugin_merchantdb_postgres.c b/src/backenddb/plugin_merchantdb_postgres.c index ce87ecd7..b965b0e6 100644 --- a/src/backenddb/plugin_merchantdb_postgres.c +++ b/src/backenddb/plugin_merchantdb_postgres.c @@ -1103,6 +1103,7 @@ postgres_delete_order (void *cls, * @param order_id order id used to perform the lookup * @param[out] claim_token the claim token generated for the order, * NULL to only test if the order exists + * @param[out] h_post_data set to the hash of the POST data that created the order * @param[out] contract_terms where to store the retrieved contract terms, * NULL to only test if the order exists * @return transaction status @@ -1112,6 +1113,7 @@ postgres_lookup_order (void *cls, const char *instance_id, const char *order_id, struct TALER_ClaimTokenP *claim_token, + struct GNUNET_HashCode *h_post_data, json_t **contract_terms) { struct PostgresClosure *pg = cls; @@ -1128,6 +1130,8 @@ postgres_lookup_order (void *cls, &j), GNUNET_PQ_result_spec_auto_from_type ("claim_token", &ct), + GNUNET_PQ_result_spec_auto_from_type ("h_post_data", + h_post_data), GNUNET_PQ_result_spec_end }; @@ -1340,6 +1344,7 @@ postgres_lookup_orders (void *cls, * @param cls closure * @param instance_id identifies the instance responsible for the order * @param order_id alphanumeric string that uniquely identifies the proposal + * @param h_post_order hash of the POST data for idempotency checks * @param pay_deadline how long does the customer have to pay for the order * @param claim_token token to use for access control * @param contract_terms proposal data to store @@ -1349,6 +1354,7 @@ static enum GNUNET_DB_QueryStatus postgres_insert_order (void *cls, const char *instance_id, const char *order_id, + const struct GNUNET_HashCode *h_post_order, struct GNUNET_TIME_Absolute pay_deadline, const struct TALER_ClaimTokenP *claim_token, const json_t *contract_terms) @@ -1360,6 +1366,7 @@ postgres_insert_order (void *cls, GNUNET_PQ_query_param_string (order_id), GNUNET_PQ_query_param_absolute_time (&pay_deadline), GNUNET_PQ_query_param_auto_from_type (claim_token), + GNUNET_PQ_query_param_auto_from_type (h_post_order), GNUNET_PQ_query_param_absolute_time (&now), TALER_PQ_query_param_json (contract_terms), GNUNET_PQ_query_param_end @@ -6285,6 +6292,7 @@ libtaler_plugin_merchantdb_postgres_init (void *cls) "SELECT" " contract_terms" ",claim_token" + ",h_post_data" " FROM merchant_orders" " WHERE merchant_orders.merchant_serial=" " (SELECT merchant_serial " @@ -7098,13 +7106,14 @@ libtaler_plugin_merchantdb_postgres_init (void *cls) ",order_id" ",pay_deadline" ",claim_token" + ",h_post_data" ",creation_time" ",contract_terms)" " SELECT merchant_serial," - " $2, $3, $4, $5, $6" + " $2, $3, $4, $5, $6, $7" " FROM merchant_instances" " WHERE merchant_id=$1", - 5), + 7), /* for postgres_unlock_inventory() */ GNUNET_PQ_make_prepare ("unlock_inventory", "DELETE" diff --git a/src/backenddb/test_merchantdb.c b/src/backenddb/test_merchantdb.c index afb1a380..e0ec18d4 100644 --- a/src/backenddb/test_merchantdb.c +++ b/src/backenddb/test_merchantdb.c @@ -1352,10 +1352,16 @@ test_insert_order (const struct InstanceData *instance, const struct OrderData *order, enum GNUNET_DB_QueryStatus expected_result) { + struct GNUNET_HashCode h_post; + + memset (&h_post, + 42, + sizeof (h_post)); TEST_COND_RET_ON_FAIL (expected_result == plugin->insert_order (plugin->cls, instance->instance.id, order->id, + &h_post, order->pay_deadline, &order->claim_token, order->contract), @@ -1377,11 +1383,18 @@ test_lookup_order (const struct InstanceData *instance, { struct TALER_ClaimTokenP ct; json_t *lookup_terms = NULL; + struct GNUNET_HashCode oh; + struct GNUNET_HashCode wh; + + memset (&wh, + 42, + sizeof (wh)); if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT != plugin->lookup_order (plugin->cls, instance->instance.id, order->id, &ct, + &oh, &lookup_terms)) { GNUNET_log (GNUNET_ERROR_TYPE_ERROR, @@ -1390,10 +1403,12 @@ test_lookup_order (const struct InstanceData *instance, json_decref (lookup_terms); return 1; } - if ((1 != json_equal (order->contract, - lookup_terms)) || - (0 != GNUNET_memcmp (&order->claim_token, - &ct))) + if ( (1 != json_equal (order->contract, + lookup_terms)) || + (0 != GNUNET_memcmp (&order->claim_token, + &ct)) || + (0 != GNUNET_memcmp (&oh, + &wh)) ) { GNUNET_log (GNUNET_ERROR_TYPE_ERROR, "Lookup order failed: incorrect order returned\n"); @@ -2002,16 +2017,21 @@ run_test_orders (struct TestOrders_Closure *cls) TEST_RET_ON_FAIL (test_lookup_order (&cls->instance, &cls->orders[0])); /* Make sure it fails correctly for nonexistent orders */ - if (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS != - plugin->lookup_order (plugin->cls, - cls->instance.instance.id, - cls->orders[1].id, - NULL, - NULL)) { - GNUNET_log (GNUNET_ERROR_TYPE_ERROR, - "Lookup order failed\n"); - return 1; + struct GNUNET_HashCode unused; + + if (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS != + plugin->lookup_order (plugin->cls, + cls->instance.instance.id, + cls->orders[1].id, + NULL, + &unused, + NULL)) + { + GNUNET_log (GNUNET_ERROR_TYPE_ERROR, + "Lookup order failed\n"); + return 1; + } } /* Test lookups on multiple orders */ TEST_RET_ON_FAIL (test_insert_order (&cls->instance, diff --git a/src/include/taler_merchantdb_plugin.h b/src/include/taler_merchantdb_plugin.h index a9726ce9..0270dca4 100644 --- a/src/include/taler_merchantdb_plugin.h +++ b/src/include/taler_merchantdb_plugin.h @@ -950,6 +950,7 @@ struct TALER_MERCHANTDB_Plugin * @param order_id order id used to perform the lookup * @param[out] claim_token the claim token generated for the order, * NULL to only test if the order exists + * @param[out] h_post_data set to the hash of the POST data that created the order * @param[out] contract_terms where to store the retrieved contract terms, * NULL to only test if the order exists * @return transaction status @@ -959,6 +960,7 @@ struct TALER_MERCHANTDB_Plugin const char *instance_id, const char *order_id, struct TALER_ClaimTokenP *claim_token, + struct GNUNET_HashCode *h_post_data, json_t **contract_terms); @@ -1004,6 +1006,7 @@ struct TALER_MERCHANTDB_Plugin * @param cls closure * @param instance_id identifies the instance responsible for the order * @param order_id alphanumeric string that uniquely identifies the order + * @param h_post_order hash of the POST data for idempotency checks * @param pay_deadline how long does the customer have to pay for the order * @param claim_token token to use for access control * @param contract_terms proposal data to store @@ -1013,6 +1016,7 @@ struct TALER_MERCHANTDB_Plugin (*insert_order)(void *cls, const char *instance_id, const char *order_id, + const struct GNUNET_HashCode *h_post_order, struct GNUNET_TIME_Absolute pay_deadline, const struct TALER_ClaimTokenP *claim_token, const json_t *contract_terms); -- cgit v1.2.3