Improving error message for creation of collections (#565) (#568)

gbif · May 2, 2024 · e3fc0e6 · e3fc0e6
1 parent 540bb31
commit e3fc0e6
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 61 deletions.
diff --git a/...main/java/org/gbif/registry/pipelines/DefaultRegistryPipelinesHistoryTrackingService.java b/...main/java/org/gbif/registry/pipelines/DefaultRegistryPipelinesHistoryTrackingService.java
@@ -13,33 +13,6 @@
  */
 package org.gbif.registry.pipelines;
 
-import com.fasterxml.jackson.databind.ObjectMapper;
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Preconditions;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Ordering;
-import freemarker.template.TemplateException;
-import java.io.IOException;
-import java.time.LocalDateTime;
-import java.time.format.DateTimeFormatter;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.EnumMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.Set;
-import java.util.UUID;
-import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.function.Consumer;
-import java.util.stream.Collectors;
-import javax.annotation.Nullable;
-import org.apache.commons.lang3.StringUtils;
-import java.util.stream.Collectors;
 import org.gbif.api.model.common.paging.Pageable;
 import org.gbif.api.model.common.paging.PagingResponse;
 import org.gbif.api.model.pipelines.*;
@@ -103,6 +76,8 @@
 import org.springframework.context.annotation.Lazy;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
+import org.springframework.web.client.HttpClientErrorException;
+import org.springframework.web.client.HttpServerErrorException;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.annotations.VisibleForTesting;
@@ -111,8 +86,6 @@
 import com.google.common.collect.Ordering;
 
 import freemarker.template.TemplateException;
-import org.springframework.web.client.HttpClientErrorException;
-import org.springframework.web.client.HttpServerErrorException;
 
 /** Service that allows to re-run pipeline steps on a specific attempt. */
 @Service

diff --git a/registry-pipelines/src/main/java/org/gbif/registry/pipelines/issues/GithubApiClient.java b/registry-pipelines/src/main/java/org/gbif/registry/pipelines/issues/GithubApiClient.java
@@ -13,12 +13,15 @@
  */
 package org.gbif.registry.pipelines.issues;
 
-import com.fasterxml.jackson.annotation.JsonIgnore;
 import java.util.List;
 import java.util.Set;
+
+import org.springframework.web.bind.annotation.*;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
 import lombok.Builder;
 import lombok.Data;
-import org.springframework.web.bind.annotation.*;
 
 public interface GithubApiClient {
   @PostMapping("/issues")

diff --git a/registry-pipelines/src/main/java/org/gbif/registry/pipelines/issues/GithubClientConfig.java b/registry-pipelines/src/main/java/org/gbif/registry/pipelines/issues/GithubClientConfig.java
@@ -13,12 +13,14 @@
  */
 package org.gbif.registry.pipelines.issues;
 
-import com.fasterxml.jackson.databind.DeserializationFeature;
-import com.fasterxml.jackson.databind.ObjectMapper;
 import org.gbif.ws.client.ClientBuilder;
+
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
 /** Lightweight client for the GitHub API. */
 @Configuration
 public class GithubClientConfig {

diff --git a/...ity/src/main/java/org/gbif/registry/security/grscicoll/GrSciCollAuthorizationService.java b/...ity/src/main/java/org/gbif/registry/security/grscicoll/GrSciCollAuthorizationService.java
@@ -41,6 +41,11 @@
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+
 import static org.gbif.registry.security.SecurityContextCheck.checkUserInRole;
 import static org.gbif.registry.security.UserRoles.GRSCICOLL_ADMIN_ROLE;
 
@@ -239,34 +244,48 @@ public boolean allowedToModifyCollection(
   }
 
   /** An editor or mediator can create an institution if they have their country in the scopes. */
-  public boolean allowedToCreateInstitution(
+  public EntityOperationResponse allowedToCreateInstitution(
       Institution institution, Authentication authentication) {
     if (SecurityContextCheck.checkUserInRole(authentication, GRSCICOLL_ADMIN_ROLE)) {
-      return true;
+      return new EntityOperationResponse(true);
     }
 
     if (institution == null) {
-      return false;
+      return new EntityOperationResponse(false, "Institution cannot be null");
+    }
+    Country country = extractCountry(institution);
+    boolean allowedToModifyCountry = allowedToModifyCountry(authentication.getName(), country);
+    if (allowedToModifyCountry) {
+      return new EntityOperationResponse(true);
+    }
+    else {
+      return new EntityOperationResponse(false, "Not allowed to create institutions for the country: " + country);
     }
-
-    return allowedToModifyCountry(authentication.getName(), extractCountry(institution));
   }
 
   /**
    * An editor or mediator can create collections if they have their institution or country in the
    * scopes.
    */
-  public boolean allowedToCreateCollection(Collection collection, Authentication authentication) {
+  public EntityOperationResponse allowedToCreateCollection(Collection collection, Authentication authentication) {
     if (SecurityContextCheck.checkUserInRole(authentication, GRSCICOLL_ADMIN_ROLE)) {
-      return true;
+      return new EntityOperationResponse(true);
     }
 
     if (collection == null) {
-      return false;
+      return new EntityOperationResponse(false, "Collection cannot be null");
     }
+    Country country = extractCountry(collection);
+    UUID institutionKey = collection.getInstitutionKey();
+    boolean allowedToModifyEntity = allowedToModifyEntity(authentication.getName(), institutionKey);
+    boolean allowedToModifyCountry = allowedToModifyCountry(authentication.getName(), country);
 
-    return allowedToModifyEntity(authentication.getName(), collection.getInstitutionKey())
-        || allowedToModifyCountry(authentication.getName(), extractCountry(collection));
+    if (allowedToModifyCountry || allowedToModifyEntity) {
+      return new EntityOperationResponse(true, "");
+    }
+    else {
+      return new EntityOperationResponse(false, "Not allowed to create collections under the institution " + institutionKey + "or in the country " + country);
+    }
   }
 
   public boolean allowedToUpdateChangeSuggestion(
@@ -285,7 +304,7 @@ public boolean allowedToUpdateChangeSuggestion(
 
       if (changeSuggestion.getType() == Type.CREATE) {
         return allowedToCreateInstitution(
-            readEntity(changeSuggestion.getSuggestedEntity(), Institution.class), authentication);
+            readEntity(changeSuggestion.getSuggestedEntity(), Institution.class), authentication).isAllowed();
       } else if (changeSuggestion.getType() == Type.DELETE) {
         return allowedToDeleteInstitution(authentication, changeSuggestion.getEntityKey());
       } else if (changeSuggestion.getType() == Type.MERGE) {
@@ -310,7 +329,7 @@ public boolean allowedToUpdateChangeSuggestion(
       Collection suggestedEntity =
           readEntity(changeSuggestion.getSuggestedEntity(), Collection.class);
       if (changeSuggestion.getType() == Type.CREATE) {
-        return allowedToCreateCollection(suggestedEntity, authentication);
+        return allowedToCreateCollection(suggestedEntity, authentication).isAllowed();
       } else if (changeSuggestion.getType() == Type.DELETE) {
         return allowedToDeleteCollection(authentication, changeSuggestion.getEntityKey());
       } else if (changeSuggestion.getType() == Type.MERGE) {
@@ -347,4 +366,17 @@ private <T> T readEntity(String content, Class<T> clazz) {
       return null;
     }
   }
+
+  @Getter
+  @Setter
+  @AllArgsConstructor
+  @NoArgsConstructor
+  public static class EntityOperationResponse {
+    private boolean allowed;
+    private String message;
+
+    public EntityOperationResponse(boolean allowed){
+      this(allowed,null);
+    }
+  }
 }
diff --git a/...rc/main/java/org/gbif/registry/security/grscicoll/GrSciCollEditorAuthorizationFilter.java b/...rc/main/java/org/gbif/registry/security/grscicoll/GrSciCollEditorAuthorizationFilter.java
@@ -60,9 +60,6 @@
 @Component
 public class GrSciCollEditorAuthorizationFilter extends OncePerRequestFilter {
 
-  private static final Logger LOG =
-      LoggerFactory.getLogger(GrSciCollEditorAuthorizationFilter.class);
-
   public static final String GRSCICOLL_PATH = "grscicoll";
   public static final Pattern ENTITY_PATTERN =
       Pattern.compile(".*/grscicoll/(collection|institution)/([a-f0-9-]+).*");
@@ -79,10 +76,10 @@ public class GrSciCollEditorAuthorizationFilter extends OncePerRequestFilter {
       Pattern.compile(".*/grscicoll/(collection|institution)/([a-f0-9-]+)/merge$");
   public static final Pattern CONVERSION_PATTERN =
       Pattern.compile(".*/grscicoll/institution/([a-f0-9-]+)/convertToCollection$");
-
   public static final String INSTITUTION = "institution";
   public static final String COLLECTION = "collection";
-
+  private static final Logger LOG =
+      LoggerFactory.getLogger(GrSciCollEditorAuthorizationFilter.class);
   private final GrSciCollAuthorizationService authService;
   private final AuthenticationFacade authenticationFacade;
   private final ObjectMapper objectMapper;
@@ -237,21 +234,26 @@ private void checkInstitutionAndCollectionCreationPermissions(
     if ("POST".equals(request.getMethod()) && createEntityMatch.find()) {
       String entityType = createEntityMatch.group(1);
 
-      boolean allowed = false;
+      GrSciCollAuthorizationService.EntityOperationResponse entityOperationResponse =
+          new GrSciCollAuthorizationService.EntityOperationResponse();
       if (INSTITUTION.equalsIgnoreCase(entityType)) {
         Institution institution = readEntity(request, Institution.class);
-        allowed = authService.allowedToCreateInstitution(institution, authentication);
+        entityOperationResponse = authService.allowedToCreateInstitution(institution, authentication);
       } else if (COLLECTION.equalsIgnoreCase(entityType)) {
         Collection collection = readEntity(request, Collection.class);
-        allowed = authService.allowedToCreateCollection(collection, authentication);
+        entityOperationResponse = authService.allowedToCreateCollection(collection, authentication);
       }
 
-      if (!allowed) {
-        throw new WebApplicationException(
-            MessageFormat.format(
-                "User {0} is not allowed to create entity {1}",
-                authentication.getName(), entityType),
-            HttpStatus.FORBIDDEN);
+      if (!entityOperationResponse.isAllowed()) {
+        String errorMessage = MessageFormat.format(
+          "User {0} is not allowed to create entity {1}",
+          authentication.getName(), entityType);
+
+        if (entityOperationResponse.getMessage() != null && !entityOperationResponse.getMessage().isEmpty()) {
+          errorMessage += ". Reason: " + entityOperationResponse.getMessage();
+        }
+
+        throw new WebApplicationException(errorMessage, HttpStatus.FORBIDDEN);
       }
     }
   }

diff --git a/...ice/src/main/java/org/gbif/registry/service/collections/batch/CollectionBatchHandler.java b/...ice/src/main/java/org/gbif/registry/service/collections/batch/CollectionBatchHandler.java
@@ -60,7 +60,7 @@ public CollectionBatchHandler(
 
   @Override
   boolean allowedToCreateEntity(Collection entity, Authentication authentication) {
-    return authorizationService.allowedToCreateCollection(entity, authentication);
+    return authorizationService.allowedToCreateCollection(entity, authentication).isAllowed();
   }
 
   @Override

diff --git a/...ce/src/main/java/org/gbif/registry/service/collections/batch/InstitutionBatchHandler.java b/...ce/src/main/java/org/gbif/registry/service/collections/batch/InstitutionBatchHandler.java
@@ -59,7 +59,7 @@ public InstitutionBatchHandler(
 
   @Override
   boolean allowedToCreateEntity(Institution entity, Authentication authentication) {
-    return authorizationService.allowedToCreateInstitution(entity, authentication);
+    return authorizationService.allowedToCreateInstitution(entity, authentication).isAllowed();
   }
 
   @Override