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.

Sign up for GitHub

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

Jump to bottom

DataProxy: Adds oauth pass-through option for datasources #15205

Merged
merged 21 commits into from Mar 25, 2019
Merged

DataProxy: Adds oauth pass-through option for datasources #15205

merged 21 commits into from Mar 25, 2019

Conversation

seanlaff
Copy link
Contributor

@seanlaff seanlaff commented Feb 1, 2019 •
edited

Closes #12556

Adds a datasource option for passing onwards the current user's oauth access token.

screen shot 2019-02-01 at 5 01 54 pm

How it works

Anytime a user logs in with an oauth provider, we'll encrypt and persist their oauth access-token, refresh-token, token-type, and token-expiry in the user_auth table. Any time the datasource proxy is called on a datasource that has the oauthPassThru flag set to true, we'll lookup the user's token, run it through tokenSource() (which handles refreshes) and pass it along as an Authorization header to the datasource.

I opted to persist the tokens because scheduled reports would not work otherwise.

Previously users would not get an entry in the user_auth table if the auth provider did not provide an upstream auth_id for them. In this MR I changed the behavior to allow entries in that table to populate even if the user does not have an upstream id. I did this because this table seemed like the most natural place to keep the oauth tokens, and gatekeeping that table by auth_id no longer made sense since oauth providers are not required to provide an upstream id.

I also added an index to the user_id column in user_auth since it's used for lookups.

Open to feedback, thanks!

@CLAassistant
Copy link

CLAassistant commented Feb 1, 2019 •
edited

CLA assistant check
All committers have signed the CLA.

@marefr marefr requested a review from bergquist February 4, 2019 10:03
@Asgoret Asgoret mentioned this pull request Feb 4, 2019
Closed
@bergquist
Copy link
Contributor

Hi! I've done a quick review of the PR and I think this is something that would be useful for Grafana.

We are currently very short on time before the release of 6.0 but I'll get back to this eventually with an in-depth review. We won't be able to get this into 6.0 but I think this is a good candidate for 6.1

@bergquist bergquist self-assigned this Mar 5, 2019
bergquist
bergquist requested changes Mar 8, 2019
View reviewed changes
Copy link
Contributor

@bergquist bergquist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Sean!

Great meeting you at GrafanaCon. Such a blast to see someone finding abandoned features in Grafana :) This PR is something we want to merge but I'd like to see a few changes before we do so. Esp requiring specifying which OAuth provider that should be used for passing creds to the datasource. Perhaps you have an use case that I haven't thought about but right now I think we can default to the last logged in provider. Or even better. Find which provider is used for the current session.

Some other minor remarks. Great work otherwise! I hope we have time to get this merged before 6.1!

public/app/features/datasources/partials/http_settings.html Outdated Show resolved Hide resolved
pkg/api/pluginproxy/ds_proxy.go Show resolved Hide resolved
pkg/api/pluginproxy/ds_proxy.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/migrations/user_auth_oauth_mig.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/user_auth.go Outdated Show resolved Hide resolved
@seanlaff
Copy link
Contributor Author

seanlaff commented Mar 14, 2019 •
edited

@bergquist Think I've addressed all of the concerns mentioned so far- let me know if you have more feedback! :)

bergquist
bergquist reviewed Mar 19, 2019
View reviewed changes
Copy link
Contributor

@bergquist bergquist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected.

Would like to see some minor tweeks before we merge this.
Great work!

pkg/services/sqlstore/user_auth.go Outdated Show resolved Hide resolved
public/app/features/datasources/partials/http_settings.html Show resolved Hide resolved
pkg/services/sqlstore/user_auth_test.go Outdated Show resolved Hide resolved
pkg/login/ext_user.go Outdated Show resolved Hide resolved
pkg/api/pluginproxy/ds_proxy.go Outdated Show resolved Hide resolved
pkg/api/pluginproxy/ds_proxy.go Show resolved Hide resolved
pkg/services/sqlstore/user_auth.go Show resolved Hide resolved
@daniellee daniellee added this to the 6.1 milestone Mar 25, 2019
@daniellee
Copy link
Contributor

daniellee commented Mar 25, 2019 •
edited

Taking over the review here as Carl is on vacation this week and it would be nice to get this into the 6.1 release. I have tested a bit but would have to use this in a datasource plugin to really test this out. You have resolved all the feedback you got from Carl so don't see anything stopping this from being merged.

I will add this to the plugin auth guide once I get to use this in a datasource: http://docs.grafana.org/plugins/developing/auth-for-datasources/

Looking forward to adding support for this in some of our plugins. Thanks for taking the time to implement all of Carl's feedback!

@daniellee daniellee merged commit fda7e68 into grafana:master Mar 25, 2019
@daniellee daniellee changed the title Add oauth pass-thru option for datasources DataProxy: Add oauth pass-thru option for datasources Mar 26, 2019
@daniellee daniellee changed the title DataProxy: Add oauth pass-thru option for datasources DataProxy: Adds oauth pass-thru option for datasources Mar 27, 2019
@daniellee daniellee changed the title DataProxy: Adds oauth pass-thru option for datasources DataProxy: Adds oauth pass-through option for datasources Mar 27, 2019
daniellee added a commit that referenced this pull request Mar 27, 2019
@daniellee
Chore: changelog notes for #13825,#15205,#14877,#16227
bbdc1e6
ryantxu added a commit to ryantxu/grafana that referenced this pull request Mar 27, 2019
@ryantxu
Merge remote-tracking branch 'grafana/master' into rename-tsvm-to-trace
932f623 
* grafana/master:
  Chore: Implement revive (grafana#16200)
  InfluxDB: Fix tag names with periods in alerting (grafana#16255)
  Fix: Table Panel and string values & numeric formatting (grafana#16249)
  Tech: Patch lib updates, update yarn.lock (grafana#16250)
  Chore: docs whats new article for the 6.1 release (grafana#16251)
  Chore: Storybook improvements (grafana#16239)
  Feat: Introduce Button and LinkButton components to @grafana/ui (grafana#16228)
  Chore: changelog adds note for grafana#16234
  Fix: Prometheus regex ad-hoc filters w/ wildcards (grafana#16234)
  Chore: changelog notes for grafana#13825,grafana#15205,grafana#14877,grafana#16227
  Fix: Alert email variable name typo fixed (grafana#16232)
  Fix: scripts changelog cli per page set to 100
  Fix: Dashboard history diff & white theme fix (grafana#16231)
  Merge pull request grafana#16241 from grafana/hugoh/no-implicit-any
  Chore: Theme consistency, rems => pixels or variables (grafana#16235)
  Chore: Theme consistency, rems => pixels (grafana#16145)
  changelog: adds notes for grafana#16229 and grafana#16227
ryantxu added a commit to ryantxu/grafana that referenced this pull request Mar 27, 2019
@ryantxu
Merge remote-tracking branch 'grafana/master' into big-value-component
f294471 
* grafana/master:
  Refactor: Rename TimeSeriesVM to GraphSeriesXY (grafana#16216)
  Chore: Implement revive (grafana#16200)
  InfluxDB: Fix tag names with periods in alerting (grafana#16255)
  Fix: Table Panel and string values & numeric formatting (grafana#16249)
  Tech: Patch lib updates, update yarn.lock (grafana#16250)
  Chore: docs whats new article for the 6.1 release (grafana#16251)
  Chore: Storybook improvements (grafana#16239)
  Feat: Introduce Button and LinkButton components to @grafana/ui (grafana#16228)
  Chore: changelog adds note for grafana#16234
  Fix: Prometheus regex ad-hoc filters w/ wildcards (grafana#16234)
  Chore: changelog notes for grafana#13825,grafana#15205,grafana#14877,grafana#16227
  Fix: Alert email variable name typo fixed (grafana#16232)
  Fix: scripts changelog cli per page set to 100
  Fix: Dashboard history diff & white theme fix (grafana#16231)
  Merge pull request grafana#16241 from grafana/hugoh/no-implicit-any
  Chore: Theme consistency, rems => pixels or variables (grafana#16235)
  Chore: Theme consistency, rems => pixels (grafana#16145)
  changelog: adds notes for grafana#16229 and grafana#16227
@bergquist
Copy link
Contributor

🍾

@hakten
Copy link

hakten commented Jul 10, 2020

When I turn on 'Forward OAuth Identity', I don't see that OAuth Identity Forwarding details. I am using Grafana 7.0.3 and trying to use the forward option with SimpleJson data source.
Does not seeing that option means SimpleJson does not support it? Or is UI changed in later versions?

@seanlaff seanlaff mentioned this pull request Aug 6, 2020
Closed
seanlaff added a commit to seanlaff/grafana that referenced this pull request Aug 10, 2020
@seanlaff
Handle updating refresh token in oauth pass-thru
01b163b 
This fixes a small logic bug in my original implementation of oauth pass-thru grafana#15205 that was brought up in the discussion of grafana#12131

The pass-thru implementation relies on `TokenSource()` to refresh tokens if necessary. The access token from that call was compared with the one on disk to determine if the new token should be persisted... however this missed a case. A provider may send the same access token back with a new refresh token (see https://tools.ietf.org/html/rfc6749#section-6). This change checks all fields of the token grafana cares about when determining equivalance. Also added some more context to the log statements.
@seanlaff seanlaff mentioned this pull request Aug 10, 2020
Merged
@pantaoran pantaoran mentioned this pull request Oct 20, 2020
Closed
@mandrews10 mandrews10 mentioned this pull request Mar 11, 2021
Closed
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bergquist bergquist bergquist requested changes

Assignees

@bergquist bergquist

Labels
add to changelog area/datasource/proxy area/datasource pr/external This PR is from external contributor
Projects
None yet
Milestone
6.1.0-beta1
Development

Successfully merging this pull request may close these issues.

Support passing user's OAuth identity to datasources
7 participants
@seanlaff @CLAassistant @bergquist @daniellee @hakten @marefr @ying-jeanne