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.

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

make keyring fallback optional #7038

Conversation

felix-ht
Copy link

@felix-ht felix-ht commented Nov 15, 2022

Fixes #1917 - the root cause of this issue seems to be that any request made by poetry tries to get auth information from the keyring even if the config of the PasswordManager has no indication that the requets needs auth in the first place. This is also the reason why any operation that does any kind of requests fails, with the keyring errors.
This was caused by some code that was bypassing the normal keyring opreations, which i removed.

I updated the test so that they still pass using the normal poetry keyring repo. I also made the keyring protected so access to it is discouraged.

Pull Request Check List

Resolves: #issue-number-here

  • Added tests for changed code.
  • Updated documentation for changed code.

@felix-ht felix-ht marked this pull request as draft November 15, 2022 18:46
@neersighted
Copy link
Member

I'm not sure this approach is good; some indexes rate-limit users based on credentials (e.g. have lower rate limits for unauthenticated users). Always providing credentials, instead of waiting for an error and retrying, seems to be a better behavior as it generates fewer network requests.

@felix-ht
Copy link
Author

felix-ht commented Nov 15, 2022

I'm not sure this approach is good; some indexes rate-limit users based on credentials (e.g. have lower rate limits for unauthenticated users). Always providing credentials, instead of waiting for an error and retrying, seems to be a better behavior as it generates fewer network requests.

This is not what this does. This doesn’t try without credentials as a first shot - instead is properly uses the config to add the credentials only if needed. So no issues with rate limiting.

@felix-ht
Copy link
Author

@neersighted now i see - the assumption is that it should use the keyring even if there is no config - in case there just happens to be a suitable config in the normal namespace of the keyring

@felix-ht
Copy link
Author

felix-ht commented Nov 15, 2022

while this certainly makes sense to have, wouldn’t this very specifc feature - legacy support for keyring entries not created by poetry not be the perefect candiate for a config flag?

@felix-ht
Copy link
Author

felix-ht commented Nov 15, 2022

allright i changed this PR so that the fallback is now an option that can be configured. @neersighted would be great if you could take a look.

(I would assume that this would require some more documentation/discussion before this can merged)

@felix-ht felix-ht mentioned this pull request Nov 15, 2022
2 tasks
@felix-ht felix-ht changed the title fixed issues with the keyring make keyring fallback optional Nov 16, 2022
@felix-ht
Copy link
Author

felix-ht commented Apr 3, 2023

don't think that this will get merged, so closing the pr

Copy link

github-actions bot commented Mar 3, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyring errors during non-publishing operations
2 participants