Add full support for OBO user requests #533
Conversation
aangelisc
added
enhancement
|
Backend code coverage report for PR #533
|
|
Frontend code coverage report for PR #533 |
There was a problem hiding this comment.
Awesome that a recent version of Plugin SDK already supports headers for CheckHealth.
This fixes #509, but could we keep the grafana/grafana-plugin-sdk-go#579 open since that is a general feature request that would be good to consider by itself?
| @@ -43,7 +43,7 @@ func (c *Client) TestRequest(ctx context.Context, datasourceSettings *models.Dat | |||
| } | |||
|
|
|||
| // TODO: This is a workaround because Plugin SDK doesn't expose user context for CheckHealth | |||
There was a problem hiding this comment.
This TODO not needed anymore
There was a problem hiding this comment.
Good job here 👍
One slighly weird thing is that if the user creating the data source is not logged with OAuth, the check fails with:
I am not sure what would be the best UX here for that situation, we could probably catch the error and show something like "Unable to check data source, the current user has not signed in with OAuth". WDYT?
This error can be due to other cause like not enabled auth token pass-thru. Ideally user should not see this error at all. I would add a task to intestigate how to show this type of authentication only if OAuth was used or to specifically check whether user was signed in using OAuth and show error message which explicitly tells that auth isn't possible becaue OAuth wasn't used. Let's keep it to a separate task, this PR fixes check health. Since it's experimental feature under a feature flag, it's OK if it fails when enabled for non OAuth. It's not enabled by defailt and users which enable it expected to understand scenario when it works. |
My preference would be to actually prevent any requests made if a user signed in with the standard method (service account) attempts to make requests via an OAuth datasource. I think we should maybe implement a catch-all for this situation and return an error along the lines of |
Sounds good to me
Also agree, just remember to log the original error so we can debug in case we are masking an unexpected error |
Great, in this case I'll merge this and open a separate issue as suggested by @kostrse with the above implementation as a solution 😊 |
This PR ensures that all requests make use of the user identity if the datasource is configured to do so.
grafana-plugin-sdk-gorecently added the appropriate support for header forwarding allowing theX-ID-TOKENheader to be accessed in external plugins.Fixes #509
Fixes grafana/grafana-plugin-sdk-go#579