Alon Bar-Lev has posted comments on this change. Change subject: aaa: add support for basic athentication ......................................................................
Patch Set 6: (8 comments) good, minor comments. next phase is to make these options we have configurable, using the shell like config in uutils. http://gerrit.ovirt.org/#/c/37299/6/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/BasicAuthServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/BasicAuthServlet.java: Line 30: request.getSession(true).setAttribute(SSOUtils.USER_CREDENTIALS, credentials); Line 31: response.sendRedirect(request.getContextPath() + SSOUtils.LOGIN_PHASE2_URI); Line 32: } else if (enableBasicAuth) { Line 33: response.setHeader("WWW-Authenticate", "Basic realm=\"" + realm + "\""); Line 34: response.sendError(HttpServletResponse.SC_UNAUTHORIZED); I still unsure what happens when you press ESC after 1st attempt... from my understanding you keep get 401. can you confirm? Line 35: } else { Line 36: response.sendRedirect(request.getContextPath() + SSOUtils.LOGIN_PHASE2_URI); Line 37: } Line 38: } http://gerrit.ovirt.org/#/c/37299/6/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOServlet.java: this rename should be in base patch http://gerrit.ovirt.org/#/c/37299/6/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase1Servlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase1Servlet.java: Line 23: boolean acceptBasicAuth = (boolean) request.getSession().getServletContext().getAttribute(SSOUtils.ACCEPT_BASIC_AUTH_HEADERS); Line 24: request.getSession(true).setAttribute(SSOUtils.PARAMS_MAP, new HashMap<>(request.getParameterMap())); Line 25: Line 26: if (acceptBasicAuth) { Line 27: response.sendRedirect(request.getContextPath() + SSOUtils.LOGIN_EXTERNAL_URI); redirect to external should be done if external auth is enabled, you have this flag. if not we can redirect to other url that does the same but is not monitored by apache. Line 28: } else { Line 29: response.sendRedirect(request.getContextPath() + SSOUtils.LOGIN_PHASE2_URI); Line 30: } Line 31: } http://gerrit.ovirt.org/#/c/37299/6/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase2Servlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase2Servlet.java: Line 30: protected void service(HttpServletRequest request, HttpServletResponse response) Line 31: throws ServletException, IOException { Line 32: if (StringUtils.isEmpty(SSOUtils.getParameter(request, SSOUtils.POST_ACTION_URL))) { Line 33: throw new RuntimeException("No post action url found in request."); Line 34: } I am unsure we need this, as we checked this in phase1 already, no? Line 35: HttpSession session = request.getSession(true); Line 36: if (SSOUtils.isUserAuthenticated(session)) { Line 37: request.getRequestDispatcher(SSOUtils.LOGIN_PHASE3_URI).forward(request, response); Line 38: } else { Line 35: HttpSession session = request.getSession(true); Line 36: if (SSOUtils.isUserAuthenticated(session)) { Line 37: request.getRequestDispatcher(SSOUtils.LOGIN_PHASE3_URI).forward(request, response); Line 38: } else { Line 39: authenticateUser(session, request, response); I am unsure why you split this function, easier to follow the entire sequence of redirect in one place. at least move the finally and catch here so the entire logic will be at one place. Line 40: } Line 41: } Line 42: Line 43: private boolean containsUserCredentialsInParams(Map<String, String[]> paramMap) { Line 39: authenticateUser(session, request, response); Line 40: } Line 41: } Line 42: Line 43: private boolean containsUserCredentialsInParams(Map<String, String[]> paramMap) { not sure what the advantage of one time usage one line function... you can collapse it within the caller. Line 44: return paramMap.containsKey(USERNAME) && paramMap.containsKey(PASSWORD) && paramMap.containsKey(PROFILE); Line 45: } Line 46: Line 47: private void authenticateUser(HttpSession session, HttpServletRequest request, HttpServletResponse response) Line 64: } catch (SQLException ex) { Line 65: log.error("Internal Database Error", ex); Line 66: throw new AuthenticationException("Internal Database Error", ex); Line 67: } Line 68: } catch (AuthenticationException ex) { any reason we keep the AuthenticationException class? what is the advantage of usage? Line 69: request.getRequestDispatcher("/WEB-INF/login.jsp?msg=" + ex.getMessage()).forward(request, response); Line 70: } finally { Line 71: request.getSession().removeAttribute(SSOUtils.USER_CREDENTIALS); Line 72: } http://gerrit.ovirt.org/#/c/37299/6/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase3Servlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase3Servlet.java: Line 18: protected void service(HttpServletRequest request, HttpServletResponse response) Line 19: throws ServletException, IOException { Line 20: if (StringUtils.isEmpty(request.getParameter(SSOUtils.POST_ACTION_URL))) { Line 21: throw new RuntimeException("No post action url found in request."); Line 22: } here too... I think we do not need this as we already checked it at phase1 Line 23: HttpSession session = request.getSession(true); Line 24: if (SSOUtils.isUserAuthenticated(session)) { Line 25: SSOUtils.redirectToModule( Line 26: session, -- To view, visit http://gerrit.ovirt.org/37299 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If09285f0e6cd8909f21aa7e88ae1a3c1a30763c2 Gerrit-PatchSet: 6 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
