Changing the `api_key` is not safe on a threaded server

[azundo]

The api_key is set as a module global and then subsequently imported internally in the AbstractAPI class. Changing the api_key on one thread can impact the value of api_key on another thread in between changing it and invoking the api. Because of this it's not possible to safely support making requests to the beyonic api on behalf of multiple users from a threaded python server. Ideally there would be a way to create an instance of the api client that does not share state with other threads.

[lc0rp]

Hey Ben, This is noted. The original use-case was for a single client, but this makes sense. Did you figure out a fix? If so, we'll happily merge a PR. Else, we'll queue it up for fixing. - Luke. ᐧ

…

On Fri, May 24, 2019 at 7:14 PM Ben Best ***@***.***> wrote: The api_key is set as a module global and then subsequently imported internally in the AbstractAPI class. Changing the api_key on one thread can impact the value of api_key on another thread in between changing it and invoking the api. Because of this it's not possible to safely support making requests to the beyonic api on behalf of multiple users from a threaded python server. Ideally there would be a way to create an instance of the api client that does not share state with other threads. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#17?email_source=notifications&email_token=AAT5CIPRKS5E3AQC2PYOMR3PXBZFTA5CNFSM4HPTHFTKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GVZY3CA>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT5CIPFLUVKX442JSOYZBLPXBZFTANCNFSM4HPTHFTA> .

[azundo]

For now we're just going to use a lock to ensure threads don't clobber each other. We don't have enough volume for that to be any sort of a performance concern, but want to ensure we don't run into the occasional race condition. I briefly thought about how to support the existing library semantics while providing the ability to instantiate client instances. I don't have a great suggestion for how to change to a client model that can be instantiated with an api key while still allowing the current behavior of updating the api_key directly at the module level. One slightly ugly option is to duplicate much of the AbstractApi class to be instantiated with options instead of imported. I haven't tested this code in any way but I did put together a quick and dirty poc: azundo@1d0d7e3

[azundo]

Here's a snippet for how we use locking to safeguard changing the api_key:

from threading import RLock
from contextlib import contextmanager
import beyonic as beyonic_mod

beyonic_mod.api_key = None
# use a reentrant lock so `beyonic_api` contexts can be nested without deadlocking
_beyonic_lock = RLock()

@contextmanager
def beyonic_api(api_key):
    original_api_key = beyonic_mod.api_key
    with _beyonic_lock:
        try:
            beyonic_mod.api_key = api_key
            yield beyonic_mod
        finally:
            beyonic_mod.api_key = original_api_key

And example usage:

with beyonic_api(MY_API_KEY) as beyonic:
    beyonic.Payment.list()

[lc0rp]

Interesting approach!

We just released a new version of the API that may present another approach - we needed to allow folks to alter the Duplicate-Check-Key header for each request, so we enabled the ability to pass it in directly as one of the kwargs to any request as "duplicate_check_key".

The api client would pull it out just before sending the request and add it to the header. We could potentially add "api_version", or "api_key" or expose a "header_overrides" dict directly that could allow per-request overrides of any supported headers.

Relevant code below. And do test the latest version :)

def _request(self, method, **kwargs):
    # Check if duplicate_check_key is set in kwargs, and if so, add it to the header_overrides
    duplicate_check_key = kwargs.pop('duplicate_check_key', None)
    header_overrides = {'Duplicate-Check-Key': duplicate_check_key} if duplicate_check_key else {}
    response_content, status_code = self._client.request(
        method=method,
        url=self._url,
        =self._build_headers(header_overrides=header_overrides),
        params=kwargs,
    )
    return self._parse_response(response_content, status_code)

def _build_headers(self, **kwargs):
    headers = {}
    if self._api_key:
        headers.update({"Authorization": "Token %s" % self._api_key, })

    if self._api_version:
        headers.update({"Beyonic-Version": self._api_version, })

    headers.update({"Beyonic-Client": "Python", })
    headers.update({"Beyonic-Client-Version": beyonic.__version__, })

    # Override headers
    if 'header_overrides' in kwargs:
        headers.update(kwargs['header_overrides'])

    return headers

[lc0rp]

Usage would become something like this:

import beyonic

payment = beyonic.Payment.create(phonenumber='+80000000001',
                       first_name='Kennedy',
                       last_name='Amani',
                       amount='1200',
                       currency='BXC',
                       description='Per diem',
                       callback_url='https://my.website/payments/callback',
                       metadata={'id': '1234', 'name': 'Lucy'},
                       duplicate_check_key="ab594c14986612f6167a",
                       api_key="ab594c14986612f6167a975e1c369e71edab6900"
                       )

print payment  # Examine the returned object

OR

import beyonic

payment = beyonic.Payment.create(phonenumber='+80000000001',
                       first_name='Kennedy',
                       last_name='Amani',
                       amount='1200',
                       currency='BXC',
                       description='Per diem',
                       callback_url='https://my.website/payments/callback',
                       metadata={'id': '1234', 'name': 'Lucy'},
                       header_overrides={
                               "Duplicate-Check-Key": "ab594c14986612f6167a",
                               "Authorization": "Token ab594c14986612f6167a975e1c369e71edab6900"
                           }
                       )

print payment  # Examine the returned object