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.

Sign up for GitHub

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

Jump to bottom

SNOW-630142 Custom Auth #1215

Merged
merged 33 commits into from
Nov 24, 2022
Merged

SNOW-630142 Custom Auth #1215

merged 33 commits into from
Nov 24, 2022

Conversation

sfc-gh-kwagner
Copy link
Contributor

@sfc-gh-kwagner sfc-gh-kwagner commented Jul 29, 2022 •
edited by sfc-gh-mkeller
Loading

Description
Implemented custom auth and refactored the current auth methods to use the new custom auth class

Testing

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-630142

  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 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.

    Please write a short description of how your code change solves the related issue.

@sfc-gh-kwagner
Copy link
Contributor Author

Just using GitHub to run some tests for me

@sfc-gh-kwagner sfc-gh-kwagner marked this pull request as ready for review August 2, 2022 21:41
sfc-gh-mkeller
sfc-gh-mkeller requested changes Aug 5, 2022
View reviewed changes
src/snowflake/connector/auth_by_plugin.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth_by_plugin.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth_by_plugin.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth_usrpwdmfa.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth_by_plugin.py Outdated Show resolved Hide resolved
src/snowflake/connector/connection.py Outdated Show resolved Hide resolved
src/snowflake/connector/connection.py Outdated Show resolved Hide resolved
src/snowflake/connector/connection.py Outdated Show resolved Hide resolved
src/snowflake/connector/reauth_by_plugin.py Outdated Show resolved Hide resolved
src/snowflake/connector/reauth_default.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1215 (0ea7365) into main (253b2ce) will decrease coverage by 0.25%.
The diff coverage is 83.06%.

@@            Coverage Diff             @@
##             main    #1215      +/-   ##
==========================================
- Coverage   81.70%   81.45%   -0.26%     
==========================================
  Files          59       62       +3     
  Lines        8331     8650     +319     
  Branches     1353     1282      -71     
==========================================
+ Hits         6807     7046     +239     
- Misses       1224     1293      +69     
- Partials      300      311      +11
Impacted Files Coverage Δ
src/snowflake/connector/network.py 78.87% <0.00%> (-2.31%) ⬇️
src/snowflake/connector/auth_keypair.py 78.57% <60.00%> (-0.70%) ⬇️
src/snowflake/connector/reauth_webbrowser.py 62.50% <62.50%> (ø)
src/snowflake/connector/auth_idtoken.py 93.75% <66.66%> (-6.25%) ⬇️
src/snowflake/connector/reauth_by_plugin.py 70.00% <70.00%> (ø)
src/snowflake/connector/connection.py 89.22% <79.41%> (-1.29%) ⬇️
src/snowflake/connector/reauth_default.py 80.00% <80.00%> (ø)
src/snowflake/connector/auth_by_plugin.py 93.33% <93.33%> (-0.55%) ⬇️
src/snowflake/connector/auth.py 80.62% <100.00%> (+0.82%) ⬆️
src/snowflake/connector/auth_default.py 93.33% <100.00%> (+1.02%) ⬆️
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sfc-gh-mkeller sfc-gh-mkeller force-pushed the kwagner-snow-630142 branch 3 times, most recently from e32c2e4 to e1d4245 Compare October 19, 2022 23:47
sfc-gh-aling
sfc-gh-aling reviewed Nov 1, 2022
View reviewed changes
src/snowflake/connector/auth/_auth.py Show resolved Hide resolved
src/snowflake/connector/auth/by_plugin.py Show resolved Hide resolved
src/snowflake/connector/auth/by_plugin.py Show resolved Hide resolved
src/snowflake/connector/auth/by_plugin.py Show resolved Hide resolved
src/snowflake/connector/auth/by_plugin.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth/keypair.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth/keypair.py Show resolved Hide resolved
src/snowflake/connector/auth/keypair.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth/keypair.py Show resolved Hide resolved
src/snowflake/connector/auth/webbrowser.py Outdated Show resolved Hide resolved
@sfc-gh-aling
Copy link
Collaborator

I think we need to align the implementation signature with the protocol, which should all just be keyword arguments

