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

[WIP] Support HTTP authentication prompt after 401 #5331

Closed
wants to merge 6 commits into from

Conversation

simonlsk
Copy link
Contributor

@simonlsk simonlsk commented Jan 25, 2021

Addresses issue iterative/dvc-http#26.

Affected components:
HTTP tree

When HTTP request returns 401 with www-authenticate, the username is prompted if not existent, then the password is prompted if inexistent or fetched from OS secret manager.
If ask_password is set to true prompt for password anyway.

@simonlsk simonlsk force-pushed the http-intuitive-auth branch from 2ddec61 to 53404df Compare January 25, 2021 18:04
dvc/tree/http.py Outdated
user = ask_username(host)
self.path_info.user = user
config_level = "local"
with self.repo.config.edit(config_level) as conf:
Copy link
Contributor

Choose a reason for hiding this comment

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

There have been a few questions about this before, but our remotes don't modify their own config. Some remotes like gdrive, save things to .dvc/tmp, but don't modify the config. Looks like we don't really need to do it, right? We can ask again or make the user set user in .dvc/config.local by himself. Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see how it breaks the design a little bit.
Asking again / asking the user to run a command is a bit unnatural UX (different than git for instance). Can you try to help me find a more appropriate way to save the username as a side effect, that will be satisfying design-wise?

Copy link
Contributor

Choose a reason for hiding this comment

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

@simonlsk Great point. Where does git store the username in such cases?

Copy link
Contributor Author

@simonlsk simonlsk Jan 26, 2021

Choose a reason for hiding this comment

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

That's a good question, my git installation set my credential.helper as osxkeychain and I am only prompted once about any combination of user, host, password. I thought that you could only fetch a password once you know the user, host combination and that the username was being saved in .git/config, but I was wrong. I just read the code in git and it looks like it can fetch entries without knowing the username.
I will try to workaround this, and let you know.

@efiop
Copy link
Contributor

efiop commented Jan 25, 2021

Oops, we had an issue with a recent pytest version. Fixed in upstream, please rebase.

@simonlsk simonlsk force-pushed the http-intuitive-auth branch from 53404df to 2c4c033 Compare January 27, 2021 07:18
@simonlsk simonlsk requested a review from efiop January 27, 2021 08:16
@simonlsk
Copy link
Contributor Author

There is a security issue using keyring for that purpose. Closing the PR for now.

@simonlsk simonlsk closed this Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants