Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Created API key empty just after uploading a table #15466

Closed
Jesus89 opened this issue Feb 7, 2020 · 17 comments
Closed

Created API key empty just after uploading a table #15466

Jesus89 opened this issue Feb 7, 2020 · 17 comments

Comments

@Jesus89
Copy link
Member

Jesus89 commented Feb 7, 2020

Context

This bug has been found using CARTOframes. When a map that uses a private table is published a Maps API key is created.

Steps to Reproduce

Using CARTOframes:

  1. Upload a table to CARTO: to_carto(gdf, 'new_table_name', if_exists='replace')
  2. Create a Map: map_viz = Map(Layer('new_table_name'))
  3. Publish the Map: map_viz.publish(name='New_Table_Name', password='1234', if_exists='replace')

Current Result

If the table has been just uploaded the API key is not properly created (empty tables) and there is no Exception from the backend.

Expected result

API key creation should work or at least raise an Exception.

Additional info

Related to:

cc @CartoDB/rt-managers

@oriolbx
Copy link
Contributor

oriolbx commented Feb 7, 2020

@Jesus89 This seems a bug of CARTOframes itself. If no client has reported this, I think it should be dealt as a CARTOframes issue/bug and not as an RT issue.

@Jesus89
Copy link
Member Author

Jesus89 commented Feb 7, 2020

Hi @oriolbx. It's a bug in CartoDB because the API keys creation fails and it does not return any Exception. The lib carto-python should be also checked.

CARTOframes is the way we have detected that bug and also, in order to fix this in CF, it must be fixed in CartoDB first.

The RT tag was added by @cmongut so maybe he can explain better the need for this to be in RT.

@oriolbx
Copy link
Contributor

oriolbx commented Feb 7, 2020

Oh ok, thanks! Sorry for the confusion.

@alrocar
Copy link
Contributor

alrocar commented Feb 12, 2020

@Jesus89 I'm not able to reproduce the issue at all.

Do we have a notebook that consistently fails? That would be helpful.

Thanks!

@Jesus89
Copy link
Member Author

Jesus89 commented Feb 12, 2020

[Notebook sent] :)

You can find more information in the original issues linked in the description. Q: do we have deployed changed related to API keys generation in the last 10 days?

@alrocar
Copy link
Contributor

alrocar commented Feb 12, 2020

I cannot reproduce the issue, thus I don't know what happened, but I've checked one of the wrong API keys are correct in our metadata database and the pg_role was created, but it hadn't any GRANT.

I'm going to force that very same error introducing an exception in the moment the permissions are granted, and do an E2E test locally to see what happens.

@alrocar
Copy link
Contributor

alrocar commented Feb 13, 2020

I've checked that any error sent by our API arrives to the client and it's reported to the user, so that's just fine.

Besides that I spotted the issue:

I've been looking at Kibana and I'm seeing several 422 errors in the POST to the api_keys endpoint and then a GET with a proper api_key name, so I'm assuming it is this part of the code which is being executed.

Basically, in CARTOframes the API key name is built from a hash of the layer names, which seems legit in order to reuse API keys. Since the API key name is unique, if the server raises a not valid name error, CF is trying to GET the existing API key by name, assuming it contains the required permissions.

The problem is the table referenced in the API key might not exist, so CF retrieves an API key that does not have any table granted, since the original table was previously dropped, the reference was lost and the API key is never created again.

To reproduce this:

  • Create an API key from the dashboard and grant any of your tables.
  • Run a DROP TABLE with the SQL API
  • Your API key still exists BUT it has no granted table since it was dropped.
  • Now, since API key names are hashed from the table names granted from CF, anytime you want to publish a map with the referenced table in the API key, it won't work, since it won't create a new api key but get an empty one.

There are a number of possible solutions to this situation:

  • We could try to delete API keys on DROP TABLES or refresh them on CREATE TABLES. We'll have to stop and think about the implications of this, since it involves triggers and async stuff which we know it ends up being cumbersome and problematic.
  • OTOH we are making a heavy usage of the Auth API in CARTOframes and we could try to solve this specific case. I think we should stick to the hashed api key names and I can think of a couple of fixes:
    • Recreate the API key from CF if it exists (create -> error -> delete -> create) on every map publication. It has the advantage of the api key being always up to date, but the problem that the token would change and might break other published maps with the same tables.
    • Create a refresh or touch endpoint in the Auth API, which basically will keep the same token for an existing api key, but recreate their table permissions. On creation error (due to the API key already exists) CF would call this method instead of the GET api key one. This method would only make sense for CF, since we know for sure two api keys with the same name require the same permissions and it'd work since I the grants metadata is preserved, so besides the tables might have been dropped, we know which were the tables granted for the API key.

