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

Make keyring usage opt-in #9434

Closed
wants to merge 6 commits into from
Closed

Make keyring usage opt-in #9434

wants to merge 6 commits into from

Conversation

nbraud
Copy link

@nbraud nbraud commented Jan 6, 2021 •
edited
Loading

  • Made the import of keyring lazy
  • Added options in auth.MultiDomainBasicAuth and session.PipSession to disable keyring usage.
  • Added corresponding, user-facing knobs: a CLI flag, a config file setting, an environment variable.
  • Added a NEWS entry

@nbraud
Copy link
Author

nbraud commented Jan 6, 2021

I would need help adding the user-facing knobs for this.

As far as I can tell, the only PipSession instance is created in cli.req_command, but I cannot figure out how options get there.

@nbraud
Copy link
Author

nbraud commented Jan 6, 2021

I would also need help fixing the tests that monkeypatch the keyring >_>'

@nbraud nbraud mentioned this pull request Jan 6, 2021
Closed
@takluyver
Copy link
Member

FWIW, lazy importing should be unnecessary with recent versions of keyring (>= 21.6), as it now initialises itself lazily. Of course, there's no particular harm in having another lazy layer, but it won't make the big difference it did with earlier versions of keyring.

@pradyunsg
Copy link
Member

For the user-facing knob, you'll want to define an option in the cmdoptions.py file, and import from it.

See how any --no-warn-<whatever> are defined and used. Notably, you don't need to do anything extra to allow environment variables and configuration file options -- that happens automagically. :)

@nbraud
network.auth: Refactor MultiDomainBasicAuth to load keyring lazily
2914cb4 
Per the discussion in pypa#8719 (keyring support should be opt-in)
> [...] defer importing keyring at all until we confirm the opt-in
> pypa#8719 (comment)
@nbraud
network.auth.MultiDomainBasicAuth: Allow disabling keyring
510f490 
Kept it enabled by default, so as not to break tests.
@nbraud
network.session: Add a use_keyring option, defaulting to false
ac244c6 
Closes pypa#8719
@nbraud
Add NEWS entry
3932945
@nbraud
test_network_auth: Refactor not to monkeypatch network.auth.keyring
4557a92 
Instead, we can inherit from `MultiDomainBasicAuth` and override the
`get_keyring` class method.
@nbraud
cli.cmdoptions: Add 'use_keyring' flag, defaulting to False
58f6136
@nbraud nbraud force-pushed the keyring/opt-in branch 2 times, most recently from b901782 to 58f6136 Compare February 1, 2021 13:51
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 18, 2021
@yan12125 yan12125 mentioned this pull request Jun 11, 2021
Closed
@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Sep 18, 2021
@pradyunsg
Copy link
Member

@nbraud Are you still interested in moving this forward?

This PR is still a draft (which is used to "clearly tag when you're coding a work in progress"), so I don't think anyone has actually reviewed it yet. OTOH, this is also significantly out of date, and needs updating to deal with the fact that pip's codebase and CI infrastructure has changed since this PR was filed.

@pradyunsg
Copy link
Member

Closing this out given the lack of activity.

@pradyunsg pradyunsg closed this May 8, 2023
@pfmoore
Copy link
Member

pfmoore commented May 8, 2023

Also this is covered by #8719

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
needs rebase or merge PR has conflicts with current master S: awaiting response Waiting for a response/more information
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nbraud @takluyver @pradyunsg @BrownTruck @pfmoore