fix refund handling: allow refund increases for the same coin

contrib/gana

@@ -1 +1 @@
-Subproject commit e8c9ce8cb78b3594a1f4e6fe03c42eda5a08fb7e
+Subproject commit 73f61323554df47079e19cd4236d148e2c17a1b3

src/exchange/taler-exchange-httpd.c

@@ -238,7 +238,7 @@ handle_post_coins (const struct TEH_RequestHandler *rh,
                            root);
   return TALER_MHD_reply_with_error (connection,
                                      MHD_HTTP_NOT_FOUND,
-                                     TALER_EC_OPERATION_INVALID,
+                                     TALER_EC_OPERATION_UNKNOWN,
                                      "requested operation on coin unknown");
 }
 
@@ -1145,7 +1145,7 @@ run_main_loop (int fh,
                         (-1 == fh) ? serve_port : 0,
                         NULL, NULL,
                         &handle_mhd_request, NULL,
-                        MHD_OPTION_THREAD_POOL_SIZE, (unsigned int) 32,
+                        MHD_OPTION_THREAD_POOL_SIZE, (unsigned int) 4,
                         MHD_OPTION_LISTEN_BACKLOG_SIZE, (unsigned int) 1024,
                         MHD_OPTION_LISTEN_SOCKET, fh,
                         MHD_OPTION_EXTERNAL_LOGGER, &TALER_MHD_handle_logs,

src/exchange/taler-exchange-httpd_refund.c

@@ -105,12 +105,15 @@ refund_transaction (void *cls,
                     MHD_RESULT *mhd_ret)
 {
   const struct TALER_EXCHANGEDB_Refund *refund = cls;
-  struct TALER_EXCHANGEDB_TransactionList *tl;
+  struct TALER_EXCHANGEDB_TransactionList *tl; /* head of original list */
-  const struct TALER_EXCHANGEDB_DepositListEntry *dep;
+  struct TALER_EXCHANGEDB_TransactionList *tlx; /* head of sublist that applies to merchant and contract */
-  const struct TALER_EXCHANGEDB_RefundListEntry *ref;
+  struct TALER_EXCHANGEDB_TransactionList *tln; /* next element, during iteration */
+  struct TALER_EXCHANGEDB_TransactionList *tlp; /* previous element in 'tl' list, during iteration */
   enum GNUNET_DB_QueryStatus qs;
-  int deposit_found;
+  bool deposit_found; /* deposit_total initialized? */
-  int refund_found;
+  bool refund_found; /* refund_total initialized? */
+  struct TALER_Amount deposit_total;
+  struct TALER_Amount refund_total;
 
   tl = NULL;
   qs = TEH_plugin->get_coin_transactions (TEH_plugin->cls,
@@ -127,71 +130,159 @@ refund_transaction (void *cls,
                                              "database transaction failure");
     return qs;
   }
-  dep = NULL;
+  deposit_found = false;
-  ref = NULL;
+  refund_found = false;
-  deposit_found = GNUNET_NO;
+  tlx = NULL; /* relevant subset of transactions */
-  refund_found = GNUNET_NO;
+  tln = NULL;
-  for (struct TALER_EXCHANGEDB_TransactionList *tlp = tl;
+  tlp = NULL;
-       NULL != tlp;
+  for (struct TALER_EXCHANGEDB_TransactionList *tli = tl;
-       tlp = tlp->next)
+       NULL != tli;
+       tli = tln)
   {
-    switch (tlp->type)
+    tln = tli->next;
+    switch (tli->type)
     {
     case TALER_EXCHANGEDB_TT_DEPOSIT:
-      if (GNUNET_NO == deposit_found)
       {
-        if ( (0 == memcmp (&tlp->details.deposit->merchant_pub,
+        const struct TALER_EXCHANGEDB_DepositListEntry *dep;
-                           &refund->details.merchant_pub,
+
-                           sizeof (struct TALER_MerchantPublicKeyP))) &&
+        dep = tli->details.deposit;
-             (0 == memcmp (&tlp->details.deposit->h_contract_terms,
+        if ( (0 == GNUNET_memcmp (&dep->merchant_pub,
-                           &refund->details.h_contract_terms,
+                                  &refund->details.merchant_pub)) &&
-                           sizeof (struct GNUNET_HashCode))) )
+             (0 == GNUNET_memcmp (&dep->h_contract_terms,
+                                  &refund->details.h_contract_terms)) )
         {
-          dep = tlp->details.deposit;
+          /* check if we already send the money for this /deposit */
-          deposit_found = GNUNET_YES;
+          if (dep->done)
+          {
+            TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
+                                                    tlx);
+            TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
+                                                    tln);
+            /* money was already transferred to merchant, can no longer refund */
+            *mhd_ret = TALER_MHD_reply_with_error (connection,
+                                                   MHD_HTTP_GONE,
+                                                   TALER_EC_REFUND_MERCHANT_ALREADY_PAID,
+                                                   "money already sent to merchant");
+            return GNUNET_DB_STATUS_HARD_ERROR;
+          }
+
+          /* deposit applies and was not yet wired; add to total (it is NOT
+             the case that multiple deposits of the same coin for the same
+             contract are really allowed (see UNIQUE constraint on 'deposits'
+             table), but in case this changes we tolerate it with this code
+             anyway). *///
+          if (deposit_found)
+          {
+            GNUNET_assert (0 <=
+                           TALER_amount_add (&deposit_total,
+                                             &deposit_total,
+                                             &dep->amount_with_fee));
+          }
+          else
+          {
+            deposit_total = dep->amount_with_fee;
+            deposit_found = true;
+          }
+          /* move 'tli' from 'tl' to 'tlx' list */
+          if (NULL == tlp)
+            tl = tln;
+          else
+            tlp->next = tln;
+          tli->next = tlx;
+          tlx = tli;
           break;
         }
+        else
+        {
+          tlp = tli;
+        }
+        break;
       }
-      break;
     case TALER_EXCHANGEDB_TT_MELT:
       /* Melts cannot be refunded, ignore here */
       break;
     case TALER_EXCHANGEDB_TT_REFUND:
-      if (GNUNET_NO == refund_found)
       {
-        /* First, check if existing refund request is identical */
+        const struct TALER_EXCHANGEDB_RefundListEntry *ref;
-        if ( (0 == memcmp (&tlp->details.refund->merchant_pub,
+
-                           &refund->details.merchant_pub,
+        ref = tli->details.refund;
-                           sizeof (struct TALER_MerchantPublicKeyP))) &&
+        if ( (0 != GNUNET_memcmp (&ref->merchant_pub,
-             (0 == memcmp (&tlp->details.refund->h_contract_terms,
+                                  &refund->details.merchant_pub)) ||
-                           &refund->details.h_contract_terms,
+             (0 != GNUNET_memcmp (&ref->h_contract_terms,
-                           sizeof (struct GNUNET_HashCode))) &&
+                                  &refund->details.h_contract_terms)) )
-             (tlp->details.refund->rtransaction_id ==
-              refund->details.rtransaction_id) )
         {
-          ref = tlp->details.refund;
+          tlp = tli;
-          refund_found = GNUNET_YES;
+          break; /* refund does not apply to our transaction */
-          break;
         }
-        /* Second, check if existing refund request conflicts */
+        /* Check if existing refund request matches in everything but the amount */
-        if ( (0 == memcmp (&tlp->details.refund->merchant_pub,
+        if ( (ref->rtransaction_id ==
-                           &refund->details.merchant_pub,
+              refund->details.rtransaction_id) &&
-                           sizeof (struct TALER_MerchantPublicKeyP))) &&
+             (0 != TALER_amount_cmp (&ref->refund_amount,
-             (0 == memcmp (&tlp->details.refund->h_contract_terms,
+                                     &refund->details.refund_amount)) )
-                           &refund->details.h_contract_terms,
-                           sizeof (struct GNUNET_HashCode))) &&
-             (tlp->details.refund->rtransaction_id !=
-              refund->details.rtransaction_id) )
         {
-          refund_found = GNUNET_SYSERR;
+          /* Generate precondition failed response, with ONLY the conflicting entry */
-          /* NOTE: Alternatively we could total up all existing
+          TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-             refunds and check if the sum still permits the
+                                                  tlx);
-             refund requested (thus allowing multiple, partial
+          TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-             refunds). Fow now, we keep it simple. */
+                                                  tln);
-          break;
+          tli->next = NULL;
+          *mhd_ret = TALER_MHD_reply_json_pack (
+            connection,
+            MHD_HTTP_PRECONDITION_FAILED,
+            "{s:s, s:I, s:o}",
+            "hint",
+            "conflicting refund with different amount but same refund transaction ID",
+            "code", (json_int_t) TALER_EC_REFUND_INCONSISTENT_AMOUNT,
+            "history", TEH_RESPONSE_compile_transaction_history (
+              &refund->coin.coin_pub,
+              tli));
+          TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
+                                                  tli);
+          return GNUNET_DB_STATUS_HARD_ERROR;
         }
+        /* Check if existing refund request matches in everything including the amount */
+        if ( (ref->rtransaction_id ==
+              refund->details.rtransaction_id) &&
+             (0 == TALER_amount_cmp (&ref->refund_amount,
+                                     &refund->details.refund_amount)) )
+        {
+          /* we can blanketly approve, as this request is identical to one
+             we saw before */
+          *mhd_ret = reply_refund_success (connection,
+                                           &refund->coin.coin_pub,
+                                           ref);
+          TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
+                                                  tlx);
+          TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
+                                                  tl);
+          /* we still abort the transaction, as there is nothing to be
+             committed! */
+          return GNUNET_DB_STATUS_HARD_ERROR;
+        }
+
+        /* We have another refund, that relates, add to total */
+        if (refund_found)
+        {
+          GNUNET_assert (0 <=
+                         TALER_amount_add (&refund_total,
+                                           &refund_total,
+                                           &ref->refund_amount));
+        }
+        else
+        {
+          refund_total = ref->refund_amount;
+          refund_found = true;
+        }
+        /* move 'tli' from 'tl' to 'tlx' list */
+        if (NULL == tlp)
+          tl = tln;
+        else
+          tlp->next = tln;
+        tli->next = tlx;
+        tlx = tli;
+        break;
       }
