-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
feat: use personal access token in OAuth2 databases #20280
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20280 +/- ##
==========================================
- Coverage 66.86% 66.85% -0.01%
==========================================
Files 1847 1848 +1
Lines 70567 70573 +6
Branches 7744 7745 +1
==========================================
Hits 47184 47184
- Misses 21382 21388 +6
Partials 2001 2001
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
""" | ||
parameters = request.args.to_dict() | ||
if "error" in parameters: | ||
raise Exception(parameters["error"]) # XXX: raise custom SIP-40 exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! XXX
is a traditional way of adding a todo (vim will highlight it by default, eg).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I use it for temporary todos, while still working on the PR.
@betodealmeida cool feature! did it get any field use yet? 🙂 |
c27613f
to
b4861ac
Compare
8bf29df
to
cafd28d
Compare
cafd28d
to
216fb6d
Compare
state = jwt.encode( | ||
payload=payload, | ||
key=current_app.config["SECRET_KEY"], | ||
algorithm="HS256", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want the algo to be configurable?
# Settings to enable OAuth2 in the Google Sheets DB engine spec. | ||
# GSHEETS_OAUTH2_CLIENT_ID = "XXX" | ||
# GSHEETS_OAUTH2_CLIENT_SECRET = "YYY" | ||
# GSHEETS_OAUTH2_REDIRECT_URI = "http://localhost:8088/api/v1/database/oauth2/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be a dict:
OAUTH2_CREDENTIALS = {
"GSheets": {
"CLIENT_ID": "XXX",
...
}
@betodealmeida Any progress with that? What is missing for that to be merged? I am seeking for the same solution but for Trino Oauth2. maybe I can help with that feature (although I am new in contributing to open source) |
Same use case as you @shohamyamin (trino + Oauth2). |
Closing this, since it was easier to start the work from scratch in a new branch. I hope we can have this up and running in the next couple weeks. |
SUMMARY
Implementation of #20300, adding general support for OAuth2 in databases and a GSheets implementation.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION