-
Notifications
You must be signed in to change notification settings - Fork 177
Shard long passwords on Windows #597
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
Conversation
|
It may take me a few days to get to reviewing this, but thank you for your contribution. I'll run the integration tests today. |
|
Please take a look at the unit test failure :) |
01ef32b to
a5edcda
Compare
|
Hi @benc-db , thanks for checking it! Some options I see:
What would you prefer? |
|
I think the key thing is that, if the test doesn't pass on Ubuntu, is it not passing because we're not setting the test up correctly, is the new behavior not compatible with ubuntu, or is the change regressing something. So long as we're not in the third category, we can skip the test (@pytest.mark.skip), though ideally we would detect the OS and only skip for Ubuntu. If we're in the second category, and the test is just pointing this out, we'll also want to document the limitation. |
|
After looking at the failure again, I think if you can patch keyring to just read/write the secret to files in the test, it should work regardless of OS. Ultimately the test just needs to validate your slice and rebuild logic works, regardless of keyring implementation. |
And if you can't get that to work, I can update the tests myself, but might be a couple of weeks until I can get to it. |
|
Thanks @benc-db, I will try to implement the patch option than. Thanks for the feedback. |
|
Hi @benc-db, I think the issue is solved now. I created the Mock Keyring Backend class that will write to files in a temp directory (copied some a bit of the logic from the integration test). Also expanded the UT a bit by also testing if the delete cleans-up the password properly. |
|
Looks good, I'm going to add a changelog entry and merge. |
|
Closing in favor of #599 (moved to this fork and added Changelog). Thanks @thijs-nijhuis-shell! |
Resolves #563
Description
When using the oAuth U2M flow on a windows laptop, the credentials are not stored in the Windows Credential Store because it exceed 1280 characters. Therefore, each call to Databricks results in a new authentication request (and opens a new tab in your browser). In this PR, all interaction with keyring (get, set and delete) is redirect to a custom function that will check the length and platform before storing the credentials. If on Windows and credentials are longer then 1280 characters, it will shard the password in chunk of 1280 and store those along with some meta data. When getting or deleting the credentials, it will check for this metadata and, if so fetch all shards and return the concatenated result or delete them all in case of a delete.
I have tested my code on a windows laptop as well as on a mac. On the mac, I first got the credentials with the current version in 1.7.8 so that it is stored, then replaced connections.py with the new version and ran a 'dbt debug' again to see if the stored credentials was fetched an used. It was.
I have added 2 Unit Tests to test both storing a long and a short password. All Unit Test succeeded.
I tried running the integration test but couldn't get them to run. The tox command seems to not like the bash commands on a windows laptop and running the python command directly causes all integration to fail on not finding the 'test' profile.
Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.