-
Notifications
You must be signed in to change notification settings - Fork 8
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
New feature: Allow authentication by personal tokens #53
Conversation
Thanks for the contribution! This is very close to what we're doing in the fork we're running at work, however we have the user set their PAT over SSH and then persist that so no one has to update their .ssh/config during onboarding or when their PAT expires. Maybe we can have multiple modes of operation? |
Sounds great! Could you share the code of the fork you are using at work so we can merge both modes of operation? |
let token = match &self.user()?.token { | ||
None => self.gitlab.fetch_token_for_user(self.user()?).await?, | ||
Some(token) => token.clone(), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy will probably complain about this since it can be if let Some(..) = ... else ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review! It seems like clippy didn't have a problem with this particular line but, however, with two other lines in my code. I fixed them accordingly.
|
||
# Cargo.toml | ||
[dependencies] | ||
my-crate = { version = "0.1", registry = "my-gitlab-project" } | ||
``` | ||
|
||
In your CI build, setup a `before_script` step to replace the connection string with one containing the CI token: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave these lines in please? A CI token would still be required to build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I removed them because embedding the CI token into the registry's URL does not work anymore with the newest cargo version. (Please correct me if I oversee something.)
I changed the commit such that these lines remain and a note regarding newer versions of cargo is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true, these instructions were wrong as established in #28 but it was never corrected
Hey @w4, could you give it another look? |
I've been using this code at work for personal access token usage with gitlab.com private repos. It's working well it would be great to see this merged. |
Merged, thanks for the contribution. This isn't the most ideal solution with rotating keys (the SSH subcommand shines there) but it does the job. |
Using this project as-is requires to enter an admin personal token into the
config.toml
. This is not always possible and safe, e.g. when running this service on a multi-user server. This PR allows authentication by personal tokens with onlyread_api
permissions, removing the need for an admin personal token. Reading the package list, reading metadata and downloading crates is all done using these personal tokens. The personal tokens do not need to be entered into this service'sconfig.toml
, but rather only on the client's.ssh/config
.I've added a few more minor changes:
cargo-get
inside.gitab-ci.yaml
snippet