Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 23: (6 comments) http://gerrit.ovirt.org/#/c/36119/23/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOLogoutServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOLogoutServlet.java: Line 20: throws ServletException, IOException { Line 21: if (StringUtils.isEmpty(request.getParameter(SSOUtils.OPAQUE))) { Line 22: throw new RuntimeException("No application data found in request."); Line 23: } Line 24: if (StringUtils.isEmpty(request.getParameter(SSOUtils.POST_LOGIN_URL))) { well, it is not actually post login :) so let's rename it to post_action_url? Line 25: throw new RuntimeException("No post login process url found in request."); Line 26: } Line 27: HttpSession existingSession = request.getSession(false); Line 28: if (existingSession != null) { http://gerrit.ovirt.org/#/c/36119/23/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOServlet.java: Line 28: @Override Line 29: protected void service(HttpServletRequest request, HttpServletResponse response) Line 30: throws ServletException, IOException { Line 31: if (StringUtils.isEmpty(request.getParameter(SSOUtils.OPAQUE))) { Line 32: throw new RuntimeException("No application data found in request."); this is valid... just do not provide back anything. Line 33: } Line 34: if (StringUtils.isEmpty(request.getParameter(SSOUtils.POST_LOGIN_URL))) { Line 35: throw new RuntimeException("No post login process url found in request."); Line 36: } Line 63: log.error("Internal Database Error", ex); Line 64: throw new AuthenticationException("Internal Database Error", ex); Line 65: } Line 66: } catch (AuthenticationException ex) { Line 67: request.setAttribute("profiles", AuthenticationUtils.getAvailableProfiles(session)); why profiles is set here? Line 68: request.getRequestDispatcher("/WEB-INF/login.jsp?msg=" + ex.getMessage()).forward(request, response); Line 69: } Line 70: } Line 71: http://gerrit.ovirt.org/#/c/36119/23/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSORedirectServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSORedirectServlet.java: Line 18: @Override Line 19: protected void service(HttpServletRequest request, HttpServletResponse response) Line 20: throws ServletException, IOException { Line 21: if (StringUtils.isEmpty(request.getParameter(SSOUtils.OPAQUE))) { Line 22: throw new RuntimeException("No application data found in request."); this is valid, just do not send it back. Line 23: } Line 24: if (StringUtils.isEmpty(request.getParameter(SSOUtils.POST_LOGIN_URL))) { Line 25: throw new RuntimeException("No post login url found in request."); Line 26: } http://gerrit.ovirt.org/#/c/36119/23/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUserGroupManager.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUserGroupManager.java: Line 29: } Line 30: Line 31: public String getGroupByExternalId(String authzName, String externalId) throws SQLException { Line 32: return executeQuery("SELECT id FROM ad_groups WHERE domain = ? AND external_id = ?", authzName, externalId, "id"); Line 33: } I wounder if not better to pass a list of ids and do select external_id, id from .... where external_id in (x, y, z). single query to extract all ids. Line 34: Line 35: private String executeQuery(String sql, String authzName, String externalId, String columnName) throws SQLException { Line 36: String retVal = null; Line 37: try ( http://gerrit.ovirt.org/#/c/36119/23/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java: Line 54: payload.put("userId", userId); Line 55: payload.put("userName", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 56: payload.put("email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 57: payload.put("groupIds", getGroupIds(userGroupManager, authzName, principalRecord)); Line 58: payload.put("opaque", request.getParameter(OPAQUE)); oh... please put the opaque as query parameter :) it is not part of the ticket or authenticated data. Line 59: payload.put("profile", session.getAttribute(SSOUtils.SSO_PROFILE_ATTR_NAME)); Line 60: payload.put("principalRecord", principalRecord); Line 61: payload.put("authRecord", session.getAttribute(SSOUtils.SSO_AUTH_RECORD_ATTR_NAME)); Line 62: -- To view, visit http://gerrit.ovirt.org/36119 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4894fc12653027271b6abd4dd5313b10593703fa Gerrit-PatchSet: 23 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
