QPID-5562 : [Java Broker] make all failed ACL checks throw AccessControlException

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1569552 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 53 insertions, 39 deletions

diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/HttpManagementUtil.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/HttpManagementUtil.java
index 9a2f0dd1f6..674ff71232 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/HttpManagementUtil.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/HttpManagementUtil.java
@@ -48,7 +48,6 @@ import org.apache.qpid.server.security.auth.AuthenticatedPrincipal;
 import org.apache.qpid.server.security.auth.AuthenticationResult.AuthenticationStatus;
 import org.apache.qpid.server.security.auth.SubjectAuthenticationResult;
 import org.apache.qpid.server.security.auth.UsernamePrincipal;
-import org.apache.qpid.server.security.auth.manager.ExternalAuthenticationManager;
 import org.apache.qpid.server.security.auth.manager.ExternalAuthenticationManagerFactory;
 import org.apache.qpid.server.util.ServerScopedRuntimeException;
 import org.apache.qpid.transport.network.security.ssl.SSLUtil;
@@ -108,18 +107,16 @@ public class HttpManagementUtil
                 throw new SecurityException("Only authenticated users can access the management interface");
             }
             LogActor actor = createHttpManagementActor(broker, request);
-            if (hasAccessToManagement(broker.getSecurityManager(), subject, actor))
-            {
-                saveAuthorisedSubject(session, subject, actor);
-            }
-            else
-            {
-                throw new AccessControlException("Access to the management interface denied");
-            }
+
+            assertManagementAccess(broker.getSecurityManager(), subject, actor);
+
+            saveAuthorisedSubject(session, subject, actor);
+
+
         }
     }
 
