QPID-4292: Java Web Management - standardise of the use of SC_FORBIDDEN and avoid ugly stack trace in logs in response to some authorisation failures

Work of Robbie Gemmell <robbie@apache.org> and myself.

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

9 files changed, 25 insertions, 25 deletions

diff --git a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/DefinedFileServlet.java b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/DefinedFileServlet.java
index d8a8395550..e6ae47dcff 100644
--- a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/DefinedFileServlet.java
+++ b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/DefinedFileServlet.java
@@ -73,7 +73,7 @@ public class DefinedFileServlet extends HttpServlet
         }
         else
         {
-            response.sendError(404, "unknown file: "+ _filename);
+            response.sendError(HttpServletResponse.SC_NOT_FOUND, "unknown file: "+ _filename);
         }
     }
 }
diff --git a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/FileServlet.java b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/FileServlet.java
index f8ca082d79..24e5e7c049 100644
--- a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/FileServlet.java
+++ b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/FileServlet.java
@@ -20,11 +20,8 @@
  */
 package org.apache.qpid.server.management.plugin.servlet;
 
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
-import java.net.URI;
-import java.net.URISyntaxException;
 import java.net.URL;
 import java.util.Collections;
 import java.util.HashMap;
@@ -101,7 +98,7 @@ public class FileServlet extends HttpServlet
         }
         else
         {
-            response.sendError(404, "unknown file: "+ filename);
+            response.sendError(HttpServletResponse.SC_NOT_FOUND, "unknown file: "+ filename);
         }
 
     }
diff --git a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java
index 3920443b07..4bbb43be70 100644
--- a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java
+++ b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/MessageServlet.java
@@ -435,7 +435,7 @@ public class MessageServlet extends AbstractServlet
             }
             else
             {
-                response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+                response.setStatus(HttpServletResponse.SC_FORBIDDEN);
             }
         }
         catch(RuntimeException e)
@@ -473,7 +473,7 @@ public class MessageServlet extends AbstractServlet
         }
         else
         {
-            response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+            response.setStatus(HttpServletResponse.SC_FORBIDDEN);
         }
 
     }
diff --git a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
index f2ca25d664..203fa66ff9 100644
--- a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
+++ b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
@@ -19,6 +19,7 @@ package org.apache.qpid.server.management.plugin.servlet.rest;
 import java.io.BufferedWriter;
 import java.io.IOException;
 import java.io.Writer;
