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

Data source: Handle datasource withCredentials option properly #23380

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

hvtuananh
Copy link
Contributor

@hvtuananh hvtuananh commented Apr 7, 2020

The fetch() API won't send cookies or other type of credentials unless
you set the credentials init option. Some datasources like Prometheus
and Elasticsearch have withCredentials option in Browser access mode,
but this option is not currently getting passed in the fetch() API.

What this PR does / why we need it:

This PR corrects the behavior of the With Credentials checkbox on some datasources like Prometheus and Elasticsearch.

Which issue(s) this PR fixes:

Fixes #23338.

Special notes for your reviewer:

This is my first time contributing to the Grafana project, so comments and suggestions are appreciated.

The fetch() API won't send cookies or other type of credentials unless
you set the credentials init option. Some datasources like Prometheus
and Elasticsearch have `withCredentials` option in Browser access mode,
but this option is not currently getting passed in the fetch() API.

Fixes grafana#23338.
@CLAassistant
Copy link

CLAassistant commented Apr 7, 2020

CLA assistant check)
All committers have signed the CLA.

@daniellee daniellee added pr/external This PR is from external contributor area/frontend labels Apr 7, 2020
@daniellee daniellee requested review from a team, mckn and dprokop and removed request for a team April 7, 2020 08:06
@mckn
Copy link
Contributor

mckn commented Apr 7, 2020

@hvtuananh welcome to the project! Really great of you to jump in an helping out 🙌

What do you think about changing it from withCredentials to credentials and pass limit|omit|same-origin instead? Just to make this a bit more flexible?

@mckn mckn added this to the 6.7.3 milestone Apr 7, 2020
@hvtuananh
Copy link
Contributor Author

hvtuananh commented Apr 7, 2020

@mckn the options.withCredentials var is passed all the way from datasource plugins, like https://github.com/grafana/grafana/blob/master/public/app/plugins/datasource/prometheus/datasource.ts#L147. Do you suggest we change all references of withCredentials to credentials?

Also I found that during testing, the parseInitFromOptions is used for more than just datasources (like save new passwords), so I'm picturing we can do something like:

if withCredentials is:
    undef: set credentials to same-origin (default)
    false: set credentials to omit
    true:  set credentials to include

@mckn
Copy link
Contributor

mckn commented Apr 7, 2020

@mckn the options.withCredentials var is passed all the way from datasource plugins, like https://github.com/grafana/grafana/blob/master/public/app/plugins/datasource/prometheus/datasource.ts#L147. Do you suggest we change all references of withCredentials to credentials?

Also I found that during testing, the parseInitFromOptions is used for more than just datasources (like save new passwords), so I'm picturing we can do something like:

if withCredentials is:
    undef: set credentials to same-origin (default)
    false: set credentials to omit
    true:  set credentials to include

Aha, mybad 🙈 I thought it was a newly added option but now I see that it was used previously since angular http was supporting that flag. Then I totally agree with you that this is the best way of doing it.

@mckn mckn merged commit afd8ffd into grafana:master Apr 7, 2020
@hvtuananh
Copy link
Contributor Author

Thank you for reviewing!

peterholmberg pushed a commit that referenced this pull request Apr 9, 2020
…23380)

The fetch() API won't send cookies or other type of credentials unless
you set the credentials init option. Some datasources like Prometheus
and Elasticsearch have `withCredentials` option in Browser access mode,
but this option is not currently getting passed in the fetch() API.

Fixes #23338.
@marefr marefr changed the title Fix: Handle datasource withCredentials option properly (#23338) Data source: Handle datasource withCredentials option properly Apr 23, 2020
aknuds1 pushed a commit that referenced this pull request Apr 23, 2020
…23380)

The fetch() API won't send cookies or other type of credentials unless
you set the credentials init option. Some datasources like Prometheus
and Elasticsearch have `withCredentials` option in Browser access mode,
but this option is not currently getting passed in the fetch() API.

Fixes #23338.