-    public static boolean hasAccessToManagement(final SecurityManager securityManager, Subject subject, LogActor actor)
+    public static void assertManagementAccess(final SecurityManager securityManager, Subject subject, LogActor actor)
     {
         // TODO: We should eliminate SecurityManager.setThreadSubject in favour of Subject.doAs
         SecurityManager.setThreadSubject(subject); // Required for accessManagement check
@@ -128,12 +125,13 @@ public class HttpManagementUtil
         {
             try
             {
-                return Subject.doAs(subject, new PrivilegedExceptionAction<Boolean>()
+                Subject.doAs(subject, new PrivilegedExceptionAction<Void>()
                 {
                     @Override
-                    public Boolean run() throws Exception
+                    public Void run()
                     {
-                        return securityManager.accessManagement();
+                        securityManager.accessManagement();
+                        return null;
                     }
                 });
             }
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java
index 9ca23ce1ce..3eafa7c294 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java
@@ -19,6 +19,7 @@ package org.apache.qpid.server.management.plugin.servlet.rest;
 
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.security.AccessControlException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
@@ -426,21 +427,22 @@ public class MessageServlet extends AbstractServlet
             // FIXME: added temporary authorization check until we introduce management layer
             // and review current ACL rules to have common rules for all management interfaces
             String methodName = isMoveTransaction? "moveMessages":"copyMessages";
-            if (isQueueUpdateMethodAuthorized(methodName, vhost))
-            {
-                final Queue destinationQueue = getQueueFromVirtualHost(destQueueName, vhost);
-                final List messageIds = new ArrayList((List) providedObject.get("messages"));
-                QueueEntryTransaction txn =
-                        isMoveTransaction
-                                ? new MoveTransaction(sourceQueue, messageIds, destinationQueue)
-                                : new CopyTransaction(sourceQueue, messageIds, destinationQueue);
-                vhost.executeTransaction(txn);
-                response.setStatus(HttpServletResponse.SC_OK);
-            }
-            else
-            {
-                response.setStatus(HttpServletResponse.SC_FORBIDDEN);
-            }
+            authorizeMethod(methodName, vhost);
+
+
+            final Queue destinationQueue = getQueueFromVirtualHost(destQueueName, vhost);
+            final List messageIds = new ArrayList((List) providedObject.get("messages"));
+            QueueEntryTransaction txn =
+                    isMoveTransaction
+                            ? new MoveTransaction(sourceQueue, messageIds, destinationQueue)
+                            : new CopyTransaction(sourceQueue, messageIds, destinationQueue);
+            vhost.executeTransaction(txn);
+            response.setStatus(HttpServletResponse.SC_OK);
+
+        }
+        catch(AccessControlException e)
+        {
+            response.setStatus(HttpServletResponse.SC_FORBIDDEN);
         }
         catch(RuntimeException e)
         {
@@ -470,22 +472,23 @@ public class MessageServlet extends AbstractServlet
 
         // FIXME: added temporary authorization check until we introduce management layer
         // and review current ACL rules to have common rules for all management interfaces
-        if (isQueueUpdateMethodAuthorized("deleteMessages", vhost))
+        try
         {
+            authorizeMethod("deleteMessages", vhost);
             vhost.executeTransaction(new DeleteTransaction(sourceQueue, messageIds));
             response.setStatus(HttpServletResponse.SC_OK);
         }
-        else
+        catch (AccessControlException e)
         {
             response.setStatus(HttpServletResponse.SC_FORBIDDEN);
         }
 
     }
 
-    private boolean isQueueUpdateMethodAuthorized(String methodName, VirtualHost host)
+    private void authorizeMethod(String methodName, VirtualHost host)
     {
         SecurityManager securityManager = host.getSecurityManager();
-        return securityManager.authoriseMethod(Operation.UPDATE, "VirtualHost.Queue", methodName);
+        securityManager.authoriseMethod(Operation.UPDATE, "VirtualHost.Queue", methodName);
     }
 
 }
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
index 45e0c2dab8..e6bc46aa77 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
@@ -27,7 +27,6 @@ import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.log4j.Logger;
-import org.apache.qpid.server.security.QpidSecurityException;
 import org.apache.qpid.server.model.*;
 import org.codehaus.jackson.map.ObjectMapper;
 import org.codehaus.jackson.map.SerializationConfig;
@@ -498,7 +497,7 @@ public class RestServlet extends AbstractServlet
 
     private void setResponseStatus(HttpServletResponse response, RuntimeException e) throws IOException
     {
-        if (e instanceof AccessControlException || e.getCause() instanceof QpidSecurityException)
+        if (e instanceof AccessControlException)
         {
             if (LOGGER.isDebugEnabled())
             {
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
index a29a875071..5441dc95c4 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
@@ -241,7 +241,11 @@ public class SaslServlet extends AbstractServlet
 
             Broker broker = getBroker();
             LogActor actor = HttpManagementUtil.getOrCreateAndCacheLogActor(request, broker);
-            if (!HttpManagementUtil.hasAccessToManagement(broker.getSecurityManager(), subject, actor))
+            try
+            {
+                HttpManagementUtil.assertManagementAccess(broker.getSecurityManager(), subject, actor);
+            }
+            catch(SecurityException e)
             {
                 sendError(response, HttpServletResponse.SC_FORBIDDEN);
                 return;
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/UserPreferencesServlet.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/UserPreferencesServlet.java
index 355b5df177..01657b131d 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/UserPreferencesServlet.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/UserPreferencesServlet.java
@@ -64,11 +64,16 @@ public class UserPreferencesServlet extends AbstractServlet
     private void getUserPreferences(String authenticationProviderName, String userId, HttpServletResponse response)
             throws IOException
     {
-        if (!userPreferencesOperationAuthorized(userId))
+        try
+        {
+            assertUserPreferencesOperationAuthorized(userId);
+        }
+        catch (SecurityException e)
         {
             response.sendError(HttpServletResponse.SC_FORBIDDEN, "Viewing of preferences is not allowed");
             return;
         }
+
         Map<String, Object> preferences = null;
         PreferencesProvider preferencesProvider = getPreferencesProvider(authenticationProviderName);
         if (preferencesProvider == null)
@@ -171,11 +176,16 @@ public class UserPreferencesServlet extends AbstractServlet
 
             String userId = elements[1];
 
-            if (!userPreferencesOperationAuthorized(userId))
+            try
+            {
+                assertUserPreferencesOperationAuthorized(userId);
+            }
+            catch (SecurityException e)
             {
                 response.sendError(HttpServletResponse.SC_FORBIDDEN, "Deletion of preferences is not allowed");
                 return;
             }
+
             String providerName =  elements[0];
             Set<String> users = providerUsers.get(providerName);
 
@@ -226,8 +236,8 @@ public class UserPreferencesServlet extends AbstractServlet
         return provider;
     }
 
-    private boolean userPreferencesOperationAuthorized(String userId)
+    private void assertUserPreferencesOperationAuthorized(String userId)
     {
-        return getBroker().getSecurityManager().authoriseUserOperation(Operation.UPDATE, userId);
+        getBroker().getSecurityManager().authoriseUserOperation(Operation.UPDATE, userId);
     }
 }