Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 16: (1 comment) ok, one last note regarding this phase, now we need to see the engine side... :) http://gerrit.ovirt.org/#/c/36119/16/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSORedirectServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSORedirectServlet.java: Line 25: request.getRequestDispatcher("/WEB-INF/login.jsp").forward(request, response); Line 26: } Line 27: } Line 28: Line 29: private void handleAuthenticatedUser(HttpSession session, HttpServletRequest request, HttpServletResponse response) still I do not understand the value of having this function... why can't we have the entire logic of the servlet at one place? also doing validation of servlet parameters so deep within the logic is not best practice, as you first validate parameters, then proceed to logic. also the SQLException out of redirectToModule is not something that is expected, as it is not database related function. Line 30: throws ServletException, IOException { Line 31: String module = request.getParameter(SSOUtils.MODULE); Line 32: if (StringUtils.isEmpty(module)) { Line 33: throw new RuntimeException("No module name found in request."); -- 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: 16 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
