Return-Path: 
 <commits-return-6744-apmail-sling-commits-archive=sling.apache.org@sling.apache.org>
Delivered-To: apmail-sling-commits-archive@www.apache.org
Received: (qmail 88000 invoked from network); 19 Jan 2010 11:25:31 -0000
Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.3)
  by minotaur.apache.org with SMTP; 19 Jan 2010 11:25:31 -0000
Received: (qmail 8775 invoked by uid 500); 19 Jan 2010 11:25:31 -0000
Delivered-To: apmail-sling-commits-archive@sling.apache.org
Received: (qmail 8718 invoked by uid 500); 19 Jan 2010 11:25:31 -0000
Mailing-List: contact commits-help@sling.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:commits-help@sling.apache.org>
List-Unsubscribe: <mailto:commits-unsubscribe@sling.apache.org>
List-Post: <mailto:commits@sling.apache.org>
List-Id: <commits.sling.apache.org>
Reply-To: dev@sling.apache.org
Delivered-To: mailing list commits@sling.apache.org
Received: (qmail 8709 invoked by uid 99); 19 Jan 2010 11:25:31 -0000
Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136)
    by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 19 Jan 2010 11:25:31 +0000
X-ASF-Spam-Status: No, hits=-2000.0 required=10.0
	tests=ALL_TRUSTED
X-Spam-Check-By: apache.org
Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4)
    by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 19 Jan 2010 11:25:30 +0000
Received: by eris.apache.org (Postfix, from userid 65534)
	id 86BB123889FA; Tue, 19 Jan 2010 11:25:10 +0000 (UTC)
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r900728 - in /sling/trunk/bundles/commons/auth: ./
 src/main/java/org/apache/sling/commons/auth/impl/ src/test/ src/test/java/
 src/test/java/org/ src/test/java/org/apache/
 src/test/java/org/apache/sling/ src/test/java/org/apache/sling/common...
Date: Tue, 19 Jan 2010 11:25:10 -0000
To: commits@sling.apache.org
From: fmeschbe@apache.org
X-Mailer: svnmailer-1.0.8
Message-Id: <20100119112510.86BB123889FA@eris.apache.org>

Author: fmeschbe
Date: Tue Jan 19 11:25:09 2010
New Revision: 900728

URL: http://svn.apache.org/viewvc?rev=900728&view=rev
Log:
SLING-1287 Ensure the impersonation cookie's value is properly quoted if it contains non-token characters. For ease of implementation, we always quote the cookie when setting but accept unquoted cookies when reading.

Added:
    sling/trunk/bundles/commons/auth/src/test/
    sling/trunk/bundles/commons/auth/src/test/java/
    sling/trunk/bundles/commons/auth/src/test/java/org/
    sling/trunk/bundles/commons/auth/src/test/java/org/apache/
    sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/
    sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/
    sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/
    sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/
    sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/SlingAuthenticatorTest.java   (with props)
Modified:
    sling/trunk/bundles/commons/auth/pom.xml
    sling/trunk/bundles/commons/auth/src/main/java/org/apache/sling/commons/auth/impl/SlingAuthenticator.java

Modified: sling/trunk/bundles/commons/auth/pom.xml
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/commons/auth/pom.xml?rev=900728&r1=900727&r2=900728&view=diff
==============================================================================
--- sling/trunk/bundles/commons/auth/pom.xml (original)
+++ sling/trunk/bundles/commons/auth/pom.xml Tue Jan 19 11:25:09 2010
@@ -128,9 +128,15 @@
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-api</artifactId>
         </dependency>
+        
+        <!-- Test Dependencies -->
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
         </dependency>
+        <dependency>
+            <groupId>org.slf4j</groupId>
+            <artifactId>slf4j-simple</artifactId>
+        </dependency>
     </dependencies>
 </project>

Modified: sling/trunk/bundles/commons/auth/src/main/java/org/apache/sling/commons/auth/impl/SlingAuthenticator.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/commons/auth/src/main/java/org/apache/sling/commons/auth/impl/SlingAuthenticator.java?rev=900728&r1=900727&r2=900728&view=diff
==============================================================================
--- sling/trunk/bundles/commons/auth/src/main/java/org/apache/sling/commons/auth/impl/SlingAuthenticator.java (original)
+++ sling/trunk/bundles/commons/auth/src/main/java/org/apache/sling/commons/auth/impl/SlingAuthenticator.java Tue Jan 19 11:25:09 2010
@@ -677,33 +677,53 @@
      *
      * @param response The {@link DeliveryHttpServletResponse} on which to send
      *            back the cookie.
