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

Response HTTP Headers / CORS #43

Closed
St4NNi opened this issue May 19, 2023 · 4 comments · Fixed by #45
Closed

Response HTTP Headers / CORS #43

St4NNi opened this issue May 19, 2023 · 4 comments · Fixed by #45
Assignees
Labels
feature New feature or request

Comments

@St4NNi
Copy link
Contributor

St4NNi commented May 19, 2023

Hi,

Thanks again for the great work. While working with s3s we encountered another possible issue:

Some of our users want to integrate contents provided via s3(s) into their own external websites. Unfortunately it seems impossible to set the appropriate CORS response headers (Access-Control-Allow-Origins etc.) for buckets / objects. AWS S3 uses the PutBucketCors endpoint to configure the appropriate CORS response headers for a specific bucket.

I think a solution could be combined with fixing the issue #15 because they are both interacting with response http headers. Are any configuration options planned to allow for this ?
Maybe a quick solution would include the possibility to set response headers in S3Result manually (similar to access for request headers in S3Request) but i guess this would just be a clunky unpolished solution.

Thanks in advance!

@Nugine Nugine added the feature New feature or request label May 19, 2023
@Nugine
Copy link
Owner

Nugine commented May 19, 2023

Thanks for your feedback!

#15 is not related as it focuses on overriding response headers from client side.

The S3 output type does not contain all info we need, which breaks my previous assumption.

Solutions:

  1. Make S3Result<T> carry both the S3 output and http response fields.
  2. Add CORS headers in related S3 output types.
  3. ... Do you have any suggestions?

And, does CORS affect error responses?

@St4NNi
Copy link
Contributor Author

St4NNi commented May 19, 2023

Hi,

Yes #15 is only loosely related, but anyways both issues deal with response headers so may have some common mechanisms.

Regarding the actual question and possible solutions:

I don't think that it would be a good idea to include the the CORS Headers in the S3Output types (2) as I like the close relationship between the output types and the actual Amazon S3 documentation. Amazon S3 (and most other S3 compatible interfaces) set the CORS Header as part of some kind of meta information related to the bucket, in my short experiments the CORS headers are only set on successful responses with 2xx status codes and not for errors.

I would prefer a solution that could set any arbitrary type of HTTP header for the response and would not be limited to CORS headers. There might be other interesting HTTP header types to set (Set-Cookie, Keep-Alive etc.).

IMHO the best way to achieve this is by extending the S3Result<T> type to carry also http response information (including additional headers) (1). But I am flexible and would certainly adapt to any other solution.

BR

@Nugine
Copy link
Owner

Nugine commented May 19, 2023

Ok. Following the tower model, I think the right position for custom http response information is the Result type.

async fn(S3Request<I>) -> Result<S3Response<O>, S3Error>

where I is operation input, O is operation output.

type S3Result<T> = Result<S3Response<T>, S3Error>
#[non_exhaustive]
pub struct S3Response<T> {
    pub output: T,                       // S3 operation output
    pub headers: HeaderMap<HeaderValue>, // overriding headers in `output`
    pub extensions: Extensions,          // extensions for passing info to middlewares
}

impl<T> S3Response<T> {
    pub fn new(output: T) -> Self { todo!() }
}

It's a major breaking change but seems easy for users to fix.

@Nugine Nugine self-assigned this May 19, 2023
@St4NNi
Copy link
Contributor Author

St4NNi commented May 19, 2023

Yeah, this sounds reasonable to me. With the additional benefit if making the whole user-facing API more future proof, as long as the S3Response::new(output: T) -> Self{...} function stays stable it would be quite easy to add additional parameters to the S3Response.

If you need any help, just let me know !

@Nugine Nugine mentioned this issue May 21, 2023
@Nugine Nugine added this to the v0.6.0 milestone May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants