Merge pull request #14 from jducoeur/QI.9v5kfif

[QI.9v5kfif] Fix problem with removing multiple guests at a time

querki/scala/shared/src/main/scala/querki/security/SecurityFunctions.scala

@@ -22,6 +22,11 @@ trait SecurityFunctions {
    * Fetch all of the members and invitees of this Space.
    */
   def getMembers():Future[(Seq[PersonInfo], Seq[PersonInfo])]
+
+  /**
+   * Fetch the Person info for the current User in the current Space, if they are a member of it.
+   */
+  def getMyInfo(): Future[Option[PersonInfo]]
 
   /**
    * Invite people to join this Space. This may be any number of invitees by email address and any

querki/scalajvm/app/querki/security/SecurityFunctionsImpl.scala

@@ -54,19 +54,24 @@ class SecurityFunctionsImpl(info:AutowireParams)(implicit e:Ecology) extends Spa
       case head :: tail :: _ => (head, tail)
     }
   }
-
+
+  def toPersonInfo(person:Thing):Future[PersonInfo] = {
+    ClientApi.thingInfo(person, rc).map(PersonInfo(_, AccessControl.personRoles(person).map(role => oid2tid(role.id))))
+  }
+
   def getMembers():Future[(Seq[PersonInfo], Seq[PersonInfo])] = {
-
-    def toPersonInfo(person:Thing):Future[PersonInfo] = {
-      ClientApi.thingInfo(person, rc).map(PersonInfo(_, AccessControl.personRoles(person).map(role => oid2tid(role.id))))
-    }
-
     for {
       members <- Future.sequence(Person.members(state).toSeq.map(toPersonInfo(_)))
       invitees <- Future.sequence(Person.invitees(state).toSeq.map(toPersonInfo(_)))
     }
       yield (members, invitees)
   }
+
+  def getMyInfo(): Future[Option[PersonInfo]] = {
+    Person.localPerson(user)
+      .map(personThing => toPersonInfo(personThing).map(Some(_)))
+      .getOrElse(Future.successful(None))
+  }
 
   lazy val maxMembers = Config.getInt("querki.public.maxMembersPerSpace", 100)
 

querki/scalajvm/app/querki/spaces/SpaceMembersActor.scala

@@ -102,11 +102,10 @@ private [spaces] class SpaceMembersActor(e:Ecology, val spaceId:OID, val spaceRo
 				if (AccessControl.isManager(rc.requesterOrAnon, state)) {
 					val resultRM = (RequestM.successful(true) /: memberIds) { (last, memberId) =>
 						last.flatMap { soFar =>
-							if (soFar) {
-								Person.removePerson(rc, memberId)(state, this)
-							} else {
-								RequestM.successful(false)
-							}
+							// Accumulate the results: return true iff all of these return true.
+							// Do we care about these results? Not sure, but QI.9v5kfif was what
+							// happened when we got over-enthusiastic about interpreting them:
+  						Person.removePerson(rc, memberId)(state, this).map(_ && soFar)
 						}
 					}
 

querki/scalajvm/test/querki/security/SecurityMidFuncs.scala

@@ -9,6 +9,7 @@ import AllFuncs._
 import org.scalactic.source.Position
 import org.scalatest.Matchers._
 import querki.test.mid.ClientState.withUser
+import querki.test.mid.EmailTesting.inviteHashPre
 
 /**
  * Not included in AllFuncs, since the functions here are relatively specialized and I don't want to pollute that
@@ -29,6 +30,9 @@ trait SecurityMidFuncs {
 
   def getMembers(): TestOp[(Seq[PersonInfo], Seq[PersonInfo])] =
     TestOp.client { _[SecurityFunctions].getMembers().call() }
+
+  def getMyInfo(): TestOp[Option[PersonInfo]] =
+    TestOp.client { _[SecurityFunctions].getMyInfo().call() }
 
   def invite(emails:Seq[String], collabs:Seq[TID]): TestOp[InviteResponse] =
     TestOp.client { _[SecurityFunctions].invite(emails, collabs).call() }
@@ -67,20 +71,18 @@ trait SecurityMidFuncs {
       yield role
   }
 
-  def getPersonInfoFor(user: TestUser): TestOp[PersonInfo] = {
+  def getPersonInfoFor(user: TestUser)(implicit pos: Position): TestOp[PersonInfo] = {
     for {
       std <- getStd
-      userId <- ClientState.userId(user)
-      // Note that we grant the role to the *Person*, not the *User*. So we need to fetch the Person records, and find
-      // the desired one:
-      membersAndInvitees <- getMembers
-      members = membersAndInvitees._1
+      personInfoOpt <- withUser(user) {
+        for {
+          info <- getMyInfo()
+        }
+          yield info
+      }
     }
-      // TODO: wow, we actually have no good way of finding this! We have the user, and are trying to find the right person.
-      // The connection is via Identity, but we don't have that on either side. Should we provide a way to get the
-      // current user's Person ID in the current Space, as a better way to do this, instead of comparing display names?
-      yield members.find(_.person.displayName == user.display).getOrElse(throw new Exception(s"Couldn't find Space member '${user.base}' to grant Role to!"))
-  }
+      yield personInfoOpt.getOrElse(throw new Exception(s"No Person found for '${user.base}!'"))
+ }
 
   def grantRoleTo(user: TestUser, role: TID): TestOp[Unit] = {
     for {
@@ -104,14 +106,60 @@ trait SecurityMidFuncs {
       yield ()
   }
 
+  /**
+   * This mimics what happens in EditShareableInvite.
+   */
+  def createShareableLinkForRole(role: TID,
+                                 name: String,
+                                 requiresMembership: Boolean,
+                                 isOpenInvite: Boolean): TestOp[SharedLinkInfo] =
+  {
+    for {
+      std <- getStd
+      inviteThing <-
+        makeThing(
+          std.security.sharedInviteModel,
+          name,
+          std.security.inviteRoleLink :=> role.underlying,
+          std.security.inviteRequiresMembership :=> requiresMembership,
+          // TBD: EditShareableInvite has another clause here that I don't entirely understand:
+          std.security.isOpenInvite :=> isOpenInvite)
+      linkInfo <- getOneSharedLink(TOID(inviteThing.underlying))
+    }
+      yield linkInfo
+  }
+
+  /**
+   * Given a URL from [[getSharedLinkURL()]], this extracts the bit we care about.
+   */
+  def extractOpenInviteHash(link: String): String = {
+    val hashPos = link.indexOf(inviteHashPre) + inviteHashPre.length
+    link.substring(hashPos)
+  }
+
+  def setInstancePermsFor(thing: TID, perm: ThingInfo, to: ThingInfo): TestOp[Unit] = {
+    for {
+      perms <- permsFor(thing)
+      instancePermThing = perms.instancePermThing.get
+      _ <- changeProp(instancePermThing, perm :=> to)
+    }
+      yield ()
+  }
+
+  def setToMembersOnly(thing: TID): TestOp[Unit] = {
+    getStd.flatMap { std =>
+      setInstancePermsFor(thing, std.security.canReadPerm, std.security.members)
+    }
+  }
+
   /**
    * This (and the two convenience functions after it) are the easiest way to check read visibility of a particular
    * Thing for the specified user.
    */
   def checkNameRealityFor(shouldBeReal: Boolean, name: String, who: TestUser)(implicit position: Position): TestOp[Unit] = {
     for {
       check <- withUser(who) { getThingInfo(TID(name)) }
-      _ = assert(check.isTag != shouldBeReal, s"Expected $name to be ${if (shouldBeReal) "real" else "tag"}, and it wasn't!")
+      _ = assert(check.isTag != shouldBeReal, s"Expected $name to be ${if (shouldBeReal) "real" else "tag"} for ${who.display}, and it wasn't!")
     }
       yield ()
   }

querki/scalajvm/test/querki/security/SecurityMidTests.scala

@@ -143,9 +143,7 @@ object SecurityMidTests {
       instance <- makeThing(model, instanceName)
 
       // Make the Space members-only readable:
-      perms <- permsFor(space)
-      instancePermThing = perms.instancePermThing.get
-      _ <- changeProp(instancePermThing, std.security.canReadPerm :=> std.security.members)
+      _ <- setToMembersOnly(space)
 
       // Initially, the member isn't a member, and can't see it:
       _ <- checkNameIsMissingFor(instanceName, member)
@@ -157,12 +155,56 @@ object SecurityMidTests {
       // Remove the member, and confirm they can't see it again:
       personInfo <- getPersonInfoFor(member)
       _ <- removeFromSpace(personInfo.person.oid)
-      _ <- spew(s"Removed person")
       _ <- checkNameIsMissingFor(instanceName, member)
     }
       yield ()
 
   }
+
+  def qi9v5kfifRemoveMultipleGuests: TestOp[Unit] = {
+    val ownerName = "QI9v5kfif Owner"
+    val spaceName = "QI9v5kfif Space"
+
+    for {
+      _ <- step("Regression test for QI.9v5kfif")
+      std <- getStd
+      owner <- setupUserAndSpace(ownerName, spaceName)
+      spaceId <- clientSpaceId()
+
+      // Create a role with read access:
+      roleId <- createRole("Role Name", std.security.canReadPerm)
+      // Create an open invite with that role:
+      sharedLinkInfo <- createShareableLinkForRole(roleId, "The Invite", false, true)
+      linkUrl <- getSharedLinkURL(sharedLinkInfo.thingInfo.oid2)
+      hash = extractOpenInviteHash(linkUrl)
+
+      // The Space is members-only, so we can work with visibility:
+      _ <- setToMembersOnly(spaceId)
+
+      model <- makeModel("The Model")
+      instanceName = "The Instance"
+      instance <- makeThing(model, instanceName)
+      _ <- ClientState.cache
+
+      spaceInfo <- fetchSpaceInfo()
+      guest1 <- addGuest(hash, spaceInfo)
+      guest2 <- addGuest(hash, spaceInfo)
+
+      _ <- checkNameIsRealFor(instanceName, guest1)
+      _ <- checkNameIsRealFor(instanceName, guest2)
+
+      _ <- ClientState.switchToUser(owner)
+
+      guest1Info <- getPersonInfoFor(guest1)
+      guest2Info <- getPersonInfoFor(guest2)
+      _ <- removeFromSpace(guest1Info.person.oid, guest2Info.person.oid)
+
+      // This was where the bug showed up: only one of these two guests would actually be removed:
+      _ <- checkNameIsMissingFor(instanceName, guest1)
+      _ <- checkNameIsMissingFor(instanceName, guest2)
+    }
+      yield ()
+  }
 }
 
 @Slow
