Yair Zaslavsky has uploaded a new change for review.
Change subject: core: Fix coverity issues
......................................................................
core: Fix coverity issues
These issues were introduced due authentication refactoring
Change-Id: I57ff7df3075d4ca36a50016c6786ca8a69cee3e0
Signed-off-by: Yair Zaslavsky <yzasl...@redhat.com>
---
M
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/internal/InternalDirectory.java
M
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/nop/NopDirectory.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/authentication/provisional/ProvisionalDirectory.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddGroupCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryGroupByIdQuery.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryUserByIdQuery.java
M backend/manager/modules/common/exclude-filters.xml
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Configuration.java
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Directory.java
M
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
M
backend/manager/modules/common/src/test/java/org/ovirt/engine/core/authentication/ConfigurationTest.java
13 files changed, 84 insertions(+), 53 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/87/23787/1
diff --git
a/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/internal/InternalDirectory.java
b/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/internal/InternalDirectory.java
index 9c784f2..24d6460 100644
---
a/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/internal/InternalDirectory.java
+++
b/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/internal/InternalDirectory.java
@@ -16,6 +16,11 @@
*/
public class InternalDirectory implements Directory {
/**
+ *
+ */
+ private static final long serialVersionUID = 6614140186031169227L;
+
+ /**
* The name of the directory:
*/
private String name;
diff --git
a/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/nop/NopDirectory.java
b/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/nop/NopDirectory.java
index c350b56..04f1a8f 100644
---
a/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/nop/NopDirectory.java
+++
b/backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/nop/NopDirectory.java
@@ -12,6 +12,10 @@
public class NopDirectory implements Directory {
/**
+ *
+ */
+ private static final long serialVersionUID = 3719648746441818198L;
+ /**
* The name of the directory.
*/
private String name;
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/authentication/provisional/ProvisionalDirectory.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/authentication/provisional/ProvisionalDirectory.java
index 83ea9fd..6dd812c 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/authentication/provisional/ProvisionalDirectory.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/authentication/provisional/ProvisionalDirectory.java
@@ -27,6 +27,11 @@
* It will exist only while the engine is migrated to use the new directory
interfaces, then it will be removed.
*/
public class ProvisionalDirectory implements Directory {
+ /**
+ *
+ */
+ private static final long serialVersionUID = 1L;
+
private Log log = LogFactory.getLog(ProvisionalDirectory.class);
/**
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java
index 5f0f45c..8fed103 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java
@@ -44,8 +44,10 @@
protected DirectoryGroup getAdGroup() {
if (mGroup == null && !getGroupId().equals(Guid.Empty)) {
DbGroup dbGroup =
DbFacade.getInstance().getDbGroupDao().get(getGroupId());
- Directory directory =
DirectoryManager.getInstance().getDirectory(dbGroup.getDomain());
- mGroup = directory.findGroup(dbGroup.getExternalId());
+ if (dbGroup != null) {
+ Directory directory =
DirectoryManager.getInstance().getDirectory(dbGroup.getDomain());
+ mGroup = directory.findGroup(dbGroup.getExternalId());
+ }
}
return mGroup;
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddGroupCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddGroupCommand.java
index df10277..2162f8f 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddGroupCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddGroupCommand.java
@@ -37,6 +37,12 @@
String directoryName = getParameters().getDirectory();
ExternalId id = getParameters().getId();
Directory directory =
DirectoryManager.getInstance().getDirectory(directoryName);
+ if (directory == null) {
+ if (directoryGroup == null) {
+
addCanDoActionMessage(VdcBllMessages.USER_MUST_EXIST_IN_DIRECTORY);
+ return false;
+ }
+ }
directoryGroup = directory.findGroup(id);
if (directoryGroup == null) {
addCanDoActionMessage(VdcBllMessages.USER_MUST_EXIST_IN_DIRECTORY);
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryGroupByIdQuery.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryGroupByIdQuery.java
index 77d902b..79c8341 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryGroupByIdQuery.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryGroupByIdQuery.java
@@ -17,8 +17,12 @@
final String directoryName = getParameters().getDomain();
final ExternalId id = getParameters().getId();
final Directory directory =
DirectoryManager.getInstance().getDirectory(directoryName);
- final DirectoryGroup group = directory.findGroup(id);
- getQueryReturnValue().setReturnValue(group);
+ if (directory == null) {
+ getQueryReturnValue().setSucceeded(false);
+ } else {
+ final DirectoryGroup group = directory.findGroup(id);
+ getQueryReturnValue().setReturnValue(group);
+ }
}
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryUserByIdQuery.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryUserByIdQuery.java
index e23ed89..de1bd81 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryUserByIdQuery.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDirectoryUserByIdQuery.java
@@ -17,8 +17,12 @@
String directoryName = getParameters().getDomain();
ExternalId id = getParameters().getId();
Directory directory =
DirectoryManager.getInstance().getDirectory(directoryName);
- DirectoryUser user = directory.findUser(id);
- getQueryReturnValue().setReturnValue(user);
+ if (directory == null) {
+ getQueryReturnValue().setSucceeded(false);
+ } else {
+ DirectoryUser user = directory.findUser(id);
+ getQueryReturnValue().setReturnValue(user);
+ }
}
}
diff --git a/backend/manager/modules/common/exclude-filters.xml
b/backend/manager/modules/common/exclude-filters.xml
index 34a9283..dc95680 100644
--- a/backend/manager/modules/common/exclude-filters.xml
+++ b/backend/manager/modules/common/exclude-filters.xml
@@ -59,20 +59,6 @@
<Bug code="Se"/>
</Match>
- <!--
- findbugs complains that getBoolean returns null.
- In the method usages there are null checks.
-
- findbugs reason:
- NP: Method with Boolean return type returns explicit null
(NP_BOOLEAN_RETURN_NULL)
- -->
-
- <Match>
- <Class name="org.ovirt.engine.core.authentication.Configuration" />
- <Method name="getBoolean"/>
- <Bug code="NP"/>
- </Match>
-
<!--
findbugs complains that file is an unwritten field.
This is meant to be - the file field is initialized at the CTOR
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
index 7c0796a..2ba3dcb 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
@@ -5,6 +5,7 @@
import java.util.ArrayList;
import java.util.Deque;
import java.util.List;
+
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
@@ -60,9 +61,12 @@
if (profiles == null) {
profiles = new ArrayList<AuthenticationProfile>(1);
for (AuthenticationProfile profile :
AuthenticationProfileManager.getInstance().getProfiles()) {
- Authenticator authenticator =
profile.getAuthenticator();
- if (authenticator instanceof NegotiatingAuthenticator)
{
- profiles.add(0, profile);
+ if (profile != null) {
+ Authenticator authenticator =
profile.getAuthenticator();
+ if (authenticator instanceof
NegotiatingAuthenticator) {
+ profiles.add(0, profile);
+ }
+
}
}
}
@@ -113,6 +117,12 @@
while (!stack.isEmpty()) {
// Resume the negotiation with the profile at the top of the stack:
AuthenticationProfile profile =
AuthenticationProfileManager.getInstance().getProfile(stack.peek());
+ if (profile == null) {
+ session.invalidate();
+ rsp.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ return;
+ }
+
NegotiatingAuthenticator authenticator =
(NegotiatingAuthenticator) profile.getAuthenticator();
NegotiationResult result = authenticator.negotiate(req, rsp);
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Configuration.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Configuration.java
index ce464dc..c8cc66c 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Configuration.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Configuration.java
@@ -208,14 +208,22 @@
return getKeys().isEmpty();
}
- public Boolean getBoolean(String key) {
+ public boolean getBoolean(String key, boolean defaultValueIfNull) {
String image = getString(key);
if (image == null) {
- return null;
+ return defaultValueIfNull;
}
return Boolean.parseBoolean(image);
}
+ public boolean getBoolean(String key) {
+ return getBoolean(key, false);
+ }
+
+ // public Boolean getBoolean(String key) {
+ // return getBoolean(key, "false");
+ // }
+
public Integer getInteger(String key) {
String image = getString(key);
if (image == null) {
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Directory.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Directory.java
index 9e8cea3..684f096 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Directory.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Directory.java
@@ -1,5 +1,6 @@
package org.ovirt.engine.core.authentication;
+import java.io.Serializable;
import java.util.List;
import org.ovirt.engine.core.common.utils.ExternalId;
@@ -8,7 +9,7 @@
* A directory is an object that manages a collection of users and groups,
usually stored in an external system like an
* LDAP database.
*/
-public interface Directory {
+public interface Directory extends Serializable {
/**
* Returns the name of the directory.
*
diff --git
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
index 2ebc98b..2d37b26 100644
---
a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
+++
b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
@@ -6,12 +6,12 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
-import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
-import java.util.Map;
import java.util.ServiceLoader;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
import org.jboss.modules.Module;
import org.jboss.modules.ModuleIdentifier;
@@ -42,12 +42,12 @@
/**
* Here we store factories indexed by type.
*/
- private Map<String, Factory<O>> factoriesByType;
+ private ConcurrentMap<String, Factory<O>> factoriesByType;
/**
* Here we store the objects, indexed by name.
*/
- private Map<String, O> objectsByName;
+ private ConcurrentMap<String, O> objectsByName;
/**
* We need to remember which class loaders have already been used in order
to avoid instantiating factories multiple
@@ -59,10 +59,8 @@
// Save the factory interface class:
this.factoryInterface = factoryInterface;
- // Create the indexes for factories and objects (note that these don't
need to be concurrent hash maps because
- // we use the copy on write technique to avoid synchronization issues):
- factoriesByType = new HashMap<String, Factory<O>>();
- objectsByName = new HashMap<String, O>();
+ factoriesByType = new ConcurrentHashMap<String, Factory<O>>();
+ objectsByName = new ConcurrentHashMap<String, O>();
// Create the set of already use class visitedClassLoaders:
visitedClassLoaders = new HashSet<ClassLoader>();
@@ -74,10 +72,8 @@
*
* @param factory the factory to register
*/
- public synchronized void registerFactory(Factory<O> factory) {
- HashMap<String, Factory<O>> newFactories = new HashMap<String,
Factory<O>>(factoriesByType);
- newFactories.put(factory.getType(), factory);
- factoriesByType = newFactories;
+ public void registerFactory(Factory<O> factory) {
+ factoriesByType.put(factory.getType(), factory);
}
/**
@@ -202,10 +198,12 @@
// the files are processed, so it is better to sort them so that
objects will always be created in the same
// order regardless of how the filesystem decides to store the entries
of the directory:
File[] files = directory.listFiles();
- sort(files);
- for (File file : files) {
- if (file.getName().endsWith(".conf")) {
- loadFile(file);
+ if (files != null) {
+ sort(files);
+ for (File file : files) {
+ if (file.getName().endsWith(".conf")) {
+ loadFile(file);
+ }
}
}
}
@@ -230,8 +228,8 @@
}
// Check if the object has been explicitly disabled, if it is then
return immediately:
- Boolean enabled = config.getBoolean(ENABLED_PARAMETER);
- if (enabled != null && !enabled.booleanValue()) {
+ Boolean enabled = config.getBoolean(ENABLED_PARAMETER, false);
+ if (!enabled.booleanValue()) {
return;
}
@@ -342,16 +340,14 @@
* @param name th ename of the object to register
* @param object the object to register
*/
- protected synchronized void registerObject(String name, O object) {
- HashMap<String, O> newObjects = new HashMap<String, O>(objectsByName);
- newObjects.put(name, object);
- objectsByName = newObjects;
+ protected void registerObject(String name, O object) {
+ objectsByName.put(name, object);
}
/**
* Forget all the factories and objects.
*/
- public synchronized void clear() {
+ public void clear() {
factoriesByType.clear();
objectsByName.clear();
}
diff --git
a/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/authentication/ConfigurationTest.java
b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/authentication/ConfigurationTest.java
index 34e2c74..826bd2d 100644
---
a/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/authentication/ConfigurationTest.java
+++
b/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/authentication/ConfigurationTest.java
@@ -95,11 +95,11 @@
@Test
public void testGetFile() throws Exception {
File file = writeConf(
- "file=/file"
+ "file=file"
);
Configuration config = Configuration.loadFile(file);
assertNotNull(config);
- assertEquals(new File("/file"), config.getFile("file"));
+ assertEquals(new File("file"), config.getFile("file"));
}
/**
@@ -347,13 +347,13 @@
@Test
public void testTypedFile() throws Exception {
File file = writeConf(
- "a=/b"
+ "a=b"
);
Configuration config = Configuration.loadFile(file);
assertNotNull(config);
TypedFile view = config.getView(TypedFile.class);
assertNotNull(view);
- assertEquals(new File("/b"), view.getA());
+ assertEquals(new File("b"), view.getA());
}
public interface TypedFile {
--
To view, visit http://gerrit.ovirt.org/23787
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I57ff7df3075d4ca36a50016c6786ca8a69cee3e0
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