-     * @param name The name of the cookie to send.
-     * @param value The value of cookie.
+     * @param user The name of the user to impersonate as. This will be quoted
+     *            and used as the cookie value.
      * @param maxAge The maximum age of the cookie in seconds. Positive values
      *            are persisted on the browser for the indicated number of
      *            seconds, setting the age to 0 (zero) causes the cookie to be
      *            deleted in the browser and using a negative value defines a
      *            temporary cookie to be deleted when the browser exits.
-     * @param path The cookie path to use. If empty or <code>null</code> the
+     * @param path The cookie path to use. This is intended to be the web
+     *            application's context path. If this is empty or
+     *            <code>null</code> the root path will be used assuming the web
+     *            application is registered at the root context.
+     * @param owner The name of the user originally authenticated in the request
+     *            and who is now impersonating as <i>user</i>.
      */
-    private void sendCookie(HttpServletResponse response, String name,
-            String value, int maxAge, String path) {
-
-        if (path == null || path.length() == 0) {
-            log.debug("sendCookie: Using root path ''/''");
-            path = "/";
+    private void sendSudoCookie(HttpServletResponse response,
+            final String user, final int maxAge, final String path,
+            final String owner) {
+
+        final String quotedUser;
+        try {
+            quotedUser = quoteCookieValue(user);
+        } catch (IllegalArgumentException iae) {
+            log.error(
+                "sendCookie: Failed to quote value '{}' of cookie {}: {}",
+                new Object[] { user, this.sudoCookieName, iae.getMessage() });
+            return;
         }
 
-        Cookie cookie = new Cookie(name, value);
+        if (quotedUser != null) {
+            Cookie cookie = new Cookie(this.sudoCookieName, quotedUser);
             cookie.setMaxAge(maxAge);
-        cookie.setPath(path);
+            cookie.setPath((path == null || path.length() == 0) ? "/" : path);
+            try {
+                cookie.setComment(owner + " impersonates as " + user);
+            } catch (IllegalArgumentException iae) {
+                // ignore
+            }
+
             response.addCookie(cookie);
 
-        // Tell a potential proxy server that this cookie is uncacheable
+            // Tell a potential proxy server that this request is uncacheable
+            // due to the Set-Cookie header being sent
             if (this.cacheControl) {
                 response.addHeader("Cache-Control", "no-cache=\"Set-Cookie\"");
             }
         }
+    }
 
     /**
      * Handles impersonation based on the request parameter for impersonation
@@ -740,7 +760,7 @@
         if (cookies != null) {
             for (int i = 0; currentSudo == null && i < cookies.length; i++) {
                 if (sudoCookieName.equals(cookies[i].getName())) {
-                    currentSudo = cookies[i].getValue();
+                    currentSudo = unquoteCookieValue(cookies[i].getValue());
                 }
             }
         }
@@ -759,6 +779,7 @@
         }
 
         // sudo the session if needed
+        final String authUser = session.getUserID();
         if (sudo != null && sudo.length() > 0) {
             Credentials creds = new SimpleCredentials(sudo, new char[0]);
             session = session.impersonate(creds);
@@ -772,16 +793,14 @@
                 // active due to cookie setting
 
                 // clear impersonation
-                this.sendCookie(res, this.sudoCookieName, "", 0,
-                    req.getContextPath());
+                this.sendSudoCookie(res, "", 0, req.getContextPath(), authUser);
 
             } else if (currentSudo == null || !currentSudo.equals(sudo)) {
                 // Parameter set to a name. As the cookie is not set yet
                 // or is set to another name, send the cookie with current sudo
 
                 // (re-)set impersonation
-                this.sendCookie(res, this.sudoCookieName, sudo, -1,
-                    req.getContextPath());
+                this.sendSudoCookie(res, sudo, -1, req.getContextPath(), authUser);
             }
         }
 
@@ -813,6 +832,81 @@
         return path;
     }
 
+    /**
+     * Ensures the cookie value is properly quoted for transmission to the
+     * client.
+     * <p>
+     * The problem is, that the character set of cookie values is limited by
+     * RFC 2109 defining that a cookie value must be token or quoted-string
+     * according to RFC-2616:
+     * <pre>
+     * token = 1*<any CHAR except CTLs or separators>
+     * separators = "(" | ")" | "<" | ">" | "@"
+     *                | "," | ";" | ":" | "\" | <">
+     *                | "/" | "[" | "]" | "?" | "="
+     *                | "{" | "}" | SP | HT
+     *
+     * quoted-string = ( <"> *(qdtext | quoted-pair ) <"> )
+     * qdtext = <any TEXT except <">>
+     * quoted-pair = "\" CHAR
+     *
+     * @param value The cookie value to quote
+     * @return The quoted cookie value
+     * @throws IllegalArgumentException If the cookie value is <code>null</code>
+     *             or cannot be quoted, primarily because it contains a quote
+     *             sign.
+     */
+    static String quoteCookieValue(final String value) {
+        // method is package private to enable unit testing
+
+        if (value == null) {
+            throw new IllegalArgumentException("Cookie value may not be null");
+        }
+
+        StringBuilder builder = new StringBuilder(value.length() * 2);
+        builder.append('"');
+        for (int i=0; i < value.length(); i++) {
+            char c = value.charAt(i);
+            if (c == '"') {
+                builder.append("\\\"");
+            } else if (c == 127 || (c < 32 && c != '\t')) {
+                throw new IllegalArgumentException(
+                    "Cookie value may not contain CTL character");
+            } else {
+                builder.append(c);
+            }
+        }
+        builder.append('"');
+
+        return builder.toString();
+    }
+
+    /**
+     * Removes (optional) quotes from a cookie value to get the raw value of
+     * the cookie.
+     *
+     * @param value The cookie value to unquote
+     * @return The unquoted cookie value
+     */
+    static String unquoteCookieValue(final String value) {
+        // method is package private to enable unit testing
+
+        // return value unmodified if null, empty or not starting with a quote
+        if (value == null || value.length() == 0 || value.charAt(0) != '"') {
+            return value;
+        }
+
+        StringBuilder builder = new StringBuilder(value.length());
+        for (int i=1; i < value.length()-1; i++) {
+            char c = value.charAt(i);
+            if (c != '\\') {
+                builder.append(c);
+            }
+        }
+
+        return builder.toString();
+    }
+
     private static class SlingAuthenticatorSession {
 
         static final String ATTR_NAME = "$$org.apache.sling.commons.auth.impl.SlingAuthenticatorSession$$";
@@ -831,7 +925,7 @@
                         session.logout();
                     }
                 } catch (Throwable t) {
-                    // TODO: log
+                    // TODO: might log
                 } finally {
                     session = null;
                 }

Added: sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/SlingAuthenticatorTest.java
URL: http://svn.apache.org/viewvc/sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/SlingAuthenticatorTest.java?rev=900728&view=auto
==============================================================================
--- sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/SlingAuthenticatorTest.java (added)
+++ sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/SlingAuthenticatorTest.java Tue Jan 19 11:25:09 2010
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.commons.auth.impl;
+
+import junit.framework.TestCase;
+
+public class SlingAuthenticatorTest extends TestCase {
+
+    public void test_quoteCookieValue() {
+
+        try {
+            SlingAuthenticator.quoteCookieValue(null);
+            fail("Expected IllegalArgumentExcepion on null value");
+        } catch (IllegalArgumentException iae) {
+            // expected
+        }
+
+        checkQuote("\"", "\"\\\"\"");
+        checkQuote("simplevalue", "\"simplevalue\"");
+        checkQuote("simple value", "\"simple value\"");
+        checkQuote("email@address.com", "\"email@address.com\"");
+
+        checkQuote("string\ttab", "\"string\ttab\"");
+
+        try {
+            SlingAuthenticator.quoteCookieValue("string\rCR");
+            fail("Expected IllegalArgumentExcepion on value containing CR");
+        } catch (IllegalArgumentException iae) {
+            // expected
+        }
+    }
+
+    public void test_unquoteCookieValue() {
+
+        assertNull(SlingAuthenticator.unquoteCookieValue(null));
+        assertEquals("", SlingAuthenticator.unquoteCookieValue(""));
+
+        checkUnQuote("unquoted", "unquoted");
+        checkUnQuote("unquoted\"", "unquoted\"");
+        checkUnQuote("un\"quoted", "un\"quoted");
+
+        checkUnQuote("\"\\\"\"", "\"");
+        checkUnQuote("\"simplevalue\"", "simplevalue");
+        checkUnQuote("\"simple value\"", "simple value");
+        checkUnQuote("\"email@address.com\"", "email@address.com");
+
+        checkUnQuote("\"string\ttab\"", "string\ttab");
+    }
+
+    private void checkQuote(final String value, final String expected) {
+        final String actual = SlingAuthenticator.quoteCookieValue(value);
+        assertEquals(expected, actual);
+    }
+
+    private void checkUnQuote(final String value, final String expected) {
+        final String actual = SlingAuthenticator.unquoteCookieValue(value);
+        assertEquals(expected, actual);
+    }
+}

Propchange: sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/SlingAuthenticatorTest.java
------------------------------------------------------------------------------
    svn:eol-style = native

Propchange: sling/trunk/bundles/commons/auth/src/test/java/org/apache/sling/commons/auth/impl/SlingAuthenticatorTest.java
------------------------------------------------------------------------------
    svn:keywords = Author Date Id Revision Rev Url