Skip to content

Commit

Permalink
Fixes #25825: Deleted OIDC user are reactivated after new provisioning
Browse files Browse the repository at this point in the history
  • Loading branch information
@clarktsiory @VinceMacBuche
clarktsiory authored and VinceMacBuche committed Nov 7, 2024
1 parent 9a3f5f5 commit 837058e
Show file tree
Hide file tree
Showing 2 changed files with 138 additions and 94 deletions.
4 changes: 2 additions & 2 deletions ...sources/rudder/rudder-core/src/main/scala/com/normation/rudder/users/UserRepository.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ class InMemoryUserRepository(userBase: Ref[Map[String, UserInfo]], sessionBase:
override def addUser(origin: String, user: String, trace: EventTrace, isCaseSensitive: Boolean): IOResult[Boolean] = {
userBase.get
.flatMap(m => {
val current = m.collect { case (k, u) if u.managedBy == origin => k }.toList
val current = m.collect { case (k, u) if u.managedBy == origin && u.status != UserStatus.Deleted => k }.toList
setExistingUsers(origin, current :+ user, trace, isCaseSensitive)
})
.map(!_.contains(user))
Expand Down Expand Up @@ -752,7 +752,7 @@ class JdbcUserRepository(doobie: Doobie) extends UserRepository {
override def addUser(origin: String, user: String, trace: EventTrace, isCaseSensitive: Boolean): IOResult[Boolean] = {
transactIOResult(s"Error when adding user '${user}' from '${origin}'") { xa =>
(for {
current <- fr"select id from users where managedby = ${origin}".query[String].to[List]
current <- fr"select id from users where managedby = ${origin} and status <> 'deleted'".query[String].to[List]
added = current :+ user
notAdded <- setUsers(origin, added, trace, isCaseSensitive)
} yield {
Expand Down
228 changes: 136 additions & 92 deletions ...ces/rudder/rudder-core/src/test/scala/com/normation/rudder/users/UserRepositoryTest.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,58 @@ trait UserRepositoryTest extends Specification with Loggable {
})
}

"Creating users in an empty repo should create them all activated" >> {

// not sure if we should have an empty json for otherInfo
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, users, traceInit).runNow
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
}
"Setting users should handle case sensitivity parameter false value and return non-updated users" >> {
val notUpdated = repo
.setExistingUsers(
AUTH_PLUGIN_NAME_LOCAL,
users.map {
case "alice" => "AlIcE" // the update will not occur
case o => o
},
traceInit,
isCaseSensitive = false
)
.runNow
notUpdated must containTheSameElementsAs(List("alice"))
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosInit)
}
"Setting users should handle case sensitivity parameter true value" >> {
// even if "alice" already exist
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, "Alice" :: users, traceInit, isCaseSensitive = true).runNow
repo.getAll().map(_.map(_.id)).runNow must containTheSameElementsAs("Alice" :: users)
}

"Getting user should raise an error for multiple found users" >> {
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, None, traceInit) *> repo.purge(
List("Alice"),
None,
Nil,
traceInit
)).runNow must beEqualTo(
List("Alice")
)
}

// BOB REMOVED
val userFileBobRemoved = users.filterNot(_ == "bob")
val dateBobRemoved = DateTime.parse("2023-09-02T02:02:02Z")
Expand All @@ -289,6 +341,11 @@ trait UserRepositoryTest extends Specification with Loggable {
}
}

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

// BOB is added again to the active users and is 'active'
val dateBobReactivated = DateTime.parse("2023-09-02T02:03:03Z")
val traceBobReactivated = EventTrace(actor, dateBobReactivated)
Expand All @@ -310,6 +367,11 @@ trait UserRepositoryTest extends Specification with Loggable {
}
}

"Reactivating 'deleted' users by setting existing users" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, users, traceBobReactivated).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobReactivated)
}

