You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by kw...@apache.org on 2015/08/12 23:05:55 UTC

svn commit: r1695610 - in /qpid/java/trunk/broker-core/src: main/java/org/apache/qpid/server/logging/ main/java/org/apache/qpid/server/security/ test/java/org/apache/qpid/server/logging/ test/java/org/apache/qpid/server/security/

Author: kwall
Date: Wed Aug 12 21:05:55 2015
New Revision: 1695610

URL: http://svn.apache.org/r1695610
Log:
QPID-6691: [Java Broker] If AccessControlProvider is in ERRORED state we should default to DENY.

work by Lorenz Quack <qu...@gmail.com>

Modified:
    qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerFileLoggerImpl.java
    qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerMemoryLoggerImpl.java
    qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/VirtualHostFileLoggerImpl.java
    qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/logging/BrokerLoggerTest.java
    qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java

Modified: qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerFileLoggerImpl.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerFileLoggerImpl.java?rev=1695610&r1=1695609&r2=1695610&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerFileLoggerImpl.java (original)
+++ qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerFileLoggerImpl.java Wed Aug 12 21:05:55 2015
@@ -158,10 +158,7 @@ public class BrokerFileLoggerImpl extend
     @Override
     public Content getFile(final String fileName)
     {
-        if (!getSecurityManager().authoriseLogsAccess(this))
-        {
-            throw new AccessControlException("Permission denied to access log content");
-        }
+        getSecurityManager().authoriseLogsAccess(this);
 
         return _rolloverWatcher == null ? null : _rolloverWatcher.getFileContent(fileName);
     }
@@ -169,10 +166,7 @@ public class BrokerFileLoggerImpl extend
     @Override
     public Content getFiles(@Param(name = "fileName") Set<String> fileName)
     {
-        if (!getSecurityManager().authoriseLogsAccess(this))
-        {
-            throw new AccessControlException("Permission denied to access log content");
-        }
+        getSecurityManager().authoriseLogsAccess(this);
 
         return _rolloverWatcher == null ? null :_rolloverWatcher.getFilesAsZippedContent(fileName);
     }
@@ -180,10 +174,7 @@ public class BrokerFileLoggerImpl extend
     @Override
     public Content getAllFiles()
     {
-        if (!getSecurityManager().authoriseLogsAccess(this))
-        {
-            throw new AccessControlException("Permission denied to access log content");
-        }
+        getSecurityManager().authoriseLogsAccess(this);
 
         return _rolloverWatcher == null ? null : _rolloverWatcher.getAllFilesAsZippedContent();
     }

Modified: qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerMemoryLoggerImpl.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerMemoryLoggerImpl.java?rev=1695610&r1=1695609&r2=1695610&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerMemoryLoggerImpl.java (original)
+++ qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/BrokerMemoryLoggerImpl.java Wed Aug 12 21:05:55 2015
@@ -105,10 +105,7 @@ public class BrokerMemoryLoggerImpl exte
     @Override
     public Collection<LogRecord> getLogEntries(long lastLogId)
     {
-        if (!getSecurityManager().authoriseLogsAccess(this))
-        {
-            throw new AccessControlException("Access to log entries is denied");
-        }
+        getSecurityManager().authoriseLogsAccess(this);
 
         List<LogRecord> logRecords = new ArrayList<>();
         for(LogRecord record : _logRecorder)

Modified: qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/VirtualHostFileLoggerImpl.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/VirtualHostFileLoggerImpl.java?rev=1695610&r1=1695609&r2=1695610&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/VirtualHostFileLoggerImpl.java (original)
+++ qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/logging/VirtualHostFileLoggerImpl.java Wed Aug 12 21:05:55 2015
@@ -147,10 +147,7 @@ public class VirtualHostFileLoggerImpl e
     @Override
     public Content getFile(final String fileName)
     {
-        if (!getSecurityManager().authoriseLogsAccess(this))
-        {
-            throw new AccessControlException("Permission denied to access log content");
-        }
+        getSecurityManager().authoriseLogsAccess(this);
 
         return _rolloverWatcher == null ? null : _rolloverWatcher.getFileContent(fileName);
     }
@@ -158,10 +155,7 @@ public class VirtualHostFileLoggerImpl e
     @Override
     public Content getFiles(@Param(name = "fileName") Set<String> fileName)
     {
-        if (!getSecurityManager().authoriseLogsAccess(this))
-        {
-            throw new AccessControlException("Permission denied to access log content");
-        }
+        getSecurityManager().authoriseLogsAccess(this);
 
         return _rolloverWatcher == null ? null : _rolloverWatcher.getFilesAsZippedContent(fileName);
     }
@@ -170,10 +164,7 @@ public class VirtualHostFileLoggerImpl e
     @Override
     public Content getAllFiles()
     {
-        if (!getSecurityManager().authoriseLogsAccess(this))
-        {
-            throw new AccessControlException("Permission denied to access log content");
-        }
+        getSecurityManager().authoriseLogsAccess(this);
 
         return _rolloverWatcher == null ? null : _rolloverWatcher.getAllFilesAsZippedContent();
     }

Modified: qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java?rev=1695610&r1=1695609&r2=1695610&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java (original)
+++ qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java Wed Aug 12 21:05:55 2015
@@ -207,6 +207,11 @@ public class SecurityManager
                     return false;
                 }
             }
