Auth Bug: updating auth does not change results of catalog fetch

[sarahzinger]

What happened:
I was editing a config and was unable to get new catalogs for new user information back. The only way to force this for me was to to delete the datasource and come back to it.

What you expected to happen:
I would expect us to make a new catalogs request anytime other user information changes.

How to reproduce it (as minimally and precisely as possible):

1. Go to config to add a new athena datasource plugin
2. Select the keys auth method
3. Type in an incorrect key and/or region and select Choose Datasource, it'll say no options found which is expected
4. now type in the correct keys and click Choose Datasource, you'll see there's no new results and no request is made in the network tab for /catalogs (don't touch the region)
5. I then refreshed the page and tried typing in the correct keys and tried selecting Choose Datasource and found the same bug
6. If I saved, I'd get an error because I didn't have a datasource selected, and on refresh it was still broken and threw an error in the network tab for /catalogs with the error message UnrecognizedClientException: The security token included in the request is invalid. status code: 400, request id: THE_REQUEST_ID
7. the way I was able to get around this was to delete the datasource and re-add it and type in the correct credentials the first time.

Screenshots

I'm not entirely sure why this is happening, it seems like a combo problem:

1. we don't refetch whenever we click on this Choose Datasource when we probably should
2. It seems like we don't make the request to /catalogs with the latest auth info but with whatever is stored in our session or something like that.

Anything else we need to know?:

Environment:

• Grafana version: 8.4.6
• Plugin version: 1.0.4

[sarahzinger]

I think this is the same: #141

Another fix here that i didn't write in this ticket would be to show a permissions error to the user as described in that ticket

[andresmgot]

FTR, how this should work is that if any change is made to the generic AWS info (e.g. the secret key), that should mark the config as "unsaved". That should trigger a new save and to re-fetch catalogs when clicking in the selector but seems that it's not working.

[sarahzinger]

Hey, sorry just an update on this one before I head out of office for a bit. I had hoped to be able to finish this before I go, but it looks like I'm not going to be able to. If someone wants to pick this up I would understand, otherwise I'll get back to it when I get back in.

We merged and released a fix in the dependency package, here: grafana/grafana-aws-sdk-react#20 the general gist is that ResourceSelectors have a list of dependencies that it refetches on, and secret key was missing from that list so we never made the backend call.

I was then going to install that package into athena, but in my testing I realized the bug still exists! While we now make the network call when we should, the response we get back is not accurate. This is is for 2 reasons as far as I can tell:

We cache sessions with a cache key that does not update when a secret key is updated:
https://github.com/grafana/grafana-aws-sdk/blob/3a212f1b20f9ea0f76f86a0c5595cb6ab14b44f2/pkg/awsds/sessions.go#L149

we store/load api's with a "connectionKey" that does not update when a secret key is updated:
https://github.com/grafana/grafana-aws-sdk/blob/main/pkg/sql/datasource/datasource.go#L50

I was still exploring ways to fix this. One thing I noticed and found interesting was that when we make an update to a datasource we get back a version property. It seemed like a potentially good thing to make a cache key/connection key on, but I did not find an obvious way to access that within sessions (but it's very possible I missed it somewhere). I have spent most of my exploratory work looking into this, but I haven't quite landed on a solution I like here.

A maybe easier fix would be to simply add the properties that could change to the cache key, but I feel like this is dangerous from a security perspective, tbh I already think it's weird we're using this properties at all in storage. We could do some kind of hash or something?

Another thought that occurs to me is why do we have 2 forms of caching at all? Maybe the idea of a both a cache key and a connection key is not necessary. I'm not entirely sure I see a meaningful distinction between the two but I'm also not as familiar as I'd like to be with our auth stuff and assume there are reasons.

Anyway, I think this bug is bigger than I had anticipated and might require a bigger look at caching in aws plugins overall, I sort of assume that whatever difficulties we've seen in athena could exist for all aws plugins, although I haven't heard anything to that affect, which makes me question my own analysis here. I think this could probably use another set of eyes to see if there's something simple and silly I'm missing.

[andresmgot]

We cache sessions with a cache key that does not update when a secret key is updated: https://github.com/grafana/grafana-aws-sdk/blob/3a212f1b20f9ea0f76f86a0c5595cb6ab14b44f2/pkg/awsds/sessions.go#L149

Agree, this one seems the culprit.

we store/load api's with a "connectionKey" that does not update when a secret key is updated: https://github.com/grafana/grafana-aws-sdk/blob/main/pkg/sql/datasource/datasource.go#L50

In theory (we may need to double check this), this cache depends on the api Mux, and this api is instantiated with the given settings. This mean that any change in the settings should create a new api and the value stored in the cache should be overriden (regardless of the key).

A maybe easier fix would be to simply add the properties that could change to the cache key, but I feel like this is dangerous from a security perspective, tbh I already think it's weird we're using this properties at all in storage. We could do some kind of hash or something?

I agree, I would generate a hash of the settings and use that one as the key.

[sarahzinger]

hey @yaelleC gunna take this one back, if that's cool

[sarahzinger]

update: https://github.com/grafana/grafana-aws-sdk

[fridgepoet]

We looked at this together and this datasources aws-sdk-react package will need to be updated 0.0.37

[sarahzinger]

I believe this has been fixed in version 2.0.1: https://github.com/grafana/athena-datasource/releases/tag/v2.0.1