diff options
Diffstat (limited to 'java')
2 files changed, 280 insertions, 117 deletions
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java b/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java index e127e7a9d4..121f571abe 100644 --- a/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java +++ b/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java @@ -64,9 +64,9 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana private static final Logger _logger = Logger.getLogger(AMQUserManagementMBean.class); private PrincipalDatabase _principalDatabase; - private String _accessFileName; private Properties _accessRights; - // private File _accessFile; + private File _accessFile; + private ReentrantLock _accessRightsUpdate = new ReentrantLock(); // Setup for the TabularType @@ -129,9 +129,10 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana public boolean setRights(String username, boolean read, boolean write, boolean admin) { - if (_accessRights.get(username) == null) + Object oldRights = null; + if ((oldRights =_accessRights.get(username)) == null) { - // If the user doesn't exist in the user rights file check that they at least have an account. + // If the user doesn't exist in the access rights file check that they at least have an account. if (_principalDatabase.getUser(username) == null) { return false; @@ -140,7 +141,6 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana try { - _accessRightsUpdate.lock(); // Update the access rights @@ -166,8 +166,29 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana _accessRights.remove(username); } } + + //save the rights file + try + { + saveAccessFile(); + } + catch (IOException e) + { + _logger.warn("Problem occured saving '" + _accessFile + "', the access right changes will not be preserved: " + e); - saveAccessFile(); + //the rights file was not successfully saved, restore user rights to previous value + _logger.warn("Reverting attempted rights update for user'" + username + "'"); + if (oldRights != null) + { + _accessRights.put(username, oldRights); + } + else + { + _accessRights.remove(username); + } + + return false; + } } finally { @@ -184,9 +205,23 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana { if (_principalDatabase.createPrincipal(new UsernamePrincipal(username), password)) { - _accessRights.put(username, ""); - - return setRights(username, read, write, admin); + if (!setRights(username, read, write, admin)) + { + //unable to set rights for user, remove account + try + { + _principalDatabase.deletePrincipal(new UsernamePrincipal(username)); + } + catch (AccountNotFoundException e) + { + //ignore + } + return false; + } + else + { + return true; + } } return false; @@ -194,7 +229,6 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana public boolean deleteUser(String username) { - try { if (_principalDatabase.deletePrincipal(new UsernamePrincipal(username))) @@ -204,7 +238,16 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana _accessRightsUpdate.lock(); _accessRights.remove(username); - saveAccessFile(); + + try + { + saveAccessFile(); + } + catch (IOException e) + { + _logger.warn("Problem occured saving '" + _accessFile + "', the access right changes will not be preserved: " + e); + return false; + } } finally { @@ -213,15 +256,15 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana _accessRightsUpdate.unlock(); } } - return true; } } catch (AccountNotFoundException e) { _logger.warn("Attempt to delete user (" + username + ") that doesn't exist"); + return false; } - return false; + return true; } public boolean reloadData() @@ -233,12 +276,12 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana } catch (ConfigurationException e) { - _logger.info("Reload failed due to:" + e); + _logger.warn("Reload failed due to:" + e); return false; } catch (IOException e) { - _logger.info("Reload failed due to:" + e); + _logger.warn("Reload failed due to:" + e); return false; } // Reload successful @@ -320,10 +363,24 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana */ public void setAccessFile(String accessFile) throws IOException, ConfigurationException { - _accessFileName = accessFile; - - if (_accessFileName != null) + if (accessFile != null) { + _accessFile = new File(accessFile); + if (!_accessFile.exists()) + { + throw new ConfigurationException("'" + _accessFile + "' does not exist"); + } + + if (!_accessFile.canRead()) + { + throw new ConfigurationException("Cannot read '" + _accessFile + "'."); + } + + if (!_accessFile.canWrite()) + { + _logger.warn("Unable to write to access rights file '" + _accessFile + "', changes will not be preserved."); + } + loadAccessFile(); } else @@ -334,39 +391,34 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana private void loadAccessFile() throws IOException, ConfigurationException { - try + if(_accessFile == null) { - _accessRightsUpdate.lock(); - - Properties accessRights = new Properties(); - - File accessFile = new File(_accessFileName); - - if (!accessFile.exists()) + _logger.error("No jmx access rights file has been specified."); + return; + } + + if(_accessFile.exists()) + { + try { - throw new ConfigurationException("'" + _accessFileName + "' does not exist"); - } + _accessRightsUpdate.lock(); - if (!accessFile.canRead()) - { - throw new ConfigurationException("Cannot read '" + _accessFileName + "'."); + Properties accessRights = new Properties(); + accessRights.load(new FileInputStream(_accessFile)); + checkAccessRights(accessRights); + setAccessRights(accessRights); } - - if (!accessFile.canWrite()) + finally { - _logger.warn("Unable to write to access file '" + _accessFileName + "' changes will not be preserved."); + if (_accessRightsUpdate.isHeldByCurrentThread()) + { + _accessRightsUpdate.unlock(); + } } - - accessRights.load(new FileInputStream(accessFile)); - checkAccessRights(accessRights); - setAccessRights(accessRights); } - finally + else { - if (_accessRightsUpdate.isHeldByCurrentThread()) - { - _accessRightsUpdate.unlock(); - } + _logger.error("Specified jmxaccess rights file '" + _accessFile + "' does not exist."); } } @@ -385,33 +437,24 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana } } - private void saveAccessFile() + private void saveAccessFile() throws IOException { try { _accessRightsUpdate.lock(); - try - { - // Create temporary file - File tmp = File.createTempFile(_accessFileName, ".tmp"); - // Rename current file - File rights = new File(_accessFileName); + // Create temporary file + File tmp = File.createTempFile(_accessFile.getName(), ".tmp"); - FileOutputStream output = new FileOutputStream(tmp); - _accessRights.store(output, "Generated by AMQUserManagementMBean Console : Last edited by user:" + getCurrentJMXUser()); - output.close(); + FileOutputStream output = new FileOutputStream(tmp); + _accessRights.store(output, "Generated by AMQUserManagementMBean Console : Last edited by user:" + getCurrentJMXUser()); + output.close(); - // Rename new file to main file - tmp.renameTo(rights); + // Rename new file to main file + tmp.renameTo(_accessFile); - // delete tmp - tmp.delete(); - } - catch (IOException e) - { - _logger.warn("Problem occured saving '" + _accessFileName + "' changes may not be preserved. :" + e); - } + // delete tmp + tmp.delete(); } finally { @@ -420,6 +463,7 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana _accessRightsUpdate.unlock(); } } + } private String getCurrentJMXUser() diff --git a/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java b/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java index f3c07d9eb2..958ee35476 100644 --- a/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java +++ b/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java @@ -21,103 +21,213 @@ package org.apache.qpid.server.security.access.management; +import java.io.BufferedReader; import java.io.BufferedWriter; import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileReader; import java.io.FileWriter; import java.io.IOException; -import org.apache.qpid.server.security.auth.database.Base64MD5PasswordFilePrincipalDatabase; +import javax.management.MalformedObjectNameException; +import javax.management.ObjectName; + +import org.apache.commons.configuration.ConfigurationException; +import org.apache.qpid.server.management.MBeanInvocationHandlerImpl; +import org.apache.qpid.server.security.auth.database.PlainPasswordFilePrincipalDatabase; import junit.framework.TestCase; +/* Note: The main purpose is to test the jmx access rights file manipulation + * within AMQUserManagementMBean. The Principal Databases are tested by their own tests, + * this test just exercises their usage in AMQUserManagementMBean. + */ public class AMQUserManagementMBeanTest extends TestCase { - private Base64MD5PasswordFilePrincipalDatabase _database; + private PlainPasswordFilePrincipalDatabase _database; private AMQUserManagementMBean _amqumMBean; + + private File _passwordFile; + private File _accessFile; - private static final String _QPID_HOME = System.getProperty("QPID_HOME"); - - private static final String USERNAME = "testuser"; - private static final String PASSWORD = "password"; - private static final String JMXRIGHTS = "admin"; - private static final String TEMP_PASSWORD_FILE_NAME = "tempPasswordFile.tmp"; - private static final String TEMP_JMXACCESS_FILE_NAME = "tempJMXAccessFile.tmp"; + private static final String TEST_USERNAME = "testuser"; + private static final String TEST_PASSWORD = "password"; @Override protected void setUp() throws Exception { - assertNotNull("QPID_HOME not set", _QPID_HOME); - - _database = new Base64MD5PasswordFilePrincipalDatabase(); + _database = new PlainPasswordFilePrincipalDatabase(); _amqumMBean = new AMQUserManagementMBean(); + loadFreshTestPasswordFile(); + loadFreshTestAccessFile(); } @Override protected void tearDown() throws Exception { - File testFile = new File(_QPID_HOME + File.separator + TEMP_JMXACCESS_FILE_NAME + ".tmp"); - if (testFile.exists()) + _passwordFile.delete(); + _accessFile.delete(); + } + + public void testDeleteUser() + { + loadFreshTestPasswordFile(); + loadFreshTestAccessFile(); + + //try deleting a non existant user + assertFalse(_amqumMBean.deleteUser("made.up.username")); + + assertTrue(_amqumMBean.deleteUser(TEST_USERNAME)); + } + + public void testDeleteUserIsSavedToAccessFile() + { + loadFreshTestPasswordFile(); + loadFreshTestAccessFile(); + + assertTrue(_amqumMBean.deleteUser(TEST_USERNAME)); + + //check the access rights were actually deleted from the file + try{ + BufferedReader reader = new BufferedReader(new FileReader(_accessFile)); + + //check the 'generated by' comment line is present + assertTrue("File has no content", reader.ready()); + assertTrue("'Generated by' comment line was missing",reader.readLine().contains("Generated by " + + "AMQUserManagementMBean Console : Last edited by user:")); + + //there should also be a modified date/time comment line + assertTrue("File has no modified date/time comment line", reader.ready()); + assertTrue("Modification date/time comment line was missing",reader.readLine().startsWith("#")); + + //the access file should not contain any further data now as we just deleted the only user + assertFalse("User access data was present when it should have been deleted", reader.ready()); + } + catch (IOException e) { - testFile.delete(); + fail("Unable to valdate file contents due to:" + e.getMessage()); } + + } + + public void testSetRights() + { + loadFreshTestPasswordFile(); + loadFreshTestAccessFile(); + + assertFalse(_amqumMBean.setRights("made.up.username", true, false, false)); + + assertTrue(_amqumMBean.setRights(TEST_USERNAME, true, false, false)); + assertTrue(_amqumMBean.setRights(TEST_USERNAME, false, true, false)); + assertTrue(_amqumMBean.setRights(TEST_USERNAME, false, false, true)); + } + + public void testSetRightsIsSavedToAccessFile() + { + loadFreshTestPasswordFile(); + loadFreshTestAccessFile(); + + assertTrue(_amqumMBean.setRights(TEST_USERNAME, false, false, true)); + + //check the access rights were actually updated in the file + try{ + BufferedReader reader = new BufferedReader(new FileReader(_accessFile)); + + //check the 'generated by' comment line is present + assertTrue("File has no content", reader.ready()); + assertTrue("'Generated by' comment line was missing",reader.readLine().contains("Generated by " + + "AMQUserManagementMBean Console : Last edited by user:")); - testFile = new File(_QPID_HOME + File.separator + TEMP_JMXACCESS_FILE_NAME + ".old"); - if (testFile.exists()) + //there should also be a modified date/time comment line + assertTrue("File has no modified date/time comment line", reader.ready()); + assertTrue("Modification date/time comment line was missing",reader.readLine().startsWith("#")); + + //the access file should not contain any further data now as we just deleted the only user + assertTrue("User access data was not updated in the access file", + reader.readLine().equals(TEST_USERNAME + "=" + MBeanInvocationHandlerImpl.ADMIN)); + + //the access file should not contain any further data now as we just deleted the only user + assertFalse("Additional user access data was present when there should be no more", reader.ready()); + } + catch (IOException e) { - testFile.delete(); + fail("Unable to valdate file contents due to:" + e.getMessage()); } + } - testFile = new File(_QPID_HOME + File.separator + TEMP_PASSWORD_FILE_NAME + ".tmp"); - if (testFile.exists()) + public void testMBeanVersion() + { + try { - testFile.delete(); + ObjectName name = _amqumMBean.getObjectName(); + assertEquals(AMQUserManagementMBean.VERSION, Integer.parseInt(name.getKeyProperty("version"))); } - - testFile = new File(_QPID_HOME + File.separator + TEMP_PASSWORD_FILE_NAME + ".old"); - if (testFile.exists()) + catch (MalformedObjectNameException e) { - testFile.delete(); + fail(e.getMessage()); } } - public void testDeleteUser() + public void testSetAccessFileWithMissingFile() { - loadTestPasswordFile(); - loadTestAccessFile(); - - boolean deleted = false; + try + { + _amqumMBean.setAccessFile("made.up.filename"); + } + catch (IOException e) + { + fail("Should not have been an IOE." + e.getMessage()); + } + catch (ConfigurationException e) + { + assertTrue(e.getMessage(), e.getMessage().endsWith("does not exist")); + } + } + public void testSetAccessFileWithReadOnlyFile() + { + File testFile = null; try { - deleted = _amqumMBean.deleteUser(USERNAME); + testFile = File.createTempFile(this.getClass().getName(),".access.readonly"); + BufferedWriter passwordWriter = new BufferedWriter(new FileWriter(testFile, false)); + passwordWriter.write(TEST_USERNAME + ":" + TEST_PASSWORD); + passwordWriter.newLine(); + passwordWriter.flush(); + passwordWriter.close(); + + testFile.setReadOnly(); + _amqumMBean.setAccessFile(testFile.getPath()); } - catch(Exception e){ - fail("Unable to delete user: " + e.getMessage()); + catch (IOException e) + { + fail("Access file was not created." + e.getMessage()); + } + catch (ConfigurationException e) + { + fail("There should not have been a configuration exception." + e.getMessage()); } - assertTrue(deleted); + testFile.delete(); } - - + // ============================ Utility methods ========================= - private void loadTestPasswordFile() + private void loadFreshTestPasswordFile() { try { - File tempPasswordFile = new File(_QPID_HOME + File.separator + TEMP_PASSWORD_FILE_NAME); - if (tempPasswordFile.exists()) + if(_passwordFile == null) { - tempPasswordFile.delete(); + _passwordFile = File.createTempFile(this.getClass().getName(),".password"); } - tempPasswordFile.deleteOnExit(); - BufferedWriter passwordWriter = new BufferedWriter(new FileWriter(tempPasswordFile)); - passwordWriter.write(USERNAME + ":" + PASSWORD); + BufferedWriter passwordWriter = new BufferedWriter(new FileWriter(_passwordFile, false)); + passwordWriter.write(TEST_USERNAME + ":" + TEST_PASSWORD); passwordWriter.newLine(); passwordWriter.flush(); - - _database.setPasswordFile(tempPasswordFile.toString()); + passwordWriter.close(); + _database.setPasswordFile(_passwordFile.toString()); _amqumMBean.setPrincipalDatabase(_database); } catch (IOException e) @@ -126,27 +236,36 @@ public class AMQUserManagementMBeanTest extends TestCase } } - private void loadTestAccessFile() + private void loadFreshTestAccessFile() { try { - File tempAccessFile = new File(_QPID_HOME + File.separator + TEMP_JMXACCESS_FILE_NAME); - if (tempAccessFile.exists()) + if(_accessFile == null) { - tempAccessFile.delete(); + _accessFile = File.createTempFile(this.getClass().getName(),".access"); } - tempAccessFile.deleteOnExit(); - - BufferedWriter accessWriter = new BufferedWriter(new FileWriter(tempAccessFile)); - accessWriter.write(USERNAME + "=" + JMXRIGHTS); + + BufferedWriter accessWriter = new BufferedWriter(new FileWriter(_accessFile,false)); + accessWriter.write("#Last Updated By comment"); + accessWriter.newLine(); + accessWriter.write("#Date/time comment"); + accessWriter.newLine(); + accessWriter.write(TEST_USERNAME + "=" + MBeanInvocationHandlerImpl.READONLY); accessWriter.newLine(); accessWriter.flush(); + accessWriter.close(); + } + catch (IOException e) + { + fail("Unable to create test access file: " + e.getMessage()); + } - _amqumMBean.setAccessFile(tempAccessFile.toString()); + try{ + _amqumMBean.setAccessFile(_accessFile.toString()); } catch (Exception e) { - fail("Unable to create test access file: " + e.getMessage()); + fail("Unable to set access file: " + e.getMessage()); } } } |