couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benoit Chesneau <bchesn...@gmail.com>
Subject Re: [1/44] git commit: handle exposed headers
Date Tue, 04 Dec 2012 21:05:05 GMT
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, <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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message