diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java index 2507d4bf25..82b3f8bd58 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryRoleManager.java +++ b/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); diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryRoleManagerTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryRoleManagerTest.java new file mode 100644 index 0000000000..3f2d62c600 --- /dev/null +++ b/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")); + } + } +}