@Jesus89
Copy link
Member Author

Jesus89 commented Feb 14, 2020

After a meeting we found 3 options:

  1. In CARTOframes: Truncate the table if it has the same schema. Then we will avoid DROP+CREATE if the columns are the same. This can be done automatically (implicit) in the 'replace' state, or more explicit using a new 'truncate' option (+1).
  2. In CARTOframes: use a temporary table to keep the API key grants. We need to validate this because if the roles are linked to the table id instead of the name id, this is not an option.
  3. In CartoDB: perform the grants every time an API key is requested (via get or create). Then, the resulting API key will always have working permissions.

Option 1 is an improvement in CARTOframes, and we should evaluate it to improve the to_carto function and include it in the backlog (@cmongut)

Option 2 needs research (@alrocar)

Option 3 can also be implemented, but we need to test it to make sure it helps/solves the issue.

@cmongut
Copy link

cmongut commented Feb 14, 2020

  1. In CARTOframes: Truncate the table if it has the same schema. Then we will avoid DROP+CREATE if the columns are the same. This can be done automatically (implicit) in the 'replace' state, or more explicit using a new 'truncate' option (+1).

If someone deletes the table from their dashboard and then, upload another one with the same name from CARTOframes, will we continue having the same issue?

  1. In CartoDB: perform the grants every time an API key is requested (via get or create). Then, the resulting API key will always have working permissions.

How this will impact the performance?

@Jesus89
Copy link
Member Author

Jesus89 commented Feb 14, 2020

Yes, I think is someone drops a table with the SQL API or the Dashboard the issue can happen again.

There is no performance impact AFAIK with option 3.

@cmongut
Copy link

cmongut commented Feb 14, 2020

Should we go fo option 3 then? Thoughts @alrocar?

@alrocar
Copy link
Contributor

alrocar commented Feb 18, 2020

After talking to @cmongut we'll go with option 3, although it's not super-prioritary 😛

@Jesus89
Copy link
Member Author

Jesus89 commented Mar 10, 2020

ping @alrocar

@alrocar
Copy link
Contributor

alrocar commented Mar 11, 2020

So we decided to go with option 3:

CartoDB: perform the grants every time an API key is requested (via get or create). Then, the resulting API key will always have working permissions.

ensure the grants on a GET request is not a nice solution. A GET request should not modify the underlying resource. So I'm proposing this other approach:

  • Create a refresh or touch endpoint like api/v3/api_keys/{id}/grants/touch
  • It'll be a PUT (or POST), it'll refresh the GRANTS for the api key role
  • It'll return the api_key + a 200 response code

The detailed steps to implement this (in case it can be implemented inside the CF project, otherwise ask for help to the backend team):

  • Add the new endpoint in routes similar to this
  • New method in the controller similar to this
  • Add a new method refresh_grants or similar in the api_key model. It should do something like this
  • Once we have this, add the client in carto-python. Similar to this
  • Last step, change the exception handler, to something like this:
api_key = self._api_key_manager.get(name)
api_key.touch() #or api_key.refresh_grants()

This looks more complicated than it actually is, but I prefer it as a solution, better than doing non-standard things in a GET method. Apart from that, I might be missing something as I've been writing as things came up to my head

@alrocar
Copy link
Contributor

alrocar commented Mar 11, 2020

In any case, remember this just solves the case of ensuring publication of maps with right api_key grants.

If the map is not re-published, eventually the API key role might have no grants and the map be broken anyway.

@Jesus89
Copy link
Member Author

Jesus89 commented Mar 11, 2020

Thanks @alrocar

@alrocar alrocar removed their assignment Mar 11, 2020
@Jesus89
Copy link
Member Author

Jesus89 commented May 14, 2020

Fixed in CartoDB/cartoframes#1633, CartoDB/cartoframes#1628. Release 1.0.3

@Jesus89 Jesus89 closed this as completed May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants