pass Headers as part of prepare_request #872
Conversation
prepare_request adds the authentication related headers. When using Access Key methods, both Blob Storage and Data Tables uses data from the headers as part of canonicalizized data that gets signed, which is checked at the server sidej. As such, the headers must be part of what is sent to prepare_request. See Azure#871 for a walk through for Data Tables.
|
Note: With this PR, the table_00 example from azure_data_tables now works. |
sdk/core/src/headers/mod.rs
Outdated
| @@ -178,13 +193,14 @@ impl HeaderName { | |||
|
|
|||
| impl From<&'static str> for HeaderName { | |||
| fn from(s: &'static str) -> Self { | |||
| assert_eq!(s.to_lowercase(), s, "header names must be lower-case"); | |||
There was a problem hiding this comment.
This forces a canonicalization of header names to prevent issues such as what I found in data tables, where even after adding headers to the prepare_request, I found that data_tables was using Content-Type rather than content-type, which did not show up in Headers.get("content-type").
By forcing a canonicalized form here, we prevent footguns later.
sdk/core/src/headers/mod.rs
Outdated
| impl Default for Headers { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
It doesn't seem like we really need this. Can we remove it?
There was a problem hiding this comment.
It's not really needed. I added because of a default clippy check. Instead of disabling the lint or writing our own Default, I added it to the derive.
https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
sdk/core/src/headers/mod.rs
Outdated
| @@ -178,13 +193,14 @@ impl HeaderName { | |||
|
|
|||
| impl From<&'static str> for HeaderName { | |||
| fn from(s: &'static str) -> Self { | |||
| assert_eq!(s.to_lowercase(), s, "header names must be lower-case"); | |||
There was a problem hiding this comment.
This probably should be inside HeaderName::from_static or even better in a private constructor that takes a Cow<'static, str> and constructs the HeaderName which everything uses instead of constructing the HeaderName directly.
There was a problem hiding this comment.
While I'd prefer this to live in from_static, I did not want to break the const attribute, which @cataggar was invested in adding, none of the methods I can come up with are const. (for loops, chars iterator, and to_lowercase)
I added a private from_cow method which is used by the From impls that does this.
| @@ -44,13 +37,15 @@ impl AcquireLeaseBuilder { | |||
| url.query_pairs_mut().append_pair("comp", "lease"); | |||
| self.timeout.append_to_url_query(&mut url); | |||
|
|
|||
| let mut headers = Headers::new(); | |||
| headers.insert(LEASE_ACTION, "acquire"); | |||
There was a problem hiding this comment.
It would be nice if we could get headers.add(azure_core::LeaseAction::Acquire) to work, but we can leave that for a future PR.
| @@ -47,17 +47,18 @@ impl ClearPageBuilder { | |||
| self.timeout.append_to_url_query(&mut url); | |||
| url.query_pairs_mut().append_pair("comp", "page"); | |||
|
|
|||
| let mut headers = Headers::new(); | |||
| headers.insert(PAGE_WRITE, "clear"); | |||
| headers.insert(BLOB_TYPE, "PageBlob"); | |||
There was a problem hiding this comment.
Same here: it would be nice if headers.add(BlobType::PageBlob) worked.
prepare_requestadds the authentication related headers. When using Access Key methods, both Blob Storage and Data Tables uses data from the headers as part of canonicalized data that gets signed, which is checked at the server side.As such, the headers must be part of what is sent to
prepare_request.See #871 for a walk through for Data Tables.