[PECO-728] Add OAuth support #147
Conversation
kravets-levko
force-pushed
the
PECO-728-oauth-support
branch
from
26f2ffe to
e600b7b
Compare
Signed-off-by: Levko Kravets <[email protected]>
kravets-levko
force-pushed
the
PECO-728-oauth-support
branch
from
e600b7b to
9bf2327
Compare
…lAuthentication Signed-off-by: Levko Kravets <[email protected]>
Signed-off-by: Levko Kravets <[email protected]>
kravets-levko
force-pushed
the
PECO-728-oauth-support
branch
from
5effef8 to
7c15742
Compare
Signed-off-by: Levko Kravets <[email protected]>
kravets-levko
requested review from
arikfr,
superdupershant,
yunbodeng-db,
susodapop,
nithinkdb and
andrefurlan-db
as code owners
Signed-off-by: Levko Kravets <[email protected]>
Signed-off-by: Levko Kravets <[email protected]>
Signed-off-by: Levko Kravets <[email protected]>
Signed-off-by: Levko Kravets <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
- Coverage 96.28% 96.21% -0.07%
==========================================
Files 50 54 +4
Lines 808 952 +144
Branches 134 160 +26
==========================================
+ Hits 778 916 +138
- Misses 9 10 +1
- Partials 21 26 +5
☔ View full report in Codecov by Sentry. |
| port: number, | ||
| requestHandler: (req: IncomingMessage, res: ServerResponse) => void, | ||
| ): Promise<Server> { | ||
| const server = http.createServer(requestHandler); |
There was a problem hiding this comment.
Could this ever create issues of trying to send an https request from an http server?
There was a problem hiding this comment.
OAuth app we use is configured to allow http only. But it's not an issue, because we receive only authorization token via callback url, and then use that auth token + verifier string in another request to OAuth endpoint to obtain access and refresh tokens. All OAuth endpoints use https. So even is anyone will intercept auth code - it's basically useles without verifier which is not exposed anywhere
| } | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const errorListener = (error: Error) => { |
There was a problem hiding this comment.
This syntax is really strange to me, where did you get this? The errorListener invokes server off with itself?
There was a problem hiding this comment.
Here (and similarly in startServer) I just "promisify" server methods. Node's http server has event-based API, but since all our code is Promise-based, I wrapped server creation and stopping routines. How it works: first I create an error handler function and attach it to server's error event. Then I invoke a method I need, and if it was successful - I remove that error listener (therefore I store it in variable), and resolve promise. If called method emits an error - my error handler catches it, removes itself and rejects promise. Error handler here is needed only once to handle a single error, therefore I keep a function to be able to unregister it once it's no longer needed
* [PECO-728] Add OAuth support Signed-off-by: Levko Kravets <[email protected]> * Cleanup DBSQLClient code; remove redundant and no longer needed NoSaslAuthentication Signed-off-by: Levko Kravets <[email protected]> * DBSQLClient: options for auth types Signed-off-by: Levko Kravets <[email protected]> * Tests Signed-off-by: Levko Kravets <[email protected]> * Tests Signed-off-by: Levko Kravets <[email protected]> * Fix: move comment to appropriate place Signed-off-by: Levko Kravets <[email protected]> * Improve tests Signed-off-by: Levko Kravets <[email protected]> * Use proper client ID; improve callback handling Signed-off-by: Levko Kravets <[email protected]> --------- Signed-off-by: Levko Kravets <[email protected]> Signed-off-by: nithinkdb <[email protected]>
PECO-728 Add OAuth support
Related to (and partially based on) databricks/databricks-sql-python#15