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

fix: remove exception when keyring is locked #8227

Merged

Conversation

real-yfprojects
Copy link
Contributor

Resolves: #1917
This inherits and therefore closes #6612.

  • src/poetry/utils/password_manager.py (PoetryKeyring.get_credential): Catch KeyringError and RuntimeError when accessing keyring.

Copy link
Contributor

@dimbleby dimbleby left a comment

Choose a reason for hiding this comment

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

review of #6612 indicated that unit test would be required, which seems reasonable.

I'd suggest adding a LockedBackend to tests/conftest.py - cf the existing DummyBackend etc - and writing a test that uses that in test_authenticator.py.

src/poetry/utils/password_manager.py Show resolved Hide resolved
@real-yfprojects
Copy link
Contributor Author

Ok so I added a few tests but I am very unsure whether I did it correctly and sufficiently. Please review.

@dimbleby
Copy link
Contributor

tests look plausible to me. I'm not a fan of the many imports of KeyringLocked in conftest.py - just import it once and be done! - but that's not a big deal.

@real-yfprojects
Copy link
Contributor Author

I'm not a fan of the many imports of KeyringLocked in conftest.py - just import it once and be done!

Me neither, but I thought there must a reason the existing code only imports keyring on demand instead of adding the imports to the top of the file.

@dimbleby
Copy link
Contributor

There''s already a keyring import at the top of the file, I see no reason to be afraid of another

from keyring.backend import KeyringBackend

@real-yfprojects
Copy link
Contributor Author

Ok, fixed

@radoering
Copy link
Member

Ok so I added a few tests but I am very unsure whether I did it correctly and sufficiently. Please review.

You only tested one of your except branches. If you add another test with KeyringError instead of KeyRingLocked in test_password_manager.py, it will be good enough IMO.

tests/utils/test_authenticator.py Outdated Show resolved Hide resolved
@radoering radoering mentioned this pull request Sep 28, 2023
MatthieuBizien and others added 6 commits October 3, 2023 17:24
* src/poetry/utils/password_manager.py (PoetryKeyring.get_credential)
* tests/conftest.py : Implement `LockedBackend` inheriting `KeyringBackend` and throwing `KeyringLocked` on access.

* tests\utils\test_authenticator.py : Test that locked keyrings still lead to a request.

* tests\utils\test_password_manager.py : Test that locked keyrings do not fail.
* tests/utils/test_authenticator.py
* tests/conftest.py
@radoering radoering merged commit 47a3a19 into python-poetry:master Oct 3, 2023
20 checks passed
@radoering radoering mentioned this pull request Oct 3, 2023
2 tasks
@real-yfprojects real-yfprojects deleted the pr/MatthieuBizien/6612 branch October 3, 2023 18:06
@arneyjfs
Copy link

Had to manually set backend keyring for private and public repos

Fantastic to see keyring improvements being released in this version. However I still had a convoluted route to getting authentication working when trying to install packages from google artifact registry over SSH (5 hours in total). I know 1.7.0 is not released yet so there are still some bugs expected but since this issue is closed I thought I would log here all the loops I had to jump through in case it is helpful in finding any more bugs that might have been missed. The crux of the issue was I had to set the backend manually depending on if I was installing from public or private repos.

Install process

  1. Start out with poetry 1.6.1 through normal install methods
curl -sSL https://install.python-poetry.org | python3 -

Cannot directly install 1.7.0 from git as I end up with a module not found: cleo error (separate issue)

  1. upgrade to 1.7.0.dev0
curl -sSL https://install.python-poetry.org | python3 - --git https://github.com/python-poetry/poetry.git@master
  1. Install keyrings.google-artifactregistry-auth latest version (1.1.2 in my case)
poetry self add keyrings.google-artifactregistry-auth
  1. This was the particularly tricky part to work out... I eventually found that I have to explicitly set the keyring backends to get a full install of both packages from my private Artefact Registry repo, and public the PyPi public repo. If I set export PYTHON_KEYRING_BACKEND=keyring.backends.fail.Keyring then obviously I cannot authenticate to google since I need the keyrings.gauth.GooglePythonAuth for that. However if I set export PYTHON_KEYRING_BACKEND=keyrings.gauth.GooglePythonAuth then all other packages not from my python repo hang forever (presumably waiting to be authed or something?). Therefore the below worked for me:

    4.1 Remove private packages from pyproject.toml, then explicitly remove keyring backend and install public packages:

     export PYTHON_KEYRING_BACKEND=keyring.backends.fail.Keyring
     poetry install

    4.2 explicitly set keyring backend to google auth, and install the remaining private packages:

     export PYTHON_KEYRING_BACKEND=keyrings.gauth.GooglePythonAuth
     poetry add  --source <repo-id> <package-name>

This then completed successfully for me. I hope it is helpful. Please let me know if any more info is required or if I am making a silly mistake. Thanks!

System:
Ubuntu 22.04.3 LTS Over SSH
keyrings-google-artifactregistry-auth 1.1.2
Poetry 1.7.0.dev0

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
5 participants