Alon Bar-Lev has posted comments on this change. Change subject: core, webadmin: Modify webadmin to use enginesso for authentication ......................................................................
Patch Set 3: (20 comments) ok, good, except for the permissions role manager which I need to review once I understand the rest. thanks! http://gerrit.ovirt.org/#/c/36619/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/CreateUserSessionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/CreateUserSessionCommand.java: Line 21: String engineSessionId = null; Line 22: try { Line 23: byte s[] = new byte[64]; Line 24: SecureRandom.getInstance("SHA1PRNG").nextBytes(s); Line 25: engineSessionId = new Base64(0).encodeToString(s); we need one place to have the logic of generate session identifier... this is security sensitive. we can do this within the SessionDataContainer instance. another option is to use this command also in the normal login flow. Line 26: } catch (NoSuchAlgorithmException e) { Line 27: throw new RuntimeException(e); Line 28: } Line 29: SessionDataContainer.getInstance().setUser(engineSessionId, getParameters().getUser()); 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. 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 is 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. 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: FiltersHelper.isAuthenticated(httpreq) 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... 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 it will redirect back... so you can redirect unconditionally. 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..., postProc... temp variables. 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]; = "" ? Line 46: } Line 47: try { Line 48: SSOUtils.checkUserAndGroupsAuthorization(userId, username, groupIds); Line 49: boolean isAdmin = SSOUtils.isAdminUser(userId, groupIds); Line 53: } Line 54: HttpSession httpSession = request.getSession(true); Line 55: httpSession.setAttribute( Line 56: SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 57: SSOUtils.attachUserToSession(userId, username, email, groupIds, isAdmin)); if this is SSOUtils you can just provide the request so it will take whatever it wants... Line 58: } catch (SSOAuthorizationException ex) { Line 59: log.error(ex.getMessage()); Line 60: throw new RuntimeException(ex); Line 61: } Line 55: httpSession.setAttribute( Line 56: SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 57: SSOUtils.attachUserToSession(userId, username, email, groupIds, isAdmin)); Line 58: } catch (SSOAuthorizationException ex) { Line 59: log.error(ex.getMessage()); . log.error("Cannot <something: {}", ex.getMessage()); log.debug("Exception", ex); Line 60: throw new RuntimeException(ex); Line 61: } Line 62: response.sendRedirect(applicationUrl); Line 63: } http://gerrit.ovirt.org/#/c/36619/3/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOUtils.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/SSOUtils.java: most probably this can be merged with FiltersHelper Line 1: package org.ovirt.engine.core.utils.servlet; Line 2: Line 3: import org.apache.commons.lang.StringUtils; Line 4: import org.ovirt.engine.core.common.VdcObjectType; Line 36: backend = (BackendLocal) initial.lookup("java:global/engine/bll/Backend!org.ovirt.engine.core.common.interfaces.BackendLocal"); Line 37: } Line 38: catch (Exception exception) { Line 39: throw new RuntimeException("Can't find reference to backend bean.", exception); Line 40: } don't we have a helper to execute commands from servlet without this? Line 41: Line 42: permissionsRoleManager = new SSOPermissionsRoleManager(); Line 43: } Line 44: Line 67: public static void checkUserAndGroupsAuthorization(Guid userId, Line 68: String username, Line 69: String[] groupIds) throws SSOAuthorizationException { Line 70: try { Line 71: if (permissionsRoleManager.getEntityPermissionsForUserAndGroups(userId, does this run a command? Line 72: StringUtils.join(groupIds, ","), Line 73: ActionGroup.LOGIN, Line 74: BOTTOM_OBJECT_ID, Line 75: VdcObjectType.Bottom, Line 81: throw new SSOAuthorizationException("Internal Database Error", ex); Line 82: } Line 83: } Line 84: Line 85: public static boolean isAdminUser(Guid userId, String[] groupIds) throws SSOAuthorizationException { can't we have single command that perform anything we like and returns status and if user admin? Line 86: try { Line 87: List<Role> userRoles = permissionsRoleManager.getAnyAdminRoleForUserAndGroups(userId, StringUtils.join(groupIds, ",")); Line 88: if (!userRoles.isEmpty()) { Line 89: log.debug("User logged to admin using role '{}'", userRoles.get(0).getname()); http://gerrit.ovirt.org/#/c/36619/3/frontend/webadmin/modules/webadmin/src/main/webapp/WEB-INF/web.xml File frontend/webadmin/modules/webadmin/src/main/webapp/WEB-INF/web.xml: Line 26: <filter-name>SSOLoginFilter</filter-name> Line 27: <filter-class>org.ovirt.engine.core.utils.servlet.SSOLoginFilter</filter-class> Line 28: <init-param> Line 29: <param-name>login-url</param-name> Line 30: <param-value>/sso/login?</param-value> why do you need the '?' Line 31: </init-param> Line 32: <init-param> Line 33: <param-name>post-login-url</param-name> Line 34: <param-value>/sso/post-login</param-value> Line 35: </init-param> Line 36: </filter> Line 37: <filter-mapping> Line 38: <filter-name>SSOLoginFilter</filter-name> Line 39: <url-pattern>/sso/login</url-pattern> it should not be on login Line 40: </filter-mapping> Line 41: Line 42: <filter-mapping> Line 43: <filter-name>SSOLoginFilter</filter-name> Line 40: </filter-mapping> Line 41: Line 42: <filter-mapping> Line 43: <filter-name>SSOLoginFilter</filter-name> Line 44: <url-pattern>/WebAdmin.html</url-pattern> you modified it to index.jsp no? Line 45: </filter-mapping> Line 46: Line 47: <filter> Line 48: <filter-name>SessionValidationFilter</filter-name> Line 41: Line 42: <filter-mapping> Line 43: <filter-name>SSOLoginFilter</filter-name> Line 44: <url-pattern>/WebAdmin.html</url-pattern> Line 45: </filter-mapping> I thought of putthing these filters at end of chain for now... only if other mechanisms do not work. Line 46: Line 47: <filter> Line 48: <filter-name>SessionValidationFilter</filter-name> Line 49: <filter-class>org.ovirt.engine.core.aaa.filters.SessionValidationFilter</filter-class> http://gerrit.ovirt.org/#/c/36619/3/frontend/webadmin/modules/webadmin/src/main/webapp/index.jsp File frontend/webadmin/modules/webadmin/src/main/webapp/index.jsp: Line 1: <jsp:forward page="/sso/login?app_url=${pageContext.request.contextPath}/WebAdmin.html?${pageContext.request.queryString}" /> no... this should be protected by the filter, there should no no special content. the filter will redirect. -- 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: 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
