This is an automated email from the ASF dual-hosted git repository.

michaelo pushed a commit to branch BZ-63684/9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git

commit d5f3c97a05e050a1b38b801e9545b92d97c93407
Author: Michael Osipov <micha...@apache.org>
AuthorDate: Thu Aug 22 14:34:31 2019 +0200

    BZ 63684: Wrapper never passed to RealmBase#hasRole() for given security 
constraints
---
 java/org/apache/catalina/realm/RealmBase.java      |  2 +-
 .../apache/catalina/realm/UserDatabaseRealm.java   |  2 ++
 .../apache/catalina/core/TestStandardWrapper.java  | 31 +++++++++++++++++-----
 webapps/docs/changelog.xml                         |  5 ++++
 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/java/org/apache/catalina/realm/RealmBase.java 
b/java/org/apache/catalina/realm/RealmBase.java
index 833973a..aa542a7 100644
--- a/java/org/apache/catalina/realm/RealmBase.java
+++ b/java/org/apache/catalina/realm/RealmBase.java
@@ -856,7 +856,7 @@ public abstract class RealmBase extends LifecycleMBeanBase 
implements Realm {
                     log.debug("  No user authenticated, cannot grant access");
             } else {
                 for (int j = 0; j < roles.length; j++) {
-                    if (hasRole(null, principal, roles[j])) {
+                    if (hasRole(request.getWrapper(), principal, roles[j])) {
                         status = true;
                         if( log.isDebugEnabled() )
                             log.debug( "Role found:  " + roles[j]);
diff --git a/java/org/apache/catalina/realm/UserDatabaseRealm.java 
b/java/org/apache/catalina/realm/UserDatabaseRealm.java
index a552fc4..64957a9 100644
--- a/java/org/apache/catalina/realm/UserDatabaseRealm.java
+++ b/java/org/apache/catalina/realm/UserDatabaseRealm.java
@@ -108,6 +108,8 @@ public class UserDatabaseRealm extends RealmBase {
         }
         if (!(principal instanceof User)) {
             // Play nice with SSO and mixed Realms
+            // No need to pass the wrapper here because role mapping has been
+            // performed already a few lines above
             return super.hasRole(null, principal, role);
         }
         if ("*".equals(role)) {
diff --git a/test/org/apache/catalina/core/TestStandardWrapper.java 
b/test/org/apache/catalina/core/TestStandardWrapper.java
index 9358345..fbd0046 100644
--- a/test/org/apache/catalina/core/TestStandardWrapper.java
+++ b/test/org/apache/catalina/core/TestStandardWrapper.java
@@ -259,14 +259,14 @@ public class TestStandardWrapper extends TomcatBaseTest {
 
         // No file system docBase required
         Context ctx = tomcat.addContext("", null);
-        ctx.addRoleMapping("testRole2", "very-complex-role-name");
-        /* We won't map "testRole3" to "another-very-complex-role-name" to make
-         * it fail intentionally.
-         */
+        ctx.addRoleMapping("testRole", "very-complex-role-name");
 
-        Wrapper wrapper = Tomcat.addServlet(ctx, "servlet", 
TestServlet.class.getName());
+        Wrapper wrapper = Tomcat.addServlet(ctx, "servlet", 
RoleAllowServlet.class.getName());
         ctx.addServletMappingDecoded("/", "servlet");
 
+        ctx.setLoginConfig(new LoginConfig("BASIC", null, null, null));
+        ctx.getPipeline().addValve(new BasicAuthenticator());
+
         TesterMapRealm realm = new TesterMapRealm();
         MessageDigestCredentialHandler ch = new 
MessageDigestCredentialHandler();
         ch.setAlgorithm("SHA");
@@ -296,10 +296,27 @@ public class TestStandardWrapper extends TomcatBaseTest {
 
         Assert.assertNotNull(p);
         Assert.assertEquals("testUser", p.getName());
+        // This one is mapped
+        Assert.assertTrue(realm.hasRole(wrapper, p, "testRole"));
         Assert.assertTrue(realm.hasRole(wrapper, p, "testRole1"));
-        Assert.assertTrue(realm.hasRole(wrapper, p, "testRole2"));
+        Assert.assertFalse(realm.hasRole(wrapper, p, "testRole2"));
         Assert.assertTrue(realm.hasRole(wrapper, p, "very-complex-role-name"));
-        Assert.assertFalse(realm.hasRole(wrapper, p, "testRole3"));
+        Assert.assertTrue(realm.hasRole(wrapper, p, 
"another-very-complex-role-name"));
+
+        // This now tests RealmBase#hasResourcePermission() because we need a 
wrapper
+        // to be passed from an authenticator
+        ByteChunk bc = new ByteChunk();
+        Map<String, List<String>> reqHeaders = new HashMap<>();
+        List<String> authHeaders = new ArrayList<>();
+        // testUser, testPwd
+        authHeaders.add("Basic dGVzdFVzZXI6dGVzdFB3ZA==");
+        reqHeaders.put("Authorization", authHeaders);
+
+        int rc = getUrl("http://localhost:"; + getPort() + "/", bc, reqHeaders,
+                null);
+
+        Assert.assertEquals("OK", bc.toString());
+        Assert.assertEquals(200, rc);
     }
 
     private void doTestSecurityAnnotationsAddServlet(boolean useCreateServlet)
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 64cf807..d2abd52 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -51,6 +51,11 @@
         Avoid a possible <code>InvalidPathException</code> when obtaining a URI
         for a configuration file. (markt)
       </fix>
+      <fix>
+        <bug>63684</bug>: <code>Wrapper</code> never passed to
+        <code>RealmBase.hasRole()</code> for given security constraints.
+        (michaelo)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Web applications">


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to