Alon Bar-Lev has posted comments on this change. Change subject: core, webadmin: Modify webadmin to use enginesso for authentication ......................................................................
Patch Set 6: (9 comments) http://gerrit.ovirt.org/#/c/36619/6/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 113: boolean isAdmin = (boolean)retVal.get(GetUserEntityPermissionsParameters.IS_ADMIN); Line 114: if (loginAsAdmin && !isAdmin) { Line 115: throw new SSOAuthorizationException( Line 116: String.format("The user %s is not authorized to perform login", username)); Line 117: } I still do not understand why the above cannot be done within CreateUserSession, while setting requireAdmin within parameters. command will fail if the user does not meet the requirement. it can also call the setAdmin based on this information. it will simplify this ping-pong and remove complexity from caller. when you collapse these two commands into single command all you left is with trivial code that should go into the post login servlet, so actually nothing should remain here as it is called once and belongs to that servlet. helpers/utils are good when there are multiple consumers. Line 118: Line 119: DbUser user = new DbUser(); Line 120: user.setId(Guid.createGuidFromString(request.getParameter("userId"))); Line 121: user.setLoginName(request.getParameter("username")); Line 119: DbUser user = new DbUser(); Line 120: user.setId(Guid.createGuidFromString(request.getParameter("userId"))); Line 121: user.setLoginName(request.getParameter("username")); Line 122: user.setEmail(request.getParameter("email")); Line 123: user.setGroupIds(groupIds == null ? Collections.emptyList() : (Collection) Arrays.asList(groupIds)); why do we pass dbuser? can we convert the query string into a map and let the command build the dbuser out of whatever it wishes? notice that we probably need to serialize more complex entities anyway, so maybe passing fields within query string per field is not the right way to go... we need a single user=<blobl> which will be ticket within json or something, so for now the user= can be json, this json can be forwarded to the command as-is or as a map. the post login servlet can be a bridge without a logic. Line 124: user.setAdmin(isAdmin); Line 125: try { Line 126: VdcReturnValueBase queryRetVal = getBackend(new InitialContext()).runAction(VdcActionType.CreateUserSession, new CreateUserSessionParameters(user)); Line 127: if (!queryRetVal.getSucceeded()) { Line 132: SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 133: queryRetVal.getActionReturnValue()); Line 134: } catch (NamingException ex) { Line 135: throw new SSOAuthorizationException("Unable to get reference to backend bean", ex); Line 136: } please follow the SessionValidationFilter pattern of how to use the jndi context, and properly free it. InitialContext ctx = new InitialContext(); try { xxxxxx getBackend(ctx) xxxxx } finally { ctx.close(); } Line 137: } Line 138: Line 139: private static Map<String, Object> checkUserAndGroupsAuthorization(Guid userId, Line 140: String username, http://gerrit.ovirt.org/#/c/36619/6/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 31: String queryString = req.getQueryString(); Line 32: String requestUrl = req.getRequestURI() + (StringUtils.isEmpty(queryString) ? "" : "?" + queryString); Line 33: Line 34: if (!FiltersHelper.isAuthenticated(req)) { Line 35: ((HttpServletResponse)response).sendRedirect(loginUrl + "&app_url=" + ((HttpServletResponse) response).encodeURL(requestUrl)); won't it better to use URLEncoder/URLDecoder all over? Line 36: } else { Line 37: chain.doFilter(request, response); Line 38: } Line 39: } http://gerrit.ovirt.org/#/c/36619/6/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOLoginServlet.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOLoginServlet.java: Line 26: } Line 27: Line 28: @Override Line 29: protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { Line 30: response.sendRedirect(String.format("%s&app_url=%s&post_login_url=%s", can we fix the sso patch to accept opaque instead of app_url, as it should not care if it is a url or json or anything else we send and get back. Line 31: ssoUrl, Line 32: response.encodeURL(request.getParameter("app_url")), Line 33: response.encodeURL(postLoginUrl))); Line 34: } http://gerrit.ovirt.org/#/c/36619/6/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 20: Line 21: @Override Line 22: public void init() throws ServletException { Line 23: String strVal = getServletConfig().getInitParameter("login-as-admin"); Line 24: if (strVal != null) { don't you want to raise exception if null as you do with the other validations? if not please remove conditional: loginAsAdmin = Boolean.parseBoolean(getServletConfig().getInitParameter("login-as-admin")); Line 25: loginAsAdmin = Boolean.parseBoolean(strVal); Line 26: } Line 27: } Line 28: Line 34: Line 35: if (StringUtils.isEmpty(request.getParameter("app_url"))) { Line 36: throw new RuntimeException("No application url found in request."); Line 37: } Line 38: try { I think this try/catch can contain the entire service() to log all issues. also the SSOAuthenticationException is probably not required and you can use RuntimeException as well to simplify. Line 39: FiltersHelper.attachUserToSession(request, loginAsAdmin); Line 40: } catch (SSOAuthorizationException ex) { Line 41: log.error("Cannot do user authorization: {}", ex.getMessage()); Line 42: log.debug("Exception", ex); Line 41: log.error("Cannot do user authorization: {}", ex.getMessage()); Line 42: log.debug("Exception", ex); Line 43: throw new RuntimeException(ex); Line 44: } Line 45: response.sendRedirect(request.getParameter("app_url")); can we fix the sso patch to accept opaque instead of app_url, as it should not care if it is a url or json or anything else we send and get back. Line 46: } Line 47: http://gerrit.ovirt.org/#/c/36619/6/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 15: Line 16: @Override Line 17: protected void executeCommand() { Line 18: String engineSessionId = SessionDataContainer.getInstance().generateEngineSessionId(); Line 19: SessionDataContainer.getInstance().setUser(engineSessionId, getParameters().getUser()); lots of stuff is missing.... 1. we must have auth record to get stuff out of it such as principal name, so we need to serialize it at sso and send it over. 2. i think the same applies to principal record. 3. alternative of both is to send principal name and profile name [at least]. setProfile setAuthRecord setPrincipalRecord 4. we will need to remember (someone) to solve the password thing for the vdsm password delegation... if this feature is enabled we should delegate password either encrypted to engine certificate or engine fetch this from sso. for now, application will work without this. Line 20: getReturnValue().setActionReturnValue(engineSessionId); Line 21: setSucceeded(true); Line 22: } Line 23: -- 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: 6 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
