cooncesean / mixpanel-query-py

The Python interface to fetch data from Mixpanel.
MIT License
29 stars 17 forks source link

use new Basic authentication scheme for all API requests. #27

Closed robin900 closed 7 years ago

robin900 commented 7 years ago

https://mixpanel.com/help/reference/data-export-api#authentication

Mixpanel just shut off support for the old authentication scheme (sig, expire, etc.) at 6:26pm New York time today. So this fix is required for the package to avoid 400 responses for all API requests.

Fixes #28.

cooncesean commented 7 years ago

@thesmallestduck can we verify this tonight or tomorrow am (PST) when we get back to the office?

Seems we can still connect to Mixpanel (or we'd be seeing a number of production errors on our end, right?).

@robin900 If you need a patch faster than that, I'd recommend:

  1. Working off your fork.
  2. Moving the connection logic to a config file that allows old-style support (pre- this proposed change) as the default and optionally this new-style support introduced here.
cooncesean commented 7 years ago

Either way, we'll have this merged or feedback by noon PST tomorrow.

robin900 commented 7 years ago

@cooncesean Of course I'm already patched in production.

cooncesean commented 7 years ago

@robin900 since it doesn't sound like MP has (or will) dropped support for their old auth scheme, lets move this new auth logic to be configurable that defaults to the old scheme.

(new file) /config.py:

AUTH_SCHEME_NEW = 'new-scheme'
AUTH_SCHEME_OLD = 'old-scheme'
DEFAULT_AUTH_SCHEME = AUTH_SCHEME_OLD

connection.py:

if DEFAULT_AUTH_SCHEME == AUTH_SCHEM_OLD:
    return url_request.urlopen(request_url, timeout=self.DEFAULT_TIMEOUT if self.client.timeout is None else self.client.timeout)
else:
    request_headers = {
        'Authorization': _totext(base64.standard_b64encode(_tobytes("{}:".format(self.client.api_secret))))
    }
    request_obj = url_request.Request(request_url, headers=request_headers)
    effective_timeout = (self.DEFAULT_TIMEOUT if self.client.timeout is None else self.client.timeout)
    return url_request.urlopen(request_obj, timeout=effective_timeout)

....or something to that effect?

robin900 commented 7 years ago

@cooncesean The old auth scheme is deprecated, according to the Mixpanel documentation. The new scheme works on all API endpoints. The new scheme requires only the API secret value, which is a required argument to the client constructor already. I cannot imagine a reason to continue to use the old, deprecated auth scheme.

cooncesean commented 7 years ago

Alright -- give us a chance to pull this down and test it with against our stack. If it works w/out issue, we'll merge and cut a release.

Is this currently blocking you?

robin900 commented 7 years ago

Nope, I'm patched in production, so no hurry.

frifri commented 7 years ago

I actually like the idea of being able to choose which authentication system we want to use (see @cooncesean comment). I think we would be more comfortable merging this in if we could choose between the old and the new system. I am however not quite sure what the best way to deal with settings in this kind of project is. @robin900 do you have any input about this?

thesmallestduck commented 7 years ago

@robin900 I've attempted to add support for this means of authentication (while maintaining signature-based authentication) here: https://github.com/cooncesean/mixpanel-query-py/issues/30

When you get a chance would you mind verifying it works for you? I appreciate your PR here, and you have my apologies it's taken so long to get traction.

frifri commented 7 years ago

@robin900 Thank you for this PR but I think I will be closing it in favor of #30 since it will allow us to have support for both auth systems. Is that OK with you?