Remove redundant permission check

scm-webapp/src/main/java/sonia/scm/api/v2/resources/PermissionDtoToPermissionMapper.java

@@ -16,6 +16,6 @@ public abstract class PermissionDtoToPermissionMapper {
    * @param permissionDto the source dto
    * @return the mapped target permission object
    */
-  public abstract Permission modify(@MappingTarget Permission target, PermissionDto permissionDto);
+  public abstract void modify(@MappingTarget Permission target, PermissionDto permissionDto);
 
 }

scm-webapp/src/main/java/sonia/scm/api/v2/resources/PermissionRootResource.java

@@ -14,7 +14,6 @@ import sonia.scm.repository.Repository;
 import sonia.scm.repository.RepositoryException;
 import sonia.scm.repository.RepositoryManager;
 import sonia.scm.repository.RepositoryNotFoundException;
-import sonia.scm.repository.RepositoryPermissions;
 import sonia.scm.web.VndMediaType;
 
 import javax.inject.Inject;
@@ -69,7 +68,7 @@ public class PermissionRootResource {
   @Consumes(VndMediaType.PERMISSION)
   public Response create(@PathParam("namespace") String namespace, @PathParam("name") String name, PermissionDto permission) throws RepositoryException {
     log.info("try to add new permission: {}", permission);
-    Repository repository = checkPermission(namespace, name);
+    Repository repository = load(namespace, name);
     checkPermissionAlreadyExists(permission, repository);
     repository.getPermissions().add(dtoToModelMapper.map(permission));
     manager.modify(repository);
@@ -95,7 +94,7 @@ public class PermissionRootResource {
   @TypeHint(PermissionDto.class)
   @Path("{permission-name}")
   public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("permission-name") String permissionName) throws RepositoryException {
-    Repository repository = checkPermission(namespace, name);
+    Repository repository = load(namespace, name);
     return Response.ok(
       repository.getPermissions()
         .stream()
@@ -125,7 +124,7 @@ public class PermissionRootResource {
   @TypeHint(PermissionDto.class)
   @Path("")
   public Response getAll(@PathParam("namespace") String namespace, @PathParam("name") String name) throws RepositoryNotFoundException {
-    Repository repository = checkPermission(namespace, name);
+    Repository repository = load(namespace, name);
     List<PermissionDto> permissionDtoList = repository.getPermissions()
       .stream()
       .map(per -> modelToDtoMapper.map(per, new NamespaceAndName(repository.getNamespace(), repository.getName())))
@@ -155,7 +154,7 @@ public class PermissionRootResource {
     @PathParam("permission-name") String permissionName,
     PermissionDto permission) throws RepositoryException {
     log.info("try to update the permission with name: {}. the modified permission is: {}", permissionName, permission);
-    Repository repository = checkPermission(namespace, name);
+    Repository repository = load(namespace, name);
     Permission existingPermission = repository.getPermissions()
       .stream()
       .filter(perm -> StringUtils.isNotBlank(perm.getName()) && perm.getName().equals(permissionName))
@@ -186,7 +185,7 @@ public class PermissionRootResource {
     @PathParam("name") String name,
     @PathParam("permission-name") String permissionName) throws RepositoryException {
     log.info("try to delete the permission with name: {}.", permissionName);
-    Repository repository = checkPermission(namespace, name);
+    Repository repository = load(namespace, name);
     repository.getPermissions()
       .stream()
       .filter(perm -> StringUtils.isNotBlank(perm.getName()) && perm.getName().equals(permissionName))
@@ -208,26 +207,11 @@ public class PermissionRootResource {
    * @return the repository if the user is permitted
    * @throws RepositoryNotFoundException if the repository does not exists
    */
-  private Repository checkPermission(@PathParam("namespace") String namespace, @PathParam("name") String name) throws RepositoryNotFoundException {
+  private Repository load(String namespace, String name) throws RepositoryNotFoundException {
     return Optional.ofNullable(manager.get(new NamespaceAndName(namespace, name)))
-      .filter(repository -> {
-        checkUserPermitted(repository);
-        return true;
-      })
       .orElseThrow(() -> new RepositoryNotFoundException(name));
   }
 
-
-  /**
-   * throw exception if the user is not permitted
-   *
-   * @param repository
-   */
-  protected void checkUserPermitted(Repository repository) {
-    RepositoryPermissions.modify(repository).check();
-  }
-
-
   /**
    * check if the permission already exists in the repository
    *

scm-webapp/src/test/java/sonia/scm/api/v2/resources/PermissionRootResourceTest.java

@@ -18,10 +18,8 @@ import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.DynamicTest;
 import org.junit.jupiter.api.TestFactory;
-import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
 import sonia.scm.api.rest.AuthorizationExceptionMapper;
 import sonia.scm.repository.NamespaceAndName;
 import sonia.scm.repository.Permission;
@@ -44,14 +42,11 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.fail;
 import static org.junit.jupiter.api.DynamicTest.dynamicTest;
 import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 import static org.mockito.MockitoAnnotations.initMocks;
 
-@RunWith(MockitoJUnitRunner.Silent.class)
 @Slf4j
 public class PermissionRootResourceTest {
   private static final String REPOSITORY_NAMESPACE = "repo_namespace";
@@ -112,7 +107,7 @@ public class PermissionRootResourceTest {
   @Before
   public void prepareEnvironment() {
     initMocks(this);
-    permissionRootResource = spy(new PermissionRootResource(permissionDtoToPermissionMapper, permissionToPermissionDtoMapper, resourceLinks, repositoryManager));
+    permissionRootResource = new PermissionRootResource(permissionDtoToPermissionMapper, permissionToPermissionDtoMapper, resourceLinks, repositoryManager);
     RepositoryRootResource repositoryRootResource = new RepositoryRootResource(MockProvider
       .of(new RepositoryResource(null, null, null, null, null, null, null,null, MockProvider.of(permissionRootResource))), null);
     dispatcher.getRegistry().addSingletonResource(repositoryRootResource);
@@ -150,8 +145,7 @@ public class PermissionRootResourceTest {
   Stream<DynamicTest> missedPermissionUserForbiddenTestFactory() {
     Repository mockRepository = mock(Repository.class);
     when(mockRepository.getId()).thenReturn(REPOSITORY_NAME);
-    doThrow(AuthorizationException.class).when(permissionRootResource).checkUserPermitted(mockRepository);
+    doThrow(AuthorizationException.class).when(repositoryManager).get(any(NamespaceAndName.class));
-    when(repositoryManager.get(any(NamespaceAndName.class))).thenReturn(mockRepository);
     return createDynamicTestsToAssertResponses(
       requestGETPermission.expectedResponseStatus(403),
       requestPOSTPermission.expectedResponseStatus(403),
@@ -321,7 +315,6 @@ public class PermissionRootResourceTest {
     when(mockRepository.getId()).thenReturn(REPOSITORY_NAME);
     when(mockRepository.getNamespace()).thenReturn(REPOSITORY_NAMESPACE);
     when(mockRepository.getName()).thenReturn(REPOSITORY_NAME);
-    doNothing().when(permissionRootResource).checkUserPermitted(mockRepository);
     when(repositoryManager.get(any(NamespaceAndName.class))).thenReturn(mockRepository);
     return mockRepository;
   }
@@ -344,9 +337,9 @@ public class PermissionRootResourceTest {
       .contentType(VndMediaType.PERMISSION);
     dispatcher.invoke(request, response);
     log.info("Test the Request :{}", entry);
-    assertThat(entry.expectedResponseStatus)
+    assertThat(response.getStatus())
       .as("assert status code")
-      .isEqualTo(response.getStatus());
+      .isEqualTo(entry.expectedResponseStatus);
     if (entry.responseValidator != null) {
       entry.responseValidator.accept(response);
     }