Fix and improve withdrawal API

6 files changed, 135 insertions, 100 deletions

diff --git a/bank/src/main/kotlin/tech/libeufin/bank/BankIntegrationApi.kt b/bank/src/main/kotlin/tech/libeufin/bank/BankIntegrationApi.kt
index aebb8782..7a006535 100644
--- a/bank/src/main/kotlin/tech/libeufin/bank/BankIntegrationApi.kt
+++ b/bank/src/main/kotlin/tech/libeufin/bank/BankIntegrationApi.kt
@@ -38,16 +38,15 @@ fun Routing.bankIntegrationApi(db: Database, ctx: BankApplicationContext) {
 
     // Note: wopid acts as an authentication token.
     get("/taler-integration/withdrawal-operation/{wopid}") {
-        val wopid = call.expectUriComponent("wopid")
         // TODO long poll
-        val op = getWithdrawal(db, wopid) // throws 404 if not found.
+        val op = call.getWithdrawal(db, "wopid") // throws 404 if not found.
         val relatedBankAccount = db.bankAccountGetFromOwnerId(op.walletBankAccount)
             ?: throw internalServerError("Bank has a withdrawal not related to any bank account.")
         val suggestedExchange = ctx.suggestedWithdrawalExchange
         val confirmUrl = if (ctx.spaCaptchaURL == null) null else
             getWithdrawalConfirmUrl(
                 baseUrl = ctx.spaCaptchaURL,
-                wopId = wopid
+                wopId = op.withdrawalUuid
             )
         call.respond(
             BankWithdrawalOperationStatus(
@@ -62,20 +61,15 @@ fun Routing.bankIntegrationApi(db: Database, ctx: BankApplicationContext) {
         )
     }
     post("/taler-integration/withdrawal-operation/{wopid}") {
-        val wopid = call.expectUriComponent("wopid")
-        val uuid = try {
-            UUID.fromString(wopid)
-        } catch (e: Exception) {
-            throw badRequest("withdrawal_id query parameter was malformed")
-        }
+        val opId = call.uuidUriComponent("wopid")
         val req = call.receive<BankWithdrawalOperationPostRequest>()
 
         val (result, confirmationDone) = db.talerWithdrawalSetDetails(
-            uuid, req.selected_exchange, req.reserve_pub
+            opId, req.selected_exchange, req.reserve_pub
         )
         when (result) {
             WithdrawalSelectionResult.OP_NOT_FOUND -> throw notFound(
-                "Withdrawal operation $uuid not found", 
+                "Withdrawal operation $opId not found", 
                 TalerErrorCode.TALER_EC_END
             )
             WithdrawalSelectionResult.ALREADY_SELECTED -> throw conflict(
@@ -98,7 +92,7 @@ fun Routing.bankIntegrationApi(db: Database, ctx: BankApplicationContext) {
                 val confirmUrl: String? = if (ctx.spaCaptchaURL !== null && !confirmationDone) {
                     getWithdrawalConfirmUrl(
                         baseUrl = ctx.spaCaptchaURL,
-                        wopId = wopid
+                        wopId = opId
                     )
                 } else null
                 call.respond(BankWithdrawalOperationPostResponse(
diff --git a/bank/src/main/kotlin/tech/libeufin/bank/CoreBankApi.kt b/bank/src/main/kotlin/tech/libeufin/bank/CoreBankApi.kt
index 34009b8e..d47088d0 100644
--- a/bank/src/main/kotlin/tech/libeufin/bank/CoreBankApi.kt
+++ b/bank/src/main/kotlin/tech/libeufin/bank/CoreBankApi.kt
@@ -430,7 +430,7 @@ fun Routing.coreBankWithdrawalApi(db: Database, ctx: BankApplicationContext) {
             )
             WithdrawalCreationResult.ACCOUNT_IS_EXCHANGE -> throw conflict(
                 "Exchange account cannot perform withdrawal operation",
-                TalerErrorCode.TALER_EC_BANK_SAME_ACCOUNT
+                TalerErrorCode.TALER_EC_BANK_UNKNOWN_ACCOUNT
             )
             WithdrawalCreationResult.BALANCE_INSUFFICIENT -> throw conflict(
                 "Insufficient funds to withdraw with Taler",
@@ -447,7 +447,7 @@ fun Routing.coreBankWithdrawalApi(db: Database, ctx: BankApplicationContext) {
         }       
     }
     get("/withdrawals/{withdrawal_id}") {
-        val op = getWithdrawal(db, call.expectUriComponent("withdrawal_id"))
+        val op = call.getWithdrawal(db, "withdrawal_id")
         call.respond(
             BankAccountGetWithdrawalResponse(
                 amount = op.amount,
@@ -460,55 +460,44 @@ fun Routing.coreBankWithdrawalApi(db: Database, ctx: BankApplicationContext) {
         )
     }
     post("/withdrawals/{withdrawal_id}/abort") {
-        val op = getWithdrawal(db, call.expectUriComponent("withdrawal_id")) // Idempotency:
-        if (op.aborted) {
-            call.respond(HttpStatusCode.NoContent)
-            return@post
-        } // Op is found, it'll now fail only if previously confirmed (DB checks).
-        if (!db.talerWithdrawalAbort(op.withdrawalUuid)) throw conflict(
-            hint = "Cannot abort confirmed withdrawal", talerEc = TalerErrorCode.TALER_EC_END
-        )
-        call.respond(HttpStatusCode.NoContent)
+        val opId = call.uuidUriComponent("withdrawal_id")
+        when (db.talerWithdrawalAbort(opId)) {
+            WithdrawalAbortResult.NOT_FOUND -> throw notFound(
+                "Withdrawal operation $opId not found", 
+                TalerErrorCode.TALER_EC_END
+            )
+            WithdrawalAbortResult.CONFIRMED -> throw conflict(
+                "Cannot abort confirmed withdrawal", 
+                TalerErrorCode.TALER_EC_END
+            )
+            WithdrawalAbortResult.SUCCESS -> call.respond(HttpStatusCode.NoContent)
+        }
+        
     }
     post("/withdrawals/{withdrawal_id}/confirm") {
-        val op = getWithdrawal(db, call.expectUriComponent("withdrawal_id")) // Checking idempotency:
-        if (op.confirmationDone) {
-            call.respond(HttpStatusCode.NoContent)
-            return@post
-        }
-        if (op.aborted) throw conflict(
-            hint = "Cannot confirm an aborted withdrawal", talerEc = TalerErrorCode.TALER_EC_BANK_CONFIRM_ABORT_CONFLICT
-        ) // Checking that reserve GOT indeed selected.
-        if (!op.selectionDone) throw LibeufinBankException(
-            httpStatus = HttpStatusCode.UnprocessableEntity, talerError = TalerError(
-                hint = "Cannot confirm an unselected withdrawal", code = TalerErrorCode.TALER_EC_END.code
+        val opId = call.uuidUriComponent("withdrawal_id")
+        when (db.talerWithdrawalConfirm(opId, Instant.now())) {
+            WithdrawalConfirmationResult.OP_NOT_FOUND -> throw notFound(
+                "Withdrawal operation $opId not found", 
+                TalerErrorCode.TALER_EC_END
             )
-        ) // Confirmation conditions are all met, now put the operation
-        // to the selected state _and_ wire the funds to the exchange.
-        // Note: 'when' helps not to omit more result codes, should more
-        // be added.
-        when (db.talerWithdrawalConfirm(op.withdrawalUuid, Instant.now())) {
-            WithdrawalConfirmationResult.BALANCE_INSUFFICIENT -> throw conflict(
-                    "Insufficient funds",
-                    TalerErrorCode.TALER_EC_BANK_UNALLOWED_DEBIT
-                )
-            WithdrawalConfirmationResult.OP_NOT_FOUND ->
-                /**
-                 * Despite previous checks, the database _still_ did not
-                 * find the withdrawal operation, that's on the bank.
-                 */
-                throw internalServerError("Withdrawal operation (${op.withdrawalUuid}) not found")
-            WithdrawalConfirmationResult.EXCHANGE_NOT_FOUND ->
-                /**
-                 * That can happen because the bank did not check the exchange
-                 * exists when POST /withdrawals happened, or because the exchange
-                 * bank account got removed before this confirmation.
-                 */
-                throw conflict(
-                    hint = "Exchange to withdraw from not found",
-                    talerEc = TalerErrorCode.TALER_EC_BANK_UNKNOWN_ACCOUNT
+            WithdrawalConfirmationResult.ABORTED -> throw conflict(
+                "Cannot confirm an aborted withdrawal",
+                TalerErrorCode.TALER_EC_BANK_CONFIRM_ABORT_CONFLICT
+            )
+            WithdrawalConfirmationResult.NOT_SELECTED -> throw LibeufinBankException(
+                httpStatus = HttpStatusCode.UnprocessableEntity, talerError = TalerError(
+                    hint = "Cannot confirm an unselected withdrawal", code = TalerErrorCode.TALER_EC_END.code
                 )
-            WithdrawalConfirmationResult.CONFLICT -> throw internalServerError("Bank didn't check for idempotency")
+            )
+            WithdrawalConfirmationResult.BALANCE_INSUFFICIENT -> throw conflict(
+                "Insufficient funds",
+                TalerErrorCode.TALER_EC_BANK_UNALLOWED_DEBIT
+            )
+            WithdrawalConfirmationResult.EXCHANGE_NOT_FOUND -> throw conflict(
+                "Exchange to withdraw from not found",
+                TalerErrorCode.TALER_EC_BANK_UNKNOWN_ACCOUNT
+            )
             WithdrawalConfirmationResult.SUCCESS -> call.respond(HttpStatusCode.NoContent)
         }
     }
diff --git a/bank/src/main/kotlin/tech/libeufin/bank/Database.kt b/bank/src/main/kotlin/tech/libeufin/bank/Database.kt
index a28d7b05..8e4be686 100644
--- a/bank/src/main/kotlin/tech/libeufin/bank/Database.kt
+++ b/bank/src/main/kotlin/tech/libeufin/bank/Database.kt
@@ -1022,16 +1022,20 @@ class Database(dbConfig: String, private val bankCurrency: String, private val f
      * Aborts one Taler withdrawal, only if it wasn't previously
      * confirmed.  It returns false if the UPDATE didn't succeed.
      */
-    suspend fun talerWithdrawalAbort(opUUID: UUID): Boolean = conn { conn ->
+    suspend fun talerWithdrawalAbort(opUUID: UUID): WithdrawalAbortResult = conn { conn ->
         val stmt = conn.prepareStatement("""
             UPDATE taler_withdrawal_operations
-            SET aborted = true
-            WHERE withdrawal_uuid=? AND confirmation_done = false
-            RETURNING taler_withdrawal_id
+            SET aborted = NOT confirmation_done
+            WHERE withdrawal_uuid=?
+            RETURNING confirmation_done
         """
         )
         stmt.setObject(1, opUUID)
-        stmt.executeQueryCheck()
+        when (stmt.oneOrNull { it.getBoolean(1) }) {
+            null -> WithdrawalAbortResult.NOT_FOUND
+            true -> WithdrawalAbortResult.CONFIRMED
+            false -> WithdrawalAbortResult.SUCCESS
+        }
     }
 
     /**
@@ -1093,7 +1097,8 @@ class Database(dbConfig: String, private val bankCurrency: String, private val f
               out_no_op,
               out_exchange_not_found,
               out_balance_insufficient,
-              out_already_confirmed_conflict
+              out_not_selected,
+              out_aborted
             FROM confirm_taler_withdrawal(?, ?, ?, ?, ?);
         """
         )
@@ -1109,7 +1114,8 @@ class Database(dbConfig: String, private val bankCurrency: String, private val f
                 it.getBoolean("out_no_op") -> WithdrawalConfirmationResult.OP_NOT_FOUND
                 it.getBoolean("out_exchange_not_found") -> WithdrawalConfirmationResult.EXCHANGE_NOT_FOUND
                 it.getBoolean("out_balance_insufficient") -> WithdrawalConfirmationResult.BALANCE_INSUFFICIENT
-                it.getBoolean("out_already_confirmed_conflict") -> WithdrawalConfirmationResult.CONFLICT
+                it.getBoolean("out_not_selected") -> WithdrawalConfirmationResult.NOT_SELECTED
+                it.getBoolean("out_aborted") -> WithdrawalConfirmationResult.ABORTED
                 else -> WithdrawalConfirmationResult.SUCCESS
             }
         }
@@ -1586,6 +1592,13 @@ enum class WithdrawalSelectionResult {
     ACCOUNT_IS_NOT_EXCHANGE
 }
 
+/** Result status of withdrawal operation abortion */
+enum class WithdrawalAbortResult {
+    SUCCESS,
+    NOT_FOUND,
+    CONFIRMED
+}
+
 /**
  * This type communicates the result of a database operation
  * to confirm one withdrawal operation.
@@ -1595,16 +1608,8 @@ enum class WithdrawalConfirmationResult {
     OP_NOT_FOUND,
     EXCHANGE_NOT_FOUND,
     BALANCE_INSUFFICIENT,
-
-    /**
-     * This state indicates that the withdrawal was already
-     * confirmed BUT Kotlin did not detect it and still invoked
-     * the SQL procedure to confirm the withdrawal.  This is
-     * conflictual because only Kotlin is responsible to check
-     * for idempotency, and this state witnesses a failure in
-     * this regard.
-     */
-    CONFLICT
+    NOT_SELECTED,
+    ABORTED
 }
 
 private class NotificationWatcher(private val pgSource: PGSimpleDataSource) {
diff --git a/bank/src/main/kotlin/tech/libeufin/bank/helpers.kt b/bank/src/main/kotlin/tech/libeufin/bank/helpers.kt
index 2354f9e3..21d3b4c4 100644
--- a/bank/src/main/kotlin/tech/libeufin/bank/helpers.kt
+++ b/bank/src/main/kotlin/tech/libeufin/bank/helpers.kt
@@ -164,11 +164,19 @@ fun getTalerWithdrawUri(baseUrl: String, woId: String) = url {
 
 // Builds a withdrawal confirm URL.
 fun getWithdrawalConfirmUrl(
-    baseUrl: String, wopId: String
+    baseUrl: String, wopId: UUID
 ): String {
-    return baseUrl.replace("{woid}", wopId)
+    return baseUrl.replace("{woid}", wopId.toString())
 }
 
+fun ApplicationCall.uuidUriComponent(name: String): UUID {
+    try {
+        return UUID.fromString(expectUriComponent(name))
+    } catch (e: Exception) {
+        logger.error(e.message)
+        throw badRequest("UUID uri component malformed")
+    }
+}
 
 /**
  * This handler factors out the checking of the query param
@@ -177,15 +185,10 @@ fun getWithdrawalConfirmUrl(
  * if the query param doesn't parse into a UUID.  Currently
  * used by the Taler Web/SPA and Integration API handlers.
  */
-suspend fun getWithdrawal(db: Database, opIdParam: String): TalerWithdrawalOperation {
-    val opId = try {
-        UUID.fromString(opIdParam)
-    } catch (e: Exception) {
-        logger.error(e.message)
-        throw badRequest("withdrawal_id query parameter was malformed")
-    }
+suspend fun ApplicationCall.getWithdrawal(db: Database, name: String): TalerWithdrawalOperation {
+    val opId = uuidUriComponent(name)
     val op = db.talerWithdrawalGet(opId) ?: throw notFound(
-        hint = "Withdrawal operation $opIdParam not found", talerEc = TalerErrorCode.TALER_EC_END
+        hint = "Withdrawal operation $opId not found", talerEc = TalerErrorCode.TALER_EC_END
     )
     return op
 }
diff --git a/bank/src/test/kotlin/CoreBankApiTest.kt b/bank/src/test/kotlin/CoreBankApiTest.kt
index e598059b..a180e8bb 100644
--- a/bank/src/test/kotlin/CoreBankApiTest.kt
+++ b/bank/src/test/kotlin/CoreBankApiTest.kt
@@ -695,10 +695,24 @@ class CoreBankWithdrawalApiTest {
     // POST /accounts/USERNAME/withdrawals
     @Test
     fun create() = bankSetup { _ ->
+        // Check OK
         client.post("/accounts/merchant/withdrawals") {
             basicAuth("merchant", "merchant-password")
             jsonBody(json { "amount" to "KUDOS:9.0" }) 
         }.assertOk()
+
+        // Check exchange account
+        client.post("/accounts/exchange/withdrawals") {
+            basicAuth("exchange", "exchange-password")
+            jsonBody(json { "amount" to "KUDOS:9.0" }) 
+        }.assertConflict().assertErr(TalerErrorCode.TALER_EC_BANK_UNKNOWN_ACCOUNT)
+
+        // Check insufficient fund
+        client.post("/accounts/merchant/withdrawals") {
+            basicAuth("merchant", "merchant-password")
+            jsonBody(json { "amount" to "KUDOS:90" }) 
+        }.assertConflict().assertErr(TalerErrorCode.TALER_EC_BANK_UNALLOWED_DEBIT)
+
     }
 
     // GET /withdrawals/withdrawal_id
@@ -840,6 +854,35 @@ class CoreBankWithdrawalApiTest {
                 .assertErr(TalerErrorCode.TALER_EC_BANK_CONFIRM_ABORT_CONFLICT)
         }
 
+        // Check balance insufficient
+        client.post("/accounts/merchant/withdrawals") {
+            basicAuth("merchant", "merchant-password")
+            jsonBody(json { "amount" to "KUDOS:5" }) 
+        }.assertOk().run {
+            val resp = Json.decodeFromString<BankAccountCreateWithdrawalResponse>(bodyAsText())
+            val uuid = resp.taler_withdraw_uri.split("/").last()
+            client.post("/taler-integration/withdrawal-operation/$uuid") {
+                jsonBody(json {
+                    "reserve_pub" to randEddsaPublicKey()
+                    "selected_exchange" to IbanPayTo("payto://iban/EXCHANGE-IBAN-XYZ")
+                })
+            }.assertOk()
+
+            // Send too much money
+            client.post("/accounts/merchant/transactions") {
+                basicAuth("merchant", "merchant-password")
+                jsonBody(json {
+                    "payto_uri" to "payto://iban/EXCHANGE-IBAN-XYZ?message=payout&amount=KUDOS:5"
+                })
+            }.assertNoContent()
+
+            client.post("/withdrawals/$uuid/confirm").assertConflict()
+                .assertErr(TalerErrorCode.TALER_EC_BANK_UNALLOWED_DEBIT)
+
+            // Check can abort because not confirmed
+            client.post("/withdrawals/$uuid/abort").assertNoContent()
+        }
+
         // Check bad UUID
         client.post("/withdrawals/chocolate/confirm").assertBadRequest()
 
diff --git a/database-versioning/libeufin-bank-procedures.sql b/database-versioning/libeufin-bank-procedures.sql
index 7ad6cb4a..6da31ed8 100644
--- a/database-versioning/libeufin-bank-procedures.sql
+++ b/database-versioning/libeufin-bank-procedures.sql
@@ -687,11 +687,12 @@ CREATE OR REPLACE FUNCTION confirm_taler_withdrawal(
   OUT out_balance_insufficient BOOLEAN,
   OUT out_creditor_not_found BOOLEAN,
   OUT out_exchange_not_found BOOLEAN,
-  OUT out_already_confirmed_conflict BOOLEAN
+  OUT out_not_selected BOOLEAN,
+  OUT out_aborted BOOLEAN
 )
 LANGUAGE plpgsql AS $$
 DECLARE
-  confirmation_done_local BOOLEAN;
+  already_confirmed BOOLEAN;
   subject_local TEXT;
   reserve_pub_local BYTEA;
   selected_exchange_payto_local TEXT;
@@ -702,12 +703,14 @@ DECLARE
 BEGIN
 SELECT -- Really no-star policy and instead DECLARE almost one var per column?
   confirmation_done,
+  aborted, NOT selection_done,
   reserve_pub, subject,
   selected_exchange_payto,
   wallet_bank_account,
   (amount).val, (amount).frac
   INTO
-    confirmation_done_local,
+    already_confirmed,
+    out_aborted, out_not_selected,
     reserve_pub_local, subject_local,
     selected_exchange_payto_local,
     wallet_bank_account_local,
@@ -717,17 +720,10 @@ SELECT -- Really no-star policy and instead DECLARE almost one var per column?
 IF NOT FOUND THEN
   out_no_op=TRUE;
   RETURN;
+ELSIF already_confirmed OR out_aborted OR out_not_selected THEN
+  RETURN;
 END IF;
-out_no_op=FALSE;
-IF confirmation_done_local THEN
-  out_already_confirmed_conflict=TRUE
-  RETURN; -- Kotlin should have checked for idempotency before reaching here!
-END IF;
-out_already_confirmed_conflict=FALSE;
--- exists and wasn't confirmed, do it.
-UPDATE taler_withdrawal_operations
-  SET confirmation_done = true
-  WHERE withdrawal_uuid=in_withdrawal_uuid;
+
 -- sending the funds to the exchange, but needs first its bank account row ID
 SELECT
   bank_account_id
@@ -738,7 +734,7 @@ IF NOT FOUND THEN
   out_exchange_not_found=TRUE;
   RETURN;
 END IF;
-out_exchange_not_found=FALSE;
+
 SELECT -- not checking for accounts existence, as it was done above.
   transfer.out_balance_insufficient,
   out_credit_row_id
@@ -757,6 +753,11 @@ IF out_balance_insufficient THEN
   RETURN;
 END IF;
 
+-- Confirm operation
+UPDATE taler_withdrawal_operations
+  SET confirmation_done = true
+  WHERE withdrawal_uuid=in_withdrawal_uuid;
+
 -- Register incoming transaction
 CALL register_incoming(reserve_pub_local, tx_row_id, exchange_bank_account_id);
 END $$;