Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 2: (16 comments) http://gerrit.ovirt.org/#/c/36119/2/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/AuthenticationUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/AuthenticationUtils.java: Line 10: import org.ovirt.engine.api.extensions.aaa.Mapping; Line 11: import org.ovirt.engine.core.aaa.AcctUtils; Line 12: import org.ovirt.engine.core.aaa.AuthType; Line 13: import org.ovirt.engine.core.aaa.AuthenticationProfile; Line 14: import org.ovirt.engine.core.aaa.AuthenticationProfileRepository; I would like to avoid using these two^ Line 15: import org.ovirt.engine.core.aaa.AuthzUtils; Line 16: import org.ovirt.engine.core.aaa.DirectoryGroup; Line 17: import org.ovirt.engine.core.aaa.DirectoryUser; Line 18: import org.ovirt.engine.core.aaa.filters.FiltersHelper; Line 11: import org.ovirt.engine.core.aaa.AcctUtils; Line 12: import org.ovirt.engine.core.aaa.AuthType; Line 13: import org.ovirt.engine.core.aaa.AuthenticationProfile; Line 14: import org.ovirt.engine.core.aaa.AuthenticationProfileRepository; Line 15: import org.ovirt.engine.core.aaa.AuthzUtils; this one as well is probably bad for us^ Line 16: import org.ovirt.engine.core.aaa.DirectoryGroup; Line 17: import org.ovirt.engine.core.aaa.DirectoryUser; Line 18: import org.ovirt.engine.core.aaa.filters.FiltersHelper; Line 19: import org.ovirt.engine.core.extensions.mgr.ExtensionProxy; Line 13: import org.ovirt.engine.core.aaa.AuthenticationProfile; Line 14: import org.ovirt.engine.core.aaa.AuthenticationProfileRepository; Line 15: import org.ovirt.engine.core.aaa.AuthzUtils; Line 16: import org.ovirt.engine.core.aaa.DirectoryGroup; Line 17: import org.ovirt.engine.core.aaa.DirectoryUser; these two are not needed^ we can just use the ExtMap Line 18: import org.ovirt.engine.core.aaa.filters.FiltersHelper; Line 19: import org.ovirt.engine.core.extensions.mgr.ExtensionProxy; Line 20: import org.slf4j.Logger; Line 21: import org.slf4j.LoggerFactory; Line 14: import org.ovirt.engine.core.aaa.AuthenticationProfileRepository; Line 15: import org.ovirt.engine.core.aaa.AuthzUtils; Line 16: import org.ovirt.engine.core.aaa.DirectoryGroup; Line 17: import org.ovirt.engine.core.aaa.DirectoryUser; Line 18: import org.ovirt.engine.core.aaa.filters.FiltersHelper; we do not need the filters in this implementation (yet). Line 19: import org.ovirt.engine.core.extensions.mgr.ExtensionProxy; Line 20: import org.slf4j.Logger; Line 21: import org.slf4j.LoggerFactory; Line 22: Line 152: } Line 153: return accumulator; Line 154: } Line 155: Line 156: public static DirectoryUser mapPrincipalRecordToDirectoryUser(final String authzName, final ExtMap principalRecord) { we do not need to map, just keep use ExtMap Line 157: DirectoryUser directoryUser = null; Line 158: if (principalRecord != null) { Line 159: directoryUser = new DirectoryUser( Line 160: authzName, Line 180: } Line 181: return directoryUser; Line 182: } Line 183: Line 184: public static DirectoryGroup mapGroupRecordToDirectoryGroup(final String authzName, final ExtMap group) { same Line 185: DirectoryGroup directoryGroup = null; Line 186: if (group != null) { Line 187: directoryGroup = new DirectoryGroup( Line 188: authzName, http://gerrit.ovirt.org/#/c/36119/2/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUser.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUser.java: Line 6: import java.util.Collection; Line 7: import java.util.Collections; Line 8: import java.util.HashSet; Line 9: Line 10: public class SSOUser { why do we need this class? can't we just use the ExtMap? Line 11: Line 12: private String externalId; Line 13: Line 14: private String domain; http://gerrit.ovirt.org/#/c/36119/2/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUtils.java: Line 11: public class SSOUtils { Line 12: Line 13: public static void redirectToModule(SSOUser ssoUser, HttpServletResponse response, String module) Line 14: throws IOException { Line 15: StringBuilder redirectUrl = new StringBuilder("/ovirt-engine/"); this can be relative Line 16: redirectUrl.append(module); Line 17: redirectUrl.append("/login?"); Line 18: Line 19: StringBuilder params = new StringBuilder(""); Line 16: redirectUrl.append(module); Line 17: redirectUrl.append("/login?"); Line 18: Line 19: StringBuilder params = new StringBuilder(""); Line 20: appendParameter(params, "userExternalId", ssoUser.getExternalId()); userId the fact that engine has several ids is irrelevant... Line 21: appendParameter(params, "username", ssoUser.getLoginName()); Line 22: appendParameter(params, "domain", ssoUser.getDomain()); Line 23: appendParameter(params, "email", ssoUser.getEmail()); Line 24: appendParameter(params, "groupIds", StringUtils.join(ssoUser.getGroupIds(), ",")); Line 17: redirectUrl.append("/login?"); Line 18: Line 19: StringBuilder params = new StringBuilder(""); Line 20: appendParameter(params, "userExternalId", ssoUser.getExternalId()); Line 21: appendParameter(params, "username", ssoUser.getLoginName()); this should be user name Line 22: appendParameter(params, "domain", ssoUser.getDomain()); Line 23: appendParameter(params, "email", ssoUser.getEmail()); Line 24: appendParameter(params, "groupIds", StringUtils.join(ssoUser.getGroupIds(), ",")); Line 25: redirectUrl.append(params.toString()); Line 18: Line 19: StringBuilder params = new StringBuilder(""); Line 20: appendParameter(params, "userExternalId", ssoUser.getExternalId()); Line 21: appendParameter(params, "username", ssoUser.getLoginName()); Line 22: appendParameter(params, "domain", ssoUser.getDomain()); please use the consistent aaa term, this should be profile Line 23: appendParameter(params, "email", ssoUser.getEmail()); Line 24: appendParameter(params, "groupIds", StringUtils.join(ssoUser.getGroupIds(), ",")); Line 25: redirectUrl.append(params.toString()); Line 26: response.sendRedirect(redirectUrl.toString()); Line 20: appendParameter(params, "userExternalId", ssoUser.getExternalId()); Line 21: appendParameter(params, "username", ssoUser.getLoginName()); Line 22: appendParameter(params, "domain", ssoUser.getDomain()); Line 23: appendParameter(params, "email", ssoUser.getEmail()); Line 24: appendParameter(params, "groupIds", StringUtils.join(ssoUser.getGroupIds(), ",")); we cannot know if group id can have comma or not, so better to encode them or use some other format. Line 25: redirectUrl.append(params.toString()); Line 26: response.sendRedirect(redirectUrl.toString()); Line 27: } Line 28: Line 21: appendParameter(params, "username", ssoUser.getLoginName()); Line 22: appendParameter(params, "domain", ssoUser.getDomain()); Line 23: appendParameter(params, "email", ssoUser.getEmail()); Line 24: appendParameter(params, "groupIds", StringUtils.join(ssoUser.getGroupIds(), ",")); Line 25: redirectUrl.append(params.toString()); I believe you need to encode each. or better have json/xml and encode it into single parameter. Line 26: response.sendRedirect(redirectUrl.toString()); Line 27: } Line 28: Line 29: private static void appendParameter(StringBuilder params, String name, String value) { Line 27: } Line 28: Line 29: private static void appendParameter(StringBuilder params, String name, String value) { Line 30: if (params.length() > 0) { Line 31: params.append("&"); you can add & at start as well, nothing will happen. Line 32: } Line 33: params.append(name); Line 34: params.append("="); Line 35: params.append(value); Line 44: throws ServletException, IOException { Line 45: forwardRequest(request, response, "/WEB-INF/login.jsp"); Line 46: } Line 47: Line 48: public static void forwardToLoginPageWithMessage(HttpServletRequest request, HttpServletResponse response, String msg) well, if you have message parameter, then probably this what you mean :) but I really do not like these wrappers... these are one liners useless utilities... why not just do whatever required in code. Line 49: throws ServletException, IOException { Line 50: forwardRequest(request, response, "/WEB-INF/login.jsp?msg="+msg); Line 51: } Line 52: Line 53: private static void forwardRequest(HttpServletRequest request, HttpServletResponse response, String uri) Line 54: throws ServletException, IOException { Line 55: RequestDispatcher dispatcher = request.getRequestDispatcher(uri); Line 56: response.setContentType("text/html;charset=UTF-8"); Line 57: if (dispatcher != null) { and if null? raise exception? Line 58: dispatcher.include(request, response); Line 59: } Line 60: } Line 61: -- 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: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@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
