jaydoane commented on a change in pull request #3568:
URL: https://github.com/apache/couchdb/pull/3568#discussion_r644125381
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -1098,59 +1232,84 @@ maybe_handle_error(Error) ->
{500, <<"unknown_error">>, couch_util:to_binary(Error)}
end.
-
-error_headers(#httpd{mochi_req=MochiReq}=Req, 401=Code, ErrorStr, ReasonStr) ->
+error_headers(#httpd{mochi_req = MochiReq} = Req, 401 = Code, ErrorStr,
ReasonStr) ->
% this is where the basic auth popup is triggered
case MochiReq:get_header_value("X-CouchDB-WWW-Authenticate") of
- undefined ->
- case config:get("httpd", "WWW-Authenticate", undefined) of
undefined ->
- % If the client is a browser and the basic auth popup isn't turned
on
- % redirect to the session page.
- case ErrorStr of
- <<"unauthorized">> ->
- case config:get("couch_httpd_auth", "authentication_redirect",
undefined) of
- undefined -> {Code, []};
- AuthRedirect ->
- case config:get("couch_httpd_auth", "require_valid_user",
"false") of
- "true" ->
- % send the browser popup header no matter what if we
are require_valid_user
- {Code, [{"WWW-Authenticate", "Basic
realm=\"server\""}]};
- _False ->
- case MochiReq:accepts_content_type("application/json")
of
- true ->
- {Code, []};
- false ->
- case MochiReq:accepts_content_type("text/html") of
- true ->
- % Redirect to the path the user requested, not
- % the one that is used internally.
- UrlReturnRaw = case
MochiReq:get_header_value("x-couchdb-vhost-path") of
+ case config:get("httpd", "WWW-Authenticate", undefined) of
+ undefined ->
+ % If the client is a browser and the basic auth popup
isn't turned on
+ % redirect to the session page.
+ case ErrorStr of
+ <<"unauthorized">> ->
+ case
+ config:get("couch_httpd_auth",
"authentication_redirect", undefined)
+ of
undefined ->
- MochiReq:get(path);
- VHostPath ->
- VHostPath
- end,
- RedirectLocation = lists:flatten([
- AuthRedirect,
- "?return=",
couch_util:url_encode(UrlReturnRaw),
- "&reason=",
couch_util:url_encode(ReasonStr)
- ]),
- {302, [{"Location", absolute_uri(Req,
RedirectLocation)}]};
- false ->
- {Code, []}
- end
- end
- end
- end;
- _Else ->
- {Code, []}
+ {Code, []};
+ AuthRedirect ->
+ case
+ config:get(
+ "couch_httpd_auth",
+ "require_valid_user",
+ "false"
+ )
+ of
+ "true" ->
+ % send the browser popup header no
matter what if we are require_valid_user
Review comment:
-3 Way longer than 80 chars.
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -537,15 +641,22 @@ extract_cookie(#httpd{mochi_req = MochiReq}) ->
%%% end hack
set_auth_handlers() ->
- AuthenticationDefault = "{chttpd_auth, cookie_authentication_handler},
- {chttpd_auth, default_authentication_handler}",
+ AuthenticationDefault =
+ "{chttpd_auth, cookie_authentication_handler},\n"
Review comment:
-1
##########
File path: src/couch/src/couch_ejson_compare.erl
##########
@@ -25,87 +24,79 @@ init() ->
% partitioned row comparison
less({p, PA, A}, {p, PB, B}) ->
less([PA, A], [PB, B]);
-
less(A, B) ->
try
less_nif(A, B)
catch
- error:badarg ->
- % Maybe the EJSON structure is too deep, fallback to Erlang land.
- less_erl(A, B)
+ error:badarg ->
+ % Maybe the EJSON structure is too deep, fallback to Erlang land.
+ less_erl(A, B)
end.
less_json_ids({JsonA, IdA}, {JsonB, IdB}) ->
case less(JsonA, JsonB) of
- 0 ->
- IdA < IdB;
- Result ->
- Result < 0
+ 0 ->
+ IdA < IdB;
+ Result ->
+ Result < 0
end.
-less_json(A,B) ->
+less_json(A, B) ->
less(A, B) < 0.
-
less_nif(A, B) ->
less_erl(A, B).
-
-less_erl(A,A) -> 0;
-
-less_erl(A,B) when is_atom(A), is_atom(B) -> atom_sort(A) - atom_sort(B);
-less_erl(A,_) when is_atom(A) -> -1;
-less_erl(_,B) when is_atom(B) -> 1;
-
-less_erl(A,B) when is_number(A), is_number(B) -> A - B;
-less_erl(A,_) when is_number(A) -> -1;
-less_erl(_,B) when is_number(B) -> 1;
-
-less_erl(A,B) when is_binary(A), is_binary(B) -> couch_util:collate(A,B);
-less_erl(A,_) when is_binary(A) -> -1;
-less_erl(_,B) when is_binary(B) -> 1;
-
-less_erl(A,B) when is_list(A), is_list(B) -> less_list(A,B);
-less_erl(A,_) when is_list(A) -> -1;
-less_erl(_,B) when is_list(B) -> 1;
-
-less_erl({A},{B}) when is_list(A), is_list(B) -> less_props(A,B);
-less_erl({A},_) when is_list(A) -> -1;
-less_erl(_,{B}) when is_list(B) -> 1.
+less_erl(A, A) -> 0;
Review comment:
+1
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -993,8 +1112,10 @@ error_info({conflict, _}) ->
{409, <<"conflict">>, <<"Document update conflict.">>};
error_info({partition_overflow, DocId}) ->
Descr = <<
- "Partition limit exceeded due to update on '", DocId/binary, "'"
- >>,
+ "Partition limit exceeded due to update on '",
Review comment:
-1 Why does it sometimes end a line with the `=` sign, and other times
end the line with opening symbols (`<<` in this case)? It seems inconsistent.
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -132,19 +185,21 @@ start_link(Name, Options) ->
set_auth_handlers(),
- Options1 = Options ++ [
- {loop, fun ?MODULE:handle_request/1},
- {name, Name},
- {ip, IP}
- ],
+ Options1 =
+ Options ++
+ [
+ {loop, fun ?MODULE:handle_request/1},
+ {name, Name},
+ {ip, IP}
+ ],
ServerOpts = get_server_options("chttpd"),
Options2 = merge_server_options(Options1, ServerOpts),
case mochiweb_http:start(Options2) of
- {ok, Pid} ->
- {ok, Pid};
- {error, Reason} ->
- io:format("Failure to start Mochiweb: ~s~n", [Reason]),
- {error, Reason}
+ {ok, Pid} ->
Review comment:
+3
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -537,15 +641,22 @@ extract_cookie(#httpd{mochi_req = MochiReq}) ->
%%% end hack
set_auth_handlers() ->
- AuthenticationDefault = "{chttpd_auth, cookie_authentication_handler},
- {chttpd_auth, default_authentication_handler}",
+ AuthenticationDefault =
+ "{chttpd_auth, cookie_authentication_handler},\n"
+ " {chttpd_auth, default_authentication_handler}",
AuthenticationSrcs = couch_httpd:make_fun_spec_strs(
- config:get("chttpd", "authentication_handlers",
AuthenticationDefault)),
+ config:get("chttpd", "authentication_handlers", AuthenticationDefault)
+ ),
AuthHandlers = lists:map(
- fun(A) -> {auth_handler_name(A), couch_httpd:make_arity_1_fun(A)} end,
AuthenticationSrcs),
- AuthenticationFuns = AuthHandlers ++ [
- fun chttpd_auth:party_mode_handler/1 %% must be last
- ],
+ fun(A) -> {auth_handler_name(A), couch_httpd:make_arity_1_fun(A)} end,
Review comment:
-1
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -1491,67 +1704,98 @@ check_url_encoding_pass_test_() ->
check_url_encoding_fail_test_() ->
[
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname%")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname/doc_id%")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname/doc_id%?rev=1-abcdefgh")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname%2")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname/doc_id%2")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/user%2Fdbname%")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/user%2Fdbname/doc_id%")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("%")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/%")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/%2")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname%2%3A")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname%%3Ae")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname%2g")),
- ?_assertThrow({bad_request, invalid_url_encoding},
- check_url_encoding("/dbname%g2"))
+ ?_assertThrow(
Review comment:
-1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
