Alon Bar-Lev has posted comments on this change. Change subject: core, webadmin: Modify webadmin to use enginesso for authentication ......................................................................
Patch Set 4: (16 comments) http://gerrit.ovirt.org/#/c/36619/4/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 20: public void init(FilterConfig filterConfig) throws ServletException { Line 21: String strVal = filterConfig.getInitParameter("login-url"); Line 22: if (strVal == null) { Line 23: throw new RuntimeException("No login-url init parameter specified for SSOLoginFilter."); Line 24: } else { no else in exception pattern please. I am unsure why you need strVal temporary variale. Line 25: loginUrl = strVal; Line 26: } Line 27: } Line 28: Line 33: String queryString = req.getQueryString(); Line 34: String requestUrl = req.getRequestURI() + (StringUtils.isEmpty(queryString) ? "" : "?" + queryString); Line 35: Line 36: if (!FiltersHelper.isAuthenticated(req)) { Line 37: request.getRequestDispatcher(loginUrl + "&app_url=" + ((HttpServletResponse) response).encodeURL(requestUrl)).forward(request, response); in filter it is much better to redirect, I am unsure what the implication of dispatch. I do understand that in servlet you can do this... but in filter it is risky. Line 38: } else { Line 39: chain.doFilter(request, response); Line 40: } Line 41: } http://gerrit.ovirt.org/#/c/36619/4/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 41 Line 42 Line 43 Line 44 Line 45 see my last comment, the above is a bug and should be removed probably also from stable in favor of bellow. http://gerrit.ovirt.org/#/c/36619/4/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: can we move this to aaa module? Line 1: package org.ovirt.engine.core.utils.servlet; Line 2: Line 3: import javax.servlet.ServletException; Line 4: import javax.servlet.http.HttpServlet; Line 18: if (strVal == null) { Line 19: throw new RuntimeException("No sso-url init parameter specified."); Line 20: } else { Line 21: ssoUrl = strVal; Line 22: } no else in exception programming. not sure why you need strVal temporary variable. Line 23: Line 24: strVal = getServletConfig().getInitParameter("post-login-url"); Line 25: if (strVal == null) { Line 26: throw new RuntimeException("No post-process-url init parameter specified for SSOLoginServlet."); Line 30: } Line 31: Line 32: @Override Line 33: protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { Line 34: String url = String.format("%s&app_url=%s&post_login_url=%s", you do not need url. Line 35: ssoUrl, Line 36: response.encodeURL(request.getParameter("app_url")), Line 37: response.encodeURL(postLoginUrl)); Line 38: response.sendRedirect(String.format("%s&app_url=%s&post_login_url=%s", Line 34: String url = String.format("%s&app_url=%s&post_login_url=%s", Line 35: ssoUrl, Line 36: response.encodeURL(request.getParameter("app_url")), Line 37: response.encodeURL(postLoginUrl)); Line 38: response.sendRedirect(String.format("%s&app_url=%s&post_login_url=%s", app_url should be opaque it is meaning less to sso, it just returns the same value to post login url, see my comment there. Line 39: ssoUrl, Line 40: response.encodeURL(request.getParameter("app_url")), Line 41: response.encodeURL(postLoginUrl))); Line 42: } http://gerrit.ovirt.org/#/c/36619/4/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: can we move this to aaa module? 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.constants.SessionConstants; Line 15: Line 16: public class SSOPostLoginServlet extends HttpServlet { Line 17: private static final long serialVersionUID = 9210030009170727847L; Line 18: Line 19: private Logger log = LoggerFactory.getLogger(getClass()); final? Line 20: private boolean loginAsAdmin = false; Line 21: Line 22: @Override Line 23: public void init() throws ServletException { Line 51: httpSession.setAttribute( Line 52: SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 53: SSOUtils.attachUserToSession(request, isAdmin)); Line 54: } catch (SSOAuthorizationException ex) { Line 55: log.error(ex.getMessage(), ex); log error is: log.error("Cannot do xxxx: {}", ex.getMessage()); Line 56: log.debug("Exception", ex); Line 57: throw new RuntimeException(ex); Line 58: } Line 59: response.sendRedirect(applicationUrl); http://gerrit.ovirt.org/#/c/36619/4/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: can we move this to aaa? Line 1: package org.ovirt.engine.core.utils.servlet; Line 2: Line 3: import org.ovirt.engine.core.common.VdcObjectType; Line 4: import org.ovirt.engine.core.common.action.CreateUserSessionParameters; Line 26: Line 27: public class SSOUtils { Line 28: private static final Guid BOTTOM_OBJECT_ID = new Guid("BBB00000-0000-0000-0000-123456789BBB"); Line 29: private static BackendLocal backend; Line 30: private static Logger log = LoggerFactory.getLogger(SSOUtils.class); final and first? Line 31: Line 32: static { Line 33: // Lookup the backend bean: Line 34: try { Line 37: } Line 38: catch (Exception exception) { Line 39: throw new RuntimeException("Can't find reference to backend bean.", exception); Line 40: } Line 41: } can you please use the same methods of FiltersHelper in this regard, also please try to merge these two classes if you can. Line 42: Line 43: public static String attachUserToSession(HttpServletRequest request, boolean isAdmin) { Line 44: String engineSessionId = null; Line 45: String[] groupIds = request.getParameterValues("groupId"); Line 55: } Line 56: return engineSessionId; Line 57: } Line 58: Line 59: public static void checkUserAndGroupsAuthorization(Guid userId, hmmm.... why can't the above command check that as well? Line 60: String username, Line 61: String[] groupIds) throws SSOAuthorizationException { Line 62: VdcQueryReturnValue retVal = backend.runPublicQuery(VdcQueryType.GetUserEntityPermissions, Line 63: new GetUserEntityPermissionsParameters(userId, groupIds, ActionGroup.LOGIN, BOTTOM_OBJECT_ID, VdcObjectType.Bottom, true, true)); Line 69: throw new SSOAuthorizationException(String.format("The user %s is not authorized to perform login", username)); Line 70: } Line 71: } Line 72: Line 73: public static boolean isAdminUser(Guid userId, String username, String[] groupIds) throws SSOAuthorizationException { hmmm.... why can't the above command check that as well? get adminRequired in parameters? Line 74: VdcQueryReturnValue retVal = backend.runPublicQuery(VdcQueryType.GetAnyAdminRoleForUserAndGroups, Line 75: new GetAnyAdminRoleForUserAndGroupsParameters(userId, groupIds, true)); Line 76: if (!retVal.getSucceeded()) { Line 77: log.error("Unable to get any admin role for users and groups {}", username); http://gerrit.ovirt.org/#/c/36619/4/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}" /> again, I am unsure why this file is required -- 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: 4 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
