Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 1: (10 comments) basically I expect we do everything in sso from scratch without any legacy and delegate what we have into the webapp. in the end we just remove the entire login sequence from bll, so no reason to use it. http://gerrit.ovirt.org/#/c/36119/1/backend/manager/modules/enginesso/pom.xml File backend/manager/modules/enginesso/pom.xml: Line 20: <groupId>org.jboss.logging</groupId> Line 21: <artifactId>jboss-logging</artifactId> Line 22: <version>3.1.3.GA</version> Line 23: <scope>provided</scope> Line 24: </dependency> any reason for this? Line 25: Line 26: <dependency> Line 27: <groupId>${engine.groupId}</groupId> Line 28: <artifactId>common</artifactId> Line 27: <groupId>${engine.groupId}</groupId> Line 28: <artifactId>common</artifactId> Line 29: <version>${engine.version}</version> Line 30: <scope>provided</scope> Line 31: </dependency> I would really like to avoid the common, where do we actually use it? Line 32: Line 33: <dependency> Line 34: <groupId>${engine.groupId}</groupId> Line 35: <artifactId>aaa</artifactId> Line 41: <groupId>${engine.groupId}</groupId> Line 42: <artifactId>bll</artifactId> Line 43: <version>${engine.version}</version> Line 44: <scope>provided</scope> Line 45: </dependency> bll should not be used at all within the sso Line 46: Line 47: <dependency> Line 48: <groupId>${engine.groupId}</groupId> Line 49: <artifactId>extensions-manager</artifactId> Line 55: <groupId>${engine.groupId}</groupId> Line 56: <artifactId>utils</artifactId> Line 57: <version>${engine.version}</version> Line 58: <scope>provided</scope> Line 59: </dependency> I guess this is for branding Line 60: Line 61: <dependency> Line 62: <groupId>org.ovirt.engine.api</groupId> Line 63: <artifactId>ovirt-engine-extensions-api</artifactId> Line 80: <dependency> Line 81: <groupId>org.apache.httpcomponents</groupId> Line 82: <artifactId>httpcore</artifactId> Line 83: <scope>provided</scope> Line 84: </dependency> any reason for that? Line 85: Line 86: <dependency> Line 87: <groupId>junit</groupId> Line 88: <artifactId>junit</artifactId> http://gerrit.ovirt.org/#/c/36119/1/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSORedirectServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSORedirectServlet.java: Line 18: protected void doGet(HttpServletRequest request, HttpServletResponse response) Line 19: throws ServletException, IOException { Line 20: HttpSession session = request.getSession(false); Line 21: if (session == null || session.getAttribute("engineSession") == null) { Line 22: forwardRequest(request, response, "/WEB-INF/login.jsp"); why not redirect? Line 23: } else { Line 24: EngineSession engineSession = (EngineSession) session.getAttribute("engineSession"); Line 25: String module = request.getParameter("module"); Line 26: StringBuilder redirectUrl = new StringBuilder("/ovirt-engine/"); Line 25: String module = request.getParameter("module"); Line 26: StringBuilder redirectUrl = new StringBuilder("/ovirt-engine/"); Line 27: redirectUrl.append(module); Line 28: redirectUrl.append("/login?"); Line 29: redirectUrl.append("engineSessionId="); oh... I thought we delegate the user name, user id, set of groups, set of roles. the engine will create the session and put everything in. Line 30: redirectUrl.append(engineSession.getEngineSessionId()); Line 31: response.sendRedirect(redirectUrl.toString()); Line 32: } Line 33: http://gerrit.ovirt.org/#/c/36119/1/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 26: import org.ovirt.engine.core.common.action.VdcActionType; Line 27: import org.ovirt.engine.core.common.action.VdcLoginReturnValueBase; Line 28: import org.ovirt.engine.core.common.businessentities.aaa.DbUser; Line 29: import org.ovirt.engine.core.common.businessentities.EngineSession; Line 30: import org.ovirt.engine.core.common.constants.SessionConstants; we should perform low level login, without legacy. Line 31: import org.slf4j.Logger; Line 32: import org.slf4j.LoggerFactory; Line 33: Line 34: public class EngineSSOServlet extends HttpServlet { Line 43: protected void service(HttpServletRequest request, HttpServletResponse response) Line 44: throws ServletException, IOException { Line 45: Line 46: if (!containsUserCredentials(request.getParameterMap())) { Line 47: forwardRequest(request, response, "/WEB-INF/login.jsp"); not sure I understand this forward thing... redirect is much nicer in these trivial cases. Line 48: } else { Line 49: String username = request.getParameter(USERNAME); Line 50: String password = request.getParameter(PASSWORD); Line 51: String domain = request.getParameter(DOMAIN); Line 56: HttpSession session = request.getSession(true); Line 57: session.setAttribute("engineSession", engineSession); Line 58: forwardRequest(request, response, "/WEB-INF/ovirt.jsp"); Line 59: } else { Line 60: forwardRequest(request, response, "/WEB-INF/login-error.jsp"); just present login servlet (jsp) with parameter of what error is. Line 61: } Line 62: } Line 63: Line 64: } -- 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: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
