From 668cd7d974abd527125d42579205d7dc3f3c65d6 Mon Sep 17 00:00:00 2001 From: eikek Date: Mon, 25 Oct 2021 11:27:06 +0200 Subject: [PATCH 1/2] Refactor config validation --- .../scala/docspell/config/ConfigFactory.scala | 9 +- .../scala/docspell/config/Validation.scala | 71 +++++++++++++++ .../docspell/config/ValidationTest.scala | 25 ++++++ .../main/scala/docspell/joex/ConfigFile.scala | 33 +++---- .../docspell/restserver/ConfigFile.scala | 86 ++++++++----------- 5 files changed, 147 insertions(+), 77 deletions(-) create mode 100644 modules/config/src/main/scala/docspell/config/Validation.scala create mode 100644 modules/config/src/test/scala/docspell/config/ValidationTest.scala diff --git a/modules/config/src/main/scala/docspell/config/ConfigFactory.scala b/modules/config/src/main/scala/docspell/config/ConfigFactory.scala index 27661f99..47cfac26 100644 --- a/modules/config/src/main/scala/docspell/config/ConfigFactory.scala +++ b/modules/config/src/main/scala/docspell/config/ConfigFactory.scala @@ -28,20 +28,21 @@ object ConfigFactory { * the default config */ def default[F[_]: Async, C: ClassTag: ConfigReader](logger: Logger[F], atPath: String)( - args: List[String] + args: List[String], + validation: Validation[C] ): F[C] = findFileFromArgs(args).flatMap { case Some(file) => logger.info(s"Using config file: $file") *> - readFile[F, C](file, atPath) + readFile[F, C](file, atPath).map(validation.validOrThrow) case None => checkSystemProperty.value.flatMap { case Some(file) => logger.info(s"Using config file from system property: $file") *> - readConfig(atPath) + readConfig(atPath).map(validation.validOrThrow) case None => logger.info("Using config from environment variables!") *> - readEnv(atPath) + readEnv(atPath).map(validation.validOrThrow) } } diff --git a/modules/config/src/main/scala/docspell/config/Validation.scala b/modules/config/src/main/scala/docspell/config/Validation.scala new file mode 100644 index 00000000..2c4528a7 --- /dev/null +++ b/modules/config/src/main/scala/docspell/config/Validation.scala @@ -0,0 +1,71 @@ +/* + * Copyright 2020 Eike K. & Contributors + * + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +package docspell.config + +import cats._ +import cats.data.{NonEmptyChain, Validated, ValidatedNec} +import cats.implicits._ + +final case class Validation[C](run: C => ValidatedNec[String, C]) { + + def validOrThrow(c: C): C = + run(c) match { + case Validated.Valid(cfg) => cfg + case Validated.Invalid(errs) => + val msg = errs.toList.mkString("- ", "\n- ", "\n") + throw sys.error(s"\n\n$msg") + } + + def andThen(next: Validation[C]): Validation[C] = + Validation(c => + run(c) match { + case Validated.Valid(c2) => next.run(c2) + case f: Validated.Invalid[NonEmptyChain[String]] => + next.run(c) match { + case Validated.Valid(_) => f + case Validated.Invalid(errs2) => + Validation.invalid(f.e ++ errs2) + } + } + ) +} + +object Validation { + + def flatten[C](run: C => Validation[C]): Validation[C] = + Validation(c => run(c).run(c)) + + def failWhen[C](isInvalid: C => Boolean, msg: => String): Validation[C] = + Validation(c => if (isInvalid(c)) invalid(msg) else valid(c)) + + def okWhen[C](isValid: C => Boolean, msg: => String): Validation[C] = + Validation(c => if (isValid(c)) valid(c) else invalid(msg)) + + def valid[C](c: C): ValidatedNec[String, C] = + Validated.validNec(c) + + def invalid[C](msgs: NonEmptyChain[String]): ValidatedNec[String, C] = + Validated.Invalid(msgs) + + def invalid[C](msg: String, msgs: String*): ValidatedNec[String, C] = + Validated.Invalid(NonEmptyChain(msg, msgs: _*)) + + def asValid[C]: Validation[C] = + Validation(c => valid(c)) + + def insert[C](c: C): Validation[C] = + Validation(_ => valid(c)) + + def error[C](msg: String, msgs: String*): Validation[C] = + Validation(_ => invalid(msg, msgs: _*)) + + implicit def validationMonoid[C]: Monoid[Validation[C]] = + Monoid.instance(asValid, (v1, v2) => v1.andThen(v2)) + + def of[C](v1: Validation[C], vn: Validation[C]*): Validation[C] = + Monoid[Validation[C]].combineAll(v1 :: vn.toList) +} diff --git a/modules/config/src/test/scala/docspell/config/ValidationTest.scala b/modules/config/src/test/scala/docspell/config/ValidationTest.scala new file mode 100644 index 00000000..58588424 --- /dev/null +++ b/modules/config/src/test/scala/docspell/config/ValidationTest.scala @@ -0,0 +1,25 @@ +/* + * Copyright 2020 Eike K. & Contributors + * + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +package docspell.config + +import munit.FunSuite + +class ValidationTest extends FunSuite { + + test("thread value through validations") { + val v1 = Validation[Int](n => Validation.valid(n + 1)) + assertEquals(v1.validOrThrow(0), 1) + assertEquals(Validation.of(v1, v1, v1).validOrThrow(0), 3) + } + + test("fail if there is at least one error") { + val v1 = Validation[Int](n => Validation.valid(n + 1)) + val v2 = Validation.error[Int]("error") + assertEquals(Validation.of(v1, v2).run(0), Validation.invalid("error")) + assertEquals(Validation.of(v2, v1).run(0), Validation.invalid("error")) + } +} diff --git a/modules/joex/src/main/scala/docspell/joex/ConfigFile.scala b/modules/joex/src/main/scala/docspell/joex/ConfigFile.scala index 4b3aaf4d..41541480 100644 --- a/modules/joex/src/main/scala/docspell/joex/ConfigFile.scala +++ b/modules/joex/src/main/scala/docspell/joex/ConfigFile.scala @@ -6,14 +6,11 @@ package docspell.joex -import cats.data.Validated -import cats.data.ValidatedNec import cats.effect.Async -import cats.implicits._ import docspell.common.Logger -import docspell.config.ConfigFactory import docspell.config.Implicits._ +import docspell.config.{ConfigFactory, Validation} import docspell.joex.scheduler.CountingScheme import emil.MailAddress @@ -28,13 +25,9 @@ object ConfigFile { def loadConfig[F[_]: Async](args: List[String]): F[Config] = { val logger = Logger.log4s[F](org.log4s.getLogger) ConfigFactory - .default[F, Config](logger, "docspell.joex")(args) - .map(cfg => validOrThrow(cfg)) + .default[F, Config](logger, "docspell.joex")(args, validate) } - private def validOrThrow(cfg: Config): Config = - validate(cfg).fold(err => sys.error(err.toList.mkString("- ", "\n", "")), identity) - object Implicits { implicit val countingSchemeReader: ConfigReader[CountingScheme] = ConfigReader[String].emap(reason(CountingScheme.readString)) @@ -46,23 +39,19 @@ object ConfigFile { ConfigReader[String].emap(reason(MailAddress.parse)) } - def validate(cfg: Config): ValidatedNec[String, Config] = - List( - failWhen( - cfg.updateCheck.enabled && cfg.updateCheck.recipients.isEmpty, + def validate: Validation[Config] = + Validation.of[Config]( + Validation.failWhen( + cfg => cfg.updateCheck.enabled && cfg.updateCheck.recipients.isEmpty, "No recipients given for enabled update check!" ), - failWhen( - cfg.updateCheck.enabled && cfg.updateCheck.smtpId.isEmpty, + Validation.failWhen( + cfg => cfg.updateCheck.enabled && cfg.updateCheck.smtpId.isEmpty, "No recipients given for enabled update check!" ), - failWhen( - cfg.updateCheck.enabled && cfg.updateCheck.subject.els.isEmpty, + Validation.failWhen( + cfg => cfg.updateCheck.enabled && cfg.updateCheck.subject.els.isEmpty, "No subject given for enabled update check!" ) - ).reduce(_ |+| _).map(_ => cfg) - - def failWhen(cond: Boolean, msg: => String): ValidatedNec[String, Unit] = - Validated.condNec(!cond, (), msg) - + ) } diff --git a/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala b/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala index 3dff82e9..7a893671 100644 --- a/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala +++ b/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala @@ -8,15 +8,13 @@ package docspell.restserver import java.security.SecureRandom -import cats.Semigroup -import cats.data.{Validated, ValidatedNec} +import cats.Monoid import cats.effect.Async -import cats.implicits._ import docspell.backend.signup.{Config => SignupConfig} import docspell.common.Logger -import docspell.config.ConfigFactory import docspell.config.Implicits._ +import docspell.config.{ConfigFactory, Validation} import docspell.oidc.{ProviderConfig, SignatureAlgo} import docspell.restserver.auth.OpenId @@ -30,9 +28,10 @@ object ConfigFile { def loadConfig[F[_]: Async](args: List[String]): F[Config] = { val logger = Logger.log4s(unsafeLogger) + val validate = + Validation.of(generateSecretIfEmpty, duplicateOpenIdProvider, signKeyVsUserUrl) ConfigFactory - .default[F, Config](logger, "docspell.server")(args) - .map(cfg => Validate(cfg)) + .default[F, Config](logger, "docspell.server")(args, validate) } object Implicits { @@ -46,29 +45,8 @@ object ConfigFile { ConfigReader[String].emap(reason(OpenId.UserInfo.Extractor.fromString)) } - object Validate { - - implicit val firstConfigSemigroup: Semigroup[Config] = - Semigroup.first - - def apply(config: Config): Config = - all(config).foldLeft(valid(config))(_.combine(_)) match { - case Validated.Valid(cfg) => cfg - case Validated.Invalid(errs) => - val msg = errs.toList.mkString("- ", "\n- ", "\n") - throw sys.error(s"\n\n$msg") - } - - def all(cfg: Config) = List( - duplicateOpenIdProvider(cfg), - signKeyVsUserUrl(cfg), - generateSecretIfEmpty(cfg) - ) - - private def valid(cfg: Config): ValidatedNec[String, Config] = - Validated.validNec(cfg) - - def generateSecretIfEmpty(cfg: Config): ValidatedNec[String, Config] = + def generateSecretIfEmpty: Validation[Config] = + Validation { cfg => if (cfg.auth.serverSecret.isEmpty) { unsafeLogger.warn( "No serverSecret specified. Generating a random one. It is recommended to add a server-secret in the config file." @@ -77,10 +55,12 @@ object ConfigFile { val buffer = new Array[Byte](32) random.nextBytes(buffer) val secret = ByteVector.view(buffer) - valid(cfg.copy(auth = cfg.auth.copy(serverSecret = secret))) - } else valid(cfg) + Validation.valid(cfg.copy(auth = cfg.auth.copy(serverSecret = secret))) + } else Validation.valid(cfg) + } - def duplicateOpenIdProvider(cfg: Config): ValidatedNec[String, Config] = { + def duplicateOpenIdProvider: Validation[Config] = + Validation { cfg => val dupes = cfg.openid .filter(_.enabled) @@ -90,27 +70,31 @@ object ConfigFile { .toList val dupesStr = dupes.mkString(", ") - if (dupes.isEmpty) valid(cfg) - else Validated.invalidNec(s"There is a duplicate openId provider: $dupesStr") + if (dupes.isEmpty) Validation.valid(cfg) + else Validation.invalid(s"There is a duplicate openId provider: $dupesStr") } - def signKeyVsUserUrl(cfg: Config): ValidatedNec[String, Config] = { - def checkProvider(p: ProviderConfig): ValidatedNec[String, Config] = - if (p.signKey.isEmpty && p.userUrl.isEmpty) - Validated.invalidNec( - s"Either user-url or sign-key must be set for provider ${p.providerId.id}" - ) - else if (p.signKey.nonEmpty && p.scope.isEmpty) - Validated.invalidNec( - s"A scope is missing for OIDC auth at provider ${p.providerId.id}" - ) - else Validated.valid(cfg) + def signKeyVsUserUrl: Validation[Config] = + Validation.flatten { cfg => + def checkProvider(p: ProviderConfig): Validation[Config] = + Validation { _ => + if (p.signKey.isEmpty && p.userUrl.isEmpty) + Validation.invalid( + s"Either user-url or sign-key must be set for provider ${p.providerId.id}" + ) + else if (p.signKey.nonEmpty && p.scope.isEmpty) + Validation.invalid( + s"A scope is missing for OIDC auth at provider ${p.providerId.id}" + ) + else Validation.valid(cfg) + } - cfg.openid - .filter(_.enabled) - .map(_.provider) - .map(checkProvider) - .foldLeft(valid(cfg))(_.combine(_)) + Monoid[Validation[Config]] + .combineAll( + cfg.openid + .filter(_.enabled) + .map(_.provider) + .map(checkProvider) + ) } - } } From 26847dc9706097536b04254c1dcf903f7ff0b702 Mon Sep 17 00:00:00 2001 From: eikek Date: Mon, 25 Oct 2021 11:27:17 +0200 Subject: [PATCH 2/2] Remove default config file in docker images Since this file cannot be changed inside the image, and people need to specify a new file or env variables, it doesn't make sense to add it. Also if it is present, it is preferred to the env variables. --- docker/dockerfiles/joex.dockerfile | 3 ++- docker/dockerfiles/restserver.dockerfile | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docker/dockerfiles/joex.dockerfile b/docker/dockerfiles/joex.dockerfile index 05a49367..176cbea2 100644 --- a/docker/dockerfiles/joex.dockerfile +++ b/docker/dockerfiles/joex.dockerfile @@ -62,7 +62,8 @@ WORKDIR /opt RUN wget ${joex_url:-https://github.com/eikek/docspell/releases/download/v$version/docspell-joex-$version.zip} && \ unzip docspell-joex-*.zip && \ rm docspell-joex-*.zip && \ - ln -snf docspell-joex-* docspell-joex + ln -snf docspell-joex-* docspell-joex && \ + rm docspell-joex/conf/docspell-joex.conf # Using these data files for japanese, because they work better. See #973 RUN \ diff --git a/docker/dockerfiles/restserver.dockerfile b/docker/dockerfiles/restserver.dockerfile index aa39cfe8..1b0e59e4 100644 --- a/docker/dockerfiles/restserver.dockerfile +++ b/docker/dockerfiles/restserver.dockerfile @@ -12,7 +12,8 @@ WORKDIR /opt RUN wget ${restserver_url:-https://github.com/eikek/docspell/releases/download/v$version/docspell-restserver-$version.zip} && \ unzip docspell-restserver-*.zip && \ rm docspell-restserver-*.zip && \ - ln -snf docspell-restserver-* docspell-restserver + ln -snf docspell-restserver-* docspell-restserver && \ + rm docspell-restserver/conf/docspell-server.conf ENTRYPOINT ["/opt/docspell-restserver/bin/docspell-restserver", "-J-XX:+UseG1GC"] EXPOSE 7880