Author: kwall
Date: Wed Jul 20 13:57:19 2016
New Revision: 1753518
URL: http://svn.apache.org/viewvc?rev=1753518&view=rev
Log:
QPID-7247: Refactor preference validations to make checks more cohesive
Also improved the unit tests.
Modified:
qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/preferences/UserPreferencesImpl.java
qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/PreferencesTest.java
Modified:
qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/preferences/UserPreferencesImpl.java
URL:
http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/preferences/UserPreferencesImpl.java?rev=1753518&r1=1753517&r2=1753518&view=diff
==============================================================================
---
qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/preferences/UserPreferencesImpl.java
(original)
+++
qpid/java/trunk/broker-core/src/main/java/org/apache/qpid/server/model/preferences/UserPreferencesImpl.java
Wed Jul 20 13:57:19 2016
@@ -439,34 +439,36 @@ public class UserPreferencesImpl impleme
// validate owner
if (!principalsEqual(currentPrincipal, preference.getOwner()))
{
- throw new SecurityException(String.format("Preference '%s' not
owned by current user.",
+ throw new IllegalArgumentException(String.format("Preference
owner contradicts the current user.",
preference.getId().toString()));
}
- if (preference.getId() != null)
- {
- Preference oldPreference =
_preferences.get(preference.getId());
- if (oldPreference != null &&
!principalsEqual(oldPreference.getOwner(), preference.getOwner()))
- {
- throw new SecurityException(String.format(
- "Ownership of other user preference having id '%s'
and name '%s' cannot be changed to current user",
- preference.getId().toString(),
- preference.getName()));
- }
- }
}
}
private void checkForConflictWithExisting(final Collection<Preference>
preferences)
{
+ Principal currentPrincipal = getMainPrincipalOrThrow();
+
for (Preference preference : preferences)
{
// check for conflicts with existing preferences
final Preference storedPreference =
_preferences.get(preference.getId());
- if (storedPreference != null &&
!Objects.equals(storedPreference.getType(), preference.getType()))
+ if (storedPreference != null)
{
- throw new IllegalArgumentException("Cannot change type of
preference");
+ if (!principalsEqual(storedPreference.getOwner(),
currentPrincipal))
+ {
+ throw new SecurityException(String.format(
+ "Preference '%s' exists but is not owned by the
current user.",
+ preference.getId().toString()));
+ }
+
+ if (!Objects.equals(storedPreference.getType(),
preference.getType()))
+ {
+ throw new IllegalArgumentException("Cannot change type of
preference");
+ }
}
+
List<Preference> preferencesWithSameName =
_preferencesByName.get(preference.getName());
if (preferencesWithSameName != null)
{
Modified:
qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/PreferencesTest.java
URL:
http://svn.apache.org/viewvc/qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/PreferencesTest.java?rev=1753518&r1=1753517&r2=1753518&view=diff
==============================================================================
---
qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/PreferencesTest.java
(original)
+++
qpid/java/trunk/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/PreferencesTest.java
Wed Jul 20 13:57:19 2016
@@ -37,6 +37,8 @@ import java.util.UUID;
import javax.security.auth.Subject;
+import com.google.common.util.concurrent.ListenableFuture;
+
import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
import org.apache.qpid.server.configuration.updater.TaskExecutor;
import org.apache.qpid.server.model.ConfiguredObject;
@@ -108,7 +110,7 @@ public class PreferencesTest extends Qpi
});
}
- public void testOnlyAllowUpdateOwnedPreferences()
+ public void testProhibitNewPreferenceWithContradictingOwner()
{
final Preference p = PreferenceFactory.recover(_testObject,
PreferenceTestHelper.createPreferenceAttributes(
null,
@@ -131,9 +133,9 @@ public class PreferencesTest extends Qpi
awaitPreferenceFuture(_testObject.getUserPreferences().updateOrAppend(preferences));
fail("Saving of preferences owned by somebody else should
not be allowed");
}
- catch (SecurityException e)
+ catch (IllegalArgumentException e)
{
- // pass
+ // PASS
}
return null;
}
@@ -347,6 +349,55 @@ public class PreferencesTest extends Qpi
});
}
+ public void testProhibitPreferenceStealing() throws Exception
+ {
+ final String testGroupName = "testGroup";
+ Subject user1Subject =
TestPrincipalUtils.createTestSubject(TEST_USERNAME, testGroupName);
+
+ Map<String, Object> preferenceAttributes =
PreferenceTestHelper.createPreferenceAttributes(
+ _testObject.getId(),
+ null,
+ "X-PREF",
+ "prefname",
+ null,
+ TEST_USERNAME,
+ Collections.singleton(testGroupName),
+ Collections.<String,Object>emptyMap());
+ final Preference originalPreference =
PreferenceFactory.recover(_testObject, preferenceAttributes);
+ updateOrAppendAs(user1Subject, originalPreference);
+
+
+ Subject user2Subject =
TestPrincipalUtils.createTestSubject(TEST_USERNAME2, testGroupName);
+ Subject.doAs(user2Subject, new PrivilegedAction<Void>()
+ {
+ @Override
+ public Void run()
+ {
+ final ListenableFuture<Set<Preference>>
visiblePreferencesFuture =
_testObject.getUserPreferences().getVisiblePreferences();
+ final Set<Preference> visiblePreferences =
PreferenceTestHelper.awaitPreferenceFuture(visiblePreferencesFuture);
+ assertEquals("Unexpected number of visible preferences", 1,
visiblePreferences.size());
+
+ final Preference target = visiblePreferences.iterator().next();
+ Map<String, Object> replacementAttributes = new
HashMap(target.getAttributes());
+ replacementAttributes.put(Preference.OWNER_ATTRIBUTE,
TEST_USERNAME2);
+
+ try
+ {
+
awaitPreferenceFuture(_testObject.getUserPreferences().updateOrAppend(Arrays.asList(PreferenceFactory.recover(_testObject,
replacementAttributes))));
+ fail("The stealing of a preference must be prohibited");
+ }
+ catch (SecurityException e)
+ {
+ // pass
+ }
+
+ return null;
+ }
+ });
+
+ assertSinglePreference(user1Subject, originalPreference);
+ }
+
public void testReplace()
{
final String preferenceType = "X-testType";
@@ -1255,37 +1306,6 @@ public class PreferencesTest extends Qpi
assertEquals("Unexpected preference attributes", expectedAttributes,
p.getAttributes());
}
- public void testSavingOtherUserPreference() throws Exception
- {
- final String testGroupName = "testGroup";
- Subject user1Subject =
TestPrincipalUtils.createTestSubject(TEST_USERNAME, testGroupName);
-
- Map<String, Object> preferenceAttributes =
PreferenceTestHelper.createPreferenceAttributes(
- _testObject.getId(),
- UUID.randomUUID(),
- "X-PREF",
- "prefname",
- null,
- TEST_USERNAME,
- Collections.singleton(testGroupName),
- Collections.<String,Object>emptyMap());
- updateOrAppendAs(user1Subject, PreferenceFactory.recover(_testObject,
preferenceAttributes));
-
- Subject user2Subject =
TestPrincipalUtils.createTestSubject(TEST_USERNAME2, testGroupName);
- preferenceAttributes.put(Preference.OWNER_ATTRIBUTE, TEST_USERNAME2);
- Preference stolenPreference = PreferenceFactory.recover(_testObject,
preferenceAttributes);
-
- try
- {
- updateOrAppendAs(user2Subject, stolenPreference);
- fail("Steeling of other user preferences should not be allowed");
- }
- catch (SecurityException e)
- {
- // pass
- }
- }
-
private void updateOrAppendAs(final Subject testSubject, final
Preference... testUserPreference)
{
Subject.doAs(testSubject, new PrivilegedAction<Void>()
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
