bunq / sdk_python

Python SDK for bunq API
MIT License
106 stars 25 forks source link

Add Threadsafety for x-bunq-server-signature #7

Closed PJUllrich closed 7 years ago

PJUllrich commented 7 years ago

Hello! It's me again!

I encountered a problem when making calls with the SDK in a concurrent manner. Especially, I got the following error when making calls to the production API (no errors occurred in Sandbox) Here's the stacktrace:

# Making a call with a ApiContext to the User object. I deleted calls in my own code.
    users = generated.User.list(api.Client.ctx())
  File "/usr/lib/python3.6/site-packages/bunq/sdk/model/generated/endpoint.py", line 9480, in list
    response = api_client.get(endpoint_url, custom_headers)
  File "/usr/lib/python3.6/site-packages/bunq/sdk/client.py", line 246, in get
    custom_headers
  File "/usr/lib/python3.6/site-packages/bunq/sdk/client.py", line 109, in _request
    response.headers
  File "/usr/lib/python3.6/site-packages/bunq/sdk/security.py", line 250, in validate_response
    signer.verify(digest, base64.b64decode(headers[_HEADER_SERVER_SIGNATURE]))
  File "/usr/lib/python3.6/site-packages/requests/structures.py", line 54, in __getitem__
    return self._store[key.lower()][1]
KeyError: 'x-bunq-server-signature'

Do you have an idea what could cause the problem that out of a sudden the server signature is not found anymore when making concurrent calls and no problem occurs when making sequential calls?

Update: I experienced this issue whenever I create 2 Threads which create and 'get' Payments to the production API. So basically, I make 2 independent Payments from e.g. account A to C and account B to C and then there seems to be some kind of issue with the verification function of the bunq response

dnl-blkv commented 7 years ago

@PJUllrich The most common reason of x-bunq-server-signature is when an error is returned by API and somehow missed by the self._assert_response_success(response) method.

Could you please add this:

print(response.content.decode())

to line 103 of our client.py and send the output here?

PJUllrich commented 7 years ago

I added the requested print statement in client.py and added another one in security.py between lines 249 - 250 (just before the Exception occurs). The print statement read:

print(f'From security.py / validate_response - {headers}')

I added the outputs for

  1. A response from the server that lead to the Exception and
  2. A response for a payment made at the 'same' time that didn't lead to an Exception:

Since the output was quite long, I added it to PasteBin and redacted sensitive information:

  1. https://pastebin.com/qu6qKWTJ
  2. https://pastebin.com/hWtMmNVx

As you can see, in the headers of the 1. (exception-causing) response, the x-bunq-server-signature is indeed missing. In the 2. response, said signature is available.

dnl-blkv commented 7 years ago

@PJUllrich I am interested in the response body of the request where validation fails... These outputs include everything but that body :(

PJUllrich commented 7 years ago

That one was not printed since the verification failed I figured. Let me check once again.

dnl-blkv commented 7 years ago

@PJUllrich that's why I asked adding the print statement before the verification! :)

PJUllrich commented 7 years ago

I added it there, but the output was humongous! I thought I copied everything necessary, but apparently I missed that one response body. I'll give it another try.

Also, please post the lines of code before which you want to have the print statement. You said line 103 and I put it there, but now that I inspect the code that I have, it's actually behind the verification. I will now put the print statement before this code snippet:

if self._api_context.installation_context is not None:
    security.validate_response(
        self._api_context.installation_context.public_key_server,
        response.status_code,
        response.content,
        response.headers
    )
dnl-blkv commented 7 years ago

@PJUllrich in the newest develop version of sdk_python, the lines go as:

    response = requests.request(
        method,
        self._get_uri_full(uri_relative),
        data=request_bytes,
        headers=all_headers
    )
    # line 103
    self._assert_response_success(response)

    if self._api_context.installation_context is not None:
        security.validate_response(
            self._api_context.installation_context.public_key_server,
            response.status_code,
            response.content,
            response.headers
        )
OGKevin commented 7 years ago

Aha, @dnl-blkv he is using the SDK published on PyPi. The sdk uploaded on PyPi is not up to date with the latest version of development...

@PJUllrich could you checkout the dev branch and place the sdk in your source code ? It is not possible to upload dev branch to PyPi as of I need to supply a newer version number, according to my knowledge may need to double check this.

PJUllrich commented 7 years ago

D'oh! Look at this:

{
    "Error": [{
        "error_description": "Too many requests. You can do a maximum of 3 GET call per 3 second to this endpoint.",
        "error_description_translated": "Too many requests. You can do a maximum of 3 GET call per 3 second to this endpoint."
    }]
} {
    'Date': 'Mon, 07 Aug 2017 11:42:00 GMT',
    'Server': 'Apache',
    'X-Bunq-Client-Response-Id': 'b8b95ad5-170b-4657-9ca9-41cbe06f195f',
    'X-Bunq-Client-Request-Id': '',
    'X-Frame-Options': 'SAMEORIGIN',
    'Transfer-Encoding': 'chunked',
    'Content-Type': 'application/json',
    'Strict-Transport-Security': 'max-age=31536000;'
}

Multi-threading is too fast! 🤣

Ok, I guess I have to revisit my code in order to minimise the GET requests or live with the slightly slower performance of my script. Either way, the error should probably be thrown before the verification is done, or the server should give a signature for the error response as well.

OGKevin commented 7 years ago

@PJUllrich xD. @dnl-blkv might be an idea to support multi threads for python ?

dnl-blkv commented 7 years ago

@PJUllrich here we go! I wonder though why it goes through the assertion... Ahh wait! You've got the old version where validation is done before asserting for errors :). Hold on, we'll do a proper release and you'll be able to include the newest version where this nastiness is fixed.

dnl-blkv commented 7 years ago

@OGKevin probably yes, but a bit later

PJUllrich commented 7 years ago

Well thanks anyway guys! I guess this topic can be closed now.

dnl-blkv commented 7 years ago

@PJUllrich However, our rate limiting remains non-friendly to concurrence. For now, you can workaround it by creating a shared registry storing, for every endpoint, times of last 3 GET calls, last 5 POST calls and last 2 PUT calls and checking against those if the call can already be made (or sleeping otherwise). Here's the page where the limits are specified: https://doc.bunq.com/api/1/page/errors