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 721bef08c20e..6130c62a7d45 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java @@ -17,38 +17,64 @@ package org.apache.cloudstack.acl; +import java.util.List; + import org.apache.cloudstack.acl.RolePermission.Permission; import org.apache.cloudstack.framework.config.ConfigKey; -import java.util.List; - public interface RoleService { ConfigKey EnableDynamicApiChecker = new ConfigKey<>("Advanced", Boolean.class, "dynamic.apichecker.enabled", "false", - "If set to true, this enables the dynamic role-based api access checker and disables the default static role-based api access checker.", - true); + "If set to true, this enables the dynamic role-based api access checker and disables the default static role-based api access checker.", true); boolean isEnabled(); - Role findRole(final Long id); - Role createRole(final String name, final RoleType roleType, final String description); - Role updateRole(final Role role, final String name, final RoleType roleType, final String description); - boolean deleteRole(final Role role); - RolePermission findRolePermission(final Long id); - RolePermission findRolePermissionByUuid(final String uuid); + /** + * Searches for a role with the given ID. If the ID is null or less than zero, this method will return null. + * This method will also return null if no role is found with the provided ID. + * 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); + + Role createRole(String name, RoleType roleType, String description); + + Role updateRole(Role role, String name, RoleType roleType, String description); + + boolean deleteRole(Role role); + + RolePermission findRolePermission(Long id); + + RolePermission findRolePermissionByUuid(String uuid); + + RolePermission createRolePermission(Role role, Rule rule, Permission permission, 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); + boolean updateRolePermission(Role role, List newOrder); + boolean updateRolePermission(Role role, RolePermission rolePermission, Permission permission); + + boolean deleteRolePermission(RolePermission rolePermission); + + /** + * List all roles configured in the database. Roles that have the type {@link RoleType#Admin} will not be shown for users that are not 'root admin'. + */ List listRoles(); - List findRolesByName(final String name); - List findRolesByType(final RoleType roleType); - List findAllPermissionsBy(final Long roleId); + + /** + * Find all roles that have the giving {@link String} as part of their name. + * If the user calling the method is not a 'root admin', roles of type {@link RoleType#Admin} wil lbe removed of the returned list. + */ + List findRolesByName(String name); + + /** + * Find all roles by {@link RoleType}. If the role type is {@link RoleType#Admin}, the calling account must be a root admin, otherwise we return an empty list. + */ + List findRolesByType(RoleType roleType); + + List findAllPermissionsBy(Long roleId); } 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 5cf870bfc063..9025e89a93cd 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 @@ -17,31 +17,25 @@ package org.apache.cloudstack.api.command.admin.acl; -import com.cloud.exception.ConcurrentOperationException; -import com.cloud.exception.InsufficientCapacityException; -import com.cloud.exception.NetworkRuleConflictException; -import com.cloud.exception.ResourceAllocationException; -import com.cloud.exception.ResourceUnavailableException; -import com.cloud.user.Account; -import com.google.common.base.Strings; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + import org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.Parameter; -import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.api.response.RoleResponse; +import org.apache.commons.lang3.StringUtils; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; +import com.cloud.user.Account; +import com.google.common.base.Strings; -@APICommand(name = ListRolesCmd.APINAME, description = "Lists dynamic roles in CloudStack", responseObject = RoleResponse.class, - requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, - since = "4.9.0", - authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin}) +@APICommand(name = ListRolesCmd.APINAME, description = "Lists dynamic roles in CloudStack", responseObject = RoleResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.9.0", authorized = { + RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin}) public class ListRolesCmd extends BaseCmd { public static final String APINAME = "listRoles"; @@ -112,13 +106,13 @@ private void setupResponse(final List roles) { } @Override - public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException { - final List roles; + public void execute() { + List roles; if (getId() != null && getId() > 0L) { roles = Collections.singletonList(roleService.findRole(getId())); - } else if (!Strings.isNullOrEmpty(getName())) { + } else if (StringUtils.isNotBlank(getName())) { roles = roleService.findRolesByName(getName()); - } else if (getRoleType() != null){ + } else if (getRoleType() != null) { roles = roleService.findRolesByType(getRoleType()); } else { roles = roleService.listRoles(); 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 b1e13313117c..ae471b2486ca 100644 --- a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.Iterator; import java.util.List; import javax.inject.Inject; @@ -37,11 +38,15 @@ import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; +import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; import com.cloud.event.ActionEvent; import com.cloud.event.EventTypes; import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; +import com.cloud.user.AccountManager; import com.cloud.user.dao.AccountDao; import com.cloud.utils.ListUtils; import com.cloud.utils.component.ManagerBase; @@ -52,18 +57,23 @@ import com.google.common.base.Strings; public class RoleManagerImpl extends ManagerBase implements RoleService, Configurable, PluggableService { + + private Logger logger = Logger.getLogger(getClass()); + @Inject private AccountDao accountDao; @Inject private RoleDao roleDao; @Inject private RolePermissionsDao rolePermissionsDao; + @Inject + private AccountManager accountManager; private void checkCallerAccess() { if (!isEnabled()) { throw new PermissionDeniedException("Dynamic api checker is not enabled, aborting role operation"); } - Account caller = CallContext.current().getCallingAccount(); + Account caller = getCurrentAccount(); if (caller == null || caller.getRoleId() == null) { throw new PermissionDeniedException("Restricted API called by an invalid user account"); } @@ -79,11 +89,30 @@ public boolean isEnabled() { } @Override - public Role findRole(final Long id) { + public Role findRole(Long id) { if (id == null || id < 1L) { + logger.trace(String.format("Role ID is invalid [%s]", id)); + return null; + } + RoleVO role = roleDao.findById(id); + if (role == null) { + logger.trace(String.format("Role not found [id=%s]", id)); return null; } - return roleDao.findById(id); + 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())); + return null; + } + return role; + } + + /** + * Simple call to {@link CallContext#current()} to retrieve the current calling account. + * This method facilitates unit testing, it avoids mocking static methods. + */ + protected Account getCurrentAccount() { + return CallContext.current().getCallingAccount(); } @Override @@ -125,7 +154,7 @@ public Role updateRole(final Role role, final String name, final RoleType roleTy if (roleType != null && roleType == RoleType.Unknown) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Unknown is not a valid role type"); } - RoleVO roleVO = (RoleVO) role; + RoleVO roleVO = (RoleVO)role; if (!Strings.isNullOrEmpty(name)) { roleVO.setName(name); } @@ -214,26 +243,56 @@ public boolean deleteRolePermission(final RolePermission rolePermission) { } @Override - public List findRolesByName(final String name) { + public List findRolesByName(String name) { List roles = null; - if (!Strings.isNullOrEmpty(name)) { + if (StringUtils.isNotBlank(name)) { roles = roleDao.findAllByName(name); } + removeRootAdminRolesIfNeeded(roles); return ListUtils.toListOfInterface(roles); } + /** + * Removes roles of the given list that have the type '{@link RoleType#Admin}' if the user calling the method is not a 'root admin'. + * The actual removal is executed via {@link #removeRootAdminRoles(List)}. Therefore, if the method is called by a 'root admin', we do nothing here. + */ + protected void removeRootAdminRolesIfNeeded(List roles) { + Account account = getCurrentAccount(); + if (!accountManager.isRootAdmin(account.getId())) { + removeRootAdminRoles(roles); + } + } + + /** + * Remove all roles that have the {@link RoleType#Admin}. + */ + protected void removeRootAdminRoles(List roles) { + if (CollectionUtils.isEmpty(roles)) { + return; + } + Iterator rolesIterator = roles.iterator(); + while (rolesIterator.hasNext()) { + Role role = rolesIterator.next(); + if (RoleType.Admin == role.getRoleType()) { + rolesIterator.remove(); + } + } + + } + @Override - public List findRolesByType(final RoleType roleType) { - List roles = null; - if (roleType != null) { - roles = roleDao.findAllByRoleType(roleType); + public List findRolesByType(RoleType roleType) { + if (roleType == null || RoleType.Admin == roleType && !accountManager.isRootAdmin(getCurrentAccount().getId())) { + return Collections.emptyList(); } + List roles = roleDao.findAllByRoleType(roleType); return ListUtils.toListOfInterface(roles); } @Override public List listRoles() { List roles = roleDao.listAll(); + removeRootAdminRolesIfNeeded(roles); return ListUtils.toListOfInterface(roles); } @@ -253,7 +312,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[]{RoleService.EnableDynamicApiChecker}; + return new ConfigKey[] {RoleService.EnableDynamicApiChecker}; } @Override @@ -269,4 +328,4 @@ public List> getCommands() { cmdList.add(DeleteRolePermissionCmd.class); return cmdList; } - } +} diff --git a/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java new file mode 100644 index 000000000000..bc50f3408987 --- /dev/null +++ b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java @@ -0,0 +1,288 @@ +// 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. + +package org.apache.cloudstack.acl; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.cloudstack.acl.dao.RoleDao; +import org.apache.commons.collections.CollectionUtils; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.runners.MockitoJUnitRunner; + +import com.cloud.user.Account; +import com.cloud.user.AccountManager; + +@RunWith(MockitoJUnitRunner.class) +public class RoleManagerImplTest { + + @Spy + @InjectMocks + private RoleManagerImpl roleManagerImpl; + @Mock + private AccountManager accountManagerMock; + @Mock + private RoleDao roleDaoMock; + + @Mock + private Account accountMock; + private long accountMockId = 100l; + + @Mock + private RoleVO roleVoMock; + private long roleMockId = 1l; + + @Before + public void beforeTest() { + Mockito.doReturn(accountMockId).when(accountMock).getId(); + Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); + + Mockito.doReturn(roleMockId).when(roleVoMock).getId(); + } + + @Test + public void findRoleTestIdNull() { + Role returnedRole = roleManagerImpl.findRole(null); + Assert.assertNull(returnedRole); + } + + @Test + public void findRoleTestIdZero() { + Role returnedRole = roleManagerImpl.findRole(0l); + Assert.assertNull(returnedRole); + } + + @Test + public void findRoleTestIdNegative() { + Role returnedRole = roleManagerImpl.findRole(-1l); + Assert.assertNull(returnedRole); + } + + @Test + public void findRoleTestRoleNotFound() { + Mockito.doReturn(null).when(roleDaoMock).findById(roleMockId); + Role returnedRole = roleManagerImpl.findRole(roleMockId); + Assert.assertNull(returnedRole); + } + + @Test + public void findRoleTestNotRootAdminAndNotRoleAdminType() { + Mockito.doReturn(RoleType.DomainAdmin).when(roleVoMock).getRoleType(); + Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); + Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId); + + Role returnedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertEquals(roleMockId, returnedRole.getId()); + Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); + Mockito.verify(roleVoMock, Mockito.times(1)).getRoleType(); + } + + @Test + public void findRoleTestRootAdminAndNotRoleAdminType() { + Mockito.doReturn(RoleType.DomainAdmin).when(roleVoMock).getRoleType(); + Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); + Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); + + Role returnedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertEquals(roleMockId, returnedRole.getId()); + Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); + Mockito.verify(roleVoMock, Mockito.times(0)).getRoleType(); + } + + @Test + public void findRoleTestRootAdminAndRoleAdminType() { + Mockito.doReturn(RoleType.Admin).when(roleVoMock).getRoleType(); + Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); + Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); + + Role returnedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertEquals(roleMockId, returnedRole.getId()); + Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); + Mockito.verify(roleVoMock, Mockito.times(0)).getRoleType(); + } + + @Test + public void findRoleTestNotRootAdminAndRoleAdminType() { + Mockito.doReturn(RoleType.Admin).when(roleVoMock).getRoleType(); + Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); + Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId); + + Role returnedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertNull(returnedRole); + Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); + Mockito.verify(roleVoMock, Mockito.times(1)).getRoleType(); + } + + @Test + public void findRolesByNameTestNullRoleName() { + List rolesFound = roleManagerImpl.findRolesByName(null); + + Assert.assertTrue(CollectionUtils.isEmpty(rolesFound)); + } + + @Test + public void findRolesByNameTestEmptyRoleName() { + List rolesFound = roleManagerImpl.findRolesByName(""); + + Assert.assertTrue(CollectionUtils.isEmpty(rolesFound)); + } + + @Test + public void findRolesByNameTestBlankRoleName() { + List rolesFound = roleManagerImpl.findRolesByName(" "); + + Assert.assertTrue(CollectionUtils.isEmpty(rolesFound)); + } + + @Test + public void findRolesByNameTest() { + String roleName = "roleName"; + ArrayList toBeReturned = new ArrayList<>(); + Mockito.doReturn(toBeReturned).when(roleDaoMock).findAllByName(roleName); + + roleManagerImpl.findRolesByName(roleName); + + Mockito.verify(roleManagerImpl).removeRootAdminRolesIfNeeded(toBeReturned); + } + + @Test + public void removeRootAdminRolesIfNeededTestRootAdmin() { + Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); + Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); + + List roles = new ArrayList<>(); + roleManagerImpl.removeRootAdminRolesIfNeeded(roles); + + Mockito.verify(roleManagerImpl, Mockito.times(0)).removeRootAdminRoles(roles); + } + + @Test + public void removeRootAdminRolesIfNeededTestNonRootAdminUser() { + Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); + Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId); + + List roles = new ArrayList<>(); + roleManagerImpl.removeRootAdminRolesIfNeeded(roles); + + Mockito.verify(roleManagerImpl, Mockito.times(1)).removeRootAdminRoles(roles); + } + + @Test + public void removeRootAdminRolesTest() { + List roles = new ArrayList<>(); + Role roleRootAdmin = Mockito.mock(Role.class); + Mockito.doReturn(RoleType.Admin).when(roleRootAdmin).getRoleType(); + + Role roleDomainAdmin = Mockito.mock(Role.class); + Mockito.doReturn(RoleType.DomainAdmin).when(roleDomainAdmin).getRoleType(); + + Role roleResourceAdmin = Mockito.mock(Role.class); + Mockito.doReturn(RoleType.ResourceAdmin).when(roleResourceAdmin).getRoleType(); + + Role roleUser = Mockito.mock(Role.class); + Mockito.doReturn(RoleType.User).when(roleUser).getRoleType(); + + roles.add(roleRootAdmin); + roles.add(roleDomainAdmin); + roles.add(roleResourceAdmin); + roles.add(roleUser); + + roleManagerImpl.removeRootAdminRoles(roles); + + Assert.assertEquals(3, roles.size()); + Assert.assertEquals(roleDomainAdmin, roles.get(0)); + Assert.assertEquals(roleResourceAdmin, roles.get(1)); + Assert.assertEquals(roleUser, roles.get(2)); + } + + @Test + public void findRolesByTypeTestNullRoleType() { + List returnedRoles = roleManagerImpl.findRolesByType(null); + + Assert.assertEquals(0, returnedRoles.size()); + Mockito.verify(accountManagerMock, Mockito.times(0)).isRootAdmin(Mockito.anyLong()); + } + + @Test + public void findRolesByTypeTestAdminRoleNonRootAdminUser() { + Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); + Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId); + + List returnedRoles = roleManagerImpl.findRolesByType(RoleType.Admin); + + 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)); + } + + @Test + public void findRolesByTypeTestAdminRoleRootAdminUser() { + Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); + Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); + + List roles = new ArrayList<>(); + roles.add(Mockito.mock(Role.class)); + Mockito.doReturn(roles).when(roleDaoMock).findAllByRoleType(RoleType.Admin); + List returnedRoles = roleManagerImpl.findRolesByType(RoleType.Admin); + + Assert.assertEquals(1, returnedRoles.size()); + Mockito.verify(accountManagerMock, Mockito.times(1)).isRootAdmin(Mockito.anyLong()); + Mockito.verify(roleDaoMock, Mockito.times(1)).findAllByRoleType(Mockito.any(RoleType.class)); + } + + @Test + public void findRolesByTypeTestNonAdminRoleRootAdminUser() { + Mockito.doReturn(accountMock).when(roleManagerImpl).getCurrentAccount(); + Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); + + List roles = new ArrayList<>(); + roles.add(Mockito.mock(Role.class)); + Mockito.doReturn(roles).when(roleDaoMock).findAllByRoleType(RoleType.User); + List returnedRoles = roleManagerImpl.findRolesByType(RoleType.User); + + Assert.assertEquals(1, returnedRoles.size()); + Mockito.verify(accountManagerMock, Mockito.times(0)).isRootAdmin(Mockito.anyLong()); + Mockito.verify(roleDaoMock, Mockito.times(1)).findAllByRoleType(Mockito.any(RoleType.class)); + } + + @Test + public void listRolesTest() { + List roles = new ArrayList<>(); + roles.add(Mockito.mock(Role.class)); + + Mockito.doReturn(roles).when(roleDaoMock).listAll(); + Mockito.doNothing().when(roleManagerImpl).removeRootAdminRolesIfNeeded(roles); + + List returnedRoles = roleManagerImpl.listRoles(); + + Assert.assertEquals(roles.size(), returnedRoles.size()); + Mockito.verify(roleDaoMock).listAll(); + Mockito.verify(roleManagerImpl).removeRootAdminRolesIfNeeded(roles); + } +}