diff --git a/modules/backend/src/main/scala/docspell/backend/PasswordCrypt.scala b/modules/backend/src/main/scala/docspell/backend/PasswordCrypt.scala index 34a98bd0..fe59d2bb 100644 --- a/modules/backend/src/main/scala/docspell/backend/PasswordCrypt.scala +++ b/modules/backend/src/main/scala/docspell/backend/PasswordCrypt.scala @@ -11,10 +11,13 @@ import docspell.common.Password import org.mindrot.jbcrypt.BCrypt object PasswordCrypt { + // BCrypt requires non-empty strings def crypt(pass: Password): Password = - Password(BCrypt.hashpw(pass.pass, BCrypt.gensalt())) + if (pass.isEmpty) sys.error("Empty password given to hash") + else Password(BCrypt.hashpw(pass.pass, BCrypt.gensalt())) def check(plain: Password, hashed: Password): Boolean = - BCrypt.checkpw(plain.pass, hashed.pass) + if (plain.isEmpty || hashed.isEmpty) false + else BCrypt.checkpw(plain.pass, hashed.pass) } diff --git a/modules/backend/src/main/scala/docspell/backend/auth/Login.scala b/modules/backend/src/main/scala/docspell/backend/auth/Login.scala index 9d265280..f8003505 100644 --- a/modules/backend/src/main/scala/docspell/backend/auth/Login.scala +++ b/modules/backend/src/main/scala/docspell/backend/auth/Login.scala @@ -6,10 +6,11 @@ package docspell.backend.auth -import cats.data.{EitherT, OptionT} +import cats.data.{EitherT, NonEmptyList, OptionT} import cats.effect._ import cats.implicits._ +import docspell.backend.PasswordCrypt import docspell.backend.auth.Login._ import docspell.common._ import docspell.store.Store @@ -17,7 +18,6 @@ import docspell.store.queries.QLogin import docspell.store.records._ import docspell.totp.{OnetimePassword, Totp} -import org.mindrot.jbcrypt.BCrypt import scodec.bits.ByteVector trait Login[F[_]] { @@ -43,9 +43,32 @@ object Login { case class Config( serverSecret: ByteVector, sessionValid: Duration, - rememberMe: RememberMe + rememberMe: RememberMe, + onAccountSourceConflict: OnAccountSourceConflict ) + sealed trait OnAccountSourceConflict { + def name: String + } + object OnAccountSourceConflict { + case object Fail extends OnAccountSourceConflict { + val name = "fail" + } + case object Convert extends OnAccountSourceConflict { + val name = "convert" + } + + val all: NonEmptyList[OnAccountSourceConflict] = + NonEmptyList.of(Fail, Convert) + + def fromString(str: String): Either[String, OnAccountSourceConflict] = + all + .find(_.name.equalsIgnoreCase(str)) + .toRight( + s"Invalid on-account-source-conflict value: $str. Use one of ${all.toList.mkString(", ")}" + ) + } + case class RememberMe(enabled: Boolean, valid: Duration) { val disabled = !enabled } @@ -73,8 +96,13 @@ object Login { case object InvalidAuth extends Result { val toEither = Left("Authentication failed.") } + case class InvalidAccountSource(account: AccountId) extends Result { + val toEither = Left( + s"The account '${account.asString}' already exists from a different source (local vs openid)!" + ) + } case object InvalidTime extends Result { - val toEither = Left("Authentication failed.") + val toEither = Left("Authentication failed due expired authenticator.") } case object InvalidFactor extends Result { val toEither = Left("Authentication requires second factor.") @@ -85,6 +113,7 @@ object Login { def invalidAuth: Result = InvalidAuth def invalidTime: Result = InvalidTime def invalidFactor: Result = InvalidFactor + def invalidAccountSource(account: AccountId): Result = InvalidAccountSource(account) } def apply[F[_]: Async](store: Store[F], totp: Totp): Resource[F, Login[F]] = @@ -99,6 +128,26 @@ object Login { res <- data match { case Some(d) if checkNoPassword(d, Set(AccountSource.OpenId)) => doLogin(config, d.account, false) + case Some(d) if checkNoPassword(d, Set(AccountSource.Local)) => + config.onAccountSourceConflict match { + case OnAccountSourceConflict.Fail => + Result.invalidAccountSource(accountId).pure[F] + case OnAccountSourceConflict.Convert => + for { + _ <- logF.debug( + s"Converting account ${d.account.asString} from Local to OpenId!" + ) + _ <- store + .transact( + RUser.updateSource( + d.account.userId, + d.account.collectiveId, + AccountSource.OpenId + ) + ) + res <- doLogin(config, d.account, false) + } yield res + } case _ => Result.invalidAuth.pure[F] } @@ -125,8 +174,31 @@ object Login { data <- store.transact(QLogin.findUser(acc)) _ <- logF.trace(s"Account lookup: $data") res <- data match { - case Some(d) if check(up.pass)(d) => + case Some(d) if check(up.pass)(d, Set(AccountSource.Local)) => doLogin(config, d.account, up.rememberMe) + case Some(d) if check(up.pass)(d, Set(AccountSource.OpenId)) => + config.onAccountSourceConflict match { + case OnAccountSourceConflict.Fail => + logF.info( + s"Fail authentication because of account source mismatch (local vs openid)." + ) *> + Result.invalidAccountSource(d.account.asAccountId).pure[F] + case OnAccountSourceConflict.Convert => + for { + _ <- logF.debug( + s"Converting account ${d.account.asString} from OpenId to Local!" + ) + _ <- store + .transact( + RUser.updateSource( + d.account.userId, + d.account.collectiveId, + AccountSource.Local + ) + ) + res <- doLogin(config, d.account, up.rememberMe) + } yield res + } case _ => Result.invalidAuth.pure[F] } @@ -277,9 +349,11 @@ object Login { token <- RememberToken.user(rme.id, config.serverSecret) } yield token - private def check(givenPass: String)(data: QLogin.Data): Boolean = { - val passOk = BCrypt.checkpw(givenPass, data.password.pass) - checkNoPassword(data, Set(AccountSource.Local)) && passOk + private def check( + givenPass: String + )(data: QLogin.Data, expectedSources: Set[AccountSource]): Boolean = { + val passOk = PasswordCrypt.check(Password(givenPass), data.password) + checkNoPassword(data, expectedSources) && passOk } def checkNoPassword( diff --git a/modules/backend/src/main/scala/docspell/backend/ops/OCollective.scala b/modules/backend/src/main/scala/docspell/backend/ops/OCollective.scala index 6db532d5..d96a7333 100644 --- a/modules/backend/src/main/scala/docspell/backend/ops/OCollective.scala +++ b/modules/backend/src/main/scala/docspell/backend/ops/OCollective.scala @@ -55,10 +55,14 @@ trait OCollective[F[_]] { collectiveId: CollectiveId, userId: Ident, current: Password, - newPass: Password + newPass: Password, + expectedSources: Set[AccountSource] ): F[PassChangeResult] - def resetPassword(accountId: AccountId): F[PassResetResult] + def resetPassword( + accountId: AccountId, + expectedSources: Set[AccountSource] + ): F[PassResetResult] def getContacts( collective: CollectiveId, @@ -114,11 +118,11 @@ object OCollective { object PassResetResult { case class Success(newPw: Password) extends PassResetResult case object NotFound extends PassResetResult - case object UserNotLocal extends PassResetResult + case class InvalidSource(source: AccountSource) extends PassResetResult def success(np: Password): PassResetResult = Success(np) def notFound: PassResetResult = NotFound - def userNotLocal: PassResetResult = UserNotLocal + def invalidSource(source: AccountSource): PassResetResult = InvalidSource(source) } sealed trait PassChangeResult @@ -126,14 +130,14 @@ object OCollective { case object UserNotFound extends PassChangeResult case object PasswordMismatch extends PassChangeResult case object UpdateFailed extends PassChangeResult - case object UserNotLocal extends PassChangeResult + case class InvalidSource(source: AccountSource) extends PassChangeResult case object Success extends PassChangeResult def userNotFound: PassChangeResult = UserNotFound def passwordMismatch: PassChangeResult = PasswordMismatch def success: PassChangeResult = Success def updateFailed: PassChangeResult = UpdateFailed - def userNotLocal: PassChangeResult = UserNotLocal + def invalidSource(source: AccountSource): PassChangeResult = InvalidSource(source) } def apply[F[_]: Async]( @@ -280,7 +284,10 @@ object OCollective { def tagCloud(collective: CollectiveId): F[List[TagCount]] = store.transact(QCollective.tagCloud(collective)) - def resetPassword(accountId: AccountId): F[PassResetResult] = + def resetPassword( + accountId: AccountId, + expectedSources: Set[AccountSource] + ): F[PassResetResult] = (for { user <- OptionT(store.transact(RUser.findByAccount(accountId))) newPass <- OptionT.liftF(Password.generate[F]) @@ -289,8 +296,8 @@ object OCollective { RUser.updatePassword(user.cid, user.uid, PasswordCrypt.crypt(newPass)) ) res <- - if (user.source != AccountSource.Local) - OptionT.pure[F](PassResetResult.userNotLocal) + if (!expectedSources.contains(user.source)) + OptionT.pure[F](PassResetResult.invalidSource(user.source)) else OptionT.liftF(doUpdate.as(PassResetResult.success(newPass))) } yield res).getOrElse(PassResetResult.notFound) @@ -298,31 +305,31 @@ object OCollective { collectiveId: CollectiveId, userId: Ident, current: Password, - newPass: Password + newPass: Password, + expectedSources: Set[AccountSource] ): F[PassChangeResult] = { val q = for { - optUser <- RUser.findById(userId, collectiveId.some) - check = optUser.map(_.password).map(p => PasswordCrypt.check(current, p)) - n <- - check - .filter(identity) - .traverse(_ => - RUser.updatePassword(collectiveId, userId, PasswordCrypt.crypt(newPass)) + user <- OptionT(store.transact(RUser.findById(userId, collectiveId.some))) + check = user.password.isEmpty || PasswordCrypt.check(current, user.password) + res <- + if (check && expectedSources.contains(user.source)) + OptionT.liftF( + store + .transact( + RUser + .updatePassword(collectiveId, userId, PasswordCrypt.crypt(newPass)) + ) + .map { + case 0 => PassChangeResult.updateFailed + case _ => PassChangeResult.success + } ) - res = check match { - case Some(true) => - if (n.getOrElse(0) > 0) PassChangeResult.success - else if (optUser.exists(_.source != AccountSource.Local)) - PassChangeResult.userNotLocal - else PassChangeResult.updateFailed - case Some(false) => - PassChangeResult.passwordMismatch - case None => - PassChangeResult.userNotFound - } + else if (check && !expectedSources.contains(user.source)) + OptionT.some[F](PassChangeResult.invalidSource(user.source)) + else OptionT.some[F](PassChangeResult.passwordMismatch) } yield res - store.transact(q) + q.getOrElse(PassChangeResult.userNotFound) } def getContacts( diff --git a/modules/restserver/src/main/resources/reference.conf b/modules/restserver/src/main/resources/reference.conf index 25896f48..ecb51699 100644 --- a/modules/restserver/src/main/resources/reference.conf +++ b/modules/restserver/src/main/resources/reference.conf @@ -95,6 +95,17 @@ docspell.server { # How long the remember me cookie/token is valid. valid = "30 days" } + + # One of: fail, convert + # + # Accounts can be local or defined at a remote provider and + # integrated via OIDC. If the same account is defined in both + # sources, docspell by default fails if a user mixes logins (e.g. + # when registering a user locally and then logging in with the + # same user via OIDC). When set to `convert` docspell treats it as + # being the same and simply updates the account to reflect the new + # account source. + on-account-source-conflict = "fail" } # Settings for "download as zip" diff --git a/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala b/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala index c3cbd825..081b34bc 100644 --- a/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala +++ b/modules/restserver/src/main/scala/docspell/restserver/ConfigFile.scala @@ -11,6 +11,7 @@ import java.security.SecureRandom import cats.Monoid import cats.effect.Async +import docspell.backend.auth.Login import docspell.backend.signup.{Config => SignupConfig} import docspell.config.Implicits._ import docspell.config.{ConfigFactory, FtsType, Validation} @@ -50,6 +51,10 @@ object ConfigFile { implicit val openIdExtractorReader: ConfigReader[OpenId.UserInfo.Extractor] = ConfigReader[String].emap(reason(OpenId.UserInfo.Extractor.fromString)) + + implicit val onAccountSourceConflictReader + : ConfigReader[Login.OnAccountSourceConflict] = + ConfigReader[String].emap(reason(Login.OnAccountSourceConflict.fromString)) } def generateSecretIfEmpty: Validation[Config] = diff --git a/modules/restserver/src/main/scala/docspell/restserver/RestAppImpl.scala b/modules/restserver/src/main/scala/docspell/restserver/RestAppImpl.scala index 500a51d5..513dceb4 100644 --- a/modules/restserver/src/main/scala/docspell/restserver/RestAppImpl.scala +++ b/modules/restserver/src/main/scala/docspell/restserver/RestAppImpl.scala @@ -81,7 +81,7 @@ final class RestAppImpl[F[_]: Async]( Router( "fts" -> FullTextIndexRoutes.admin(config, backend), "user/otp" -> TotpRoutes.admin(backend), - "user" -> UserRoutes.admin(backend), + "user" -> UserRoutes.admin(backend, config.auth), "info" -> InfoRoutes.admin(config), "attachments" -> AttachmentRoutes.admin(backend), "files" -> FileRepositoryRoutes.admin(backend) @@ -129,7 +129,7 @@ final class RestAppImpl[F[_]: Async]( "person" -> PersonRoutes(backend, token), "source" -> SourceRoutes(backend, token), "user/otp" -> TotpRoutes(backend, config, token), - "user" -> UserRoutes(backend, token), + "user" -> UserRoutes(backend, config.auth, token), "collective" -> CollectiveRoutes(backend, token), "queue" -> JobQueueRoutes(backend, token), "item" -> ItemRoutes(config, backend, token), diff --git a/modules/restserver/src/main/scala/docspell/restserver/RestServer.scala b/modules/restserver/src/main/scala/docspell/restserver/RestServer.scala index e4e3b1da..f46bce0b 100644 --- a/modules/restserver/src/main/scala/docspell/restserver/RestServer.scala +++ b/modules/restserver/src/main/scala/docspell/restserver/RestServer.scala @@ -116,6 +116,7 @@ object RestServer { )( wsB: WebSocketBuilder2[F] ) = { + val logger = docspell.logging.getLogger[F] val internal = Router( "/" -> redirectTo("/app"), "/internal" -> InternalHeader(internSettings.internalRouteKey) { @@ -123,6 +124,17 @@ object RestServer { } ) val httpApp = (internal <+> restApp.routes(wsB)).orNotFound + .mapF( + _.attempt + .flatMap { eab => + eab.fold( + ex => + logger.error(ex)("Processing the request resulted in an error.").as(eab), + _ => eab.pure[F] + ) + } + .rethrow + ) Logger.httpApp(logHeaders = false, logBody = false)(httpApp) } diff --git a/modules/restserver/src/main/scala/docspell/restserver/auth/OpenId.scala b/modules/restserver/src/main/scala/docspell/restserver/auth/OpenId.scala index b65841ef..1045a055 100644 --- a/modules/restserver/src/main/scala/docspell/restserver/auth/OpenId.scala +++ b/modules/restserver/src/main/scala/docspell/restserver/auth/OpenId.scala @@ -105,6 +105,7 @@ object OpenId { import dsl._ for { + _ <- logger.debug(s"Setting up external account: ${accountId.asString}") setup <- backend.signup.setupExternal(ExternalAccount(accountId)) res <- setup match { case SignupResult.Failure(ex) => @@ -143,6 +144,7 @@ object OpenId { import dsl._ for { + _ <- logger.debug(s"Login and verify external account: ${accountId.asString}") login <- backend.login.loginExternal(config.auth)(accountId) resp <- login match { case Login.Result.Ok(session, _) => @@ -158,7 +160,9 @@ object OpenId { .map(_.addCookie(CookieData(session).asCookie(baseUrl))) case failed => - logger.error(s"External login failed: $failed") *> + logger.error( + s"External login failed: $failed. ${failed.toEither.left.getOrElse("")}" + ) *> SeeOther(location) } } yield resp diff --git a/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala b/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala index 38636cc3..90a4fb30 100644 --- a/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala +++ b/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala @@ -699,8 +699,11 @@ trait Conversions { case PassChangeResult.PasswordMismatch => BasicResult(false, "The current password is incorrect.") case PassChangeResult.UserNotFound => BasicResult(false, "User not found.") - case PassChangeResult.UserNotLocal => - BasicResult(false, "User is not local, passwords are managed externally.") + case PassChangeResult.InvalidSource(source) => + BasicResult( + false, + s"User has invalid soure: $source. Passwords are managed elsewhere." + ) } def basicResult(e: Either[Throwable, _], successMsg: String): BasicResult = diff --git a/modules/restserver/src/main/scala/docspell/restserver/routes/UserRoutes.scala b/modules/restserver/src/main/scala/docspell/restserver/routes/UserRoutes.scala index 82f040ea..243e26b9 100644 --- a/modules/restserver/src/main/scala/docspell/restserver/routes/UserRoutes.scala +++ b/modules/restserver/src/main/scala/docspell/restserver/routes/UserRoutes.scala @@ -10,7 +10,8 @@ import cats.effect._ import cats.implicits._ import docspell.backend.BackendApp -import docspell.backend.auth.AuthToken +import docspell.backend.auth.Login.OnAccountSourceConflict +import docspell.backend.auth.{AuthToken, Login} import docspell.backend.ops.OCollective import docspell.common._ import docspell.restapi.model._ @@ -24,7 +25,11 @@ import org.http4s.dsl.Http4sDsl object UserRoutes { - def apply[F[_]: Async](backend: BackendApp[F], user: AuthToken): HttpRoutes[F] = { + def apply[F[_]: Async]( + backend: BackendApp[F], + loginConfig: Login.Config, + user: AuthToken + ): HttpRoutes[F] = { val dsl = new Http4sDsl[F] {} import dsl._ @@ -36,7 +41,8 @@ object UserRoutes { user.account.collectiveId, user.account.userId, data.currentPassword, - data.newPassword + data.newPassword, + expectedAccountSources(loginConfig) ) resp <- Ok(basicResult(res)) } yield resp @@ -91,14 +97,20 @@ object UserRoutes { } } - def admin[F[_]: Async](backend: BackendApp[F]): HttpRoutes[F] = { + def admin[F[_]: Async]( + backend: BackendApp[F], + loginConfig: Login.Config + ): HttpRoutes[F] = { val dsl = new Http4sDsl[F] {} import dsl._ HttpRoutes.of { case req @ POST -> Root / "resetPassword" => for { input <- req.as[ResetPassword] - result <- backend.collective.resetPassword(input.account) + result <- backend.collective.resetPassword( + input.account, + expectedAccountSources(loginConfig) + ) resp <- Ok(result match { case OCollective.PassResetResult.Success(np) => ResetPasswordResult(true, np, "Password updated") @@ -108,14 +120,20 @@ object UserRoutes { Password(""), "Password update failed. User not found." ) - case OCollective.PassResetResult.UserNotLocal => + case OCollective.PassResetResult.InvalidSource(source) => ResetPasswordResult( false, Password(""), - "Password update failed. User is not local, passwords are managed externally." + s"Password update failed. User has unexpected source: $source. Passwords are managed externally." ) }) } yield resp } } + + private def expectedAccountSources(loginConfig: Login.Config): Set[AccountSource] = + loginConfig.onAccountSourceConflict match { + case OnAccountSourceConflict.Fail => Set(AccountSource.Local) + case OnAccountSourceConflict.Convert => AccountSource.all.toList.toSet + } } diff --git a/modules/store/src/main/scala/docspell/store/records/RUser.scala b/modules/store/src/main/scala/docspell/store/records/RUser.scala index 20d6f16c..b59447c1 100644 --- a/modules/store/src/main/scala/docspell/store/records/RUser.scala +++ b/modules/store/src/main/scala/docspell/store/records/RUser.scala @@ -204,11 +204,20 @@ object RUser { val t = Table(None) DML.update( t, - t.cid === collId && t.uid === userId && t.source === AccountSource.Local, + t.cid === collId && t.uid === userId, DML.set(t.password.setTo(hashedPass)) ) } + def updateSource( + userId: Ident, + collId: CollectiveId, + source: AccountSource + ): ConnectionIO[Int] = { + val t = Table(None) + DML.update(t, t.uid === userId && t.cid === collId, DML.set(t.source.setTo(source))) + } + def delete(user: Ident, coll: CollectiveId): ConnectionIO[Int] = { val t = Table(None) DML.delete(t, t.cid === coll && t.login === user) diff --git a/project/Dependencies.scala b/project/Dependencies.scala index a08ed204..6abd3e2c 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -56,7 +56,7 @@ object Dependencies { val scribe = Seq( "com.outr" %% "scribe" % ScribeVersion, - "com.outr" %% "scribe-slf4j" % ScribeVersion + "com.outr" %% "scribe-slf4j2" % ScribeVersion ) val sourcecode = Seq( diff --git a/website/site/content/docs/configure/authentication.md b/website/site/content/docs/configure/authentication.md index 0c579377..bb32076c 100644 --- a/website/site/content/docs/configure/authentication.md +++ b/website/site/content/docs/configure/authentication.md @@ -145,3 +145,36 @@ provider, skipping the login page for Docspell. For logging out, you can specify a `logout-url` for the provider which is used to redirect the browser after logging out from Docspell. + +## Mixing OIDC and local accounts + +Local accounts and those created from an openid provider can live next +to each other. There is only a caveat for accounts with same login +name that may occur from local and openid providers. By default, +docspell treats OIDC and local accounts always as different when +logging in. + +That means when a local user exists and the same account is trying to +login via an openid provider, docspell fails the authentication +attempt by default. It could be that these accounts belong to +different persons in reality. + +The other way around is the same: signing up an account that exists +due to an OIDC login doesn't work, because the collective already +exists. And obviously, logging in without a password doesn't work +either :). Even if a password would exists for an account created by +an OIDC flow, logging in with it doesn't work. It would allow to +bypass the openid provider (which may not be desired) + +This behavior can be changed by setting: + +``` +on-account-source-conflict = convert +``` + +in the config file (or environment variable). With this setting, +accounts with same name are treated identical, independet where they +came from. So you can login either locally **or** via the configured +openid provider. Note that this also allows users to set a local +password by themselves (which may not adhere to the password rules you +can potentially define at an openid provider).