Allon Mureinik has uploaded a new change for review.
Change subject: core: QueriesCommandBase public query validation
......................................................................
core: QueriesCommandBase public query validation
The Backend bean is just a wrapper used to call queries and actions, and
should not have any security logic in it.
This patch moves the exiting validation inside QueriesCommandBase, which
provides a couple of improvements:
1. QueriesCommandBase is more self contained, and does not rely on the
caller (Backend) for some of the validations.
2. All the security validations are done in the same place, both for
logged in and not logged in callers.
This patch contains:
1. Adding another authorization type, Public, to the VdcQueryAuthType
enum to handle Public Queries, which do not require a log in.
2. Adding the proper handling for it in QueriesCommandBase.
3. Adding a new Public value to the ConfigAuthType enum, since
configuration values permissions are handled per key.
4. Removing the specific Public Query logic from Backend.runQueryImpl.
This is a step towards removing Backend.runPublicQuery, which would
be done in a subsequent patch.
Note that this patch does NOT leave the system in a vulnerable state
- The validation removed from the Backend were added to
QueriesCommandBase, and even though Backend.runPublicQuery still
exists, it has become a meaningless wrapper method.
5. Updating QueriesCommandBaseTest, GetConfigurationValueQueryTest and
AbstractUserQueryTest to test this new functionality.
Change-Id: I6f428c6449873054f3d7d291eef6caf4c9b09bb2
Signed-off-by: Allon Mureinik <amure...@redhat.com>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValueQuery.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QueriesCommandBase.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AbstractUserQueryTest.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetConfigurationValueQueryTest.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java
M
backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/queries/VdcQueryTypeTest.java
9 files changed, 90 insertions(+), 63 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/01/23001/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java
index 675ba69..e1b3c95 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java
@@ -461,11 +461,7 @@
protected VdcQueryReturnValue runQueryImpl(VdcQueryType actionType,
VdcQueryParametersBase parameters,
boolean isPerformUserCheck) {
if (isPerformUserCheck) {
- String sessionId = addSessionToContext(parameters);
- if (StringUtils.isEmpty(sessionId)
- || SessionDataContainer.getInstance().getUser(sessionId,
parameters.getRefresh()) == null) {
- return
getErrorQueryReturnValue(VdcBllMessages.USER_IS_NOT_LOGGED_IN);
- }
+ addSessionToContext(parameters);
}
Class<CommandBase<? extends VdcActionParametersBase>> clazz =
CommandsFactory.getQueryClass(actionType.name());
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValueQuery.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValueQuery.java
index 48c8267..868bf74 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValueQuery.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValueQuery.java
@@ -4,6 +4,7 @@
import org.ovirt.engine.core.common.config.ConfigCommon;
import org.ovirt.engine.core.common.config.ConfigValues;
import org.ovirt.engine.core.common.queries.GetConfigurationValueParameters;
+import org.ovirt.engine.core.common.queries.VdcQueryType;
public class GetConfigurationValueQuery<P extends
GetConfigurationValueParameters> extends QueriesCommandBase<P> {
public GetConfigurationValueQuery(P parameters) {
@@ -13,34 +14,25 @@
@Override
protected void executeQueryCommand() {
Object returnValue = null;
- if (shouldReturnValue()) {
- try {
- final GetConfigurationValueParameters params = getParameters();
- final ConfigValues value =
ConfigValues.valueOf(params.getConfigValue().toString());
- String version = params.getVersion();
- if (version == null) {
- log.warnFormat("calling {0} ({2}) with null version, using
default {1} for version",
- GetConfigurationValueQuery.class.getSimpleName(),
ConfigCommon.defaultConfigurationVersion, value);
- version = ConfigCommon.defaultConfigurationVersion;
- }
- returnValue = Config.<Object> getValue(value, version);
- } catch (Exception e) {
- log.error("Unable to return config parameter: " +
getParameters(), e);
+ try {
+ final GetConfigurationValueParameters params = getParameters();
+ final ConfigValues value =
ConfigValues.valueOf(params.getConfigValue().toString());
+ String version = params.getVersion();
+ if (version == null) {
+ log.warnFormat("calling {0} ({2}) with null version, using
default {1} for version",
+ GetConfigurationValueQuery.class.getSimpleName(),
ConfigCommon.defaultConfigurationVersion, value);
+ version = ConfigCommon.defaultConfigurationVersion;
}
+ returnValue = Config.<Object> getValue(value, version);
+ } catch (Exception e) {
+ log.error("Unable to return config parameter: " + getParameters(),
e);
}
getQueryReturnValue().setReturnValue(returnValue);
}
- /**
- * Validates if the query should return anything or not, according to the
user's permissions:
- * <ul>
- * <li>If the query is run as an administrator (note that since we've
reached the {@link #executeQueryCommand()} method,
- * we've already validated that the use is indeed an administrator), the
results from the database queries should be returned.</li>
- * <li>If the query is run as a user, it may return results <b>ONLY</b> if
the configuration value has {@link
org.ovirt.engine.core.common.queries.ConfigurationValues.ConfigAuthType#User}.</li>
- * </ul>
- */
- private boolean shouldReturnValue() {
- return !getParameters().isFiltered() ||
!getParameters().getConfigValue().isAdmin();
+ @Override
+ protected VdcQueryType.VdcQueryAuthType getAuthType() {
+ return
VdcQueryType.VdcQueryAuthType.valueOf(getParameters().getConfigValue().getConfigAuthType().toString());
}
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QueriesCommandBase.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QueriesCommandBase.java
index 44c2b8a..3f9c4a1 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QueriesCommandBase.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QueriesCommandBase.java
@@ -1,5 +1,7 @@
package org.ovirt.engine.core.bll;
+import static
org.ovirt.engine.core.common.queries.VdcQueryType.VdcQueryAuthType;
+
import java.util.Set;
import javax.validation.ConstraintViolation;
@@ -88,7 +90,9 @@
log.error("Query execution failed due to invalid inputs. " +
returnValue.getExceptionString());
}
} else {
- String errMessage =
VdcBllMessages.USER_CANNOT_RUN_QUERY_NO_PERMISSION.name();
+ String errMessage = getUser() == null ?
+ VdcBllMessages.USER_CANNOT_RUN_QUERY_NOT_PUBLIC.name() :
+ VdcBllMessages.USER_CANNOT_RUN_QUERY_NO_PERMISSION.name();
log.error(errMessage);
returnValue.setExceptionString(errMessage);
}
@@ -101,15 +105,25 @@
* <code>false</code> otherwise.
*/
private final boolean validatePermissions() {
- // If the user requests filtered execution, his permissions are
inconsequential.
- // If the query supports filtering it should be allowed, and if not -
not.
- if (parameters.isFiltered()) {
- return !queryType.isAdmin();
- }
-
// If the query was executed internally, it should be allowed in any
event.
if (isInternalExecution) {
return true;
+ }
+
+ // If the query is public, it should always be allowed
+ if (getAuthType().isPublic()) {
+ return true;
+ }
+
+ // If the query is not public, the user must be logged on
+ if (getUser() == null) {
+ return false;
+ }
+
+ // If the user requests filtered execution, his permissions are
inconsequential.
+ // If the query supports filtering it should be allowed, and if not -
not.
+ if (parameters.isFiltered()) {
+ return !getAuthType().isAdmin();
}
// In any other event, we have admin execution, which should only be
allowed according to the user's
@@ -118,6 +132,10 @@
return getUser().isAdmin();
}
+ protected VdcQueryAuthType getAuthType() {
+ return queryType.getAuthType();
+ }
+
/**
* @return true if all parameters class and its inner members passed
* validation
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AbstractUserQueryTest.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AbstractUserQueryTest.java
index 5731df1..a70951d 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AbstractUserQueryTest.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AbstractUserQueryTest.java
@@ -29,7 +29,7 @@
}
/** Sets up a mock for {@link #user} */
- private void setUpMockUser() {
+ protected void setUpMockUser() {
userID = new Guid(UUID.randomUUID());
user = mock(DbUser.class);
when(user.getId()).thenReturn(userID);
@@ -56,6 +56,6 @@
@Test
public void testQueryIsAUserQuery() throws IllegalArgumentException,
IllegalAccessException {
assertFalse("A query tested for filtered access should not be an admin
query",
-
TestHelperQueriesCommandType.getQueryTypeFieldValue(getQuery()).isAdmin());
+
TestHelperQueriesCommandType.getQueryTypeFieldValue(getQuery()).getAuthType().isAdmin());
}
}
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetConfigurationValueQueryTest.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetConfigurationValueQueryTest.java
index 723b8c5..cd55c01 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetConfigurationValueQueryTest.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetConfigurationValueQueryTest.java
@@ -25,12 +25,21 @@
private ConfigurationValues configValue;
private boolean isFiltered;
+ private boolean isLoggedIn;
private boolean shouldSucceed;
- public GetConfigurationValueQueryTest(ConfigurationValues configValue,
boolean isFiltered, boolean shouldSucceed) {
+ public GetConfigurationValueQueryTest(ConfigurationValues configValue,
boolean isFiltered, boolean isLoggedIn, boolean shouldSucceed) {
this.configValue = configValue;
this.isFiltered = isFiltered;
+ this.isLoggedIn = isLoggedIn;
this.shouldSucceed = shouldSucceed;
+ }
+
+ @Override
+ protected void setUpMockUser() {
+ if (isLoggedIn) {
+ super.setUpMockUser();
+ }
}
/**
@@ -38,16 +47,21 @@
* <ol>
* <li>The {@link ConfigurationValues} constant to test</li>
* <li>Testing in filtered/unfiltered mode</li>
+ * <li>Whether the user logged in or not</li>
* <li>Whether this query should succeed or not</li>
* </ol>
*/
@Parameterized.Parameters
public static Collection<Object[]> data() {
return Arrays.asList(new Object[][]
- { { ConfigurationValues.MaxVmsInPool, true, true }, // A user config
value
- { ConfigurationValues.MaxVmsInPool, false, false },
- { ConfigurationValues.AdUserName, true, false}, // An admin
config value
- { ConfigurationValues.AdUserName, false, false}
+ { { ConfigurationValues.MaxVmsInPool, true, true, true }, // A user
config value
+ { ConfigurationValues.MaxVmsInPool, true, false, false },
+ { ConfigurationValues.MaxVmsInPool, false, true, false },
+ { ConfigurationValues.MaxVmsInPool, false, false, false },
+ { ConfigurationValues.AdUserName, true, true, false}, // An
admin config value
+ { ConfigurationValues.AdUserName, true, false, false},
+ { ConfigurationValues.AdUserName, false, true, false},
+ { ConfigurationValues.AdUserName, false, false, false}
});
}
@@ -67,9 +81,10 @@
Object actual = getQuery().getQueryReturnValue().getReturnValue();
if (shouldSucceed) {
- assertEquals("Got wrong value for " + configValue, expected,
actual);
+ assertEquals("[loggedin=" + isLoggedIn + "] Got wrong value for "
+ configValue, expected, actual);
} else {
- assertNull("Should get null result for " + configValue + " but got
" + actual, actual);
+ assertNull("[loggedin=" + isLoggedIn + "] Should get null result
for " + configValue + " but got "
+ + actual, actual);
}
}
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java
index 61f6b85..61a3721 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QueriesCommandBaseTest.java
@@ -66,11 +66,11 @@
for (boolean isFiltered : booleans) {
for (boolean isUserAdmin : booleans) {
for (boolean isInternalExecution : booleans) {
- boolean shouldBeAbleToRunQuery;
+ boolean shouldBeAbleToRunQuery = isInternalExecution
|| queryType.getAuthType().isPublic();
if (isFiltered) {
- shouldBeAbleToRunQuery = !queryType.isAdmin();
+ shouldBeAbleToRunQuery = shouldBeAbleToRunQuery ||
!queryType.getAuthType().isAdmin();
} else {
- shouldBeAbleToRunQuery = isInternalExecution ||
isUserAdmin;
+ shouldBeAbleToRunQuery = shouldBeAbleToRunQuery ||
isUserAdmin;
}
log.debug("Running on query: " + toString());
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java
index a3a239a..727b447 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java
@@ -4,7 +4,7 @@
MaxNumOfVmCpus(ConfigAuthType.User),
MaxNumOfVmSockets(ConfigAuthType.User),
MaxNumOfCpuPerSocket(ConfigAuthType.User),
- VdcVersion(ConfigAuthType.User),
+ VdcVersion(ConfigAuthType.Public),
// GetAllAdDomains,
SSLEnabled(ConfigAuthType.User),
CipherSuite(ConfigAuthType.User),
@@ -64,13 +64,13 @@
SupportedClusterLevels(ConfigAuthType.User),
OvfUpdateIntervalInMinutes,
OvfItemsCountPerUpdate,
- ProductRPMVersion(ConfigAuthType.User),
+ ProductRPMVersion(ConfigAuthType.Public),
RhevhLocalFSPath,
HotPlugEnabled(ConfigAuthType.User),
NetworkLinkingSupported(ConfigAuthType.User),
SupportBridgesReportByVDSM(ConfigAuthType.User),
ManagementNetwork,
- ApplicationMode(ConfigAuthType.User),
+ ApplicationMode(ConfigAuthType.Public),
ShareableDiskEnabled(ConfigAuthType.User),
DirectLUNDiskEnabled(ConfigAuthType.User),
WANDisableEffects(ConfigAuthType.User),
@@ -96,7 +96,7 @@
MaxAverageNetworkQoSValue,
MaxPeakNetworkQoSValue,
MaxBurstNetworkQoSValue,
- UserMessageOfTheDay(ConfigAuthType.User),
+ UserMessageOfTheDay(ConfigAuthType.Public),
QoSInboundAverageDefaultValue,
QoSInboundPeakDefaultValue,
QoSInboundBurstDefaultValue,
@@ -114,7 +114,8 @@
public static enum ConfigAuthType {
Admin,
- User
+ User,
+ Public
}
private ConfigAuthType authType;
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java
index 7c5d7e5..5fddbc3 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java
@@ -156,7 +156,7 @@
GetAllBookmarks,
// Configuration values
- GetConfigurationValue(VdcQueryAuthType.User),
+ GetConfigurationValue(VdcQueryAuthType.Public),
GetConfigurationValues(VdcQueryAuthType.User),
GetDefaultTimeZone(VdcQueryAuthType.User),
GetAvailableStoragePoolVersions(VdcQueryAuthType.User),
@@ -174,10 +174,10 @@
AdGroupsSearch(VdcQueryAuthType.User),
// Public services
- GetDomainList(VdcQueryAuthType.User),
- RegisterVds(VdcQueryAuthType.User),
- CheckDBConnection(VdcQueryAuthType.User),
- ValidateSession(VdcQueryAuthType.User),
+ GetDomainList(VdcQueryAuthType.Public),
+ RegisterVds(VdcQueryAuthType.Public),
+ CheckDBConnection(VdcQueryAuthType.Public),
+ ValidateSession(VdcQueryAuthType.Public),
// License queries
GetAllServerCpuList,
@@ -312,7 +312,16 @@
*/
public static enum VdcQueryAuthType {
Admin,
- User
+ User,
+ Public;
+
+ public boolean isAdmin() {
+ return this == Admin;
+ }
+
+ public boolean isPublic() {
+ return this == Public;
+ }
}
private static final String DEFAULT_PACKAGE_NAME =
"org.ovirt.engine.core.bll";
@@ -350,9 +359,5 @@
public VdcQueryAuthType getAuthType() {
return authType;
- }
-
- public boolean isAdmin() {
- return authType == VdcQueryAuthType.Admin;
}
}
diff --git
a/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/queries/VdcQueryTypeTest.java
b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/queries/VdcQueryTypeTest.java
index b4620c5..b208b50 100644
---
a/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/queries/VdcQueryTypeTest.java
+++
b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/queries/VdcQueryTypeTest.java
@@ -19,6 +19,6 @@
@Test
public void testAuthTypes() {
assertEquals("Unknown should not be an admin query",
VdcQueryAuthType.User, VdcQueryType.Unknown.getAuthType());
- assertFalse("Unknown should not be an admin query",
VdcQueryType.Unknown.isAdmin());
+ assertFalse("Unknown should not be an admin query",
VdcQueryType.Unknown.getAuthType().isAdmin());
}
}
--
To view, visit http://gerrit.ovirt.org/23001
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6f428c6449873054f3d7d291eef6caf4c9b09bb2
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
