Prevent non-admin users from making duplicates of their own or monthl…

…y downloads. Resolves #242, #184
gbif · Mar 23, 2021 · b997556 · b997556
1 parent fb32d0a
commit b997556
Show file tree

Hide file tree

Showing 9 changed files with 148 additions and 26 deletions.
diff --git a/occurrence-integration-tests/src/main/java/org/gbif/occurrence/it/OccurrenceITModule.java b/occurrence-integration-tests/src/main/java/org/gbif/occurrence/it/OccurrenceITModule.java
@@ -1,6 +1,6 @@
 package org.gbif.occurrence.it;
 
-/** This is a temporal fix for the following problem:
+/** This is a temporary fix for the following problem:
  JAR will be empty - no content was marked for inclusion!
  */
 public interface OccurrenceITModule {}
diff --git a/...ration-tests/src/test/java/org/gbif/occurrence/test/mocks/DownloadRequestServiceMock.java b/...ration-tests/src/test/java/org/gbif/occurrence/test/mocks/DownloadRequestServiceMock.java
@@ -8,6 +8,7 @@
 
 import java.io.File;
 import java.io.InputStream;
+import java.util.Date;
 import java.util.Objects;
 import javax.annotation.Nullable;
 import javax.validation.constraints.NotNull;
@@ -17,7 +18,7 @@
 import org.springframework.core.io.ResourceLoader;
 
 /**
- * Download request service that uses the directly the Callback service and mock OccurrenceDownloadService.
+ * Download request service that uses directly the Callback service and mock OccurrenceDownloadService.
  * All file download request resolve to the same test file 0011066-200127171203522.zip.
  */
 public class DownloadRequestServiceMock implements DownloadRequestService {
@@ -53,6 +54,7 @@ public String create(@NotNull DownloadRequest downloadRequest) {
     Download download = new Download();
     download.setStatus(Download.Status.PREPARING);
     download.setRequest(downloadRequest);
+    download.setCreated(new Date());
     occurrenceDownloadService.create(download);
     downloadCallbackService.processCallback(download.getKey(), Download.Status.PREPARING.name());
     return download.getKey();

diff --git a/...tegration-tests/src/test/java/org/gbif/occurrence/ws/it/OccurrenceDownloadResourceIT.java b/...tegration-tests/src/test/java/org/gbif/occurrence/ws/it/OccurrenceDownloadResourceIT.java
@@ -6,7 +6,6 @@
 import org.gbif.api.service.occurrence.DownloadRequestService;
 import org.gbif.api.service.registry.OccurrenceDownloadService;
 import org.gbif.occurrence.ws.client.OccurrenceDownloadWsClient;
-import org.gbif.ws.MethodNotAllowedException;
 import org.gbif.ws.client.ClientBuilder;
 
 import java.io.ByteArrayOutputStream;
@@ -90,7 +89,7 @@ public void startDownloadWithDifferentUserAndCreatorTest() {
     predicateDownloadRequest.setCreator("NotMe");
 
     //Exception expected
-    assertThrows(MethodNotAllowedException.class, () -> downloadWsClient.create(predicateDownloadRequest));
+    assertThrows(AccessDeniedException.class, () -> downloadWsClient.create(predicateDownloadRequest));
   }
 
   @Test

diff --git a/occurrence-ws/pom.xml b/occurrence-ws/pom.xml
@@ -173,10 +173,20 @@
       <groupId>org.gbif.registry</groupId>
       <artifactId>registry-identity</artifactId>
       <exclusions>
-      	<exclusion>
-      		<groupId>com.google.guava</groupId>
-      		<artifactId>guava</artifactId>
-      	</exclusion>
+        <exclusion>
+          <groupId>com.google.guava</groupId>
+          <artifactId>guava</artifactId>
+        </exclusion>
+      </exclusions>
+    </dependency>
+    <dependency>
+      <groupId>org.gbif.registry</groupId>
+      <artifactId>registry-security</artifactId>
+      <exclusions>
+        <exclusion>
+          <groupId>com.google.guava</groupId>
+          <artifactId>guava</artifactId>
+        </exclusion>
       </exclusions>
     </dependency>
     <dependency>

diff --git a/...nce-ws/src/main/java/org/gbif/occurrence/download/service/DownloadRequestServiceImpl.java b/...nce-ws/src/main/java/org/gbif/occurrence/download/service/DownloadRequestServiceImpl.java
@@ -157,7 +157,7 @@ public String create(DownloadRequest request) {
       String exceedSimultaneousLimit = downloadLimitsService.exceedsSimultaneousDownloadLimit(request.getCreator());
       if (exceedSimultaneousLimit != null) {
         LOG.info("Download request refused as it would exceed simultaneous limits");
-        //TODO: ENHANCE_YOUR_CALM
+        // Keep HTTP 420 ("Enhance your calm") here.
         throw new ResponseStatusException(HttpStatus.METHOD_FAILURE, "A download limitation is exceeded:\n" + exceedSimultaneousLimit + "\n");
       }
 

diff --git a/occurrence-ws/src/main/java/org/gbif/occurrence/download/service/DownloadSecurityUtil.java b/occurrence-ws/src/main/java/org/gbif/occurrence/download/service/DownloadSecurityUtil.java
@@ -8,9 +8,13 @@
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.gbif.registry.security.SecurityContextCheck;
 import org.springframework.http.HttpStatus;
+import org.springframework.security.core.Authentication;
 import org.springframework.web.server.ResponseStatusException;
 
+import static org.gbif.registry.security.UserRoles.ADMIN_ROLE;
+
 /**
  * Common security checks used for occurrence downloads.
  */
@@ -27,24 +31,38 @@ private DownloadSecurityUtil() {
 
   /**
    * Checks that the user principal.name is the creator of the download.
+   * Or, the user is an admin.
    *
    * @throws AccessControlException if no or wrong user is authenticated
    */
-  public static void assertLoginMatches(DownloadRequest request, Principal principal) {
-    if (!principal.getName().equals(request.getCreator())) {
+  public static void assertLoginMatches(DownloadRequest request, Authentication authentication, Principal principal) {
+    if (!principal.getName().equals(request.getCreator()) &&
+      !SecurityContextCheck.checkUserInRole(authentication, ADMIN_ROLE)) {
       LOG.warn("Different user authenticated [{}] than download specifies [{}]", principal.getName(),
         request.getCreator());
-      throw new ResponseStatusException(HttpStatus.METHOD_NOT_ALLOWED, principal.getName() + " not allowed to create download with creator "
-                                                            + request.getCreator());
+      throw new ResponseStatusException(HttpStatus.UNAUTHORIZED,
+        principal.getName() + " not allowed to create download with creator " + request.getCreator());
     }
   }
 
   /**
    * Asserts that a user is authenticated, returns the user principal if present.
    */
   public static Principal assertUserAuthenticated(Principal principal) {
-    // assert authenticated user is the same as in download
     return Optional.ofNullable(principal)
             .orElseThrow(() ->  new ResponseStatusException(HttpStatus.UNAUTHORIZED, "No user authenticated for creating a download"));
   }
+
+  /**
+   * Checks if the user can bypass the monthly downloads, i.e. create huge downloads themselves.
+   *
+   * Used, for example, by the monthly download user (download.gbif.org).
+   */
+  public static boolean assertMonthlyDownloadBypass(Authentication authentication) {
+    if (authentication == null || authentication.getName() == null) {
+      return false;
+    }
+
+    return SecurityContextCheck.checkUserInRole(authentication, ADMIN_ROLE);
+  }
 }
diff --git a/occurrence-ws/src/main/java/org/gbif/occurrence/ws/resources/DownloadResource.java b/occurrence-ws/src/main/java/org/gbif/occurrence/ws/resources/DownloadResource.java
@@ -12,15 +12,22 @@
  */
 package org.gbif.occurrence.ws.resources;
 
+import static org.gbif.api.model.occurrence.Download.Status.PREPARING;
+import static org.gbif.api.model.occurrence.Download.Status.RUNNING;
+import static org.gbif.api.model.occurrence.Download.Status.SUCCEEDED;
 import static org.gbif.occurrence.download.service.DownloadSecurityUtil.assertLoginMatches;
 import static org.gbif.occurrence.download.service.DownloadSecurityUtil.assertUserAuthenticated;
+import static org.gbif.occurrence.download.service.DownloadSecurityUtil.assertMonthlyDownloadBypass;
 
 import java.io.File;
 import java.io.IOException;
 import java.io.RandomAccessFile;
 import java.security.Principal;
 import java.text.SimpleDateFormat;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
 import java.util.Date;
+import java.util.EnumSet;
 import java.util.Enumeration;
 import java.util.Objects;
 import java.util.Optional;
@@ -36,6 +43,9 @@
 import com.google.common.collect.Sets;
 import org.apache.commons.lang3.StringUtils;
 
+import org.gbif.api.model.common.paging.PagingRequest;
+import org.gbif.api.model.common.paging.PagingResponse;
+import org.gbif.api.model.occurrence.Download;
 import org.gbif.api.model.occurrence.DownloadFormat;
 import org.gbif.api.model.occurrence.DownloadRequest;
 import org.gbif.api.model.occurrence.PredicateDownloadRequest;
@@ -55,6 +65,8 @@
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
 import org.springframework.security.access.annotation.Secured;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.validation.annotation.Validated;
 import org.springframework.web.bind.annotation.DeleteMapping;
 import org.springframework.web.bind.annotation.GetMapping;
@@ -111,7 +123,8 @@ public DownloadResource(
   @DeleteMapping("{key}")
   public void delDownload(@PathVariable("key") String jobId, @Autowired Principal principal) {
     // service.get returns a download or throws NotFoundException
-    assertLoginMatches(occurrenceDownloadService.get(jobId).getRequest(), principal);
+    Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+    assertLoginMatches(occurrenceDownloadService.get(jobId).getRequest(), authentication, principal);
     LOG.debug("Delete download: [{}]", jobId);
     requestService.cancel(jobId);
   }
@@ -140,7 +153,6 @@ public ResponseEntity<StreamingResponseBody> getStreamResult(
 
     try {
 
-      //LOG.debug("Ranged request, Range: {}", range);
       if (LOG.isDebugEnabled()) {
         // Temporarily, in case we find weird clients.
         LOG.debug("Range {} request, dumping all headers:", request.getMethod());
@@ -209,7 +221,7 @@ public ResponseEntity<StreamingResponseBody> getStreamResult(
 
   /**
    * Single file download.
-   * Be aware this method is called when the header HttpHeaders.RANGE is ABSENT, other the getStreamResult is invoked.
+   * Be aware this method is called when the header HttpHeaders.RANGE is ABSENT, otherwise getStreamResult is invoked.
    */
   @GetMapping(value = "{key}", produces = {APPLICATION_OCTET_STREAM_QS_VALUE, MediaType.APPLICATION_JSON_VALUE,
     "application/x-javascript"})
@@ -277,23 +289,53 @@ public ResponseEntity<String> startDownload(
     @NotNull @Valid @RequestBody PredicateDownloadRequest request, @Autowired Principal principal
   ) {
     try {
-      return ResponseEntity.ok(createDownload(request, principal));
+      Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+      return ResponseEntity.ok(createDownload(request, authentication, principal));
     } catch (ResponseStatusException rse) {
       return ResponseEntity.status(rse.getStatus()).body(rse.getReason());
     }
   }
 
   /**
    * Creates/Starts an occurrence download.
+   *
+   * Non-admin users may be given an existing download key, where the monthly download user
+   * has already created a suitable download.
    */
-  private String createDownload(DownloadRequest downloadRequest, @Autowired Principal principal) {
+  private String createDownload(DownloadRequest downloadRequest, Authentication authentication, @Autowired Principal principal) {
     Principal userAuthenticated = assertUserAuthenticated(principal);
     if (Objects.isNull(downloadRequest.getCreator())) {
       downloadRequest.setCreator(userAuthenticated.getName());
     }
-    LOG.debug("Download: [{}]", downloadRequest);
-    // assert authenticated user is the same as in download
-    assertLoginMatches(downloadRequest, userAuthenticated);
+    LOG.info("New download request: [{}]", downloadRequest);
+    // User matches (or admin user)
+    assertLoginMatches(downloadRequest, authentication, userAuthenticated);
+
+    if (!assertMonthlyDownloadBypass(authentication) &&
+      downloadRequest instanceof PredicateDownloadRequest) {
+      PredicateDownloadRequest predicateDownloadRequest = (PredicateDownloadRequest) downloadRequest;
+      if (!predicateDownloadRequest.getFormat().equals(DownloadFormat.SPECIES_LIST)) {
+
+        // Check for recent monthly downloads with the same predicate
+        PagingResponse<Download> monthlyDownloads = occurrenceDownloadService.listByUser("download.gbif.org",
+          new PagingRequest(0, 50), EnumSet.of(PREPARING, RUNNING, SUCCEEDED));
+        String existingMonthlyDownload = matchExistingDownload(monthlyDownloads, predicateDownloadRequest,
+          Date.from(Instant.now().minus(35, ChronoUnit.DAYS)));
+        if (existingMonthlyDownload != null) {
+          return existingMonthlyDownload;
+        }
+      }
+
+      // Check for recent user downloads (of this same user) with the same predicate
+      PagingResponse<Download> userDownloads = occurrenceDownloadService.listByUser(userAuthenticated.getName(),
+        new PagingRequest(0, 50), EnumSet.of(PREPARING, RUNNING, SUCCEEDED));
+      String existingUserDownload = matchExistingDownload(userDownloads, predicateDownloadRequest,
+        Date.from(Instant.now().minus(4, ChronoUnit.HOURS)));
+      if (existingUserDownload != null) {
+        return existingUserDownload;
+      }
+    }
+
     String downloadKey = requestService.create(downloadRequest);
     LOG.info("Created new download job with key [{}]", downloadKey);
     return downloadKey;
@@ -312,7 +354,8 @@ public String download(
     @Autowired Principal principal
   ) {
     Preconditions.checkArgument(!Strings.isNullOrEmpty(format), "Format can't be null");
-    return createDownload(downloadPredicate(httpRequest, emails, format, principal), principal);
+    Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+    return createDownload(downloadPredicate(httpRequest, emails, format, principal), authentication, principal);
   }
 
   @GetMapping("predicate")
@@ -331,6 +374,39 @@ public DownloadRequest downloadPredicate(
     return new PredicateDownloadRequest(predicate, creator, notificationAddress, true, downloadFormat);
   }
 
+  /**
+   * Search existingDownloads for a download with a format and predicate matching newDownload,
+   * from cutoff at the earliest.
+   *
+   * @return The download key, if there's a match.
+   */
+  private String matchExistingDownload(
+    PagingResponse<Download> existingDownloads,
+    PredicateDownloadRequest newDownload,
+    Date cutoff) {
+    for (Download existingDownload : existingDownloads.getResults()) {
+      // Downloads are in descending order by creation date
+      if (existingDownload.getCreated().before(cutoff)) {
+        return null;
+      }
+
+      if (existingDownload.getRequest() instanceof PredicateDownloadRequest) {
+        PredicateDownloadRequest existingPredicateDownload = (PredicateDownloadRequest) existingDownload.getRequest();
+        if (newDownload.getFormat() == existingPredicateDownload.getFormat() &&
+          Objects.equals(newDownload.getPredicate(), existingPredicateDownload.getPredicate())) {
+          LOG.info("Found existing {} download {} ({}) matching new download request.",
+            existingPredicateDownload.getFormat(), existingDownload.getKey(),
+            existingPredicateDownload.getCreator());
+          return existingDownload.getKey();
+        }
+      } else {
+        LOG.warn("Unexpected download type {}", existingDownload.getClass());
+      }
+    }
+
+    return null;
+  }
+
   /**
    * Transforms a String that contains elements split by ',' into a Set of strings.
    */

diff --git a/occurrence-ws/src/test/java/org/gbif/occurrence/ws/resources/DownloadResourceTest.java b/occurrence-ws/src/test/java/org/gbif/occurrence/ws/resources/DownloadResourceTest.java
@@ -3,11 +3,14 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import org.gbif.api.model.common.GbifUser;
 import org.gbif.api.model.common.GbifUserPrincipal;
+import org.gbif.api.model.common.paging.PagingResponse;
+import org.gbif.api.model.occurrence.Download;
 import org.gbif.api.model.occurrence.DownloadFormat;
 import org.gbif.api.model.occurrence.PredicateDownloadRequest;
 import org.gbif.api.model.occurrence.predicate.EqualsPredicate;
@@ -17,11 +20,13 @@
 import org.gbif.occurrence.download.service.CallbackService;
 
 import java.security.Principal;
+import java.util.Collections;
 
-import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.context.SecurityContextHolder;
 
 public class DownloadResourceTest {
 
@@ -51,21 +56,28 @@ public void testStartDownload() {
   public void testStartDownloadNotAuthenticated() {
     prepareMocks("foo");
     ResponseEntity<String> response = resource.startDownload(dl, principal);
-    assertEquals(HttpStatus.METHOD_NOT_ALLOWED, response.getStatusCode());
+    assertEquals(HttpStatus.UNAUTHORIZED, response.getStatusCode());
   }
 
   private void prepareMocks(String user) {
     CallbackService callbackService = mock(CallbackService.class);
     DownloadRequestService service = mock(DownloadRequestService.class);
     OccurrenceDownloadService downloadService = mock(OccurrenceDownloadService.class);
+
     GbifUser gbifUser = new GbifUser();
     gbifUser.setUserName(user);
     principal = new GbifUserPrincipal(gbifUser);
 
+    Authentication auth = mock(Authentication.class);
+    SecurityContextHolder.getContext().setAuthentication(auth);
+
     resource = new DownloadResource(service, callbackService, downloadService);
     dl = new PredicateDownloadRequest(new EqualsPredicate(OccurrenceSearchParameter.TAXON_KEY, "1", false), USER, null, true,
       DownloadFormat.DWCA);
+
+    PagingResponse<Download> empty = new PagingResponse<>();
+    empty.setResults(Collections.EMPTY_LIST);
+    when(downloadService.listByUser(any(), any(), any())).thenReturn(empty);
     when(service.create(dl)).thenReturn(JOB_ID);
   }
-
 }
diff --git a/pom.xml b/pom.xml
@@ -407,6 +407,11 @@
           </exclusion>
         </exclusions>
       </dependency>
+      <dependency>
+        <groupId>org.gbif.registry</groupId>
+        <artifactId>registry-security</artifactId>
+        <version>${gbif-registry.version}</version>
+      </dependency>
       <dependency>
         <groupId>org.gbif.registry</groupId>
         <artifactId>registry-metadata</artifactId>