jarno
/
element-android
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #4000 from vector-im/feature/bca/fix_space_parent 

Fix parent relation handling
This commit is contained in:
Valere 2021-09-16 11:45:29 +02:00 committed by GitHub
parent 8b980e5227 22319a0f0a
commit 100ac49cac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 297 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
changelog.d/3947.bugfix Normal file
View File

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

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

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

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

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

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

@ -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>
}

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

@ -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,102 @@ 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?
val isValidRelation: Boolean
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

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

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

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

@ -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()
)
}
}
}

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

@ -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);

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

@ -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

1
vector/src/main/res/values/strings.xml
View File

@ -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>