BTW, for WebDAV clients, I tested on Ubuntu 10.04/Gnome VFS and OS X 10.6.4/Finder.
On Wed, Sep 22, 2010 at 12:39 PM, Justin Edelson <[email protected]> wrote: > 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 >>>>> >>>>> >>> >>> > >
