Merge pull request #1829 from eikek/1827-auth-oidc-fix

Allow to authenticate with the same account from different sources

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)
 }

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 = {
+      private def check(
-        val passOk = BCrypt.checkpw(givenPass, data.password.pass)
+          givenPass: String
-        checkNoPassword(data, Set(AccountSource.Local)) && passOk
+      )(data: QLogin.Data, expectedSources: Set[AccountSource]): Boolean = {
+        val passOk = PasswordCrypt.check(Password(givenPass), data.password)
+        checkNoPassword(data, expectedSources) && passOk
       }
 
       def checkNoPassword(

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)
+            if (!expectedSources.contains(user.source))
-              OptionT.pure[F](PassResetResult.userNotLocal)
+              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)
+          user <- OptionT(store.transact(RUser.findById(userId, collectiveId.some)))
-          check = optUser.map(_.password).map(p => PasswordCrypt.check(current, p))
+          check = user.password.isEmpty || PasswordCrypt.check(current, user.password)
-          n <-
+          res <-
-            check
+            if (check && expectedSources.contains(user.source))
-              .filter(identity)
+              OptionT.liftF(
-              .traverse(_ =>
+                store
-                RUser.updatePassword(collectiveId, userId, PasswordCrypt.crypt(newPass))
+                  .transact(
+                    RUser
+                      .updatePassword(collectiveId, userId, PasswordCrypt.crypt(newPass))
+                  )
+                  .map {
+                    case 0 => PassChangeResult.updateFailed
+                    case _ => PassChangeResult.success
+                  }
               )
-          res = check match {
+            else if (check && !expectedSources.contains(user.source))
-            case Some(true) =>
+              OptionT.some[F](PassChangeResult.invalidSource(user.source))
-              if (n.getOrElse(0) > 0) PassChangeResult.success
+            else OptionT.some[F](PassChangeResult.passwordMismatch)
-              else if (optUser.exists(_.source != AccountSource.Local))
-                PassChangeResult.userNotLocal
-              else PassChangeResult.updateFailed
-            case Some(false) =>
-              PassChangeResult.passwordMismatch
-            case None =>
-              PassChangeResult.userNotFound
-          }
         } yield res
 
-        store.transact(q)
+        q.getOrElse(PassChangeResult.userNotFound)
       }
 
       def getContacts(

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"

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] =

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),

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)
   }
 

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

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 =>
+      case PassChangeResult.InvalidSource(source) =>
-        BasicResult(false, "User is not local, passwords are managed externally.")
+        BasicResult(
+          false,
+          s"User has invalid soure: $source. Passwords are managed elsewhere."
+        )
     }
 
   def basicResult(e: Either[Throwable, _], successMsg: String): BasicResult =

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
+    }
 }

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)

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(

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).