Skip to content
This repository was archived by the owner on Dec 24, 2022. It is now read-only.

Add allow credentials support #12

Closed
wants to merge 5 commits into from
Closed

Add allow credentials support #12

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6972ee5
Add allow credentials support
dnp1 Nov 22, 2017
43805f1
Remove allow credentials setter
dnp1 Nov 23, 2017
1fa5864
Add builder with support for the "allow-credentials"
dnp1 Nov 23, 2017
72abd0b
Bug Fix in builder.
dnp1 Nov 23, 2017
76f7ea9
Reformat
dnp1 Nov 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 44 additions & 14 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@
//! for a full usage example.

extern crate iron;
#[macro_use] extern crate log;
#[macro_use]
extern crate log;

use std::collections::HashSet;

Expand All @@ -61,22 +62,29 @@ use iron::headers;
/// 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 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the deprecation warning here?

#[deprecated(since="0.7.0", note="please use the `CorsMiddlewareBuilder` instead")]

CorsMiddleware {
allowed_hosts: Some(allowed_hosts),
allow_credentials: false,
}
}

pub fn allow_credentials(&mut self) {
self.allow_credentials = true;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@dnp1 dnp1 Nov 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree
I'm going to change tonight.

But this way the contract will change.

Is this a problem for you?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Allow all origins to access the resource. The
/// `Access-Control-Allow-Origin` header of the response will be set to
/// `*`.
pub fn with_allow_any() -> Self {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also here.

#[deprecated(since="0.7.0", note="please use the `CorsMiddlewareBuilder` instead")]

CorsMiddleware {
allowed_hosts: None,
allow_credentials: false,
}
}
}
Expand All @@ -85,11 +93,13 @@ impl AroundMiddleware for CorsMiddleware {
fn around(self, handler: Box<Handler>) -> Box<Handler> {
match self.allowed_hosts {
Some(allowed_hosts) => Box::new(CorsHandlerWhitelist {
handler: handler,
allowed_hosts: allowed_hosts,
handler,
allowed_hosts,
allow_credentials: self.allow_credentials,
}),
None => Box::new(CorsHandlerAllowAny {
handler: handler,
handler,
allow_credentials: self.allow_credentials,
}),
}
}
Expand All @@ -99,25 +109,30 @@ impl AroundMiddleware for CorsMiddleware {
struct CorsHandlerWhitelist {
handler: Box<Handler>,
allowed_hosts: HashSet<String>,
allow_credentials: bool,
}

/// Handler if allowing any origin.
struct CorsHandlerAllowAny {
handler: Box<Handler>,
allow_credentials: bool,
}

impl CorsHandlerWhitelist {
fn add_cors_header(&self, headers: &mut headers::Headers, origin: &headers::Origin) {
let header = format_cors_origin(origin);
headers.set(headers::AccessControlAllowOrigin::Value(header));

if self.allow_credentials {
headers.set(headers::AccessControlAllowCredentials)
}
}

fn add_cors_preflight_headers(&self,
headers: &mut headers::Headers,
origin: &headers::Origin,
acrm: &headers::AccessControlRequestMethod,
acrh: Option<&headers::AccessControlRequestHeaders>) {

self.add_cors_header(headers, origin);

// Copy the method requested by the browser in the allowed methods header
Expand Down Expand Up @@ -155,7 +170,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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're at it, can you change this to self.process_possible_cors_request(req, origin) (without the return)?

}

fn process_possible_cors_request(&self, req: &mut Request, origin: headers::Origin) -> IronResult<Response> {
Expand All @@ -165,8 +180,14 @@ impl CorsHandlerWhitelist {
if may_process {
// Everything OK, process request and add CORS header to response
self.handler.handle(req)
.map(|mut res| { self.add_cors_header(&mut res.headers, &origin); res })
.map_err(|mut err| { self.add_cors_header(&mut err.response.headers, &origin); err })
.map(|mut res| {
self.add_cors_header(&mut res.headers, &origin);
res
})
.map_err(|mut err| {
self.add_cors_header(&mut err.response.headers, &origin);
err
})
} else {
// Not adding headers
warn!("Got disallowed CORS request from {}", &origin.host.hostname);
Expand Down Expand Up @@ -203,13 +224,16 @@ impl Handler for CorsHandlerWhitelist {
impl CorsHandlerAllowAny {
fn add_cors_header(&self, headers: &mut headers::Headers) {
headers.set(headers::AccessControlAllowOrigin::Any);

if self.allow_credentials {
headers.set(headers::AccessControlAllowCredentials)
}
}

fn add_cors_preflight_headers(&self,
headers: &mut headers::Headers,
acrm: &headers::AccessControlRequestMethod,
acrh: Option<&headers::AccessControlRequestHeaders>) {

self.add_cors_header(headers);

// Copy the method requested by the browser into the allowed methods header
Expand Down Expand Up @@ -239,13 +263,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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here too: Remove the return.

}

fn process_possible_cors_request(&self, req: &mut Request) -> IronResult<Response> {
self.handler.handle(req)
.map(|mut res| { self.add_cors_header(&mut res.headers); res })
.map_err(|mut err| { self.add_cors_header(&mut err.response.headers); err })
.map(|mut res| {
self.add_cors_header(&mut res.headers);
res
})
.map_err(|mut err| {
self.add_cors_header(&mut err.response.headers);
err
})
}
}

Expand All @@ -259,15 +289,15 @@ impl Handler for CorsHandlerAllowAny {
match req.headers.get::<headers::Origin>() {
None => {
self.handler.handle(req)
},
}
Some(_) => {
match req.method {
//If is an OPTION request, check for preflight
Method::Options => self.process_possible_preflight(req),
// If is not an OPTION request, we assume a normal CORS (no preflight)
_ => self.process_possible_cors_request(req),
}
},
}
}
}
}
Expand Down