diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/users/UserRepository.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/users/UserRepository.scala index 2be4fa520d7..73b732ce73e 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/users/UserRepository.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/users/UserRepository.scala @@ -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)) @@ -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 { diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/users/UserRepositoryTest.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/users/UserRepositoryTest.scala index faaa4351dd0..41d9443f08f 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/users/UserRepositoryTest.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/users/UserRepositoryTest.scala @@ -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") @@ -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) @@ -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) @@ -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) @@ -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, @@ -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) @@ -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 = { /* @@ -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"))) @@ -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 }