Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 11: (24 comments) http://gerrit.ovirt.org/#/c/36119/11/backend/manager/modules/enginesso/pom.xml File backend/manager/modules/enginesso/pom.xml: Line 19: <dependency> Line 20: <groupId>${engine.groupId}</groupId> Line 21: <artifactId>utils</artifactId> Line 22: <version>${engine.version}</version> Line 23: </dependency> any reason to import utils? Line 24: Line 25: <dependency> Line 26: <groupId>${engine.groupId}</groupId> Line 27: <artifactId>extensions-manager</artifactId> http://gerrit.ovirt.org/#/c/36119/11/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 27: throws ServletException, IOException { Line 28: HttpSession session = request.getSession(true); Line 29: if (SSOUtils.isUserAuthenticated(session)) { Line 30: RequestDispatcher dispatcher = request.getRequestDispatcher("/sso-redirect"); Line 31: response.setContentType("text/html;charset=UTF-8"); not sure why you set content type. Line 32: dispatcher.forward(request, response); Line 33: } else if (!containsUserCredentials(request.getParameterMap())) { Line 34: RequestDispatcher dispatcher = request.getRequestDispatcher("/WEB-INF/login.jsp"); Line 35: response.setContentType("text/html;charset=UTF-8"); Line 31: response.setContentType("text/html;charset=UTF-8"); Line 32: dispatcher.forward(request, response); Line 33: } else if (!containsUserCredentials(request.getParameterMap())) { Line 34: RequestDispatcher dispatcher = request.getRequestDispatcher("/WEB-INF/login.jsp"); Line 35: response.setContentType("text/html;charset=UTF-8"); not sure why you set content type. Line 36: dispatcher.forward(request, response); Line 37: } else { Line 38: handleUnauthenticatedUser(session, request, response); Line 39: } Line 35: response.setContentType("text/html;charset=UTF-8"); Line 36: dispatcher.forward(request, response); Line 37: } else { Line 38: handleUnauthenticatedUser(session, request, response); Line 39: } I asked in the past, it is not clear to me what is the difference between these two, both present the login form. Line 40: } Line 41: Line 42: private boolean containsUserCredentials(Map<String, String[]> parameters) { Line 43: return parameters.containsKey(USERNAME) && parameters.containsKey(PASSWORD) && parameters.containsKey(DOMAIN); Line 53: try { Line 54: AuthenticationUtils.handleCredentials(session, Line 55: request.getParameter(USERNAME), Line 56: request.getParameter(PASSWORD), Line 57: request.getParameter(DOMAIN)); profile Line 58: RequestDispatcher dispatcher = request.getRequestDispatcher("/sso-redirect"); Line 59: response.setContentType("text/html;charset=UTF-8"); Line 60: dispatcher.forward(request, response); Line 61: } catch (AuthenticationError ex) { Line 63: response.setContentType("text/html;charset=UTF-8"); Line 64: dispatcher.forward(request, response); Line 65: } catch (SQLException ex) { Line 66: throw new RuntimeException(ex); Line 67: } please present a valid error to user as well, log the exception and tell user that there is an internal database error. Line 68: } Line 69: http://gerrit.ovirt.org/#/c/36119/11/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 24: if (SSOUtils.isUserAuthenticated(session)) { Line 25: handleAuthenticatedUser(session, request, response); Line 26: } else { Line 27: RequestDispatcher dispatcher = request.getRequestDispatcher("/WEB-INF/login.jsp"); Line 28: response.setContentType("text/html;charset=UTF-8"); not sure why you set content type. Line 29: dispatcher.forward(request, response); Line 30: } Line 31: } Line 32: Line 26: } else { Line 27: RequestDispatcher dispatcher = request.getRequestDispatcher("/WEB-INF/login.jsp"); Line 28: response.setContentType("text/html;charset=UTF-8"); Line 29: dispatcher.forward(request, response); Line 30: } I do not follow entirely why both this and previous servlets redicect to login jsp, but if this is needed we may consider doing that in filter, not important right now. Line 31: } Line 32: Line 33: /** Line 34: * If a module was set in the session as on attribute, clear it and redirect Line 50: } catch (SQLException ex) { Line 51: throw new RuntimeException(ex); Line 52: } Line 53: } else { Line 54: RequestDispatcher dispatcher = request.getRequestDispatcher("/WEB-INF/ovirt.jsp"); in which case can we reach here without module? maybe better to just return an error? Line 55: response.setContentType("text/html;charset=UTF-8"); Line 56: dispatcher.forward(request, response); Line 57: } Line 58: } Line 51: throw new RuntimeException(ex); Line 52: } Line 53: } else { Line 54: RequestDispatcher dispatcher = request.getRequestDispatcher("/WEB-INF/ovirt.jsp"); Line 55: response.setContentType("text/html;charset=UTF-8"); not sure why you set content type. Line 56: dispatcher.forward(request, response); Line 57: } Line 58: } http://gerrit.ovirt.org/#/c/36119/11/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: Line 24: throws IOException, ServletException, AuthenticationError, SQLException { Line 25: ExtensionProxy authn = null; Line 26: ExtensionProxy authz = null; Line 27: ExtensionProxy mapper = null; Line 28: SSOExtensionsManager.getInstance().initialize(); this should be one time during webapp initialization, you can hold reference at the webapp context instead of using these totally wrong singletons. singletons should be avoided at any cost. Line 29: for (ExtensionProxy authnExtension : SSOExtensionsManager.getInstance().getExtensionsByService(Authn.class.getName())) { Line 30: Properties config = authnExtension.getContext().get(Base.ContextKeys.CONFIGURATION); Line 31: if (profile.equals(config.getProperty(Authn.ConfigKeys.PROFILE_NAME))) { Line 32: String mapperName = authnExtension.getContext().<Properties>get(Base.ContextKeys.CONFIGURATION).getProperty(Authn.ConfigKeys.MAPPING_PLUGIN); Line 93: Authz.InvokeKeys.QUERY_FLAGS, Line 94: Authz.QueryFlags.RESOLVE_GROUPS | Authz.QueryFlags.RESOLVE_GROUPS_RECURSIVE Line 95: )); Line 96: session.setAttribute(SSOUtils.SSO_AUTHZ_ATTR_NAME, authz.getContext().get(Base.ContextKeys.INSTANCE_NAME)); Line 97: session.setAttribute(SSOUtils.SSO_PRINCIPAL_RECORD_ATTR_NAME, output.get(Authz.InvokeKeys.PRINCIPAL_RECORD)); please also store auth record, it will be handy one day. Line 98: session.setAttribute(SSOUtils.SSO_PRINCIPAL_ID_ATTR_NAME, Line 99: SSOUserManager.getInstance().getUserIdByExternalId( Line 100: (String) authz.getContext().get(Base.ContextKeys.INSTANCE_NAME), Line 101: (String) ((ExtMap) output.get(Authz.InvokeKeys.PRINCIPAL_RECORD)).get(Authz.PrincipalRecord.ID))); Line 94: Authz.QueryFlags.RESOLVE_GROUPS | Authz.QueryFlags.RESOLVE_GROUPS_RECURSIVE Line 95: )); Line 96: session.setAttribute(SSOUtils.SSO_AUTHZ_ATTR_NAME, authz.getContext().get(Base.ContextKeys.INSTANCE_NAME)); Line 97: session.setAttribute(SSOUtils.SSO_PRINCIPAL_RECORD_ATTR_NAME, output.get(Authz.InvokeKeys.PRINCIPAL_RECORD)); Line 98: session.setAttribute(SSOUtils.SSO_PRINCIPAL_ID_ATTR_NAME, this is confusing as it is not principal id... but engine id... I suggest SSO_ENGINE_PRINCIPAL_ID_ATTR_NAME Line 99: SSOUserManager.getInstance().getUserIdByExternalId( Line 100: (String) authz.getContext().get(Base.ContextKeys.INSTANCE_NAME), Line 101: (String) ((ExtMap) output.get(Authz.InvokeKeys.PRINCIPAL_RECORD)).get(Authz.PrincipalRecord.ID))); Line 102: } http://gerrit.ovirt.org/#/c/36119/11/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOExtensionsManager.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOExtensionsManager.java: Line 30: } Line 31: } Line 32: } Line 33: return instance; Line 34: } please drop the singleton, store reference in webapp context, initialize at webapp startup. Line 35: Line 36: public SSOExtensionsManager() { Line 37: super(); Line 38: getGlobalContext().put( http://gerrit.ovirt.org/#/c/36119/11/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUserManager.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUserManager.java: Line 12: Line 13: public class SSOUserManager { Line 14: Line 15: private DataSource ds; Line 16: private static final SSOUserManager instance = new SSOUserManager(); here too, we should avoid singletons. Line 17: Line 18: public static SSOUserManager getInstance() { Line 19: return instance; Line 20: } Line 19: return instance; Line 20: } Line 21: Line 22: public SSOUserManager() { Line 23: ds = EjbUtils.findResource(ContainerManagedResourceType.DATA_SOURCE); how can it knows what data source? Line 24: Line 25: if (ds == null) { Line 26: throw new RuntimeException("Failed to obtain data source"); Line 27: } Line 26: throw new RuntimeException("Failed to obtain data source"); Line 27: } Line 28: } Line 29: Line 30: public String getUserIdByExternalId(String domain, String externalId) throws SQLException { I think I commented this a lot of times, please do not use domain term anywhere in code, this is authz although in database it is by mistake domain. Line 31: Connection connection = null; Line 32: PreparedStatement ps = null; Line 33: ResultSet rs = null; Line 34: String userId = null; Line 33: ResultSet rs = null; Line 34: String userId = null; Line 35: try { Line 36: connection = ds.getConnection(); Line 37: ps = connection.prepareStatement("SELECT users.* FROM users WHERE domain = ? AND external_id = ?"); same, any reason to select * and not specific field? Line 38: ps.setString(1, domain); Line 39: ps.setString(2, externalId); Line 40: Line 41: rs = ps.executeQuery(); Line 69: ps.setString(2, externalId); Line 70: Line 71: rs = ps.executeQuery(); Line 72: if (rs.next()) { Line 73: groupId = getUUIDDefaultEmpty(rs, "id"); instead of long story for function name, just add the default as parameter, much like ExtMap. groupId = getUUID(rs, "id", UUID.fromString("00000000-0000-0000-0000-000000000000")); Line 74: } Line 75: } finally { Line 76: DbUtils.closeQuietly(rs, ps, connection); Line 77: } Line 72: if (rs.next()) { Line 73: groupId = getUUIDDefaultEmpty(rs, "id"); Line 74: } Line 75: } finally { Line 76: DbUtils.closeQuietly(rs, ps, connection); not sure why you need this, please use try-with-resources. Line 77: } Line 78: return groupId; Line 79: } Line 80: http://gerrit.ovirt.org/#/c/36119/11/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 33: redirectUrl.append("/login?"); Line 34: Line 35: Set<String> groupIds = new HashSet<>(); Line 36: for (ExtMap group : principalRecord.get(Authz.PrincipalRecord.GROUPS, Collections.<ExtMap>emptyList())) { Line 37: String groupId = SSOUserManager.getInstance().getGroupByExternalId(profile, group.<String>get(Authz.GroupRecord.ID)); if you translate group from external to internal here, why don't you do the same for user? I mean why doing two similar activities in two different places? Line 38: if (groupId != null) { Line 39: groupIds.add(groupId); Line 40: } Line 41: } Line 39: groupIds.add(groupId); Line 40: } Line 41: } Line 42: Line 43: StringBuilder params = new StringBuilder(""); new StringBuilder() will have the same effect. Line 44: appendParameter(response, params, "id", userId); Line 45: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 46: appendParameter(response, params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 47: appendParameter(response, params, "email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 44: appendParameter(response, params, "id", userId); Line 45: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 46: appendParameter(response, params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 47: appendParameter(response, params, "email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 48: appendParameter(response, params, "groupIds", StringUtils.join(groupIds, ",")); you cannot know if there is a comma within groupIds... so you must escape/encode each. Line 49: redirectUrl.append(params.toString()); Line 50: response.sendRedirect(redirectUrl.toString()); Line 51: } Line 52: Line 50: response.sendRedirect(redirectUrl.toString()); Line 51: } Line 52: Line 53: private static void appendParameter(HttpServletResponse response, StringBuilder params, String name, String value) { Line 54: if (StringUtils.isNotEmpty(value)) { no need for this conditional you can expect & at beginning as well as far as I know. Line 55: params.append("&"); Line 56: params.append(name); Line 57: params.append("="); Line 58: params.append(response.encodeURL(value)); -- 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: 11 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
