Alon Bar-Lev has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 56: (5 comments) https://gerrit.ovirt.org/#/c/36119/56/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceBatchServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceBatchServlet.java: Line 265: String jsonPayload = SSOUtils.getJson(payload); Line 266: response.setContentType("application/json"); Line 267: response.setContentLength(jsonPayload.length()); Line 268: os.write(jsonPayload.getBytes("UTF-8")); Line 269: } please trace the output. Line 270: } https://gerrit.ovirt.org/#/c/36119/56/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceInteractiveServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceInteractiveServlet.java: most comments from batch servlet apply Line 1: package org.ovirt.engine.core.sso.servlets; Line 2: Line 3: import org.apache.commons.lang.StringUtils; Line 4: import org.ovirt.engine.api.extensions.Base; Line 104: Command cmd = commandsMap.get(command); Line 105: if (cmd == null) { Line 106: log.error("Unknown command found in request: {} ", command); Line 107: throw new RuntimeException(String.format("Unknown command found in request %s", command)); Line 108: } else { no else in exceptional pattern. Line 109: cmd.execute(request, response); Line 110: } Line 111: } Line 112: Line 123: "0".equals(externalParamValue)) { Line 124: String msg = "Switch user is not permitted"; Line 125: Map<String, String> userInfo = SSOUtils.getUserNameAndProfile(request); Line 126: response.sendRedirect(String.format("%s?msg=%s&sso_user=%s", config.getSsoErrorUrl(), response.encodeURL(msg), userInfo.get("userName")+"@" + userInfo.get("profile"))); Line 127: return; please avoid return at middle of functions. maybe at this interface, we can have login/logout/switch commands to make it easier to workout the sequence? Line 128: } Line 129: Line 130: if (StringUtils.isNotEmpty(request.getParameter("invalidate")) && !"0".equals(request.getParameter("invalidate"))) { Line 131: request.getSession(true).invalidate(); Line 188: public static void getSsoVersion(HttpServletResponse response) throws IOException, ServletException { Line 189: response.getWriter().println(SSOUtils.SSO_VERSION); Line 190: } Line 191: Line 192: private static void getToken(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { can we add query command instead of getSsoVersion and getToken? this can return json entity similar to the batch, we may even reuse the same entities. Line 193: if (StringUtils.isEmpty(request.getParameter(SSOUtils.POST_COMMAND_URL))) { Line 194: throw new RuntimeException("No post action url found in request."); Line 195: } Line 196: -- To view, visit https://gerrit.ovirt.org/36119 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4894fc12653027271b6abd4dd5313b10593703fa Gerrit-PatchSet: 56 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