-      break;
     case TALER_EXCHANGEDB_TT_OLD_COIN_RECOUP:
       /* Recoups cannot be refunded, ignore here */
       break;
@@ -203,54 +294,30 @@ refund_transaction (void *cls,
       break;
     }
   }
+  /* no need for 'tl' anymore, everything we may still care about is in tlx now */
+  TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
+                                          tl);
   /* handle if deposit was NOT found */
-  if (GNUNET_NO == deposit_found)
+  if (! deposit_found)
   {
     TALER_LOG_WARNING ("Deposit to /refund was not found\n");
     TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                            tl);
+                                            tlx);
     *mhd_ret = TALER_MHD_reply_with_error (connection,
                                            MHD_HTTP_NOT_FOUND,
                                            TALER_EC_REFUND_DEPOSIT_NOT_FOUND,
                                            "deposit unknown");
     return GNUNET_DB_STATUS_HARD_ERROR;
   }
-  /* handle if conflicting refund found */
-  if (GNUNET_SYSERR == refund_found)
-  {
-    *mhd_ret = TALER_MHD_reply_json_pack (
-      connection,
-      MHD_HTTP_CONFLICT,
-      "{s:s, s:I, s:o}",
-      "hint", "conflicting refund",
-      "code", (json_int_t) TALER_EC_REFUND_CONFLICT,
-      "history", TEH_RESPONSE_compile_transaction_history (
-        &refund->coin.coin_pub,
-        tl));
-    TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                            tl);
-    return GNUNET_DB_STATUS_HARD_ERROR;
-  }
-  /* handle if identical refund found */
-  if (GNUNET_YES == refund_found)
-  {
-    /* /refund already done, simply re-transmit confirmation */
-    *mhd_ret = reply_refund_success (connection,
-                                     &refund->coin.coin_pub,
-                                     ref);
-    TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                            tl);
-    return GNUNET_DB_STATUS_HARD_ERROR;
-  }
 
   /* check currency is compatible */
   if (GNUNET_YES !=
       TALER_amount_cmp_currency (&refund->details.refund_amount,
-                                 &dep->amount_with_fee))
+                                 &deposit_total))
   {
     GNUNET_break_op (0); /* currency mismatch */
     TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                            tl);
