Juan Hernandez has posted comments on this change.
Change subject: engine: Integrate Atlassian Crowd Client as a new
Authentication Domain
......................................................................
Patch Set 1: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/CrowdBrokerUtils.java
Line 39: private static boolean crowdok = false;
Line 40:
Line 41: private static Log log = LogFactory.getLog(CrowdBrokerUtils.class);
Line 42: /* The Couwd Client */
Line 43: static CrowdClient m_CrowdClient;
This "m_" prefix is a bit strange, the rest of the codebase doesn't use this
kind of prefixes. Can you make it private and remove the prefix?
Line 44:
Line 45: /* Initialize the Crowd Libraries.
Line 46: * This will attempt to load the crowd.properties file and then
Line 47: * authenticate with the Crowd Server as a Application. If all is
good
Line 51: public static boolean initCrowd() {
Line 52: /* if we have already init'd Crowd, then don't do it again */
Line 53: if (crowdok == false) {
Line 54: Properties m_CrowdConfig = new Properties();
Line 55: File f = new File("/etc/ovirt-engine/",
"crowd.properties");
We try to make locations of files and directories configurable, to get
advantage of that just use the following to get the localtion of the
configuration directory:
final File etcDir = LocalConfig.getInstance().getEtcDir();
final File crowdConfigFile = new File(etcDir, "crowd.properties");
Line 56: try {
Line 57: m_CrowdConfig.load(new FileReader(f));
Line 58: } catch (Exception e) {
Line 59: log.warnFormat("Failed to load Crowd Configuration
from file {0}{1}: {2}",
Line 57: m_CrowdConfig.load(new FileReader(f));
Line 58: } catch (Exception e) {
Line 59: log.warnFormat("Failed to load Crowd Configuration
from file {0}{1}: {2}",
Line 60: "/tmp/",
Line 61: "crowd.properties",
I guess that you used /tmp/crowd.properties for some tests, but now it should
be something like crowdConfigFile.getAbsolutePath().
Line 62: e.getMessage());
Line 63: return false;
Line 64: }
Line 65: ClientPropertiesImpl crowdClientProperties =
ClientPropertiesImpl.newInstanceFromProperties(m_CrowdConfig);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/CrowdSearchGroupsByQueryCommand.java
Line 9: }
Line 10:
Line 11: @Override
Line 12: protected void ExecuteQuery() {
Line 13: java.util.List<ad_groups> groupList =
CrowdBrokerUtils.getAllGroups();
Correct me if I am wrong, but I think that this will return the complete list
of groups regardless of the search pattern that the user types in the search
dialog. That can kill the performance if there is a large list of groups.
Line 14: setReturnValue(groupList);
Line 15: setSucceeded(true);
Line 16: }
Line 17:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/CrowdSearchUserByQueryCommand.java
Line 11: }
Line 12:
Line 13: @Override
Line 14: protected void ExecuteQuery() {
Line 15: List<AdUser> userList = CrowdBrokerUtils.getAllUsers();
Same as with groups, this ignores the query that the user types. Is that on
purpose?
Line 16: setReturnValue(userList);
Line 17: setSucceeded(true);
Line 18: }
Line 19:
--
To view, visit http://gerrit.ovirt.org/9324
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide867f16d092eb329c0ce2fccf4ebd02f3aae0df
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Justin Hammond <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Justin Hammond <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
