From 3a34e2d0c80ae37b9e4a508f2bb4fd0881474697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Weing=C3=A4rtner?= Date: Wed, 25 Apr 2018 13:25:19 -0300 Subject: [PATCH 1/5] create account with domain admin showing 'root admin' role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Domain admins should not be able to assign the role of root admin to new users. Therefore, the role ‘root admin’ (or any other of the same type) should not be visible to domain admins. --- .../apache/cloudstack/acl/RoleService.java | 35 ++- .../api/command/admin/acl/ListRolesCmd.java | 7 +- .../cloudstack/acl/RoleManagerImpl.java | 85 +++++- .../cloudstack/acl/RoleManagerImplTest.java | 275 ++++++++++++++++++ 4 files changed, 380 insertions(+), 22 deletions(-) create mode 100644 server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java 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..bb0b86146af9 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java @@ -25,30 +25,51 @@ 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); + + /** + * 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(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); 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 deleteRolePermission(RolePermission rolePermission); 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..d6579e6d3e36 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 @@ -33,6 +33,7 @@ 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; @@ -112,11 +113,11 @@ 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){ roles = roleService.findRolesByType(getRoleType()); 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..1523cbd369e4 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,16 @@ 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.AccountManagerImpl; import com.cloud.user.dao.AccountDao; import com.cloud.utils.ListUtils; import com.cloud.utils.component.ManagerBase; @@ -52,18 +58,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 +90,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 because this avoids static methods mocks. + */ + protected Account getCurrentAccount() { + return CallContext.current().getCallingAccount(); } @Override @@ -125,7 +155,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 +244,57 @@ 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 +314,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[]{RoleService.EnableDynamicApiChecker}; + return new ConfigKey[] {RoleService.EnableDynamicApiChecker}; } @Override @@ -269,4 +330,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..d9f5f2cf3cd9 --- /dev/null +++ b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java @@ -0,0 +1,275 @@ +package org.apache.cloudstack.acl; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +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 returedRole = roleManagerImpl.findRole(null); + Assert.assertNull(returedRole); + } + + @Test + public void findRoleTestIdZero() { + Role returedRole = roleManagerImpl.findRole(0l); + Assert.assertNull(returedRole); + } + + @Test + public void findRoleTestIdNegative() { + Role returedRole = roleManagerImpl.findRole(-1l); + Assert.assertNull(returedRole); + } + + @Test + public void findRoleTestRoleNotFound() { + Mockito.doReturn(null).when(roleDaoMock).findById(roleMockId); + Role returedRole = roleManagerImpl.findRole(roleMockId); + Assert.assertNull(returedRole); + } + + @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 returedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertEquals(roleMockId, returedRole.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 returedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertEquals(roleMockId, returedRole.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 returedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertEquals(roleMockId, returedRole.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 returedRole = roleManagerImpl.findRole(roleMockId); + + Assert.assertNull(returedRole); + 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); + } +} From 64693d3bcb3c4489302c90ac208cdc729f31d7e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Weing=C3=A4rtner?= Date: Wed, 25 Apr 2018 13:28:28 -0300 Subject: [PATCH 2/5] License and formatting --- .../apache/cloudstack/acl/RoleService.java | 27 ++-- .../api/command/admin/acl/ListRolesCmd.java | 27 ++-- .../cloudstack/acl/RoleManagerImpl.java | 10 +- .../cloudstack/acl/RoleManagerImplTest.java | 135 ++++++++++-------- 4 files changed, 103 insertions(+), 96 deletions(-) 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 bb0b86146af9..bcb021d42c4d 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java @@ -17,11 +17,11 @@ 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", @@ -35,39 +35,42 @@ public interface RoleService { */ Role findRole(Long id); - Role createRole(final String name, final RoleType roleType, final String description); + Role createRole(String name, RoleType roleType, String description); - Role updateRole(final Role role, final String name, final RoleType roleType, final String description); + Role updateRole(Role role, String name, RoleType roleType, String description); - boolean deleteRole(final Role role); + boolean deleteRole(Role role); - RolePermission findRolePermission(final Long id); + RolePermission findRolePermission(Long id); - RolePermission findRolePermissionByUuid(final String uuid); + RolePermission findRolePermissionByUuid(String uuid); - RolePermission createRolePermission(final Role role, final Rule rule, final Permission permission, final String description); + RolePermission createRolePermission(Role role, Rule rule, Permission permission, 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(Role role, List newOrder); - boolean updateRolePermission(final Role role, final RolePermission rolePermission, final Permission permission); + 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(); /** * 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. + * 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. + * 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); 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 d6579e6d3e36..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,32 +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"; @@ -113,13 +106,13 @@ private void setupResponse(final List roles) { } @Override - public void execute(){ + public void execute() { List roles; if (getId() != null && getId() > 0L) { roles = Collections.singletonList(roleService.findRole(getId())); } 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 1523cbd369e4..c511f16b9184 100644 --- a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -47,7 +47,6 @@ import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; import com.cloud.user.AccountManager; -import com.cloud.user.AccountManagerImpl; import com.cloud.user.dao.AccountDao; import com.cloud.utils.ListUtils; import com.cloud.utils.component.ManagerBase; @@ -110,7 +109,7 @@ public Role findRole(Long id) { /** * Simple call to {@link CallContext#current()} to retrieve the current calling account. - * This method facilitates unit testing because this avoids static methods mocks. + * This method facilitates unit testing because this avoids static methods mocks. */ protected Account getCurrentAccount() { return CallContext.current().getCallingAccount(); @@ -255,7 +254,7 @@ public List findRolesByName(String name) { /** * 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. + * 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(); @@ -265,7 +264,7 @@ protected void removeRootAdminRolesIfNeeded(List roles) { } /** - * Remove all roles that have the {@link RoleType#Admin}. + * Remove all roles that have the {@link RoleType#Admin}. */ protected void removeRootAdminRoles(List roles) { if (CollectionUtils.isEmpty(roles)) { @@ -283,8 +282,7 @@ protected void removeRootAdminRoles(List roles) { @Override public List findRolesByType(RoleType roleType) { - if (roleType == null || - RoleType.Admin == roleType && !accountManager.isRootAdmin(getCurrentAccount().getId())) { + if (roleType == null || RoleType.Admin == roleType && !accountManager.isRootAdmin(getCurrentAccount().getId())) { return Collections.emptyList(); } List roles = roleDao.findAllByRoleType(roleType); 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 d9f5f2cf3cd9..957bb56e73c6 100644 --- a/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java @@ -1,7 +1,21 @@ -package org.apache.cloudstack.acl; +// 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. -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +package org.apache.cloudstack.acl; import java.util.ArrayList; import java.util.List; @@ -21,7 +35,6 @@ import com.cloud.user.Account; import com.cloud.user.AccountManager; - @RunWith(MockitoJUnitRunner.class) public class RoleManagerImplTest { @@ -32,11 +45,11 @@ public class RoleManagerImplTest { private AccountManager accountManagerMock; @Mock private RoleDao roleDaoMock; - + @Mock private Account accountMock; private long accountMockId = 100l; - + @Mock private RoleVO roleVoMock; private long roleMockId = 1l; @@ -45,28 +58,28 @@ public class RoleManagerImplTest { 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 returedRole = roleManagerImpl.findRole(null); Assert.assertNull(returedRole); } - + @Test public void findRoleTestIdZero() { Role returedRole = roleManagerImpl.findRole(0l); Assert.assertNull(returedRole); } - + @Test public void findRoleTestIdNegative() { Role returedRole = roleManagerImpl.findRole(-1l); Assert.assertNull(returedRole); } - + @Test public void findRoleTestRoleNotFound() { Mockito.doReturn(null).when(roleDaoMock).findById(roleMockId); @@ -79,9 +92,9 @@ 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 returedRole = roleManagerImpl.findRole(roleMockId); - + Assert.assertEquals(roleMockId, returedRole.getId()); Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); Mockito.verify(roleVoMock, Mockito.times(1)).getRoleType(); @@ -92,61 +105,61 @@ 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 returedRole = roleManagerImpl.findRole(roleMockId); - + Assert.assertEquals(roleMockId, returedRole.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 returedRole = roleManagerImpl.findRole(roleMockId); - + Assert.assertEquals(roleMockId, returedRole.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 returedRole = roleManagerImpl.findRole(roleMockId); - + Assert.assertNull(returedRole); 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"; @@ -154,120 +167,120 @@ public void findRolesByNameTest() { 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 () { + 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 () { + 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 () { + 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 () { + 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); From 34b2c2a1954ba007f9f5a81bdb7b2096819b05ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Weing=C3=A4rtner?= Date: Thu, 26 Apr 2018 07:39:47 -0300 Subject: [PATCH 3/5] Break long sentence into multiple lines --- api/src/main/java/org/apache/cloudstack/acl/RoleService.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 bcb021d42c4d..6130c62a7d45 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java @@ -31,7 +31,9 @@ public interface RoleService { /** * 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. + * 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); From 27374a334c3bdc3c22175cda95bf227d56945b0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Weing=C3=A4rtner?= Date: Thu, 26 Apr 2018 07:58:46 -0300 Subject: [PATCH 4/5] Fix wording of method 'getCurrentAccount' --- .../main/java/org/apache/cloudstack/acl/RoleManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c511f16b9184..ae471b2486ca 100644 --- a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -109,7 +109,7 @@ public Role findRole(Long id) { /** * Simple call to {@link CallContext#current()} to retrieve the current calling account. - * This method facilitates unit testing because this avoids static methods mocks. + * This method facilitates unit testing, it avoids mocking static methods. */ protected Account getCurrentAccount() { return CallContext.current().getCallingAccount(); From e25884f68075493f26af5ada12a23bc695703671 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Weing=C3=A4rtner?= Date: Thu, 26 Apr 2018 16:51:44 -0300 Subject: [PATCH 5/5] fix typo in variable name --- .../cloudstack/acl/RoleManagerImplTest.java | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) 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 957bb56e73c6..bc50f3408987 100644 --- a/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/acl/RoleManagerImplTest.java @@ -64,27 +64,27 @@ public void beforeTest() { @Test public void findRoleTestIdNull() { - Role returedRole = roleManagerImpl.findRole(null); - Assert.assertNull(returedRole); + Role returnedRole = roleManagerImpl.findRole(null); + Assert.assertNull(returnedRole); } @Test public void findRoleTestIdZero() { - Role returedRole = roleManagerImpl.findRole(0l); - Assert.assertNull(returedRole); + Role returnedRole = roleManagerImpl.findRole(0l); + Assert.assertNull(returnedRole); } @Test public void findRoleTestIdNegative() { - Role returedRole = roleManagerImpl.findRole(-1l); - Assert.assertNull(returedRole); + Role returnedRole = roleManagerImpl.findRole(-1l); + Assert.assertNull(returnedRole); } @Test public void findRoleTestRoleNotFound() { Mockito.doReturn(null).when(roleDaoMock).findById(roleMockId); - Role returedRole = roleManagerImpl.findRole(roleMockId); - Assert.assertNull(returedRole); + Role returnedRole = roleManagerImpl.findRole(roleMockId); + Assert.assertNull(returnedRole); } @Test @@ -93,9 +93,9 @@ public void findRoleTestNotRootAdminAndNotRoleAdminType() { Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId); - Role returedRole = roleManagerImpl.findRole(roleMockId); + Role returnedRole = roleManagerImpl.findRole(roleMockId); - Assert.assertEquals(roleMockId, returedRole.getId()); + Assert.assertEquals(roleMockId, returnedRole.getId()); Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); Mockito.verify(roleVoMock, Mockito.times(1)).getRoleType(); } @@ -106,9 +106,9 @@ public void findRoleTestRootAdminAndNotRoleAdminType() { Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); - Role returedRole = roleManagerImpl.findRole(roleMockId); + Role returnedRole = roleManagerImpl.findRole(roleMockId); - Assert.assertEquals(roleMockId, returedRole.getId()); + Assert.assertEquals(roleMockId, returnedRole.getId()); Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); Mockito.verify(roleVoMock, Mockito.times(0)).getRoleType(); } @@ -119,9 +119,9 @@ public void findRoleTestRootAdminAndRoleAdminType() { Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); Mockito.doReturn(true).when(accountManagerMock).isRootAdmin(accountMockId); - Role returedRole = roleManagerImpl.findRole(roleMockId); + Role returnedRole = roleManagerImpl.findRole(roleMockId); - Assert.assertEquals(roleMockId, returedRole.getId()); + Assert.assertEquals(roleMockId, returnedRole.getId()); Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); Mockito.verify(roleVoMock, Mockito.times(0)).getRoleType(); } @@ -132,9 +132,9 @@ public void findRoleTestNotRootAdminAndRoleAdminType() { Mockito.doReturn(roleVoMock).when(roleDaoMock).findById(roleMockId); Mockito.doReturn(false).when(accountManagerMock).isRootAdmin(accountMockId); - Role returedRole = roleManagerImpl.findRole(roleMockId); + Role returnedRole = roleManagerImpl.findRole(roleMockId); - Assert.assertNull(returedRole); + Assert.assertNull(returnedRole); Mockito.verify(accountManagerMock).isRootAdmin(accountMockId); Mockito.verify(roleVoMock, Mockito.times(1)).getRoleType(); }