Alon Bar-Lev has posted comments on this change. Change subject: aaa: add support for basic athentication ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/37299/1/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: Line 41: authenticateUser(session, request, response); Line 42: } Line 43: } Line 44: Line 45: private boolean containsUserCredentials(boolean basicAuth, HttpServletRequest request) { I am unsure why this function should change. if we have parameters, then we can ignore the basic, no? Line 46: return basicAuth ? Line 47: StringUtils.isNotEmpty(request.getHeader(HEADER_AUTHORIZATION)) : Line 48: request.getParameterMap().containsKey(USERNAME) && request.getParameterMap().containsKey(PASSWORD) && request.getParameterMap().containsKey(PROFILE); Line 49: } Line 52: throws ServletException, IOException { Line 53: boolean basicAuth = false; Line 54: try { Line 55: if (StringUtils.isNotEmpty(request.getParameter(BASIC_AUTH))) { Line 56: basicAuth = Boolean.parseBoolean(request.getParameter(BASIC_AUTH)); this should be in servlet init parameter, it is part of application configuration. Line 57: } Line 58: if (basicAuth && StringUtils.isEmpty(request.getHeader(HEADER_AUTHORIZATION))) { Line 59: throw new AuthenticationException("Unauthorized"); Line 60: } Line 73: log.error("Internal Database Error", ex); Line 74: throw new AuthenticationException("Internal Database Error", ex); Line 75: } Line 76: } catch (AuthenticationException ex) { Line 77: if (basicAuth) { if we have basic and not header, then we need to redirect to this servlet. else we need to redirect to sso (this) servlet. what I am trying to say is that we can simplify the logic by splitting this servlet into two. servlet1 if basic redirect to basic handling else redirect to login basic handling servlet if no authorization headers send 401 else accept credentials and store into session redirect to login login servlet if we have credentials in session use these otherwise if we have credentials in parameters use these instead of "redirect to login" you can also forward... same result I think. also, we should have another servlet of unauthorized, remember? so that if user press ESC in apache the apache can redirect to this unauthorized servlet redirect to login Line 78: request.getRequestDispatcher("/basic").forward(request, response); Line 79: } else { Line 80: request.getRequestDispatcher("/WEB-INF/login.jsp?msg=" + ex.getMessage()).forward(request, response); Line 81: } Line 93: } else { Line 94: credentials = new Credentials(); Line 95: credentials.username = request.getParameter(USERNAME); Line 96: credentials.password = request.getParameter(PASSWORD); Line 97: credentials.profile = request.getParameter(PROFILE); if we got explicit parameter we can ignore the headers. Line 98: } Line 99: return credentials; Line 100: } Line 101: Line 103: Credentials credentials = new Credentials(); Line 104: credentials.password = password; Line 105: int separator = user.lastIndexOf("@"); Line 106: if (separator != -1) { Line 107: credentials.profile = user.substring(separator + 1); only if profile exists, so that xxx@ddd will not be valid if profile is dropped? should be xxx@ddd@profile Line 108: credentials.username = user.substring(0, separator); Line 109: } else { Line 110: separator = user.indexOf("\\"); Line 111: if (separator != -1) { Line 106: if (separator != -1) { Line 107: credentials.profile = user.substring(separator + 1); Line 108: credentials.username = user.substring(0, separator); Line 109: } else { Line 110: separator = user.indexOf("\\"); this is not required, it is legacy for restapi only. Line 111: if (separator != -1) { Line 112: credentials.profile = user.substring(0, separator); Line 113: credentials.username = user.substring(separator + 1); Line 114: } -- 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: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
