Fix authorization checks in attachment upload

src/main/java/eu/bbmri_eric/negotiator/api/controller/v3/AttachmentController.java

@@ -1,10 +1,10 @@
 package eu.bbmri_eric.negotiator.api.controller.v3;
 
+import eu.bbmri_eric.negotiator.configuration.security.auth.NegotiatorUserDetailsService;
 import eu.bbmri_eric.negotiator.dto.attachments.AttachmentDTO;
 import eu.bbmri_eric.negotiator.dto.attachments.AttachmentMetadataDTO;
 import eu.bbmri_eric.negotiator.service.AttachmentService;
 import java.util.List;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.MediaType;
@@ -25,7 +25,6 @@ public class AttachmentController {
 
   private final AttachmentService storageService;
 
-  @Autowired
   public AttachmentController(AttachmentService storageService) {
     this.storageService = storageService;
   }
@@ -39,7 +38,11 @@ public AttachmentMetadataDTO createForNegotiation(
       @PathVariable String negotiationId,
       @RequestParam("file") MultipartFile file,
       @Nullable @RequestParam("organizationId") String organizationId) {
-    return storageService.createForNegotiation(negotiationId, organizationId, file);
+    return storageService.createForNegotiation(
+        NegotiatorUserDetailsService.getCurrentlyAuthenticatedUserInternalId(),
+        negotiationId,
+        organizationId,
+        file);
   }
 
   @GetMapping(

src/main/java/eu/bbmri_eric/negotiator/api/controller/v3/NegotiationController.java

@@ -91,6 +91,9 @@
   @ResponseStatus(HttpStatus.NO_CONTENT)
   NegotiationDTO update(
       @Valid @PathVariable String id, @Valid @RequestBody NegotiationCreateDTO request) {
+    if (!isCreator(negotiationService.findById(id, false))) {
+      throw new ResponseStatusException(HttpStatus.FORBIDDEN);
+    }
     return negotiationService.update(id, request);
   }
 

src/main/java/eu/bbmri_eric/negotiator/database/model/Negotiation.java

@@ -172,6 +172,11 @@ public int hashCode() {
     return Objects.hash(getId());
   }
 
+  /**
+   * Returns all resources involved in the negotiation.
+   *
+   * @return an UnmodifiableSet of resources
+   */
   public Set<Resource> getResources() {
     return requests.stream()
         .flatMap(request -> request.getResources().stream())

src/main/java/eu/bbmri_eric/negotiator/exceptions/EntityNotStorableException.java

@@ -2,5 +2,14 @@
 
 public class EntityNotStorableException extends RuntimeException {
 
-  private static final String errorMessage = "There was an error in the negotiation";
+  private static final String errorMessage =
+      "There was an error persisting the entity. Please try again later.";
+
+  public EntityNotStorableException() {
+    super(errorMessage);
+  }
+
+  public EntityNotStorableException(String message) {
+    super(message);
+  }
 }

src/main/java/eu/bbmri_eric/negotiator/exceptions/NegotiatorExceptionHandler.java

@@ -1,6 +1,5 @@
 package eu.bbmri_eric.negotiator.exceptions;
 
-import eu.bbmri_eric.negotiator.dto.error.ErrorResponse;
 import eu.bbmri_eric.negotiator.dto.negotiation.NegotiationFilters;
 import jakarta.servlet.ServletException;
 import jakarta.validation.ConstraintViolationException;
@@ -159,17 +158,26 @@ public final ResponseEntity<HttpErrorResponseModel> handleLazyInitializationExce
     ConstraintViolationException.class,
     MaxUploadSizeExceededException.class
   })
-  public final ResponseEntity<ErrorResponse> handleBadRequestExceptions(
+  public final ResponseEntity<HttpErrorResponseModel> handleBadRequestExceptions(
       RuntimeException ex, WebRequest request) {
-    ErrorResponse errorResponse =
-        new ErrorResponse(HttpStatus.BAD_REQUEST.value(), ex.getMessage());
+    HttpErrorResponseModel errorResponse =
+        HttpErrorResponseModel.builder()
+            .title("Bad request.")
+            .detail(ex.getMessage())
+            .status(HttpStatus.BAD_REQUEST.value())
+            .build();
     return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST);
   }
 
   @ExceptionHandler({ForbiddenRequestException.class})
-  public final ResponseEntity<ErrorResponse> handleForbiddenException(
+  public final ResponseEntity<HttpErrorResponseModel> handleForbiddenException(
       ForbiddenRequestException ex, WebRequest request) {
-    ErrorResponse errorResponse = new ErrorResponse(HttpStatus.FORBIDDEN.value(), ex.getMessage());
+    HttpErrorResponseModel errorResponse =
+        HttpErrorResponseModel.builder()
+            .title("Forbidden.")
+            .detail(ex.getMessage())
+            .status(HttpStatus.FORBIDDEN.value())
+            .build();
     return new ResponseEntity<>(errorResponse, HttpStatus.FORBIDDEN);
   }
 

src/main/java/eu/bbmri_eric/negotiator/service/AttachmentService.java

@@ -10,8 +10,17 @@ public interface AttachmentService {
 
   AttachmentMetadataDTO create(MultipartFile file);
 
+  /**
+   * Upload an attachment for a negotiation.
+   *
+   * @param userId The user id of the person uploading the attachment.
+   * @param negotiationId The negotiation id.
+   * @param organizationId The organization id.
+   * @param file The file to upload.
+   * @return The metadata of the uploaded attachment.
+   */
   AttachmentMetadataDTO createForNegotiation(
-      String negotiationId, @Nullable String organizationId, MultipartFile file);
+      Long userId, String negotiationId, @Nullable String organizationId, MultipartFile file);
 
   AttachmentMetadataDTO findMetadataById(String id);
 

src/main/java/eu/bbmri_eric/negotiator/service/DBAttachmentService.java

@@ -4,18 +4,20 @@
 import eu.bbmri_eric.negotiator.database.model.Attachment;
 import eu.bbmri_eric.negotiator.database.model.Negotiation;
 import eu.bbmri_eric.negotiator.database.model.Organization;
+import eu.bbmri_eric.negotiator.database.model.Person;
 import eu.bbmri_eric.negotiator.database.model.Resource;
 import eu.bbmri_eric.negotiator.database.repository.AttachmentRepository;
 import eu.bbmri_eric.negotiator.database.repository.NegotiationRepository;
 import eu.bbmri_eric.negotiator.database.repository.OrganizationRepository;
+import eu.bbmri_eric.negotiator.database.repository.PersonRepository;
 import eu.bbmri_eric.negotiator.dto.attachments.AttachmentDTO;
 import eu.bbmri_eric.negotiator.dto.attachments.AttachmentMetadataDTO;
 import eu.bbmri_eric.negotiator.exceptions.EntityNotFoundException;
+import eu.bbmri_eric.negotiator.exceptions.EntityNotStorableException;
 import eu.bbmri_eric.negotiator.exceptions.ForbiddenRequestException;
 import java.io.IOException;
 import java.util.List;
 import org.modelmapper.ModelMapper;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.lang.Nullable;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
@@ -24,49 +26,60 @@
 @Service(value = "DefaultAttachmentService")
 public class DBAttachmentService implements AttachmentService {
 
-  @Autowired private final AttachmentRepository attachmentRepository;
-  @Autowired private final ModelMapper modelMapper;
-  @Autowired private final NegotiationRepository negotiationRepository;
-  @Autowired private OrganizationRepository organizationRepository;
-  @Autowired private PersonService personService;
-  @Autowired private NegotiationService negotiationService;
+  private final AttachmentRepository attachmentRepository;
+  private final ModelMapper modelMapper;
+  private final NegotiationRepository negotiationRepository;
+  private OrganizationRepository organizationRepository;
+  private PersonService personService;
+  private NegotiationService negotiationService;
+  private PersonRepository personRepository;
 
-  @Autowired
   public DBAttachmentService(
       AttachmentRepository attachmentRepository,
       NegotiationRepository negotiationRepository,
       PersonService personService,
       NegotiationService negotiationService,
-      ModelMapper modelMapper) {
+      ModelMapper modelMapper,
+      PersonRepository personRepository) {
     this.attachmentRepository = attachmentRepository;
     this.negotiationRepository = negotiationRepository;
     this.modelMapper = modelMapper;
     this.personService = personService;
     this.negotiationService = negotiationService;
+    this.personRepository = personRepository;
   }
 
   @Override
   @Transactional
   public AttachmentMetadataDTO createForNegotiation(
-      String negotiationId, @Nullable String organizationId, MultipartFile file) {
+      Long userId, String negotiationId, @Nullable String organizationId, MultipartFile file) {
+    Negotiation negotiation = fetchNegotiation(negotiationId);
+    Organization organization = fetchAddressedOrganization(organizationId);
+    checkAuthorization(userId, negotiation);
+    return saveAttachment(file, negotiation, organization);
+  }
 
-    Attachment attachment;
-    Negotiation negotiation =
-        negotiationRepository
-            .findById(negotiationId)
-            .orElseThrow(() -> new EntityNotFoundException(negotiationId));
+  private Negotiation fetchNegotiation(String negotiationId) {
+    return negotiationRepository
+        .findById(negotiationId)
+        .orElseThrow(() -> new EntityNotFoundException(negotiationId));
+  }
 
-    Organization organization;
+  @Nullable
+  private Organization fetchAddressedOrganization(@Nullable String organizationId) {
+    Organization organization = null;
     if (organizationId != null) {
       organization =
           organizationRepository
               .findByExternalId(organizationId)
               .orElseThrow(() -> new EntityNotFoundException(organizationId));
-
-    } else {
-      organization = null;
     }
+    return organization;
+  }
 
+  private AttachmentMetadataDTO saveAttachment(
+      MultipartFile file, Negotiation negotiation, Organization organization) {
+    Attachment attachment;
     try {
       attachment =
           Attachment.builder()
@@ -81,28 +94,26 @@
       Attachment saved = attachmentRepository.save(attachment);
       return modelMapper.map(saved, AttachmentMetadataDTO.class);
     } catch (IOException e) {
-      throw new RuntimeException(e);
+      throw new EntityNotStorableException("The attachment could not be stored.");
+    }
+  }
+
+  private void checkAuthorization(Long userId, Negotiation negotiation) {
+    Person uploader =
+        personRepository.findById(userId).orElseThrow(() -> new EntityNotFoundException(userId));
+    if (!uploader.isAdmin()
+        && !uploader.equals(negotiation.getCreatedBy())
+        && negotiation.getResources().stream().noneMatch(uploader.getResources()::contains)) {
+      throw new ForbiddenRequestException(
+          "User %s is not authorized to upload attachments for this negotiation."
+              .formatted(userId));
     }
   }
 
   @Override
   @Transactional
   public AttachmentMetadataDTO create(MultipartFile file) {
-    Attachment attachment;
-    try {
-      attachment =
-          Attachment.builder()
-              .name(file.getOriginalFilename())
-              .payload(file.getBytes())
-              .contentType(file.getContentType())
-              .size(file.getSize())
-              .build();
-
-      Attachment saved = attachmentRepository.save(attachment);
-      return modelMapper.map(saved, AttachmentMetadataDTO.class);
-    } catch (IOException e) {
-      throw new RuntimeException(e);
-    }
+    return saveAttachment(file, null, null);
   }
 
   @Override

src/test/java/eu/bbmri_eric/negotiator/integration/api/v3/AttachmentControllerTests.java

@@ -217,14 +217,18 @@ public void testGetById_IsUnauthorized_whenBasicAuth() throws Exception {
         .andExpect(status().isUnauthorized());
   }
 
-  //  @Test
-  //  @WithUserDetails("researcher")
-  //  public void test_GetList_NoAttachmentsAreReturned() throws Exception {
-  //    mockMvc
-  //        .perform(MockMvcRequestBuilders.get(ENDPOINT))
-  //        .andExpect(status().isOk())
-  //        .andExpect(content().json("[]"));
-  //  }
+  @Test
+  @WithUserDetails("researcher")
+  void createAttachmentForNegotiation_notPartOfNegotiation_forbidden() throws Exception {
+    byte[] data = "Hello, World!".getBytes();
+    String fileName = "text.txt";
+    MockMultipartFile file =
+        new MockMultipartFile("file", fileName, MediaType.APPLICATION_OCTET_STREAM_VALUE, data);
+
+    mockMvc
+        .perform(multipart(WITH_NEGOTIATIONS_ENDPOINT).file(file))
+        .andExpect(status().isForbidden());
+  }
 
   @Test
   public void testGetList_IsUnauthorized_whenNoAuth() throws Exception {

src/test/java/eu/bbmri_eric/negotiator/integration/api/v3/NegotiationControllerTests.java

@@ -979,7 +979,7 @@ public void testUpdate_Unauthorized_whenWrongAuth() throws Exception {
   }
 
   @Test
-  @WithMockUser
+  @WithUserDetails("TheResearcher")
   public void testUpdate_BadRequest_whenRequestIsAlreadyAssignedToAnotherNegotiation()
       throws Exception {
     // It tries to update the known NEGOTIATION_2 by assigning the request of NEGOTIATION_1