From 6f1e834753691cead69b8b308724bece47ce9510 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Sat, 30 Apr 2016 01:05:39 +0530 Subject: [PATCH] CLOUDSTACK-8562: Make role permissions orderable - Makes role permissions orderable in UI/backend - Role permissions evaluated by fixed order - Rules draggable in UI - Migration script adds a default order Signed-off-by: Rohit Yadav --- .../apache/cloudstack/acl/RoleService.java | 7 +- .../apache/cloudstack/api/ApiConstants.java | 1 + .../admin/acl/UpdateRolePermissionCmd.java | 51 ++++++----- .../classes/resources/messages.properties | 1 + .../acl/dao/RolePermissionsDao.java | 15 ++-- .../acl/dao/RolePermissionsDaoImpl.java | 90 ++++++++++++------- .../cloudstack/acl/RoleManagerImpl.java | 4 +- test/integration/smoke/test_dynamicroles.py | 50 +++++++++-- tools/marvin/marvin/lib/base.py | 2 +- ui/dictionary.jsp | 1 + ui/scripts/roles.js | 30 ++++++- 11 files changed, 166 insertions(+), 86 deletions(-) diff --git a/api/src/org/apache/cloudstack/acl/RoleService.java b/api/src/org/apache/cloudstack/acl/RoleService.java index 47d7e906b06..59eef51e782 100644 --- a/api/src/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/org/apache/cloudstack/acl/RoleService.java @@ -39,11 +39,10 @@ public interface RoleService { RolePermission createRolePermission(final Role role, final Rule rule, final RolePermission.Permission permission, final String description); /** * updateRolePermission updates the order/position of an role permission - * The list of role permissions is treated and consumed as an ordered linked list - * @param rolePermission The role permission that needs to be re-ordered - * @param parentRolePermission The parent role permission after which the role permission should be placed + * @param role The role whose permissions needs to be re-ordered + * @param newOrder The new list of ordered role permissions */ - boolean updateRolePermission(final RolePermission rolePermission, final RolePermission parentRolePermission); + boolean updateRolePermission(final Role role, final List newOrder); boolean deleteRolePermission(final RolePermission rolePermission); List listRoles(); diff --git a/api/src/org/apache/cloudstack/api/ApiConstants.java b/api/src/org/apache/cloudstack/api/ApiConstants.java index caeabb33614..51acf38728e 100755 --- a/api/src/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/org/apache/cloudstack/api/ApiConstants.java @@ -359,6 +359,7 @@ public class ApiConstants { public static final String ROLE_NAME = "rolename"; public static final String PERMISSION = "permission"; public static final String RULE = "rule"; + public static final String RULE_ORDER = "ruleorder"; public static final String USER = "user"; public static final String ACTIVE_ONLY = "activeonly"; public static final String TOKEN = "token"; diff --git a/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRolePermissionCmd.java b/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRolePermissionCmd.java index 24f83b94d84..055265c5ccc 100644 --- a/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRolePermissionCmd.java +++ b/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRolePermissionCmd.java @@ -18,6 +18,7 @@ package org.apache.cloudstack.api.command.admin.acl; import com.cloud.user.Account; +import org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RolePermission; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.APICommand; @@ -28,9 +29,13 @@ import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.Parameter; import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.response.RolePermissionResponse; +import org.apache.cloudstack.api.response.RoleResponse; import org.apache.cloudstack.api.response.SuccessResponse; import org.apache.cloudstack.context.CallContext; +import java.util.ArrayList; +import java.util.List; + @APICommand(name = UpdateRolePermissionCmd.APINAME, description = "Updates a role permission order", responseObject = SuccessResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.9.0", @@ -42,32 +47,24 @@ public class UpdateRolePermissionCmd extends BaseCmd { //////////////// API parameters ///////////////////// ///////////////////////////////////////////////////// - @Parameter(name = ApiConstants.ID, type = CommandType.UUID, required = true, entityType = RolePermissionResponse.class, - description = "ID of the role permission", validations = {ApiArgValidator.PositiveNumber}) - private Long ruleId; + @Parameter(name = ApiConstants.ROLE_ID, type = CommandType.UUID, required = true, entityType = RoleResponse.class, + description = "ID of the role", validations = {ApiArgValidator.PositiveNumber}) + private Long roleId; - @Parameter(name = ApiConstants.PARENT, type = CommandType.STRING, required = true, entityType = RolePermissionResponse.class, - description = "The parent role permission uuid, use 0 to move this rule at the top of the list", validations = {ApiArgValidator.NotNullOrEmpty}) - private String parentRuleUuid; + @Parameter(name = ApiConstants.RULE_ORDER, type = CommandType.LIST, collectionType = CommandType.UUID, required = true, entityType = RolePermissionResponse.class, + description = "The parent role permission uuid, use 0 to move this rule at the top of the list") + private List rulePermissionOrder; ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// - public Long getRuleId() { - return ruleId; + public Long getRoleId() { + return roleId; } - public RolePermission getParentRolePermission() { - // A null or 0 previous id means this rule is moved to the top of the list - if (parentRuleUuid.equals("0")) { - return null; - } - final RolePermission parentRolePermission = roleService.findRolePermissionByUuid(parentRuleUuid); - if (parentRolePermission == null) { - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid parent role permission id provided"); - } - return parentRolePermission; + public List getRulePermissionOrder() { + return rulePermissionOrder; } ///////////////////////////////////////////////////// @@ -86,12 +83,20 @@ public class UpdateRolePermissionCmd extends BaseCmd { @Override public void execute() { - final RolePermission rolePermission = roleService.findRolePermission(getRuleId()); - if (rolePermission == null) { - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role permission id provided"); + final Role role = roleService.findRole(getRoleId()); + if (role == null) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role id provided"); } - CallContext.current().setEventDetails("Role permission id: " + rolePermission.getId() + ", parent role permission uuid:" + parentRuleUuid); - boolean result = roleService.updateRolePermission(rolePermission, getParentRolePermission()); + CallContext.current().setEventDetails("Reordering permissions for role id: " + role.getId()); + final List rolePermissionsOrder = new ArrayList<>(); + for (Long rolePermissionId : getRulePermissionOrder()) { + final RolePermission rolePermission = roleService.findRolePermission(rolePermissionId); + if (rolePermission == null) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Provided role permission(s) do not exist"); + } + rolePermissionsOrder.add(rolePermission); + } + boolean result = roleService.updateRolePermission(role, rolePermissionsOrder); SuccessResponse response = new SuccessResponse(getCommandName()); response.setSuccess(result); setResponseObject(response); diff --git a/client/WEB-INF/classes/resources/messages.properties b/client/WEB-INF/classes/resources/messages.properties index badf61bff88..f95331c5994 100644 --- a/client/WEB-INF/classes/resources/messages.properties +++ b/client/WEB-INF/classes/resources/messages.properties @@ -1075,6 +1075,7 @@ label.roletype=Role Type label.add.role=Add Role label.edit.role=Edit Role label.delete.role=Delete Role +message.role.ordering.fail=Reordering of rule permissions aborted as the list has changed while you were making changes. Please try again. label.root.certificate=Root certificate label.root.disk.controller=Root disk controller label.root.disk.offering=Root Disk Offering diff --git a/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDao.java b/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDao.java index 337de4b6af1..37544919657 100644 --- a/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDao.java +++ b/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDao.java @@ -18,6 +18,8 @@ package org.apache.cloudstack.acl.dao; import com.cloud.utils.db.GenericDao; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RolePermission; import org.apache.cloudstack.acl.RolePermissionVO; import java.util.List; @@ -32,18 +34,11 @@ public interface RolePermissionsDao extends GenericDao { /** * Moves an existing role permission under a given parent role permission - * @param item an existing role permission - * @param parentId the new parent ID after move for the role permission + * @param role the existing role + * @param newOrder the new role permissions order * @return returns true on success */ - boolean update(final RolePermissionVO item, final RolePermissionVO parent); - - /** - * Removes an existing role permission and updates the ordered linked-list of role's permissions list - * @param id the ID of the role permission to be removed - * @return returns true on success - */ - boolean remove(Long id); + boolean update(final Role role, final List newOrder); /** * Returns ordered linked-list of role permission for a given role diff --git a/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDaoImpl.java b/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDaoImpl.java index 3f9c0f4aec6..960b652a3b4 100644 --- a/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDaoImpl.java +++ b/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDaoImpl.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.acl.dao; +import com.cloud.utils.db.Attribute; import com.cloud.utils.db.Filter; import com.cloud.utils.db.GenericDaoBase; import com.cloud.utils.db.SearchBuilder; @@ -24,6 +25,10 @@ import com.cloud.utils.db.SearchCriteria; import com.cloud.utils.db.Transaction; import com.cloud.utils.db.TransactionCallback; import com.cloud.utils.db.TransactionStatus; +import com.cloud.utils.db.UpdateBuilder; +import com.cloud.utils.exception.CloudRuntimeException; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RolePermission; import org.apache.cloudstack.acl.RolePermissionVO; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -31,7 +36,9 @@ import org.springframework.stereotype.Component; import javax.ejb.Local; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; @Component @Local(value = {RolePermissionsDao.class}) @@ -39,14 +46,20 @@ public class RolePermissionsDaoImpl extends GenericDaoBase RolePermissionsSearch; + private Attribute sortOrderAttribute; public RolePermissionsDaoImpl() { super(); RolePermissionsSearch = createSearchBuilder(); + RolePermissionsSearch.and("uuid", RolePermissionsSearch.entity().getUuid(), SearchCriteria.Op.EQ); RolePermissionsSearch.and("roleId", RolePermissionsSearch.entity().getRoleId(), SearchCriteria.Op.EQ); RolePermissionsSearch.and("sortOrder", RolePermissionsSearch.entity().getSortOrder(), SearchCriteria.Op.EQ); RolePermissionsSearch.done(); + + sortOrderAttribute = _allAttributes.get("sortOrder"); + + assert (sortOrderAttribute != null) : "Couldn't find one of these attributes"; } private boolean updateSortOrder(final RolePermissionVO permissionBeingMoved, final RolePermissionVO parentPermission) { @@ -83,59 +96,68 @@ public class RolePermissionsDaoImpl extends GenericDaoBase permissionsList = findAllByRoleIdSorted(item.getRoleId()); if (permissionsList != null && permissionsList.size() > 0) { - item.setSortOrder(permissionsList.size()); + RolePermission lastRule = permissionsList.get(permissionsList.size() - 1); + item.setSortOrder(lastRule.getSortOrder() + 1); } return super.persist(item); } @Override - public boolean remove(Long id) { - final RolePermissionVO itemToBeRemoved = findById(id); - if (itemToBeRemoved == null) { + public boolean update(final Role role, final List newOrder) { + if (role == null || newOrder == null || newOrder.isEmpty()) { return false; } - boolean updateStatus = true; - final boolean removal = super.remove(id); - if (removal) { - long sortOrder = 0L; - for (final RolePermissionVO permission : findAllByRoleIdSorted(itemToBeRemoved.getRoleId())) { - permission.setSortOrder(sortOrder++); - if (!update(permission.getId(), permission)) { - updateStatus = false; - break; - } - } - } - return removal && updateStatus; - } - - @Override - public boolean update(final RolePermissionVO item, final RolePermissionVO parent) { return Transaction.execute(new TransactionCallback() { @Override public Boolean doInTransaction(TransactionStatus status) { - return updateSortOrder(item, parent); + final String failMessage = "The role's rule permissions list has changed while you were making updates, aborted re-ordering of rules. Please try again."; + final List currentOrder = findAllByRoleIdSorted(role.getId()); + if (role.getId() < 1L || newOrder.size() != currentOrder.size()) { + throw new CloudRuntimeException(failMessage); + } + final Set newOrderSet = new HashSet<>(); + for (final RolePermission permission : newOrder) { + if (permission == null) { + continue; + } + newOrderSet.add(permission.getId()); + } + final Set currentOrderSet = new HashSet<>(); + for (final RolePermission permission : currentOrder) { + currentOrderSet.add(permission.getId()); + } + if (!newOrderSet.equals(currentOrderSet)) { + throw new CloudRuntimeException(failMessage); + } + long sortOrder = 0L; + for (RolePermission rolePermission : newOrder) { + final SearchCriteria sc = RolePermissionsSearch.create(); + sc.setParameters("uuid", rolePermission.getUuid()); + sc.setParameters("roleId", role.getId()); + sc.setParameters("sortOrder", rolePermission.getSortOrder()); + + final UpdateBuilder ub = getUpdateBuilder(rolePermission); + ub.set(rolePermission, sortOrderAttribute, sortOrder); + final int result = update(ub, sc, null); + if (result < 1) { + throw new CloudRuntimeException(failMessage); + } + sortOrder++; + } + return true; } }); } - public List findAllByRoleAndParentId(final Long parentId, final Long roleId) { - if (parentId == null || roleId == null) { - return Collections.emptyList(); - } - SearchCriteria sc = RolePermissionsSearch.create(); - sc.setParameters("roleId", roleId); - sc.setParameters("parentId", parentId); - return listBy(sc); - } - @Override public List findAllByRoleIdSorted(final Long roleId) { - SearchCriteria sc = RolePermissionsSearch.create(); + final SearchCriteria sc = RolePermissionsSearch.create(); if (roleId != null && roleId > 0L) { sc.setParameters("roleId", roleId); } - List rolePermissionList = listBy(sc, new Filter(RolePermissionVO.class, "sortOrder", true, null, null)); + final Filter searchBySorted = new Filter(RolePermissionVO.class, "sortOrder", true, null, null); + searchBySorted.addOrderBy(RolePermissionVO.class, "id", true); + final List rolePermissionList = listBy(sc, searchBySorted); if (rolePermissionList == null) { return Collections.emptyList(); } diff --git a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java index 4112bb6605a..7363b13b35a 100644 --- a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -193,9 +193,9 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override @ActionEvent(eventType = EventTypes.EVENT_ROLE_PERMISSION_UPDATE, eventDescription = "updating Role Permission order") - public boolean updateRolePermission(final RolePermission rolePermission, final RolePermission parentRolePermission) { + public boolean updateRolePermission(final Role role, final List newOrder) { checkCallerAccess(); - return rolePermission != null && rolePermissionsDao.update((RolePermissionVO) rolePermission, (RolePermissionVO) parentRolePermission); + return role != null && newOrder != null && rolePermissionsDao.update(role, newOrder); } @Override diff --git a/test/integration/smoke/test_dynamicroles.py b/test/integration/smoke/test_dynamicroles.py index 0a41a6dc486..99614374553 100644 --- a/test/integration/smoke/test_dynamicroles.py +++ b/test/integration/smoke/test_dynamicroles.py @@ -21,6 +21,7 @@ from marvin.cloudstackException import CloudstackAPIException from marvin.lib.base import Account, Role, RolePermission from marvin.lib.utils import cleanup_resources from nose.plugins.attrib import attr +from random import shuffle import copy import random @@ -378,21 +379,54 @@ class TestDynamicRoles(cloudstackTestCase): # Move last item to the top rule = permissions.pop(len(permissions)-1) permissions = [rule] + permissions - rule.update(self.apiclient, parent=0) + rule.update(self.apiclient, ruleorder=",".join(map(lambda x: x.id, permissions))) validate_permissions_list(permissions) # Move to the bottom rule = permissions.pop(0) permissions = permissions + [rule] - rule.update(self.apiclient, parent=permissions[-2].id) + rule.update(self.apiclient, ruleorder=",".join(map(lambda x: x.id, permissions))) validate_permissions_list(permissions) - # Move item in mid - rule = permissions.pop(1) - new_parent = permissions[1] - permissions.insert(2, rule) - rule.update(self.apiclient, parent=new_parent.id) - validate_permissions_list(permissions) + # Random shuffles + for _ in range(3): + shuffle(permissions) + rule.update(self.apiclient, ruleorder=",".join(map(lambda x: x.id, permissions))) + validate_permissions_list(permissions) + + + @attr(tags=['advanced', 'simulator', 'basic', 'sg'], required_hardware=False) + def test_rolepermission_lifecycle_concurrent_updates(self): + """ + Tests concurrent order updation of role permission + """ + permissions = [self.rolepermission] + rules = ['list*', '*Vol*', 'listCapabilities'] + for rule in rules: + data = copy.deepcopy(self.testdata["rolepermission"]) + data['rule'] = rule + permission = RolePermission.create( + self.apiclient, + data + ) + self.cleanup.append(permission) + permissions.append(permission) + + + # The following rule is considered to be created by another mgmt server + data = copy.deepcopy(self.testdata["rolepermission"]) + data['rule'] = "someRule*" + permission = RolePermission.create( + self.apiclient, + data + ) + self.cleanup.append(permission) + + shuffle(permissions) + try: + permission.update(self.apiclient, ruleorder=",".join(map(lambda x: x.id, permissions))) + self.fail("Reordering should fail in case of concurrent updates by other user") + except CloudstackAPIException: pass @attr(tags=['advanced', 'simulator', 'basic', 'sg'], required_hardware=False) diff --git a/tools/marvin/marvin/lib/base.py b/tools/marvin/marvin/lib/base.py index dc88457e7cc..63f07232a43 100755 --- a/tools/marvin/marvin/lib/base.py +++ b/tools/marvin/marvin/lib/base.py @@ -156,7 +156,7 @@ class RolePermission: """Update the role permission""" cmd = updateRolePermission.updateRolePermissionCmd() - cmd.id = self.id + cmd.roleid = self.roleid [setattr(cmd, k, v) for k, v in kwargs.items()] return apiclient.updateRolePermission(cmd) diff --git a/ui/dictionary.jsp b/ui/dictionary.jsp index a2636fcdc1c..37596917257 100644 --- a/ui/dictionary.jsp +++ b/ui/dictionary.jsp @@ -1074,6 +1074,7 @@ dictionary = { 'label.add.role': '', 'label.edit.role': '', 'label.delete.role': '', +'message.role.ordering.fail': '', 'label.root.disk.controller': '', 'label.root.disk.offering': '', }; diff --git a/ui/scripts/roles.js b/ui/scripts/roles.js index 373fbac9668..eae088fafa8 100644 --- a/ui/scripts/roles.js +++ b/ui/scripts/roles.js @@ -16,6 +16,7 @@ // under the License. (function($, cloudStack) { var apiList = []; + var rolePermissions = []; cloudStack.sections.roles = { title: 'label.roles', id: 'roles', @@ -174,19 +175,37 @@ moveDrag: { action: function(args) { var rule = args.context.multiRule[0]; - var newParentId = args.prevItem ? args.prevItem.id : 0; + var prevItemId = args.prevItem ? args.prevItem.id : 0; + + var ruleOrder = []; + $.each(rolePermissions, function(idx, item) { + var itemId = item.id; + if (idx == 0 && prevItemId == 0) { + ruleOrder.push(rule.id); + } + if (itemId == rule.id) { + return true; + } + ruleOrder.push(item.id); + if (prevItemId == itemId) { + ruleOrder.push(rule.id); + } + }); + $.ajax({ url: createURL('updateRolePermission'), data: { - id: rule.id, - parent: newParentId + roleid: rule.roleid, + ruleorder: ruleOrder.join() }, success: function(json) { args.response.success(); $(window).trigger('cloudStack.fullRefresh'); }, error: function(json) { - args.response.error(parseXMLHttpResponse(json)); + cloudStack.dialog.notice({ + message: 'message.role.ordering.fail' + }); } }); } @@ -277,6 +296,9 @@ dataType: 'json', success: function(json) { var rules = json.listrolepermissionsresponse.rolepermission; + if (rules) { + rolePermissions = rules; + } args.response.success({ data: rules });