+                                            tlx);
     *mhd_ret = TALER_MHD_reply_with_error (connection,
                                            MHD_HTTP_BAD_REQUEST,
                                            TALER_EC_REFUND_CURRENCY_MISMATCH,
@@ -258,56 +325,36 @@ refund_transaction (void *cls,
     return GNUNET_DB_STATUS_HARD_ERROR;
   }
 
-  /* check if we already send the money for the /deposit */
+  /* check total refund amount is sufficiently low */
-  qs = TEH_plugin->test_deposit_done (TEH_plugin->cls,
+  if (refund_found)
-                                      session,
+    GNUNET_break (0 <=
-                                      &refund->coin.coin_pub,
+                  TALER_amount_add (&refund_total,
-                                      &dep->merchant_pub,
+                                    &refund_total,
-                                      &dep->h_contract_terms,
+                                    &refund->details.refund_amount));
-                                      &dep->h_wire);
+  else
-  if (GNUNET_DB_STATUS_HARD_ERROR == qs)
+    refund_total = refund->details.refund_amount;
-  {
-    /* Internal error, we first had the deposit in the history,
-       but now it is gone? */
-    GNUNET_break (0);
-    TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                            tl);
-    *mhd_ret = TALER_MHD_reply_with_error (connection,
-                                           MHD_HTTP_INTERNAL_SERVER_ERROR,
-                                           TALER_EC_REFUND_DB_INCONSISTENT,
-                                           "database inconsistent (deposit data became inaccessible during transaction)");
-    return qs;
-  }
-  if (GNUNET_DB_STATUS_SOFT_ERROR == qs)
-    return qs; /* go and retry */
 