+            else
+            {
+                // Default to DENY when the accessControlProvider is in unhealthy state.
+                return false;
+            }
         }
         // getting here means either allowed or abstained from all plugins
         return true;
@@ -633,18 +638,21 @@ public class SecurityManager
         }
     }
 
-    public boolean authoriseLogsAccess(ConfiguredObject configuredObject)
+    public void authoriseLogsAccess(ConfiguredObject configuredObject)
     {
         Class<? extends ConfiguredObject> categoryClass = configuredObject.getCategoryClass();
         final ObjectType objectType = getACLObjectTypeManagingConfiguredObjectOfCategory(categoryClass);
         final ObjectProperties objectProperties = objectType == BROKER ? ObjectProperties.EMPTY : new ObjectProperties((String)configuredObject.getAttribute(ConfiguredObject.NAME));
-        return checkAllPlugins(new AccessCheck()
+        if(!checkAllPlugins(new AccessCheck()
         {
             Result allowed(AccessControl plugin)
             {
                 return plugin.authorise(ACCESS_LOGS, objectType, objectProperties);
             }
-        });
+        }))
+        {
+           throw new AccessControlException("Permission denied to access log content");
+        };
     }
 
     public static class PublishAccessCheckCacheEntry

Modified: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/logging/BrokerLoggerTest.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/logging/BrokerLoggerTest.java?rev=1695610&r1=1695609&r2=1695610&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/logging/BrokerLoggerTest.java (original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/logging/BrokerLoggerTest.java Wed Aug 12 21:05:55 2015
@@ -72,7 +72,6 @@ public class BrokerLoggerTest extends Qp
         Model model = BrokerModel.getInstance();
 
         SecurityManager securityManager = mock(SecurityManager.class);
-        when(securityManager.authoriseLogsAccess(any(ConfiguredObject.class))).thenReturn(true);
 
         _broker = mock(Broker.class);
         when(_broker.getSecurityManager()).thenReturn(securityManager);

Modified: qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java
URL: http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java?rev=1695610&r1=1695609&r2=1695610&view=diff
==============================================================================
--- qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java (original)
+++ qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java Wed Aug 12 21:05:55 2015
@@ -897,6 +897,28 @@ public class SecurityManagerTest extends
         assertDeleteAuthorization(mock, Operation.DELETE, ObjectType.VIRTUALHOST, properties, vhl);
     }
 
