Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 7: (9 comments) http://gerrit.ovirt.org/#/c/36119/7/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/AuthenticationUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/AuthenticationUtils.java: please see previous comment regarding single flow. Line 1: package org.ovirt.engine.core.sso.utils; Line 2: Line 3: import org.ovirt.engine.api.extensions.Base; Line 4: import org.ovirt.engine.api.extensions.ExtMap; http://gerrit.ovirt.org/#/c/36119/7/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/DBConfigUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/DBConfigUtils.java: Line 1: package org.ovirt.engine.core.sso.utils; can't we just load what we need without complexity? Line 2: Line 3: import org.apache.commons.lang.StringUtils; Line 4: import org.ovirt.engine.core.common.businessentities.VdcOption; Line 5: import org.ovirt.engine.core.common.config.ConfigCommon; http://gerrit.ovirt.org/#/c/36119/7/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOExtensionsManager.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOExtensionsManager.java: Line 36: "AdUserId", Line 37: "AdUserPassword", Line 38: "LDAPProviderTypes", Line 39: "LDAPSecurityAuthentication", Line 40: "LdapServers" why do we need these? oh! you want to support the legacy providers... now I understand why you used the engine manager! can we just ignore these legacy for now? probably better to have upgrade code to create proper extension files. Line 41: )); Line 42: Line 43: public static SSOExtensionsManager getInstance() { Line 44: if (instance == null) { http://gerrit.ovirt.org/#/c/36119/7/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOGroup.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOGroup.java: I do not think we need this class, we can use plain extmap of group record. Line 1: package org.ovirt.engine.core.sso.utils; Line 2: Line 3: import java.util.HashSet; Line 4: import java.util.Set; http://gerrit.ovirt.org/#/c/36119/7/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUser.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUser.java: why can't we just use extmap of principal record? Line 1: package org.ovirt.engine.core.sso.utils; Line 2: Line 3: import java.util.ArrayList; Line 4: import java.util.Collection; http://gerrit.ovirt.org/#/c/36119/7/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUserManager.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUserManager.java: I do not think we need the mapping, we can just use the extmap. Line 1: package org.ovirt.engine.core.sso.utils; Line 2: Line 3: import org.ovirt.engine.core.utils.ejb.ContainerManagedResourceType; Line 4: import org.ovirt.engine.core.utils.ejb.EjbUtils; http://gerrit.ovirt.org/#/c/36119/7/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java: Line 22: redirectUrl.append("/login?"); Line 23: Line 24: StringBuilder params = new StringBuilder(""); Line 25: appendParameter(params, "id", user.getId().toString()); Line 26: appendParameter(params, "externalId", user.getExternalId()); why do you need this? Line 27: appendParameter(params, "domain", user.getDomain()); Line 28: appendParameter(params, "username", user.getLoginName()); Line 29: appendParameter(params, "firstname", user.getFirstName()); Line 30: appendParameter(params, "lastname", user.getLastName()); Line 23: Line 24: StringBuilder params = new StringBuilder(""); Line 25: appendParameter(params, "id", user.getId().toString()); Line 26: appendParameter(params, "externalId", user.getExternalId()); Line 27: appendParameter(params, "domain", user.getDomain()); no domain please, profile. but why do we need this? Line 28: appendParameter(params, "username", user.getLoginName()); Line 29: appendParameter(params, "firstname", user.getFirstName()); Line 30: appendParameter(params, "lastname", user.getLastName()); Line 31: appendParameter(params, "department", user.getDepartment()); Line 27: appendParameter(params, "domain", user.getDomain()); Line 28: appendParameter(params, "username", user.getLoginName()); Line 29: appendParameter(params, "firstname", user.getFirstName()); Line 30: appendParameter(params, "lastname", user.getLastName()); Line 31: appendParameter(params, "department", user.getDepartment()); first, last, dep are not used Line 32: appendParameter(params, "email", user.getEmail()); Line 33: appendParameter(params, "groupIds", StringUtils.join(user.getGroupIds(), ",")); Line 34: redirectUrl.append(params.toString()); Line 35: response.sendRedirect(redirectUrl.toString()); -- To view, visit http://gerrit.ovirt.org/36119 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4894fc12653027271b6abd4dd5313b10593703fa Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
