Skip to content

Commit

Permalink
Fixes #24839: We cannot login with a user login containing uppercase …
Browse files Browse the repository at this point in the history
…letter if the option case-sensitivity is set to false
  • Loading branch information
clarktsiory authored and fanf committed May 6, 2024
1 parent 05bd32d commit 7cc61e3
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ package com.normation.rudder.users
*/

import cats.data.NonEmptyList
import com.normation.errors.Inconsistency
import com.normation.errors.IOResult
import com.normation.errors.SystemError
import com.normation.rudder.db.Doobie
import com.normation.rudder.domain.logger.ApplicationLoggerPure
import com.normation.utils.DateFormaterService
Expand All @@ -47,6 +49,7 @@ import doobie.*
import doobie.free.connection.unit as connectionUnit
import doobie.implicits.*
import doobie.postgres.implicits.*
import doobie.util.invariant.UnexpectedContinuation
import org.joda.time.DateTime
import zio.*
import zio.interop.catz.*
Expand Down Expand Up @@ -195,7 +198,7 @@ trait UserRepository {
*/
def getAll(): IOResult[List[UserInfo]]

def get(userId: String): IOResult[Option[UserInfo]]
def get(userId: String, isCaseSensitive: Boolean = true): IOResult[Option[UserInfo]]
}

object UserRepository {
Expand Down Expand Up @@ -487,8 +490,14 @@ class InMemoryUserRepository(userBase: Ref[Map[String, UserInfo]], sessionBase:
}
}

override def get(userId: String): IOResult[Option[UserInfo]] = {
userBase.get.map(_.get(userId))
override def get(userId: String, isCaseSensitive: Boolean): IOResult[Option[UserInfo]] = {
userBase.get.flatMap(_.collect {
case (k, v) if (if (isCaseSensitive) k == userId else k.equalsIgnoreCase(userId)) => v
} match {
case Nil => ZIO.none
case u :: Nil => ZIO.some(u)
case _ => Left(Inconsistency(s"Multiple users found for id '${userId}'")).toIO
})
}

override def updateInfo(
Expand Down Expand Up @@ -860,10 +869,17 @@ class JdbcUserRepository(doobie: Doobie) extends UserRepository {
transactIOResult(s"Error when retrieving user information")(xa => sql.query[UserInfo].to[List].transact(xa))
}

override def get(userId: String): IOResult[Option[UserInfo]] = {
val sql = sql"""select * from users where id = ${userId}"""
override def get(userId: String, isCaseSensitive: Boolean): IOResult[Option[UserInfo]] = {
val filter = if (isCaseSensitive) fr"id = ${userId} " else fr"lower(id) = ${userId.toLowerCase()}"
val sql = fr"""select * from users where""" ++ filter

transactIOResult(s"Error when retrieving user information for '${userId}'")(xa => sql.query[UserInfo].option.transact(xa))
// When resultset is exhausted, it means we have many conflicting users, we raise an Inconsistency error
transactIOResult(s"Error when retrieving user information for '${userId}'")(xa =>
sql.query[UserInfo].option.transact(xa)
).mapError {
case SystemError(_, UnexpectedContinuation) => Inconsistency(s"Multiple users found for id '${userId}'")
case e => e
}
}

override def updateInfo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,27 @@ trait UserRepositoryTest extends Specification with Loggable {
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosInit)
}

"Getting user should handle case sensitivity parameter" >> {
repo.get("alice").runNow must beSome
repo.get("Alice").runNow must beNone
repo.get("Alice", isCaseSensitive = false).runNow must beSome
}

"Getting user should raise an error for multiple found users" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, users :+ "Alice", traceInit).runNow
repo.get("alice").runNow must beSome
repo.get("Alice").runNow must beSome
repo.get("alice", isCaseSensitive = false).either.runNow must beLeft(
errors.Inconsistency("Multiple users found for id 'alice'")
)
repo.get("Alice", isCaseSensitive = false).either.runNow must beLeft(
errors.Inconsistency("Multiple users found for id 'Alice'")
)
(repo.delete(List("Alice"), None, Nil, traceInit) *> repo.purge(List("Alice"), None, Nil, traceInit)).runNow must beEqualTo(
List("Alice")
)
}

"If an user is removed from list, it is marked as 'deleted' (but node erased)" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, userFileBobRemoved, traceBobRemoved).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobRemoved)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import com.normation.errors.*
import com.normation.rudder.Role
import com.normation.rudder.api.*
import com.normation.rudder.domain.logger.ApplicationLogger
import com.normation.rudder.domain.logger.ApplicationLoggerPure
import com.normation.rudder.users.*
import com.normation.rudder.users.RudderUserDetail
import com.normation.rudder.web.services.UserSessionLogEvent
Expand Down Expand Up @@ -406,7 +407,15 @@ class RudderInMemoryUserDetailsService(val authConfigProvider: UserDetailListPro
@throws(classOf[DisabledException])
override def loadUserByUsername(username: String): RudderUserDetail = {
userRepository
.get(username)
.get(username, isCaseSensitive = authConfigProvider.authConfig.isCaseSensitive)
.catchSome {
case err: Inconsistency =>
ApplicationLoggerPure
.warn(
s"User '${username}' was found in Rudder base, but with an error: ${err.fullMsg}. Please check/remove duplicate users, and consider reloading users."
)
.as(None)
}
.flatMap {
case Some(user) if (user.status != UserStatus.Deleted) =>
authConfigProvider.getUserByName(username) match {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,15 @@ trait UserDetailListProvider {
*/
def getUserByName(username: String): PureResult[RudderUserDetail] = {
val conf = authConfig
val u = if (conf.isCaseSensitive) username else username.toLowerCase()
conf.users.get(u).toRight(Unexpected(s"User with username '${username}' was not found"))
conf.users
.get(username)
.orElse(
conf.users.collectFirst {
case (u, userDetail) if !conf.isCaseSensitive && u.toLowerCase() == username.toLowerCase() => userDetail
}
)
.toRight(Unexpected(s"User with username '${username}' was not found"))

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class RudderUserDetailsTest extends Specification {
<user name="ADMIN" role="administrator" password="c7ad44cbad762a5da0a452f9e854fdc1e0e7a52a38015f23f3eab1d80b931dd472634dfac71cd34ebc35d16ab7fb8a90c81f975113d6c7538dc69dd8de9077ec"/>
</authentication>

val userXML_empty: Elem = <authentication>
val userXML_empty: Elem = <authentication case-sensitivity="false">
<user name="admin" roles="administrator"/>
</authentication>

Expand All @@ -113,6 +113,27 @@ class RudderUserDetailsTest extends Specification {
(userDetailList.users.size must beEqualTo(0))
}

"retrieving user by name with case sensitivity" >> {
val userDetailListProvider = new UserDetailListProvider {
override def authConfig: ValidatedUserList = getUserDetailList(userXML_1, "userXML_1")

}

(userDetailListProvider.getUserByName("admin") must beRight) and (
userDetailListProvider.getUserByName("Admin") must beLeft
)
}

"retrieving user by name *without* case sensitivity" >> {
val userDetailListProvider = new UserDetailListProvider {
override def authConfig: ValidatedUserList = getUserDetailList(userXML_empty, "userXML_empty")
}

(userDetailListProvider.getUserByName("admin") must beRight) and (
userDetailListProvider.getUserByName("Admin") must beRight
)
}

"an account without password field get a random 32 chars pass" >> {
val userDetailList = getUserDetailList(userXML_empty, "userXML_empty")

Expand Down

0 comments on commit 7cc61e3

Please sign in to comment.