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:[email protected]
