search

CartoDB / carto-python

CARTO Python client
https://carto.com
BSD 3-Clause "New" or "Revised" License
154 stars 62 forks source link

Auth API based client #75

Closed juanignaciosl closed 6 years ago

juanignaciosl commented 6 years ago

First, some context :_)

I'm going to implement CartoDB/cartoframes/issues/170, which will provide easy access to example datasets and maps from CARTO Frames. Those examples will be loaded from a CARTO account with example maps and datasets.

CARTO Frames always checks if the provided user and API key are valid for authentication, and that's a problem for a test account. Unless we dropped the requirement for an API key (and that's something that we all want to avoid) we must use the soon-to-be-released Auth API default public API key. Although Auth API is not feature-complete yet, today we've released an improvement that allows validating any pair of user and API key: CartoDB/cartodb/pull/13564.

Currently, APIKeyAuthClient is based on api_key parameter, and, as the migration to Auth API will be gradual, that shouldn't change soon.

As I need a client to access CARTO based on Auth API header to check validation, I propose implementing a AuthAPIKeyClient that extends APIKeyAuthClient. Its responsability is the same than APIKeyAuthClient (providing an authenticated way to send requests to CARTO), but using the header instead of the parameter.

Then, I'd move the is_authenticated method from CARTO Frames to carto-python, because it seems useful.

I'd like to create a new class, AuthAPIKeyClient, because currently, send doesn't have a way to send headers. In order to avoid creating a new class, we should add a "use auth header flag" somewhere and infest the code with ifs, which doesn't seem too clean.

Please keep in mind that I've just begun programming Python, so all of this might be bullshit :-) I wanted to share my approach before beginning coding.

What do you think, @andy-esch @alrocar @danicarrion ?

PS: the implementation of is_authenticated will be based in the improved Auth API endpoint (CartoDB/cartodb/pull/13564), removing the query to information_schema.tables.

alrocar commented 6 years ago

Looks good to me.

We had a FR to add the is_authenticated method so happy to know you are contributing it :)

About the implementation you propose seems good. Bear in mind that carto-python is heavily based on pyrestcli, maybe you want to take a look to the auth classes, specifically to this one.

Anything you need please tell me :)

juanignaciosl commented 6 years ago

Thank you very much for that! Stay tuned for a PR soon :-)

juanignaciosl commented 6 years ago

Closing this, let's follow in #69.