Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 12: (16 comments) ok... 830 lines and counting (down) :) http://gerrit.ovirt.org/#/c/36119/12/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 73: log.error("Unable to Authenticate User", ex); Line 74: request.getRequestDispatcher("/WEB-INF/login.jsp?" + ex.getMessage()).forward(request, response); Line 75: } catch (SQLException ex) { Line 76: log.error("Internal Database Error", ex); Line 77: request.getRequestDispatcher("/WEB-INF/login.jsp?" + DB_ERR_MSG).forward(request, response); you have 2 times forward to login.jsp, I think it can be at one place... try { if no credentials throw "credentials required" authenticate() catch() { present login } Line 78: } Line 79: } Line 80: http://gerrit.ovirt.org/#/c/36119/12/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 23: HttpSession session = request.getSession(true); Line 24: if (SSOUtils.isUserAuthenticated(session)) { Line 25: handleAuthenticatedUser(session, request, response); Line 26: } else { Line 27: request.getRequestDispatcher("/WEB-INF/login.jsp").forward(request, response); in future this will not be required as well as the webadmin will redirect to sso not the other way around. Line 28: } Line 29: } Line 30: Line 31: /** Line 36: */ Line 37: private void handleAuthenticatedUser(HttpSession session, HttpServletRequest request, HttpServletResponse response) Line 38: throws ServletException, IOException { Line 39: String module = request.getParameter(SSOUtils.MODULE); Line 40: if (StringUtils.isNotEmpty(module)) { if there is no module this should be an error, no reason to handle it "nicely". Line 41: try { Line 42: SSOUtils.redirectToModule( Line 43: (SSOUserGroupManager) getServletContext().getAttribute(SSOUtils.USER_GROUP_MANAGER), Line 44: (ExtMap) session.getAttribute(SSOUtils.SSO_PRINCIPAL_RECORD_ATTR_NAME), http://gerrit.ovirt.org/#/c/36119/12/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 14: import java.util.Properties; Line 15: Line 16: public class AuthenticationUtils { Line 17: Line 18: public static void handleCredentials(SSOExtensionsManager ssoExtensionManager, can't you get the context out of HttpSession::getServletContext()? Line 19: HttpSession session, Line 20: String user, Line 21: String password, Line 22: String profile) http://gerrit.ovirt.org/#/c/36119/12/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 16: Line 17: public class SSOExtensionsManager extends ExtensionsManager { Line 18: Line 19: private static final String ENGINE_EXTENSION_ENABLED = "ENGINE_EXTENSION_ENABLED_"; Line 20: private static final SSOLocalConfig localConfig = new SSOLocalConfig(); can we get this from context as well? Line 21: private static Logger log = LoggerFactory.getLogger(SSOExtensionsManager.class); Line 22: Line 23: public SSOExtensionsManager() { Line 24: super(); Line 21: private static Logger log = LoggerFactory.getLogger(SSOExtensionsManager.class); Line 22: Line 23: public SSOExtensionsManager() { Line 24: super(); Line 25: getGlobalContext().put( where does this getGlobalContext() comes from? Line 26: Base.GlobalContextKeys.APPLICATION_NAME, Line 27: Base.ApplicationNames.OVIRT_ENGINE); Line 28: } Line 29: http://gerrit.ovirt.org/#/c/36119/12/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 31: try (Connection connection = ds.getConnection(); PreparedStatement ps = connection.prepareStatement(sql);) { Line 32: ps.setString(1, profile); Line 33: ps.setString(2, externalId); Line 34: Line 35: ResultSet rs = ps.executeQuery(); try-with-resources here too Line 36: if (rs.next()) { Line 37: userId = getUUIDDefaultEmpty(rs, "user_id", UUID.fromString("00000000-0000-0000-0000-000000000000")); Line 38: } Line 39: } Line 33: ps.setString(2, externalId); Line 34: Line 35: ResultSet rs = ps.executeQuery(); Line 36: if (rs.next()) { Line 37: userId = getUUIDDefaultEmpty(rs, "user_id", UUID.fromString("00000000-0000-0000-0000-000000000000")); can't it be generic so you get whatever you need based on type? if not, the last parameter is default anyway... no need to make function name longer... getUUID(rs, "user_id") -> returns null if no getUUID(rs, "user_id", UUID.fromString()) -> returns default if none BTW: this function is almost useless... as what I expect is: <UUID>getField(rs.getObject("user_id"), UUID.fromString("...")) protected static Object getField(Object o, Object default) { return o == null ? default : o; } I am unsure what rs.wasNull() is doing... we must look into specific field, checking the entire result for null is not something we should do. Line 38: } Line 39: } Line 40: return userId; Line 41: } Line 36: if (rs.next()) { Line 37: userId = getUUIDDefaultEmpty(rs, "user_id", UUID.fromString("00000000-0000-0000-0000-000000000000")); Line 38: } Line 39: } Line 40: return userId; but... while I read the code, the place to return the generic uuid for user that was not found is at caller not at dao level... just like the handling of groups that were not found. so this function should return plain null if user not found, reducing the application logic from dao level. Line 41: } Line 42: Line 43: protected static String getUUIDDefaultEmpty(ResultSet resultSet, String columnName, UUID defaultVal) throws SQLException { Line 44: UUID uuid = (UUID) resultSet.getObject(columnName); Line 49: } Line 50: Line 51: public String getGroupByExternalId(String profile, String externalId) throws SQLException { Line 52: String groupId = null; Line 53: String sql = "SELECT id FROM ad_groups WHERE domain = ? AND external_id = ?"; both functions should be the same, just the query differs... think of it. Line 54: try (Connection connection = ds.getConnection(); PreparedStatement ps = connection.prepareStatement(sql);) { Line 55: ps.setString(1, profile); Line 56: ps.setString(2, externalId); Line 57: Line 50: Line 51: public String getGroupByExternalId(String profile, String externalId) throws SQLException { Line 52: String groupId = null; Line 53: String sql = "SELECT id FROM ad_groups WHERE domain = ? AND external_id = ?"; Line 54: try (Connection connection = ds.getConnection(); PreparedStatement ps = connection.prepareStatement(sql);) { usually nicer to put each in own line. try ( Connection connection = ds.getConnection(); PreparedStatement ps = connection.prepareStatement(sql); ) { Line 55: ps.setString(1, profile); Line 56: ps.setString(2, externalId); Line 57: Line 58: ResultSet rs = ps.executeQuery(); Line 54: try (Connection connection = ds.getConnection(); PreparedStatement ps = connection.prepareStatement(sql);) { Line 55: ps.setString(1, profile); Line 56: ps.setString(2, externalId); Line 57: Line 58: ResultSet rs = ps.executeQuery(); try-with-resource as well. Line 59: if (rs.next()) { Line 60: groupId = getUUIDDefaultEmpty(rs, "id", UUID.fromString("00000000-0000-0000-0000-000000000000")); Line 61: } Line 62: } http://gerrit.ovirt.org/#/c/36119/12/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 35: redirectUrl.append("/login?"); Line 36: Line 37: Set<String> groupIds = new HashSet<>(); Line 38: for (ExtMap group : principalRecord.get(Authz.PrincipalRecord.GROUPS, Collections.<ExtMap>emptyList())) { Line 39: String groupId = userGroupManager.getGroupByExternalId(profile, group.<String>get(Authz.GroupRecord.ID)); you should take the authz name from the session, not the profile. Line 40: if (groupId != null) { Line 41: groupIds.add(response.encodeURL(groupId)); Line 42: } Line 43: } Line 39: String groupId = userGroupManager.getGroupByExternalId(profile, group.<String>get(Authz.GroupRecord.ID)); Line 40: if (groupId != null) { Line 41: groupIds.add(response.encodeURL(groupId)); Line 42: } Line 43: } hmm... sorry that I did not see that... you need to do this recursive. see DirectoryUtils::flatGroups Line 44: Line 45: String userId = userGroupManager.getUserIdByExternalId(profile, principalRecord.<String>get(Authz.PrincipalRecord.ID)); Line 46: StringBuilder params = new StringBuilder(); Line 47: appendParameter(response, params, "id", userId); Line 41: groupIds.add(response.encodeURL(groupId)); Line 42: } Line 43: } Line 44: Line 45: String userId = userGroupManager.getUserIdByExternalId(profile, principalRecord.<String>get(Authz.PrincipalRecord.ID)); if null put the generic id here you should take the authz name from the session, not the profile. Line 46: StringBuilder params = new StringBuilder(); Line 47: appendParameter(response, params, "id", userId); Line 48: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 49: appendParameter(response, params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 47: appendParameter(response, params, "id", userId); Line 48: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 49: appendParameter(response, params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 50: appendParameter(response, params, "email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 51: appendParameter(response, params, "groupIds", StringUtils.join(groupIds, ",")); still need to encode group id as you cannot know what it contains, maybe add multiple groupId=&groupId= Line 52: redirectUrl.append(params.toString()); Line 53: response.sendRedirect(redirectUrl.toString()); Line 54: } Line 55: -- 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: 12 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