// BOB is kept removed after setting the users again without BOB
val dateBobRemovedIdempotent = DateTime.parse("2023-09-02T02:03:04Z")
val traceBobRemovedIdempotent = EventTrace(actor, dateBobRemovedIdempotent)
Expand All @@ -330,6 +392,13 @@ trait UserRepositoryTest extends Specification with Loggable {
}
}

"If an user is reloaded from the same origin, it should be kept as is" >> {
// removing it again first, then a second time to check idempotency
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, userFileBobRemoved, traceBobRemovedIdempotent).runNow
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, userFileBobRemoved, traceBobRemovedIdempotent).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobRemovedIdempotent)
}

// BOB added back from OIDC
val dateBobOidc = DateTime.parse("2023-09-03T03:03:03Z")
val traceBobOidc = EventTrace(actor, dateBobOidc)
Expand All @@ -355,11 +424,16 @@ trait UserRepositoryTest extends Specification with Loggable {
}
}

"If an user is created by an other module and file reloaded, it's Active and remains so" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_REMOTE, List("bob"), traceBobOidc).runNow
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, userFileBobRemoved, traceReload).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobOidc)
}

// William is added from OIDC
val dateWilliamOidc = DateTime.parse("2023-09-05T05:05:05Z")
val traceWilliamOidc = EventTrace(actor, dateWilliamOidc)
val userInfosWilliamOidc = {
// Alice is added along with Bob
userInfosBobOidc :+ UserInfo(
"william",
dateWilliamOidc,
Expand All @@ -373,12 +447,62 @@ trait UserRepositoryTest extends Specification with Loggable {
)
}

"If an user is also added in the other module" >> {
repo.addUser(AUTH_PLUGIN_NAME_REMOTE, "william", traceWilliamOidc).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosWilliamOidc)
}

// Bob is disabled
val dateBobDisabled = DateTime.parse("2023-09-06T06:06:06Z")
val traceBobDisabled = EventTrace(actor, dateBobDisabled)
val userInfosBobDisabled = {
userInfosWilliamOidc.map {
case u if (u.id == "bob") =>
u.modify(_.status)
.setTo(UserStatus.Disabled)
.modify(_.statusHistory)
.using(StatusHistory(UserStatus.Disabled, traceBobDisabled) :: _)

case u => u
}
}

"some user is disabled" >> {
val disabled = repo.disable(List("bob"), None, List.empty, traceBobDisabled).runNow
(disabled must containTheSameElementsAs(List("bob"))) and (
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobDisabled)
)
}

// Xavier is added from OIDC
val dateXavierOidc = DateTime.parse("2023-09-07T07:07:07Z")
val traceXavierOidc = EventTrace(actor, dateXavierOidc)
val userInfosXavierOidc = {
userInfosBobDisabled :+ UserInfo(
"xavier",
dateXavierOidc,
UserStatus.Active,
AUTH_PLUGIN_NAME_REMOTE,
None,
None,
None,
StatusHistory(UserStatus.Active, traceXavierOidc) :: Nil,
Json.Obj()
)
}

"Some user is added in remote with some other remote users disabled/deleted" >> {
// Bob: disabled, William: deleted, adding a user should not change the state of existing ones
val isAdded = repo.addUser(AUTH_PLUGIN_NAME_REMOTE, "xavier", traceXavierOidc).runNow
isAdded must beTrue and (repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosXavierOidc))
}

// All OIDC users deleted
val dateOidcDeleted = DateTime.parse("2023-09-06T06:06:06Z")
val dateOidcDeleted = DateTime.parse("2023-09-09T08:08:08Z")
val traceOidcDeleted = EventTrace(actor, dateOidcDeleted)
val userInfosOidcDeleted = {
userInfosWilliamOidc.map {
case u if u.id == "bob" || u.id == "william" =>
userInfosXavierOidc.map {
case u if Set("bob", "william", "xavier").contains(u.id) =>
u.modify(_.status)
.setTo(UserStatus.Deleted)
.modify(_.statusHistory)
Expand All @@ -388,8 +512,13 @@ trait UserRepositoryTest extends Specification with Loggable {
}
}

"If users are set to an empty list" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_REMOTE, List.empty, traceOidcDeleted).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosOidcDeleted)
}

