Fix issue in accessing device shared via group

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) {
+                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<String> 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);
             }
-
+            authorizedUser = MultitenantUtils.getTenantAwareUsername(authorizedUser);
-            // this is the user who initiates the request
-            String authorizedUser = MultitenantUtils.getTenantAwareUsername(CarbonContext.getThreadLocalCarbonContext().getUsername());
-
             // 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()) {

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"})
+        "org.wso2.carbon.context.CarbonContext", "org.wso2.carbon.user.core.service.RealmService"})
-@PrepareForTest({DeviceMgtAPIUtils.class, MultitenantUtils.class, CarbonContext.class})
+@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());
 

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,23 +68,32 @@ public class DeviceAccessAuthorizationServiceImpl implements DeviceAccessAuthori
         }
         //check for group permissions
         try {
-            if (groupPermissions == null || groupPermissions.length == 0) {
+            return isSharedViaGroup(deviceIdentifier, username);
-                return false;
+        } catch (GroupManagementException | UserStoreException e) {
-            }
-            for (String groupPermission : groupPermissions) {
-                if (!isAuthorizedViaGroup(username, deviceIdentifier, groupPermission)) {
-                    //if at least one fails, authorization fails
-                    return false;
-                }
-            }
-            return true;
-        } catch (GroupManagementException 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<DeviceGroup> groupsWithDevice = DeviceManagementDataHolder.getInstance()
+                .getGroupManagementProviderService().getGroups(deviceIdentifier, false);
+        String[] userRoles = DeviceManagementDataHolder.getInstance().getRealmService()
+                .getTenantUserRealm(getTenantId()).getUserStoreManager().getRoleListOfUser(username);
+        for (DeviceGroup deviceGroup : groupsWithDevice) {
+            List<String> sharingRoles = DeviceManagementDataHolder.getInstance()
+                    .getGroupManagementProviderService().getRoles(deviceGroup.getGroupId());
+            for (String role : userRoles) {
+                if (sharingRoles.contains(role)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
     @Override
     public boolean isUserAuthorized(DeviceIdentifier deviceIdentifier, String username)
             throws DeviceAccessAuthorizationException {

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;
-        try {
         int tenantId = CarbonContext.getThreadLocalCarbonContext().getTenantId();
         String user = this.getUser();
-            DeviceManagementDAOFactory.openConnection();
+        boolean isUserAuthorized;
-            if (this.isSameUser(user, request.getOwner())) {
+        try {
-                enrolmentInfo = deviceDAO.getEnrolment(deviceId, request, tenantId);
+            isUserAuthorized = DeviceManagementDataHolder.getInstance()
-            } else {
+                    .getDeviceAccessAuthorizationService().isUserAuthorized(deviceId, user);
-                boolean isAdminUser = DeviceManagementDataHolder.getInstance().getDeviceAccessAuthorizationService().
+        } catch (DeviceAccessAuthorizationException e) {
-                        isDeviceAdminUser();
+            throw new OperationManagementException("Error occurred while checking the device access permissions for '" +
-                if (isAdminUser) {
+                    deviceId.getType() + "' device carrying the identifier '" +
-                    enrolmentInfo = deviceDAO.getEnrolment(deviceId, request, tenantId);
+                    deviceId.getId() + "' of owner '" + request.getOwner() + "'", e);
+
         }
-                //TODO : Add a check for group admin if this fails
+        if (!isUserAuthorized) {
+            return null;
         }
+        EnrolmentInfo enrolmentInfo;
+        try {
+            DeviceManagementDAOFactory.openConnection();
+            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();
         }