System roles should not be modifiable

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

@@ -35,6 +35,7 @@ package sonia.scm.repository;
 
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
+import org.apache.shiro.authz.UnauthorizedException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.EagerSingleton;
@@ -76,6 +77,7 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager
 
   @Override
   public RepositoryRole create(RepositoryRole repositoryRole) {
+    assertNoSystemRole(repositoryRole);
     String type = repositoryRole.getType();
     if (Util.isEmpty(type)) {
       repositoryRole.setType(repositoryRoleDAO.getType());
@@ -93,6 +95,7 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager
 
   @Override
   public void delete(RepositoryRole repositoryRole) {
+    assertNoSystemRole(repositoryRole);
     logger.info("delete repositoryRole {} of type {}", repositoryRole.getName(), repositoryRole.getType());
     managerDaoAdapter.delete(
       repositoryRole,
@@ -108,6 +111,7 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager
 
   @Override
   public void modify(RepositoryRole repositoryRole) {
+    assertNoSystemRole(repositoryRole);
     logger.info("modify repositoryRole {} of type {}", repositoryRole.getName(), repositoryRole.getType());
     managerDaoAdapter.modify(
       repositoryRole,
@@ -130,11 +134,17 @@ public class DefaultRepositoryRoleManager extends AbstractRepositoryRoleManager
 
   @Override
   public RepositoryRole get(String id) {
-    RepositoryRolePermissions.read();
+    RepositoryRolePermissions.read().check();
 
     return findSystemRole(id).orElse(findCustomRole(id));
   }
 
+  private void assertNoSystemRole(RepositoryRole repositoryRole) {
+    if (findSystemRole(repositoryRole.getId()).isPresent()) {
+      throw new UnauthorizedException("system roles cannot be modified");
+    }
+  }
+
   private RepositoryRole findCustomRole(String id) {
     RepositoryRole repositoryRole = repositoryRoleDAO.get(id);
 

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

@@ -0,0 +1,87 @@
+package sonia.scm.repository;
+
+import org.apache.shiro.authz.UnauthorizedException;
+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;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+import sonia.scm.security.RepositoryPermissionProvider;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.*;
+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.when;
+
+@ExtendWith(MockitoExtension.class)
+class DefaultRepositoryRoleManagerTest {
+
+  private final Subject subject = mock(Subject.class);
+  private final ThreadState subjectThreadState = new SubjectThreadState(subject);
+
+  @Mock
+  private RepositoryRoleDAO dao;
+  @Mock
+  private RepositoryPermissionProvider permissionProvider;
+
+  @InjectMocks
+  private DefaultRepositoryRoleManager manager;
+
+  @BeforeEach
+  void initUser() {
+    subjectThreadState.bind();
+    doAnswer(invocation -> {
+      String permission = invocation.getArguments()[0].toString();
+      if (!subject.isPermitted(permission)) {
+        throw new UnauthorizedException(permission);
+      }
+      return null;
+    }).when(subject).checkPermission(anyString());
+    ThreadContext.bind(subject);
+  }
+
+  @AfterEach
+  void cleanupContext() {
+    ThreadContext.unbindSubject();
+  }
+
+  @Nested
+  class WithAuthorizedUser {
+
+    @BeforeEach
+    void authorizeUser() {
+      when(subject.isPermitted(any(String.class))).thenReturn(true);
+    }
+
+    @Test
+    void shouldReturnNull_forNotExistingRole() {
+      RepositoryRole role = manager.get("noSuchRole");
+      assertThat(role).isNull();
+    }
+  }
+
+  @Nested
+  class WithUnauthorizedUser {
+
+    @BeforeEach
+    void authorizeUser() {
+      when(subject.isPermitted(any(String.class))).thenReturn(false);
+    }
+
+    @Test
+    void x() {
+      assertThrows(UnauthorizedException.class, () -> manager.get("noSuchRole"));
+    }
+  }
+}