+    public void testDenyWhenAccessControlProviderIsErrored()
+    {
+        AccessControlProvider erroredACLProvider = mock(AccessControlProvider.class);
+        when(erroredACLProvider.getState()).thenReturn(State.ERRORED);
+        when(erroredACLProvider.getAccessControl()).thenReturn(_accessControl);
+        when(_broker.getAccessControlProviders()).thenReturn(Collections.singleton(erroredACLProvider));
+        when(_broker.getChildren(AccessControlProvider.class)).thenReturn(Collections.singleton(erroredACLProvider));
+
+        assertAuthorisationDenied();
+    }
+
+    public void testDenyWhenAccessControlProviderHasNoAccessControl()
+    {
+        AccessControlProvider erroredACLProvider = mock(AccessControlProvider.class);
+        when(erroredACLProvider.getState()).thenReturn(State.ACTIVE);
+        when(erroredACLProvider.getAccessControl()).thenReturn(null);
+        when(_broker.getAccessControlProviders()).thenReturn(Collections.singleton(erroredACLProvider));
+        when(_broker.getChildren(AccessControlProvider.class)).thenReturn(Collections.singleton(erroredACLProvider));
+
+        assertAuthorisationDenied();
+    }
+
     private VirtualHost getMockVirtualHost()
     {
         VirtualHost vh = mock(VirtualHost.class);
@@ -1046,27 +1068,152 @@ public class SecurityManagerTest extends
         verify(_accessControl, times(2)).authorise(eq(aclOperation), eq(aclObjectType), eq(expectedProperties));
     }
 
+    private void assertAuthorisationDenied()
+    {
+        ConfiguredObject mockConfiguredObject = mock(BrokerLogger.class);
+        when(mockConfiguredObject.getCategoryClass()).thenReturn(BrokerLogger.class);
+
+        try
+        {
+            _securityManager.authorise(Operation.UPDATE, mockConfiguredObject);
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.authoriseCreate(mockConfiguredObject);
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.authoriseDelete(mockConfiguredObject);
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.authoriseLogsAccess(mockConfiguredObject);
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.authoriseUserUpdate("guest");
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.authoriseCreateConnection(mock(AMQPConnection.class));
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.authorisePublish(true, TEST_QUEUE, TEST_EXCHANGE, TEST_VIRTUAL_HOST);
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            Queue mockQueue = mock(Queue.class);
+            when(mockQueue.getParent(VirtualHost.class)).thenReturn(_virtualHost);
+            _securityManager.authorisePurge(mockQueue);
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.accessManagement();
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            _securityManager.authoriseMethod(Operation.UPDATE, "testComponent", "testMethod", TEST_VIRTUAL_HOST);
+            fail("AccessControlException is expected");
+        }
+        catch(AccessControlException e)
+        {
+            // pass
+        }
+
+    }
+
     public void testAuthoriseLogsAccessOnBroker()
     {
         configureAccessPlugin(Result.ALLOWED);
-        assertTrue(_securityManager.authoriseLogsAccess(_broker));
+        _securityManager.authoriseLogsAccess(_broker);
 
         verify(_accessControl).authorise(ACCESS_LOGS, BROKER, ObjectProperties.EMPTY);
 
         configureAccessPlugin(Result.DENIED);
-        assertFalse(_securityManager.authoriseLogsAccess(_broker));
+        try
+        {
+            _securityManager.authoriseLogsAccess(_broker);
+            fail("AccessControlException is expected");
+        }
+        catch (AccessControlException e)
+        {
+            // pass
+        }
         verify(_accessControl, times(2)).authorise(ACCESS_LOGS, BROKER, ObjectProperties.EMPTY);
     }
 
     public void testAuthoriseLogsAccessOnVirtualHost()
     {
         configureAccessPlugin(Result.ALLOWED);
-        assertTrue(_securityManager.authoriseLogsAccess(_virtualHost));
+        _securityManager.authoriseLogsAccess(_virtualHost);
         ObjectProperties expectedObjectProperties = new ObjectProperties((String)_virtualHost.getAttribute(ConfiguredObject.NAME));
         verify(_accessControl).authorise(ACCESS_LOGS, VIRTUALHOST, expectedObjectProperties);
 
         configureAccessPlugin(Result.DENIED);
-        assertFalse(_securityManager.authoriseLogsAccess(_virtualHost));
+        try
+        {
+            _securityManager.authoriseLogsAccess(_virtualHost);
+            fail("AccessControlException is expected");
+        }
+        catch (AccessControlException e)
+        {
+            // pass
+        }
         verify(_accessControl, times(2)).authorise(ACCESS_LOGS, VIRTUALHOST, expectedObjectProperties);
     }
 
@@ -1099,6 +1246,4 @@ public class SecurityManagerTest extends
         return properties;
     }
 
-
-
 }



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org