Alon Bar-Lev has posted comments on this change. Change subject: aaa: Modify webadmin and userportal to use enginesso for authentication ......................................................................
Patch Set 9: (5 comments) oh! you have done the ui part, nice!!!! next revision I will be able to test this. http://gerrit.ovirt.org/#/c/36619/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java: Line 48: || request.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY) != null; Line 49: } Line 50: Line 51: public static boolean isSessionValid(String session) throws NamingException { Line 52: InitialContext ctx = new InitialContext(); not sure it is healthy to open/close context multiple time if we have multiple backend commands. Line 53: try { Line 54: VdcQueryReturnValue returnValue = getBackend(ctx) Line 55: .runPublicQuery(VdcQueryType.ValidateSession, Line 56: new VdcQueryParametersBase(session)); http://gerrit.ovirt.org/#/c/36619/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSOLoginFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSOLoginFilter.java: Line 43: Line 44: if (!FiltersHelper.isAuthenticated(req)) { Line 45: ((HttpServletResponse) response).sendRedirect(String.format("%s%s&opaque=%s", req.getServletContext().getContextPath(), loginUrl, ((HttpServletResponse) response).encodeURL(requestUri))); Line 46: } else if (!isSessionValid(req)) { Line 47: ((HttpServletResponse) response).sendRedirect(String.format("%s%s&opaque=%s", req.getServletContext().getContextPath(), logoutUrl, ((HttpServletResponse) response).encodeURL(requestUri))); I am unsure I understand why logout is important, if we have no valid session we go to login - always. logout should be used only if user choose to logout. Line 48: } else { Line 49: chain.doFilter(request, response); Line 50: } Line 51: } Line 62: log.error("Unable to get reference to backend bean.", ex); Line 63: throw new RuntimeException("Unable to get reference to backend bean.", ex); Line 64: } Line 65: return isValid; Line 66: } I thought we already have a filter with this logic, see SessionMgmtFilter, all you need is to chain after it. Line 67: Line 68: @Override Line 69: public void destroy() { Line 70: } http://gerrit.ovirt.org/#/c/36619/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOPostLoginServlet.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOPostLoginServlet.java: Line 63: SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 64: queryRetVal.getActionReturnValue()); Line 65: } finally { Line 66: ctx.close(); Line 67: } again... the try try should be reduced to: try { } catch () { } finally { if (ctx != null) { try { ctx.close(); } catch (Naming... ) { log.error(); } } } Line 68: } catch (Exception ex) { Line 69: log.error("Exception creating user session", ex.getMessage()); Line 70: throw new RuntimeException("Exception creating user session", ex); Line 71: } Line 68: } catch (Exception ex) { Line 69: log.error("Exception creating user session", ex.getMessage()); Line 70: throw new RuntimeException("Exception creating user session", ex); Line 71: } Line 72: response.sendRedirect((String) payload.get("opaque")); this can go into the try/catch block, will be easier to follow. Line 73: } Line 74: -- To view, visit http://gerrit.ovirt.org/36619 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iff0aee9d0f5ee606ff7f397cab69017ca7d9df08 Gerrit-PatchSet: 9 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
