TRELLO-2680 TRELLO-2901 split reportfiles endpoints for better security

app/controllers/ReportFileController.scala

@@ -23,7 +23,6 @@ import play.api.mvc.Action
 import play.api.mvc.AnyContent
 import play.api.mvc.ControllerComponents
 import play.api.mvc.MultipartFormData
-import repositories.report.ReportRepositoryInterface
 import utils.DateUtils.frenchFileFormatDate
 
 import java.io.File
@@ -37,21 +36,34 @@ class ReportFileController(
     visibleReportOrchestrator: VisibleReportOrchestrator,
     authenticator: Authenticator[User],
     signalConsoConfiguration: SignalConsoConfiguration,
-    controllerComponents: ControllerComponents,
-    reportRepository: ReportRepositoryInterface
+    controllerComponents: ControllerComponents
 )(implicit val ec: ExecutionContext)
     extends BaseController(authenticator, controllerComponents) {
 
   val logger: Logger = Logger(this.getClass)
 
   val reportFileMaxSizeInBytes = signalConsoConfiguration.reportFileMaxSize * 1024 * 1024
 
-  def downloadReportFile(uuid: ReportFileId, filename: String): Action[AnyContent] = Act.public.generousLimit.async {
-    _ =>
+  def legacyDownloadReportFile(uuid: ReportFileId, filename: String): Action[AnyContent] =
+    Act.public.generousLimit.async { _ =>
       reportFileOrchestrator
-        .downloadReportAttachment(uuid, filename)
+        .legacyDownloadReportAttachment(uuid, filename)
         .map(signedUrl => Redirect(signedUrl))
-  }
+    }
+
+  def downloadFileNotYetUsedInReport(uuid: ReportFileId, filename: String): Action[AnyContent] =
+    Act.public.generousLimit.async {
+      reportFileOrchestrator
+        .downloadFileNotYetUsedInReport(uuid, filename)
+        .map(signedUrl => Redirect(signedUrl))
+    }
+
+  def downloadFileUsedInReport(uuid: ReportFileId, filename: String): Action[AnyContent] =
+    Act.secured.all.allowImpersonation.async { request =>
+      reportFileOrchestrator
+        .downloadFileUsedInReport(uuid, filename, request.identity)
+        .map(signedUrl => Redirect(signedUrl))
+    }
 
   def retrieveReportFiles(): Action[JsValue] = Act.userAware.allowImpersonation.async(parse.json) { request =>
     // Validate the incoming JSON request body against the expected format
@@ -82,10 +94,24 @@ class ReportFileController(
         )
     }
 
-  def deleteReportFile(uuid: ReportFileId, filename: String): Action[AnyContent] =
+  def legacyDeleteReportFile(uuid: ReportFileId, filename: String): Action[AnyContent] =
     Act.userAware.forbidImpersonation.async { implicit request =>
       reportFileOrchestrator
-        .removeReportFile(uuid, filename, request.identity)
+        .legacyRemoveReportFile(uuid, filename, request.identity)
+        .map(_ => NoContent)
+    }
+
+  def deleteFileUsedInReport(fileId: ReportFileId, filename: String): Action[AnyContent] =
+    Act.secured.admins.async {
+      reportFileOrchestrator
+        .removeFileUsedInReport(fileId, filename)
+        .map(_ => NoContent)
+    }
+
+  def deleteFileNotYetUsedInReport(fileId: ReportFileId, filename: String): Action[AnyContent] =
+    Act.public.standardLimit.async {
+      reportFileOrchestrator
+        .removeFileNotYetUsedInReport(fileId, filename)
         .map(_ => NoContent)
     }
 

app/loader/SignalConsoApplicationLoader.scala

@@ -422,6 +422,7 @@ class SignalConsoComponents(
   val reportFileOrchestrator =
     new ReportFileOrchestrator(
       reportFileRepository,
+      visibleReportOrchestrator,
       antivirusScanActor,
       s3Service,
       reportZipExportService,
@@ -858,8 +859,7 @@ class SignalConsoComponents(
       visibleReportOrchestrator,
       cookieAuthenticator,
       signalConsoConfiguration,
-      controllerComponents,
-      reportRepository
+      controllerComponents
     )
 
   val healthController = new HealthController(cookieAuthenticator, controllerComponents)

app/orchestrators/ReportFileOrchestrator.scala

@@ -8,6 +8,7 @@ import cats.implicits.toFunctorOps
 import cats.implicits.toTraverseOps
 import cats.instances.future._
 import controllers.error.AppError
+import controllers.error.AppError.AttachmentNotFound
 import controllers.error.AppError._
 import models._
 import models.report._
@@ -34,9 +35,13 @@ import java.time.OffsetDateTime
 import java.util.UUID
 import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
+import scala.util.Try
+import scala.util.Failure
+import scala.util.Success
 
 class ReportFileOrchestrator(
     reportFileRepository: ReportFileRepositoryInterface,
+    visibleReportOrchestrator: VisibleReportOrchestrator,
     antivirusScanActor: ActorRef[AntivirusScanActor.ScanCommand],
     s3Service: S3ServiceInterface,
     reportZipExportService: ReportZipExportService,
@@ -110,14 +115,23 @@ class ReportFileOrchestrator(
       res                 <- reportFilesToDelete.map(file => remove(file.id, file.filename)).sequence
     } yield res
 
-  def removeReportFile(fileId: ReportFileId, filename: String, user: Option[User]): Future[Int] =
+  def removeFileUsedInReport(fileId: ReportFileId, filename: String): Future[_] =
     for {
-      maybeReportFile <- reportFileRepository
-        .get(fileId)
-        .ensure(AttachmentNotFound(reportFileId = fileId, reportFileName = filename))(predicate =
-          _.exists(_.filename == filename)
-        )
-      reportFile <- maybeReportFile.liftTo[Future](AttachmentNotFound(fileId, filename))
+      file <- getFileByIdAndName(fileId, filename)
+      _    <- Future.fromTry(checkIsUsedInReport(file))
+      _    <- remove(fileId, filename)
+    } yield ()
+
+  def removeFileNotYetUsedInReport(fileId: ReportFileId, filename: String): Future[_] =
+    for {
+      file <- getFileByIdAndName(fileId, filename)
+      _    <- Future.fromTry(checkIsNotYetUsedInReport(file))
+      _    <- remove(fileId, filename)
+    } yield ()
+
+  def legacyRemoveReportFile(fileId: ReportFileId, filename: String, user: Option[User]): Future[_] =
+    for {
+      reportFile <- getFileByIdAndName(fileId, filename)
       userHasDeleteFilePermission = user.exists(user => UserRole.Admins.contains(user.userRole))
       _ <- reportFile.reportId match {
         case Some(_) if userHasDeleteFilePermission => reportFileRepository.delete(fileId)
@@ -126,33 +140,44 @@ class ReportFileOrchestrator(
           Future.failed(CantPerformAction)
         case None => reportFileRepository.delete(fileId)
       }
-      res <- remove(fileId, filename)
-    } yield res
+      _ <- remove(fileId, filename)
+    } yield ()
 
-  private def remove(fileId: ReportFileId, filename: String): Future[Int] = for {
-    res <- reportFileRepository.delete(fileId)
-    _   <- s3Service.delete(filename)
-  } yield res
+  def legacyDownloadReportAttachment(reportFileId: ReportFileId, filename: String): Future[String] = {
+    logger.info(s"Downloading file with id $reportFileId")
+    for {
+      file <- getReportAttachmentOrRescan(reportFileId, filename)
+    } yield s3Service.getSignedUrl(file.storageFilename)
+  }
 
-  def downloadReportAttachment(reportFileId: ReportFileId, filename: String): Future[String] = {
+  def downloadFileNotYetUsedInReport(reportFileId: ReportFileId, filename: String): Future[String] = {
     logger.info(s"Downloading file with id $reportFileId")
-    getReportAttachmentOrRescan(reportFileId, filename).flatMap {
-      case Right(reportFile) => Future.successful(s3Service.getSignedUrl(reportFile.storageFilename))
-      case Left(error)       => Future.failed(error)
-    }
+    for {
+      file <- getReportAttachmentOrRescan(reportFileId, filename)
+      _    <- Future.fromTry(checkIsNotYetUsedInReport(file))
+    } yield s3Service.getSignedUrl(file.storageFilename)
   }
 
-  private def getReportAttachmentOrRescan(reportFileId: ReportFileId, filename: String) =
-    reportFileRepository
-      .get(reportFileId)
-      .flatMap {
-        case Some(reportFile) =>
-          validateAntivirusScanAndRescheduleScanIfNecessary(reportFile).map {
-            case (Scanned, file)    => Right(file)
-            case (NotScanned, file) => Left(AttachmentNotReady(file.id): AppError)
-          }
-        case _ => Future.successful(Left(AttachmentNotFound(reportFileId, filename)))
+  def downloadFileUsedInReport(reportFileId: ReportFileId, filename: String, user: User): Future[String] = {
+    logger.info(s"Downloading file with id $reportFileId")
+    for {
+      file     <- getReportAttachmentOrRescan(reportFileId, filename)
+      reportId <- Future.fromTry(checkIsUsedInReport(file))
+      _        <- visibleReportOrchestrator.checkReportIsVisible(reportId, user)
+    } yield s3Service.getSignedUrl(file.storageFilename)
+  }
+
+  private def getReportAttachmentOrRescan(reportFileId: ReportFileId, filename: String): Future[ReportFile] =
+    for {
+      maybeFile <- reportFileRepository
+        .get(reportFileId)
+      fileFound       <- maybeFile.liftTo[Future](AttachmentNotFound(reportFileId, filename))
+      antivirusResult <- validateAntivirusScanAndRescheduleScanIfNecessary(fileFound)
+      fileScanned = antivirusResult match {
+        case (Scanned, file)    => file
+        case (NotScanned, file) => throw AttachmentNotReady(file.id)
       }
+    } yield fileScanned
 
   private def validateAntivirusScanAndRescheduleScanIfNecessary(
       reportFile: ReportFile
@@ -205,6 +230,33 @@ class ReportFileOrchestrator(
       res <- reportZipExportService.reportAttachmentsZip(report.creationDate, filteredFilesByOrigin)
     } yield res
 
+  private def getFileByIdAndName(fileId: ReportFileId, filename: String) =
+    for {
+      maybeFile <- reportFileRepository.get(fileId)
+      file      <- maybeFile.filter(_.filename == filename).liftTo[Future](AttachmentNotFound(fileId, filename))
+    } yield file
+
+  private def remove(fileId: ReportFileId, filename: String): Future[Int] = for {
+    res <- reportFileRepository.delete(fileId)
+    _   <- s3Service.delete(filename)
+  } yield res
+
+  private def checkIsNotYetUsedInReport(file: ReportFile): Try[Unit] =
+    file.reportId match {
+      case None => Success(())
+      case Some(_) =>
+        logger.warn(s"Cannot act on file ${file.id} this way, because it IS linked to a report")
+        Failure(CantPerformAction)
+    }
+
+  private def checkIsUsedInReport(file: ReportFile): Try[UUID] =
+    file.reportId match {
+      case Some(reportId) => Success(reportId)
+      case None =>
+        logger.warn(s"Cannot act on file ${file.id} this way, because it is NOT linked to a report")
+        Failure(CantPerformAction)
+    }
+
 }
 
 object ReportFileOrchestrator {

app/services/ExcelColumnsService.scala

@@ -110,7 +110,7 @@ object ExcelColumnsService {
             .filter(file => file.origin == ReportFileOrigin.Consumer && shouldBeVisibleToUser(userRole, report))
             .map(file =>
               s"${signalConsoConfiguration.apiURL.toString}${routes.ReportFileController
-                  .downloadReportFile(file.id, file.filename)
+                  .downloadFileUsedInReport(file.id, file.filename)
                   .url}"
             )
             .mkString("\n")

conf/routes

@@ -29,10 +29,14 @@ POST          /api/email-validation/check-and-validate                controller
 POST          /api/rating                                             controllers.RatingController.rateBlockingInfoInSubcategoryUsefulness()
 # egalement utilisé dans le dashboard
 GET           /api/constants/countries                                controllers.ConstantController.getCountries()
-# for direct download of a single attachment
-# egalement utilisé dans le dashboard
-GET           /api/reports/files/:uuid/:filename                      controllers.ReportFileController.downloadReportFile(uuid: ReportFileId, filename)
-
+# for direct viewing of a single attachment
+# also used in the dashboard for the pro when he's viewing the attachment of his response that is not yet submitted
+GET           /api/reports/files/temporary/:fileId/:filename          controllers.ReportFileController.downloadFileNotYetUsedInReport(fileId: ReportFileId, filename)
+# for deleting an attachment that was uploaded but the report isn't submitted yet
+# also used in the dashboard for the pro when his response isn't submitted yet
+DELETE        /api/reports/files/temporary/:fileId/:filename          controllers.ReportFileController.deleteFileNotYetUsedInReport(fileId: ReportFileId, filename)
+# this next one is LEGACY, to be removed very soon, once the frontend is updated (was used both in website and dashboard)
+GET           /api/reports/files/:uuid/:filename                      controllers.ReportFileController.legacyDownloadReportFile(uuid: ReportFileId, filename)
 
 ###########################################
 ########## REPORTS SEARCH/LIST PAGE #######
@@ -74,10 +78,14 @@ POST          /api/reports/:uuid/assign/:userId                       controller
 ########## FOR VIEWING/EDITING ATTACHMENTS OF AN EXISTING REPORT #######
 ########################################################################
 
+# for viewing a single attachment (and the thumbnails)
+GET           /api/reports/files/used/:fileId/:filename               controllers.ReportFileController.downloadFileUsedInReport(fileId: ReportFileId, filename)
 # button "download all attachments"
 GET           /api/reports/:uuid/files                                controllers.ReportFileController.downloadAllFilesAsZip(uuid: java.util.UUID, origin: Option[ReportFileOrigin])
 # for deletion of a single attachment
-DELETE        /api/reports/files/:uuid/:filename                      controllers.ReportFileController.deleteReportFile(uuid: ReportFileId, filename)
+DELETE        /api/reports/files/used/:fileId/:filename               controllers.ReportFileController.deleteFileUsedInReport(fileId: ReportFileId, filename)
+# This next one is LEGACY, to be deleted very shortly once we do the change in the frontend
+DELETE        /api/reports/files/:uuid/:filename                      controllers.ReportFileController.legacyDeleteReportFile(uuid: ReportFileId, filename)
 # for adding a single attachment (for both pro and admin)
 POST          /api/reports/files                                      controllers.ReportFileController.uploadReportFile(reportFileId: Option[java.util.UUID])
 # something related to the antivirus scan of the files (but I couldn't find how to trigger it)