Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #24839: We cannot login with a user login containing uppercase letter if the option case-sensitivity is set to false #5655

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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