Peer-Review

scm-core/src/main/java/sonia/scm/repository/PermissionAlreadyExistsException.java

@@ -5,7 +5,7 @@ import java.text.MessageFormat;
 public class PermissionAlreadyExistsException extends RepositoryException {
 
   public PermissionAlreadyExistsException(Repository repository, String permissionName) {
-    super(MessageFormat.format("the permission {0} of the repository {1}/{2} is already exists", permissionName, repository.getNamespace(), repository.getName()));
+    super(MessageFormat.format("the permission {0} of the repository {1}/{2} already exists", permissionName, repository.getNamespace(), repository.getName()));
   }
 
 }

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

@@ -38,7 +38,6 @@ import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.Provider;
 
 /**
- * @author mkarray
  * @since 2.0.0
  */
 @Provider

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

@@ -38,7 +38,6 @@ import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.Provider;
 
 /**
- * @author mkarray
  * @since 2.0.0
  */
 @Provider

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

@@ -38,7 +38,6 @@ import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.Provider;
 
 /**
- * @author mkarray
  * @since 2.0.0
  */
 @Provider

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

@@ -6,11 +6,25 @@ import com.webcohesion.enunciate.metadata.rs.StatusCodes;
 import com.webcohesion.enunciate.metadata.rs.TypeHint;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang.StringUtils;
-import sonia.scm.repository.*;
+import sonia.scm.repository.NamespaceAndName;
+import sonia.scm.repository.PermissionAlreadyExistsException;
+import sonia.scm.repository.PermissionNotFoundException;
+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;
-import javax.ws.rs.*;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
 import javax.ws.rs.core.Response;
 import java.net.URI;
 import java.util.List;
@@ -58,7 +72,7 @@ public class PermissionRootResource {
     checkPermissionAlreadyExists(permission, repository);
     repository.getPermissions().add(dtoToModelMapper.map(permission));
     manager.modify(repository);
-    return Response.created(URI.create(resourceLinks.permission().self(namespace,name,permission.getName()))).build();
+    return Response.created(URI.create(resourceLinks.permission().self(namespace, name, permission.getName()))).build();
   }
 
 
