You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ri...@apache.org on 2009/04/13 15:44:31 UTC
svn commit: r764458 - in /qpid/branches/0.5-fix/qpid: ./
java/broker/src/main/java/org/apache/qpid/server/security/access/management/
java/broker/src/test/java/org/apache/qpid/server/security/access/management/
Author: ritchiem
Date: Mon Apr 13 13:44:30 2009
New Revision: 764458
URL: http://svn.apache.org/viewvc?rev=764458&view=rev
Log:
QPID-1655: use a File object to hold reference to access file instead of a String to fix issue with createTempFile and absolute paths. Stop catching IOExceptions in saveAccessFile() and make calling methods catch them to check for and report failure and act accordingly to reverse actions in memory. Add additional unit tests to cover access rights file manipulation.
merged from trunk r748686
Modified:
qpid/branches/0.5-fix/qpid/ (props changed)
qpid/branches/0.5-fix/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java
qpid/branches/0.5-fix/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java
Propchange: qpid/branches/0.5-fix/qpid/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Mon Apr 13 13:44:30 2009
@@ -1 +1 @@
-/qpid/trunk/qpid:742626,743015,743028-743029,743304,743306,743311,743357,744113,747363,747367,747369-747370,747376,747783,747868-747870,747875,748561,748591,748641,748680
+/qpid/trunk/qpid:742626,743015,743028-743029,743304,743306,743311,743357,744113,747363,747367,747369-747370,747376,747783,747868-747870,747875,748561,748591,748641,748680,748686
Modified: qpid/branches/0.5-fix/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java
URL: http://svn.apache.org/viewvc/qpid/branches/0.5-fix/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java?rev=764458&r1=764457&r2=764458&view=diff
==============================================================================
--- qpid/branches/0.5-fix/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java (original)
+++ qpid/branches/0.5-fix/qpid/java/broker/src/main/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBean.java Mon Apr 13 13:44:30 2009
@@ -64,9 +64,9 @@
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 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 @@
try
{
-
_accessRightsUpdate.lock();
// Update the access rights
@@ -166,8 +166,29 @@
_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 @@
{
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 boolean deleteUser(String username)
{
-
try
{
if (_principalDatabase.deletePrincipal(new UsernamePrincipal(username)))
@@ -204,7 +238,16 @@
_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 @@
_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 @@
}
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 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 @@
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 @@
}
}
- 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 @@
_accessRightsUpdate.unlock();
}
}
+
}
private String getCurrentJMXUser()
Modified: qpid/branches/0.5-fix/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java
URL: http://svn.apache.org/viewvc/qpid/branches/0.5-fix/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java?rev=764458&r1=764457&r2=764458&view=diff
==============================================================================
--- qpid/branches/0.5-fix/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java (original)
+++ qpid/branches/0.5-fix/qpid/java/broker/src/test/java/org/apache/qpid/server/security/access/management/AMQUserManagementMBeanTest.java Mon Apr 13 13:44:30 2009
@@ -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());
}
+
+ }
- testFile = new File(_QPID_HOME + File.separator + TEMP_JMXACCESS_FILE_NAME + ".old");
- if (testFile.exists())
+ 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:"));
+
+ //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 @@
}
}
- 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());
}
}
}
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:commits-subscribe@qpid.apache.org