From 4e0b5104382a1271261a24c4b97cb50f63aea976 Mon Sep 17 00:00:00 2001 From: Christian Grothoff Date: Thu, 13 Aug 2020 00:45:02 +0200 Subject: extend tests to discover corner cases from #6478, fix code to actually work for those cases --- src/exchange/taler-exchange-httpd_refund.c | 1 - src/lib/exchange_api_common.c | 17 ++++++-- src/lib/exchange_api_refund.c | 65 ++++++++++++++++++------------ src/testing/test_exchange_api.c | 31 ++++++++++---- 4 files changed, 77 insertions(+), 37 deletions(-) (limited to 'src') diff --git a/src/exchange/taler-exchange-httpd_refund.c b/src/exchange/taler-exchange-httpd_refund.c index b60a93b5d..955f1c51d 100644 --- a/src/exchange/taler-exchange-httpd_refund.c +++ b/src/exchange/taler-exchange-httpd_refund.c @@ -337,7 +337,6 @@ refund_transaction (void *cls, if (1 == TALER_amount_cmp (&refund_total, &deposit_total) ) { - GNUNET_break_op (0); /* cannot refund more than original value */ *mhd_ret = TALER_MHD_reply_json_pack ( connection, MHD_HTTP_CONFLICT, diff --git a/src/lib/exchange_api_common.c b/src/lib/exchange_api_common.c index b07ac1112..3591a7bbc 100644 --- a/src/lib/exchange_api_common.c +++ b/src/lib/exchange_api_common.c @@ -643,6 +643,7 @@ TALER_EXCHANGE_verify_coin_history ( { struct TALER_MerchantSignatureP sig; struct TALER_Amount refund_fee; + struct TALER_Amount sig_amount; struct TALER_RefundRequestPS rr = { .purpose.size = htonl (sizeof (rr)), .purpose.purpose = htonl (TALER_SIGNATURE_MERCHANT_REFUND), @@ -658,7 +659,7 @@ TALER_EXCHANGE_verify_coin_history ( GNUNET_JSON_spec_fixed_auto ("merchant_pub", &rr.merchant), GNUNET_JSON_spec_uint64 ("rtransaction_id", - &rr.rtransaction_id), // FIXME: shouldn't this be NBO!? + &rr.rtransaction_id), GNUNET_JSON_spec_end () }; @@ -670,9 +671,19 @@ TALER_EXCHANGE_verify_coin_history ( GNUNET_break_op (0); return GNUNET_SYSERR; } - abort (); // FIXME: this shows the case is not tested! ... + if (0 > + TALER_amount_add (&sig_amount, + &refund_fee, + &amount)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } TALER_amount_hton (&rr.refund_amount, - &amount); + &sig_amount); + rr.rtransaction_id = GNUNET_htonll (rr.rtransaction_id); + TALER_amount_hton (&rr.refund_amount, + &sig_amount); if (GNUNET_OK != GNUNET_CRYPTO_eddsa_verify (TALER_SIGNATURE_MERCHANT_REFUND, &rr, diff --git a/src/lib/exchange_api_refund.c b/src/lib/exchange_api_refund.c index 7c758ba2a..ee634e7f6 100644 --- a/src/lib/exchange_api_refund.c +++ b/src/lib/exchange_api_refund.c @@ -162,10 +162,6 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, GNUNET_break_op (0); return GNUNET_SYSERR; } - // FIXME: check that 'history' contains a coin history that - // demonstrates that another refund would exceed the deposit amount! - - len = json_array_size (history); if (0 == len) { @@ -197,13 +193,6 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, GNUNET_break_op (0); return GNUNET_SYSERR; } - if (GNUNET_YES != - TALER_amount_cmp_currency (&amount, - &rtotal)) - { - GNUNET_break_op (0); - return GNUNET_SYSERR; - } if (0 == strcasecmp (type, "DEPOSIT")) { @@ -264,6 +253,13 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, if (have_deposit) { /* this cannot really happen, but we conservatively support it anyway */ + if (GNUNET_YES != + TALER_amount_cmp_currency (&amount, + &dtotal)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } GNUNET_break (0 <= TALER_amount_add (&dtotal, &dtotal, @@ -280,6 +276,7 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, { struct TALER_MerchantSignatureP sig; struct TALER_Amount refund_fee; + struct TALER_Amount sig_amount; struct TALER_RefundRequestPS rr = { .purpose.size = htonl (sizeof (rr)), .purpose.purpose = htonl (TALER_SIGNATURE_MERCHANT_REFUND), @@ -307,8 +304,17 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, GNUNET_break_op (0); return GNUNET_SYSERR; } + if (0 > + TALER_amount_add (&sig_amount, + &refund_fee, + &amount)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } TALER_amount_hton (&rr.refund_amount, - &amount); + &sig_amount); + rr.rtransaction_id = GNUNET_htonll (rr.rtransaction_id); if (GNUNET_OK != GNUNET_CRYPTO_eddsa_verify (TALER_SIGNATURE_MERCHANT_REFUND, &rr, @@ -318,7 +324,6 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, GNUNET_break_op (0); return GNUNET_SYSERR; } - if ( (0 != GNUNET_memcmp (&rh->depconf.h_contract_terms, &rr.h_contract_terms)) || (0 != GNUNET_memcmp (&rh->depconf.merchant, @@ -338,6 +343,13 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, if (have_refund) { + if (GNUNET_YES != + TALER_amount_cmp_currency (&amount, + &rtotal)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } GNUNET_break (0 <= TALER_amount_add (&rtotal, &rtotal, @@ -352,10 +364,10 @@ verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, else { /* unexpected type, new version on server? */ - GNUNET_break_op (0); GNUNET_log (GNUNET_ERROR_TYPE_ERROR, "Unexpected type `%s' in response\n", type); + GNUNET_break_op (0); return GNUNET_SYSERR; } } @@ -561,6 +573,7 @@ handle_refund_finished (void *cls, j)) { GNUNET_break (0); + hr.http_status = 0; hr.ec = TALER_EC_REFUND_INVALID_FAILURE_PROOF_BY_EXCHANGE; hr.hint = "conflict information provided by exchange is invalid"; break; @@ -580,6 +593,7 @@ handle_refund_finished (void *cls, j)) { GNUNET_break (0); + hr.http_status = 0; hr.ec = TALER_EC_REFUND_INVALID_FAILURE_PROOF_BY_EXCHANGE; hr.hint = "failed precondition proof returned by exchange is invalid"; break; @@ -653,29 +667,30 @@ TALER_EXCHANGE_refund (struct TALER_EXCHANGE_Handle *exchange, TALER_EXCHANGE_RefundCallback cb, void *cb_cls) { - struct TALER_RefundRequestPS rr; + struct TALER_RefundRequestPS rr = { + .purpose.purpose = htonl (TALER_SIGNATURE_MERCHANT_REFUND), + .purpose.size = htonl (sizeof (rr)), + .h_contract_terms = *h_contract_terms, + .rtransaction_id = GNUNET_htonll (rtransaction_id), + .coin_pub = *coin_pub + }; struct TALER_MerchantSignatureP merchant_sig; + struct TALER_EXCHANGE_RefundHandle *rh; + struct GNUNET_CURL_Context *ctx; + json_t *refund_obj; + CURL *eh; + char arg_str[sizeof (struct TALER_CoinSpendPublicKeyP) * 2 + 32]; GNUNET_assert (GNUNET_YES == TEAH_handle_is_ready (exchange)); - rr.purpose.purpose = htonl (TALER_SIGNATURE_MERCHANT_REFUND); - rr.purpose.size = htonl (sizeof (struct TALER_RefundRequestPS)); - rr.h_contract_terms = *h_contract_terms; - rr.coin_pub = *coin_pub; GNUNET_CRYPTO_eddsa_key_get_public (&merchant_priv->eddsa_priv, &rr.merchant.eddsa_pub); - rr.rtransaction_id = GNUNET_htonll (rtransaction_id); TALER_amount_hton (&rr.refund_amount, amount); GNUNET_CRYPTO_eddsa_sign (&merchant_priv->eddsa_priv, &rr, &merchant_sig.eddsa_sig); - struct TALER_EXCHANGE_RefundHandle *rh; - struct GNUNET_CURL_Context *ctx; - json_t *refund_obj; - CURL *eh; - char arg_str[sizeof (struct TALER_CoinSpendPublicKeyP) * 2 + 32]; { char pub_str[sizeof (struct TALER_CoinSpendPublicKeyP) * 2]; diff --git a/src/testing/test_exchange_api.c b/src/testing/test_exchange_api.c index e410b1805..d30597b20 100644 --- a/src/testing/test_exchange_api.c +++ b/src/testing/test_exchange_api.c @@ -552,20 +552,35 @@ run (void *cls, * fakebank and the second to actually check there are not * other transfers around. */// TALER_TESTING_cmd_check_bank_empty ("check_bank_transfer-pre-refund"), - TALER_TESTING_cmd_refund ("refund-ok", - MHD_HTTP_OK, - "EUR:5", - "deposit-refund-1"), - TALER_TESTING_cmd_refund ("refund-ok-double", - MHD_HTTP_OK, - "EUR:5", - "deposit-refund-1"), + TALER_TESTING_cmd_refund_with_id ("refund-ok", + MHD_HTTP_OK, + "EUR:3", + "deposit-refund-1", + 3), + TALER_TESTING_cmd_refund_with_id ("refund-ok-double", + MHD_HTTP_OK, + "EUR:3", + "deposit-refund-1", + 3), /* Previous /refund(s) had id == 0. */ TALER_TESTING_cmd_refund_with_id ("refund-conflicting", MHD_HTTP_CONFLICT, "EUR:5", "deposit-refund-1", 1), + TALER_TESTING_cmd_deposit ("deposit-refund-insufficient-refund", + "withdraw-coin-r1", + 0, + bc.user42_payto, + "{\"items\":[{\"name\":\"ice cream\",\"value\":\"EUR:4\"}]}", + GNUNET_TIME_UNIT_MINUTES, + "EUR:4", + MHD_HTTP_CONFLICT), + TALER_TESTING_cmd_refund_with_id ("refund-ok-increase", + MHD_HTTP_OK, + "EUR:2", + "deposit-refund-1", + 2), /** * Spend 4.99 EUR of the refunded 4.99 EUR coin (1ct gone * due to refund) (merchant would receive EUR:4.98 due to -- cgit v1.2.3