Skip to content

SNOW-1825495 OAuth flows implementation #2135

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

Merged
merged 56 commits into from
Apr 14, 2025

Conversation

sfc-gh-mkeller
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller commented Jan 14, 2025

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1825621

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    In this PR I add support for OUath authorization code flow. It's very similar to external browser authentication, but it uses oauth and more options are more better for users.
    This change has been tested manually, as it's fairly complicated to setup and we don't do unit tests for the different authentication methods.

  4. (Optional) PR for stored-proc connector: Not applicable

@sfc-gh-mkeller sfc-gh-mkeller added the DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector label Jan 14, 2025
@sfc-gh-mkeller sfc-gh-mkeller self-assigned this Jan 14, 2025
Copy link
Contributor

@sfc-gh-eworoshow sfc-gh-eworoshow left a comment

Choose a reason for hiding this comment

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

Great!

A couple over-arching thoughts:

  • I only really looked at the security semantics, we should have additional reviewers for the Pythonic HTTP bits.
  • Ideally we find some way to add automated tests for this flow. Do we have browser automation testing, for example, that we could use to build an integration test?

@sfc-gh-mkeller
Copy link
Collaborator Author

Do we have browser automation testing, for example, that we could use to build an integration test?

We do this manually when we mess with the authentication code, otherwise we just don't touch these pieces of the code and we don't test them. None of them have any dependencies outside of std lib, so there's no need to

Copy link
Contributor

@sfc-gh-eworoshow sfc-gh-eworoshow left a comment

Choose a reason for hiding this comment

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

Broadly LGTM modulo a few minor comments!

We should have the "new owners" take a look too. This is complex and sensitive enough that I think it benefits from additional eyes.

Copy link
Contributor

@sfc-gh-eworoshow sfc-gh-eworoshow left a comment

Choose a reason for hiding this comment

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

Nice! Stamped.

I'd really love more eyes on this, however. Can we perhaps involve the team taking over this driver to take a second look? There's enough security-sensitive handling here that more than one reviewer is ~necessary.

Copy link
Collaborator

@sfc-gh-jszczerbinski sfc-gh-jszczerbinski left a comment

Choose a reason for hiding this comment

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

Can you provide some testing? I think it can be done pretty easily. We could inject fake/mock webbrowser object and access token request? The fake web browser could do a redirect call immediately (extracted from the url).

@sfc-gh-mmishchenko sfc-gh-mmishchenko force-pushed the mkeller/SNOW-1825621/oauth-code-flow-support branch 2 times, most recently from a850492 to 3fbb1ed Compare February 19, 2025 10:48
@sfc-gh-mmishchenko sfc-gh-mmishchenko force-pushed the mkeller/SNOW-1825621/oauth-code-flow-support branch from 8210acf to 6bad2e1 Compare February 25, 2025 13:42
@sfc-gh-mmishchenko sfc-gh-mmishchenko force-pushed the mkeller/SNOW-1825621/oauth-code-flow-support branch 3 times, most recently from f4d206c to dafeb5b Compare March 10, 2025 00:17
@sfc-gh-mmishchenko sfc-gh-mmishchenko force-pushed the mkeller/SNOW-1825621/oauth-code-flow-support branch from e920d82 to 984246e Compare March 13, 2025 10:33
@sfc-gh-pmansour sfc-gh-pmansour self-requested a review March 14, 2025 16:49
@sfc-gh-mhofman sfc-gh-mhofman dismissed their stale review March 27, 2025 14:16

My requests were addressed

@sfc-gh-mmishchenko sfc-gh-mmishchenko merged commit c4084bf into main Apr 14, 2025
95 checks passed
@sfc-gh-mmishchenko sfc-gh-mmishchenko deleted the mkeller/SNOW-1825621/oauth-code-flow-support branch April 14, 2025 15:44
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants