On Friday 04 April 2014 12:20:33 Carsten Ziegeler wrote:
> Why do you want to keep getPath() unchanged? You have a get method, get
> something and then have to check the value and then default to /.
> (In the end I don't care where the fix ends up, more curious)
because we have otherwise get something and change it to a default in
getPath() *and* we are closer to SLING-722:
Index:
bundles/auth/core/src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
===================================================================
---
bundles/auth/core/src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
(revision 1584529)
+++
bundles/auth/core/src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
(working copy)
@@ -680,13 +680,11 @@
private AuthenticationInfo getAuthenticationInfo(HttpServletRequest
request, HttpServletResponse response) {
// Get the path used to select the authenticator, if the SlingServlet
- // itself has been requested without any more info, this will be null
+ // itself has been requested without any more info, this will be
empty
// and we assume the root (SLING-722)
- final String path = getPath(request);
+ String path = getPath(request);
if (path.length() == 0) {
- // should not happen, be safe an return anonymous credentials
- log.warn("get authentication info: request path is empty;
assuming anonymous");
- return getAnonymousCredentials();
+ path = "/";
}
final List<AbstractAuthenticationHandlerHolder>[] localArray =
this.authHandlerCache.findApplicableHolder(request);
O.
> Carsten
>
> 2014-04-04 12:13 GMT+02:00 Oliver Lietz <[email protected]>:
> > On Friday 04 April 2014 11:16:36 Carsten Ziegeler wrote:
> > > So I guess its as simple as this:
> > >
> > > Index:
> > > src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
> > > =================================================================== ---
> > > src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
> > > (Revision 1584521)
> > > +++
> > > src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
> > > (Arbeitskopie)
> > > @@ -674,6 +674,9 @@
> > >
> > > if (request.getPathInfo() != null) {
> > >
> > > sb.append(request.getPathInfo());
> > >
> > > }
> > >
> > > + if ( sb.length() == 0 ) {
> > > + sb.append("/");
> > > + }
> > >
> > > return sb.toString();
> > >
> > > }
> > >
> > > @@ -683,11 +686,6 @@
> > >
> > > // itself has been requested without any more info, this will
> > > be
> > >
> > > null
> > >
> > > // and we assume the root (SLING-722)
> > > final String path = getPath(request);
> > >
> > > - if (path.length() == 0) {
> > > - // should not happen, be safe an return anonymous
> >
> > credentials
> >
> > > - log.warn("get authentication info: request path is empty;
> > > assuming anonymous");
> > > - return getAnonymousCredentials();
> > > - }
> > >
> > > final List<AbstractAuthenticationHandlerHolder>[] localArray =
> > >
> > > this.authHandlerCache.findApplicableHolder(request);
> > >
> > > for (int m = 0; m < localArray.length; m++) {
> >
> > So we have a case where #getServletPath() returns "" and #getPathInfo()
> > null?
> >
> >
> > http://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletRequest
> > .html#getServletPath()
> >
> > Returns the part of this request's URL that calls the servlet. This path
> > starts with a "/" character and includes either the servlet name or a
> > path to
> > the servlet, but does not include any extra path information or a query
> > string. Same as the value of the CGI variable SCRIPT_NAME.
> > This method will return an empty string ("") if the servlet used to
> > process this request was matched using the "/*" pattern.
> >
> >
> >
> > http://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletRequest
> > .html#getPathInfo()
> >
> > Returns any extra path information associated with the URL the client
> > sent when it made this request. The extra path information follows the
> > servlet path
> > but precedes the query string and will start with a "/" character.
> > This method returns null if there was no extra path information.
> > Same as the value of the CGI variable PATH_INFO.
> >
> > @Carsten: I would keep getPath() unchanged and change the path after the
> >
> > comment for SLING-722:
> > String path = getPath(request);
> > if (path.length() == 0) {
> >
> > path = "/";
> >
> > }
> >
> > Regards,
> > O.
> >
> > > Do we have an issue for this?
> > >
> > > Regards
> > > Carsten
> > >
> > > 2014-04-04 10:17 GMT+02:00 Antonio Sanso <[email protected]>:
> > > > On Apr 4, 2014, at 10:14 AM, Carsten Ziegeler <[email protected]
> > > >
> > > > <mailto:[email protected]>> wrote:
> > > > the
> > > >
> > > > fallback at the end is still needed I guess.
> > > >
> > > > for sure.... :)
> > > >
> > > > regards
> > > >
> > > > antonio
> > > >
> > > >
> > > > Carsten
> > > >
> > > >
> > > > 2014-04-04 10:01 GMT+02:00 Antonio Sanso <[email protected]<mailto:
> > > > [email protected]>>:
> > > >
> > > >
> > > > On Apr 4, 2014, at 9:54 AM, Carsten Ziegeler
> > > > <[email protected]<mailto: [email protected]>> wrote:
> > > >
> > > > So we could just add the check to the getPath() method, and this
> > > > should fix
> > > > the problem, right?
> > > >
> > > >
> > > > I think so. At that point we can as well remove the associated
> > > > getAnonymousCredentials() since getPath can never be empty anymore.
> >
> > Will
> >
> > > > be at least "/"
> > > >
> > > > regards
> > > >
> > > > antonio
> > > >
> > > > Carsten
> > > >
> > > >
> > > > 2014-04-04 9:11 GMT+02:00 Antonio Sanso <[email protected]<mailto:
> > > > [email protected]>>:
> > > >
> > > > hi carsten
> > > > On Apr 3, 2014, at 6:34 PM, Carsten Ziegeler
> > > > <[email protected]<mailto: [email protected]>>
> > > > wrote:
> > > >
> > > > Hi Antonio,
> > > >
> > > > where is the NPE happening?
> > > >
> > > > I copied the subject from SLING-722 . With the current code there is
> > > > actual no NPE but some functional issue (see my mail to Oliver)
> > > >
> > > > regards
> > > >
> > > > antonio
> > > >
> > > >
> > > > Carsten
> > > >
> > > >
> > > > 2014-04-03 16:12 GMT+02:00 Antonio Sanso <[email protected]<mailto:
> > > > [email protected]>>:
> > > >
> > > > hi *,
> > > >
> > > > SLING-722 introduced an important fix to solve some issue related to
> > > > sling
> > > > being registered in a Servlet container.
> > > > Unluckily this fix has been removed in SLING-2998.
> > > > The rationale behind this is not too clear to me. I am not against
> > > > SLING-2998 per se.
> > > > I think though that SLING-2998 should also contain the fix contained
> > > > in
> > > > SLING-722.
> > > >
> > > > @oliver WDYT?
> > > >
> > > > regards
> > > >
> > > > antonio
> > > >
> > > >
> > > > [0] https://issues.apache.org/jira/browse/SLING-722
> > > > [1] https://issues.apache.org/jira/browse/SLING-2998
