Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 8: (10 comments) ok, much better! waiting for you to remove legacy providers so I can see what we are left with. http://gerrit.ovirt.org/#/c/36119/8/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: Line 96: SSOUserManager.getInstance().getUserIdByExternalId( Line 97: (String) authz.getContext().get(Base.ContextKeys.INSTANCE_NAME), Line 98: (String) ((ExtMap) output.get(Authz.InvokeKeys.PRINCIPAL_RECORD)).get(Authz.PrincipalRecord.ID))); Line 99: } else { Line 100: throw new AuthenticationError(AuthnMessageMapper.mapMessageErrorCode(outputMap)); pattern is always: if (fail) { throw exception } continue as usual Line 101: } Line 102: } Line 103: http://gerrit.ovirt.org/#/c/36119/8/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/AuthnMessageMapper.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/AuthnMessageMapper.java: Line 12: public static final String mapMessageErrorCode(ExtMap outputMap) { Line 13: String msg = USER_FAILED_TO_AUTHENTICATE; Line 14: int authResult = outputMap.<Integer>get(Authn.InvokeKeys.RESULT); Line 15: if (authResult == Authn.AuthResult.CREDENTIALS_EXPIRED) { Line 16: boolean addedUserPasswordExpiredCDA = false; what is CDA? Line 17: if (outputMap.<String> get(Authn.InvokeKeys.CREDENTIALS_CHANGE_URL) != null) { Line 18: msg = String.format(USER_PASSWORD_EXPIRED_CHANGE_URL_PROVIDED, Line 19: outputMap.<String>get(Authn.InvokeKeys.CREDENTIALS_CHANGE_URL)); Line 20: addedUserPasswordExpiredCDA = true; http://gerrit.ovirt.org/#/c/36119/8/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: this can be much simpler once the legacy providers are dropped. Line 1: package org.ovirt.engine.core.sso.utils; Line 2: Line 3: import org.apache.commons.lang.StringUtils; Line 4: import org.ovirt.engine.core.common.businessentities.VdcOption; http://gerrit.ovirt.org/#/c/36119/8/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: Line 33: ResultSet rs = null; Line 34: String userId = null; Line 35: try { Line 36: connection = ds.getConnection(); Line 37: ps = connection.prepareStatement("SELECT users.* FROM users WHERE domain = ? AND external_id = ?"); why not ask for what we actually need? Line 38: ps.setString(1, domain); Line 39: ps.setString(2, externalId); Line 40: Line 41: rs = ps.executeQuery(); Line 55: } Line 56: return uuid.toString(); Line 57: } Line 58: Line 59: public String getGroupByExternalId(String domain, String externalId) throws SQLException { domain -> authzName all over please, the fact that the database column is domain should not be exposed. Line 60: Connection connection = null; Line 61: PreparedStatement ps = null; Line 62: ResultSet rs = null; Line 63: String groupId = null; Line 63: String groupId = null; Line 64: try { Line 65: connection = ds.getConnection(); Line 66: ps = connection.prepareStatement( Line 67: "SELECT * FROM ad_groups WHERE domain = ? AND external_id = ?"); same, instead of * ask for what you need. Line 68: ps.setString(1, domain); Line 69: ps.setString(2, externalId); Line 70: Line 71: rs = ps.executeQuery(); http://gerrit.ovirt.org/#/c/36119/8/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 41: } Line 42: Line 43: StringBuilder params = new StringBuilder(""); Line 44: appendParameter(params, "id", userId); Line 45: appendParameter(params, "externalId", principalRecord.<String>get(Authz.PrincipalRecord.ID)); we should not forward external id as far as I understand. Line 46: appendParameter(params, "domain", profile); Line 47: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 48: appendParameter(params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 49: appendParameter(params, "firstname", principalRecord.<String>get(Authz.PrincipalRecord.FIRST_NAME)); Line 42: Line 43: StringBuilder params = new StringBuilder(""); Line 44: appendParameter(params, "id", userId); Line 45: appendParameter(params, "externalId", principalRecord.<String>get(Authz.PrincipalRecord.ID)); Line 46: appendParameter(params, "domain", profile); we should not forward authz/profile as far as I understand. Line 47: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 48: appendParameter(params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 49: appendParameter(params, "firstname", principalRecord.<String>get(Authz.PrincipalRecord.FIRST_NAME)); Line 50: appendParameter(params, "lastname", principalRecord.<String>get(Authz.PrincipalRecord.LAST_NAME)); Line 47: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 48: appendParameter(params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 49: appendParameter(params, "firstname", principalRecord.<String>get(Authz.PrincipalRecord.FIRST_NAME)); Line 50: appendParameter(params, "lastname", principalRecord.<String>get(Authz.PrincipalRecord.LAST_NAME)); Line 51: appendParameter(params, "department", principalRecord.<String>get(Authz.PrincipalRecord.DEPARTMENT)); these 3 are not important either. Line 52: appendParameter(params, "email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 53: appendParameter(params, "groupIds", StringUtils.join(groupIds, ",")); Line 54: redirectUrl.append(params.toString()); Line 55: response.sendRedirect(redirectUrl.toString()); Line 51: appendParameter(params, "department", principalRecord.<String>get(Authz.PrincipalRecord.DEPARTMENT)); Line 52: appendParameter(params, "email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 53: appendParameter(params, "groupIds", StringUtils.join(groupIds, ",")); Line 54: redirectUrl.append(params.toString()); Line 55: response.sendRedirect(redirectUrl.toString()); you must url encode each parameter. Line 56: } Line 57: Line 58: private static void appendParameter(StringBuilder params, String name, String value) { Line 59: if (StringUtils.isNotEmpty(value)) { -- 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: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
