Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 5: (15 comments) http://gerrit.ovirt.org/#/c/36119/5/backend/manager/modules/enginesso/pom.xml File backend/manager/modules/enginesso/pom.xml: Line 20: <groupId>${engine.groupId}</groupId> Line 21: <artifactId>aaa</artifactId> Line 22: <version>${engine.version}</version> Line 23: <scope>provided</scope> Line 24: </dependency> can we drop this ^? Line 25: Line 26: <dependency> Line 27: <groupId>${engine.groupId}</groupId> Line 28: <artifactId>extensions-manager</artifactId> http://gerrit.ovirt.org/#/c/36119/5/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: protected void service(HttpServletRequest request, HttpServletResponse response) Line 27: throws ServletException, IOException { Line 28: HttpSession session = request.getSession(true); Line 29: if (isUserAuthenticated(session)) { Line 30: handleAuthenticatedUser(session, request, response); this is redirect pending, simply to menu if we have nowhere else to go. Line 31: } else Line 32: if (!containsUserCredentials(request.getParameterMap())) { Line 33: String module = request.getParameter(MODULE); Line 34: if (module != null) { Line 28: HttpSession session = request.getSession(true); Line 29: if (isUserAuthenticated(session)) { Line 30: handleAuthenticatedUser(session, request, response); Line 31: } else Line 32: if (!containsUserCredentials(request.getParameterMap())) { indent? Line 33: String module = request.getParameter(MODULE); Line 34: if (module != null) { Line 35: session.setAttribute(MODULE, module); Line 36: } Line 32: if (!containsUserCredentials(request.getParameterMap())) { Line 33: String module = request.getParameter(MODULE); Line 34: if (module != null) { Line 35: session.setAttribute(MODULE, module); Line 36: } better not use session but redirect with parameters no? Line 37: SSOUtils.forwardToLoginPage(request, response); Line 38: } else { Line 39: handleUnauthenticatedUser(session, request, response); Line 40: } Line 35: session.setAttribute(MODULE, module); Line 36: } Line 37: SSOUtils.forwardToLoginPage(request, response); Line 38: } else { Line 39: handleUnauthenticatedUser(session, request, response); not sure I understand when we reach here Line 40: } Line 41: } Line 42: Line 43: public boolean isUserAuthenticated(HttpSession session) { Line 53: * to module with user details. Line 54: * If module parameter is found in the request redirect to the module with Line 55: * user details, otherwise forward to index page. Line 56: */ Line 57: private void handleAuthenticatedUser(HttpSession session, HttpServletRequest request, HttpServletResponse response) best to store it at request variable to not create conflict when two concurrent request are in place. Line 58: throws ServletException, IOException { Line 59: String module = (String) session.getAttribute(MODULE); Line 60: if (module != null) { Line 61: session.removeAttribute(MODULE); http://gerrit.ovirt.org/#/c/36119/5/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/AuthenticationUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/AuthenticationUtils.java: I really like to see single sequence of authentication/authorization flow, not split into several functions. splitting does not enhance readability. 1. locate profile 2. perform user mapping 3. perform authn 4. perform auth record mapping 5. perform authz I try to follow the code, but I just cannot... need a clean sequence. please move all db related activities into separate class, these are called if the above sequence succeeds and have nothing to do with the sequence. Line 1: package org.ovirt.engine.core.sso.utils; Line 2: Line 3: import org.ovirt.engine.api.extensions.Base; Line 4: import org.ovirt.engine.api.extensions.ExtMap; Line 28: private static final Logger log = LoggerFactory.getLogger(EngineSSOServlet.class); Line 29: Line 30: public static boolean handleCredentials(HttpSession session, String username, String password, String profile) Line 31: throws IOException, ServletException, AuthenticationError { Line 32: UserProfile userProfile = translateUser(username, profile); it is not per user... so I was confused at first. it just UserProfile out of string profile, so it actually a Profile. there is no need to supply the username. Line 33: boolean authenticated = false; Line 34: String errorMsg = null; Line 35: if (userProfile == null || userProfile.authn == null || userProfile.authz == null) { Line 36: errorMsg = String.format("Error in obtaining profile %s", profile); Line 32: UserProfile userProfile = translateUser(username, profile); Line 33: boolean authenticated = false; Line 34: String errorMsg = null; Line 35: if (userProfile == null || userProfile.authn == null || userProfile.authz == null) { Line 36: errorMsg = String.format("Error in obtaining profile %s", profile); please throw exception and avoid nesting. Line 37: } else { Line 38: ExtMap outputMap = userProfile.authn.invoke(new ExtMap().mput( Line 39: Base.InvokeKeys.COMMAND, Line 40: Authn.InvokeCommands.AUTHENTICATE_CREDENTIALS Line 34: String errorMsg = null; Line 35: if (userProfile == null || userProfile.authn == null || userProfile.authz == null) { Line 36: errorMsg = String.format("Error in obtaining profile %s", profile); Line 37: } else { Line 38: ExtMap outputMap = userProfile.authn.invoke(new ExtMap().mput( we need to map user before as well if there is a mapper Line 39: Base.InvokeKeys.COMMAND, Line 40: Authn.InvokeCommands.AUTHENTICATE_CREDENTIALS Line 41: ).mput( Line 42: Authn.InvokeKeys.USER, Line 47: ) Line 48: ); Line 49: if (outputMap.<Integer>get(Base.InvokeKeys.RESULT) == Base.InvokeResult.SUCCESS && Line 50: outputMap.<Integer>get(Authn.InvokeKeys.RESULT) == Authn.AuthResult.SUCCESS) { Line 51: doLogin(session, it is not actually login :) I do suggest to move the code here, or at least call these functions: doAuthn() doAuthz() :) Line 52: userProfile, Line 53: outputMap.<ExtMap>get(Authn.InvokeKeys.AUTH_RECORD)); Line 54: authenticated = true; Line 55: } else { Line 52: userProfile, Line 53: outputMap.<ExtMap>get(Authn.InvokeKeys.AUTH_RECORD)); Line 54: authenticated = true; Line 55: } else { Line 56: errorMsg = AuthnMessageMapper.mapMessageErrorCode(outputMap); always: if (fail) { throw exception } continue as usual Line 57: } Line 58: } Line 59: if (!authenticated) { Line 60: log.error(errorMsg); Line 58: } Line 59: if (!authenticated) { Line 60: log.error(errorMsg); Line 61: throw new AuthenticationError(errorMsg); Line 62: } please avoid conditionals... just catch exception at function scope log and rethrow if you like logging here, although logging will be done at caller, so not important here. Line 63: return authenticated; Line 64: } Line 65: Line 66: public static void doLogin(HttpSession session, UserProfile userProfile, ExtMap authRecord) throws IOException, Line 59: if (!authenticated) { Line 60: log.error(errorMsg); Line 61: throw new AuthenticationError(errorMsg); Line 62: } Line 63: return authenticated; no reason, if no exception it succeeds. Line 64: } Line 65: Line 66: public static void doLogin(HttpSession session, UserProfile userProfile, ExtMap authRecord) throws IOException, Line 67: ServletException { Line 87: session.setAttribute(EngineSSOServlet.SSO_PRINCIPAL_ATTR_NAME, Line 88: mapToDbUser(authRecord, (String) userProfile.authz.getContext().get(Base.ContextKeys.INSTANCE_NAME))); Line 89: } Line 90: Line 91: private static DbUser mapToDbUser(ExtMap principalRecord, String profile) { I forgot about the fact that we store users, can we avoid it? if user have no permissions we can give it random uuid... or "guest" uuid. why should we save it? Line 92: DbUser user; Line 93: try { Line 94: String externalId = principalRecord.get(Authz.PrincipalRecord.ID); Line 95: user = DbUserManager.getInstance().getUserByExternalId(profile, externalId); -- 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: 5 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
