-
Notifications
You must be signed in to change notification settings - Fork 36
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
reflect auth headers back into response objects #59
Conversation
Node: the clippy check is failing because a failing |
@jasonwhite @daniel-noland Any thoughts in this? |
src/app.rs
Outdated
@@ -451,6 +457,23 @@ where | |||
} | |||
} | |||
|
|||
fn extract_auth_header(headers: HeaderMap) -> Option<BTreeMap<String, String>> { |
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.
Looks like there isn't a reason to take ownership of headers
here. Changing this to headers: &HeaderMap
should avoid the need to .clone()
the HeaderMap
above.
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.
Fixed
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.
Sorry for the long delay on this one. The email notification for this got lost in my mail box. Hope you didn't lose the motivation to get this merged. ;)
Looks good overall! Just a couple small changes and it should be good to go.
@@ -248,6 +250,7 @@ where | |||
) -> Result<Response<Body>, Error> { | |||
// Get the host name and scheme. | |||
let uri = req.base_uri().path_and_query("/").build().unwrap(); | |||
let headers = req.headers().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.
I think this clone is necessary, thoughts?
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.
Does it compile without the clone? Seems like it should, but I'm not sure since the lifetime requirements of futures gets a little complicated.
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.
No, doesn't compile without it. The req.into_body()
call consumes the req
object, so the lifetimes don't work out.
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.
Example
❯ cargo build
Compiling rudolfs v0.3.6 (/home/jmob/dev/rudolfs)
error[E0505]: cannot move out of `req` because it is borrowed
--> src/app.rs:255:46
|
253 | let headers = req.headers();
| ------------- borrow of `req` occurs here
254 |
255 | match from_json::<lfs::BatchRequest>(req.into_body()).await {
| ^^^ move out of `req` occurs here
...
270 | uri, &headers, &storage, object, operation, size,
| ------- borrow later captured here by closure
For more information about this error, try `rustc --explain E0505`.
error: could not compile `rudolfs` due to previous error
@@ -77,6 +77,8 @@ jobs: | |||
- uses: actions-rs/clippy-check@v1 | |||
with: | |||
token: ${{ secrets.GITHUB_TOKEN }} | |||
args: --all-features -- --allow dead_code |
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.
Not sure if this is desired, but CI wouldn't pass without it:
warning: associated items `new`, `len`, and `from_stream` are never used
--> src/lru.rs:43:12
|
38 | impl<K> Cache<K>
| ---------------- associated items in this implementation
...
43 | pub fn new() -> Self {
| ^^^
...
56 | pub fn len(&self) -> usize {
| ^^^
...
64 | pub async fn from_stream<S>(mut stream: S) -> Result<Self, S::Error>
| ^^^^^^^^^^^
|
= note: `#[warn(dead_code)]` on by default
warning: type alias `S3DiskCache` is never used
--> src/storage/mod.rs:55:10
|
55 | pub type S3DiskCache = Cached<Disk, S3>;
| ^^^^^^^^^^^
warning: method `into_parts` is never used
--> src/storage/mod.rs:111:12
|
106 | impl StorageKey {
| --------------- method in this implementation
...
111 | pub fn into_parts(self) -> (Namespace, Oid) {
| ^^^^^^^^^^
warning: method `stream` is never used
--> src/storage/mod.rs:148:12
|
139 | impl LFSObject {
| -------------- method in this implementation
...
148 | pub fn stream(self) -> ByteStream {
| ^^^^^^
warning: associated function `new` is never used
--> src/storage/cached.rs:101:18
|
96 | impl<C, S> Backend<C, S>
| ------------------------ associated function in this implementation
...
101 | pub async fn new(
| ^^^
warning: associated function `new` is never used
--> src/storage/disk.rs:44:18
|
43 | impl Backend {
| ------------ associated function in this implementation
44 | pub async fn new(root: PathBuf) -> Result<Self, io::Error> {
| ^^^
warning: associated function `new` is never used
--> src/storage/encrypt.rs:37:12
|
36 | impl<S> Backend<S> {
| ------------------ associated function in this implementation
37 | pub fn new(key: [u8; 32], storage: S) -> Self {
| ^^^
warning: associated function `new` is never used
--> src/storage/retrying.rs:35:12
|
34 | impl<S> Backend<S> {
| ------------------ associated function in this implementation
35 | pub fn new(storage: S) -> Self {
| ^^^
warning: method `into_backoff` is never used
--> src/storage/s3.rs:89:12
|
86 | impl InitError {
| -------------- method in this implementation
...
89 | pub fn into_backoff(self) -> backoff::Error<InitError> {
| ^^^^^^^^^^^^
warning: associated function `new` is never used
--> src/storage/s3.rs:149:18
|
148 | impl Backend {
| ------------ associated function in this implementation
149 | pub async fn new(
| ^^^
warning: associated function `with_client` is never used
--> src/storage/s3.rs:217:18
|
216 | impl<C> Backend<C> {
| ------------------ associated function in this implementation
217 | pub async fn with_client(
| ^^^^^^^^^^^
warning: associated function `new` is never used
--> src/storage/verify.rs:55:12
|
54 | impl<S> Backend<S> {
| ------------------ associated function in this implementation
55 | pub fn new(storage: S) -> Self {
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.
Ah. That's fine. I'll fix that up separately.
No worries! Definitely know the plight.
Thanks, I think I addressed everything, happy to address any other concerns too. |
🎉 |
If an
Authentication
header is present in the request to the server, then reflect it in the response object from the server. If this is not present thegit-lfs
client will attempt to send unauthenticated requests to the server. With this changerudolfs
can be protected behind a Kubernetes NGINXIngress
resource that uses thenginx.ingress.kubernetes.io/auth-url
annotation.