From 8b5bfb145e335209e261056a691ffeb7372ca0e1 Mon Sep 17 00:00:00 2001 From: GaOrtiga <49285692+GaOrtiga@users.noreply.github.com> Date: Mon, 1 May 2023 10:26:10 -0300 Subject: [PATCH] create parameter to determine whether roles are public or private (#6960) Co-authored-by: Gabriel Ortiga Fernandes Co-authored-by: dahn --- .../java/org/apache/cloudstack/acl/Role.java | 1 + .../apache/cloudstack/acl/RoleService.java | 10 +- .../api/command/admin/acl/CreateRoleCmd.java | 11 +- .../api/command/admin/acl/ImportRoleCmd.java | 10 +- .../api/command/admin/acl/ListRolesCmd.java | 3 +- .../api/command/admin/acl/RoleCmd.java | 1 + .../api/command/admin/acl/UpdateRoleCmd.java | 9 +- .../api/response/BaseRoleResponse.java | 9 + .../api/command/test/CreateRoleCmdTest.java | 4 +- .../api/command/test/ImportRoleCmdTest.java | 2 +- .../api/command/test/UpdateRoleCmdTest.java | 2 +- .../org/apache/cloudstack/acl/RoleVO.java | 11 + .../apache/cloudstack/acl/dao/RoleDao.java | 14 +- .../cloudstack/acl/dao/RoleDaoImpl.java | 41 ++- .../META-INF/db/schema-41800to41810.sql | 2 + .../cloudstack/acl/RoleManagerImpl.java | 71 +++-- .../cloudstack/acl/RoleManagerImplTest.java | 12 +- test/integration/smoke/test_private_roles.py | 275 ++++++++++++++++++ tools/marvin/marvin/lib/base.py | 4 + 19 files changed, 435 insertions(+), 57 deletions(-) create mode 100644 test/integration/smoke/test_private_roles.py diff --git a/api/src/main/java/org/apache/cloudstack/acl/Role.java b/api/src/main/java/org/apache/cloudstack/acl/Role.java index c25d7a99867..5e5ffd583d8 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/Role.java +++ b/api/src/main/java/org/apache/cloudstack/acl/Role.java @@ -23,4 +23,5 @@ import org.apache.cloudstack.api.InternalIdentity; public interface Role extends RoleEntity, InternalIdentity, Identity { RoleType getRoleType(); boolean isDefault(); + boolean isPublicRole(); } diff --git a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java index 578c13ef6fd..9becce643d4 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java @@ -38,15 +38,17 @@ public interface RoleService { * Moreover, we will check if the requested role is of 'Admin' type; roles with 'Admin' type should only be visible to 'root admins'. * Therefore, if a non-'root admin' user tries to search for an 'Admin' role, this method will return null. */ + Role findRole(Long id, boolean removePrivateRoles); + Role findRole(Long id); - Role createRole(String name, RoleType roleType, String description); + Role createRole(String name, RoleType roleType, String description, boolean publicRole); - Role createRole(String name, Role role, String description); + Role createRole(String name, Role role, String description, boolean publicRole); - Role importRole(String name, RoleType roleType, String description, List> rules, boolean forced); + Role importRole(String name, RoleType roleType, String description, List> rules, boolean forced, boolean isPublicRole); - Role updateRole(Role role, String name, RoleType roleType, String description); + Role updateRole(Role role, String name, RoleType roleType, String description, Boolean publicRole); boolean deleteRole(Role role); diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java index 2b8800319f6..8a469df1f1f 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java @@ -48,6 +48,10 @@ public class CreateRoleCmd extends RoleCmd { description = "ID of the role to be cloned from. Either roleid or type must be passed in") private Long roleId; + @Parameter(name = ApiConstants.IS_PUBLIC, type = CommandType.BOOLEAN, description = "Indicates whether the role will be visible to all users (public) or only to root admins (private)." + + " If this parameter is not specified during the creation of the role its value will be defaulted to true (public).") + private boolean publicRole = true; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -60,6 +64,9 @@ public class CreateRoleCmd extends RoleCmd { return roleId; } + public boolean isPublicRole() { + return publicRole; + } ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// @@ -81,10 +88,10 @@ public class CreateRoleCmd extends RoleCmd { } CallContext.current().setEventDetails("Role: " + getRoleName() + ", from role: " + getRoleId() + ", description: " + getRoleDescription()); - role = roleService.createRole(getRoleName(), existingRole, getRoleDescription()); + role = roleService.createRole(getRoleName(), existingRole, getRoleDescription(), isPublicRole()); } else { CallContext.current().setEventDetails("Role: " + getRoleName() + ", type: " + getRoleType() + ", description: " + getRoleDescription()); - role = roleService.createRole(getRoleName(), getRoleType(), getRoleDescription()); + role = roleService.createRole(getRoleName(), getRoleType(), getRoleDescription(), isPublicRole()); } if (role == null) { diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ImportRoleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ImportRoleCmd.java index 4f3e9d15af7..058650cf42c 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ImportRoleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ImportRoleCmd.java @@ -64,6 +64,10 @@ public class ImportRoleCmd extends RoleCmd { description = "Force create a role with the same name. This overrides the role type, description and rule permissions for the existing role. Default is false.") private Boolean forced; + @Parameter(name = ApiConstants.IS_PUBLIC, type = CommandType.BOOLEAN, description = "Indicates whether the role will be visible to all users (public) or only to root admins (private)." + + " If this parameter is not specified during the creation of the role its value will be defaulted to true (public).") + private boolean publicRole = true; + @Inject ApiServerService _apiServer; @@ -114,6 +118,10 @@ public class ImportRoleCmd extends RoleCmd { return (forced != null) ? forced : false; } + public boolean isPublicRole() { + return publicRole; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// @@ -130,7 +138,7 @@ public class ImportRoleCmd extends RoleCmd { } CallContext.current().setEventDetails("Role: " + getRoleName() + ", type: " + getRoleType() + ", description: " + getRoleDescription()); - Role role = roleService.importRole(getRoleName(), getRoleType(), getRoleDescription(), getRules(), isForced()); + Role role = roleService.importRole(getRoleName(), getRoleType(), getRoleDescription(), getRules(), isForced(), isPublicRole()); if (role == null) { throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to import role"); } diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ListRolesCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ListRolesCmd.java index b55dc80e705..fef2b27eaa5 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ListRolesCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/ListRolesCmd.java @@ -92,6 +92,7 @@ public class ListRolesCmd extends BaseListCmd { roleResponse.setRoleType(role.getRoleType()); roleResponse.setDescription(role.getDescription()); roleResponse.setIsDefault(role.isDefault()); + roleResponse.setPublicRole(role.isPublicRole()); roleResponse.setObjectName("role"); roleResponses.add(roleResponse); } @@ -104,7 +105,7 @@ public class ListRolesCmd extends BaseListCmd { public void execute() { Pair, Integer> roles; if (getId() != null && getId() > 0L) { - roles = new Pair, Integer>(Collections.singletonList(roleService.findRole(getId())), 1); + roles = new Pair<>(Collections.singletonList(roleService.findRole(getId(), true)), 1); } else if (StringUtils.isNotBlank(getName()) || StringUtils.isNotBlank(getKeyword())) { roles = roleService.findRolesByName(getName(), getKeyword(), getStartIndex(), getPageSizeVal()); } else if (getRoleType() != null) { diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java index e652918c4ca..4c317d06b13 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java @@ -58,6 +58,7 @@ public abstract class RoleCmd extends BaseCmd { response.setRoleName(role.getName()); response.setRoleType(role.getRoleType()); response.setDescription(role.getDescription()); + response.setPublicRole(role.isPublicRole()); response.setResponseName(getCommandName()); response.setObjectName("role"); setResponseObject(response); diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java index 227aaf54f7f..7d002cd889b 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java @@ -52,6 +52,9 @@ public class UpdateRoleCmd extends RoleCmd { @Parameter(name = ApiConstants.DESCRIPTION, type = BaseCmd.CommandType.STRING, description = "The description of the role") private String roleDescription; + @Parameter(name = ApiConstants.IS_PUBLIC, type = CommandType.BOOLEAN, description = "Indicates whether the role will be visible to all users (public) or only to root admins (private).") + private Boolean publicRole; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -64,6 +67,10 @@ public class UpdateRoleCmd extends RoleCmd { return roleName; } + public Boolean isPublicRole() { + return publicRole; + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// @@ -80,7 +87,7 @@ public class UpdateRoleCmd extends RoleCmd { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role id provided"); } CallContext.current().setEventDetails("Role: " + getRoleName() + ", type:" + getRoleType() + ", description: " + getRoleDescription()); - role = roleService.updateRole(role, getRoleName(), getRoleType(), getRoleDescription()); + role = roleService.updateRole(role, getRoleName(), getRoleType(), getRoleDescription(), isPublicRole()); setupResponse(role); } diff --git a/api/src/main/java/org/apache/cloudstack/api/response/BaseRoleResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/BaseRoleResponse.java index 339d092c1f2..b1bba905bc6 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/BaseRoleResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/BaseRoleResponse.java @@ -36,6 +36,11 @@ public class BaseRoleResponse extends BaseResponse { @Param(description = "the description of the role") private String roleDescription; + @SerializedName(ApiConstants.IS_PUBLIC) + @Param(description = "Indicates whether the role will be visible to all users (public) or only to root admins (private)." + + " If this parameter is not specified during the creation of the role its value will be defaulted to true (public).") + private boolean publicRole = true; + public void setId(String id) { this.id = id; } @@ -47,4 +52,8 @@ public class BaseRoleResponse extends BaseResponse { public void setDescription(String description) { this.roleDescription = description; } + + public void setPublicRole(boolean publicRole) { + this.publicRole = publicRole; + } } diff --git a/api/src/test/java/org/apache/cloudstack/api/command/test/CreateRoleCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/test/CreateRoleCmdTest.java index a910de789a5..4b9d4fd8974 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/test/CreateRoleCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/test/CreateRoleCmdTest.java @@ -54,7 +54,7 @@ public class CreateRoleCmdTest { when(role.getDescription()).thenReturn("User test"); when(role.getName()).thenReturn("testuser"); when(role.getRoleType()).thenReturn(RoleType.User); - when(roleService.createRole(createRoleCmd.getRoleName(), createRoleCmd.getRoleType(), createRoleCmd.getRoleDescription())).thenReturn(role); + when(roleService.createRole(createRoleCmd.getRoleName(), createRoleCmd.getRoleType(), createRoleCmd.getRoleDescription(), true)).thenReturn(role); createRoleCmd.execute(); RoleResponse response = (RoleResponse) createRoleCmd.getResponseObject(); Assert.assertEquals((String) ReflectionTestUtils.getField(response, "roleName"), role.getName()); @@ -71,7 +71,7 @@ public class CreateRoleCmdTest { when(newRole.getDescription()).thenReturn("User test"); when(newRole.getName()).thenReturn("testuser"); when(newRole.getRoleType()).thenReturn(RoleType.User); - when(roleService.createRole(createRoleCmd.getRoleName(), role, createRoleCmd.getRoleDescription())).thenReturn(newRole); + when(roleService.createRole(createRoleCmd.getRoleName(), role, createRoleCmd.getRoleDescription(), true)).thenReturn(newRole); createRoleCmd.execute(); RoleResponse response = (RoleResponse) createRoleCmd.getResponseObject(); Assert.assertEquals((String) ReflectionTestUtils.getField(response, "roleName"), newRole.getName()); diff --git a/api/src/test/java/org/apache/cloudstack/api/command/test/ImportRoleCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/test/ImportRoleCmdTest.java index 8de01489dab..6299c1ed8e2 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/test/ImportRoleCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/test/ImportRoleCmdTest.java @@ -93,7 +93,7 @@ public class ImportRoleCmdTest { when(role.getDescription()).thenReturn("test user imported"); when(role.getName()).thenReturn("Test User"); when(role.getRoleType()).thenReturn(RoleType.User); - when(roleService.importRole(anyString(),any(), anyString(), any(), anyBoolean())).thenReturn(role); + when(roleService.importRole(anyString(), any(), anyString(), any(), anyBoolean(), anyBoolean())).thenReturn(role); importRoleCmd.execute(); RoleResponse response = (RoleResponse) importRoleCmd.getResponseObject(); diff --git a/api/src/test/java/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java index d298defb603..84b91525742 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java @@ -58,7 +58,7 @@ public class UpdateRoleCmdTest extends TestCase{ when(role.getDescription()).thenReturn("Default user"); when(role.getName()).thenReturn("User"); when(role.getRoleType()).thenReturn(RoleType.User); - when(roleService.updateRole(role,updateRoleCmd.getRoleName(),updateRoleCmd.getRoleType(),updateRoleCmd.getRoleDescription())).thenReturn(role); + when(roleService.updateRole(role, updateRoleCmd.getRoleName(), updateRoleCmd.getRoleType(), updateRoleCmd.getRoleDescription(), updateRoleCmd.isPublicRole())).thenReturn(role); when(role.getId()).thenReturn(1L); when(role.getDescription()).thenReturn("Description Initial"); when(role.getName()).thenReturn("User"); diff --git a/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java b/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java index f5a0cebc75f..d4647255fc6 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/acl/RoleVO.java @@ -55,6 +55,9 @@ public class RoleVO implements Role { @Column(name = "is_default") private boolean isDefault = false; + @Column(name = "public_role") + private boolean publicRole = true; + @Column(name = GenericDao.REMOVED_COLUMN) private Date removed; @@ -120,4 +123,12 @@ public class RoleVO implements Role { public String toString() { return ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "name", "uuid", "roleType"); } + + public boolean isPublicRole() { + return publicRole; + } + + public void setPublicRole(boolean publicRole) { + this.publicRole = publicRole; + } } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDao.java b/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDao.java index 36833d5e790..a776f7b6fe6 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDao.java @@ -26,13 +26,15 @@ import org.apache.cloudstack.acl.RoleVO; import java.util.List; public interface RoleDao extends GenericDao { - List findAllByName(String roleName); + List findAllByName(String roleName, boolean showPrivateRole); - Pair, Integer> findAllByName(final String roleName, String keyword, Long offset, Long limit); + Pair, Integer> findAllByName(final String roleName, String keyword, Long offset, Long limit, boolean showPrivateRole); - List findAllByRoleType(RoleType type); - List findByName(String roleName); - RoleVO findByNameAndType(String roleName, RoleType type); + List findAllByRoleType(RoleType type, boolean showPrivateRole); + List findByName(String roleName, boolean showPrivateRole); + RoleVO findByNameAndType(String roleName, RoleType type, boolean showPrivateRole); - Pair, Integer> findAllByRoleType(RoleType type, Long offset, Long limit); + Pair, Integer> findAllByRoleType(RoleType type, Long offset, Long limit, boolean showPrivateRole); + + Pair, Integer> listAllRoles(Long startIndex, Long limit, boolean showPrivateRole); } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDaoImpl.java index b4938a1e833..06d3108076a 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/acl/dao/RoleDaoImpl.java @@ -35,32 +35,41 @@ public class RoleDaoImpl extends GenericDaoBase implements RoleDao private final SearchBuilder RoleByNameSearch; private final SearchBuilder RoleByTypeSearch; private final SearchBuilder RoleByNameAndTypeSearch; + private final SearchBuilder RoleByIsPublicSearch; public RoleDaoImpl() { super(); RoleByNameSearch = createSearchBuilder(); RoleByNameSearch.and("roleName", RoleByNameSearch.entity().getName(), SearchCriteria.Op.LIKE); + RoleByNameSearch.and("isPublicRole", RoleByNameSearch.entity().isPublicRole(), SearchCriteria.Op.EQ); RoleByNameSearch.done(); RoleByTypeSearch = createSearchBuilder(); RoleByTypeSearch.and("roleType", RoleByTypeSearch.entity().getRoleType(), SearchCriteria.Op.EQ); + RoleByTypeSearch.and("isPublicRole", RoleByTypeSearch.entity().isPublicRole(), SearchCriteria.Op.EQ); RoleByTypeSearch.done(); RoleByNameAndTypeSearch = createSearchBuilder(); RoleByNameAndTypeSearch.and("roleName", RoleByNameAndTypeSearch.entity().getName(), SearchCriteria.Op.EQ); RoleByNameAndTypeSearch.and("roleType", RoleByNameAndTypeSearch.entity().getRoleType(), SearchCriteria.Op.EQ); + RoleByNameAndTypeSearch.and("isPublicRole", RoleByNameAndTypeSearch.entity().isPublicRole(), SearchCriteria.Op.EQ); RoleByNameAndTypeSearch.done(); + + RoleByIsPublicSearch = createSearchBuilder(); + RoleByIsPublicSearch.and("isPublicRole", RoleByIsPublicSearch.entity().isPublicRole(), SearchCriteria.Op.EQ); + RoleByIsPublicSearch.done(); } @Override - public List findAllByName(final String roleName) { - return findAllByName(roleName, null, null, null).first(); + public List findAllByName(final String roleName, boolean showPrivateRole) { + return findAllByName(roleName, null, null, null, showPrivateRole).first(); } @Override - public Pair, Integer> findAllByName(final String roleName, String keyword, Long offset, Long limit) { + public Pair, Integer> findAllByName(final String roleName, String keyword, Long offset, Long limit, boolean showPrivateRole) { SearchCriteria sc = RoleByNameSearch.create(); + filterPrivateRolesIfNeeded(sc, showPrivateRole); if (StringUtils.isNotEmpty(roleName)) { sc.setParameters("roleName", roleName); } @@ -72,28 +81,44 @@ public class RoleDaoImpl extends GenericDaoBase implements RoleDao } @Override - public List findAllByRoleType(final RoleType type) { - return findAllByRoleType(type, null, null).first(); + public List findAllByRoleType(final RoleType type, boolean showPrivateRole) { + return findAllByRoleType(type, null, null, showPrivateRole).first(); } - public Pair, Integer> findAllByRoleType(final RoleType type, Long offset, Long limit) { + public Pair, Integer> findAllByRoleType(final RoleType type, Long offset, Long limit, boolean showPrivateRole) { SearchCriteria sc = RoleByTypeSearch.create(); + filterPrivateRolesIfNeeded(sc, showPrivateRole); sc.setParameters("roleType", type); return searchAndCount(sc, new Filter(RoleVO.class, "id", true, offset, limit)); } @Override - public List findByName(String roleName) { + public List findByName(String roleName, boolean showPrivateRole) { SearchCriteria sc = RoleByNameSearch.create(); + filterPrivateRolesIfNeeded(sc, showPrivateRole); sc.setParameters("roleName", roleName); return listBy(sc); } @Override - public RoleVO findByNameAndType(String roleName, RoleType type) { + public RoleVO findByNameAndType(String roleName, RoleType type, boolean showPrivateRole) { SearchCriteria sc = RoleByNameAndTypeSearch.create(); + filterPrivateRolesIfNeeded(sc, showPrivateRole); sc.setParameters("roleName", roleName); sc.setParameters("roleType", type); return findOneBy(sc); } + + @Override + public Pair, Integer> listAllRoles(Long startIndex, Long limit, boolean showPrivateRole) { + SearchCriteria sc = RoleByIsPublicSearch.create(); + filterPrivateRolesIfNeeded(sc, showPrivateRole); + return searchAndCount(sc, new Filter(RoleVO.class, "id", true, startIndex, limit)); + } + + public void filterPrivateRolesIfNeeded(SearchCriteria sc, boolean showPrivateRole) { + if (!showPrivateRole) { + sc.setParameters("isPublicRole", true); + } + } } diff --git a/engine/schema/src/main/resources/META-INF/db/schema-41800to41810.sql b/engine/schema/src/main/resources/META-INF/db/schema-41800to41810.sql index ae4be2d3122..e595f007a6d 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-41800to41810.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-41800to41810.sql @@ -19,3 +19,5 @@ -- Schema upgrade from 4.18.0.0 to 4.18.1.0 --; +-- create_public_parameter_on_roles. #6960 +ALTER TABLE `cloud`.`roles` ADD COLUMN `public_role` tinyint(1) NOT NULL DEFAULT '1' COMMENT 'Indicates whether the role will be visible to all users (public) or only to root admins (private). If this parameter is not specified during the creation of the role its value will be defaulted to true (public).'; diff --git a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java index f2f4a6fe06a..ff97d7ecf6f 100644 --- a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -56,7 +56,6 @@ import com.cloud.utils.ListUtils; import com.cloud.utils.Pair; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.component.PluggableService; -import com.cloud.utils.db.Filter; import com.cloud.utils.db.Transaction; import com.cloud.utils.db.TransactionCallback; import com.cloud.utils.db.TransactionStatus; @@ -95,7 +94,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu } @Override - public Role findRole(Long id) { + public Role findRole(Long id, boolean removePrivateRoles) { if (id == null || id < 1L) { logger.trace(String.format("Role ID is invalid [%s]", id)); return null; @@ -105,14 +104,18 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu logger.trace(String.format("Role not found [id=%s]", id)); return null; } - Account account = getCurrentAccount(); - if (!accountManager.isRootAdmin(account.getId()) && RoleType.Admin == role.getRoleType()) { - logger.debug(String.format("Role [id=%s, name=%s] is of 'Admin' type and is only visible to 'Root admins'.", id, role.getName())); + if (!isCallerRootAdmin() && (RoleType.Admin == role.getRoleType() || (!role.isPublicRole() && removePrivateRoles))) { + logger.debug(String.format("Role [id=%s, name=%s] is either of 'Admin' type or is private and is only visible to 'Root admins'.", id, role.getName())); return null; } return role; } + @Override + public Role findRole(Long id) { + return findRole(id, false); + } + /** * Simple call to {@link CallContext#current()} to retrieve the current calling account. * This method facilitates unit testing, it avoids mocking static methods. @@ -140,7 +143,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override @ActionEvent(eventType = EventTypes.EVENT_ROLE_CREATE, eventDescription = "creating Role") - public Role createRole(final String name, final RoleType roleType, final String description) { + public Role createRole(final String name, final RoleType roleType, final String description, boolean publicRole) { checkCallerAccess(); if (roleType == null || roleType == RoleType.Unknown) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role type provided"); @@ -148,7 +151,9 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu return Transaction.execute(new TransactionCallback() { @Override public RoleVO doInTransaction(TransactionStatus status) { - RoleVO role = roleDao.persist(new RoleVO(name, roleType, description)); + RoleVO role = new RoleVO(name, roleType, description); + role.setPublicRole(publicRole); + role = roleDao.persist(role); CallContext.current().putContextParameter(Role.class, role.getUuid()); return role; } @@ -157,12 +162,14 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override @ActionEvent(eventType = EventTypes.EVENT_ROLE_CREATE, eventDescription = "creating role by cloning another role") - public Role createRole(String name, Role role, String description) { + public Role createRole(String name, Role role, String description, boolean publicRole) { checkCallerAccess(); return Transaction.execute(new TransactionCallback() { @Override public RoleVO doInTransaction(TransactionStatus status) { - RoleVO newRoleVO = roleDao.persist(new RoleVO(name, role.getRoleType(), description)); + RoleVO newRole = new RoleVO(name, role.getRoleType(), description); + newRole.setPublicRole(publicRole); + RoleVO newRoleVO = roleDao.persist(newRole); if (newRoleVO == null) { throw new CloudRuntimeException("Unable to create the role: " + name + ", failed to persist in DB"); } @@ -181,7 +188,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override @ActionEvent(eventType = EventTypes.EVENT_ROLE_IMPORT, eventDescription = "importing Role") - public Role importRole(String name, RoleType type, String description, List> rules, boolean forced) { + public Role importRole(String name, RoleType type, String description, List> rules, boolean forced, boolean isPublicRole) { checkCallerAccess(); if (StringUtils.isEmpty(name)) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role name provided"); @@ -190,7 +197,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role type provided"); } - List existingRoles = roleDao.findByName(name); + List existingRoles = roleDao.findByName(name, isCallerRootAdmin()); if (CollectionUtils.isNotEmpty(existingRoles) && !forced) { throw new CloudRuntimeException("Role already exists"); } @@ -199,7 +206,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override public RoleVO doInTransaction(TransactionStatus status) { RoleVO newRole = null; - RoleVO existingRole = roleDao.findByNameAndType(name, type); + RoleVO existingRole = roleDao.findByNameAndType(name, type, isCallerRootAdmin()); if (existingRole != null) { if (existingRole.isDefault()) { throw new CloudRuntimeException("Failed to import the role: " + name + ", default role cannot be overriden"); @@ -216,11 +223,14 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu existingRole.setName(name); existingRole.setRoleType(type); existingRole.setDescription(description); + existingRole.setPublicRole(isPublicRole); roleDao.update(existingRole.getId(), existingRole); newRole = existingRole; } else { - newRole = roleDao.persist(new RoleVO(name, type, description)); + RoleVO role = new RoleVO(name, type, description); + role.setPublicRole(isPublicRole); + newRole = roleDao.persist(role); } if (newRole == null) { @@ -243,16 +253,23 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override @ActionEvent(eventType = EventTypes.EVENT_ROLE_UPDATE, eventDescription = "updating Role") - public Role updateRole(final Role role, final String name, final RoleType roleType, final String description) { + public Role updateRole(final Role role, final String name, final RoleType roleType, final String description, Boolean publicRole) { checkCallerAccess(); - if (role.isDefault()) { - throw new PermissionDeniedException("Default roles cannot be updated"); - } if (roleType != null && roleType == RoleType.Unknown) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Unknown is not a valid role type"); } RoleVO roleVO = (RoleVO)role; + + if (role.isDefault()) { + if (publicRole == null || roleType != null || !StringUtils.isAllEmpty(name, description)) { + throw new PermissionDeniedException("Default roles cannot be updated (with the exception of making it private/public)."); + } + roleVO.setPublicRole(publicRole); + roleDao.update(role.getId(), roleVO); + return role; + } + if (StringUtils.isNotEmpty(name)) { roleVO.setName(name); } @@ -268,6 +285,10 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu roleVO.setDescription(description); } + if (publicRole == null) { + publicRole = role.isPublicRole(); + } + roleVO.setPublicRole(publicRole); roleDao.update(role.getId(), roleVO); return role; } @@ -363,7 +384,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override public Pair, Integer> findRolesByName(String name, String keyword, Long startIndex, Long limit) { if (StringUtils.isNotBlank(name) || StringUtils.isNotBlank(keyword)) { - Pair, Integer> data = roleDao.findAllByName(name, keyword, startIndex, limit); + Pair, Integer> data = roleDao.findAllByName(name, keyword, startIndex, limit, isCallerRootAdmin()); int removed = removeRootAdminRolesIfNeeded(data.first()); return new Pair,Integer>(ListUtils.toListOfInterface(data.first()), Integer.valueOf(data.second() - removed)); } @@ -375,8 +396,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu * The actual removal is executed via {@link #removeRootAdminRoles(List)}. Therefore, if the method is called by a 'root admin', we do nothing here. */ protected int removeRootAdminRolesIfNeeded(List roles) { - Account account = getCurrentAccount(); - if (!accountManager.isRootAdmin(account.getId())) { + if (!isCallerRootAdmin()) { return removeRootAdminRoles(roles); } return 0; @@ -408,10 +428,10 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override public Pair, Integer> findRolesByType(RoleType roleType, Long startIndex, Long limit) { - if (roleType == null || RoleType.Admin == roleType && !accountManager.isRootAdmin(getCurrentAccount().getId())) { + if (roleType == null || RoleType.Admin == roleType && !isCallerRootAdmin()) { return new Pair, Integer>(Collections.emptyList(), 0); } - Pair, Integer> data = roleDao.findAllByRoleType(roleType, startIndex, limit); + Pair, Integer> data = roleDao.findAllByRoleType(roleType, startIndex, limit, isCallerRootAdmin()); return new Pair,Integer>(ListUtils.toListOfInterface(data.first()), Integer.valueOf(data.second())); } @@ -424,8 +444,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override public Pair, Integer> listRoles(Long startIndex, Long limit) { - Pair, Integer> data = roleDao.searchAndCount(null, - new Filter(RoleVO.class, "id", Boolean.TRUE, startIndex, limit)); + Pair, Integer> data = roleDao.listAllRoles(startIndex, limit, isCallerRootAdmin()); int removed = removeRootAdminRolesIfNeeded(data.first()); return new Pair,Integer>(ListUtils.toListOfInterface(data.first()), Integer.valueOf(data.second() - removed)); } @@ -439,6 +458,10 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu return Collections.emptyList(); } + private boolean isCallerRootAdmin() { + return accountManager.isRootAdmin(getCurrentAccount().getId()); + } + @Override public Permission getRolePermission(String permission) { if (StringUtils.isEmpty(permission)) { diff --git a/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java index 049bc17e0b6..9f0d40a6c6b 100644 --- a/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java @@ -166,7 +166,7 @@ public class RoleManagerImplTest { String roleName = "roleName"; List roles = new ArrayList<>(); Pair, Integer> toBeReturned = new Pair(roles, 0); - Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByName(roleName, null, null, null); + Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByName(roleName, null, null, null, false); roleManagerImpl.findRolesByName(roleName); Mockito.verify(roleManagerImpl).removeRootAdminRolesIfNeeded(roles); @@ -239,7 +239,7 @@ public class RoleManagerImplTest { Assert.assertEquals(0, returnedRoles.size()); Mockito.verify(accountManagerMock, Mockito.times(1)).isRootAdmin(Mockito.anyLong()); - Mockito.verify(roleDaoMock, Mockito.times(0)).findAllByRoleType(Mockito.any(RoleType.class)); + Mockito.verify(roleDaoMock, Mockito.times(0)).findAllByRoleType(Mockito.any(RoleType.class), Mockito.anyBoolean()); } @Test @@ -250,11 +250,11 @@ public class RoleManagerImplTest { List roles = new ArrayList<>(); roles.add(Mockito.mock(Role.class)); Pair, Integer> toBeReturned = new Pair(roles, 1); - Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByRoleType(RoleType.Admin, null, null); + Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByRoleType(RoleType.Admin, null, null, true); List returnedRoles = roleManagerImpl.findRolesByType(RoleType.Admin); Assert.assertEquals(1, returnedRoles.size()); - Mockito.verify(accountManagerMock, Mockito.times(1)).isRootAdmin(Mockito.anyLong()); + Mockito.verify(accountManagerMock, Mockito.times(2)).isRootAdmin(Mockito.anyLong()); } @Test @@ -265,11 +265,11 @@ public class RoleManagerImplTest { List roles = new ArrayList<>(); roles.add(Mockito.mock(Role.class)); Pair, Integer> toBeReturned = new Pair(roles, 1); - Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByRoleType(RoleType.User, null, null); + Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByRoleType(RoleType.User, null, null, true); List returnedRoles = roleManagerImpl.findRolesByType(RoleType.User); Assert.assertEquals(1, returnedRoles.size()); - Mockito.verify(accountManagerMock, Mockito.times(0)).isRootAdmin(Mockito.anyLong()); + Mockito.verify(accountManagerMock, Mockito.times(1)).isRootAdmin(Mockito.anyLong()); } @Test diff --git a/test/integration/smoke/test_private_roles.py b/test/integration/smoke/test_private_roles.py new file mode 100644 index 00000000000..32d48833348 --- /dev/null +++ b/test/integration/smoke/test_private_roles.py @@ -0,0 +1,275 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from marvin.cloudstackAPI import * +from marvin.cloudstackTestCase import cloudstackTestCase +from marvin.lib.base import Account, Role, User +from marvin.lib.utils import cleanup_resources +from nose.plugins.attrib import attr + +import random + + +class TestData(object): + """Test data object that is required to create resources + """ + def __init__(self): + self.testdata = { + "accountadmin": { + "email": "mtu@test.cloud", + "firstname": "Marvin", + "lastname": "TestAdminAccount", + "username": "TestAdminAccount", + "password": "password" + }, + "accountdomainadmin": { + "email": "mtu@test.cloud", + "firstname": "Marvin", + "lastname": "TestDomainAdminAccount", + "username": "TestDomainAdminAccount", + "password": "password" + }, + "accountroleuser": { + "email": "mtu@test.cloud", + "firstname": "Marvin", + "lastname": "TestUserAccount", + "username": "TestUserAccount", + "password": "password" + }, + "roleadmin": { + "name": "MarvinFake Admin Role ", + "type": "Admin", + "description": "Fake Admin Role created by Marvin test" + }, + "roleuser": { + "name": "MarvinFake User Role ", + "type": "User", + "description": "Fake User Role created by Marvin test", + "ispublic": False + }, + "publicrole": { + "name": "MarvinFake Public Role ", + "type": "User", + "description": "Fake Public Role created by Marvin test" + }, + "importrole": { + "name": "MarvinFake Import Role ", + "type": "User", + "description": "Fake Import User Role created by Marvin test", + "ispublic": True, + "rules": [{"rule":"list*", "permission":"allow","description":"Listing apis"}, + {"rule":"get*", "permission":"allow","description":"Get apis"}, + {"rule":"update*", "permission":"deny","description":"Update apis"}] + }, + "roledomainadmin": { + "name": "MarvinFake DomainAdmin Role ", + "type": "DomainAdmin", + "description": "Fake Domain-Admin Role created by Marvin test", + "ispublic": False + }, + "apiConfig": { + "listApis": "allow", + "listAccounts": "allow", + "listClusters": "deny", + "*VM*": "allow", + "*Host*": "deny" + } + } + + +class TestPrivateRoles(cloudstackTestCase): + """Tests Visibility of private and public roles + """ + + def setUp(self): + self.apiclient = self.testClient.getApiClient() + self.testdata = TestData().testdata + + self.testdata["roleadmin"]["name"] += self.getRandomString() + self.testdata["roledomainadmin"]["name"] += self.getRandomString() + self.testdata["roleuser"]["name"] += self.getRandomString() + self.cleanup = [] + + self.role_admin = Role.create( + self.apiclient, + self.testdata["roleadmin"] + ) + self.cleanup.append(self.role_admin) + + self.role_domain_admin = Role.create( + self.apiclient, + self.testdata["roledomainadmin"] + ) + self.cleanup.append(self.role_domain_admin) + + self.private_role = Role.create( + self.apiclient, + self.testdata["roleuser"] + ) + self.cleanup.append(self.private_role) + + self.account_admin = Account.create( + self.apiclient, + self.testdata["accountadmin"], + roleid=self.role_admin.id + ) + self.cleanup.append(self.account_admin) + + self.account_domain_admin = Account.create( + self.apiclient, + self.testdata["accountdomainadmin"], + roleid=self.role_domain_admin.id + ) + self.cleanup.append(self.account_domain_admin) + + self.admin_apiclient = self.testClient.getUserApiClient( + UserName=self.account_admin.name, + DomainName='ROOT', + type=1 + ) + + self.domain_admin_apiclient = self.testClient.getUserApiClient( + UserName=self.account_domain_admin.name, + DomainName='ROOT', + type=2 + ) + + def tearDown(self): + super(TestPrivateRoles, self).tearDown() + + def getRandomString(self): + return "".join(random.choice("abcdefghijklmnopqrstuvwxyz0123456789") for _ in range(10)) + + def asserts_visibility_of_private_role(self, role_id): + list_roles_domain_admin = Role.list(self.domain_admin_apiclient, id=role_id) + self.assertEqual( + list_roles_domain_admin, + None, + "Domain Admins should not be able to list private roles" + ) + + list_roles_admin = Role.list(self.admin_apiclient, id=role_id) + self.assertNotEqual( + list_roles_admin, + None, + "Admins should be able to list private roles" + ) + + def asserts_visibility_of_public_role(self, role_id): + list_roles_domain_admin = Role.list(self.domain_admin_apiclient, id=role_id) + self.assertNotEqual( + list_roles_domain_admin, + None, + "Domain Admins should be able to list public roles" + ) + + list_roles_admin = Role.list(self.admin_apiclient, id=role_id) + self.assertNotEqual( + list_roles_admin, + None, + "Admins should be able to list public roles" + ) + + @attr(tags=['simulator', 'basic'], required_hardware=False) + def test_create_role(self): + """ + 1. Create a private role + 2. Create a public role + 3. Verify whether their visibility is as expected + """ + self.testdata["roleuser"]["name"] += self.getRandomString() + self.testdata["publicrole"]["name"] += self.getRandomString() + private_role = Role.create( + self.apiclient, + self.testdata["roleuser"] + ) + self.cleanup.append(self.private_role) + public_role = Role.create( + self.apiclient, + self.testdata["publicrole"] + ) + self.cleanup.append(self.public_role) + self.asserts_visibility_of_private_role(private_role.id) + self.asserts_visibility_of_public_role(public_role.id) + + @attr(tags=['simulator', 'basic'], required_hardware=False) + def test_update_role(self): + """ + 1. Create a public role + 2. Check if its visibility is public + 3. Update it to make it private + 4. Verify if its visibility is private + """ + self.testdata["publicrole"]["name"] += self.getRandomString() + role = Role.create( + self.apiclient, + self.testdata["publicrole"] + ) + self.cleanup.append(role) + self.asserts_visibility_of_public_role(role.id) + role.update(self.apiclient, id=role.id, ispublic=False) + self.asserts_visibility_of_private_role(role.id) + + @attr(tags=['simulator', 'basic'], required_hardware=False) + def test_import_role(self): + """ + 1. Import a public role + 2. Import a private role + 3. Verify their visibility + """ + self.testdata["importrole"]["name"] += self.getRandomString() + imported_public_role = Role.importRole( + self.apiclient, + self.testdata["importrole"] + ) + self.cleanup.append(imported_public_role) + self.testdata["importrole"]["name"] += self.getRandomString() + self.testdata["importrole"]["ispublic"] = False + imported_private_role = Role.importRole( + self.apiclient, + self.testdata["importrole"] + ) + self.cleanup.append(imported_private_role) + + self.asserts_visibility_of_public_role(imported_public_role.id) + self.asserts_visibility_of_private_role(imported_private_role.id) + + @attr(tags=['simulator', 'basic'], required_hardware=False) + def test_login_private_role(self): + """ + 1. Crate a User account with a private role + 2. Login with the created account + 3. Verify that the login was successful + """ + account_private_role = Account.create( + self.apiclient, + self.testdata["accountroleuser"], + roleid=self.private_role.id + ) + self.cleanup.append(account_private_role) + + response = User.login( + self.apiclient, + username=account_private_role.name, + password=self.testdata["accountroleuser"]["password"] + ) + + self.assertNotEqual( + response.sessionkey, + None, + "Accounts using private roles should be able to login." + ) diff --git a/tools/marvin/marvin/lib/base.py b/tools/marvin/marvin/lib/base.py index 9d28d5a3f2a..46015f7d7b7 100755 --- a/tools/marvin/marvin/lib/base.py +++ b/tools/marvin/marvin/lib/base.py @@ -108,6 +108,8 @@ class Role: cmd.roleid = services["roleid"] if "description" in services: cmd.description = services["description"] + if "ispublic" in services: + cmd.ispublic = services["ispublic"] return Role(apiclient.createRole(cmd).__dict__) @@ -122,6 +124,8 @@ class Role: cmd.description = services["description"] if "forced" in services: cmd.type = services["forced"] + if "ispublic" in services: + cmd.ispublic = services["ispublic"] return Role(apiclient.importRole(cmd).__dict__)