-  if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT == qs)
+  if (1 == TALER_amount_cmp (&refund_total,
-  {
+                             &deposit_total) )
-    /* money was already transferred to merchant, can no longer refund */
-    TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                            tl);
-    *mhd_ret = TALER_MHD_reply_with_error (connection,
-                                           MHD_HTTP_GONE,
-                                           TALER_EC_REFUND_MERCHANT_ALREADY_PAID,
-                                           "money already sent to merchant");
-    return GNUNET_DB_STATUS_HARD_ERROR;
-  }
-
-  /* check refund amount is sufficiently low */
-  if (1 == TALER_amount_cmp (&refund->details.refund_amount,
-                             &dep->amount_with_fee) )
   {
     GNUNET_break_op (0); /* cannot refund more than original value */
+    *mhd_ret = TALER_MHD_reply_json_pack (
+      connection,
+      MHD_HTTP_CONFLICT,
+      "{s:s, s:I, s:o}",
+      "hint",
+      "total amount refunded exceeds total amount deposited for this coin",
+      "code", (json_int_t) TALER_EC_REFUND_CONFLICT_DEPOSIT_INSUFFICIENT,
+      "history", TEH_RESPONSE_compile_transaction_history (
+        &refund->coin.coin_pub,
+        tlx));
     TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                            tl);