// BOB deleted, then purged and added back in OIDC
val dateBobPurged = DateTime.parse("2023-09-07T07:07:07Z")
val dateBobPurged = DateTime.parse("2023-09-08T08:08:08Z")
val traceBobPurged = EventTrace(actor, dateBobPurged)
val userInfosBobPurged = {
/*
Expand All @@ -416,94 +545,9 @@ trait UserRepositoryTest extends Specification with Loggable {
}
}

"Creating users in an empty repo should create them all activated" >> {

// not sure if we should have an empty json for otherInfo
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, users, traceInit).runNow
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
}
"Setting users should handle case sensitivity parameter false value and return non-updated users" >> {
val notUpdated = repo
.setExistingUsers(
AUTH_PLUGIN_NAME_LOCAL,
users.map {
case "alice" => "AlIcE" // the update will not occur
case o => o
},
traceInit,
isCaseSensitive = false
)
.runNow
notUpdated must containTheSameElementsAs(List("alice"))
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosInit)
}
"Setting users should handle case sensitivity parameter true value" >> {
// even if "alice" already exist
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, "Alice" :: users, traceInit, isCaseSensitive = true).runNow
repo.getAll().map(_.map(_.id)).runNow must containTheSameElementsAs("Alice" :: users)
}

"Getting user should raise an error for multiple found users" >> {
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, None, 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)
}

"Reactivating 'deleted' users by setting existing users" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, users, traceBobReactivated).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobReactivated)
}

"If an user is reloaded from the same origin, it should be kept as is" >> {
// removing it again first, then a second time to check idempotency
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, userFileBobRemoved, traceBobRemovedIdempotent).runNow
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, userFileBobRemoved, traceBobRemovedIdempotent).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobRemovedIdempotent)
}

"If an user is created by an other module and file reloaded, it's Active and remains so" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_REMOTE, List("bob"), traceBobOidc).runNow
repo.setExistingUsers(AUTH_PLUGIN_NAME_LOCAL, userFileBobRemoved, traceReload).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosBobOidc)
}

"If an user is also added in the other module" >> {
repo.addUser(AUTH_PLUGIN_NAME_REMOTE, "william", traceWilliamOidc).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosWilliamOidc)
}

"If users are set to an empty list" >> {
repo.setExistingUsers(AUTH_PLUGIN_NAME_REMOTE, List.empty, traceOidcDeleted).runNow
repo.getAll().runNow.toUTC must containTheSameElementsAs(userInfosOidcDeleted)
}

"If an user is purged, then everything about it is lost and it is created fresh" >> {
// we only purge deleted users
(repo.delete(List("bob"), None, Nil, Some(UserStatus.Active), traceBobOidc) *>
(repo.delete(List("bob"), None, Nil, Some(UserStatus.Disabled), traceBobOidc) *>
repo.purge(List("bob"), None, Nil, traceBobOidc)).runNow
repo.getAll().runNow must beLike(_.map(_.id) must not(contain("bob")))

Expand Down Expand Up @@ -548,7 +592,7 @@ trait UserRepositoryTest extends Specification with Loggable {
repo
.purge(Nil, Some(dateInit.plusYears(1)), Nil, EventTrace(actor, DateTime.now()))
.tap(users => errors.effectUioUnit(logger.debug(s"Users were purged: ${users}")))
.runNow must containTheSameElementsAs(List("alice", "charlie", "mallory", "bob", "william"))
.runNow must containTheSameElementsAs(List("alice", "charlie", "mallory", "bob", "william", "xavier"))

repo.getAll().runNow must beEmpty
}
Expand Down

0 comments on commit 837058e

Please sign in to comment.