@@ -174,6 +216,7 @@ class SecurityMidTests extends MidTestBase {
       runTest(regressionTestQIbu6oeej)
       runTest(regressionTestQIbu6of67)
       runTest(qi7w4gbn6RemoveMembers)
+      runTest(qi9v5kfifRemoveMultipleGuests)
     }
   }
 }

querki/scalajvm/test/querki/test/mid/ClientState.scala

@@ -3,7 +3,8 @@ package querki.test.mid
 import play.api.mvc.{Result, Session}
 import monocle.Lens
 import monocle.macros.GenLens
-import querki.data.{SpaceInfo, UserInfo, TID}
+import org.scalactic.source.Position
+import querki.data.{TID, SpaceInfo, UserInfo}
 
 /**
  * Describes the state of the "client" -- the current User, their session info, the Space they are looking at, and
@@ -35,14 +36,28 @@ object ClientState {
   /**
    * Switches to a different active user. This user must have previously been cached!
    */
-  def switchToUser(user: TestUser): TestOp[Unit] = {
+  def switchToUser(user: TestUser)(implicit pos: Position): TestOp[Unit] = {
     for {
       _ <- cache
-      _ <- TestOp.update(state => TestState.clientL.set(state.clientCache(user.base))(state))
+      userState <- fetchUserState(user)
+      _ <- TestOp.update(state => TestState.clientL.set(userState)(state))
     }
       yield ()
   }
 
+  def fetchUserState(user: TestUser)(implicit pos: Position): TestOp[ClientState] = {
+    for {
+      cache <- TestOp.fetch(_.clientCache)
+      baseName = user.base
+      state <-
+        cache.get(baseName) match {
+          case Some(state) => TestOp.pure(state)
+          case None => TestOp.error(new Exception(s"Unable to find state for a user named '$baseName'!"))
+        }
+    }
+      yield state
+  }
+
   /**
     * Performs the specified operation with the specified user, and then restores the current user.
     */