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

docs: jwks_uri addition to OAUTH provider #24928

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

kravi21
Copy link
Contributor

@kravi21 kravi21 commented Aug 9, 2023

jwks_uri token is needed by the OAUTH to fetch the key and create token needed to connect to sso provider.
This will enable the OAUTH to verify the request and process it further.
If this is missing , then sso request was failing to connect to the OPENID type SSO request and will give following error
[Error] authorizing OAuth access token: Missing "jwks_uri" in metadata

Reference: https://stackoverflow.com/questions/72035617/runtimeerror-missing-jwks-uri-in-metadata-for-flask-and-azure-ad-authlib

@kravi21
Copy link
Contributor Author

kravi21 commented Aug 9, 2023

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@sfirke
Copy link
Member

sfirke commented Sep 14, 2023

hi @kravi21 thanks for this! Here's what I'm thinking:

  • Superset uses Flask AppBuilder (FAB) for authentication.
  • I'd rather have a single set of docs to update. I think that should be in FAB.
  • Right now FAB docs list lots of off-the-shelf options like Azure, Google, but they aren't clear about how to create a new one that isn't listed. Which I think will sometimes need jwks_uri like you are adding here, but won't always?

If you can change your comment on that line to be # may be required to generate token I think we could merge this.

And then the proper fix I believe would be to improve the FAB docs to show how you create a new OAuth provider setup. Can you say what OAuth provider you are using? Maybe we could use that as an example.

jwks_uri may be required to generate token
@kravi21
Copy link
Contributor Author

kravi21 commented Sep 19, 2023

@sfirke I have updated the comment as suggested by you, Please review

@sfirke sfirke merged commit a724850 into apache:master Sep 19, 2023
28 checks passed
eschutho pushed a commit to Superset-Community-Partners/superset that referenced this pull request Sep 21, 2023
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants