Uh oh!
There was an error while loading. Please reload this page.
[PECO-728] Add OAuth support - #147
Conversation
26f2ffe to
e600b7bCompareSigned-off-by: Levko Kravets <levko.ne@gmail.com>
e600b7b to
9bf2327Compare…lAuthentication Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
5effef8 to
7c15742CompareSigned-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
codecov-commenter
commented
Jun 22, 2023
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. |
nithinkdb
left a comment
There was a problem hiding this comment.
Overall looks good, I just have a couple questions
| 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 <levko.ne@gmail.com> * Cleanup DBSQLClient code; remove redundant and no longer needed NoSaslAuthentication Signed-off-by: Levko Kravets <levko.ne@gmail.com> * DBSQLClient: options for auth types Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Tests Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Tests Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Fix: move comment to appropriate place Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Improve tests Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Use proper client ID; improve callback handling Signed-off-by: Levko Kravets <levko.ne@gmail.com> --------- Signed-off-by: Levko Kravets <levko.ne@gmail.com> Signed-off-by: nithinkdb <nithin.krishnamurthi@databricks.com>
PECO-728 Add OAuth support
Related to (and partially based on) databricks/databricks-sql-python#15