Fix parent relation handling

changelog.d/3947.bugfix

@@ -0,0 +1 @@
+A removed room from a space can't be re-added as it won't be shown in add-room

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt

@@ -19,6 +19,8 @@ package org.matrix.android.sdk.common
 import android.content.Context
 import android.net.Uri
 import androidx.lifecycle.Observer
+import androidx.test.internal.runner.junit4.statement.UiThreadStatement
+import androidx.test.internal.runner.junit4.statement.UiThreadStatement.runOnUiThread
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.delay
@@ -59,13 +61,15 @@ class CommonTestHelper(context: Context) {
     fun getTestInterceptor(session: Session): MockOkHttpInterceptor? = TestNetworkModule.interceptorForSession(session.sessionId) as? MockOkHttpInterceptor
 
     init {
-        Matrix.initialize(
-                context,
-                MatrixConfiguration(
-                        applicationFlavor = "TestFlavor",
-                        roomDisplayNameFallbackProvider = TestRoomDisplayNameFallbackProvider()
-                )
-        )
+        UiThreadStatement.runOnUiThread {
+            Matrix.initialize(
+                    context,
+                    MatrixConfiguration(
+                            applicationFlavor = "TestFlavor",
+                            roomDisplayNameFallbackProvider = TestRoomDisplayNameFallbackProvider()
+                    )
+            )
+        }
         matrix = Matrix.getInstance(context)
     }
 

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/session/space/SpaceHierarchyTest.kt

@@ -32,10 +32,19 @@ import org.junit.runners.JUnit4
 import org.junit.runners.MethodSorters
 import org.matrix.android.sdk.InstrumentedTest
 import org.matrix.android.sdk.api.query.ActiveSpaceFilter
+import org.matrix.android.sdk.api.query.QueryStringValue
 import org.matrix.android.sdk.api.session.Session
+import org.matrix.android.sdk.api.session.events.model.EventType
+import org.matrix.android.sdk.api.session.events.model.toContent
+import org.matrix.android.sdk.api.session.events.model.toModel
+import org.matrix.android.sdk.api.session.room.model.PowerLevelsContent
+import org.matrix.android.sdk.api.session.room.model.RoomJoinRulesAllowEntry
 import org.matrix.android.sdk.api.session.room.model.RoomSummary
 import org.matrix.android.sdk.api.session.room.model.RoomType
 import org.matrix.android.sdk.api.session.room.model.create.CreateRoomParams
+import org.matrix.android.sdk.api.session.room.model.create.RestrictedRoomPreset
+import org.matrix.android.sdk.api.session.room.powerlevels.PowerLevelsHelper
+import org.matrix.android.sdk.api.session.room.powerlevels.Role
 import org.matrix.android.sdk.api.session.room.roomSummaryQueryParams
 import org.matrix.android.sdk.common.CommonTestHelper
 import org.matrix.android.sdk.common.SessionTestParams
@@ -386,6 +395,8 @@ class SpaceHierarchyTest : InstrumentedTest {
             // The room should have disapear from flat children
             GlobalScope.launch(Dispatchers.Main) { flatAChildren.observeForever(childObserver) }
         }
+
+        commonTestHelper.signOutAndClose(session)
     }
 
     data class TestSpaceCreationResult(
@@ -434,6 +445,57 @@ class SpaceHierarchyTest : InstrumentedTest {
         return TestSpaceCreationResult(spaceId, roomIds)
     }
 
+    @Suppress("EXPERIMENTAL_API_USAGE")
+    private fun createPrivateSpace(session: Session,
+                                   spaceName: String,
+                                   childInfo: List<Triple<String, Boolean, Boolean?>>
+            /** Name, auto-join, canonical*/
+    ): TestSpaceCreationResult {
+        var spaceId = ""
+        commonTestHelper.waitWithLatch {
+            GlobalScope.launch {
+                spaceId = session.spaceService().createSpace(spaceName, "My Private Space", null, false)
+                it.countDown()
+            }
+        }
+
+        val syncedSpace = session.spaceService().getSpace(spaceId)
+        val viaServers = listOf(session.sessionParams.homeServerHost ?: "")
+
+        val roomIds =
+                childInfo.map { entry ->
+                    var roomId = ""
+                    commonTestHelper.waitWithLatch {
+                        GlobalScope.launch {
+                            val homeServerCapabilities = session
+                                    .getHomeServerCapabilities()
+                            roomId = session.createRoom(CreateRoomParams().apply {
+                                name = entry.first
+                                this.featurePreset = RestrictedRoomPreset(
+                                        homeServerCapabilities,
+                                        listOf(
+                                                RoomJoinRulesAllowEntry.restrictedToRoom(spaceId)
+                                        )
+                                )
+                            })
+                            it.countDown()
+                        }
+                    }
+                    roomId
+                }
+
+        roomIds.forEachIndexed { index, roomId ->
+            runBlocking {
+                syncedSpace!!.addChildren(roomId, viaServers, null, childInfo[index].second)
+                val canonical = childInfo[index].third
+                if (canonical != null) {
+                    session.spaceService().setSpaceParent(roomId, spaceId, canonical, viaServers)
+                }
+            }
+        }
+        return TestSpaceCreationResult(spaceId, roomIds)
+    }
+
     @Test
     fun testRootSpaces() {
         val session = commonTestHelper.createAccount("John", SessionTestParams(true))
@@ -473,5 +535,111 @@ class SpaceHierarchyTest : InstrumentedTest {
         val rootSpaces = session.spaceService().getRootSpaceSummaries()
 
         assertEquals("Unexpected number of root spaces ${rootSpaces.map { it.name }}", 2, rootSpaces.size)
+
+        commonTestHelper.signOutAndClose(session)
+    }
+
+    @Test
+    fun testParentRelation() {
+        val aliceSession = commonTestHelper.createAccount("Alice", SessionTestParams(true))
+        val bobSession = commonTestHelper.createAccount("Bib", SessionTestParams(true))
+
+        val spaceAInfo = createPrivateSpace(aliceSession, "Private Space A", listOf(
+                Triple("General", true /*suggested*/, true/*canonical*/),
+                Triple("Random", true, true)
+        ))
+
+        commonTestHelper.runBlockingTest {
+            aliceSession.getRoom(spaceAInfo.spaceId)!!.invite(bobSession.myUserId, null)
+        }
+
+        commonTestHelper.runBlockingTest {
+            bobSession.joinRoom(spaceAInfo.spaceId, null, emptyList())
+        }
+
+        var bobRoomId = ""
+        commonTestHelper.waitWithLatch {
+            GlobalScope.launch {
+                bobRoomId = bobSession.createRoom(CreateRoomParams().apply { name = "A Bob Room" })
+                bobSession.getRoom(bobRoomId)!!.invite(aliceSession.myUserId)
+                it.countDown()
+            }
+        }
+
+        commonTestHelper.runBlockingTest {
+            aliceSession.joinRoom(bobRoomId)
+        }
+
+        commonTestHelper.waitWithLatch { latch ->
+            commonTestHelper.retryPeriodicallyWithLatch(latch) {
+                aliceSession.getRoomSummary(bobRoomId)?.membership?.isActive() == true
+            }
+        }
+
+        commonTestHelper.waitWithLatch {
+            GlobalScope.launch {
+                bobSession.spaceService().setSpaceParent(bobRoomId, spaceAInfo.spaceId, false, listOf(bobSession.sessionParams.homeServerHost ?: ""))
+                it.countDown()
+            }
+        }
+
+        commonTestHelper.waitWithLatch { latch ->
+            commonTestHelper.retryPeriodicallyWithLatch(latch) {
+                val stateEvent = aliceSession.getRoom(bobRoomId)!!.getStateEvent(EventType.STATE_SPACE_PARENT, QueryStringValue.Equals(spaceAInfo.spaceId))
+                stateEvent != null
+            }
+        }
+
+        // This should be an invalid space parent relation, because no opposite child and bob is not admin of the space
+        commonTestHelper.runBlockingTest {
+            // we can see the state event
+            // but it is not valid and room is not in hierarchy
+            assertTrue("Bob Room should not be listed as a child of the space", aliceSession.getRoomSummary(bobRoomId)?.flattenParentIds?.isEmpty() == true)
+        }
+
+        // Let's now try to make alice admin of the room
+
+        commonTestHelper.waitWithLatch {
+            GlobalScope.launch {
+                val room = bobSession.getRoom(bobRoomId)!!
+                val currentPLContent = room
+                        .getStateEvent(EventType.STATE_ROOM_POWER_LEVELS)
+                        ?.let { it.content.toModel<PowerLevelsContent>() }
+
+                val newPowerLevelsContent = currentPLContent
+                        ?.setUserPowerLevel(aliceSession.myUserId, Role.Admin.value)
+                        ?.toContent()
+
+                room.sendStateEvent(EventType.STATE_ROOM_POWER_LEVELS, null, newPowerLevelsContent!!)
+                it.countDown()
+            }
+        }
+
+        commonTestHelper.waitWithLatch { latch ->
+            commonTestHelper.retryPeriodicallyWithLatch(latch) {
+                val powerLevelsHelper = aliceSession.getRoom(bobRoomId)!!
+                        .getStateEvent(EventType.STATE_ROOM_POWER_LEVELS)
+                        ?.content
+                        ?.toModel<PowerLevelsContent>()
+                        ?.let { PowerLevelsHelper(it) }
+                powerLevelsHelper!!.isUserAllowedToSend(aliceSession.myUserId, true, EventType.STATE_SPACE_PARENT)
+            }
+        }
+
+        commonTestHelper.waitWithLatch {
+            GlobalScope.launch {
+                aliceSession.spaceService().setSpaceParent(bobRoomId, spaceAInfo.spaceId, false, listOf(bobSession.sessionParams.homeServerHost ?: ""))
+                it.countDown()
+            }
+        }
+
+        commonTestHelper.waitWithLatch { latch ->
+            commonTestHelper.retryPeriodicallyWithLatch(latch) {
+                bobSession.getRoomSummary(bobRoomId)?.flattenParentIds?.contains(spaceAInfo.spaceId) == true
+            }
+        }
+
+        commonTestHelper.signOutAndClose(aliceSession)
+        commonTestHelper.signOutAndClose(bobSession)
     }
 }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/space/SpaceService.kt

@@ -94,5 +94,7 @@ interface SpaceService {
      */
     suspend fun setSpaceParent(childRoomId: String, parentSpaceId: String, canonical: Boolean, viaServers: List<String>)
 
+    suspend fun removeSpaceParent(childRoomId: String, parentSpaceId: String)
+
     fun getRootSpaceSummaries(): List<RoomSummary>
 }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryUpdater.kt

@@ -23,6 +23,7 @@ import org.matrix.android.sdk.api.session.events.model.EventType
 import org.matrix.android.sdk.api.session.events.model.toModel
 import org.matrix.android.sdk.api.session.room.accountdata.RoomAccountDataTypes
 import org.matrix.android.sdk.api.session.room.model.Membership
+import org.matrix.android.sdk.api.session.room.model.PowerLevelsContent
 import org.matrix.android.sdk.api.session.room.model.RoomAliasesContent
 import org.matrix.android.sdk.api.session.room.model.RoomCanonicalAliasContent
 import org.matrix.android.sdk.api.session.room.model.RoomJoinRulesContent
@@ -31,6 +32,7 @@ import org.matrix.android.sdk.api.session.room.model.RoomTopicContent
 import org.matrix.android.sdk.api.session.room.model.RoomType
 import org.matrix.android.sdk.api.session.room.model.VersioningState
 import org.matrix.android.sdk.api.session.room.model.create.RoomCreateContent
+import org.matrix.android.sdk.api.session.room.powerlevels.PowerLevelsHelper
 import org.matrix.android.sdk.api.session.room.send.SendState
 import org.matrix.android.sdk.internal.crypto.EventDecryptor
 import org.matrix.android.sdk.internal.crypto.MXCRYPTO_ALGORITHM_MEGOLM
@@ -207,63 +209,101 @@ internal class RoomSummaryUpdater @Inject constructor(
                     }
                     .toMap()
 
-            lookupMap.keys.forEach { lookedUp ->
-                if (lookedUp.roomType == RoomType.SPACE) {
-                    // get childrens
+            // First handle child relations
+            lookupMap.keys.asSequence()
+                    .filter { it.roomType == RoomType.SPACE }
+                    .forEach { lookedUp ->
+                        // get childrens
 
-                    lookedUp.children.clearWith { it.deleteFromRealm() }
+                        lookedUp.children.clearWith { it.deleteFromRealm() }
 
-                    RoomChildRelationInfo(realm, lookedUp.roomId).getDirectChildrenDescriptions().forEach { child ->
+                        RoomChildRelationInfo(realm, lookedUp.roomId).getDirectChildrenDescriptions().forEach { child ->
 
-                        lookedUp.children.add(
-                                realm.createObject<SpaceChildSummaryEntity>().apply {
-                                    this.childRoomId = child.roomId
-                                    this.childSummaryEntity = RoomSummaryEntity.where(realm, child.roomId).findFirst()
-                                    this.order = child.order
+                            lookedUp.children.add(
+                                    realm.createObject<SpaceChildSummaryEntity>().apply {
+                                        this.childRoomId = child.roomId
+                                        this.childSummaryEntity = RoomSummaryEntity.where(realm, child.roomId).findFirst()
+                                        this.order = child.order
 //                                    this.autoJoin = child.autoJoin
-                                    this.viaServers.addAll(child.viaServers)
-                                }
-                        )
+                                        this.viaServers.addAll(child.viaServers)
+                                    }
+                            )
 
-                        RoomSummaryEntity.where(realm, child.roomId)
-                                .process(RoomSummaryEntityFields.MEMBERSHIP_STR, Membership.activeMemberships())
-                                .findFirst()
-                                ?.let { childSum ->
-                                    lookupMap.entries.firstOrNull { it.key.roomId == lookedUp.roomId }?.let { entry ->
-                                        if (entry.value.indexOfFirst { it.roomId == childSum.roomId } == -1) {
-                                            // add looked up as a parent
-                                            entry.value.add(childSum)
+                            RoomSummaryEntity.where(realm, child.roomId)
+                                    .process(RoomSummaryEntityFields.MEMBERSHIP_STR, Membership.activeMemberships())
+                                    .findFirst()
+                                    ?.let { childSum ->
+                                        lookupMap.entries.firstOrNull { it.key.roomId == lookedUp.roomId }?.let { entry ->
+                                            if (entry.value.indexOfFirst { it.roomId == childSum.roomId } == -1) {
+                                                // add looked up as a parent
+                                                entry.value.add(childSum)
+                                            }
                                         }
                                     }
+                        }
+                    }
+
+            // Now let's check parent relations
+
+            lookupMap.keys
+                    .forEach { lookedUp ->
+                        lookedUp.parents.clearWith { it.deleteFromRealm() }
+                        // can we check parent relations here??
+                        /**
+                         * rooms can claim parents via the m.space.parent state event.
+                         * canonical determines whether this is the main parent for the space.
+                         *
+                         * To avoid abuse where a room admin falsely claims that a room is part of a space that it should not be,
+                         * clients could ignore such m.space.parent events unless either
+                         * (a) there is a corresponding m.space.child event in the claimed parent, or
+                         * (b) the sender of the m.space.child event has a sufficient power-level to send such an m.space.child event in the parent.
+                         * (It is not necessarily required that that user currently be a member of the parent room - only the m.room.power_levels event is inspected.)
+                         * [Checking the power-level rather than requiring an actual m.space.child event in the parent allows for "secret" rooms (see below).]
+                         */
+                        RoomChildRelationInfo(realm, lookedUp.roomId).getParentDescriptions()
+                                .map { parentInfo ->
+                                    // Is it a valid parent relation?
+                                    // Check if it's a child of the parent?
+                                    var isValidRelation = false
+                                    val parent = lookupMap.firstNotNullOfOrNull { if (it.key.roomId == parentInfo.roomId) it.value else null }
+                                    if (parent?.firstOrNull { it.roomId == lookedUp.roomId } != null) {
+                                        // there is a corresponding m.space.child event in the claimed parent
+                                        isValidRelation = true
+                                    } else {
+                                        // check if sender can post child relation in parent?
+                                        val senderId = parentInfo.stateEventSender
+                                        val parentRoomId = parentInfo.roomId
+                                        val powerLevelsHelper = CurrentStateEventEntity
+                                                .getOrNull(realm, parentRoomId, "", EventType.STATE_ROOM_POWER_LEVELS)
+                                                ?.root
+                                                ?.let { ContentMapper.map(it.content).toModel<PowerLevelsContent>() }
+                                                ?.let { PowerLevelsHelper(it) }
+
+                                        isValidRelation = powerLevelsHelper?.isUserAllowedToSend(senderId, true, EventType.STATE_SPACE_CHILD) ?: false
+                                    }
+
+                                    if (isValidRelation) {
+                                        lookedUp.parents.add(
+                                                realm.createObject<SpaceParentSummaryEntity>().apply {
+                                                    this.parentRoomId = parentInfo.roomId
+                                                    this.parentSummaryEntity = RoomSummaryEntity.where(realm, parentInfo.roomId).findFirst()
+                                                    this.canonical = parentInfo.canonical
+                                                    this.viaServers.addAll(parentInfo.viaServers)
+                                                }
+                                        )
+
+                                        RoomSummaryEntity.where(realm, parentInfo.roomId)
+                                                .process(RoomSummaryEntityFields.MEMBERSHIP_STR, Membership.activeMemberships())
+                                                .findFirst()
+                                                ?.let { parentSum ->
+                                                    if (lookupMap[parentSum]?.indexOfFirst { it.roomId == lookedUp.roomId } == -1) {
+                                                        // add lookedup as a parent
+                                                        lookupMap[parentSum]?.add(lookedUp)
+                                                    }
+                                                }
+                                    }
                                 }
                     }
-                } else {
-                    lookedUp.parents.clearWith { it.deleteFromRealm() }
-                    // can we check parent relations here??
-                    RoomChildRelationInfo(realm, lookedUp.roomId).getParentDescriptions()
-                            .map { parentInfo ->
-
-                                lookedUp.parents.add(
-                                        realm.createObject<SpaceParentSummaryEntity>().apply {
-                                            this.parentRoomId = parentInfo.roomId
-                                            this.parentSummaryEntity = RoomSummaryEntity.where(realm, parentInfo.roomId).findFirst()
-                                            this.canonical = parentInfo.canonical
-                                            this.viaServers.addAll(parentInfo.viaServers)
-                                        }
-                                )
-
-                                RoomSummaryEntity.where(realm, parentInfo.roomId)
-                                        .process(RoomSummaryEntityFields.MEMBERSHIP_STR, Membership.activeMemberships())
-                                        .findFirst()
-                                        ?.let { parentSum ->
-                                            if (lookupMap[parentSum]?.indexOfFirst { it.roomId == lookedUp.roomId } == -1) {
-                                                // add lookedup as a parent
-                                                lookupMap[parentSum]?.add(lookedUp)
-                                            }
-                                        }
-                            }
-                }
-            }
 
             // Simple algorithm to break cycles
             // Need more work to decide how to break, probably need to be as consistent as possible

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/space/DefaultSpace.kt

@@ -89,7 +89,6 @@ internal class DefaultSpace(
                 body = SpaceChildContent(
                         order = null,
                         via = null,
-//                        autoJoin = null,
                         suggested = null
                 ).toContent()
         )

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/space/DefaultSpaceService.kt

@@ -222,4 +222,23 @@ internal class DefaultSpaceService @Inject constructor(
                 ).toContent()
         )
     }
+
+    override suspend fun removeSpaceParent(childRoomId: String, parentSpaceId: String) {
+        val room = roomGetter.getRoom(childRoomId)
+                ?: throw IllegalArgumentException("Unknown Room $childRoomId")
+
+        val existingEvent = room.getStateEvent(EventType.STATE_SPACE_PARENT, QueryStringValue.Equals(parentSpaceId))
+        if (existingEvent != null) {
+            // Should i check if it was sent by me?
+            // we don't check power level, it will throw if you cannot do that
+            room.sendStateEvent(
+                    eventType = EventType.STATE_SPACE_PARENT,
+                    stateKey = parentSpaceId,
+                    body = SpaceParentContent(
+                            via = null,
+                            canonical = null
+                    ).toContent()
+            )
+        }
+    }
 }

vector/src/main/java/im/vector/app/features/command/Command.kt

@@ -48,7 +48,7 @@ enum class Command(val command: String, val parameters: String, @StringRes val d
     CONFETTI("/confetti", "<message>", R.string.command_confetti, false),
     SNOWFALL("/snowfall", "<message>", R.string.command_snow, false),
     CREATE_SPACE("/createspace", "<name> <invitee>*", R.string.command_description_create_space,  true),
-    ADD_TO_SPACE("/addToSpace", "spaceId", R.string.command_description_create_space, true),
+    ADD_TO_SPACE("/addToSpace", "spaceId", R.string.command_description_add_to_space, true),
     JOIN_SPACE("/joinSpace", "spaceId", R.string.command_description_join_space, true),
     LEAVE_ROOM("/leave", "<roomId?>", R.string.command_description_leave_room, true),
     UPGRADE_ROOM("/upgraderoom", "newVersion", R.string.command_description_upgrade_room, true);

vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsViewModel.kt

@@ -33,6 +33,7 @@ import im.vector.app.core.platform.VectorViewModel
 import im.vector.app.features.session.coroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
+import org.matrix.android.sdk.api.extensions.tryOrNull
 import org.matrix.android.sdk.api.session.Session
 
 class SpaceManageRoomsViewModel @AssistedInject constructor(
@@ -104,6 +105,10 @@ class SpaceManageRoomsViewModel @AssistedInject constructor(
                 } catch (failure: Throwable) {
                     errorList.add(failure)
                 }
+                tryOrNull {
+                    // also remove space parent if any? and if I can
+                    session.spaceService().removeSpaceParent(it, state.spaceId)
+                }
             }
             if (errorList.isEmpty()) {
                 // success

vector/src/main/res/values/strings.xml

@@ -3399,6 +3399,7 @@
     <string name="dev_tools_event_content_hint">Event content</string>
 
     <string name="command_description_create_space">Create a Space</string>
+    <string name="command_description_add_to_space">Add to the given Space</string>
     <string name="command_description_join_space">Join the Space with the given id</string>
     <string name="command_description_leave_room">Leave room with given id (or current room if null)</string>
     <string name="command_description_upgrade_room">Upgrades a room to a new version</string>