Martin Peřina has posted comments on this change. Change subject: fix handling of admin user while login ......................................................................
Patch Set 1: (3 comments) http://gerrit.ovirt.org/#/c/34551/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultiLevelAdministrationHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultiLevelAdministrationHandler.java: Line 53: * @return True if user is admin Line 54: */ Line 55: public static boolean isAdminUser(DbUser user) { Line 56: List<Role> userRoles = Line 57: getRoleDAO().getAnyAdminRoleForUserAndGroups(user.getId(), StringUtils.join(user.getGroupIds(), ",")); Please move conversion from List to comma separated string to DAO. IMHO it's better to make type conversion at the last moment and only at places where it's really needed. Line 58: if (userRoles.size() > 0) { Line 59: log.debug("LoginAdminUser: User logged to admin using role '{}'", userRoles.get(0).getname()); Line 60: return true; Line 61: } Line 54: */ Line 55: public static boolean isAdminUser(DbUser user) { Line 56: List<Role> userRoles = Line 57: getRoleDAO().getAnyAdminRoleForUserAndGroups(user.getId(), StringUtils.join(user.getGroupIds(), ",")); Line 58: if (userRoles.size() > 0) { > i think !userRoles.isEmpty() is nicer. +1 Line 59: log.debug("LoginAdminUser: User logged to admin using role '{}'", userRoles.get(0).getname()); Line 60: return true; Line 61: } Line 62: return false; http://gerrit.ovirt.org/#/c/34551/1/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RoleDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RoleDAOTest.java: Line 96: /** Line 97: * Ensures the right collection of roles is returned Line 98: */ Line 99: @Test Line 100: public void testGetAllForUsersAndGroups() { You need to refactor this test because logic of the getAnyAdminRoleForUserAndGroups differs from previous getAllForUserAndGroups Line 101: List<Role> result = dao.getAnyAdminRoleForUserAndGroups(USER_ID, Line 102: GROUP_IDS, ApplicationMode.AllModes.getValue()); Line 103: assertNotNull(result); Line 104: assertFalse(result.isEmpty()); -- To view, visit http://gerrit.ovirt.org/34551 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6aa489199c904008e46a650f11877091931ee5de Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eli Mesika <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
