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 index 337ffd271..e6a822870 100644 --- 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/occurrence-integration-tests/src/test/java/org/gbif/occurrence/test/mocks/DownloadRequestServiceMock.java b/occurrence-integration-tests/src/test/java/org/gbif/occurrence/test/mocks/DownloadRequestServiceMock.java index c73da9ae9..08eda717f 100644 --- a/occurrence-integration-tests/src/test/java/org/gbif/occurrence/test/mocks/DownloadRequestServiceMock.java +++ b/occurrence-integration-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/occurrence-integration-tests/src/test/java/org/gbif/occurrence/ws/it/OccurrenceDownloadResourceIT.java b/occurrence-integration-tests/src/test/java/org/gbif/occurrence/ws/it/OccurrenceDownloadResourceIT.java index 371286e90..c9d520baf 100644 --- a/occurrence-integration-tests/src/test/java/org/gbif/occurrence/ws/it/OccurrenceDownloadResourceIT.java +++ b/occurrence-integration-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 index e94418c60..e119f9782 100644 --- a/occurrence-ws/pom.xml +++ b/occurrence-ws/pom.xml @@ -173,10 +173,20 @@ org.gbif.registry registry-identity - - com.google.guava - guava - + + com.google.guava + guava + + + + + org.gbif.registry + registry-security + + + com.google.guava + guava + diff --git a/occurrence-ws/src/main/java/org/gbif/occurrence/download/service/DownloadRequestServiceImpl.java b/occurrence-ws/src/main/java/org/gbif/occurrence/download/service/DownloadRequestServiceImpl.java index 6627f6250..57bd11b77 100644 --- a/occurrence-ws/src/main/java/org/gbif/occurrence/download/service/DownloadRequestServiceImpl.java +++ b/occurrence-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 index 120377687..0c0248288 100644 --- 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,15 +31,17 @@ 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()); } } @@ -43,8 +49,20 @@ public static void assertLoginMatches(DownloadRequest request, Principal princip * 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 index 3c71d0793..d01ed0ba1 100644 --- 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 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 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,7 +289,8 @@ public ResponseEntity 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()); } @@ -285,15 +298,44 @@ public ResponseEntity startDownload( /** * 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 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 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 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 index 413edcbfb..c88bdc650 100644 --- 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 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 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 index 6c147ab4f..a3d51e108 100644 --- a/pom.xml +++ b/pom.xml @@ -407,6 +407,11 @@ + + org.gbif.registry + registry-security + ${gbif-registry.version} + org.gbif.registry registry-metadata