diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs new file mode 100644 index 00000000..fb495459 --- /dev/null +++ b/.git-blame-ignore-revs @@ -0,0 +1,4 @@ +# .git-blame-ignore-revs + +# scalafmt mass change +3487e7a60194a66886572c899a6a93d7355dc376 diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 00000000..3cf273cb --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1 @@ +* @broadinstitute/dsp-core-services diff --git a/.github/workflows/format.yaml b/.github/workflows/format.yaml new file mode 100644 index 00000000..026d95e8 --- /dev/null +++ b/.github/workflows/format.yaml @@ -0,0 +1,28 @@ +name: Check formatting for modified files with scalafmt + +on: + pull_request: + paths-ignore: ['**.md'] + +jobs: + format: + + runs-on: ubuntu-latest + + steps: + + - uses: actions/checkout@v4 + with: + fetch-depth: 2 + ref: ${{ github.event.pull_request.head.sha }} + + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + distribution: 'temurin' + java-version: 17 + cache: sbt + + - name: Check formatting for modified files + run: | + sbt scalafmtCheckAll diff --git a/.scalafmt.conf b/.scalafmt.conf index 7aea47e7..c2a939b9 100644 --- a/.scalafmt.conf +++ b/.scalafmt.conf @@ -1,15 +1,17 @@ -version = 2.4.2 +version = 3.8.3 align = none align.openParenCallSite = true align.openParenDefnSite = true maxColumn = 120 continuationIndent.defnSite = 2 assumeStandardLibraryStripMargin = true -danglingParentheses = true +danglingParentheses.preset = true rewrite.rules = [SortImports, RedundantBraces, RedundantParens, SortModifiers] -docstrings = JavaDoc +docstrings.style = keep project.excludeFilters = [ Dependencies.scala, Settings.scala, build.sbt ] +runner.dialect = scala213 +project.git = true diff --git a/build.sbt b/build.sbt index 00fb0d51..579ee45c 100644 --- a/build.sbt +++ b/build.sbt @@ -6,23 +6,28 @@ version := "0.2" organization := "org.broadinstitute" -scalaVersion := "2.13.10" +scalaVersion := "2.13.15" val akkaV = "2.6.18" val akkaHttpV = "10.2.7" val slickV = "3.3.3" -val workbenchGoogleV = "0.28-3ad3700" -val scalaTestV = "3.2.11" + +val scalaTestV = "3.2.19" + +val workbenchLibsHash = "9254729" +val workbenchGoogleV = s"0.32-$workbenchLibsHash" +val workbenchNotificationsV = s"0.7-$workbenchLibsHash" + resolvers ++= Seq( "Broad Artifactory Releases" at "https://broadinstitute.jfrog.io/broadinstitute/libs-release/", "Broad Artifactory Snapshots" at "https://broadinstitute.jfrog.io/broadinstitute/libs-snapshot/") libraryDependencies ++= Seq( - "org.webjars" % "swagger-ui" % "4.1.3", + "org.webjars" % "swagger-ui" % "5.17.14", "com.typesafe.scala-logging" %% "scala-logging" % "3.9.5", "com.typesafe.akka" %% "akka-http-spray-json" % "10.2.9", - "com.google.protobuf" % "protobuf-java" % "4.0.0-rc-2", + "com.google.protobuf" % "protobuf-java" % "4.29.0-RC1", "io.sentry" % "sentry" % "6.9.2", "io.sentry" % "sentry-logback" % "6.9.2", "org.broadinstitute.dsde.workbench" %% "workbench-google" % workbenchGoogleV @@ -32,7 +37,7 @@ libraryDependencies ++= Seq( exclude("org.bouncycastle", "bcprov-ext-jdk15on") exclude("org.bouncycastle", "bcutil-jdk15on") exclude("org.bouncycastle", "bcpkix-jdk15on"), - "org.broadinstitute.dsde.workbench" %% "workbench-notifications" % "0.7-9254729" + "org.broadinstitute.dsde.workbench" %% "workbench-notifications" % workbenchNotificationsV exclude("com.typesafe.akka", "akka-protobuf-v3_2.13") exclude("com.google.protobuf", "protobuf-java"), "com.typesafe.akka" %% "akka-http" % akkaHttpV, @@ -46,19 +51,19 @@ libraryDependencies ++= Seq( "commons-io" % "commons-io" % "2.17.0", "commons-codec" % "commons-codec" % "1.15", "mysql" % "mysql-connector-java" % "8.0.28", - "org.liquibase" % "liquibase-core" % "4.7.1", - "org.hsqldb" % "hsqldb" % "2.6.1", + "org.liquibase" % "liquibase-core" % "4.30.0", + "org.hsqldb" % "hsqldb" % "2.7.4", "com.sendgrid" % "sendgrid-java" % "2.2.2", - "ch.qos.logback" % "logback-classic" % "1.4.14", + "ch.qos.logback" % "logback-classic" % "1.5.12", "org.broadinstitute.dsde.workbench" %% "sam-client" % "0.1-4cde1ff", "com.azure" % "azure-identity" % "1.12.2", - "com.azure" % "azure-core-management" % "1.15.0", + "com.azure" % "azure-core-management" % "1.15.5", //---------- Test libraries -------------------// "org.broadinstitute.dsde.workbench" %% "workbench-google" % workbenchGoogleV % Test classifier "tests", "com.typesafe.akka" %% "akka-testkit" % akkaV % Test, "com.typesafe.akka" %% "akka-http-testkit" % akkaHttpV % Test, "org.scalatest" %% "scalatest" % scalaTestV % Test, - "org.mockito" %% "mockito-scala-scalatest" % "1.17.12" % Test, + "org.mockito" %% "mockito-scala-scalatest" % "1.17.37" % Test, "org.yaml" % "snakeyaml" % "1.33" % Test ) diff --git a/docker/build.sh b/docker/build.sh index c0e2e920..44906260 100755 --- a/docker/build.sh +++ b/docker/build.sh @@ -89,7 +89,7 @@ fi function make_jar() { echo "building thurloe jar..." - docker run --rm -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:openjdk-17.0.2_1.7.2_2.13.10 /working/docker/install.sh /working + docker run --rm -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-17.0.13_11_1.10.5_2.13.15 /working/docker/install.sh /working } function docker_cmd() diff --git a/docker/build_jar.sh b/docker/build_jar.sh index c4d80b5f..3456ec8b 100755 --- a/docker/build_jar.sh +++ b/docker/build_jar.sh @@ -10,7 +10,7 @@ echo "building thurloe jar..." docker run --rm -v $PWD:/working \ -v jar-cache:/root/.ivy \ --v jar-cache:/root/.ivy2 sbtscala/scala-sbt:openjdk-17.0.2_1.7.2_2.13.10 /working/docker/install.sh /working +-v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-17.0.13_11_1.10.5_2.13.15 /working/docker/install.sh /working EXIT_CODE=$? diff --git a/project/build.properties b/project/build.properties index c8fcab54..db1723b0 100644 --- a/project/build.properties +++ b/project/build.properties @@ -1 +1 @@ -sbt.version=1.6.2 +sbt.version=1.10.5 diff --git a/project/plugins.sbt b/project/plugins.sbt index fcaf5eca..ba86fd3b 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -1,13 +1,13 @@ -addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "1.1.1") +addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "2.3.0") -addSbtPlugin("org.scoverage" % "sbt-scoverage" % "2.0.5") +addSbtPlugin("org.scoverage" % "sbt-scoverage" % "2.2.2") -addSbtPlugin("org.scalameta" % "sbt-scalafmt" % "2.4.6") +addSbtPlugin("org.scalameta" % "sbt-scalafmt" % "2.5.2") addSbtPlugin( - "com.github.cb372" % "sbt-explicit-dependencies" % "0.2.16" + "com.github.cb372" % "sbt-explicit-dependencies" % "0.3.1" ) // Use `unusedCompileDependencies` to see unused dependencies -addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.9.34") +addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.13.0") -addDependencyTreePlugin \ No newline at end of file +addDependencyTreePlugin diff --git a/src/main/resources/application.conf b/src/main/resources/application.conf index 4a9a9a86..bcd96932 100644 --- a/src/main/resources/application.conf +++ b/src/main/resources/application.conf @@ -9,6 +9,10 @@ akka.http { } } +auth { + googleClientId = "" +} + swagger { docsPath = "swagger/thurloe.yaml" uiVersion = "2.1.1" diff --git a/src/main/scala/thurloe/Main.scala b/src/main/scala/thurloe/Main.scala index ba1265a2..b798f987 100644 --- a/src/main/scala/thurloe/Main.scala +++ b/src/main/scala/thurloe/Main.scala @@ -41,10 +41,9 @@ object Main extends App { val routes = new ThurloeServiceActor(samDao) for { - binding <- Http().newServerAt("0.0.0.0", 8000).bind(routes.route).recover { - case t: Throwable => - system.log.error("FATAL - failure starting http server", t) - throw t + binding <- Http().newServerAt("0.0.0.0", 8000).bind(routes.route).recover { case t: Throwable => + system.log.error("FATAL - failure starting http server", t) + throw t } _ = system.log.info("Thurloe now available for all your key/value pair and notification needs.") _ <- binding.whenTerminated @@ -56,7 +55,8 @@ object Main extends App { val pem = GoogleCredentialModes.Pem(WorkbenchEmail(gcsConfig.getString("clientEmail")), - new File(gcsConfig.getString("pathToPem"))) + new File(gcsConfig.getString("pathToPem")) + ) val pubSubDAO = new HttpGooglePubSubDAO( gcsConfig.getString("appName"), pem, diff --git a/src/main/scala/thurloe/crypto/Encryption.scala b/src/main/scala/thurloe/crypto/Encryption.scala index 84c7dca6..2465186b 100644 --- a/src/main/scala/thurloe/crypto/Encryption.scala +++ b/src/main/scala/thurloe/crypto/Encryption.scala @@ -52,10 +52,12 @@ case object Aes256Cbc { } final def decrypt(encryptedBytes: EncryptedBytes, secretKey: SecretKey): Try[Array[Byte]] = - validateLength("Secret key", secretKey.key, keySize) validateAnotherLength ("Initialization vector", encryptedBytes.iv, blockSize) map { - _ => - val cipher = init(Cipher.DECRYPT_MODE, secretKey.key, encryptedBytes.iv) - cipher.doFinal(encryptedBytes.cipherText) + validateLength("Secret key", secretKey.key, keySize) validateAnotherLength ("Initialization vector", + encryptedBytes.iv, + blockSize + ) map { _ => + val cipher = init(Cipher.DECRYPT_MODE, secretKey.key, encryptedBytes.iv) + cipher.doFinal(encryptedBytes.cipherText) } } diff --git a/src/main/scala/thurloe/dataaccess/HttpSamDAO.scala b/src/main/scala/thurloe/dataaccess/HttpSamDAO.scala index 89a196cd..68b659ca 100644 --- a/src/main/scala/thurloe/dataaccess/HttpSamDAO.scala +++ b/src/main/scala/thurloe/dataaccess/HttpSamDAO.scala @@ -37,9 +37,9 @@ class HttpSamDAO(config: Config, cloudServiceAuthTokenProvider: CloudServiceAuth protected def adminApi(samApiClient: ApiClient) = new AdminApi(samApiClient) override def getUserById(userId: String): List[sam.model.User] = - try { + try adminApi(getApiClient).adminGetUsersByQuery(userId, userId, userId, 5).asScala.toList - } catch { + catch { case e: Exception => logger.warn(s"Sam user not found: $userId", e) List.empty diff --git a/src/main/scala/thurloe/dataaccess/HttpSendGridDAO.scala b/src/main/scala/thurloe/dataaccess/HttpSendGridDAO.scala index 2f4b5a74..3ff7d250 100644 --- a/src/main/scala/thurloe/dataaccess/HttpSendGridDAO.scala +++ b/src/main/scala/thurloe/dataaccess/HttpSendGridDAO.scala @@ -32,7 +32,7 @@ class HttpSendGridDAO(samDao: SamDAO) extends SendGridDAO with LazyLogging { } } - //Looks up a KVP, converting empty values or missing KVPs into None + // Looks up a KVP, converting empty values or missing KVPs into None private def lookupNonEmptyKeyValuePair(userId: String, key: String) = dataAccess .lookup(samDao, userId, key) @@ -42,7 +42,7 @@ class HttpSendGridDAO(samDao: SamDAO) extends SendGridDAO with LazyLogging { } .recover(_ => None) - //There are two cases that need to be handled when looking up the preferred contact email: + // There are two cases that need to be handled when looking up the preferred contact email: // 1) If the contactEmail is not present at all, the DB query will throw an exception. So that needs to be handled. // 2) If the contactEmail is present but blank, it also needs to be ignored. Thurloe accepts arbitrary key/value pairs // and makes no guarantees about what the data might look like, so a blank value or invalid email is a valid case. @@ -54,12 +54,13 @@ class HttpSendGridDAO(samDao: SamDAO) extends SendGridDAO with LazyLogging { // how profiles are populated. def lookupPreferredEmail(userId: WorkbenchUserId): Future[WorkbenchEmail] = lookupNonEmptyKeyValuePair(userId.value, "contactEmail") flatMap { - case Some(kvp) => Future.successful(WorkbenchEmail(kvp.keyValuePair.value)) //contactEmail was found and non-empty + case Some(kvp) => + Future.successful(WorkbenchEmail(kvp.keyValuePair.value)) // contactEmail was found and non-empty case None => logger.info(s"Failed to get stored contactEmail for ${userId.value}. Defaulting to account email for user.") lookupNonEmptyKeyValuePair(userId.value, "email") flatMap { case Some(kvp) => - Future.successful(WorkbenchEmail(kvp.keyValuePair.value)) //account email was found and non-empty + Future.successful(WorkbenchEmail(kvp.keyValuePair.value)) // account email was found and non-empty case None => Future.failed(new KeyNotFoundException(userId.value, "email")) } } diff --git a/src/main/scala/thurloe/dataaccess/SendGridDAO.scala b/src/main/scala/thurloe/dataaccess/SendGridDAO.scala index 2530a07b..b514755b 100644 --- a/src/main/scala/thurloe/dataaccess/SendGridDAO.scala +++ b/src/main/scala/thurloe/dataaccess/SendGridDAO.scala @@ -36,7 +36,8 @@ trait SendGridDAO { new NotificationException(StatusCodes.BadRequest, "No recipient specified", Seq.empty, - notification.notificationId) + notification.notificationId + ) ) ) ) @@ -45,12 +46,12 @@ trait SendGridDAO { Future.traverse(_)(lookupPreferredEmail).map(replyToEmails => Option(replyToEmails)) } getOrElse Future.successful(None) - val emailSubstitutionsFuture = Future.traverse(notification.emailLookupSubstitutions.toList) { - case (key, id) => lookupPreferredEmail(id).map(email => key -> email.value) + val emailSubstitutionsFuture = Future.traverse(notification.emailLookupSubstitutions.toList) { case (key, id) => + lookupPreferredEmail(id).map(email => key -> email.value) } - val nameSubstitutionsFuture = Future.traverse(notification.nameLookupSubstitution.toList) { - case (key, id) => lookupUserName(id).map(name => key -> name) + val nameSubstitutionsFuture = Future.traverse(notification.nameLookupSubstitution.toList) { case (key, id) => + lookupUserName(id).map(name => key -> name) } val recipientFirstNameSubstitutionFuture = notification.userId match { @@ -82,7 +83,8 @@ trait SendGridDAO { def createEmail(toAddress: WorkbenchEmail, replyTos: Option[Set[WorkbenchEmail]], notificationId: String, - substitutions: Map[String, String] = Map.empty): SendGrid.Email = { + substitutions: Map[String, String] = Map.empty + ): SendGrid.Email = { val email = new SendGrid.Email() email.addTo(toAddress.value) @@ -115,8 +117,8 @@ trait SendGridDAO { case class NotificationException(statusCode: StatusCode, message: String, recipients: Seq[String], - notificationId: String) - extends Exception { + notificationId: String +) extends Exception { override def getMessage = s"Error message: [${message}], recipients: [${recipients.mkString(",")}], notificationId: [${notificationId}]" } diff --git a/src/main/scala/thurloe/dataaccess/auth/CloudServiceAuthTokenProvider.scala b/src/main/scala/thurloe/dataaccess/auth/CloudServiceAuthTokenProvider.scala index 2c3b10ba..6bc190a6 100644 --- a/src/main/scala/thurloe/dataaccess/auth/CloudServiceAuthTokenProvider.scala +++ b/src/main/scala/thurloe/dataaccess/auth/CloudServiceAuthTokenProvider.scala @@ -24,7 +24,8 @@ object CloudServiceAuthTokenProvider { val gcsConfig = config.getConfig("gcs") val pem = GoogleCredentialModes.Pem(WorkbenchEmail(gcsConfig.getString("clientEmail")), - new File(gcsConfig.getString("pathToPem"))) + new File(gcsConfig.getString("pathToPem")) + ) new GcpAuthTokenProvider(pem) } diff --git a/src/main/scala/thurloe/database/ThurloeDatabaseConnector.scala b/src/main/scala/thurloe/database/ThurloeDatabaseConnector.scala index b70a3f0d..9442d7f8 100644 --- a/src/main/scala/thurloe/database/ThurloeDatabaseConnector.scala +++ b/src/main/scala/thurloe/database/ThurloeDatabaseConnector.scala @@ -48,15 +48,15 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { userId: String, key: String, value: String, - iv: String): Try[UserKeyValuePairWithId] = + iv: String + ): Try[UserKeyValuePairWithId] = Aes256Cbc.decrypt(EncryptedBytes(value, iv), secretKey) map { decryptedBytes => UserKeyValuePairWithId(id, UserKeyValuePair(userId, KeyValuePair(key, new String(decryptedBytes, "UTF-8")))) } private def interpretDatabaseResponse(resultSequence: Seq[DatabaseRow]): Seq[Future[UserKeyValuePairWithId]] = - resultSequence map { - case DatabaseRow(id, userId, key, value, iv) => - Future.fromTry(databaseValuesToUserKeyValuePair(id, userId, key, value, iv)) + resultSequence map { case DatabaseRow(id, userId, key, value, iv) => + Future.fromTry(databaseValuesToUserKeyValuePair(id, userId, key, value, iv)) } /** @@ -81,7 +81,7 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { } else { // If we get back multiple results, we assume the record with the b2c id is the most recent and return that one. results - // The user record is a java obj so we need to handle nulls properly + // The user record is a java obj so we need to handle nulls properly .find(samUserRecord => Option(samUserRecord.getAzureB2CId).isDefined) .map(Future.successful) .getOrElse( @@ -89,7 +89,7 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { InvalidDatabaseStateException( s"Too many results returned from sam, none of which have an AzureB2cId: ${results.size}." + s"\nResults: ${results - .map(samRecord => s"GoogleSubjectId: ${samRecord.getGoogleSubjectId}, AzureB2cId: ${samRecord.getAzureB2CId}, SamId: ${samRecord.getId}")}" + + .map(samRecord => s"GoogleSubjectId: ${samRecord.getGoogleSubjectId}, AzureB2cId: ${samRecord.getAzureB2CId}, SamId: ${samRecord.getId}")}" + s"\nQuery: $userId" ) ) @@ -97,16 +97,15 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { } // Fallback on the userId if we can't find the user in sam, this vastly simplifies the logic in the rest of the code - samUser.recoverWith({ - case _: Exception => - logger.warn(s"Unable to find user in sam, falling back on userId: $userId") - val dummySamUser = new sam.model.User() - dummySamUser.setGoogleSubjectId(userId) - dummySamUser.setAzureB2CId(userId) - dummySamUser.setId(userId) - - Future.successful(dummySamUser) - }) + samUser.recoverWith { case _: Exception => + logger.warn(s"Unable to find user in sam, falling back on userId: $userId") + val dummySamUser = new sam.model.User() + dummySamUser.setGoogleSubjectId(userId) + dummySamUser.setAzureB2CId(userId) + dummySamUser.setId(userId) + + Future.successful(dummySamUser) + } } private def lookupWithConstraint(constraint: DbKeyValuePair => Rep[Boolean]): Future[Seq[UserKeyValuePairWithId]] = { @@ -114,8 +113,8 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { for { responseSequence <- database.run(query.result.transactionally) - result <- Future.sequence(interpretDatabaseResponse(responseSequence map { - case (id, userId, key, value, iv) => DatabaseRow(id, userId, key, value, iv) + result <- Future.sequence(interpretDatabaseResponse(responseSequence map { case (id, userId, key, value, iv) => + DatabaseRow(id, userId, key, value, iv) })) } yield result } @@ -131,13 +130,14 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { results <- lookupWithConstraint(thurloeRecord => thurloeRecord.key === key && (thurloeRecord.userId === samUser.getId || thurloeRecord.userId === samUser.getGoogleSubjectId || thurloeRecord.userId === samUser.getAzureB2CId) ) - result <- if (results.isEmpty) { - Future.failed(KeyNotFoundException(userId, key)) - } else if (results.size == 1) { - Future.successful(results.head) - } else { - Future.successful(handleConflictingKeys(results)) - } + result <- + if (results.isEmpty) { + Future.failed(KeyNotFoundException(userId, key)) + } else if (results.size == 1) { + Future.successful(results.head) + } else { + Future.successful(handleConflictingKeys(results)) + } } yield result.copy(userKeyValuePair = result.userKeyValuePair.copy(userId = userId)) /* @@ -240,7 +240,8 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { * @return The type of operation which was carried out (as a Future) */ private def databaseWrite(userKeyValuePair: UserKeyValuePair, - encryptedValue: EncryptedBytes): Future[DatabaseOperation] = { + encryptedValue: EncryptedBytes + ): Future[DatabaseOperation] = { val lookupExists = lookupWithConstraint(thurloeRecord => thurloeRecord.key === userKeyValuePair.keyValuePair.key && thurloeRecord.userId === userKeyValuePair.userId ) @@ -262,7 +263,7 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { userKeyValuePair.keyValuePair.key, encryptedValue.base64CipherText, encryptedValue.base64Iv - ) + ) for { affectedRowsCount <- database.run(action.transactionally) @@ -272,10 +273,11 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { private def update(oldKeyValuePair: UserKeyValuePairWithId, userKeyValuePair: UserKeyValuePair, - newEncryptedValue: EncryptedBytes): Future[DatabaseOperation] = + newEncryptedValue: EncryptedBytes + ): Future[DatabaseOperation] = // We've just looked up and found an entry, so this ID should never be None. However, belt and braces... oldKeyValuePair.id match { - case None => Future.failed(new KeyNotFoundException(userKeyValuePair.userId, userKeyValuePair.keyValuePair.key)) + case None => Future.failed(new KeyNotFoundException(userKeyValuePair.userId, userKeyValuePair.keyValuePair.key)) case Some(rowId) => // NB: Using sqlu"..." strings does clever DB magic to turn this into a proper parameterised DB command to avoid insertion attacks. def sqlUpdateCommand: DBIO[Int] = @@ -318,11 +320,12 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { for { affectedRowCount <- affectedRowsCountFuture - _ <- if (affectedRowCount > 0) { - Future.successful(()) - } else { - Future.failed(KeyNotFoundException(userId, key)) - } + _ <- + if (affectedRowCount > 0) { + Future.successful(()) + } else { + Future.failed(KeyNotFoundException(userId, key)) + } } yield () } @@ -361,9 +364,8 @@ case object ThurloeDatabaseConnector extends DataAccess with LazyLogging { } } throw e - } finally { + } finally dbConnection.close() - } } } } diff --git a/src/main/scala/thurloe/notification/NotificationMonitor.scala b/src/main/scala/thurloe/notification/NotificationMonitor.scala index ba188e25..2609a88c 100644 --- a/src/main/scala/thurloe/notification/NotificationMonitor.scala +++ b/src/main/scala/thurloe/notification/NotificationMonitor.scala @@ -36,18 +36,21 @@ object NotificationMonitorSupervisor { sendGridDAO: SendGridDAO, templateIdsByType: Map[String, String], fireCloudPortalUrl: String, - samDao: SamDAO)(implicit executionContext: ExecutionContext): Props = + samDao: SamDAO + )(implicit executionContext: ExecutionContext): Props = Props( - new NotificationMonitorSupervisor(pollInterval, - pollIntervalJitter, - pubSubDao, - pubSubTopicName, - pubSubSubscriptionName, - workerCount, - sendGridDAO, - templateIdsByType, - fireCloudPortalUrl, - samDao: SamDAO) + new NotificationMonitorSupervisor( + pollInterval, + pollIntervalJitter, + pubSubDao, + pubSubTopicName, + pubSubSubscriptionName, + workerCount, + sendGridDAO, + templateIdsByType, + fireCloudPortalUrl, + samDao: SamDAO + ) ) } @@ -98,13 +101,11 @@ class NotificationMonitorSupervisor( } override val supervisorStrategy = - OneForOneStrategy() { - case e => { - logger.error("error sending notification", e) - // start one to replace the error, stop the errored child so that we also drop its mailbox (i.e. restart not good enough) - startOne() - Stop - } + OneForOneStrategy() { case e => + logger.error("error sending notification", e) + // start one to replace the error, stop the errored child so that we also drop its mailbox (i.e. restart not good enough) + startOne() + Stop } } @@ -120,7 +121,8 @@ object NotificationMonitor { templateIdsByType: Map[String, String], fireCloudPortalUrl: String, dataAccess: DataAccess, - samDao: SamDAO)(implicit executionContext: ExecutionContext): Props = + samDao: SamDAO + )(implicit executionContext: ExecutionContext): Props = Props( new NotificationMonitorActor(pollInterval, pollIntervalJitter, @@ -130,7 +132,8 @@ object NotificationMonitor { templateIdsByType, fireCloudPortalUrl, dataAccess, - samDao: SamDAO) + samDao: SamDAO + ) ) val notificationsOffKey = "notifications.off" @@ -144,7 +147,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, templateIdsByType: Map[String, String], fireCloudPortalUrl: String, dataAccess: DataAccess, - samDao: SamDAO)(implicit executionContext: ExecutionContext) + samDao: SamDAO +)(implicit executionContext: ExecutionContext) extends Actor with LazyLogging { @@ -212,7 +216,7 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, // shouldSendEmails is a per-environment flag to turn off all email sending if (!shouldSendEmails) return Future.successful(false) notification match { - //For workspace notifications, there are three tiers of preferences to check: + // For workspace notifications, there are three tiers of preferences to check: // 1. has the user disabled *all* notifications for their account? // key format: notifications.off // 2. has the user disabled the notification at the type-level? @@ -272,7 +276,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, Map("accessLevel" -> accessLevel, "namespace" -> workspaceName.namespace, "name" -> workspaceName.name, - "wsUrl" -> workspacePortalUrl(workspaceName)), + "wsUrl" -> workspacePortalUrl(workspaceName) + ), Map("originEmail" -> workspaceOwnerId), Map("userNameFL" -> workspaceOwnerId) ) @@ -286,7 +291,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, Map("wsName" -> workspaceName.name, "wsUrl" -> workspacePortalUrl(workspaceName), "bucketName" -> bucketName, - "bucketUrl" -> bucketUrl(bucketName)), + "bucketUrl" -> bucketUrl(bucketName) + ), Map("originEmail" -> requesterId), Map("userNameFL" -> requesterId) ) @@ -298,7 +304,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, Option(Set(requesterId)), templateId, Map("billingProjectName" -> billingProjectName, - "billingProjectNameUrl" -> billingProjectUrl(billingProjectName)), + "billingProjectNameUrl" -> billingProjectUrl(billingProjectName) + ), Map("originEmail" -> requesterId), Map("userNameFL" -> requesterId) ) @@ -312,7 +319,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, Map("accessLevel" -> accessLevel, "namespace" -> workspaceName.namespace, "name" -> workspaceName.name, - "wsUrl" -> workspacePortalUrl(workspaceName)), + "wsUrl" -> workspacePortalUrl(workspaceName) + ), Map("originEmail" -> workspaceOwnerId), Map("userNameFL" -> workspaceOwnerId) ) @@ -324,7 +332,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, workflowConfiguration, dataEntity, workflowCount, - comment) => + comment + ) => thurloe.service.Notification( Option(recipientUserid), None, @@ -353,7 +362,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, workflowConfiguration, dataEntity, workflowCount, - comment) => + comment + ) => thurloe.service.Notification( Option(recipientUserid), None, @@ -382,7 +392,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, workflowConfiguration, dataEntity, workflowCount, - comment) => + comment + ) => thurloe.service.Notification( Option(recipientUserid), None, @@ -411,7 +422,8 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, templateId, Map("wsName" -> workspaceName.name, "wsUrl" -> workspacePortalUrl(workspaceName)), Map.empty, - Map.empty) + Map.empty + ) case GroupAccessRequestNotification(recipientUserId, groupName, replyTos, requesterId) => thurloe.service.Notification( @@ -442,9 +454,7 @@ class NotificationMonitorActor(val pollInterval: FiniteDuration, } override val supervisorStrategy = - OneForOneStrategy() { - case _ => { - Escalate - } + OneForOneStrategy() { case _ => + Escalate } } diff --git a/src/main/scala/thurloe/security/CSPDirective.scala b/src/main/scala/thurloe/security/CSPDirective.scala new file mode 100644 index 00000000..0a22cfca --- /dev/null +++ b/src/main/scala/thurloe/security/CSPDirective.scala @@ -0,0 +1,16 @@ +package thurloe.security + +import akka.http.scaladsl.model.headers.RawHeader +import akka.http.scaladsl.server.Directives._ +import akka.http.scaladsl.server.Route + +object CSPDirective { + private val cspHeader = RawHeader( + "Content-Security-Policy", + "default-src 'self'; script-src 'self' 'unsafe-inline'; img-src 'self' data:; style-src 'self' 'unsafe-inline'; connect-src 'self'; form-action 'none';" + ) + + def addCSP(route: Route): Route = respondWithHeader(cspHeader) { + route + } +} diff --git a/src/main/scala/thurloe/service/ApiDataModels.scala b/src/main/scala/thurloe/service/ApiDataModels.scala index 72583877..2b26a8bb 100644 --- a/src/main/scala/thurloe/service/ApiDataModels.scala +++ b/src/main/scala/thurloe/service/ApiDataModels.scala @@ -5,13 +5,13 @@ import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport.{ WorkbenchEmailFormat, WorkbenchUserIdFormat } -import spray.json.DefaultJsonProtocol +import spray.json.{DefaultJsonProtocol, RootJsonFormat} object ApiDataModelsJsonProtocol extends DefaultJsonProtocol { - implicit val keyValuePairFormat = jsonFormat2(KeyValuePair) - implicit val userKeyValuePairFormat = jsonFormat2(UserKeyValuePair) - implicit val userKeyValuePairsFormat = jsonFormat2(UserKeyValuePairs) - implicit val notificationFormat = jsonFormat7(Notification) + implicit val keyValuePairFormat: RootJsonFormat[KeyValuePair] = jsonFormat2(KeyValuePair) + implicit val userKeyValuePairFormat: RootJsonFormat[UserKeyValuePair] = jsonFormat2(UserKeyValuePair) + implicit val userKeyValuePairsFormat: RootJsonFormat[UserKeyValuePairs] = jsonFormat2(UserKeyValuePairs) + implicit val notificationFormat: RootJsonFormat[Notification] = jsonFormat7(Notification) } object ThurloeQuery { @@ -37,14 +37,16 @@ object ThurloeQuery { ThurloeQuery(getValues(asMap.get(UserIdParam)), getValues(asMap.get(KeyParam)), getValues(asMap.get(ValueParam)), - getKeys(asMap.get(UnrecognizedParams))) + getKeys(asMap.get(UnrecognizedParams)) + ) } } final case class ThurloeQuery(userId: Option[Seq[String]], key: Option[Seq[String]], value: Option[Seq[String]], - unrecognizedFilters: Option[Seq[String]]) { + unrecognizedFilters: Option[Seq[String]] +) { def isEmpty: Boolean = userId.isEmpty && key.isEmpty && value.isEmpty } @@ -62,4 +64,5 @@ case class Notification(userId: Option[WorkbenchUserId], notificationId: String, substitutions: Map[String, String], emailLookupSubstitutions: Map[String, WorkbenchUserId], - nameLookupSubstitution: Map[String, WorkbenchUserId]) + nameLookupSubstitution: Map[String, WorkbenchUserId] +) diff --git a/src/main/scala/thurloe/service/FireCloudProtectedServices.scala b/src/main/scala/thurloe/service/FireCloudProtectedServices.scala index 227ccfda..e1fad0a9 100644 --- a/src/main/scala/thurloe/service/FireCloudProtectedServices.scala +++ b/src/main/scala/thurloe/service/FireCloudProtectedServices.scala @@ -16,7 +16,7 @@ trait FireCloudProtectedServices extends ThurloeService with NotificationService val fireCloudProtectedRoutes: Route = optionalHeaderValueByName(fcHeader) { case Some(x) if x.equals(fcId) => pathPrefix("api")(keyValuePairRoutes ~ notificationRoutes) case Some(_) => complete(StatusCodes.BadRequest, s"Invalid '$fcHeader' Header Provided") - case None => complete(StatusCodes.BadRequest, s"Request is missing required HTTP header '$fcHeader'") + case None => complete(StatusCodes.BadRequest, s"Request is missing required HTTP header '$fcHeader'") } } diff --git a/src/main/scala/thurloe/service/StatusService.scala b/src/main/scala/thurloe/service/StatusService.scala index b0a326ef..20ca8f97 100644 --- a/src/main/scala/thurloe/service/StatusService.scala +++ b/src/main/scala/thurloe/service/StatusService.scala @@ -20,7 +20,9 @@ trait StatusService { case Failure(e) => complete(StatusCodes.InternalServerError, HttpEntity(ContentTypes.`application/json`, - s"""{"status": "down", "error": "${e.getMessage()}"}""")) + s"""{"status": "down", "error": "${e.getMessage()}"}""" + ) + ) } } } diff --git a/src/main/scala/thurloe/service/ThurloeService.scala b/src/main/scala/thurloe/service/ThurloeService.scala index f7fdd086..509852b5 100644 --- a/src/main/scala/thurloe/service/ThurloeService.scala +++ b/src/main/scala/thurloe/service/ThurloeService.scala @@ -71,7 +71,7 @@ trait ThurloeService extends LazyLogging { } val getAllRoute: Route = - path(ThurloePrefix / Segment) { (userId) => + path(ThurloePrefix / Segment) { userId => get { onComplete(dataAccess.lookup(samDao, userId)) { case Success(userKeyValuePairs) => diff --git a/src/main/scala/thurloe/service/ThurloeServiceActor.scala b/src/main/scala/thurloe/service/ThurloeServiceActor.scala index 0c606a81..449a514a 100644 --- a/src/main/scala/thurloe/service/ThurloeServiceActor.scala +++ b/src/main/scala/thurloe/service/ThurloeServiceActor.scala @@ -7,6 +7,7 @@ import akka.util.ByteString import com.typesafe.config.ConfigFactory import thurloe.dataaccess.{HttpSendGridDAO, SamDAO} import thurloe.database.ThurloeDatabaseConnector +import thurloe.security.CSPDirective.addCSP class ThurloeServiceActor(httpSamDao: SamDAO) extends FireCloudProtectedServices with StatusService { val authConfig = ConfigFactory.load().getConfig("auth") @@ -14,12 +15,13 @@ class ThurloeServiceActor(httpSamDao: SamDAO) extends FireCloudProtectedServices val samDao = httpSamDao override val dataAccess = ThurloeDatabaseConnector override val sendGridDAO = new HttpSendGridDAO(samDao) - protected val swaggerUiPath = "META-INF/resources/webjars/swagger-ui/4.1.3" + protected val swaggerUiPath = "META-INF/resources/webjars/swagger-ui/5.17.14" - def route: Route = + def route: Route = addCSP { swaggerUiService ~ statusRoute ~ fireCloudProtectedRoutes + } - val swaggerUiService = { + val swaggerUiService = path("") { get { serveIndex @@ -39,7 +41,6 @@ class ThurloeServiceActor(httpSamDao: SamDAO) extends FireCloudProtectedServices getFromResourceDirectory(swaggerUiPath) } } - } private val serveIndex: Route = { val swaggerOptions = diff --git a/src/test/scala/thurloe/dataaccess/MockSendGridDAO.scala b/src/test/scala/thurloe/dataaccess/MockSendGridDAO.scala index 79952df5..4bc76757 100644 --- a/src/test/scala/thurloe/dataaccess/MockSendGridDAO.scala +++ b/src/test/scala/thurloe/dataaccess/MockSendGridDAO.scala @@ -52,17 +52,20 @@ class MockSendGridDAO extends SendGridDAO { val testUserName3 = ("Elvin", "") val testUserContactEmail3 = "" - val notificationMonitorPreferredEmails = (for (i <- 0 until 10 * 4) yield (s"bar$i" -> (s"bar$i", s"bar$i"))) + val notificationMonitorPreferredEmails = for (i <- 0 until 10 * 4) yield s"bar$i" -> (s"bar$i", s"bar$i") val preferredEmailMap = Map( testUserId1 -> (testUserEmail1, testUserContactEmail), testUserId2 -> (testUserEmail2, testUserContactEmail2) ) ++ notificationMonitorPreferredEmails - val notificationMonitorUserNames = (for (i <- 0 until 10 * 4) yield (s"bar$i" -> (s"First$i", s"Last$i"))) + val notificationMonitorUserNames = for (i <- 0 until 10 * 4) yield s"bar$i" -> (s"First$i", s"Last$i") val nameMap = - Map(testUserId1 -> testUserName1, testUserId2 -> testUserName2, testUserId3 -> testUserName3) ++ notificationMonitorUserNames + Map(testUserId1 -> testUserName1, + testUserId2 -> testUserName2, + testUserId3 -> testUserName3 + ) ++ notificationMonitorUserNames def lookupPreferredEmail(userId: WorkbenchUserId): Future[WorkbenchEmail] = Future { preferredEmailMap get userId.value match { diff --git a/src/test/scala/thurloe/notification/NotificationMonitorSpec.scala b/src/test/scala/thurloe/notification/NotificationMonitorSpec.scala index 5ba3168c..01c14aaa 100644 --- a/src/test/scala/thurloe/notification/NotificationMonitorSpec.scala +++ b/src/test/scala/thurloe/notification/NotificationMonitorSpec.scala @@ -76,7 +76,8 @@ class NotificationMonitorSpec(_system: ActorSystem) WorkspaceInvitedNotification(WorkbenchEmail(s"foo$i"), id, WorkspaceName("namespace", "name"), - "some-bucket-name") + "some-bucket-name" + ) } // wait for all the messages to be published and throw an error if one happens (i.e. use Await.result not Await.ready) @@ -119,7 +120,8 @@ class NotificationMonitorSpec(_system: ActorSystem) workerCount, sendGridDAO, Map("WorkspaceRemovedNotification" -> "valid_notification_id1", - "WorkspaceAddedNotification" -> "valid_notification_id1"), + "WorkspaceAddedNotification" -> "valid_notification_id1" + ), "foo", samDao ) @@ -185,7 +187,8 @@ class NotificationMonitorSpec(_system: ActorSystem) workerCount, sendGridDAO, Map("WorkspaceRemovedNotification" -> "valid_notification_id1", - "WorkspaceAddedNotification" -> "valid_notification_id1"), + "WorkspaceAddedNotification" -> "valid_notification_id1" + ), "foo", samDao ) @@ -211,7 +214,8 @@ class NotificationMonitorSpec(_system: ActorSystem) Await.result(ThurloeDatabaseConnector.set( UserKeyValuePairs(userId, Seq(KeyValuePair(NotificationMonitor.notificationsOffKey, "true"))) ), - Duration.Inf) + Duration.Inf + ) // wait for all the messages to be published and throw an error if one happens (i.e. use Await.result not Await.ready) val testNotifications = Seq(removedNotification, addedNotification) @@ -262,7 +266,8 @@ class NotificationMonitorSpec(_system: ActorSystem) "some-config", "some-entity", 15, - "no comment") + "no comment" + ) Await.result( ThurloeDatabaseConnector.set( @@ -335,7 +340,8 @@ class NotificationMonitorSpec(_system: ActorSystem) "some-config", "some-entity", 15, - "no comment") + "no comment" + ) Await.result( ThurloeDatabaseConnector.set( @@ -364,7 +370,8 @@ class NotificationMonitorSpec(_system: ActorSystem) val topic = "topic" val workerCount = 1 - val sendGridDAO = new MockSendGridDAOWithException // throws an KeyNotFoundException when calling `sendNotifications` + val sendGridDAO = + new MockSendGridDAOWithException // throws an KeyNotFoundException when calling `sendNotifications` system.actorOf( NotificationMonitorSupervisor.props( 10 milliseconds, @@ -375,7 +382,8 @@ class NotificationMonitorSpec(_system: ActorSystem) workerCount, sendGridDAO, Map("WorkspaceRemovedNotification" -> "valid_notification_id1", - "WorkspaceAddedNotification" -> "valid_notification_id1"), + "WorkspaceAddedNotification" -> "valid_notification_id1" + ), "foo", samDao ) @@ -395,7 +403,8 @@ class NotificationMonitorSpec(_system: ActorSystem) Await.result(ThurloeDatabaseConnector.set( UserKeyValuePairs(userId, Seq(KeyValuePair(NotificationMonitor.notificationsOffKey, "false"))) ), - Duration.Inf) + Duration.Inf + ) val testNotifications = Seq(removedNotification, addedNotification) Await.result( diff --git a/src/test/scala/thurloe/service/NotificationServiceSpec.scala b/src/test/scala/thurloe/service/NotificationServiceSpec.scala index dccd91f8..a81d0f1c 100644 --- a/src/test/scala/thurloe/service/NotificationServiceSpec.scala +++ b/src/test/scala/thurloe/service/NotificationServiceSpec.scala @@ -20,21 +20,24 @@ class NotificationServiceSpec extends AnyFunSpec with ScalatestRouteTest { "valid_notification_id1", Map.empty, Map.empty, - Map.empty) + Map.empty + ) val validNotification2 = Notification(Some(WorkbenchUserId("a_user_id")), None, Option(Set(WorkbenchUserId("a_user_id"))), "valid_notification_id1", Map.empty, Map.empty, - Map.empty) + Map.empty + ) val invalidNotification = Notification(Some(WorkbenchUserId("a_user_id")), None, None, "invalid_notification_id1", Map.empty, Map.empty, - Map.empty) + Map.empty + ) def notificationService = new NotificationService { val sendGridDAO = new MockSendGridDAO @@ -54,7 +57,9 @@ class NotificationServiceSpec extends AnyFunSpec with ScalatestRouteTest { } it("should send a list of valid notifications to a user") { - Post("/notification", List(validNotification, validNotification2)) ~> notificationService.notificationRoutes ~> check { + Post("/notification", + List(validNotification, validNotification2) + ) ~> notificationService.notificationRoutes ~> check { assertResult("OK") { responseAs[String] } @@ -73,7 +78,9 @@ class NotificationServiceSpec extends AnyFunSpec with ScalatestRouteTest { } it("should not send an invalid notification to a user in a list with a valid notification") { - Post("/notification", List(invalidNotification, validNotification)) ~> notificationService.notificationRoutes ~> check { + Post("/notification", + List(invalidNotification, validNotification) + ) ~> notificationService.notificationRoutes ~> check { assertResult(StatusCodes.BadRequest) { status } @@ -81,7 +88,9 @@ class NotificationServiceSpec extends AnyFunSpec with ScalatestRouteTest { } it("should send a valid notification to a user with no contactEmail set") { - Post("/notification", List(validNotification.copy(userId = Some(WorkbenchUserId("a_user_id2"))))) ~> notificationService.notificationRoutes ~> check { + Post("/notification", + List(validNotification.copy(userId = Some(WorkbenchUserId("a_user_id2")))) + ) ~> notificationService.notificationRoutes ~> check { assertResult("OK") { responseAs[String] } @@ -92,7 +101,9 @@ class NotificationServiceSpec extends AnyFunSpec with ScalatestRouteTest { } it("throw an exception when sending a valid notification to a user with no contact settings") { - Post("/notification", List(validNotification.copy(userId = Some(WorkbenchUserId("a_user_id3"))))) ~> notificationService.notificationRoutes ~> check { + Post("/notification", + List(validNotification.copy(userId = Some(WorkbenchUserId("a_user_id3")))) + ) ~> notificationService.notificationRoutes ~> check { assertResult(StatusCodes.InternalServerError) { status } @@ -111,7 +122,9 @@ class NotificationServiceSpec extends AnyFunSpec with ScalatestRouteTest { } it("throw an exception when sending a notification with no userId or userEmail set") { - Post("/notification", List(validNotification.copy(userId = None))) ~> notificationService.notificationRoutes ~> check { + Post("/notification", + List(validNotification.copy(userId = None)) + ) ~> notificationService.notificationRoutes ~> check { assertResult(StatusCodes.BadRequest) { status } diff --git a/src/test/scala/thurloe/service/ThurloeServiceActorSpec.scala b/src/test/scala/thurloe/service/ThurloeServiceActorSpec.scala new file mode 100644 index 00000000..c988439e --- /dev/null +++ b/src/test/scala/thurloe/service/ThurloeServiceActorSpec.scala @@ -0,0 +1,28 @@ +package thurloe.service + +import akka.http.scaladsl.model.StatusCodes +import akka.http.scaladsl.testkit.ScalatestRouteTest +import org.mockito.MockitoSugar.mock +import org.scalatest.funspec.AnyFunSpec +import org.scalatest.matchers.should.Matchers +import thurloe.dataaccess.HttpSamDAO + +class ThurloeServiceActorSpec extends AnyFunSpec with Matchers with ScalatestRouteTest { + + val service = new ThurloeServiceActor(mock[HttpSamDAO]) + + describe("ThurloeServiceActor") { + it("include Content-Security-Policy header in main route") { + Get("/") ~> service.route ~> check { + val cspHeader = header("Content-Security-Policy") + cspHeader shouldBe defined + cspHeader.get.value should include("default-src 'self'") + cspHeader.get.value should include("script-src 'self' 'unsafe-inline'") + + assertResult(StatusCodes.OK) { + status + } + } + } + } +}