View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0006498Talermechant backendpublic2020-08-20 11:342024-01-12 14:04
ReporterFlorian Dold Assigned ToChristian Grothoff  
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Product Versiongit (master) 
Target Version0.8Fixed in Version0.8 
Summary0006498: /orders/{order_id} returns wrong refund amount instead of error when unauthenticated
DescriptionReproducible via

  ./testrunner 'test-merchant-refund-api'

When the client doesn't provide authentication (h_contract or token), the merchant backend returns a response instead of an error.

But this response contains a *wrong* refund amount:

{
  refunded: false,
  refund_pending: false,
  refund_amount: 'TESTKUDOS:0'
}

When giving authentication, the following response is returned instead:


{ refunded: true, refund_pending: true, refund_amount: 'TESTKUDOS:5' }
TagsNo tags attached.

Activities

jonathanbuchanan

2020-08-22 10:16

reporter   ~0016696

The merchant always requires the h_contract_terms to be correct if the order was claimed as of fa24290..838e8bf. Also, the condition for redirecting to the fulfillment url didn't match the spec, so that has been fixed as well. This needs to be tested in the merchant test suite, but the wallet integration test for refunds now passes.

Christian Grothoff

2020-08-22 13:44

manager   ~0016697

I think the above change from Jonathan is wrong, it contradicts what I think we need for browsers bookmarking order pages.
(see https://docs.taler.net/core/api-merchant.html#get--orders-$ORDER_ID). Now, *if* the order is claimed and paid and the authentication is wrong, IMO we discussed redirecting to fulfillment. For HTTP this should be via 302, for JSON via the StatusGotoResponse.

Christian Grothoff

2020-08-22 19:38

manager   ~0016702

0d05df5..1ac2e62 reverts Jonathan's patch and adds my own.

Mostly,

  if (god->unclaimed) // this line is NEW!
      token_match = (0 == GNUNET_memcmp (&db_claim_token,
                                         &god->claim_token));

as otherwise we may set 'token_match' to true if both tokens are zero (contract claimed, no token provided by client), and then
would fail to enter this branch later:

  if ( (! token_match) &&
       (! contract_match) )

I hope this helps!

Christian Grothoff

2020-08-22 19:38

manager   ~0016703

Florian: Please confirm.

Florian Dold

2020-08-23 22:15

manager   ~0016715

Nope, doesn't quite work yet.

When you refresh the /orders/{order_id} page after claiming, but *before *the payment arrived, the page must NOT redirect to the fulfillment URL yet. Otherwise, you send the user into a redirect loop that keeps on looping until the payment arrives or the browser gets tired of redirecting.

Florian Dold

2020-08-23 22:17

manager   ~0016716

Same applies to the JSON response, which (via the long-polling JavaScript) would lead to the same result.

Florian Dold

2020-08-23 22:24

manager   ~0016717

And just like the wallet integration test already demands, we should instead return 402 payment required while the order is claimed but still not paid for.

Christian Grothoff

2020-08-23 22:26

manager   ~0016718

Last edited: 2020-08-23 22:40

 About: 0016715 -- not quite so fast. If someone accesses the page with a *wrong* claim token, would redirecting them to fulfillment not (by creating a new order) get them back to the page with a *correct* claim token, thereby breaking the loop???

Christian Grothoff

2020-08-23 22:39

manager   ~0016719

I would think 1ac2e62..e357d77 fixes point 16717, but you didn't say which test shows the error.
$ ./testrunner 'test-payment-multiple'
succeeds with and without the change, and I'm not sure which test you are talking about (or then how to run it).

Florian Dold

2020-08-23 22:48

manager   ~0016720

The relevant test case here is test-paywall-flow.

Yes, for a wrong claim tokens the redirect makes sense.

But from trying to test the Android wallet on the blog demo, it looks like the redirect/202 also happens if the claim token is correct (and the order is claimed but unpaid).

Christian Grothoff

2020-08-23 23:00

manager   ~0016721

I think the wallet is wrong here now: you claimed the contract, but are still using the claim token:

GET /orders/2020.236-008F2MXFGVWAJ?token=EKCHWRFK6X3MVE9ET98YZ2Q6SW&session_id=mysession-one HTTP/1.1
Accept: application/json, text/plain, */*
User-Agent: axios/0.19.2
Host: localhost:8083
Connection: close

HTTP/1.1 202 Accepted
Connection: close
Content-Length: 56
Content-Type: application/json
Access-Control-Allow-Origin: *
Date: Sun, 23 Aug 2020 20:51:33 GMT

{
  "fulfillment_url": "https://example.com/article42"
}

Once you claimed the contract, the wallet MUST use the contract hash.

Florian Dold

2020-08-23 23:17

manager   ~0016722

Eh, this is not about the wallet!

The test case is making a request that the browser would make (via JS) on the /orders/{id} page!

(This request is not made by the wallet, but by the harness.)

Maybe you could have a look at the example I've added to the 007 design doc? That should spell out all request for the detached wallet case.

Florian Dold

2020-08-23 23:25

manager   ~0016723

https://git.taler.net/docs.git/commit/?id=e75db5acf7db8bf51d5e5e0ea7c6c19652432b89

(Somehow the docs builder seems to not work)

Christian Grothoff

2020-08-23 23:25

manager   ~0016724

Ah, I see. e357d77..8d73d07 should fix this (certainly fixes the test, shouldn't break anything else).

Florian Dold

2020-08-24 07:57

manager   ~0016726

But the bug I originally reported is still there (or re-occurred). The merchant responds with something that indicates there is no refund to the public API, even though that contradicts the private API:

  ./testrunner 'test-merchant-refund-api'

It might be a good idea to run all the test cases to ensure nothing else is broken, otherwise this is just a game of whack-a-mole:

    ./testrunner '*'

(It'll print the source files of failed test cases at the end ...)

Christian Grothoff

2020-08-24 14:19

manager   ~0016740

8d73d07..f23e2c2 implements changes discussed on Mumble, spec was updated earlier. Awaiting wallet test update.

Florian Dold

2020-08-24 16:51

manager   ~0016748

Wallet test case now passes, both with the fulfillment_url variant and the fulfillment_message variant.

Christian Grothoff

2021-09-02 18:23

manager   ~0018372

Fix committed to master branch.

Related Changesets

merchant: master 8d73d07d

2020-08-24 01:19

Christian Grothoff


Details Diff		 fix 0006498 Affected Issues
0006498
mod - src/backend/taler-merchant-httpd_get-orders-ID.c Diff File

merchant: master f23e2c2c

2020-08-24 16:12

Christian Grothoff


Details Diff		 make fulfillment URL optional, fix 0006498 as discussed Affected Issues
0006498
mod - src/backend/taler-merchant-httpd_get-orders-ID.c Diff File
mod - src/backend/taler-merchant-httpd_private-get-orders-ID.c Diff File
mod - src/backend/taler-merchant-httpd_private-post-orders.c Diff File
mod - src/backenddb/plugin_merchantdb_postgres.c Diff File

Issue History

Date Modified Username Field Change
2020-08-20 11:34 Florian Dold New Issue
2020-08-20 11:34 Florian Dold Status new => assigned
2020-08-20 11:34 Florian Dold Assigned To => Marcello Stanisci
2020-08-20 11:35 Florian Dold Assigned To Marcello Stanisci => jonathanbuchanan
2020-08-20 14:13 Florian Dold Summary /orders/{order_id} returns wrong refund amount instead of error when unathenticated => /orders/{order_id} returns wrong refund amount instead of error when unauthenticated
2020-08-22 10:16 jonathanbuchanan Note Added: 0016696
2020-08-22 13:44 Christian Grothoff Note Added: 0016697
2020-08-22 19:38 Christian Grothoff Note Added: 0016702
2020-08-22 19:38 Christian Grothoff Assigned To jonathanbuchanan => Florian Dold
2020-08-22 19:38 Christian Grothoff Status assigned => feedback
2020-08-22 19:38 Christian Grothoff Note Added: 0016703
2020-08-23 22:15 Florian Dold Note Added: 0016715
2020-08-23 22:15 Florian Dold Assigned To Florian Dold => Christian Grothoff
2020-08-23 22:17 Florian Dold Note Added: 0016716
2020-08-23 22:17 Florian Dold Status feedback => assigned
2020-08-23 22:24 Florian Dold Note Added: 0016717
2020-08-23 22:26 Christian Grothoff Note Added: 0016718
2020-08-23 22:39 Christian Grothoff Note Added: 0016719
2020-08-23 22:40 Christian Grothoff Note Edited: 0016718
2020-08-23 22:48 Florian Dold Note Added: 0016720
2020-08-23 23:00 Christian Grothoff Note Added: 0016721
2020-08-23 23:00 Christian Grothoff Assigned To Christian Grothoff => Florian Dold
2020-08-23 23:17 Florian Dold Note Added: 0016722
2020-08-23 23:25 Florian Dold Note Added: 0016723
2020-08-23 23:25 Christian Grothoff Note Added: 0016724
2020-08-23 23:26 Christian Grothoff Status assigned => feedback
2020-08-24 07:57 Florian Dold Note Added: 0016726
2020-08-24 13:09 Christian Grothoff Assigned To Florian Dold => Christian Grothoff
2020-08-24 14:19 Christian Grothoff Note Added: 0016740
2020-08-24 14:19 Christian Grothoff Product Version => git (master)
2020-08-24 14:19 Christian Grothoff Target Version => 0.8
2020-08-24 16:51 Florian Dold Status feedback => resolved
2020-08-24 16:51 Florian Dold Resolution open => fixed
2020-08-24 16:51 Florian Dold Note Added: 0016748
2020-10-03 14:09 Christian Grothoff Fixed in Version => 0.8
2021-08-24 16:23 Christian Grothoff Status resolved => closed
2021-09-02 18:22 Christian Grothoff Changeset attached => Taler-merchant master f23e2c2c
2021-09-02 18:22 Christian Grothoff Changeset attached => Taler-merchant master 8d73d07d
2021-09-02 18:23 Christian Grothoff Note Added: 0018372
2024-01-12 14:04 Christian Grothoff Category merchant backend API (C) => mechant backend