Fixed bug in permissions management

gms-ui/src/components/MembersPanel.vue

 <template>
-<b-tab title="Members" v-if="model.permission === 'ADMIN' || model.permission === 'MANAGE_MEMBERS' || model.permission === 'VIEW_MEMBERS'">
+<b-tab title="Members" :title-link-class="{ 'd-none': (model.permission === 'TRAVERSE') }">
   <div v-if="model.membersPanel !== null">
     <b-list-group v-for="member in model.membersPanel.items" id="members-list" v-bind:key="member.memberId">
       <b-list-group-item href="#" @click.prevent="openUser(member)">

gms-ui/src/components/PermissionsPanel.vue

 <template>
-<b-tab title="Permissions" v-if="model.permission === 'ADMIN'">
+<b-tab title="Permissions" :title-link-class="{ 'd-none': (model.permission !== 'ADMIN') }">
   <div v-if="model.permissionsPanel !== null">
     <table class="table b-table table-striped table-hover">
       <thead>

gms/src/main/java/it/inaf/ia2/gms/controller/MembersController.java

@@ -4,7 +4,6 @@ import it.inaf.ia2.gms.manager.MembershipManager;
 import it.inaf.ia2.gms.manager.PermissionsManager;
 import it.inaf.ia2.gms.model.request.AddMemberRequest;
 import it.inaf.ia2.gms.model.response.PaginatedData;
-import it.inaf.ia2.gms.model.Permission;
 import it.inaf.ia2.gms.model.RapUser;
 import it.inaf.ia2.gms.model.request.PaginatedModelRequest;
 import it.inaf.ia2.gms.model.request.RemoveMemberRequest;
@@ -52,16 +51,7 @@ public class MembersController {
         GroupEntity group = groupsService.getGroupById(request.getGroupId());
 
         membershipManager.addMember(group, request.getUserId());
-
-        Permission currentUserPermission = permissionsManager.getCurrentUserPermission(group);
-
-        if (currentUserPermission == Permission.MANAGE_MEMBERS) {
-            // Automatically assign the VIEW_MEMBERS permission ("Add collaborator" feature)
-            permissionsManager.addPermission(group, request.getUserId(), Permission.VIEW_MEMBERS);
-        } else if (request.getPermission() != null) {
-            // Admin users can specify a permission
         permissionsManager.addPermission(group, request.getUserId(), request.getPermission());
-        }
 
         return new ResponseEntity<>(getMembersPanel(group, request), HttpStatus.CREATED);
     }
