Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: engine: Integrate Atlassian Crowd Client as a new
Authentication Domain
......................................................................
Patch Set 1: (11 inline comments)
Mostly style comments.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/CrowdBrokerUtils.java
Line 35:
Line 36: public class CrowdBrokerUtils {
Line 37:
Line 38: /* have we previously init'ed Crowd? */
Line 39: private static boolean crowdok = false;
Why a boolean if it ran instead of checking m_CrowdClient for null? Just use a
local variable while setting it up and only when done assign the static var.
Line 40:
Line 41: private static Log log = LogFactory.getLog(CrowdBrokerUtils.class);
Line 42: /* The Couwd Client */
Line 43: static CrowdClient m_CrowdClient;
Line 85: retVal.setName(user.getDisplayName());
Line 86: retVal.setUserName(user.getName());
Line 87: retVal.setEmail(user.getEmailAddress());
Line 88: /* All Crowd Based Users live in the "Crowd" domain */
Line 89: retVal.setDomainControler("Crowd");
This magic string, is it the same used in LdapBrokerUtils and LdapFactory?
Line 90: /* Crowd doesn't have a something like a Guid, so the
following code
Line 91: * checks if we have stored a previous Guid, and if not,
create a new one
Line 92: * and update a Crowd User Attribute called oVirtGuid with it
so
Line 93: * next time we retrive this user, the Guid is stable
Line 272: * the crowd console.
Line 273: */
Line 274: public static List<AdUser> getAllUsers() {
Line 275: List<AdUser> users = new ArrayList<AdUser>();
Line 276: List<User> cusers = new ArrayList<User>();
I prefer a clear variable name. So crowd_users would be preferred.
Line 277: initCrowd();
Line 278:
Line 279: try {
Line 280: /* search for all users */
Line 281: cusers = m_CrowdClient.searchUsers(new NullRestriction()
{}, 0, 200);
Line 282: } catch (Exception e) {
Line 283: log.errorFormat("Crowd Search Failed: {0}",
e.getMessage());
Line 284: }
Line 285: for (User un : cusers) {
I'd prefer crowd_user instead of un.
Line 286: if (un.isActive()) {
Line 287: AdUser ovirtuser = convertCrowdtoAdUser(un);
Line 288: users.add(ovirtuser);
Line 289: }
Line 283: log.errorFormat("Crowd Search Failed: {0}",
e.getMessage());
Line 284: }
Line 285: for (User un : cusers) {
Line 286: if (un.isActive()) {
Line 287: AdUser ovirtuser = convertCrowdtoAdUser(un);
Maybe aduser instead of ovirtuser? Or just user since you're adding it to users.
Line 288: users.add(ovirtuser);
Line 289: }
Line 290: }
Line 291: return users;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/CrowdGetAdGroupByGroupIdCommand.java
Line 19: if (group != null) {
Line 20: setSucceeded(true);
Line 21: } else {
Line 22: setSucceeded(false);
Line 23: }
Personally I always write this as setSucceeded(group != null).
Line 24: }
Line 25:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/CrowdGetAdUserByUserIdCommand.java
Line 20: setSucceeded(true);
Line 21: setReturnValue(user);
Line 22: } else {
Line 23: setSucceeded(false);
Line 24: }
Would be nice if you wrote this the same as the previous query
(CrowdGetAdGroupByGroupIdCommand):
setReturnValue(user);
setSucceeded(user != null);
Any particular reason you didn't?
Line 25:
Line 26: }
Line 27:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/CrowdGetAdUserByUserNameCommand.java
Line 19: setSucceeded(true);
Line 20: setReturnValue(user);
Line 21: } else {
Line 22: setSucceeded(false);
Line 23: }
Same comment as CrowdGetAdGroupByGroupIdCommand.ExecuteQuery. Though now I see
this maybe CrowdGetAdGroupByGroupIdCommand is the odd one out?
Line 24:
Line 25: }
Line 26:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LdapBrokerUtils.java
Line 54: }
Line 55: if (!filterInternalDomain) {
Line 56: results.add(Config.<String>
GetValue(ConfigValues.AdminDomain).trim());
Line 57: /* Only add the Crowd Domain if it can initilize correctly
*/
Line 58: if (CrowdBrokerUtils.initCrowd())
I think the coding style is to include the braces, also for single line
statements.
Line 59: results.add("Crowd");
Line 60: }
Line 61: return results;
Line 62: }
Line 55: if (!filterInternalDomain) {
Line 56: results.add(Config.<String>
GetValue(ConfigValues.AdminDomain).trim());
Line 57: /* Only add the Crowd Domain if it can initilize correctly
*/
Line 58: if (CrowdBrokerUtils.initCrowd())
Line 59: results.add("Crowd");
I'd like to see this as some static somewhere instead of a magic string.
Line 60: }
Line 61: return results;
Line 62: }
Line 63:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LdapFactory.java
Line 20:
Line 21: public static LdapBroker getInstance(String domain) {
Line 22: if (domain.equalsIgnoreCase(internalDomain)) {
Line 23: return internalInstance;
Line 24: } else if (domain.equalsIgnoreCase("Crowd")) {
Same here about a static instead of a magic string.
Line 25: return crowdInstance;
Line 26: } else {
Line 27: return ldapInstance;
Line 28: }
--
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: Ewoud Kohl van Wijngaarden <[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
