I commented on the code inline, but to clarify here, what I was trying
to suggest below is to ignore the request method and *only* using the
Accepts header.
To me, the logic I think we should be expressing is:
if (isBrowserRequest(request)) {
if (isAjaxRequest(request)) {
return 403;
} else {
hand off to AuthenticationHandler;
}
} else {
return 401;
}
The login page is a web browser-specific notion. Any other HTTP client
should really get a 401 response. This would include curl, wget, mobile
apps, etc...
I posted an alternative patch here: http://codereview.appspot.com/2252043
This also fixes SLING-1428, which isn't strictly related; the reason for
this inclusion is that the test is the basically the same. This can, of
course, be peeled off into a separate commit.
Justin
On 9/21/10 4:11 AM, Felix Meschberger wrote:
> Hi,
>
> Ok, I have uploaded another patch with two methods to check for WebDAV
> initial request and Ajax request.
>
> I for now left out any more intense Accepts header testing since none of
> the WebDAV clients I tested with (see JavaDoc) sent an Accepts header at
> all.
>
> I also ommited adding a header with an URL to use for login. For one the
> main URL to be used is part of the LoginServlet configuration (outside
> of the control of the SlingAuthenticator class) and second because just
> sending a regular GET request to the same URL should actually also
> trigger a login redirect. In essence: there is no use in such a header.
>
> As for Flex: This is really a sad situation currently. For now, I do not
> know how to deal with this and would like to leave it out. We can still
> fix it once someone comes along with more insight.
>
> Regards
> Felix
>
> Am 20.09.2010 21:44, schrieb Justin Edelson:
>> On 9/20/10 1:56 PM, Felix Meschberger wrote:
>>> Hi,
>>>
>>> Am 20.09.2010 17:16, schrieb Justin Edelson:
>>>> Two comments:
>>>>
>>>> 1) As mentioned, I think we should use the Accepts header instead of
>>>> specifically checking for the OPTIONS method and return a 401 response
>>>> unless the Accepts header is included in the request and *explicitly*
>>>> contains text/html. Since I don't have the time right now to research
>>>> this, could you just pull out this block:
>>>
>>> As I said, this is about WebDAV clients which (I would assume, could/did
>>> not find anything concrete in this respect, but this is what my
>>> experience tells) start communication with an OPTIONS request. Probably
>>> to find out about DAV support (OPTIONS response must have a DAV response
>>> header indicating the level of DAV support).
>>>
>>> Such an OPTIONS request will generally not have an Accepts header. But
>>> we could use the Accepts header check as negative check: Assume WebDAV
>>> OPTIONS request if Accepts header is not present.
>> Right, I'm suggesting that if a request doesn't have an Accepts header,
>> give it a 401.
>>
>> Accepts value Response code
>> ------------- -------------
>> (no header) 401
>> */* 401
>> text/xml 401
>> text/html 302 or 403 (depends on X-Requested-With)
>> text/xml,text/html 302 or 403 (depends on X-Requested-With)
>>
>> I would assume that a WebDAV client would either not use the Accepts
>> header or pass */*. But, as I said, I haven't had the time to test this.
>>
>>>
>>>>
>>>> + if (HttpConstants.METHOD_OPTIONS.equals(request.getMethod())) {
>>>> +
>>>> + // Presumably this is WebDAV. If HTTP Basic is fully enabled
>>>> or
>>>> + // enabled for preemptive credential support, we just request
>>>> + // HTTP Basic credentials. Otherwise (HTTP Basic is fully
>>>> + // switched off, 403 is sent back)
>>>> + if (httpBasicHandler != null) {
>>>> + httpBasicHandler.sendUnauthorized(response);
>>>> + return;
>>>> + }
>>>>
>>>> into a "shouldReturnUnauthorizedResponse" method? That way, where this
>>>> logic gets change will be more obvious.
>>>
>>> Makes sense. For consistency I would then also craete a method "isAjax"
>>> (or such) for the other test.
>> Agreed.
>>
>>>>
>>>> 2) As Ian suggested, we should include the form path into the response
>>>> somehow. We could use the Location header, but perhaps we should use a
>>>> custom header like X-Login-Form.
>>>>
>>>> The goal of #2 is to allow a JavaScript client to do either (a) show a
>>>> custom login dialog or (b) redirect the user to the login form
>>>> (presumably keeping some kind of state).
>>>
>>> Makes sense - I just did not add this yet to the proposed patch. I would
>>> propose to use a custom header instead of reusing the Location header,
>>> which has specific meaning according to RFC 2616.
>> Well, I think this use case falls under the spec, but I don't feel
>> strongly about this one way or the other.
>>
>>>
>>>>
>>>> Might also be a good idea to get some feedback on this from a Flex
>>>> person. IIRC, there were weird issues with 401/403 responses with Flex.
>>>
>>> We might want to treat Flex similar to Ajax, probably.
>> I don't think this will work (at least it wouldn't work a year ago,
>> maybe things have changed). When running inside the browser, any non-200
>> response status appears to the Flash runtime as a 500. I don't know if
>> there's an equivalent to X-Requested-With: XmlHttpRequest for Flash
>> clients. This is apparently a browser limitation (see
>> http://bugs.adobe.com/jira/browse/SDK-11841).
>>
>> Justin
>>
>>>
>>> Regards
>>> Felix
>>>
>>>>
>>>> Justin
>>>>
>>>> On 9/20/10 10:19 AM, Felix Meschberger wrote:
>>>>> Hi all,
>>>>>
>>>>> I have uploaded a proposed patch including support for both issues to
>>>>> http://codereview.appspot.com/2192046/.
>>>>>
>>>>> Please comment. Thanks.
>>>>>
>>>>> Regards
>>>>> Felix
>>>>>
>>>>>
>>>>>
>>>>> Am 17.09.2010 16:59, schrieb Felix Meschberger:
>>>>>> Hi all,
>>>>>>
>>>>>> I am trying to tackle issues SLING-1400 [1] and SLING-1745 [2].
>>>>>>
>>>>>> The first issue is about WebDAV clients connecting to Sling on root with
>>>>>> an OPTIONS request and not being happy with a redirect response,
>>>>>> obviously.
>>>>>>
>>>>>> The second issue is about client side JavaScript application framework
>>>>>> which may send XHR requests to Sling, mainly POSTs destined for the POST
>>>>>> Servlet but probably also other stuff. Such framework are also generally
>>>>>> not very happy getting redirect responses back.
>>>>>>
>>>>>> Solutions for both problems would probably have to be implemented in the
>>>>>> SlingAuthenticator.doLogin method, which is called after an unsuccessful
>>>>>> login or after a first request noticing that authentication is required.
>>>>>>
>>>>>> So here are the options I came up with:
>>>>>>
>>>>>> * Send back a 401 response, at least for the OPTIONS request
>>>>>> to trigger a regular HTTP Basic Authentication
>>>>>> * Send back a 403 response, to indicate that access is currently
>>>>>> forbidden (we discussed this option earlier [3]).
>>>>>>
>>>>>> My questions:
>>>>>>
>>>>>> - Would it be ok to special case the OPTIONS request ?
>>>>>> - Shall we generally only send back a generic credentials request
>>>>>> (may be a redirect or a form directly or whatever) if the
>>>>>> original request was GET and send back either 401 or 403 for
>>>>>> all non-GET requests, including HEAD ?
>>>>>> - Is it a good idea to send back 401 generally ?
>>>>>> - Should we only send back 401 if HTTP Basic authentication is
>>>>>> at enabled fully or enabled preemptively and send back 403 if
>>>>>> HTTP Basic authentication is switched off completely ?
>>>>>> - Am I completely off track ?
>>>>>>
>>>>>> WDYT ?
>>>>>>
>>>>>> Regards
>>>>>> Felix
>>>>>>
>>>>>>
>>>>>>
>>>>>> [1] https://issues.apache.org/jira/browse/SLING-1400
>>>>>> [2] https://issues.apache.org/jira/browse/SLING-1745
>>>>>> [3] http://markmail.org/message/jwsvk6swnxvvfsyz
>>>>
>>>>
>>
>>
