Add allow credentials support #12
Conversation
src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn allow_credentials(&mut self) { | ||
| self.allow_credentials = true; | ||
| } |
There was a problem hiding this comment.
I'm not sure yet about this API. I think a setter method like this is not a good solution.
I could imagine two ways to configure the credentials: Either as constructor argument, or as a builder. Maybe something like this?
CorsMiddlewareBuilder::with_whitelist(allowed_hosts) // Or: with_allow_any()
.allow_credentials(true)
.build();What are your thoughts?
There was a problem hiding this comment.
I agree
I'm going to change tonight.
But this way the contract will change.
Is this a problem for you?
There was a problem hiding this comment.
We could deprecate the constructor functions (with_whitelist and allow_credentials) on the CorsMiddleware object itself and simply add the builder in parallel. This way it's compatible and we can remove it in the next version.
| @@ -155,7 +193,7 @@ impl CorsHandlerWhitelist { | |||
| } | |||
|
|
|||
| // If we don't have an Access-Control-Request-Method header, treat as a possible OPTION CORS call | |||
| return self.process_possible_cors_request(req, origin) | |||
| return self.process_possible_cors_request(req, origin); | |||
There was a problem hiding this comment.
If you're at it, can you change this to self.process_possible_cors_request(req, origin) (without the return)?
| @@ -239,13 +286,19 @@ impl CorsHandlerAllowAny { | |||
| } | |||
|
|
|||
| // If we don't have an Access-Control-Request-Method header, treat as a possible OPTION CORS call | |||
| return self.process_possible_cors_request(req) | |||
| return self.process_possible_cors_request(req); | |||
|
|
||
| pub fn build(&self) -> CorsMiddleware { | ||
| CorsMiddleware { allowed_hosts: self.allowed_hosts.clone(), allow_credentials: self.allow_credentials } | ||
| } |
There was a problem hiding this comment.
Can you change the build method to consume self? Then you can avoid the clone().
| @@ -58,16 +59,45 @@ use iron::method::Method; | |||
| use iron::status; | |||
| use iron::headers; | |||
|
|
|||
| /// The struct that builds a CorsMiddleware | |||
| pub struct CorsMiddlewareBuilder { | |||
There was a problem hiding this comment.
Please use #[derive(Debug, Clone, PartialEq)].
| /// The struct that holds the CORS configuration. | ||
| pub struct CorsMiddleware { | ||
| allowed_hosts: Option<HashSet<String>>, | ||
| allow_credentials: bool, | ||
| } | ||
|
|
||
| impl CorsMiddleware { | ||
| /// Specify which origin hosts are allowed to access the resource. | ||
| pub fn with_whitelist(allowed_hosts: HashSet<String>) -> Self { |
There was a problem hiding this comment.
Can you add the deprecation warning here?
#[deprecated(since="0.7.0", note="please use the `CorsMiddlewareBuilder` instead")]
| @@ -77,6 +107,7 @@ impl CorsMiddleware { | |||
| pub fn with_allow_any() -> Self { | |||
There was a problem hiding this comment.
And also here.
#[deprecated(since="0.7.0", note="please use the `CorsMiddlewareBuilder` instead")]
| CorsMiddlewareBuilder { allow_credentials: false, allowed_hosts: None } | ||
| } | ||
|
|
||
| /// Specify which origin hosts are allowed to access the resource. |
There was a problem hiding this comment.
Can you add another line here?
/// If you don't specify any allowed hosts, then any host will be allowed to access the resource.
| self.allowed_hosts = Some(allowed_hosts); | ||
| self | ||
| } | ||
| /// Specify Access-Control-Allow-Credentials |
There was a problem hiding this comment.
Can you document the default value?
/// By default, the `AccessControlAllowCredentials` header will be set to `false`.
No description provided.