@@ -73,20 +63,7 @@ public class MembersController {
 
         membershipManager.removeMember(group, request.getUserId());
 
-        Permission currentUserPermission = permissionsManager.getCurrentUserPermission(group);
-
-        // For users having the MANAGE_MEMBERS permission, the VIEW_MEMBERS permission
-        // is automatically assigned when they add a member ("Add collaborator" feature).
-        // We want to keep also the reverse behavior.
-        // If the member permission is not VIEW_MEMBERS that means that it has been
-        // changed by an ADMIN user, so we don't remove it.
-        boolean removeCollaborator = currentUserPermission == Permission.MANAGE_MEMBERS
-                && permissionsManager.getUserPermission(group, request.getUserId()) == Permission.VIEW_MEMBERS;
-
-        // ADMIN users can choose if delete also the permission or not.
-        boolean adminRemovePermission = currentUserPermission == Permission.ADMIN && request.isRemoveAlsoPermission();
-
-        if (removeCollaborator || adminRemovePermission) {
+        if (request.isRemoveAlsoPermission()) {
             permissionsManager.removePermission(group, request.getUserId());
         }
 

gms/src/main/java/it/inaf/ia2/gms/manager/PermissionsManager.java

@@ -15,6 +15,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
@@ -72,16 +73,19 @@ public class PermissionsManager extends UserAwareComponent {
         return null;
     }
 
-    public Permission getUserPermission(GroupEntity group, String userId) {
-        verifyUserCanManagePermissions(group);
-        List<PermissionEntity> permissions = permissionsService.findUserPermissions(group, userId);
-        return PermissionUtils.getGroupPermission(group, permissions).orElse(null);
-    }
-
     public PermissionEntity addPermission(GroupEntity group, String userId, Permission permission) {
-        verifyUserCanManagePermissions(group);
+
+        Permission currentUserPermission = getCurrentUserPermission(group);
+
+        if (currentUserPermission == Permission.MANAGE_MEMBERS && permission == Permission.VIEW_MEMBERS) {
+            // Automatically assign the VIEW_MEMBERS permission ("Add collaborator" feature)
+            return permissionsService.addPermission(group, userId, Permission.VIEW_MEMBERS);
+        } else if (currentUserPermission == Permission.ADMIN) {
+            // Admin users can specify a permission
             return permissionsService.addPermission(group, userId, permission);
         }
+        throw unauthorizedExceptionSupplier(group).get();
+    }
 
     public PermissionEntity updatePermission(GroupEntity group, String userId, Permission permission) {
         verifyUserCanManagePermissions(group);
@@ -89,16 +93,47 @@ public class PermissionsManager extends UserAwareComponent {
     }
 
     public void removePermission(GroupEntity group, String userId) {
-        verifyUserCanManagePermissions(group);
+
+        Permission currentUserPermission = getCurrentUserPermission(group);
+
+        // For users having the MANAGE_MEMBERS permission, the VIEW_MEMBERS permission
+        // is automatically assigned when they add a member ("Add collaborator" feature).
+        // We want to keep also the reverse behavior.
+        if (currentUserPermission == Permission.MANAGE_MEMBERS) {
+            if (getUserPermission(group, userId, false) == Permission.VIEW_MEMBERS) {
                 permissionsService.removePermission(group, userId);
             }
+            // If the member permission is not VIEW_MEMBERS that means that it has been
+            // changed by an ADMIN user, so we don't remove it.
+        } else if (currentUserPermission == Permission.ADMIN) {
+            permissionsService.removePermission(group, userId);
+        } else {
+            throw unauthorizedExceptionSupplier(group).get();
+        }
+    }
+
+    public Permission getUserPermission(GroupEntity group, String userId) {
+        return getUserPermission(group, userId, true);
+    }
+
+    private Permission getUserPermission(GroupEntity group, String userId, boolean verify) {
+        if (verify) {
+            verifyUserCanManagePermissions(group);
+        }
+        List<PermissionEntity> permissions = permissionsService.findUserPermissions(group, userId);
+        return PermissionUtils.getGroupPermission(group, permissions).orElse(null);
+    }
 
     private void verifyUserCanManagePermissions(GroupEntity group) {
         Permission permission = getCurrentUserPermission(group);
         if (permission != Permission.ADMIN) {
-            loggingDAO.logAction("Unauthorized attempt to manage permissions");
-            throw new UnauthorizedException("Only admin users can handle permissions");
+            throw unauthorizedExceptionSupplier(group).get();
+        }
     }
+
+    private Supplier<UnauthorizedException> unauthorizedExceptionSupplier(GroupEntity group) {
+        loggingDAO.logAction("Unauthorized attempt to manage permissions [group_id=" + group.getId() + "]");
+        return () -> new UnauthorizedException("You don't have the privileges for managing the requested permission");
     }
 
     public List<PermissionEntity> getCurrentUserPermissions(GroupEntity group) {

gms/src/main/java/it/inaf/ia2/gms/model/request/AddMemberRequest.java

 package it.inaf.ia2.gms.model.request;
 
 import it.inaf.ia2.gms.model.Permission;
+import javax.validation.constraints.NotNull;
 
 public class AddMemberRequest extends MemberRequest {
 
-    /**
-     * When adding a member it is possible to assign also a permission. This
-     * field can be null (in that case the user is member of the group but
-     * he/she can't do nothing (not even seeing him/her group membership).
-     */
+    @NotNull
     private Permission permission;
 
     public Permission getPermission() {

gms/src/test/java/it/inaf/ia2/gms/controller/MembersControllerTest.java

 package it.inaf.ia2.gms.controller;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
-import it.inaf.ia2.gms.authn.SessionData;
 import it.inaf.ia2.gms.manager.MembershipManager;
 import it.inaf.ia2.gms.manager.PermissionsManager;
 import it.inaf.ia2.gms.model.request.AddMemberRequest;
@@ -57,7 +56,6 @@ public class MembersControllerTest {
         group.setPath("parent_id.group_id");
 
         when(groupsService.getGroupById(eq("group_id"))).thenReturn(group);
-        when(permissionsManager.getCurrentUserPermission(eq(group))).thenReturn(Permission.ADMIN);
     }
 
     @Test
@@ -68,10 +66,11 @@ public class MembersControllerTest {
         request.setUserId("user_id");
         request.setPaginatorPage(1);
         request.setPaginatorPageSize(10);
+        request.setPermission(Permission.VIEW_MEMBERS);
 
         mockMvc.perform(post("/member")
                 .content(mapper.writeValueAsString(request))
-                .contentType(MediaType.APPLICATION_JSON_UTF8))
+                .contentType(MediaType.APPLICATION_JSON_VALUE))
                 .andExpect(status().isCreated())
                 .andExpect(jsonPath("$.currentPage", is(1)));
 

gms/src/test/java/it/inaf/ia2/gms/manager/PermissionsManagerIntegrationTest.java

 package it.inaf.ia2.gms.manager;
 
 import it.inaf.ia2.gms.DataSourceConfig;
-import it.inaf.ia2.gms.authn.RapPrincipal;
 import it.inaf.ia2.gms.model.Permission;
 import it.inaf.ia2.gms.model.RapUser;
 import it.inaf.ia2.gms.model.UserPermission;
@@ -13,9 +12,7 @@ import it.inaf.ia2.gms.persistence.model.PermissionEntity;
 import it.inaf.ia2.gms.rap.RapClient;
 import it.inaf.ia2.gms.service.PermissionsService;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 import javax.servlet.http.HttpServletRequest;
 import javax.sql.DataSource;
 import static org.junit.Assert.assertEquals;

gms/src/test/java/it/inaf/ia2/gms/manager/PermissionsManagerTest.java

+package it.inaf.ia2.gms.manager;
+
+import it.inaf.ia2.gms.exception.UnauthorizedException;
+import it.inaf.ia2.gms.model.Permission;
+import it.inaf.ia2.gms.persistence.LoggingDAO;
+import it.inaf.ia2.gms.persistence.model.GroupEntity;
+import it.inaf.ia2.gms.persistence.model.PermissionEntity;
+import it.inaf.ia2.gms.service.PermissionsService;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import javax.servlet.http.HttpServletRequest;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class PermissionsManagerTest {
+
+    private static final String USER_ID = "USER_ID";
+    private static final String TARGET_USER_ID = "TARGET_USER_ID";
+
+    @Mock
+    private PermissionsService permissionsService;
+    @Mock
+    private HttpServletRequest request;
+    @Mock
+    private LoggingDAO loggingDAO;
+
+    @InjectMocks
+    private PermissionsManager permissionsManager;
+
+    @Before
+    public void setUp() {
+        UserAwareComponentTestUtil.setUser(permissionsManager, USER_ID);
+    }
+
+    @Test
+    public void addPermissionFromManager() {
+
+        GroupEntity group = getGroup();
+
+        when(permissionsService.findUserPermissions(eq(group), eq(USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.MANAGE_MEMBERS));
+
+        permissionsManager.addPermission(group, TARGET_USER_ID, Permission.VIEW_MEMBERS);
+
+        verify(permissionsService, times(1)).addPermission(eq(group), eq(TARGET_USER_ID), eq(Permission.VIEW_MEMBERS));
+    }
+
+    @Test
+    public void addPermissionFromAdmin() {
+
+        GroupEntity group = getGroup();
+
+        when(permissionsService.findUserPermissions(eq(group), eq(USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.ADMIN));
+
+        permissionsManager.addPermission(group, TARGET_USER_ID, Permission.MANAGE_MEMBERS);
+
+        verify(permissionsService, times(1)).addPermission(eq(group), eq(TARGET_USER_ID), eq(Permission.MANAGE_MEMBERS));
+    }
+
+    @Test(expected = UnauthorizedException.class)
+    public void addPermissionFromUnauthorized() {
+
+        GroupEntity group = getGroup();
+
+        when(permissionsService.findUserPermissions(eq(group), eq(USER_ID)))
+                .thenReturn(new ArrayList<>());
+
+        permissionsManager.addPermission(group, TARGET_USER_ID, Permission.MANAGE_MEMBERS);
+    }
+
+    @Test
+    public void removePermissionFromManager() {
+
+        GroupEntity group = getGroup();
+
+        when(permissionsService.findUserPermissions(eq(group), eq(USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.MANAGE_MEMBERS));
+
+        when(permissionsService.findUserPermissions(eq(group), eq(TARGET_USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.VIEW_MEMBERS));
+
+        permissionsManager.removePermission(group, TARGET_USER_ID);
+
+        verify(permissionsService, times(1)).removePermission(eq(group), eq(TARGET_USER_ID));
+    }
+
+    @Test
+    public void removePermissionFromManagerAddedByAdmin() {
+
+        GroupEntity group = getGroup();
+
+        when(permissionsService.findUserPermissions(eq(group), eq(USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.MANAGE_MEMBERS));
+
+        when(permissionsService.findUserPermissions(eq(group), eq(TARGET_USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.MANAGE_MEMBERS));
+
+        permissionsManager.removePermission(group, TARGET_USER_ID);
+
+        verify(permissionsService, never()).removePermission(any(), any());
+    }
+
+    @Test
+    public void removePermissionFromAdmin() {
+
+        GroupEntity group = getGroup();
+
+        when(permissionsService.findUserPermissions(eq(group), eq(USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.ADMIN));
+
+        permissionsManager.removePermission(group, TARGET_USER_ID);
+
+        verify(permissionsService, times(1)).removePermission(eq(group), eq(TARGET_USER_ID));
+    }
+
+    @Test(expected = UnauthorizedException.class)
+    public void removePermissionFromUnauthorized() {
+
+        GroupEntity group = getGroup();
+
+        when(permissionsService.findUserPermissions(eq(group), eq(USER_ID)))
+                .thenReturn(getUserPermissions(group, Permission.VIEW_MEMBERS));
+
+        permissionsManager.removePermission(group, TARGET_USER_ID);
+    }
+
+    private List<PermissionEntity> getUserPermissions(GroupEntity group, Permission permission) {
+        PermissionEntity entity = new PermissionEntity();
+        entity.setPermission(permission);
+        entity.setGroupId(group.getId());
+        entity.setGroupPath(group.getPath());
+        entity.setUserId(USER_ID);
+        return Collections.singletonList(entity);
+    }
+
+    private GroupEntity getGroup() {
+        GroupEntity group = new GroupEntity();
+        group.setId("group_id");
+        group.setPath("parent_id.group_id");
+        return group;
+    }
+}