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

add rattler_auth to authenticate requests #191

Merged
merged 24 commits into from
Jun 13, 2023

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented May 7, 2023

No description provided.

@wolfv
Copy link
Contributor Author

wolfv commented May 7, 2023

@baszalmstra unfortunately I had to make two functions, one for reqwest::blocking::Client and one for reqwest::Client. Maybe you have an idea on how to improve this?

Apart from that it works like a charm :)

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Nice work!

Some comments:

  • authentication is currently a singleton and it cant be turned on or off. I dont like how intransparent, uncontrollable and potentially thread unsafe it is. It would be nice if you pass in an object for authentication. There you could also specify the key for the keychain for instance. Authentication::from_keychain would be nice to have!

  • Please dont use unwrap in library code unless you are sure the code cannot fail. And even then, please add a comment and/or use expect.

crates/rattler_auth/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_auth/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_auth/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_auth/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_auth/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_auth/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

This looks pretty close to done to me! A note:

  • Since you can only really use an AuthenticatedClient and there is no UnauthenticatedClient maybe we can just call it Client?
  • I think by Default the "client" should be unauthenticated unless you initialize it with credentials. See my other comment.
  • It would be very nice if there is also a From<reqwest::Client> implementation. That way the library remains super simple to use with just reqwest.
  • Random thought: If there is direct conversion from reqwest::Client to rattler_networking::Client we can use impl Into<rattler_networking::Client> as an argument where authentication is required. That way people can keep using the reqwest::Client too.

crates/rattler_networking/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_networking/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_networking/src/lib.rs Outdated Show resolved Hide resolved
@wolfv
Copy link
Contributor Author

wolfv commented May 15, 2023

The idea of having a default key is that multiple tools could share the same namespace for credentials (eg rattler-build and px are both logged in to the same sites).

@baszalmstra
Copy link
Collaborator

I think thats dangerous behavior because if you set certain (invalid) credentials it affects all programs that use the default behavior. Its worst kind of a global variable you can have..

To me its also not logical that if you would do ‘px login’ it also works with ‘rattler-build publish’ (or whatever). Id much rather have a seperate ‘rattler-build login’.

Ever more so as this is a library, I think we should allow users to opt-in by specifying the same key. It should not be the default behavior.

@wolfv
Copy link
Contributor Author

wolfv commented May 15, 2023

Hmm but for each channel you want to use with rattler build (not for publishing, just for pulling packages from) you'd need to re-login. A lot of programs use global configurations for credentials (eg netrc files) or global proxy configurations (HTTP_PROXY environment variable etc).

One can also easily opt out of the global sharing by using a different key.

I think it would be pretty nice if px and rattler build share the credentials (which we can achieve either way).

@wolfv
Copy link
Contributor Author

wolfv commented May 17, 2023

just for the record, I am working on a keyring-rs compatible fallback storage that saves the authentication to a file in the ~/.rattler_auth.json location (trying to make that configurable as well).

It's a bit challenging because of multi-threading and file locking but hopefully can get it done soon.

@wolfv wolfv force-pushed the authenticated_requests branch from 41d105e to cb05a51 Compare June 12, 2023 13:20
@wolfv wolfv force-pushed the authenticated_requests branch from cb05a51 to 92061b2 Compare June 12, 2023 15:02
@wolfv wolfv merged commit 215ea64 into conda:main Jun 13, 2023
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