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

OAuth: Support JMES path lookup when retrieving user email #14683

Merged
merged 13 commits into from
Aug 26, 2019

Conversation

bobmshannon
Copy link
Contributor

Closes #13353

This PR aims to increase the generality of the generic OAuth provider by allowing users to specify a JMES Path that Grafana will use to search for an e-mail address.

@bobmshannon bobmshannon changed the title Bs/more generic email attribute Allow JMES path to be used to retrieve user e-mail Dec 28, 2018
@bobmshannon bobmshannon changed the title Allow JMES path to be used to retrieve user e-mail Support JMES path lookup when retrieving user e-mail address Dec 28, 2018
@bobmshannon bobmshannon force-pushed the bs/more_generic_email_attribute branch 3 times, most recently from 844d91d to e4b20bf Compare December 29, 2018 00:04
@torkelo
Copy link
Member

torkelo commented Jan 4, 2019

this looks ok, thoughs @DanCech ?

@marefr
Copy link
Contributor

marefr commented Jun 25, 2019

@bobmshannon this looks like a really interesting approach for supporting extraction of dynamic json properties and I also see that this can be useful for other use cases in Grafana. Sorry for this PR review to have stalled, but are you still up for trying to get this to a merge:able state? We'll be available for any kind of support and review you may need.

Looks like CLA is not signed yet which is weird since you've previously commited/contributed to Grafana - Please double check CLA.

Then solve conflicts/rebase on master

Thank you in advance!

@marefr marefr added the prio/medium Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jun 25, 2019
Copy link
Collaborator

@DanCech DanCech left a comment

Choose a reason for hiding this comment

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

Don't know how I missed this, the idea seems great just needs some tidying up.

pkg/social/generic_oauth.go Outdated Show resolved Hide resolved
conf/defaults.ini Outdated Show resolved Hide resolved
@bobmshannon
Copy link
Contributor Author

Hey all, thanks for taking a look at this. I'll address the comments and rebase against master soon so we can get this merged.

@DanCech
Copy link
Collaborator

DanCech commented Jun 25, 2019

awesome @bobmshannon! apologies for the delay in getting back to you

@marefr
Copy link
Contributor

marefr commented Jun 25, 2019

@ryantxu I heard that you maybe were looking into dynamic transformation of json so JMES Path may be interesting for you to look into if you already haven't.

@ryantxu
Copy link
Member

ryantxu commented Jun 26, 2019

I am looking at add a general API datasource -- essentially read JSON and transform it to our internal data format. I tried various JSON transformation libraries and had the most success with http://jsonata.org/ See grafana-graphql-datasource for examples of the JSONata input that will transform semi-complex github response format

JAMES looks great for picking a value out of a json document (this use case), but I found it pretty difficult to use to transform a general JSON document.

JAMES seems to be more widely used:
https://www.npmtrends.com/jmespath-vs-jsonata-vs-jsonnet-vs-jsonpath-vs-node-jq-vs-object-mapper

both libraries have a go implementation. I'm not sure we need to use the same one everywhere. I will also try again to see if I can get JAMES working again.

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2019

CLA assistant check
All committers have signed the CLA.

@marefr
Copy link
Contributor

marefr commented Jun 27, 2019

@bobmshannon Could you please rebase your commits on master and to resolve the issue with signing the CLA either add the email of your first commit to your GitHub account or rewrite/squash your commits after rebased with master. Thanks

@bobmshannon bobmshannon force-pushed the bs/more_generic_email_attribute branch 5 times, most recently from b47d967 to c593d2c Compare June 27, 2019 11:13
Signed-off-by: Bob Shannon <bobs@dropbox.com>
@bobmshannon
Copy link
Contributor Author

@dance @marefr I've rebased and signed the CLA. Let me know if any additional changes are needed to the PR. Thanks :)

@marefr marefr requested review from DanCech and marefr June 27, 2019 11:24
@bobmshannon bobmshannon force-pushed the bs/more_generic_email_attribute branch from 943f7ce to 97ee6f8 Compare August 24, 2019 23:51
@bobmshannon
Copy link
Contributor Author

@marefr I've updated the PR to address the remaining comments.

Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Looks great 👍 Just a minor comment and then we should be able to merge this

pkg/login/social/generic_oauth.go Outdated Show resolved Hide resolved
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Very nice 🎉

@marefr marefr changed the title Support JMES path lookup when retrieving user e-mail address OAuth: Support JMES path lookup when retrieving user email Aug 26, 2019
@marefr marefr added add to changelog and removed pr/needs-manual-testing Before merge some help with manual testing & verification is requested prio/medium Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Aug 26, 2019
@marefr marefr added this to the 6.4 milestone Aug 26, 2019
@marefr marefr merged commit 056dbc7 into grafana:master Aug 26, 2019
@marefr
Copy link
Contributor

marefr commented Aug 26, 2019

Thank you for contributing to Grafana! Sorry it took quite a while for us, but really appreciate your help and attitude for getting this merged to master.

@DanCech
Copy link
Collaborator

DanCech commented Aug 26, 2019

Nice! Thanks @bobmshannon

ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  @grafana/data: improve the CircularVector api (grafana#18716)
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
@bobmshannon bobmshannon deleted the bs/more_generic_email_attribute branch August 27, 2019 04:18
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 28, 2019
* grafana/master:
  Explore: Use PanelQueryState to handle querying (grafana#18694)
  Chore: Improve err message for notifications (grafana#18757)
  @grafana/toolkit: add package versions to the ci report (grafana#18751)
  @grafana/data: Matchers and Transforms (grafana#16756)
  Docs: Document LDAP config reload in admin http api (grafana#18739)
  center NoDataSourceCallToActionCard in Explore (grafana#18752)
  DataLinks: enable data links in Gauge, BarGauge and SingleStat2 panel (grafana#18605)
  DashboardDatasource: reuse query results within a dashboard (grafana#16660)
  Plugins: show a clear error on the plugin page when it failed to load (grafana#18733)
  Chore: Use ruleId instead of alertId as log keyword (grafana#18738)
  @grafana/data: improve the CircularVector api (grafana#18716)
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
@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
Labels
add to changelog pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: allow email address attribute from OAuth token response to be even more configurable
7 participants