Alon Bar-Lev has posted comments on this change. Change subject: core : Add engine sso ......................................................................
Patch Set 14: (10 comments) http://gerrit.ovirt.org/#/c/36119/14/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 34: public void init(ServletConfig config) throws ServletException { Line 35: super.init(config); Line 36: SSOLocalConfig localConfig = new SSOLocalConfig(); Line 37: SSOExtensionsManager ssoExtensionManager = new SSOExtensionsManager(localConfig); Line 38: ssoExtensionManager.initialize(); it is not singleton now, no reason why the constructor will not initialize, then this sequence is simpler: config.getServletContext().setAttribute(SSOUtils.SSO_LOCAL_CONFIG, localConfig); config.getServletContext().setAttribute(SSOUtils.EXTENSION_MANAGER, new SSOExtensionsManager(localConfig)); Line 39: config.getServletContext().setAttribute(SSOUtils.EXTENSION_MANAGER, ssoExtensionManager); Line 40: config.getServletContext().setAttribute(SSOUtils.USER_GROUP_MANAGER, new SSOUserGroupManager()); Line 41: config.getServletContext().setAttribute(SSOUtils.SSO_LOCAL_CONFIG, localConfig); Line 42: } Line 37: SSOExtensionsManager ssoExtensionManager = new SSOExtensionsManager(localConfig); Line 38: ssoExtensionManager.initialize(); Line 39: config.getServletContext().setAttribute(SSOUtils.EXTENSION_MANAGER, ssoExtensionManager); Line 40: config.getServletContext().setAttribute(SSOUtils.USER_GROUP_MANAGER, new SSOUserGroupManager()); Line 41: config.getServletContext().setAttribute(SSOUtils.SSO_LOCAL_CONFIG, localConfig); why doing this in specific servlet? isn't there a listener to enable webapp initialization? it is not healthy to put in specific servlet as I do not think that servlet load order is guaranteed. so there must be something that enables initialization before servlets. if not, please move this to separate servlet that only initialize our webapp context. Line 42: } Line 43: Line 44: @Override Line 45: protected void service(HttpServletRequest request, HttpServletResponse response) Line 72: request.getRequestDispatcher("/WEB-INF/login.jsp?msg=" + ex.getMessage()).forward(request, response); Line 73: } catch (SQLException ex) { Line 74: log.error("Internal Database Error", ex); Line 75: request.getRequestDispatcher("/WEB-INF/login.jsp?msg=" + DB_ERR_MSG).forward(request, response); Line 76: } maybe better to handle the errors in one place, throw exception when sql issue is. btw: why AuthenticationError and not AuthenticationException? try { try { } catch(SQLException e) { log.error(e); throw new AuthenticationError("Internal database error", e); } } catch (AuthenticationError e) { log.debug("Exception", e); request.getRequestDispatcher("/WEB-INF/login.jsp?msg=" + ex.getMessage()).forward(reques t, response); } Line 77: } Line 78: http://gerrit.ovirt.org/#/c/36119/14/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 39: throw new RuntimeException(ex); Line 40: } Line 41: } else { Line 42: request.getRequestDispatcher("/WEB-INF/ovirt.jsp").forward(request, response); Line 43: } module empty = error please... just throw an exception. anyway, even if not, when you add conditionals trivial block first. so: if (StringUtils.isEmpty(module)) { request.getRequestDispatcher("/WEB-INF/ovirt.jsp").forward(request, response); } else { non trivial logic } but again, I do not see these utilities as user usable ones, so there is no need to add end user logic. Line 44: } http://gerrit.ovirt.org/#/c/36119/14/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 20: throw new RuntimeException("Error looking up resource " + DATA_SOURCE); Line 21: } Line 22: if (ds == null) { Line 23: throw new RuntimeException("Failed to obtain data source"); Line 24: } I do not think there a reason not to add try/catch to the entire function to make it nicer. try { ds = (DataSource) new InitialContext().lookup(DATA_SOURCE); if (ds == null) { throw new RuntimeException("Failed to obtain data source"); } } catch (NamingException ex) { throw new RuntimeException("Failed to obtain data source"); } Line 25: } Line 26: Line 27: public String getUserIdByExternalId(String profile, String externalId) throws SQLException { Line 28: return executeQuery("SELECT user_id FROM users WHERE domain = ? AND external_id = ?", profile, externalId, "user_id"); Line 23: throw new RuntimeException("Failed to obtain data source"); Line 24: } Line 25: } Line 26: Line 27: public String getUserIdByExternalId(String profile, String externalId) throws SQLException { authz name not profile Line 28: return executeQuery("SELECT user_id FROM users WHERE domain = ? AND external_id = ?", profile, externalId, "user_id"); Line 29: } Line 30: Line 31: public String getGroupByExternalId(String profile, String externalId) throws SQLException { Line 27: public String getUserIdByExternalId(String profile, String externalId) throws SQLException { Line 28: return executeQuery("SELECT user_id FROM users WHERE domain = ? AND external_id = ?", profile, externalId, "user_id"); Line 29: } Line 30: Line 31: public String getGroupByExternalId(String profile, String externalId) throws SQLException { authz name not profile Line 32: return executeQuery("SELECT id FROM ad_groups WHERE domain = ? AND external_id = ?", profile, externalId, "id"); Line 33: } Line 34: Line 35: private String executeQuery(String sql, String profile, String externalId, String columnName) throws SQLException { Line 31: public String getGroupByExternalId(String profile, String externalId) throws SQLException { Line 32: return executeQuery("SELECT id FROM ad_groups WHERE domain = ? AND external_id = ?", profile, externalId, "id"); Line 33: } Line 34: Line 35: private String executeQuery(String sql, String profile, String externalId, String columnName) throws SQLException { authz name not profile Line 36: String retVal = null; Line 37: try ( Line 38: Connection connection = ds.getConnection(); Line 39: PreparedStatement ps = connection.prepareStatement(sql); http://gerrit.ovirt.org/#/c/36119/14/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 41: String userId = userGroupManager.getUserIdByExternalId(authzName, principalRecord.<String>get(Authz.PrincipalRecord.ID)); Line 42: if (userId == null) { Line 43: userId = DEFAULT_USER_ID; Line 44: } Line 45: StringBuilder params = new StringBuilder(); move this before you get user id to be symmetric with how groups are constructed. Line 46: appendParameter(response, params, "id", userId); Line 47: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 48: appendParameter(response, params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 49: appendParameter(response, params, "email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 42: if (userId == null) { Line 43: userId = DEFAULT_USER_ID; Line 44: } Line 45: StringBuilder params = new StringBuilder(); Line 46: appendParameter(response, params, "id", userId); userId or similar please. Line 47: String principal = principalRecord.get(Authz.PrincipalRecord.PRINCIPAL); Line 48: appendParameter(response, params, "username", principal != null ? principal : principalRecord.<String>get(Authz.PrincipalRecord.NAME)); Line 49: appendParameter(response, params, "email", principalRecord.<String>get(Authz.PrincipalRecord.EMAIL)); Line 50: principalRecord.put(Authz.PrincipalRecord.GROUPS, flatGroups(principalRecord, Authz.PrincipalRecord.GROUPS, new ArrayList<ExtMap>())); -- 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: 14 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
