Ravi Nori has posted comments on this change. Change subject: core, webadmin: Modify webadmin to use enginesso for authentication ......................................................................
Patch Set 3: (8 comments) http://gerrit.ovirt.org/#/c/36619/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/LogoutUserCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/LogoutUserCommand.java: Line 44: } Line 45: SessionDataContainer.getInstance().removeSessionOnLogout(getParameters().getSessionId()); Line 46: } Line 47: Line 48: if (SessionDataContainer.getInstance().getUser(getParameters().getSessionId(), false) != null) { > I am unsure why logout is to be changed within this scope. In the new sso sequence the session in SessionDataContained contains only user info. The Authn is not set, so when the Logout is initiated the method SessionDataContainer.getInstance().removeSessionOnLogout is not invoked. So in this case we check is the user still exists for the session and remove the session from SessionDataContainer Line 49: SessionDataContainer.getInstance().removeSessionOnLogout(getParameters().getSessionId()); Line 50: } Line 51: setSucceeded(true); Line 52: } http://gerrit.ovirt.org/#/c/36619/3/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOLoginFilter.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOLoginFilter.java: Line 26: throw new RuntimeException("No login-url init parameter specified for SSOLoginFilter."); Line 27: } else { Line 28: loginUrl = strVal; Line 29: } Line 30: strVal = filterConfig.getInitParameter("post-login-url"); > why do we need the post login url here? all we need is to know where login Done Line 31: if (strVal == null) { Line 32: throw new RuntimeException("No post_login_url init parameter specified for SSOLoginFilter."); Line 33: } else { Line 34: postLoginUrl = strVal; Line 42: if (requestUrl == null) { Line 43: String queryString = ((HttpServletRequest) request).getQueryString(); Line 44: requestUrl = ((HttpServletRequest) request).getRequestURI() + Line 45: (StringUtils.isEmpty(queryString) ? "" : "?" + queryString); Line 46: } > why do we need app_url here? this is sso parameter. Done Line 47: Line 48: HttpSession session = ((HttpServletRequest) request).getSession(false); Line 49: if (session == null || Line 50: session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY) == null || Line 47: Line 48: HttpSession session = ((HttpServletRequest) request).getSession(false); Line 49: if (session == null || Line 50: session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY) == null || Line 51: !SSOUtils.isSessionValid((String) session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY))) { > why not use the: Done Line 52: request.getRequestDispatcher(loginUrl + "&app_url=" + ((HttpServletResponse) response).encodeURL(requestUrl)).forward(request, response); Line 53: } else { Line 54: chain.doFilter(request, response); Line 55: } Line 48: HttpSession session = ((HttpServletRequest) request).getSession(false); Line 49: if (session == null || Line 50: session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY) == null || Line 51: !SSOUtils.isSessionValid((String) session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY))) { Line 52: request.getRequestDispatcher(loginUrl + "&app_url=" + ((HttpServletResponse) response).encodeURL(requestUrl)).forward(request, response); > I must ask why not redirect... If we redirect the URL will change, I am not a big fan of that Line 53: } else { Line 54: chain.doFilter(request, response); Line 55: } Line 56: } http://gerrit.ovirt.org/#/c/36619/3/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOLoginServlet.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOLoginServlet.java: Line 41: String appUrlParam = "&app_url=" + response.encodeURL(applicationUrl); Line 42: String postProcessLoginUrlParam = "&post_login_url=" + response.encodeURL(postLoginUrl); Line 43: Line 44: HttpSession session = request.getSession(false); Line 45: if (session == null || session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY) == null) { > you do not need to check anything, worse case the sso will have session and Done Line 46: response.sendRedirect(ssoUrl + appUrlParam + postProcessLoginUrlParam); Line 47: } else if (!SSOUtils.isSessionValid((String) session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY))) { Line 48: response.sendRedirect(ssoReauthenticateUrl + appUrlParam + postProcessLoginUrlParam); Line 49: } Line 42: String postProcessLoginUrlParam = "&post_login_url=" + response.encodeURL(postLoginUrl); Line 43: Line 44: HttpSession session = request.getSession(false); Line 45: if (session == null || session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY) == null) { Line 46: response.sendRedirect(ssoUrl + appUrlParam + postProcessLoginUrlParam); > just String.format() here all parameters, you do not need the appUrl..., po Done Line 47: } else if (!SSOUtils.isSessionValid((String) session.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY))) { Line 48: response.sendRedirect(ssoReauthenticateUrl + appUrlParam + postProcessLoginUrlParam); Line 49: } Line 50: } http://gerrit.ovirt.org/#/c/36619/3/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOPostLoginServlet.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOPostLoginServlet.java: Line 41: String username = request.getParameter("username"); Line 42: String email = request.getParameter("email"); Line 43: String[] groupIds = request.getParameterValues("groupId"); Line 44: if (groupIds == null) { Line 45: groupIds = new String[0]; > = "" ? Can be removed, not needed Line 46: } Line 47: try { Line 48: SSOUtils.checkUserAndGroupsAuthorization(userId, username, groupIds); Line 49: boolean isAdmin = SSOUtils.isAdminUser(userId, groupIds); -- 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: 3 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
