From 9cf5fd2edbe354e2145da039a29bbff23fd8a5e4 Mon Sep 17 00:00:00 2001 From: charitha Date: Wed, 22 Jan 2020 18:54:50 +0530 Subject: [PATCH] Fix issue in accessing device shared via group --- .../impl/DeviceManagementServiceImpl.java | 50 ++++++++++++++++--- .../impl/DeviceManagementServiceImplTest.java | 37 ++++++++++---- .../DeviceAccessAuthorizationServiceImpl.java | 34 ++++++++----- .../operation/mgt/OperationManagerImpl.java | 34 ++++++------- 4 files changed, 110 insertions(+), 45 deletions(-) diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImpl.java b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImpl.java index c058b1ac69..70f61283ec 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImpl.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImpl.java @@ -60,6 +60,8 @@ import org.wso2.carbon.device.mgt.common.exceptions.DeviceManagementException; import org.wso2.carbon.device.mgt.common.exceptions.DeviceTypeNotFoundException; import org.wso2.carbon.device.mgt.common.exceptions.InvalidConfigurationException; import org.wso2.carbon.device.mgt.common.exceptions.InvalidDeviceException; +import org.wso2.carbon.device.mgt.common.group.mgt.DeviceGroup; +import org.wso2.carbon.device.mgt.common.group.mgt.GroupManagementException; import org.wso2.carbon.device.mgt.common.operation.mgt.Activity; import org.wso2.carbon.device.mgt.common.operation.mgt.Operation; import org.wso2.carbon.device.mgt.common.operation.mgt.OperationManagementException; @@ -89,6 +91,7 @@ import org.wso2.carbon.device.mgt.jaxrs.util.DeviceMgtAPIUtils; import org.wso2.carbon.policy.mgt.common.PolicyManagementException; import org.wso2.carbon.policy.mgt.core.PolicyManagerService; import org.wso2.carbon.user.api.UserStoreException; +import org.wso2.carbon.user.api.UserStoreManager; import org.wso2.carbon.utils.multitenancy.MultitenantUtils; import javax.validation.Valid; @@ -108,6 +111,7 @@ import javax.ws.rs.core.Response; import java.text.ParseException; import java.text.SimpleDateFormat; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.List; @@ -196,18 +200,52 @@ public class DeviceManagementServiceImpl implements DeviceManagementService { RequestValidationUtil.validateStatus(excludeStatus); request.setExcludeStatus(excludeStatus); } + // this is the user who initiates the request + String authorizedUser = CarbonContext.getThreadLocalCarbonContext().getUsername(); + if (groupId != 0) { - request.setGroupId(groupId); + try { + int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId(); + UserStoreManager userStoreManager = DeviceMgtAPIUtils.getRealmService() + .getTenantUserRealm(tenantId).getUserStoreManager(); + String[] userRoles = userStoreManager.getRoleListOfUser(authorizedUser); + boolean isPermitted = false; + if (deviceAccessAuthorizationService.isDeviceAdminUser()) { + isPermitted = true; + } else { + List roles = DeviceMgtAPIUtils.getGroupManagementProviderService().getRoles(groupId); + for (String userRole : userRoles) { + if (roles.contains(userRole)) { + isPermitted = true; + break; + } + } + if (!isPermitted) { + DeviceGroup deviceGroup = DeviceMgtAPIUtils.getGroupManagementProviderService() + .getGroup(groupId, false); + if (deviceGroup != null && authorizedUser.equals(deviceGroup.getOwner())) { + isPermitted = true; + } + } + } + if (isPermitted) { + request.setGroupId(groupId); + } else { + return Response.status(Response.Status.FORBIDDEN).entity( + new ErrorResponse.ErrorResponseBuilder().setMessage("Current user '" + authorizedUser + + "' doesn't have enough privileges to list devices of group '" + + groupId + "'").build()).build(); + } + } catch (GroupManagementException | UserStoreException e) { + throw new DeviceManagementException(e); + } } if (role != null && !role.isEmpty()) { request.setOwnerRole(role); } - - // this is the user who initiates the request - String authorizedUser = MultitenantUtils.getTenantAwareUsername(CarbonContext.getThreadLocalCarbonContext().getUsername()); - + authorizedUser = MultitenantUtils.getTenantAwareUsername(authorizedUser); // check whether the user is device-mgt admin - if (deviceAccessAuthorizationService.isDeviceAdminUser()) { + if (deviceAccessAuthorizationService.isDeviceAdminUser() || request.getGroupId() > 0) { if (user != null && !user.isEmpty()) { request.setOwner(MultitenantUtils.getTenantAwareUsername(user)); } else if (userPattern != null && !userPattern.isEmpty()) { diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImplTest.java b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImplTest.java index bdd8693c9a..a040e16ba2 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImplTest.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/DeviceManagementServiceImplTest.java @@ -75,6 +75,7 @@ import org.wso2.carbon.device.mgt.jaxrs.service.impl.util.DeviceMgtAPITestHelper import org.wso2.carbon.device.mgt.jaxrs.util.DeviceMgtAPIUtils; import org.wso2.carbon.policy.mgt.common.PolicyManagementException; import org.wso2.carbon.policy.mgt.core.PolicyManagerService; +import org.wso2.carbon.user.core.service.RealmService; import org.wso2.carbon.utils.multitenancy.MultitenantUtils; import javax.ws.rs.core.Response; @@ -89,8 +90,8 @@ import static org.mockito.MockitoAnnotations.initMocks; */ @PowerMockIgnore({"javax.ws.rs.*", "org.apache.log4j.*"}) @SuppressStaticInitializationFor({"org.wso2.carbon.device.mgt.jaxrs.util.DeviceMgtAPIUtils", - "org.wso2.carbon.context.CarbonContext"}) -@PrepareForTest({DeviceMgtAPIUtils.class, MultitenantUtils.class, CarbonContext.class}) + "org.wso2.carbon.context.CarbonContext", "org.wso2.carbon.user.core.service.RealmService"}) +@PrepareForTest({DeviceMgtAPIUtils.class, MultitenantUtils.class, CarbonContext.class, RealmService.class}) public class DeviceManagementServiceImplTest { private static final Log log = LogFactory.getLog(DeviceManagementServiceImplTest.class); @@ -185,6 +186,8 @@ public class DeviceManagementServiceImplTest { .toReturn(TENANT_AWARE_USERNAME); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(Mockito.mock(CarbonContext.class, Mockito.RETURNS_MOCKS)); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Response response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, DEFAULT_USERNAME, null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, @@ -271,6 +274,8 @@ public class DeviceManagementServiceImplTest { CarbonContext carbonContext = Mockito.mock(CarbonContext.class, Mockito.RETURNS_MOCKS); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(carbonContext); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Mockito.when(carbonContext.getTenantId()).thenReturn(-1234); Mockito.when(carbonContext.getUsername()).thenReturn(DEFAULT_USERNAME); Mockito.when(deviceAccessAuthorizationService.isDeviceAdminUser()).thenReturn(true); @@ -294,6 +299,8 @@ public class DeviceManagementServiceImplTest { .toReturn(deviceAccessAuthorizationService); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(carbonContext); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Mockito.when(carbonContext.getTenantId()).thenReturn(-1234); Mockito.when(carbonContext.getUsername()).thenReturn(DEFAULT_USERNAME); Mockito.when(deviceAccessAuthorizationService.isDeviceAdminUser()).thenReturn(true); @@ -323,6 +330,8 @@ public class DeviceManagementServiceImplTest { .toReturn(TENANT_AWARE_USERNAME); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(Mockito.mock(CarbonContext.class, Mockito.RETURNS_MOCKS)); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Mockito.when(deviceAccessAuthorizationService.isDeviceAdminUser()).thenReturn(true); Response response = this.deviceManagementService @@ -348,10 +357,12 @@ public class DeviceManagementServiceImplTest { .when(MultitenantUtils.class, "getTenantAwareUsername", DEFAULT_USERNAME); PowerMockito.doReturn("newuser@carbon.super").when(MultitenantUtils.class, "getTenantAwareUsername", "newuser"); Mockito.when(this.deviceAccessAuthorizationService.isDeviceAdminUser()).thenReturn(false); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Response response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, "newuser", null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, DEFAULT_EXCLUDED_STATUS, - DEFAULT_STATUS, 1, null, null, false, 10, 5); + DEFAULT_STATUS, 0, null, null, false, 10, 5); Assert.assertEquals(response.getStatus(), Response.Status.UNAUTHORIZED.getStatusCode()); Mockito.reset(this.deviceAccessAuthorizationService); } @@ -367,18 +378,20 @@ public class DeviceManagementServiceImplTest { .toReturn(TENANT_AWARE_USERNAME); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(Mockito.mock(CarbonContext.class, Mockito.RETURNS_MOCKS)); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Response response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, DEFAULT_USERNAME, null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, - DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 1, null, ifModifiedSince, false, 10, 5); + DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 0, null, ifModifiedSince, false, 10, 5); Assert.assertEquals(response.getStatus(), Response.Status.NOT_MODIFIED.getStatusCode()); response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, DEFAULT_USERNAME, null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, - DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 1, null, ifModifiedSince, true, 10, 5); + DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 0, null, ifModifiedSince, true, 10, 5); Assert.assertEquals(response.getStatus(), Response.Status.NOT_MODIFIED.getStatusCode()); response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, DEFAULT_USERNAME, null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, - DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 1, null, "ErrorModifiedSince", false, 10, 5); + DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 0, null, "ErrorModifiedSince", false, 10, 5); Assert.assertEquals(response.getStatus(), Response.Status.BAD_REQUEST.getStatusCode()); } @@ -393,18 +406,20 @@ public class DeviceManagementServiceImplTest { .toReturn(TENANT_AWARE_USERNAME); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(Mockito.mock(CarbonContext.class, Mockito.RETURNS_MOCKS)); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Response response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, DEFAULT_USERNAME, null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, - DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 1, since, null, false, 10, 5); + DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 0, since, null, false, 10, 5); Assert.assertEquals(response.getStatus(), Response.Status.OK.getStatusCode()); response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, DEFAULT_USERNAME, null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, - DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 1, since, null, true, 10, 5); + DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 0, since, null, true, 10, 5); Assert.assertEquals(response.getStatus(), Response.Status.OK.getStatusCode()); response = this.deviceManagementService .getDevices(null, TEST_DEVICE_TYPE, DEFAULT_USERNAME, null, DEFAULT_ROLE, DEFAULT_OWNERSHIP, - DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 1, "ErrorSince", null, false, 10, 5); + DEFAULT_EXCLUDED_STATUS, DEFAULT_STATUS, 0, "ErrorSince", null, false, 10, 5); Assert.assertEquals(response.getStatus(), Response.Status.BAD_REQUEST.getStatusCode()); } @@ -418,6 +433,8 @@ public class DeviceManagementServiceImplTest { .toReturn(TENANT_AWARE_USERNAME); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(Mockito.mock(CarbonContext.class, Mockito.RETURNS_MOCKS)); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Mockito.when(this.deviceManagementProviderService .getAllDevices(Mockito.any(PaginationRequest.class), Mockito.anyBoolean())) .thenThrow(new DeviceManagementException()); @@ -439,6 +456,8 @@ public class DeviceManagementServiceImplTest { .toReturn(TENANT_AWARE_USERNAME); PowerMockito.stub(PowerMockito.method(CarbonContext.class, "getThreadLocalCarbonContext")) .toReturn(Mockito.mock(CarbonContext.class, Mockito.RETURNS_MOCKS)); + PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getRealmService")) + .toReturn(Mockito.mock(RealmService.class, Mockito.RETURNS_MOCKS)); Mockito.when(this.deviceAccessAuthorizationService.isDeviceAdminUser()) .thenThrow(new DeviceAccessAuthorizationException()); diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/authorization/DeviceAccessAuthorizationServiceImpl.java b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/authorization/DeviceAccessAuthorizationServiceImpl.java index 5db998544f..9cddd7effc 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/authorization/DeviceAccessAuthorizationServiceImpl.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/authorization/DeviceAccessAuthorizationServiceImpl.java @@ -38,7 +38,6 @@ import org.wso2.carbon.user.api.UserStoreException; import java.util.Iterator; import java.util.List; - /** * Implementation of DeviceAccessAuthorization service. */ @@ -69,21 +68,30 @@ public class DeviceAccessAuthorizationServiceImpl implements DeviceAccessAuthori } //check for group permissions try { - if (groupPermissions == null || groupPermissions.length == 0) { - return false; - } - for (String groupPermission : groupPermissions) { - if (!isAuthorizedViaGroup(username, deviceIdentifier, groupPermission)) { - //if at least one fails, authorization fails - return false; + return isSharedViaGroup(deviceIdentifier, username); + } catch (GroupManagementException | UserStoreException e) { + throw new DeviceAccessAuthorizationException("Unable to authorize the access to device : " + + deviceIdentifier.getId() + " for the user : " + + username, e); + } + } + + private boolean isSharedViaGroup(DeviceIdentifier deviceIdentifier, String username) + throws GroupManagementException, UserStoreException { + List groupsWithDevice = DeviceManagementDataHolder.getInstance() + .getGroupManagementProviderService().getGroups(deviceIdentifier, false); + String[] userRoles = DeviceManagementDataHolder.getInstance().getRealmService() + .getTenantUserRealm(getTenantId()).getUserStoreManager().getRoleListOfUser(username); + for (DeviceGroup deviceGroup : groupsWithDevice) { + List sharingRoles = DeviceManagementDataHolder.getInstance() + .getGroupManagementProviderService().getRoles(deviceGroup.getGroupId()); + for (String role : userRoles) { + if (sharingRoles.contains(role)) { + return true; } } - return true; - } catch (GroupManagementException e) { - throw new DeviceAccessAuthorizationException("Unable to authorize the access to device : " + - deviceIdentifier.getId() + " for the user : " + - username, e); } + return false; } @Override diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/operation/mgt/OperationManagerImpl.java b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/operation/mgt/OperationManagerImpl.java index f2141d7a97..699aee61f3 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/operation/mgt/OperationManagerImpl.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/operation/mgt/OperationManagerImpl.java @@ -1079,21 +1079,25 @@ public class OperationManagerImpl implements OperationManager { private EnrolmentInfo getEnrolmentInfo(DeviceIdentifier deviceId, PaginationRequest request) throws OperationManagementException { - EnrolmentInfo enrolmentInfo = null; + int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId(); + String user = this.getUser(); + boolean isUserAuthorized; + try { + isUserAuthorized = DeviceManagementDataHolder.getInstance() + .getDeviceAccessAuthorizationService().isUserAuthorized(deviceId, user); + } catch (DeviceAccessAuthorizationException e) { + throw new OperationManagementException("Error occurred while checking the device access permissions for '" + + deviceId.getType() + "' device carrying the identifier '" + + deviceId.getId() + "' of owner '" + request.getOwner() + "'", e); + + } + if (!isUserAuthorized) { + return null; + } + EnrolmentInfo enrolmentInfo; try { - int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId(); - String user = this.getUser(); DeviceManagementDAOFactory.openConnection(); - if (this.isSameUser(user, request.getOwner())) { - enrolmentInfo = deviceDAO.getEnrolment(deviceId, request, tenantId); - } else { - boolean isAdminUser = DeviceManagementDataHolder.getInstance().getDeviceAccessAuthorizationService(). - isDeviceAdminUser(); - if (isAdminUser) { - enrolmentInfo = deviceDAO.getEnrolment(deviceId, request, tenantId); - } - //TODO : Add a check for group admin if this fails - } + enrolmentInfo = deviceDAO.getEnrolment(deviceId, request, tenantId); } catch (DeviceManagementDAOException e) { throw new OperationManagementException("Error occurred while retrieving enrollment data of '" + deviceId.getType() + "' device carrying the identifier '" + @@ -1101,10 +1105,6 @@ public class OperationManagerImpl implements OperationManager { } catch (SQLException e) { throw new OperationManagementException( "Error occurred while opening a connection to the data source", e); - } catch (DeviceAccessAuthorizationException e) { - throw new OperationManagementException("Error occurred while checking the device access permissions for '" + - deviceId.getType() + "' device carrying the identifier '" + - deviceId.getId() + "' of owner '" + request.getOwner() + "'", e); } finally { DeviceManagementDAOFactory.closeConnection(); }