You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@couchdb.apache.org by be...@apache.org on 2011/05/23 08:39:48 UTC
svn commit: r1126332 - /couchdb/trunk/src/couchdb/couch_httpd.erl
Author: benoitc
Date: Mon May 23 06:39:47 2011
New Revision: 1126332
URL: http://svn.apache.org/viewvc?rev=1126332&view=rev
Log:
Fix authentication. Jquery append "*.*" to accept by
default so if we test text/html first it will alway be true. Then test
first if application/json was given and then test if text/html then
others.
Modified:
couchdb/trunk/src/couchdb/couch_httpd.erl
Modified: couchdb/trunk/src/couchdb/couch_httpd.erl
URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_httpd.erl?rev=1126332&r1=1126331&r2=1126332&view=diff
==============================================================================
--- couchdb/trunk/src/couchdb/couch_httpd.erl (original)
+++ couchdb/trunk/src/couchdb/couch_httpd.erl Mon May 23 06:39:47 2011
@@ -768,24 +768,29 @@ error_headers(#httpd{mochi_req=MochiReq}
% 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("text/html") of
- false ->
- {Code, []};
+ case MochiReq:accepts_content_type("application/json") 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
- 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)}]}
+ {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
+ 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;
Re: svn commit: r1126332 -
/couchdb/trunk/src/couchdb/couch_httpd.erl
Posted by Caolan McMahon <ca...@gmail.com>.
Just to be clear, jQuery always appends '*/*' to the Accepts header when
making ajax requests. And it doesn't set a q value.
That means without this patch it is impossible to get a 401 response
from CouchDB using jQuery, you are always redirected. Its quite possible
they have a good reason for doing it that way, so other ajax libraries
might suffer from the same problem.
I definitely think this needs to be merged into 1.1.x otherwise it's
going to cause a lot of problems with CouchApps!
I agree the *correct* solution would be to use
MochiReq:accepted_encodings and cycle through the accepted content types
in order of preference until one is supported. Unfortuntely it seems
CouchApps can't work reliably with that system, being unable to set a
preference.
Caolan
Re: svn commit: r1126332 - /couchdb/trunk/src/couchdb/couch_httpd.erl
Posted by Filipe David Manana <fd...@apache.org>.
On Mon, May 23, 2011 at 12:55 PM, Robert Newson <ro...@gmail.com> wrote:
> I filed a ticket (COUCHDB-1175) to track this. I'm concerned about
> changes to multiple branches with no ticket (this is just one example,
> there are others).
>
> This fix seems insufficient to me. Should we find out the q-value of
> 'application/json' and 'text/plain' and use whichever is higher? The
> mere presence of 'application/json' is not sufficient to decide if
> it's prefered.
I don't think there's a serious issue.
The mochiweb request function accepts_content_encoding/1 takes q=0
values into account. So that means if the Accept header is like for
example:
"Accept: text/plain, application/json; q=0"
or
"Accept: text/plain, application/*; q=0, */*; q=1"
and we call Req:accepts_content_encoding("application/json"), it will
return false for both cases
As for Q values > 0, HTTP spec [1] seems to say that for all values
the client lists with q > 0, the server can decide which one it wants,
not necessarily the one with highest q value.
[1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
>
> B.
>
> On 23 May 2011 12:40, Filipe David Manana <fd...@apache.org> wrote:
>> On Mon, May 23, 2011 at 7:46 AM, Benoit Chesneau <bc...@gmail.com> wrote:
>>>
>>> We should probably merged it in 1.1.x too since it fixes a regression
>>> compared to 1.0.x
>>
>> I haven't tested it, only looked at the diff, but it looks ok to me.
>>
>>>
>>> - benoît
>>>
>>
>>
>>
>> --
>> Filipe David Manana,
>> fdmanana@gmail.com, fdmanana@apache.org
>>
>> "Reasonable men adapt themselves to the world.
>> Unreasonable men adapt the world to themselves.
>> That's why all progress depends on unreasonable men."
>>
>
--
Filipe David Manana,
fdmanana@gmail.com, fdmanana@apache.org
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
Re: svn commit: r1126332 - /couchdb/trunk/src/couchdb/couch_httpd.erl
Posted by Robert Newson <ro...@gmail.com>.
I filed a ticket (COUCHDB-1175) to track this. I'm concerned about
changes to multiple branches with no ticket (this is just one example,
there are others).
This fix seems insufficient to me. Should we find out the q-value of
'application/json' and 'text/plain' and use whichever is higher? The
mere presence of 'application/json' is not sufficient to decide if
it's prefered.
B.
On 23 May 2011 12:40, Filipe David Manana <fd...@apache.org> wrote:
> On Mon, May 23, 2011 at 7:46 AM, Benoit Chesneau <bc...@gmail.com> wrote:
>>
>> We should probably merged it in 1.1.x too since it fixes a regression
>> compared to 1.0.x
>
> I haven't tested it, only looked at the diff, but it looks ok to me.
>
>>
>> - benoît
>>
>
>
>
> --
> Filipe David Manana,
> fdmanana@gmail.com, fdmanana@apache.org
>
> "Reasonable men adapt themselves to the world.
> Unreasonable men adapt the world to themselves.
> That's why all progress depends on unreasonable men."
>
Re: svn commit: r1126332 - /couchdb/trunk/src/couchdb/couch_httpd.erl
Posted by Filipe David Manana <fd...@apache.org>.
On Mon, May 23, 2011 at 7:46 AM, Benoit Chesneau <bc...@gmail.com> wrote:
>
> We should probably merged it in 1.1.x too since it fixes a regression
> compared to 1.0.x
I haven't tested it, only looked at the diff, but it looks ok to me.
>
> - benoît
>
--
Filipe David Manana,
fdmanana@gmail.com, fdmanana@apache.org
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
Re: svn commit: r1126332 - /couchdb/trunk/src/couchdb/couch_httpd.erl
Posted by Benoit Chesneau <bc...@gmail.com>.
On Mon, May 23, 2011 at 8:39 AM, <be...@apache.org> wrote:
> Author: benoitc
> Date: Mon May 23 06:39:47 2011
> New Revision: 1126332
>
> URL: http://svn.apache.org/viewvc?rev=1126332&view=rev
> Log:
> Fix authentication. Jquery append "*.*" to accept by
> default so if we test text/html first it will alway be true. Then test
> first if application/json was given and then test if text/html then
> others.
>
>
> Modified:
> couchdb/trunk/src/couchdb/couch_httpd.erl
>
> Modified: couchdb/trunk/src/couchdb/couch_httpd.erl
> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_httpd.erl?rev=1126332&r1=1126331&r2=1126332&view=diff
> ==============================================================================
> --- couchdb/trunk/src/couchdb/couch_httpd.erl (original)
> +++ couchdb/trunk/src/couchdb/couch_httpd.erl Mon May 23 06:39:47 2011
> @@ -768,24 +768,29 @@ error_headers(#httpd{mochi_req=MochiReq}
> % 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("text/html") of
> - false ->
> - {Code, []};
> + case MochiReq:accepts_content_type("application/json") 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
> - 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)}]}
> + {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
> + 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;
>
>
>
We should probably merged it in 1.1.x too since it fixes a regression
compared to 1.0.x
- benoît