On Dec 4, 2012, at 22:05 , Benoit Chesneau <[email protected]> wrote:
> couple of observations:
>
> 1 aesthetic, not sure why you changed cors_headers which was based on
> pattern to 2 functions but wtfm ;)
Yeah, not sure how else to do it cleanly because the headers of a response
dictate what the exposed headers set is.
> We are always returning a content-type in couchdb maybe we could simplify
> the code here.
We are, but only when it is application/x-www-form-urlencoded,
multipart/form-data,
or text/plain, they get the treatment. It sure isn’t pretty, but that’s what
the spec dictates.
> I am not happy to modify the header case in the ExposedHeaders, maybe we
> coulld have one list transformed in a proplist {"lower", {"RealCase"} and
> test the other against it and eventually add the missing one using a
> lists:foldl.
Yeah, I totally went with what was easier for now on this one. This can
easily be added later.
> I won't have any time to make the patch tonight, these are only
> observations I am happy to discuss.
Thanks! :)
Jan
--
>
> - benoît
>
>
> On Tue, Dec 4, 2012 at 9:43 PM, <[email protected]> wrote:
>
>> Updated Branches:
>> refs/heads/1368-fix-multipart-header-parts a67308e32 -> ce55c6e18
>> (forced update)
>>
>>
>> handle exposed headers
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/couchdb/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/couchdb/commit/ce55c6e1
>> Tree: http://git-wip-us.apache.org/repos/asf/couchdb/tree/ce55c6e1
>> Diff: http://git-wip-us.apache.org/repos/asf/couchdb/diff/ce55c6e1
>>
>> Branch: refs/heads/1368-fix-multipart-header-parts
>> Commit: ce55c6e18866411a4325b9caecab0af205ac07c2
>> Parents: c432156
>> Author: Jan Lehnardt <[email protected]>
>> Authored: Mon Nov 12 13:53:04 2012 +0100
>> Committer: Jan Lehnardt <[email protected]>
>> Committed: Tue Dec 4 21:42:26 2012 +0100
>>
>> ----------------------------------------------------------------------
>> src/couchdb/couch_httpd.erl | 29 ++++++++++---------
>> src/couchdb/couch_httpd_cors.erl | 50 +++++++++++++++++++++++++++++---
>> test/etap/231-cors.t | 7 +++-
>> 3 files changed, 65 insertions(+), 21 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/couchdb/blob/ce55c6e1/src/couchdb/couch_httpd.erl
>> ----------------------------------------------------------------------
>> diff --git a/src/couchdb/couch_httpd.erl b/src/couchdb/couch_httpd.erl
>> index eea530c..b71a6fc 100644
>> --- a/src/couchdb/couch_httpd.erl
>> +++ b/src/couchdb/couch_httpd.erl
>> @@ -462,10 +462,11 @@ serve_file(Req, RelativePath, DocumentRoot) ->
>> serve_file(#httpd{mochi_req=MochiReq}=Req, RelativePath, DocumentRoot,
>> ExtraHeaders) ->
>> log_request(Req, 200),
>> - {ok, MochiReq:serve_file(RelativePath, DocumentRoot, server_header()
>> ++
>> - couch_httpd_cors:cors_headers(Req) ++
>> - couch_httpd_auth:cookie_auth_header(Req, [])
>> ++
>> - ExtraHeaders)}.
>> + ResponseHeaders = server_header()
>> + ++ couch_httpd_auth:cookie_auth_header(Req, [])
>> + ++ ExtraHeaders,
>> + {ok, MochiReq:serve_file(RelativePath, DocumentRoot,
>> + couch_httpd_cors:cors_headers(Req, ResponseHeaders))}.
>>
>> qs_value(Req, Key) ->
>> qs_value(Req, Key, undefined).
>> @@ -616,9 +617,9 @@ start_response_length(#httpd{mochi_req=MochiReq}=Req,
>> Code, Headers, Length) ->
>> log_request(Req, Code),
>> couch_stats_collector:increment({httpd_status_codes, Code}),
>> Headers1 = Headers ++ server_header() ++
>> - couch_httpd_auth:cookie_auth_header(Req, Headers) ++
>> - couch_httpd_cors:cors_headers(Req),
>> - Resp = MochiReq:start_response_length({Code, Headers1, Length}),
>> + couch_httpd_auth:cookie_auth_header(Req, Headers),
>> + Headers2 = couch_httpd_cors:cors_headers(Req, Headers1),
>> + Resp = MochiReq:start_response_length({Code, Headers2, Length}),
>> case MochiReq:get(method) of
>> 'HEAD' -> throw({http_head_abort, Resp});
>> _ -> ok
>> @@ -629,8 +630,8 @@ start_response(#httpd{mochi_req=MochiReq}=Req, Code,
>> Headers) ->
>> log_request(Req, Code),
>> couch_stats_collector:increment({httpd_status_codes, Code}),
>> CookieHeader = couch_httpd_auth:cookie_auth_header(Req, Headers),
>> - Headers2 = Headers ++ server_header() ++ CookieHeader ++
>> - couch_httpd_cors:cors_headers(Req),
>> + Headers1 = Headers ++ server_header() ++ CookieHeader,
>> + Headers2 = couch_httpd_cors:cors_headers(Req, Headers1),
>> Resp = MochiReq:start_response({Code, Headers2}),
>> case MochiReq:get(method) of
>> 'HEAD' -> throw({http_head_abort, Resp});
>> @@ -664,9 +665,9 @@ start_chunked_response(#httpd{mochi_req=MochiReq}=Req,
>> Code, Headers) ->
>> couch_stats_collector:increment({httpd_status_codes, Code}),
>> Headers1 = http_1_0_keep_alive(MochiReq, Headers),
>> Headers2 = Headers1 ++ server_header() ++
>> - couch_httpd_auth:cookie_auth_header(Req, Headers1) ++
>> - couch_httpd_cors:cors_headers(Req),
>> - Resp = MochiReq:respond({Code, Headers2, chunked}),
>> + couch_httpd_auth:cookie_auth_header(Req, Headers1),
>> + Headers3 = couch_httpd_cors:cors_headers(Req, Headers2),
>> + Resp = MochiReq:respond({Code, Headers3, chunked}),
>> case MochiReq:get(method) of
>> 'HEAD' -> throw({http_head_abort, Resp});
>> _ -> ok
>> @@ -695,10 +696,10 @@ send_response(#httpd{mochi_req=MochiReq}=Req, Code,
>> Headers, Body) ->
>> true -> ok
>> end,
>> Headers2 = Headers1 ++ server_header() ++
>> - couch_httpd_cors:cors_headers(Req) ++
>> couch_httpd_auth:cookie_auth_header(Req, Headers1),
>> + Headers3 = couch_httpd_cors:cors_headers(Req, Headers2),
>>
>> - {ok, MochiReq:respond({Code, Headers2, Body})}.
>> + {ok, MochiReq:respond({Code, Headers3, Body})}.
>>
>> send_method_not_allowed(Req, Methods) ->
>> send_error(Req, 405, [{"Allow", Methods}], <<"method_not_allowed">>,
>> ?l2b("Only " ++ Methods ++ " allowed")).
>>
>>
>> http://git-wip-us.apache.org/repos/asf/couchdb/blob/ce55c6e1/src/couchdb/couch_httpd_cors.erl
>> ----------------------------------------------------------------------
>> diff --git a/src/couchdb/couch_httpd_cors.erl
>> b/src/couchdb/couch_httpd_cors.erl
>> index 17f634a..7cfefc3 100644
>> --- a/src/couchdb/couch_httpd_cors.erl
>> +++ b/src/couchdb/couch_httpd_cors.erl
>> @@ -20,7 +20,7 @@
>>
>> -include("couch_db.hrl").
>>
>> --export([is_preflight_request/1, cors_headers/1]).
>> +-export([is_preflight_request/1, cors_headers/2]).
>>
>> -define(SUPPORTED_HEADERS, "Accept, Accept-Language, Content-Type," ++
>> "Expires, Last-Modified, Pragma, Origin, Content-Length," ++
>> @@ -30,6 +30,12 @@
>> -define(SUPPORTED_METHODS, "GET, HEAD, POST, PUT, DELETE," ++
>> "TRACE, CONNECT, COPY, OPTIONS").
>>
>> +% as defined in http://www.w3.org/TR/cors/#terminology
>> +-define(SIMPLE_HEADERS, ["Cache-Control", "Content-Language",
>> + "Content-Type", "Expires", "Last-Modified", "Pragma"]).
>> +-define(SIMPLE_CONTENT_TYPE_VALUES, ["application/x-www-form-urlencoded",
>> + "multipart/form-data", "text/plain"]).
>> +
>> % TODO: - pick a sane default
>> -define(CORS_DEFAULT_MAX_AGE, 12345).
>>
>> @@ -173,11 +179,12 @@
>> send_preflight_response(#httpd{mochi_req=MochiReq}=Req, Headers) ->
>>
>> % cors_headers/1
>>
>> -cors_headers(MochiReq) ->
>> +cors_headers(MochiReq, RequestHeaders) ->
>> EnableCors = enable_cors(),
>> - cors_headers(MochiReq, EnableCors).
>> + CorsHeaders = do_cors_headers(MochiReq, EnableCors),
>> + maybe_apply_cors_headers(CorsHeaders, RequestHeaders).
>>
>> -cors_headers(#httpd{mochi_req=MochiReq}, true) ->
>> +do_cors_headers(#httpd{mochi_req=MochiReq}, true) ->
>> Host = couch_httpd_vhost:host(MochiReq),
>> AcceptedOrigins = get_accepted_origins(Host),
>> case MochiReq:get_header_value("Origin") of
>> @@ -191,9 +198,42 @@ cors_headers(#httpd{mochi_req=MochiReq}, true) ->
>> handle_cors_headers(couch_util:to_list(Origin),
>> Host, AcceptedOrigins)
>> end;
>> -cors_headers(_MochiReq, false) ->
>> +do_cors_headers(_MochiReq, false) ->
>> [].
>>
>> +maybe_apply_cors_headers([], RequestHeaders) ->
>> + RequestHeaders;
>> +maybe_apply_cors_headers(CorsHeaders, RequestHeaders) ->
>> + % for each RequestHeader that isn't in SimpleHeaders,
>> + % (or Content-Type with SIMPLE_CONTENT_TYPE_VALUES)
>> + % append to Access-Control-Exposed-Headers
>> + % return: RequestHeaders ++ CorsHeaders ++ ACEH
>> +
>> + LowerRequestHeaders = [string:to_lower(K) || {K,_V} <-
>> RequestHeaders],
>> + LowerSimpleHeaders = [string:to_lower(H) || H <- ?SIMPLE_HEADERS],
>> +
>> + ExposedHeaders0 = LowerRequestHeaders -- LowerSimpleHeaders,
>> +
>> + % here we may have not moved Content-Type into ExposedHeaders,
>> + % now we need to check whether the Content-Type valus is
>> + % in ?SIMPLE_CONTENT_TYPE_VALUES and if it isn’t add Content-
>> + % Type to to ExposedHeaders
>> + ContentType = string:to_lower(
>> + proplists:get_value("Content-Type", RequestHeaders)),
>> +
>> + IncludeContentType = lists:member(ContentType,
>> ?SIMPLE_CONTENT_TYPE_VALUES),
>> + ExposedHeaders = case IncludeContentType of
>> + false ->
>> + ExposedHeaders0 ++ ["Content-Type"];
>> + true ->
>> + ExposedHeaders0
>> + end,
>> +
>> + CorsHeaders
>> + ++ RequestHeaders
>> + ++ [{"Access-Control-Exposed-Headers",
>> + string:join(ExposedHeaders, ", ")}].
>> +
>>
>> handle_cors_headers(_Origin, _Host, []) ->
>> [];
>>
>>
>> http://git-wip-us.apache.org/repos/asf/couchdb/blob/ce55c6e1/test/etap/231-cors.t
>> ----------------------------------------------------------------------
>> diff --git a/test/etap/231-cors.t b/test/etap/231-cors.t
>> index efc19a6..3e44e1c 100644
>> --- a/test/etap/231-cors.t
>> +++ b/test/etap/231-cors.t
>> @@ -32,7 +32,7 @@ server() ->
>> main(_) ->
>> test_util:init_code_path(),
>>
>> - etap:plan(25),
>> + etap:plan(27),
>> case (catch test()) of
>> ok ->
>> etap:end_tests();
>> @@ -203,7 +203,10 @@ test_db_request(VHost) ->
>> {ok, _, RespHeaders, _Body} ->
>> etap:is(proplists:get_value("Access-Control-Allow-Origin",
>> RespHeaders),
>> "http://example.com";,
>> - "db Access-Control-Allow-Origin ok");
>> + "db Access-Control-Allow-Origin ok"),
>> + etap:is(proplists:get_value("Access-Control-Exposed-Headers",
>> RespHeaders),
>> + "server, Content-Type",
>> + "db Access-Control-ExposedHeaders ok");
>> _ ->
>> etap:is(false, true, "ibrowse failed")
>> end.
>>
>>
