Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 3: (15 comments) http://gerrit.ovirt.org/#/c/36119/3/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/AuthenticationUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/AuthenticationUtils.java: Line 19: public class AuthenticationUtils { Line 20: Line 21: private static final Logger log = LoggerFactory.getLogger(EngineSSOServlet.class); Line 22: Line 23: public static boolean handleCredentials(HttpSession session, String username, String password, String domain) domain->profile everywhere please Line 24: throws IOException, ServletException { Line 25: if (StringUtils.isEmpty(username) || StringUtils.isEmpty(domain)) { Line 26: return false; Line 27: } Line 23: public static boolean handleCredentials(HttpSession session, String username, String password, String domain) Line 24: throws IOException, ServletException { Line 25: if (StringUtils.isEmpty(username) || StringUtils.isEmpty(domain)) { Line 26: return false; Line 27: } exception? or at least proper message? Line 28: UserProfile userProfile = translateUser(username, domain); Line 29: boolean authenticated = false; Line 30: if (userProfile == null || userProfile.profile == null) { Line 31: log.error("Error in obtaining profile {}", userProfile.profile); Line 24: throws IOException, ServletException { Line 25: if (StringUtils.isEmpty(username) || StringUtils.isEmpty(domain)) { Line 26: return false; Line 27: } Line 28: UserProfile userProfile = translateUser(username, domain); probably all you need is the authn so you can already resolve this at caller and pass it. Line 29: boolean authenticated = false; Line 30: if (userProfile == null || userProfile.profile == null) { Line 31: log.error("Error in obtaining profile {}", userProfile.profile); Line 32: } else { Line 28: UserProfile userProfile = translateUser(username, domain); Line 29: boolean authenticated = false; Line 30: if (userProfile == null || userProfile.profile == null) { Line 31: log.error("Error in obtaining profile {}", userProfile.profile); Line 32: } else { there should be an optional mapper for the user as well Line 33: ExtMap outputMap = userProfile.profile.getAuthn().invoke(new ExtMap().mput( Line 34: Base.InvokeKeys.COMMAND, Line 35: Authn.InvokeCommands.AUTHENTICATE_CREDENTIALS Line 36: ).mput( Line 43: ); Line 44: if (outputMap.<Integer>get(Base.InvokeKeys.RESULT) == Base.InvokeResult.SUCCESS && Line 45: outputMap.<Integer>get(Authn.InvokeKeys.RESULT) == Authn.AuthResult.SUCCESS) { Line 46: doLogin(session, Line 47: userProfile.profile, again, what you need here is authz so you can resolve this on caller and work here only with extension proxy, no need for UserProfile class to reach here Line 48: outputMap.<ExtMap>get(Authn.InvokeKeys.AUTH_RECORD)); Line 49: authenticated = true; Line 50: } else { Line 51: log.error("Failure in authentication to profile {}. Invocation Result code is {}. Authn result code is {}", Line 50: } else { Line 51: log.error("Failure in authentication to profile {}. Invocation Result code is {}. Authn result code is {}", Line 52: userProfile.profile, Line 53: outputMap.<Integer>get(Base.InvokeKeys.RESULT), Line 54: outputMap.<Integer>get(Authn.InvokeKeys.RESULT) I believe some of the results should be translated into human readable text and sent to login, for now you can use the constant as is. Line 55: ); Line 56: } Line 57: } Line 58: return authenticated; Line 54: outputMap.<Integer>get(Authn.InvokeKeys.RESULT) Line 55: ); Line 56: } Line 57: } Line 58: return authenticated; best to return exception if failure, as you can have code, message etc... Line 59: } Line 60: Line 61: public static void doLogin(HttpSession session, AuthenticationProfile profile, ExtMap authRecord) throws IOException, Line 62: ServletException { Line 78: authRecord Line 79: ); Line 80: } Line 81: Line 82: session.setAttribute(EngineSSOServlet.SSO_DOMAIN_ATTR_NAME, AuthzUtils.getName(profile.getAuthz())); please do not use AuthzUtils, nor the profile. Line 83: session.setAttribute(EngineSSOServlet.SSO_PRINCIPAL_RECORD_ATTR_NAME, AuthzUtils.fetchPrincipalRecord(profile.getAuthz(), authRecord)); Line 84: } Line 85: Line 86: private static UserProfile translateUser(String username, String domain) { Line 79: ); Line 80: } Line 81: Line 82: session.setAttribute(EngineSSOServlet.SSO_DOMAIN_ATTR_NAME, AuthzUtils.getName(profile.getAuthz())); Line 83: session.setAttribute(EngineSSOServlet.SSO_PRINCIPAL_RECORD_ATTR_NAME, AuthzUtils.fetchPrincipalRecord(profile.getAuthz(), authRecord)); can we copy the fetch principal record here? Line 84: } Line 85: Line 86: private static UserProfile translateUser(String username, String domain) { Line 87: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(domain); http://gerrit.ovirt.org/#/c/36119/3/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 18: private static final String DOMAIN = "domain"; Line 19: private static final String MODULE = "module"; Line 20: private static final String LOGIN_FAILED_MSG = "Login failed, please try again."; Line 21: public static final String SSO_PRINCIPAL_RECORD_ATTR_NAME = "PRINCIPAL_RECORD"; Line 22: public static final String SSO_DOMAIN_ATTR_NAME = "DOMAIN"; AUTHZ_NAME Line 23: Line 24: @Override Line 25: protected void service(HttpServletRequest request, HttpServletResponse response) Line 26: throws ServletException, IOException { Line 31: if (!containsUserCredentials(request.getParameterMap())) { Line 32: String module = request.getParameter(MODULE); Line 33: if (module != null) { Line 34: session.setAttribute(MODULE, module); Line 35: } question: why do we need to mix this servlet with redirect? why if there is a session can't it redirect user to another servlet/jsp? Line 36: SSOUtils.forwardToLoginPage(request, response); Line 37: } else { Line 38: handleUnauthenticatedUser(session, request, response); Line 39: } http://gerrit.ovirt.org/#/c/36119/3/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUser.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUser.java: I really don't like this class, as we do have the ext maps for principal record Line 1: package org.ovirt.engine.core.sso.servlets; Line 2: Line 3: import org.ovirt.engine.core.aaa.DirectoryUser; Line 4: http://gerrit.ovirt.org/#/c/36119/3/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOUtils.java: Line 16: public class SSOUtils { Line 17: Line 18: public static void redirectToModule(ExtMap principalRecord, String domain, HttpServletResponse response, String module) Line 19: throws IOException { Line 20: StringBuilder redirectUrl = new StringBuilder("/ovirt-engine/"); should be relative Line 21: redirectUrl.append(module); Line 22: redirectUrl.append("/login?"); Line 23: Line 24: StringBuilder params = new StringBuilder(""); Line 21: redirectUrl.append(module); Line 22: redirectUrl.append("/login?"); Line 23: Line 24: StringBuilder params = new StringBuilder(""); Line 25: appendParameter(params, "userExternalId", principalRecord.<String> get(Authz.PrincipalRecord.ID)); please no external terms in this module... this is plain id Line 26: appendParameter(params, "domain", domain); Line 27: appendParameter(params, "namespace", principalRecord.<String>get(Authz.PrincipalRecord.NAMESPACE)); Line 28: appendParameter(params, "username", getLoginName(principalRecord)); Line 29: appendParameter(params, "firstname", principalRecord.<String>get(Authz.PrincipalRecord.FIRST_NAME)); Line 22: redirectUrl.append("/login?"); Line 23: Line 24: StringBuilder params = new StringBuilder(""); Line 25: appendParameter(params, "userExternalId", principalRecord.<String> get(Authz.PrincipalRecord.ID)); Line 26: appendParameter(params, "domain", domain); authz name Line 27: appendParameter(params, "namespace", principalRecord.<String>get(Authz.PrincipalRecord.NAMESPACE)); Line 28: appendParameter(params, "username", getLoginName(principalRecord)); Line 29: appendParameter(params, "firstname", principalRecord.<String>get(Authz.PrincipalRecord.FIRST_NAME)); Line 30: appendParameter(params, "lastname", principalRecord.<String>get(Authz.PrincipalRecord.LAST_NAME)); -- 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: 3 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-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