(cherry picked from commit afd8ffd)
aknuds1 added a commit that referenced this pull request Apr 23, 2020
* AuthProxy: Fixes bug where long username could not be cached (#22926)

(cherry picked from commit 6c9d833)

* Server: Exit with 0 if no error (#23312)

Make grafana-server exit with 0 if no error occurred.

(cherry picked from commit 5645d74)

* Dashboard: Save json should preserve folderId (#23314)

(cherry picked from commit 7e3b43e)

* TimeSrv: Try to parse 8 and 15 digit numbers as timestamps if parsing as date fails (#21694)

* Try to parse 8 and 15 digit numbers as timestamps if parsing as date fails

Fixes #19738

* Add tests

(cherry picked from commit c89ad9b)

* BackendSrv: include credentials when withCredentials option is set (#23380)

The fetch() API won't send cookies or other type of credentials unless
you set the credentials init option. Some datasources like Prometheus
and Elasticsearch have `withCredentials` option in Browser access mode,
but this option is not currently getting passed in the fetch() API.

Fixes #23338.

(cherry picked from commit afd8ffd)

* Dashlist: Fixed dashlist broken in edit mode (#23426)

(cherry picked from commit 363bf75)

* Admin: Fix Synced via LDAP message for non-LDAP external users (#23477)

* UserAdmin: remove Synced via LDAP message for non-LDAP users

* UserAdmin: show "Synced via <provider>" message for external users

(cherry picked from commit 4d81cec)

* Graphite: Fixed cannot read finally of undefiend (#23512)

(cherry picked from commit 61460ea)

* Hangouts: fixes notifications for alerts with empty message (#23559)

* Hangouts: fixes notifications for alerts with empty message

* Update pkg/services/alerting/notifiers/googlechat.go

Co-Authored-By: Marcus Efraimsson <marcus.efraimsson@gmail.com>

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
(cherry picked from commit 2661054)

* Variables: fixes error when setting adhoc variables values (#23580)

(cherry picked from commit 0091885)

* Release 6.7.3

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

* ci-metrics-publisher.sh: Fix linting issue

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

* TablePanel: Fix XSS issue in header column rename (backport) (#23814)

* escaping html when rendering table header alias.

* fixed tooltip.

Co-authored-by: Marcus Andersson <marcus.andersson@grafana.com>

* Security: Fix annotation popup XSS vulnerability (#23813)

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
(cherry picked from commit 3955e8c)

Co-authored-by: Jon McKenzie <jcmcken@gmail.com>
Co-authored-by: Peter Holmberg <peterholmberg@users.noreply.github.com>
Co-authored-by: Jesse Tan <jessetan@users.noreply.github.com>
Co-authored-by: Tuan Anh Hoang-Vu <hvtuananh@gmail.com>
Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
Co-authored-by: Alexander Zobnin <alexanderzobnin@gmail.com>
Co-authored-by: Hugo Häggmark <hugo.haggmark@grafana.com>
Co-authored-by: Marcus Andersson <marcus.andersson@grafana.com>
jfrabaute added a commit to jfrabaute/grafana that referenced this pull request Jul 19, 2021
The loki datasource was not passing the credentials info to the fetch()
API when loki was in Browser access mode.

This is similar to:
grafana#23380
ivanahuckova pushed a commit that referenced this pull request Jul 20, 2021
The loki datasource was not passing the credentials info to the fetch()
API when loki was in Browser access mode.

This is similar to:
#23380
grafanabot pushed a commit that referenced this pull request Jul 21, 2021
The loki datasource was not passing the credentials info to the fetch()
API when loki was in Browser access mode.

This is similar to:
#23380

(cherry picked from commit aa3a462)
ivanahuckova pushed a commit that referenced this pull request Jul 21, 2021
…) (#37054)

The loki datasource was not passing the credentials info to the fetch()
API when loki was in Browser access mode.

This is similar to:
#23380

(cherry picked from commit aa3a462)

Co-authored-by: Fabrice <jfr@core-services.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datasources using backendSrv (e.g. Elasticsearch and Prometheus) no longer respect withCredentials auth option
6 participants