+                                            tlx);
-    *mhd_ret = TALER_MHD_reply_with_error (connection,
-                                           MHD_HTTP_PRECONDITION_FAILED,
-                                           TALER_EC_REFUND_INSUFFICIENT_FUNDS,
-                                           "refund requested exceeds original value");
     return GNUNET_DB_STATUS_HARD_ERROR;
   }
   TEH_plugin->free_coin_transaction_list (TEH_plugin->cls,
-                                          tl);
+                                          tlx);
+
 
   /* Finally, store new refund data */
   qs = TEH_plugin->insert_refund (TEH_plugin->cls,

src/exchangedb/plugin_exchangedb_postgres.c

@@ -986,6 +986,7 @@ postgres_get_session (void *cls)
                               ",wire"
                               ",coin_sig"
                               ",deposit_serial_id"
+                              ",done"
                               " FROM deposits"
                               "    JOIN known_coins kc"
                               "      USING (coin_pub)"
@@ -2178,7 +2179,7 @@ postgres_insert_withdraw_info (
     return qs;
   }
 
-#if 0
+#if 1
   /* update reserve balance */
   reserve.pub = collectable->reserve_pub;
   if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT !=
@@ -4124,6 +4125,7 @@ add_coin_deposit (void *cls,
     chc->have_deposit_or_melt = true;
     deposit = GNUNET_new (struct TALER_EXCHANGEDB_DepositListEntry);
     {
+      uint8_t done = 0;
       struct GNUNET_PQ_ResultSpec rs[] = {
         TALER_PQ_RESULT_SPEC_AMOUNT ("amount_with_fee",
                                      &deposit->amount_with_fee),
@@ -4149,6 +4151,8 @@ add_coin_deposit (void *cls,
                                               &deposit->csig),
         GNUNET_PQ_result_spec_uint64 ("deposit_serial_id",
                                       &serial_id),
+        GNUNET_PQ_result_spec_auto_from_type ("done",
+                                              &done),
         GNUNET_PQ_result_spec_end
       };
 
@@ -4162,6 +4166,7 @@ add_coin_deposit (void *cls,
         chc->failed = true;
         return;
       }
+      deposit->done = (0 != done);
     }
     tl = GNUNET_new (struct TALER_EXCHANGEDB_TransactionList);
     tl->next = chc->head;

src/include/taler_exchangedb_plugin.h

@@ -665,6 +665,11 @@ struct TALER_EXCHANGEDB_DepositListEntry
    */
   struct TALER_Amount deposit_fee;
 
+  /**
+   * Has the deposit been wired?
+   */
+  bool done;
+
 };
 
 

src/lib/exchange_api_refund.c

@@ -153,7 +153,6 @@ handle_refund_finished (void *cls,
     .http_status = (unsigned int) response_code
   };
 
-
   rh->job = NULL;
   switch (response_code)
   {
@@ -179,7 +178,9 @@ handle_refund_finished (void *cls,
     break;
   case MHD_HTTP_BAD_REQUEST:
     /* This should never happen, either us or the exchange is buggy
-       (or API version conflict); just pass JSON reply to the application */
+       (or API version conflict); also can happen if the currency
+       differs (which we should obviously never support).
+       Just pass JSON reply to the application */
     hr.ec = TALER_JSON_get_error_code (j);
     hr.hint = TALER_JSON_get_error_hint (j);
     break;
@@ -196,6 +197,13 @@ handle_refund_finished (void *cls,
     hr.ec = TALER_JSON_get_error_code (j);
     hr.hint = TALER_JSON_get_error_hint (j);
     break;
+  case MHD_HTTP_CONFLICT:
+    /* Requested total refunds exceed deposited amount */
+    hr.ec = TALER_JSON_get_error_code (j);
+    hr.hint = TALER_JSON_get_error_hint (j);
+    // FIXME: check that 'history' contains a coin history that
+    // demonstrates that another refund would exceed the deposit amount!
+    break;
   case MHD_HTTP_GONE:
     /* Kind of normal: the money was already sent to the merchant
        (it was too late for the refund). */
@@ -203,16 +211,12 @@ handle_refund_finished (void *cls,
     hr.hint = TALER_JSON_get_error_hint (j);
     break;
   case MHD_HTTP_PRECONDITION_FAILED:
-    /* Client request was inconsistent; might be a currency mismatch
+    /* Two different refund requests were made about the same deposit, but
-       problem.  */
+       carrying identical refund transaction ids.  */
-    hr.ec = TALER_JSON_get_error_code (j);
-    hr.hint = TALER_JSON_get_error_hint (j);
-    break;
-  case MHD_HTTP_CONFLICT:
-    /* Two refund requests were made about the same deposit, but
-       carrying different refund transaction ids.  */
     hr.ec = TALER_JSON_get_error_code (j);
     hr.hint = TALER_JSON_get_error_hint (j);
+    // FIXME: check that 'history' contains a duly signed refund request
+    // for the same rtransaction ID but a different amount!
     break;
   case MHD_HTTP_INTERNAL_SERVER_ERROR:
     /* Server had an internal issue; we should retry, but this API

src/testing/test_exchange_api_twisted.c

@@ -202,7 +202,7 @@ run (void *cls,
                               "EUR:5",
                               "deposit-refund-to-fail"),
     TALER_TESTING_cmd_refund ("refund-insufficient-funds",
-                              MHD_HTTP_PRECONDITION_FAILED,
+                              MHD_HTTP_CONFLICT,
                               "EUR:50",
                               "deposit-refund-1"),
     TALER_TESTING_cmd_end ()