You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by mi...@apache.org on 2019/08/22 12:34:55 UTC

[tomcat] 01/01: BZ 63684: Wrapper never passed to RealmBase#hasRole() for given security constraints

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

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

commit 8a54c7f00de0caef5d0c18feaab3cd20e7a0826f
Author: Michael Osipov <mi...@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                         |  9 +++++++
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/java/org/apache/catalina/realm/RealmBase.java b/java/org/apache/catalina/realm/RealmBase.java
index dd1761c..d321c56 100644
--- a/java/org/apache/catalina/realm/RealmBase.java
+++ b/java/org/apache/catalina/realm/RealmBase.java
@@ -858,7 +858,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 38f8822..bd2a7aa 100644
--- a/java/org/apache/catalina/realm/UserDatabaseRealm.java
+++ b/java/org/apache/catalina/realm/UserDatabaseRealm.java
@@ -117,6 +117,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..a169b77 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 6fed418..c1478e7 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -45,6 +45,15 @@
   issues do not "pop up" wrt. others).
 -->
 <section name="Tomcat 8.5.46 (markt)" rtext="in development">
+  <subsection name="Catalina">
+    <changelog>
+      <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">
     <changelog>
       <fix>


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