Eli Mesika has uploaded a new change for review. Change subject: fix handling of admin user while login ......................................................................
fix handling of admin user while login GetAllRolesByUserIdAndGroupIds was called whenever a user was logged in in order to find out if the user is an admin or not. In original code, all roles were retrieved and the check for the admin role type was done in a loop inside the MultiLevelAdministrationHandler::isAdminUser() code. Since the required functionality is only to find if any admin role is attached to the logging user, I had changed the SP code to return only one role (if exists) and have the role_type checked in the SP level. SP name and DAO method names were changed to match the real functionality. Change-Id: I6aa489199c904008e46a650f11877091931ee5de Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1156465 Signed-off-by: Eli Mesika <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultiLevelAdministrationHandler.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAO.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAODbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RoleDAOTest.java M packaging/dbscripts/multi_level_administration_sp.sql 5 files changed, 19 insertions(+), 22 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/51/34551/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultiLevelAdministrationHandler.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultiLevelAdministrationHandler.java index e9e5356..304fa0f 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultiLevelAdministrationHandler.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MultiLevelAdministrationHandler.java @@ -6,7 +6,6 @@ import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.businessentities.Permissions; import org.ovirt.engine.core.common.businessentities.Role; -import org.ovirt.engine.core.common.businessentities.RoleType; import org.ovirt.engine.core.common.businessentities.aaa.DbUser; import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; @@ -55,13 +54,10 @@ */ public static boolean isAdminUser(DbUser user) { List<Role> userRoles = - getRoleDAO().getAllForUserAndGroups(user.getId(), StringUtils.join(user.getGroupIds(), ",")); - - for (Role r : userRoles) { - if (r.getType() == RoleType.ADMIN) { - log.debug("LoginAdminUser: User logged to admin using role '{}'", r.getname()); - return true; - } + getRoleDAO().getAnyAdminRoleForUserAndGroups(user.getId(), StringUtils.join(user.getGroupIds(), ",")); + if (userRoles.size() > 0) { + log.debug("LoginAdminUser: User logged to admin using role '{}'", userRoles.get(0).getname()); + return true; } return false; } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAO.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAO.java index 2b51f56..b732e0c 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAO.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAO.java @@ -22,7 +22,7 @@ Role getByName(String name); /** - * This method gets the Roles for the given user and its groups. The purpose of this method is to be able to get + * This method gets the admin Roles for the given user and its groups. The purpose of this method is to be able to get * roles even if the user is not already added to DB, by getting the roles for his groups * * @param userId @@ -31,10 +31,10 @@ * comma delimited list of group IDs of the user * @return the list of the roles */ - List<Role> getAllForUserAndGroups(Guid userId, String groupIds); + List<Role> getAnyAdminRoleForUserAndGroups(Guid userId, String groupIds); /** - * This method gets the Roles for the given user and its groups. The purpose of this method is to be able to get + * This method gets the admin Roles for the given user and its groups. The purpose of this method is to be able to get * roles even if the user is not already added to DB, by getting the roles for his groups * * @param userId @@ -45,5 +45,5 @@ * application mode to obtain roles for * @return the list of the roles */ - List<Role> getAllForUserAndGroups(Guid userId, String groupIds, int appMode); + List<Role> getAnyAdminRoleForUserAndGroups(Guid userId, String groupIds, int appMode); } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAODbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAODbFacadeImpl.java index ccb930a..b54d113 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAODbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RoleDAODbFacadeImpl.java @@ -65,18 +65,18 @@ } @Override - public List<Role> getAllForUserAndGroups(Guid id, String groupIds) { + public List<Role> getAnyAdminRoleForUserAndGroups(Guid id, String groupIds) { Integer appMode = Config.<Integer> getValue(ConfigValues.ApplicationMode); - return getAllForUserAndGroups(id, groupIds, appMode.intValue()); + return getAnyAdminRoleForUserAndGroups(id, groupIds, appMode.intValue()); } @Override - public List<Role> getAllForUserAndGroups(Guid id, String groupIds, int appMode) { + public List<Role> getAnyAdminRoleForUserAndGroups(Guid id, String groupIds, int appMode) { MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() .addValue("user_id", id) .addValue("group_ids", groupIds) .addValue("app_mode", appMode); - return getCallsHandler().executeReadList("GetAllRolesByUserIdAndGroupIds", + return getCallsHandler().executeReadList("GetAnyAdminRoleByUserIdAndGroupIds", RolesRowMapper.instance, parameterSource); } diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RoleDAOTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RoleDAOTest.java index 754a02a..57cf6ad 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RoleDAOTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RoleDAOTest.java @@ -98,19 +98,19 @@ */ @Test public void testGetAllForUsersAndGroups() { - List<Role> result = dao.getAllForUserAndGroups(USER_ID, + List<Role> result = dao.getAnyAdminRoleForUserAndGroups(USER_ID, GROUP_IDS, ApplicationMode.AllModes.getValue()); assertNotNull(result); assertFalse(result.isEmpty()); assertEquals(16, result.size()); - List<Role> result1 = dao.getAllForUserAndGroups(USER_ID, + List<Role> result1 = dao.getAnyAdminRoleForUserAndGroups(USER_ID, GROUP_IDS, ApplicationMode.VirtOnly.getValue()); assertNotNull(result1); assertFalse(result1.isEmpty()); assertEquals(15, result1.size()); - List<Role> result2 = dao.getAllForUserAndGroups(USER_ID, + List<Role> result2 = dao.getAnyAdminRoleForUserAndGroups(USER_ID, GROUP_IDS, ApplicationMode.GlusterOnly.getValue()); assertNotNull(result2); assertFalse(result2.isEmpty()); @@ -119,7 +119,7 @@ @Test public void testGetAllForUsersAndGroupsInvalidUserAndGroups() { - List<Role> result = dao.getAllForUserAndGroups(Guid.newGuid(), + List<Role> result = dao.getAnyAdminRoleForUserAndGroups(Guid.newGuid(), Guid.newGuid().toString(), ApplicationMode.AllModes.getValue()); assertNotNull(result); assertTrue(result.isEmpty()); diff --git a/packaging/dbscripts/multi_level_administration_sp.sql b/packaging/dbscripts/multi_level_administration_sp.sql index 383380e..e6a4c94 100644 --- a/packaging/dbscripts/multi_level_administration_sp.sql +++ b/packaging/dbscripts/multi_level_administration_sp.sql @@ -306,7 +306,7 @@ -Create or replace FUNCTION GetAllRolesByUserIdAndGroupIds(v_user_id UUID, v_group_ids text, v_app_mode INTEGER) +Create or replace FUNCTION GetAnyAdminRoleByUserIdAndGroupIds(v_user_id UUID, v_group_ids text, v_app_mode INTEGER) RETURNS SETOF roles STABLE AS $procedure$ BEGIN @@ -314,8 +314,9 @@ FROM roles INNER JOIN permissions ON permissions.role_id = roles.id WHERE (roles.app_mode & v_app_mode) > 0 + AND role_type = 1 -- admin AND (permissions.ad_element_id = v_user_id - or permissions.ad_element_id in(select id from getElementIdsByIdAndGroups(v_user_id, v_group_ids))); + or permissions.ad_element_id in(select id from getElementIdsByIdAndGroups(v_user_id, v_group_ids))) LIMIT 1; END; $procedure$ LANGUAGE plpgsql; -- To view, visit http://gerrit.ovirt.org/34551 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6aa489199c904008e46a650f11877091931ee5de Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eli Mesika <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
