From c4f76a199b9034432f604136825ac8846d9d6883 Mon Sep 17 00:00:00 2001 From: Nicolas Vazquez Date: Wed, 23 Aug 2017 13:18:58 -0300 Subject: [PATCH] FR26: Update rule permission of a role permission (#48) --- .../apache/cloudstack/acl/RoleService.java | 4 +- .../apache/cloudstack/api/ApiConstants.java | 1 + .../admin/acl/UpdateRolePermissionCmd.java | 56 ++++++++++++++++--- .../classes/resources/messages.properties | 1 + .../acl/dao/RolePermissionsDao.java | 10 ++++ .../acl/dao/RolePermissionsDaoImpl.java | 14 +++++ .../cloudstack/acl/RoleManagerImpl.java | 6 ++ test/integration/smoke/test_dynamicroles.py | 40 +++++++++++++ ui/dictionary.jsp | 1 + ui/scripts/roles.js | 23 ++++++++ ui/scripts/ui/widgets/multiEdit.js | 35 ++++++++++-- 11 files changed, 177 insertions(+), 14 deletions(-) diff --git a/api/src/org/apache/cloudstack/acl/RoleService.java b/api/src/org/apache/cloudstack/acl/RoleService.java index 59eef51e782..98d170b5853 100644 --- a/api/src/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/org/apache/cloudstack/acl/RoleService.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.acl; +import org.apache.cloudstack.acl.RolePermission.Permission; import org.apache.cloudstack.framework.config.ConfigKey; import java.util.List; @@ -36,13 +37,14 @@ public interface RoleService { RolePermission findRolePermission(final Long id); RolePermission findRolePermissionByUuid(final String uuid); - RolePermission createRolePermission(final Role role, final Rule rule, final RolePermission.Permission permission, final String description); + RolePermission createRolePermission(final Role role, final Rule rule, final Permission permission, final String description); /** * updateRolePermission updates the order/position of an role permission * @param role The role whose permissions needs to be re-ordered * @param newOrder The new list of ordered role permissions */ boolean updateRolePermission(final Role role, final List newOrder); + boolean updateRolePermission(final Role role, final RolePermission rolePermission, final Permission permission); 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 a366226d1c5..311dfd2c530 100755 --- a/api/src/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/org/apache/cloudstack/api/ApiConstants.java @@ -373,6 +373,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_ID = "ruleid"; public static final String RULE_ORDER = "ruleorder"; public static final String USER = "user"; public static final String ACTIVE_ONLY = "activeonly"; 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 055265c5ccc..90a23c947fb 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 @@ -20,6 +20,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.RolePermission.Permission; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiArgValidator; @@ -51,10 +52,16 @@ public class UpdateRolePermissionCmd extends BaseCmd { description = "ID of the role", validations = {ApiArgValidator.PositiveNumber}) private Long roleId; - @Parameter(name = ApiConstants.RULE_ORDER, type = CommandType.LIST, collectionType = CommandType.UUID, required = true, entityType = RolePermissionResponse.class, + @Parameter(name = ApiConstants.RULE_ORDER, type = CommandType.LIST, collectionType = CommandType.UUID, entityType = RolePermissionResponse.class, description = "The parent role permission uuid, use 0 to move this rule at the top of the list") private List rulePermissionOrder; + @Parameter(name = ApiConstants.RULE_ID, type = CommandType.UUID, entityType = RolePermissionResponse.class, description = "Role permission rule id") + private Long ruleId; + + @Parameter(name = ApiConstants.PERMISSION, type = CommandType.STRING, description = "Rule permission, can be: allow or deny") + private String rulePermission; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -67,6 +74,21 @@ public class UpdateRolePermissionCmd extends BaseCmd { return rulePermissionOrder; } + public Long getRuleId() { + return ruleId; + } + + public Permission getRulePermission() { + if (this.rulePermission == null) { + return null; + } + if (!this.rulePermission.equalsIgnoreCase(Permission.ALLOW.toString()) && + !this.rulePermission.equalsIgnoreCase(Permission.DENY.toString())) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Values for permission parameter should be: allow or deny"); + } + return rulePermission.equalsIgnoreCase(Permission.ALLOW.toString()) ? Permission.ALLOW : Permission.DENY; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// @@ -84,19 +106,35 @@ public class UpdateRolePermissionCmd extends BaseCmd { @Override public void execute() { final Role role = roleService.findRole(getRoleId()); + boolean result = false; if (role == null) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role id provided"); } - 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"); + if (getRulePermissionOrder() != null) { + if (getRuleId() != null || getRulePermission() != null) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Parameters permission and ruleid must be mutually exclusive with ruleorder"); } - rolePermissionsOrder.add(rolePermission); + 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); + } + result = roleService.updateRolePermission(role, rolePermissionsOrder); + } else if (getRuleId() != null && getRulePermission() != null) { + if (getRulePermissionOrder() != null) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Parameters permission and ruleid must be mutually exclusive with ruleorder"); + } + RolePermission rolePermission = roleService.findRolePermission(getRuleId()); + if (rolePermission == null) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid rule id provided"); + } + CallContext.current().setEventDetails("Updating permission for rule id: " + getRuleId() + " to: " + getRulePermission().toString()); + result = roleService.updateRolePermission(role, rolePermission, getRulePermission()); } - 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 35fba1e61ef..cc522382862 100644 --- a/client/WEB-INF/classes/resources/messages.properties +++ b/client/WEB-INF/classes/resources/messages.properties @@ -1106,6 +1106,7 @@ 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. +message.role.update.fail=Failed updating rule permission 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 37544919657..c9aeba1c599 100644 --- a/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDao.java +++ b/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDao.java @@ -20,6 +20,7 @@ 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.RolePermission.Permission; import org.apache.cloudstack.acl.RolePermissionVO; import java.util.List; @@ -40,6 +41,15 @@ public interface RolePermissionsDao extends GenericDao { */ boolean update(final Role role, final List newOrder); + /** + * Updates existing role permission + * @param role role of which rule belongs + * @param rolePermission role permission + * @param permission permission + * @return true on success, false if not + */ + boolean update(final Role role, final RolePermission rolePermission, final Permission permission); + /** * Returns ordered linked-list of role permission for a given role * @param roleId the ID of the 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 8f6fa83f012..32faf4e9a8c 100644 --- a/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDaoImpl.java +++ b/engine/schema/src/org/apache/cloudstack/acl/dao/RolePermissionsDaoImpl.java @@ -29,6 +29,7 @@ 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.RolePermission.Permission; import org.apache.cloudstack.acl.RolePermissionVO; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -149,6 +150,19 @@ public class RolePermissionsDaoImpl extends GenericDaoBase findAllByRoleIdSorted(final Long roleId) { final SearchCriteria sc = RolePermissionsSearch.create(); diff --git a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java index 27cb3d0238a..6cf8f9711b3 100644 --- a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -204,6 +204,12 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu return role != null && newOrder != null && rolePermissionsDao.update(role, newOrder); } + @Override + public boolean updateRolePermission(Role role, RolePermission rolePermission, RolePermission.Permission permission) { + checkCallerAccess(); + return role != null && rolePermissionsDao.update(role, rolePermission, permission); + } + @Override @ActionEvent(eventType = EventTypes.EVENT_ROLE_PERMISSION_DELETE, eventDescription = "deleting Role Permission") public boolean deleteRolePermission(final RolePermission rolePermission) { diff --git a/test/integration/smoke/test_dynamicroles.py b/test/integration/smoke/test_dynamicroles.py index 99614374553..5d105f6a427 100644 --- a/test/integration/smoke/test_dynamicroles.py +++ b/test/integration/smoke/test_dynamicroles.py @@ -394,6 +394,46 @@ class TestDynamicRoles(cloudstackTestCase): 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_update_permission(self): + """ + Tests update of Allow to Deny permission of a rule + """ + permissions = [self.rolepermission] + + rule = permissions.pop(0) + rule.update(self.apiclient, ruleid=rule.id, permission='deny') + + list_rolepermissions = RolePermission.list(self.apiclient, roleid=self.role.id) + self.assertEqual( + list_rolepermissions[0].permission, + 'deny', + msg="List of role permissions do not match created list of permissions" + ) + + rule.update(self.apiclient, ruleid=rule.id, permission='allow') + + list_rolepermissions = RolePermission.list(self.apiclient, roleid=self.role.id) + self.assertEqual( + list_rolepermissions[0].permission, + 'allow', + msg="List of role permissions do not match created list of permissions" + ) + + @attr(tags=['advanced', 'simulator', 'basic', 'sg'], required_hardware=False) + def test_rolepermission_lifecycle_update_permission_negative(self): + """ + Tests negative test for setting incorrect value as permission + """ + permissions = [self.rolepermission] + + rule = permissions.pop(0) + try: + rule.update(self.apiclient, ruleid=rule.id, permission='some_other_value') + except Exception: + pass + else: + self.fail("Negative test: Setting permission to 'some_other_value' should not be successful, failing") @attr(tags=['advanced', 'simulator', 'basic', 'sg'], required_hardware=False) def test_rolepermission_lifecycle_concurrent_updates(self): diff --git a/ui/dictionary.jsp b/ui/dictionary.jsp index 89242ba1cc8..4f1d0b2c84f 100644 --- a/ui/dictionary.jsp +++ b/ui/dictionary.jsp @@ -1103,6 +1103,7 @@ dictionary = { 'label.edit.role': '', 'label.delete.role': '', 'message.role.ordering.fail': '', +'message.role.update.fail': '', 'label.root.disk.controller': '', 'label.root.disk.offering': '', }; diff --git a/ui/scripts/roles.js b/ui/scripts/roles.js index eae088fafa8..01700dfc9a5 100644 --- a/ui/scripts/roles.js +++ b/ui/scripts/roles.js @@ -171,6 +171,28 @@ context: context, noSelect: true, noHeaderActionsColumn: true, + selectPermission: { + action: function(args){ + $.ajax({ + url: createURL("updateRolePermission"), + data: { + roleid: args.roleid, + ruleid: args.ruleid, + permission: args.permission + }, + dataType: "json", + async: true, + success: function(json) { + $(window).trigger('cloudStack.fullRefresh'); + }, + error: function(json) { + cloudStack.dialog.notice({ + message: 'message.role.update.fail' + }); + } + }); + } + }, reorder: { moveDrag: { action: function(args) { @@ -193,6 +215,7 @@ }); $.ajax({ + type: 'POST', url: createURL('updateRolePermission'), data: { roleid: rule.roleid, diff --git a/ui/scripts/ui/widgets/multiEdit.js b/ui/scripts/ui/widgets/multiEdit.js index e14f3ef1a14..ab02891b960 100755 --- a/ui/scripts/ui/widgets/multiEdit.js +++ b/ui/scripts/ui/widgets/multiEdit.js @@ -31,6 +31,7 @@ var $item = $('
').addClass('data-item'); var multiRule = data; var reorder = options.reorder; + var selectPermission = options.selectPermission; $item.append($('').append($(''))); $tr = $('').appendTo($item.find('tbody')); @@ -189,10 +190,34 @@ return $(this).val() == data[fieldName]; }); - var matchingValue = $matchingOption.size() ? - $matchingOption.html() : data[fieldName]; + if (selectPermission) { + // Wrap div to get its html code + selectedOptionHtml = $matchingOption.clone().wrap('
').parent().html(); + // Get html code from not matching option + $matchingSelect.find('option').each( + function() { + if ($(this).val() != data[fieldName]){ + selectedOptionHtml += $(this).clone().wrap('
').parent().html(); + } + } + ); + $select = $('
').appendTo( $('').appendTo($inputTable) @@ -1193,7 +1219,8 @@ preFilter: actionPreFilter, listView: listView, tags: tags, - reorder: reorder + reorder: reorder, + selectPermission: selectPermission } ).appendTo($dataBody); });