couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Lehnardt <...@apache.org>
Subject Re: [1/44] git commit: handle exposed headers
Date Tue, 04 Dec 2012 22:44:05 GMT

On Dec 4, 2012, at 22:05 , Benoit Chesneau <bchesneau@gmail.com> 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, <jan@apache.org> 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 <jan@apache.org>
>> Authored: Mon Nov 12 13:53:04 2012 +0100
>> Committer: Jan Lehnardt <jan@apache.org>
>> 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.
>> 
>> 


Mime
View raw message