+import java.security.AccessControlException;
 import java.util.*;
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletException;
@@ -465,10 +466,13 @@ public class RestServlet extends AbstractServlet
 
     private void setResponseStatus(HttpServletResponse response, RuntimeException e) throws IOException
     {
-        if (e.getCause() instanceof AMQSecurityException)
+        if (e instanceof AccessControlException || e.getCause() instanceof AMQSecurityException)
         {
-            LOGGER.debug("Caught AMQSecurityException", e);
-            response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+            if (LOGGER.isDebugEnabled())
+            {
+                LOGGER.debug("Caught security exception, sending " + HttpServletResponse.SC_FORBIDDEN, e);
+            }
+            response.setStatus(HttpServletResponse.SC_FORBIDDEN);
         }
         else
         {
diff --git a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
index 0a035006c7..df77f9dc5d 100644
--- a/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
+++ b/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
@@ -228,7 +228,7 @@ public class SaslServlet extends AbstractServlet
             session.removeAttribute(ATTR_ID);
             session.removeAttribute(ATTR_SASL_SERVER);
             session.removeAttribute(ATTR_EXPIRY);
-            response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+            response.setStatus(HttpServletResponse.SC_FORBIDDEN);
 
             return;
         }
diff --git a/java/broker-plugins/management-http/src/main/java/resources/js/qpid/authorization/sasl.js b/java/broker-plugins/management-http/src/main/java/resources/js/qpid/authorization/sasl.js
index f003b896eb..98313c6798 100644
--- a/java/broker-plugins/management-http/src/main/java/resources/js/qpid/authorization/sasl.js
+++ b/java/broker-plugins/management-http/src/main/java/resources/js/qpid/authorization/sasl.js
@@ -71,7 +71,7 @@ var saslPlain = function saslPlain(user, password)
             },
             function(error)
             {
-                if(error.status == 401)
+                if(error.status == 403)
                 {
                     alert("Authentication Failed");
                 }
@@ -127,7 +127,7 @@ var saslCramMD5 = function saslCramMD5(user, password)
                                         },
                                         function(error)
                                         {
-                                            if(error.status == 401)
+                                            if(error.status == 403)
                                             {
                                                 alert("Authentication Failed");
                                             }
@@ -141,7 +141,7 @@ var saslCramMD5 = function saslCramMD5(user, password)
             },
             function(error)
             {
-                if(error.status == 401)
+                if(error.status == 403)
                 {
                     alert("Authentication Failed");
                 }
diff --git a/java/broker-plugins/management-http/src/test/java/org/apache/qpid/server/management/plugin/servlet/rest/VirtualHostRestTest.java b/java/broker-plugins/management-http/src/test/java/org/apache/qpid/server/management/plugin/servlet/rest/VirtualHostRestTest.java
index 9e4431f92d..cafba7c62a 100644
--- a/java/broker-plugins/management-http/src/test/java/org/apache/qpid/server/management/plugin/servlet/rest/VirtualHostRestTest.java
+++ b/java/broker-plugins/management-http/src/test/java/org/apache/qpid/server/management/plugin/servlet/rest/VirtualHostRestTest.java
@@ -27,6 +27,7 @@ import java.util.List;
 import java.util.Map;
 
 import javax.jms.Session;
+import javax.servlet.http.HttpServletResponse;
 
 import org.apache.qpid.client.AMQConnection;
 import org.apache.qpid.server.model.Exchange;
@@ -189,7 +190,7 @@ public class VirtualHostRestTest extends QpidRestTestCase
     {
         String queueName = getTestQueueName() + "-sorted";
         int responseCode = tryCreateQueue(queueName, "sorted", null);
-        assertEquals("Unexpected response code", 409, responseCode);
+        assertEquals("Unexpected response code", HttpServletResponse.SC_CONFLICT, responseCode);
 
         Map<String, Object> hostDetails = getRestTestHelper().getJsonAsSingletonList("/rest/virtualhost/test");
 
@@ -234,7 +235,7 @@ public class VirtualHostRestTest extends QpidRestTestCase
     {
         String queueName = getTestQueueName();
         int responseCode = tryCreateQueue(queueName, "unsupported", null);
-        assertEquals("Unexpected response code", 409, responseCode);
+        assertEquals("Unexpected response code", HttpServletResponse.SC_CONFLICT, responseCode);
 
         Map<String, Object> hostDetails = getRestTestHelper().getJsonAsSingletonList("/rest/virtualhost/test");
 
diff --git a/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java b/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java
index f85fd02199..df93b905ab 100644
--- a/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java
+++ b/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java
@@ -107,8 +107,7 @@ public class GroupRestACLTest extends QpidRestTestCase
 
         getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
 
-        //TODO: the expected response code needs changed when we overhaul the brokers error handling
-        getRestTestHelper().createGroup("anotherNewGroup", FILE_GROUP_MANAGER, HttpServletResponse.SC_CONFLICT);
+        getRestTestHelper().createGroup("anotherNewGroup", FILE_GROUP_MANAGER, HttpServletResponse.SC_FORBIDDEN);
 
         data = getRestTestHelper().getJsonAsSingletonList("/rest/groupprovider/" + FILE_GROUP_MANAGER);
         getRestTestHelper().assertNumberOfGroups(data, 4);
@@ -128,8 +127,7 @@ public class GroupRestACLTest extends QpidRestTestCase
         Map<String, Object> data = getRestTestHelper().getJsonAsSingletonList("/rest/groupprovider/" + FILE_GROUP_MANAGER);
         getRestTestHelper().assertNumberOfGroups(data, 3);
 
-        //TODO: the expected response code needs changed when we overhaul the brokers error handling
-        getRestTestHelper().removeGroup(OTHER_GROUP, FILE_GROUP_MANAGER, HttpServletResponse.SC_CONFLICT);
+        getRestTestHelper().removeGroup(OTHER_GROUP, FILE_GROUP_MANAGER, HttpServletResponse.SC_FORBIDDEN);
 
         data = getRestTestHelper().getJsonAsSingletonList("/rest/groupprovider/" + FILE_GROUP_MANAGER);
         getRestTestHelper().assertNumberOfGroups(data, 3);
@@ -155,7 +153,7 @@ public class GroupRestACLTest extends QpidRestTestCase
 
         assertNumberOfGroupMembers(OTHER_GROUP, 1);
 
-        getRestTestHelper().createNewGroupMember(FILE_GROUP_MANAGER, OTHER_GROUP, "newGroupMember", HttpServletResponse.SC_CONFLICT);
+        getRestTestHelper().createNewGroupMember(FILE_GROUP_MANAGER, OTHER_GROUP, "newGroupMember", HttpServletResponse.SC_FORBIDDEN);
         assertNumberOfGroupMembers(OTHER_GROUP, 1);
 
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
@@ -176,7 +174,7 @@ public class GroupRestACLTest extends QpidRestTestCase
 
         assertNumberOfGroupMembers(OTHER_GROUP, 1);
 
-        getRestTestHelper().removeMemberFromGroup(FILE_GROUP_MANAGER, OTHER_GROUP, OTHER_USER, HttpServletResponse.SC_CONFLICT);
+        getRestTestHelper().removeMemberFromGroup(FILE_GROUP_MANAGER, OTHER_GROUP, OTHER_USER, HttpServletResponse.SC_FORBIDDEN);
         assertNumberOfGroupMembers(OTHER_GROUP, 1);
 
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
diff --git a/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java b/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java
index 09c82b9205..88128e6a1c 100644
--- a/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java
+++ b/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java
@@ -105,7 +105,7 @@ public class UserRestACLTest extends QpidRestTestCase
 
         getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
 
-        getRestTestHelper().createOrUpdateUser(newUser, password, HttpServletResponse.SC_CONFLICT);
+        getRestTestHelper().createOrUpdateUser(newUser, password, HttpServletResponse.SC_FORBIDDEN);
         assertUserDoesNotExist(newUser);
 
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
@@ -126,7 +126,7 @@ public class UserRestACLTest extends QpidRestTestCase
         assertUserExists(OTHER_USER);
 
         getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        getRestTestHelper().removeUser(OTHER_USER, HttpServletResponse.SC_CONFLICT);
+        getRestTestHelper().removeUser(OTHER_USER, HttpServletResponse.SC_FORBIDDEN);
         assertUserExists(OTHER_USER);
 
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
@@ -149,7 +149,7 @@ public class UserRestACLTest extends QpidRestTestCase
         checkPassword(OTHER_USER, OTHER_USER, true);
 
         getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        getRestTestHelper().createOrUpdateUser(OTHER_USER, newPassword, HttpServletResponse.SC_CONFLICT);
+        getRestTestHelper().createOrUpdateUser(OTHER_USER, newPassword, HttpServletResponse.SC_FORBIDDEN);
 
         checkPassword(OTHER_USER, newPassword, false);