Fix repository role manager

scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java

@@ -53,6 +53,7 @@ import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 @Singleton @EagerSingleton
 public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager
@@ -156,38 +157,43 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager
   }
 
   private Optional<RepositoryRole> findSystemRole(String id) {
-    return repositoryPermissionProvider.availableRoles().stream().filter(role -> role.getName().equals(id)).findFirst();
+    return repositoryPermissionProvider
+      .availableRoles()
+      .stream()
+      .filter(role -> !repositoryRoleDAO.getType().equals(role.getType()))
+      .filter(role -> role.getName().equals(id)).findFirst();
   }
 
   @Override
-  public Collection<RepositoryRole> getAll() {
+  public List<RepositoryRole> getAll() {
-    return getAll(repositoryRole -> true, null);
-  }
-
-  @Override
-  public Collection<RepositoryRole> getAll(Predicate<RepositoryRole> filter, Comparator<RepositoryRole> comparator) {
     List<RepositoryRole> repositoryRoles = new ArrayList<>();
 
     if (!RepositoryRolePermissions.read().isPermitted()) {
-      return Collections.emptySet();
+      return Collections.emptyList();
     }
     for (RepositoryRole repositoryRole : repositoryPermissionProvider.availableRoles()) {
       repositoryRoles.add(repositoryRole.clone());
     }
 
-    if (comparator != null) {
-      Collections.sort(repositoryRoles, comparator);
-    }
-
     return repositoryRoles;
   }
 
   @Override
-  public Collection<RepositoryRole> getAll(Comparator<RepositoryRole> comaparator, int start, int limit) {
+  public Collection<RepositoryRole> getAll(Predicate<RepositoryRole> filter, Comparator<RepositoryRole> comparator) {
-    if (!RepositoryRolePermissions.read().isPermitted()) {
+    List<RepositoryRole> repositoryRoles = getAll();
-      return Collections.emptySet();
+
+    List<RepositoryRole> filteredRoles = repositoryRoles.stream().filter(filter::test).collect(Collectors.toList());
+
+    if (comparator != null) {
+      filteredRoles.sort(comparator);
     }
-    return Util.createSubCollection(repositoryRoleDAO.getAll(), comaparator,
+
+    return filteredRoles;
+  }
+
+  @Override
+  public Collection<RepositoryRole> getAll(Comparator<RepositoryRole> comaparator, int start, int limit) {
+    return Util.createSubCollection(getAll(), comaparator,
       (collection, item) -> {
         collection.add(item.clone());
       }, start, limit);

scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryRoleManagerTest.java

@@ -5,7 +5,6 @@ import org.apache.shiro.subject.Subject;
 import org.apache.shiro.subject.support.SubjectThreadState;
 import org.apache.shiro.util.ThreadContext;
 import org.apache.shiro.util.ThreadState;
-import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
@@ -14,19 +13,36 @@ import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+import sonia.scm.NotFoundException;
 import sonia.scm.security.RepositoryPermissionProvider;
 
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.List;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 @ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
 class DefaultRepositoryRoleManagerTest {
 
+  private static final String CUSTOM_ROLE_NAME = "customRole";
+  private static final String SYSTEM_ROLE_NAME = "systemRole";
+  private static final RepositoryRole CUSTOM_ROLE = new RepositoryRole(CUSTOM_ROLE_NAME, singletonList("custom"), "xml");
+  private static final RepositoryRole SYSTEM_ROLE = new RepositoryRole(SYSTEM_ROLE_NAME, singletonList("system"), "system");
+
   private final Subject subject = mock(Subject.class);
   private final ThreadState subjectThreadState = new SubjectThreadState(subject);
 
@@ -51,6 +67,17 @@ class DefaultRepositoryRoleManagerTest {
     ThreadContext.bind(subject);
   }
 
+  @BeforeEach
+  void initDao() {
+    when(dao.getType()).thenReturn("xml");
+  }
+
+  @BeforeEach
+  void mockExistingRole() {
+    when(dao.get(CUSTOM_ROLE_NAME)).thenReturn(CUSTOM_ROLE);
+    when(permissionProvider.availableRoles()).thenReturn(asList(CUSTOM_ROLE, SYSTEM_ROLE));
+  }
+
   @AfterEach
   void cleanupContext() {
     ThreadContext.unbindSubject();
@@ -61,7 +88,8 @@ class DefaultRepositoryRoleManagerTest {
 
     @BeforeEach
     void authorizeUser() {
-      when(subject.isPermitted(any(String.class))).thenReturn(true);
+      when(subject.isPermitted("repositoryRole:read")).thenReturn(true);
+      when(subject.isPermitted("repositoryRole:modify")).thenReturn(true);
     }
 
     @Test
@@ -69,6 +97,75 @@ class DefaultRepositoryRoleManagerTest {
       RepositoryRole role = manager.get("noSuchRole");
       assertThat(role).isNull();
     }
+
+    @Test
+    void shouldReturnRole_forExistingRole() {
+      RepositoryRole role = manager.get(CUSTOM_ROLE_NAME);
+      assertThat(role).isNotNull();
+    }
+
+    @Test
+    void shouldCreateRole() {
+      RepositoryRole role = manager.create(new RepositoryRole("new", singletonList("custom"), null));
+      assertThat(role.getType()).isEqualTo("xml");
+      verify(dao).add(role);
+    }
+
+    @Test
+    void shouldNotCreateRole_whenSystemRoleExists() {
+      assertThrows(UnauthorizedException.class, () -> manager.create(new RepositoryRole(SYSTEM_ROLE_NAME, singletonList("custom"), null)));
+      verify(dao, never()).add(any());
+    }
+
+    @Test
+    void shouldModifyRole() {
+      RepositoryRole role = new RepositoryRole(CUSTOM_ROLE_NAME, singletonList("changed"), null);
+      manager.modify(role);
+      verify(dao).modify(role);
+    }
+
+    @Test
+    void shouldNotModifyRole_whenRoleDoesNotExists() {
+      assertThrows(NotFoundException.class, () -> manager.modify(new RepositoryRole("noSuchRole", singletonList("changed"), null)));
+      verify(dao, never()).modify(any());
+    }
+
+    @Test
+    void shouldNotModifyRole_whenSystemRoleExists() {
+      assertThrows(UnauthorizedException.class, () -> manager.modify(new RepositoryRole(SYSTEM_ROLE_NAME, singletonList("changed"), null)));
+      verify(dao, never()).modify(any());
+    }
+
+    @Test
+    void shouldReturnAllRoles() {
+      List<RepositoryRole> allRoles = manager.getAll();
+      assertThat(allRoles).containsExactly(CUSTOM_ROLE, SYSTEM_ROLE);
+    }
+
+    @Test
+    void shouldReturnFilteredRoles() {
+      Collection<RepositoryRole> allRoles = manager.getAll(role -> CUSTOM_ROLE_NAME.equals(role.getName()), null);
+      assertThat(allRoles).containsExactly(CUSTOM_ROLE);
+    }
+
+    @Test
+    void shouldReturnOrderedFilteredRoles() {
+      Collection<RepositoryRole> allRoles =
+        manager.getAll(
+          role -> true,
+          Comparator.comparing(RepositoryRole::getType));
+      assertThat(allRoles).containsExactly(SYSTEM_ROLE, CUSTOM_ROLE);
+    }
+
+    @Test
+    void shouldReturnPaginatedRoles() {
+      Collection<RepositoryRole> allRoles =
+        manager.getAll(
+          Comparator.comparing(RepositoryRole::getType),
+          1, 1
+        );
+      assertThat(allRoles).containsExactly(CUSTOM_ROLE);
+    }
   }
 
   @Nested
@@ -80,8 +177,35 @@ class DefaultRepositoryRoleManagerTest {
     }
 
     @Test
-    void x() {
+    void shouldThrowException_forGet() {
-      assertThrows(UnauthorizedException.class, () -> manager.get("noSuchRole"));
+      assertThrows(UnauthorizedException.class, () -> manager.get("any"));
+    }
+
+    @Test
+    void shouldThrowException_forCreate() {
+      assertThrows(UnauthorizedException.class, () -> manager.create(new RepositoryRole("new", singletonList("custom"), null)));
+      verify(dao, never()).add(any());
+    }
+
+    @Test
+    void shouldThrowException_forModify() {
+      assertThrows(UnauthorizedException.class, () -> manager.modify(new RepositoryRole(CUSTOM_ROLE_NAME, singletonList("custom"), null)));
+      verify(dao, never()).modify(any());
+    }
+
+    @Test
+    void shouldReturnEmptyList() {
+      assertThat(manager.getAll()).isEmpty();
+    }
+
+    @Test
+    void shouldReturnEmptyFilteredList() {
+      assertThat(manager.getAll(x -> true, null)).isEmpty();
+    }
+
+    @Test
+    void shouldReturnEmptyPaginatedList() {
+      assertThat(manager.getAll(1, 1)).isEmpty();
     }
   }
 }