Add CORS Support #101
Add CORS Support #101
Conversation
| @@ -50,7 +58,7 @@ | |||
| private final String cert; | |||
| private final String key; | |||
| private final int port; | |||
| private final double gloablSoftReqPerSec; | |||
| private final double globalSoftReqPerSec; | |||
| # CORS | ||
| enable_cors = false | ||
| cors_allowed_origins = [] | ||
| cors_allowed_headers = [] |
There was a problem hiding this comment.
The correct default for headers & methods is usually * (all). Does setting an empty array return this from the server?
There was a problem hiding this comment.
i don't think we ever want to return *. if the methods list is empty it will not return the allow methods header which will use the browser defaults...
| @@ -63,6 +63,14 @@ ip_black_list = [] | |||
| enable_white_list = false | |||
| ip_white_list = [] | |||
|
|
|||
| # CORS | |||
| enable_cors = false | |||
There was a problem hiding this comment.
I really like using the nested config structure typesafe config supports:
cors = {
enable = false
allowed_origins = []
...
}
You then read the keys cors.enable, cors.allowed_origins, etc.
This makes things more organized and readable - and you can do cool things like pulling out the whole cors block as a Config object and pass it to a helper.
There was a problem hiding this comment.
agreed was just trying to follow the current conventions.. will change.
| @@ -52,6 +57,10 @@ protected void configurePipeline(ChannelHandlerContext ctx, String protocol) thr | |||
| ChannelPipeline cp = ctx.pipeline(); | |||
| cp.addLast("codec", new HttpServerCodec()); | |||
| cp.addLast("aggregator", new HttpObjectAggregator(MAX_PAYLOAD_SIZE)); | |||
|
|
|||
| if (corsConfig.isCorsSupportEnabled()) { | |||
There was a problem hiding this comment.
Any reason not to return CORS headers always, with restrictive defaults? Or at least default to on?
There was a problem hiding this comment.
probably not. most frameworks make cors an opt-in experience. for those that don't need it its just one more bit of overhead.
There was a problem hiding this comment.
for now leaving this as opt-in
| sslContext.init(keyManagers, trustAllCerts, new java.security.SecureRandom()); | ||
| // Create an ssl socket factory with our all-trusting manager | ||
| final SSLSocketFactory sslSocketFactory = sslContext.getSocketFactory(); | ||
| // System.out.println("supported ciphers: " + |
There was a problem hiding this comment.
Remove commented-out code?
There was a problem hiding this comment.
ha. copied this from nfe... will do...
| cors_allowed_origins = [] | ||
| cors_allowed_headers = [] | ||
| cors_allowed_methods = [] | ||
| cors_allow_credentials = false |
There was a problem hiding this comment.
I would document this and short_circuit below, since they are non-obvious.
It's not clear to me how short_circuit would affect interaction with a browser, or what exactly it does. I think it only affects OPTIONS requests, and might actually be a nice safeguard against accidental root ANY handlers.
There was a problem hiding this comment.
allow_credentials is a standard cors header, but i'll add documentation for both. i was toying with the idea of defaulting short circuit to true..
| @@ -37,6 +43,8 @@ | |||
| * href="https://github.com/Nordstrom/xrpc/blob/master/src/main/resources/com/nordstrom/xrpc/xrpc.conf">the | |||
| * embedded config</a> for defaults and documentation. | |||
| */ | |||
| @Accessors(fluent = true) | |||
|
have made fixes based on review... |
bin/corstest.sh
Outdated
| @@ -0,0 +1,73 @@ | |||
| #!/bin/bash -e | |||
|
|
|||
| LINE_BREAK="======================================" | |||
There was a problem hiding this comment.
It's not entirely clear why this file is in the repo; document?
demo/src/main/resources/demo.conf
Outdated
| @@ -15,4 +15,7 @@ xrpc { | |||
| # IP White List | |||
| enable_white_list = true | |||
| ip_white_list = ["127.0.0.1"] | |||
|
|
|||
| enable_cors = true | |||
There was a problem hiding this comment.
Update the demo app config?
| populateClientOverrideList(config.getObjectList("req_per_second_override")); | ||
| } | ||
|
|
||
| private CorsConfig configureCors(Config config) { |
There was a problem hiding this comment.
Style suggestion: Name getCorsConfig or buildCorsConfig.
| meterNamesByStatusCode.put(HttpResponseStatus.UNAUTHORIZED, NAME_PREFIX + "unauthorized"); | ||
| meterNamesByStatusCode.put(HttpResponseStatus.FORBIDDEN, NAME_PREFIX + "forbidden"); | ||
| meterNamesByStatusCode.put(HttpResponseStatus.NOT_FOUND, NAME_PREFIX + "notFound"); | ||
| final String namePrefix = "responseCodes."; |
There was a problem hiding this comment.
If you wanted to keep the name, this could be a static final member.
There was a problem hiding this comment.
left this as final to keep it closer to its usage
|
fixed latest review comments |
No description provided.