@sfc-gh-mkeller sfc-gh-mkeller force-pushed the kwagner-snow-630142 branch 2 times, most recently from acde21e to c9b6315 Compare November 23, 2022 00:28
sfc-gh-aling
sfc-gh-aling reviewed Nov 23, 2022
View reviewed changes
src/snowflake/connector/auth/_auth.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth/_auth.py Outdated Show resolved Hide resolved
src/snowflake/connector/auth/by_plugin.py Show resolved Hide resolved
src/snowflake/connector/auth/webbrowser.py Outdated Show resolved Hide resolved
src/snowflake/connector/connection.py Outdated Show resolved Hide resolved
test/unit/test_auth_oauth.py Show resolved Hide resolved
sfc-gh-aling
sfc-gh-aling approved these changes Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@sfc-gh-aling sfc-gh-aling left a comment

Choose a reason for hiding this comment

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

LGTM assuming the type hint comments are resolved properly.

@sfc-gh-kwagner @sfc-gh-mkeller
SNOW-630142 Custom Auth
8a6f6df 
Description
Implemented custom auth and refactored the current auth methods to use the new custom auth class

Testing
@sfc-gh-kwagner @sfc-gh-mkeller
Added auth_class and reauth_class
9bfbdbc
sfc-gh-mkeller and others added 22 commits November 22, 2022 17:32
@sfc-gh-mkeller
some more typehints
e16d554
@sfc-gh-mkeller
add custom auth type enforcement and tests
cc1f5f8
@sfc-gh-mkeller
added hard-coded API test for authenticator base class
8b6dadf
@sfc-gh-mkeller
Update src/snowflake/connector/auth_by_plugin.py
dc27b75
@sfc-gh-mkeller
remove used method, since this was only called by internal functions …
a16db74 
…there's no point in being backwards compatible
@sfc-gh-mkeller
introduce snowflake.connector.auth submodule
5487914
@sfc-gh-mkeller
get rid of reauth class
accaca7
@sfc-gh-mkeller
get rid of preprocess and other simplifications
3818ca2
@sfc-gh-mkeller
new API
83c0b06
@sfc-gh-mkeller @sfc-gh-aling
Update src/snowflake/connector/auth/by_plugin.py
af6ba03 
Co-authored-by: Adam Ling <[email protected]>
@sfc-gh-mkeller @sfc-gh-aling
Update src/snowflake/connector/auth/keypair.py
37e872a 
Co-authored-by: Adam Ling <[email protected]>
@sfc-gh-mkeller
remove unnecessary property
da44b57
@sfc-gh-mkeller
review comments
532286b
@sfc-gh-mkeller
reauthenticate fix
8d42304
@sfc-gh-mkeller
fix idtoken bug and type-hints
4800394
@sfc-gh-mkeller
consent cache idtoken bug
36082b6
@sfc-gh-mkeller
aligning the implementation signature with the protocol
392e2d9
@sfc-gh-mkeller
fix olddriver tests
e9ad581
@sfc-gh-mkeller
fix test issue
5c53354
@sfc-gh-mkeller
more test fixes
432bc3f
@sfc-gh-mkeller @sfc-gh-aling
Apply suggestions from code review
111475a 
Co-authored-by: Adam Ling <[email protected]>
@sfc-gh-mkeller
additional types
807cd57
@sfc-gh-mkeller sfc-gh-mkeller force-pushed the kwagner-snow-630142 branch 2 times, most recently from f71f6f2 to 223424e Compare November 23, 2022 18:12
sfc-gh-mkeller
sfc-gh-mkeller approved these changes Nov 24, 2022
View reviewed changes
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

🚢 🚢

@sfc-gh-mkeller sfc-gh-mkeller merged commit 9e04925 into main Nov 24, 2022
@sfc-gh-mkeller sfc-gh-mkeller deleted the kwagner-snow-630142 branch November 24, 2022 03:17
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sfc-gh-aling sfc-gh-aling sfc-gh-aling approved these changes

@sfc-gh-mkeller sfc-gh-mkeller sfc-gh-mkeller approved these changes

@sfc-gh-hchaturvedi sfc-gh-hchaturvedi Awaiting requested review from sfc-gh-hchaturvedi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sfc-gh-kwagner @codecov-commenter @sfc-gh-aling @sfc-gh-mkeller