From 6a46b13e80659cbc9917b46f74d21b4f4aaf6c3b Mon Sep 17 00:00:00 2001 From: Florian Dold Date: Fri, 20 Mar 2020 14:01:59 +0530 Subject: [PATCH] make recoup idempotent and simplify response --- src/exchange/taler-exchange-httpd_recoup.c | 140 ++++++++------------- src/include/taler_exchange_service.h | 2 - src/include/taler_testing_lib.h | 2 - src/lib/exchange_api_recoup.c | 89 +++---------- src/testing/test_auditor_api.c | 2 - src/testing/test_exchange_api.c | 3 - src/testing/test_exchange_api_revocation.c | 16 +-- src/testing/testing_api_cmd_recoup.c | 35 +----- 8 files changed, 76 insertions(+), 213 deletions(-) diff --git a/src/exchange/taler-exchange-httpd_recoup.c b/src/exchange/taler-exchange-httpd_recoup.c index de9c13a6d..53565524d 100644 --- a/src/exchange/taler-exchange-httpd_recoup.c +++ b/src/exchange/taler-exchange-httpd_recoup.c @@ -34,112 +34,46 @@ /** - * A wallet asked for /recoup, return the successful response. + * A wallet asked for /recoup (refresh variant), return the successful + * response. * * @param connection connection to the client - * @param coin_pub coin for which we are processing the recoup request * @param old_coin_pub public key of the old coin that will receive the recoup - * @param amount the amount we will wire back - * @param timestamp when did the exchange receive the /recoup request * @return MHD result code */ static int reply_recoup_refresh_success (struct MHD_Connection *connection, - const struct TALER_CoinSpendPublicKeyP *coin_pub, const struct - TALER_CoinSpendPublicKeyP *old_coin_pub, - const struct TALER_Amount *amount, - struct GNUNET_TIME_Absolute timestamp) + TALER_CoinSpendPublicKeyP *old_coin_pub) { - struct TALER_RecoupRefreshConfirmationPS pc; - struct TALER_ExchangePublicKeyP pub; - struct TALER_ExchangeSignatureP sig; - - pc.purpose.purpose = htonl (TALER_SIGNATURE_EXCHANGE_CONFIRM_RECOUP_REFRESH); - pc.purpose.size = htonl (sizeof (struct TALER_RecoupRefreshConfirmationPS)); - pc.timestamp = GNUNET_TIME_absolute_hton (timestamp); - TALER_amount_hton (&pc.recoup_amount, - amount); - pc.coin_pub = *coin_pub; - pc.old_coin_pub = *old_coin_pub; - if (GNUNET_OK != - TEH_KS_sign (&pc.purpose, - &pub, - &sig)) - { - return TALER_MHD_reply_with_error (connection, - MHD_HTTP_INTERNAL_SERVER_ERROR, - TALER_EC_EXCHANGE_BAD_CONFIGURATION, - "no keys"); - } return TALER_MHD_reply_json_pack (connection, MHD_HTTP_OK, - "{s:o, s:o, s:o, s:o, s:o}", + "{s:o, s:b}", "old_coin_pub", GNUNET_JSON_from_data_auto ( old_coin_pub), - "timestamp", GNUNET_JSON_from_time_abs ( - timestamp), - "amount", TALER_JSON_from_amount ( - amount), - "exchange_sig", - GNUNET_JSON_from_data_auto (&sig), - "exchange_pub", - GNUNET_JSON_from_data_auto (&pub)); + "refreshed", 1); } /** - * A wallet asked for /recoup, return the successful response. + * A wallet asked for /recoup (withdraw variant), return the successful + * response. * * @param connection connection to the client - * @param coin_pub coin for which we are processing the recoup request * @param reserve_pub public key of the reserve that will receive the recoup - * @param amount the amount we will wire back - * @param timestamp when did the exchange receive the /recoup request * @return MHD result code */ static int reply_recoup_success (struct MHD_Connection *connection, - const struct TALER_CoinSpendPublicKeyP *coin_pub, - const struct TALER_ReservePublicKeyP *reserve_pub, - const struct TALER_Amount *amount, - struct GNUNET_TIME_Absolute timestamp) + const struct TALER_ReservePublicKeyP *reserve_pub) { - struct TALER_RecoupConfirmationPS pc; - struct TALER_ExchangePublicKeyP pub; - struct TALER_ExchangeSignatureP sig; - - pc.purpose.purpose = htonl (TALER_SIGNATURE_EXCHANGE_CONFIRM_RECOUP); - pc.purpose.size = htonl (sizeof (struct TALER_RecoupConfirmationPS)); - pc.timestamp = GNUNET_TIME_absolute_hton (timestamp); - TALER_amount_hton (&pc.recoup_amount, - amount); - pc.coin_pub = *coin_pub; - pc.reserve_pub = *reserve_pub; - if (GNUNET_OK != - TEH_KS_sign (&pc.purpose, - &pub, - &sig)) - { - return TALER_MHD_reply_with_error (connection, - MHD_HTTP_INTERNAL_SERVER_ERROR, - TALER_EC_EXCHANGE_BAD_CONFIGURATION, - "no keys"); - } return TALER_MHD_reply_json_pack (connection, MHD_HTTP_OK, - "{s:o, s:o, s:o, s:o, s:o}", + "{s:o, s:b}", "reserve_pub", GNUNET_JSON_from_data_auto (reserve_pub), - "timestamp", GNUNET_JSON_from_time_abs ( - timestamp), - "amount", TALER_JSON_from_amount ( - amount), - "exchange_sig", - GNUNET_JSON_from_data_auto (&sig), - "exchange_pub", - GNUNET_JSON_from_data_auto (&pub)); + "refreshed", 0); } @@ -234,7 +168,9 @@ recoup_transaction (void *cls, struct RecoupContext *pc = cls; struct TALER_EXCHANGEDB_TransactionList *tl; struct TALER_Amount spent; + struct TALER_Amount recouped; enum GNUNET_DB_QueryStatus qs; + int existing_recoup_found = GNUNET_NO; /* Check whether a recoup is allowed, and if so, to which reserve / account the money should go */ @@ -310,6 +246,21 @@ recoup_transaction (void *cls, GNUNET_assert (GNUNET_OK == TALER_amount_get_zero (pc->value.currency, &spent)); + GNUNET_assert (GNUNET_OK == + TALER_amount_get_zero (pc->value.currency, + &recouped)); + /* Check if this coin has been recouped already at least once */ + for (struct TALER_EXCHANGEDB_TransactionList *pos = tl; + NULL != pos; + pos = pos->next) + { + if ( (TALER_EXCHANGEDB_TT_RECOUP == pos->type) || + (TALER_EXCHANGEDB_TT_RECOUP_REFRESH == pos->type) ) + { + existing_recoup_found = GNUNET_YES; + break; + } + } if (GNUNET_OK != TALER_EXCHANGEDB_calculate_transaction_list_totals (tl, &spent, @@ -324,6 +275,12 @@ recoup_transaction (void *cls, "failed to calculate old coin transaction history"); return GNUNET_DB_STATUS_HARD_ERROR; } + GNUNET_log (GNUNET_ERROR_TYPE_INFO, + "Recoup: calculated spent %s\n", + TALER_amount2s (&spent)); + GNUNET_log (GNUNET_ERROR_TYPE_INFO, + "Recoup: coin value %s\n", + TALER_amount2s (&pc->value)); if (GNUNET_SYSERR == TALER_amount_subtract (&pc->amount, &pc->value, @@ -341,15 +298,26 @@ recoup_transaction (void *cls, if ( (0 == pc->amount.fraction) && (0 == pc->amount.value) ) { + int ret; TEH_plugin->rollback (TEH_plugin->cls, session); - *mhd_ret = TEH_RESPONSE_reply_coin_insufficient_funds (connection, - TALER_EC_RECOUP_COIN_BALANCE_ZERO, - &pc->coin->coin_pub, - tl); + if (GNUNET_NO == existing_recoup_found) + { + *mhd_ret = TEH_RESPONSE_reply_coin_insufficient_funds (connection, + TALER_EC_RECOUP_COIN_BALANCE_ZERO, + &pc->coin->coin_pub, + tl); + ret = GNUNET_DB_STATUS_HARD_ERROR; + } + else + { + /* We didn't add any new recoup transaction, but there was at least + one recoup before, so we give a success response. */ + ret = GNUNET_DB_STATUS_SUCCESS_NO_RESULTS; + } TEH_plugin->free_coin_transaction_list (TEH_plugin->cls, tl); - return GNUNET_DB_STATUS_HARD_ERROR; + return ret; } TEH_plugin->free_coin_transaction_list (TEH_plugin->cls, tl); @@ -548,15 +516,9 @@ verify_and_execute_recoup (struct MHD_Connection *connection, } return (refreshed) ? reply_recoup_refresh_success (connection, - &coin->coin_pub, - &pc.target.old_coin_pub, - &pc.amount, - pc.now) + &pc.target.old_coin_pub) : reply_recoup_success (connection, - &coin->coin_pub, - &pc.target.reserve_pub, - &pc.amount, - pc.now); + &pc.target.reserve_pub); } diff --git a/src/include/taler_exchange_service.h b/src/include/taler_exchange_service.h index f90cdd4a4..928a808a2 100644 --- a/src/include/taler_exchange_service.h +++ b/src/include/taler_exchange_service.h @@ -1757,8 +1757,6 @@ typedef void void *cls, unsigned int http_status, enum TALER_ErrorCode ec, - const struct TALER_Amount *amount, - struct GNUNET_TIME_Absolute timestamp, const struct TALER_ReservePublicKeyP *reserve_pub, const struct TALER_CoinSpendPublicKeyP *old_coin_pub, const json_t *full_response); diff --git a/src/include/taler_testing_lib.h b/src/include/taler_testing_lib.h index 9e32d72e6..6066aa5ee 100644 --- a/src/include/taler_testing_lib.h +++ b/src/include/taler_testing_lib.h @@ -1611,7 +1611,6 @@ TALER_TESTING_cmd_refund (const char *label, * offers a coin and reserve private key. May specify * the index of the coin using "$LABEL#$INDEX" syntax. * Here, $INDEX must be a non-negative number. - * @param amount denomination to pay back. * @param melt_reference NULL if coin was not refreshed, otherwise label of the melt operation * @return the command. */ @@ -1619,7 +1618,6 @@ struct TALER_TESTING_Command TALER_TESTING_cmd_recoup (const char *label, unsigned int expected_response_code, const char *coin_reference, - const char *amount, const char *melt_reference); diff --git a/src/lib/exchange_api_recoup.c b/src/lib/exchange_api_recoup.c index 54bcab095..a3416b5bc 100644 --- a/src/lib/exchange_api_recoup.c +++ b/src/lib/exchange_api_recoup.c @@ -88,9 +88,7 @@ struct TALER_EXCHANGE_RecoupHandle /** - * Verify that the signature on the "200 OK" response - * from the exchange is valid. If it is, call the - * callback. + * Parse a recoup response. If it is valid, call the callback. * * @param ph recoup handle * @param json json reply with the signature @@ -98,30 +96,20 @@ struct TALER_EXCHANGE_RecoupHandle * #GNUNET_SYSERR if not (callback must still be called) */ static int -verify_recoup_signature_ok (const struct TALER_EXCHANGE_RecoupHandle *ph, - const json_t *json) +process_recoup_response (const struct TALER_EXCHANGE_RecoupHandle *ph, + const json_t *json) { - struct TALER_RecoupConfirmationPS pc; - struct TALER_RecoupRefreshConfirmationPS pr; - struct TALER_ExchangePublicKeyP exchange_pub; - struct TALER_ExchangeSignatureP exchange_sig; - struct TALER_Amount amount; - struct GNUNET_TIME_Absolute timestamp; - const struct TALER_EXCHANGE_Keys *key_state; + int refreshed; + struct TALER_ReservePublicKeyP reserve_pub; + struct TALER_CoinSpendPublicKeyP old_coin_pub; struct GNUNET_JSON_Specification spec_withdraw[] = { - GNUNET_JSON_spec_fixed_auto ("exchange_sig", &exchange_sig), - GNUNET_JSON_spec_fixed_auto ("exchange_pub", &exchange_pub), - TALER_JSON_spec_amount ("amount", &amount), - GNUNET_JSON_spec_absolute_time ("timestamp", ×tamp), - GNUNET_JSON_spec_fixed_auto ("reserve_pub", &pc.reserve_pub), + GNUNET_JSON_spec_boolean ("refreshed", &refreshed), + GNUNET_JSON_spec_fixed_auto ("reserve_pub", &reserve_pub), GNUNET_JSON_spec_end () }; struct GNUNET_JSON_Specification spec_refresh[] = { - GNUNET_JSON_spec_fixed_auto ("exchange_sig", &exchange_sig), - GNUNET_JSON_spec_fixed_auto ("exchange_pub", &exchange_pub), - TALER_JSON_spec_amount ("amount", &amount), - GNUNET_JSON_spec_absolute_time ("timestamp", ×tamp), - GNUNET_JSON_spec_fixed_auto ("old_coin_pub", &pr.old_coin_pub), + GNUNET_JSON_spec_boolean ("refreshed", &refreshed), + GNUNET_JSON_spec_fixed_auto ("old_coin_pub", &old_coin_pub), GNUNET_JSON_spec_end () }; @@ -133,59 +121,16 @@ verify_recoup_signature_ok (const struct TALER_EXCHANGE_RecoupHandle *ph, GNUNET_break_op (0); return GNUNET_SYSERR; } - key_state = TALER_EXCHANGE_get_keys (ph->exchange); - if (GNUNET_OK != - TALER_EXCHANGE_test_signing_key (key_state, - &exchange_pub)) + if (ph->was_refreshed != refreshed) { GNUNET_break_op (0); return GNUNET_SYSERR; } - if (ph->was_refreshed) - { - pr.purpose.purpose = htonl ( - TALER_SIGNATURE_EXCHANGE_CONFIRM_RECOUP_REFRESH); - pr.purpose.size = htonl (sizeof (pr)); - pr.timestamp = GNUNET_TIME_absolute_hton (timestamp); - TALER_amount_hton (&pr.recoup_amount, - &amount); - pr.coin_pub = ph->coin_pub; - if (GNUNET_OK != - GNUNET_CRYPTO_eddsa_verify ( - TALER_SIGNATURE_EXCHANGE_CONFIRM_RECOUP_REFRESH, - &pr.purpose, - &exchange_sig.eddsa_signature, - &exchange_pub.eddsa_pub)) - { - GNUNET_break_op (0); - return GNUNET_SYSERR; - } - } - else - { - pc.purpose.purpose = htonl (TALER_SIGNATURE_EXCHANGE_CONFIRM_RECOUP); - pc.purpose.size = htonl (sizeof (pc)); - pc.timestamp = GNUNET_TIME_absolute_hton (timestamp); - TALER_amount_hton (&pc.recoup_amount, - &amount); - pc.coin_pub = ph->coin_pub; - if (GNUNET_OK != - GNUNET_CRYPTO_eddsa_verify (TALER_SIGNATURE_EXCHANGE_CONFIRM_RECOUP, - &pc.purpose, - &exchange_sig.eddsa_signature, - &exchange_pub.eddsa_pub)) - { - GNUNET_break_op (0); - return GNUNET_SYSERR; - } - } ph->cb (ph->cb_cls, MHD_HTTP_OK, TALER_EC_NONE, - &amount, - timestamp, - ph->was_refreshed ? NULL : &pc.reserve_pub, - ph->was_refreshed ? &pr.old_coin_pub : NULL, + ph->was_refreshed ? NULL : &reserve_pub, + ph->was_refreshed ? &old_coin_pub : NULL, json); return GNUNET_OK; } @@ -217,8 +162,8 @@ handle_recoup_finished (void *cls, case MHD_HTTP_OK: ec = TALER_EC_NONE; if (GNUNET_OK != - verify_recoup_signature_ok (ph, - j)) + process_recoup_response (ph, + j)) { GNUNET_break_op (0); ec = TALER_EC_RECOUP_REPLY_MALFORMED; @@ -256,8 +201,6 @@ handle_recoup_finished (void *cls, ph->cb (ph->cb_cls, response_code, ec, - &total, - GNUNET_TIME_UNIT_FOREVER_ABS, NULL, NULL, j); @@ -299,8 +242,6 @@ handle_recoup_finished (void *cls, response_code, ec, NULL, - GNUNET_TIME_UNIT_FOREVER_ABS, - NULL, NULL, j); TALER_EXCHANGE_recoup_cancel (ph); diff --git a/src/testing/test_auditor_api.c b/src/testing/test_auditor_api.c index eea676bd4..97934f1d1 100644 --- a/src/testing/test_auditor_api.c +++ b/src/testing/test_auditor_api.c @@ -409,7 +409,6 @@ run (void *cls, TALER_TESTING_cmd_recoup ("recoup-1", MHD_HTTP_OK, "recoup-withdraw-coin-1", - "EUR:5", NULL), /** * Re-withdraw from this reserve @@ -469,7 +468,6 @@ run (void *cls, TALER_TESTING_cmd_recoup ("recoup-2", MHD_HTTP_OK, "recoup-withdraw-coin-2a", - "EUR:0.5", NULL), TALER_TESTING_cmd_end () }; diff --git a/src/testing/test_exchange_api.c b/src/testing/test_exchange_api.c index 0913688a3..e62223303 100644 --- a/src/testing/test_exchange_api.c +++ b/src/testing/test_exchange_api.c @@ -587,7 +587,6 @@ run (void *cls, TALER_TESTING_cmd_recoup ("recoup-1", MHD_HTTP_OK, "recoup-withdraw-coin-1", - "EUR:5", NULL), /* Check the money is back with the reserve */ TALER_TESTING_cmd_status ("recoup-reserve-status-1", @@ -682,12 +681,10 @@ run (void *cls, TALER_TESTING_cmd_recoup ("recoup-2", MHD_HTTP_OK, "recoup-withdraw-coin-2a", - "EUR:0.5", NULL), TALER_TESTING_cmd_recoup ("recoup-2b", MHD_HTTP_CONFLICT, "recoup-withdraw-coin-2a", - "EUR:0.5", NULL), TALER_TESTING_cmd_deposit ("recoup-deposit-revoked", "recoup-withdraw-coin-2b", diff --git a/src/testing/test_exchange_api_revocation.c b/src/testing/test_exchange_api_revocation.c index 467b93e9a..05ec06b16 100644 --- a/src/testing/test_exchange_api_revocation.c +++ b/src/testing/test_exchange_api_revocation.c @@ -117,27 +117,31 @@ run (void *cls, TALER_TESTING_cmd_recoup ("recoup-1a", MHD_HTTP_OK, "refresh-reveal-1#0", - "EUR:1", "refresh-melt-1"), TALER_TESTING_cmd_recoup ("recoup-1b", MHD_HTTP_OK, "refresh-reveal-1#1", - "EUR:1", "refresh-melt-1"), TALER_TESTING_cmd_recoup ("recoup-1c", MHD_HTTP_OK, "refresh-reveal-1#2", - "EUR:1", + "refresh-melt-1"), + /* Repeat recoup to test idempotency */ + TALER_TESTING_cmd_recoup ("recoup-1c", + MHD_HTTP_OK, + "refresh-reveal-1#2", + "refresh-melt-1"), + TALER_TESTING_cmd_recoup ("recoup-1c", + MHD_HTTP_OK, + "refresh-reveal-1#2", "refresh-melt-1"), TALER_TESTING_cmd_recoup ("recoup-1c", MHD_HTTP_OK, "refresh-reveal-1#2", - "EUR:1", "refresh-melt-1"), TALER_TESTING_cmd_recoup ("recoup-1c", MHD_HTTP_OK, "refresh-reveal-1#2", - "EUR:1", "refresh-melt-1"), /* Now we have EUR:3.83 EUR back after 3x EUR:1 in recoups */ /* Melt original coin AGAIN, but only create one 0.1 EUR coin; @@ -168,14 +172,12 @@ run (void *cls, TALER_TESTING_cmd_recoup ("recoup-2", MHD_HTTP_OK, "refresh-reveal-2", - "EUR:0.1", "refresh-melt-2"), /* Due to recoup, original coin is now at EUR:3.79 */ /* Refund original (now zombie) coin to reserve */ TALER_TESTING_cmd_recoup ("recoup-3", MHD_HTTP_OK, "withdraw-revocation-coin-1", - "EUR:3.79", NULL), /* Check the money is back with the reserve */ TALER_TESTING_cmd_status ("recoup-reserve-status-1", diff --git a/src/testing/testing_api_cmd_recoup.c b/src/testing/testing_api_cmd_recoup.c index add54b87f..b0012f82d 100644 --- a/src/testing/testing_api_cmd_recoup.c +++ b/src/testing/testing_api_cmd_recoup.c @@ -86,11 +86,6 @@ struct RecoupState */ struct TALER_TESTING_Interpreter *is; - /** - * Amount expected to be paid back. - */ - const char *amount; - /** * Handle to the ongoing operation. */ @@ -157,9 +152,6 @@ parse_coin_reference (const char *coin_reference, * @param cls closure * @param http_status HTTP response code. * @param ec taler-specific error code. - * @param amount amount the exchange will wire back for this coin. - * @param timestamp what time did the exchange receive the - * /recoup request * @param reserve_pub public key of the reserve receiving the recoup, NULL if refreshed or on error * @param old_coin_pub public key of the dirty coin, NULL if not refreshed or on error * @param full_response raw response from the exchange. @@ -168,8 +160,6 @@ static void recoup_cb (void *cls, unsigned int http_status, enum TALER_ErrorCode ec, - const struct TALER_Amount *amount, - struct GNUNET_TIME_Absolute timestamp, const struct TALER_ReservePublicKeyP *reserve_pub, const struct TALER_CoinSpendPublicKeyP *old_coin_pub, const json_t *full_response) @@ -219,28 +209,7 @@ recoup_cb (void *cls, switch (http_status) { case MHD_HTTP_OK: - /* first, check amount */ - { - struct TALER_Amount expected_amount; - - if (GNUNET_OK != - TALER_string_to_amount (ps->amount, &expected_amount)) - { - GNUNET_break (0); - TALER_TESTING_interpreter_fail (is); - return; - } - if (0 != TALER_amount_cmp (amount, &expected_amount)) - { - GNUNET_log (GNUNET_ERROR_TYPE_ERROR, - "Total amount mismatch to command %s\n", - cmd->label); - json_dumpf (full_response, stderr, 0); - TALER_TESTING_interpreter_fail (is); - return; - } - } - /* now, check old_coin_pub or reserve_pub, respectively */ + /* check old_coin_pub or reserve_pub, respectively */ if (NULL != ps->melt_reference) { const struct TALER_TESTING_Command *melt_cmd; @@ -559,7 +528,6 @@ struct TALER_TESTING_Command TALER_TESTING_cmd_recoup (const char *label, unsigned int expected_response_code, const char *coin_reference, - const char *amount, const char *melt_reference) { struct RecoupState *ps; @@ -567,7 +535,6 @@ TALER_TESTING_cmd_recoup (const char *label, ps = GNUNET_new (struct RecoupState); ps->expected_response_code = expected_response_code; ps->coin_reference = coin_reference; - ps->amount = amount; ps->melt_reference = melt_reference; { struct TALER_TESTING_Command cmd = {