@@ -84,8 +98,8 @@ public class PermissionRootResource {
     return Response.ok(
       repository.getPermissions()
         .stream()
-        .filter(permission -> StringUtils.isNotBlank(permission.getName())  && permission.getName().equals(permissionName))
-        .map(permission -> modelToDtoMapper.map(permission, new NamespaceAndName(repository.getNamespace(),repository.getName())))
+        .filter(permission -> permissionName.equals(permission.getName()))
+        .map(permission -> modelToDtoMapper.map(permission, new NamespaceAndName(repository.getNamespace(), repository.getName())))
         .findFirst()
         .orElseThrow(() -> new PermissionNotFoundException(repository, permissionName))
     ).build();
@@ -113,7 +127,7 @@ public class PermissionRootResource {
     Repository repository = checkPermission(namespace, name);
     List<PermissionDto> permissionDtoList = repository.getPermissions()
       .stream()
-      .map(per -> modelToDtoMapper.map(per, new NamespaceAndName(repository.getNamespace(),repository.getName())))
+      .map(per -> modelToDtoMapper.map(per, new NamespaceAndName(repository.getNamespace(), repository.getName())))
       .collect(Collectors.toList());
     return Response.ok(permissionDtoList).build();
   }
@@ -136,56 +150,55 @@ public class PermissionRootResource {
   @Consumes(VndMediaType.PERMISSION)
   @Path("{permission-name}")
   public Response update(@PathParam("namespace") String namespace,
-                         @PathParam("name") String name,
-                         @PathParam("permission-name") String permissionName,
-                         PermissionDto permission) throws RepositoryException {
+    @PathParam("name") String name,
+    @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.getPermissions()
       .stream()
-      .filter(perm -> StringUtils.isNotBlank(perm.getName())  && perm.getName().equals(permissionName))
+      .filter(perm -> StringUtils.isNotBlank(perm.getName()) && perm.getName().equals(permissionName))
       .findFirst()
-      .map(p ->  dtoToModelMapper.map(p, permission))
+      .map(p -> dtoToModelMapper.map(p, permission))
       .orElseThrow(() -> new PermissionNotFoundException(repository, permissionName))
-      ;
+    ;
     manager.modify(repository);
     log.info("the permission with name: {} is updated.", permissionName);
     return Response.noContent().build();
   }
 
- /**
+  /**
    * Update a permission to the user or group managed by the repository
    *
    * @param permissionName permission to delete
    * @return a web response with the status code 204
    */
- @DELETE
- @StatusCodes({
-   @ResponseCode(code = 204, condition = "delete success or nothing to delete"),
-   @ResponseCode(code = 401, condition = "not authenticated / invalid credentials"),
-   @ResponseCode(code = 403, condition = "not authorized"),
-   @ResponseCode(code = 500, condition = "internal server error")
- })
- @TypeHint(TypeHint.NO_CONTENT.class)
+  @DELETE
+  @StatusCodes({
+    @ResponseCode(code = 204, condition = "delete success or nothing to delete"),
+    @ResponseCode(code = 401, condition = "not authenticated / invalid credentials"),
+    @ResponseCode(code = 403, condition = "not authorized"),
+    @ResponseCode(code = 500, condition = "internal server error")
+  })
+  @TypeHint(TypeHint.NO_CONTENT.class)
   @Path("{permission-name}")
   public Response delete(@PathParam("namespace") String namespace,
-                         @PathParam("name") String name,
-                         @PathParam("permission-name") String permissionName) throws RepositoryException {
+    @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.getPermissions()
       .stream()
-      .filter(perm -> StringUtils.isNotBlank(perm.getName())  && perm.getName().equals(permissionName))
+      .filter(perm -> StringUtils.isNotBlank(perm.getName()) && perm.getName().equals(permissionName))
       .findFirst()
       .ifPresent(p -> repository.getPermissions().remove(p))
-      ;
+    ;
     manager.modify(repository);
     log.info("the permission with name: {} is updated.", permissionName);
     return Response.noContent().build();
   }
 
 
-
   /**
    * check if the actual user is permitted to manage the repository permissions
    * return the repository if the user is permitted
@@ -207,6 +220,7 @@ public class PermissionRootResource {
 
   /**
    * throw exception if the user is not permitted
+   *
    * @param repository
    */
   protected void checkUserPermitted(Repository repository) {

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

@@ -38,7 +38,6 @@ import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.Provider;
 
 /**
- * @author mkarray
  * @since 2.0.0
  */
 @Provider

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

@@ -2,8 +2,6 @@ package sonia.scm.api.v2.resources;
 
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.github.sdorra.shiro.ShiroRule;
-import com.github.sdorra.shiro.SubjectAware;
 import com.google.common.collect.ImmutableList;
 import lombok.ToString;
 import lombok.extern.slf4j.Slf4j;
@@ -15,7 +13,6 @@ import org.jboss.resteasy.mock.MockHttpRequest;
 import org.jboss.resteasy.mock.MockHttpResponse;
 import org.jboss.resteasy.spi.HttpRequest;
 import org.junit.Before;
-import org.junit.Rule;
 import org.junit.Test;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
@@ -25,7 +22,11 @@ import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
-import sonia.scm.repository.*;
+import sonia.scm.repository.NamespaceAndName;
+import sonia.scm.repository.Permission;
+import sonia.scm.repository.PermissionType;
+import sonia.scm.repository.Repository;
+import sonia.scm.repository.RepositoryManager;
 import sonia.scm.web.VndMediaType;
 
 import java.io.IOException;
@@ -42,14 +43,13 @@ 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.*;
+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;
 
-@SubjectAware(
-  username = "trillian",
-  password = "secret",
-  configuration = "classpath:sonia/scm/repository/shiro.ini"
-)
 @RunWith(MockitoJUnitRunner.Silent.class)
 @Slf4j
 public class PermissionRootResourceTest {
@@ -93,9 +93,6 @@ public class PermissionRootResourceTest {
 
   private final Dispatcher dispatcher = MockDispatcherFactory.createDispatcher();
 
-  @Rule
-  public ShiroRule shiro = new ShiroRule();
-
   @Mock
   private RepositoryManager repositoryManager;
 
@@ -163,13 +160,13 @@ public class PermissionRootResourceTest {
   }
 
   @Test
-  public void shouldGetAllPermissions() {
+  public void shouldGetAllPermissions() throws URISyntaxException {
     authorizedUserHasARepositoryWithPermissions(TEST_PERMISSIONS);
     assertGettingExpectedPermissions(ImmutableList.copyOf(TEST_PERMISSIONS));
   }
 
   @Test
-  public void shouldGetPermissionByName() {
+  public void shouldGetPermissionByName() throws URISyntaxException {
     authorizedUserHasARepositoryWithPermissions(TEST_PERMISSIONS);
     Permission expectedPermission = TEST_PERMISSIONS.get(0);
     assertExpectedRequest(requestGETPermission
@@ -192,7 +189,7 @@ public class PermissionRootResourceTest {
   }
 
   @Test
-  public void shouldGetCreatedPermissions() {
+  public void shouldGetCreatedPermissions() throws URISyntaxException {
     authorizedUserHasARepositoryWithPermissions(TEST_PERMISSIONS);
     Permission newPermission = new Permission("new_group_perm", PermissionType.WRITE, true);
     ArrayList<Permission> permissions = Lists.newArrayList(TEST_PERMISSIONS);
@@ -209,7 +206,7 @@ public class PermissionRootResourceTest {
   }
 
   @Test
-  public void shouldNotAddExistingPermission() {
+  public void shouldNotAddExistingPermission() throws URISyntaxException {
     authorizedUserHasARepositoryWithPermissions(TEST_PERMISSIONS);
     Permission newPermission = TEST_PERMISSIONS.get(0);
     assertExpectedRequest(requestPOSTPermission
@@ -219,7 +216,7 @@ public class PermissionRootResourceTest {
   }
 
   @Test
-  public void shouldGetUpdatedPermissions() {
+  public void shouldGetUpdatedPermissions() throws URISyntaxException {
     authorizedUserHasARepositoryWithPermissions(TEST_PERMISSIONS);
     Permission modifiedPermission = TEST_PERMISSIONS.get(0);
     // modify the type to owner
@@ -238,7 +235,7 @@ public class PermissionRootResourceTest {
 
 
   @Test
-  public void shouldDeletePermissions() {
+  public void shouldDeletePermissions() throws URISyntaxException {
     authorizedUserHasARepositoryWithPermissions(TEST_PERMISSIONS);
     Permission deletedPermission = TEST_PERMISSIONS.get(0);
     ImmutableList<Permission> expectedPermissions = ImmutableList.copyOf(TEST_PERMISSIONS.subList(1, TEST_PERMISSIONS.size()));
@@ -253,7 +250,7 @@ public class PermissionRootResourceTest {
   }
 
   @Test
-  public void deletingNotExistingPermissionShouldProcess() {
+  public void deletingNotExistingPermissionShouldProcess() throws URISyntaxException {
     authorizedUserHasARepositoryWithPermissions(TEST_PERMISSIONS);
     Permission deletedPermission = TEST_PERMISSIONS.get(0);
     ImmutableList<Permission> expectedPermissions = ImmutableList.copyOf(TEST_PERMISSIONS.subList(1, TEST_PERMISSIONS.size()));
@@ -275,7 +272,7 @@ public class PermissionRootResourceTest {
     assertGettingExpectedPermissions(expectedPermissions);
   }
 
-  private void assertGettingExpectedPermissions(ImmutableList<Permission> expectedPermissions) {
+  private void assertGettingExpectedPermissions(ImmutableList<Permission> expectedPermissions) throws URISyntaxException {
     assertExpectedRequest(requestGETAllPermissions
       .expectedResponseStatus(200)
       .responseValidator((response) -> {
@@ -337,17 +334,13 @@ public class PermissionRootResourceTest {
       .map(entry -> dynamicTest("the endpoint " + entry.description + " should return the status code " + entry.expectedResponseStatus, () -> assertExpectedRequest(entry)));
   }
 
-  private MockHttpResponse assertExpectedRequest(ExpectedRequest entry) {
+  private MockHttpResponse assertExpectedRequest(ExpectedRequest entry) throws URISyntaxException {
     MockHttpResponse response = new MockHttpResponse();
     HttpRequest request = null;
-    try {
-      request = MockHttpRequest
-        .create(entry.method, "/" + RepositoryRootResource.REPOSITORIES_PATH_V2 + entry.path)
-        .content(entry.content)
-        .contentType(VndMediaType.PERMISSION);
-    } catch (URISyntaxException e) {
-      fail(e.getMessage());
-    }
+    request = MockHttpRequest
+      .create(entry.method, "/" + RepositoryRootResource.REPOSITORIES_PATH_V2 + entry.path)
+      .content(entry.content)
+      .contentType(VndMediaType.PERMISSION);
     dispatcher.invoke(request, response);
     log.info("Test the Request :{}", entry);
     assertThat(entry.expectedResponseStatus)