Access keyring only when needed. #8078
Conversation
|
Oh I forgot to fix some existing tests. However I am unsure what I should add new tests for? |
real-yfprojects
force-pushed
the
keyring-on-demand
branch
2 times, most recently
from
270d666
to
8bfde6f
Compare
|
I just noticed #7038 does something similar. So what has to be changed so that poetry behaves as you wish? |
real-yfprojects
force-pushed
the
keyring-on-demand
branch
from
8bfde6f
to
cbc17dd
Compare
|
imo #6612 and #7037 look like a more pragmatic approach in particular #6612 doesn't seem like it needs to be much better to be mergeable: catch a slightly more general exception and add a unit test and - noting that I don't have the power to make such decisions - it would look to me to be good to go |
|
Quoting from #7037:
That's what I thought too. So I wrote this patch. From my point of view this PR can be merged immediately. As @remram44 said, fixing poetries elaborate keyring usage is inevitable. I don't want to deny a keyring request every time I run poetry. |
I just noticed ci is failing for some reason. |
Requesting a network page is first tried without retrieving authentication from the keyring. Credentials provided by other means are used already. If no authentication was found and the server answers with 401 or 403 the keyring is queried for credentials. This fixes python-poetry#1917. * src/poetry/utils/constants.py (STATUS_AUTHLIST): A list of HTTP errors requesting authentication. Currently 401 and 403. * src/poetry/utils/authenticator.py (Authenticator.request): Retrieve credentials with disabled keyring first. On auth error try again with enabled keyring. * src/poetry/utils/authenticator.py (Authenticator.get_credentials_for_url): Add `keyring` option. * src/poetry/utils/authenticator.py (Authenticator._get_credentials_for_url: ^^^ same as above * src/poetry/utils/authenticator.py (Authenticator._get_credentials_for_repository): ^^^ * src/poetry/utils/authenticator.py (AuthenticatorRepositoryConfig.get_http_credentials): ^^^ * src/poetry/utils/password_manager.py (PasswordManager.get_http_auth): ^^^ * src/poetry/utils/password_manager.py (HTTPAuthCredential): Implement `empty` property that one of username or password exists. * src/poetry/utils/authenticator.py (Authenticator): Use `HTTPAuthCredential.empty`. Only cache non-empty credentials. * tests/utils/test_authenticator.py : Fix tests.
real-yfprojects
force-pushed
the
keyring-on-demand
branch
from
cbc17dd
to
e04d674
Compare
|
So far as I can see #7038 was not merged because hitting every API twice - first to try without authentication and then with - was considered not OK. SInce this does exactly the same I don't know why it wouldn't meet the same fate. |
Well then discuss how poetry should behave instead - here or in a separate issue. |
|
... that's exactly what #8078 (comment) was doing! |
Great! However I didn't notice you were discussing which poetry behaviour would be favourable. You seemed to discuss which PR would be more pragmatic to merge or a better fix for #1917. I would like to separate these two discussions. #6612 seems better suited for a quick fix and I advocate merging it. Moreover I don't mind when this PR isn't merged in its current form. However we shouldn't leave sight of the bigger picture. Regarding #6612 I will take a look whether I can take the branch, finish the patch and open a new PR. |
They main issue is that peotry tries to get a password for any http request from the keyring. It just looks at the keyring and checks if there is a password for the netloc of the current request. This automatic checking happens for any and all http requests. There is also no duplicated api requets. If you do not either configure a http_fallback (use the netloc password store in the keyring) or explicitly set a password through poetry, the request will just fail with unauthorised. Having some poeple properly configure their password proteced routes vs breaking the workflow for most users seems like a very good trade off to me. I don't get why nobody seems to care about 9th most upvoted issues of poetry :/ |
At least none of the maintainers although this would be exactly where a maintainer has to step in. Instead they seem to dodge the hard decisions. |
|
Asking for credentials after hitting 401 is a best practice if interactivity with the user is required. It is how every browser works. I'm fact it is the only thing the 401 code is for. I'm not sure why Poetry maintainers would object to getting that response first? |
Not sure whether @neersighted is a maintainer. However he did object in #7038 (comment). |
|
per #8078 (comment): IMO a moderately improved #6612 ought to be mergeable. (so far as I know no-one has offered such an MR) |
|
The comment you link is from yourself, not a maintainer... No one is going to offer more MRs now that 3 have been rejected and maintainers are silent on what they might find acceptable. I take offense at your suggestion that this is what is stopping progress. At this point, I am running my own forked version on all my desktop computers. That's the beauty of open source: when a project goes silent or hostile, and is utterly broken for your purpose, you can patch it yourself. It is clear we are in this situation. |
er, yes. I am reiterating my opinion. While not a maintainer, I am a regular contributor and have a reasonable sense of what is likely to be acceptable.
well hopefully you are wrong; certainly no fix will happen if no fix is made. |
Well I have offered help to improve set PR. However I stumbled upon an issue I need help with before writing the code. See #6612 (comment) |
|
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. |
Pull Request Check List
Resolves: #1917
Requesting a network page is first tried without retrieving authentication from the keyring. Credentials provided by other means are used already. If no authentication was found and the server answers with 401 or 403 the keyring is queried for credentials. This fixes #1917.
src/poetry/utils/constants.py (STATUS_AUTHLIST): A list of HTTP errors requesting authentication. Currently 401 and 403.
src/poetry/utils/authenticator.py (Authenticator.request): Retrieve credentials with disabled keyring first. On auth error try again with enabled keyring.
src/poetry/utils/authenticator.py (Authenticator.get_credentials_for_url): Add
keyringoption.src/poetry/utils/authenticator.py (Authenticator._get_credentials_for_url: ^^^ same as above
src/poetry/utils/authenticator.py (Authenticator._get_credentials_for_repository): ^^^
src/poetry/utils/authenticator.py (AuthenticatorRepositoryConfig.get_http_credentials): ^^^
src/poetry/utils/password_manager.py (PasswordManager.get_http_auth): ^^^