couple of observations:
1 aesthetic, not sure why you changed cors_headers which was based on
pattern to 2 functions but wtfm ;)
We are always returning a content-type in couchdb maybe we could simplify
the code here.
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.
I won't have any time to make the patch tonight, these are only
observations I am happy to discuss.
- 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.
>
>
