Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update grammar on Matrix Ids to be more spec compliant and render error instead of infinite loading in room member list screen #3206

Merged
merged 9 commits into from
Jul 17, 2024
Merged

Update grammar on Matrix Ids to be more spec compliant and render error instead of infinite loading in room member list screen #3206

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
792667c
Iterate on MatrixPatterns.
bmarty Jul 16, 2024
5290007
Add test on MatrixPatterns functions.
bmarty Jul 16, 2024
5ae4668
No need to make the block optional.
bmarty Jul 16, 2024
79d29d6
Add extra test for room alias.
bmarty Jul 16, 2024
67e0929
Still need to support both eventId legacy and v4
bmarty Jul 16, 2024
d6c9987
Be more lenient on Matrix pattern to support existing rooms in the wi…
bmarty Jul 17, 2024
10717b5
Render errors in room member list view.
bmarty Jul 17, 2024
26aaa9e
Update screenshots
Jul 17, 2024
263c058
Fix quality and test compilation.
bmarty Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions ...matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/core/MatrixPatterns.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,41 +27,41 @@ object MatrixPatterns {
// Note: TLD is not mandatory (localhost, IP address...)
private const val DOMAIN_REGEX = ":[A-Za-z0-9.-]+(:[0-9]{2,5})?"

// See https://spec.matrix.org/v1.11/appendices/#opaque-identifiers
private const val OPAQUE_ID_REGEX = "[0-9A-Za-z-\\._~]+"

// regex pattern to find matrix user ids in a string.
// See https://matrix.org/docs/spec/appendices#historical-user-ids
// Sadly, we need to relax the regex pattern a bit as there already exist some ids that don't match the spec.
private const val MATRIX_USER_IDENTIFIER_REGEX = "^@.*?$DOMAIN_REGEX$"
private const val MATRIX_USER_IDENTIFIER_REGEX = "^@\\S+?$DOMAIN_REGEX$"
private val PATTERN_CONTAIN_MATRIX_USER_IDENTIFIER = MATRIX_USER_IDENTIFIER_REGEX.toRegex(RegexOption.IGNORE_CASE)

// regex pattern to find room ids in a string.
private const val MATRIX_ROOM_IDENTIFIER_REGEX = "![A-Z0-9.-]+$DOMAIN_REGEX"
// regex pattern to match room ids.
private const val MATRIX_ROOM_IDENTIFIER_REGEX = "^!$OPAQUE_ID_REGEX$DOMAIN_REGEX$"
bmarty marked this conversation as resolved.
Show resolved Hide resolved
private val PATTERN_CONTAIN_MATRIX_ROOM_IDENTIFIER = MATRIX_ROOM_IDENTIFIER_REGEX.toRegex(RegexOption.IGNORE_CASE)

// regex pattern to find room aliases in a string.
private const val MATRIX_ROOM_ALIAS_REGEX = "#[A-Z0-9._%#@=+-]+$DOMAIN_REGEX"
// regex pattern to match room aliases.
private const val MATRIX_ROOM_ALIAS_REGEX = "^#\\S+$DOMAIN_REGEX$"
private val PATTERN_CONTAIN_MATRIX_ALIAS = MATRIX_ROOM_ALIAS_REGEX.toRegex(RegexOption.IGNORE_CASE)

// regex pattern to find message ids in a string.
// regex pattern to match event ids.
// Sadly, we need to relax the regex pattern a bit as there already exist some ids that don't match the spec.
private const val MATRIX_EVENT_IDENTIFIER_REGEX = "^\\$.+$DOMAIN_REGEX$"
private const val MATRIX_EVENT_IDENTIFIER_REGEX = "^\\$$OPAQUE_ID_REGEX$DOMAIN_REGEX$"
private val PATTERN_CONTAIN_MATRIX_EVENT_IDENTIFIER = MATRIX_EVENT_IDENTIFIER_REGEX.toRegex(RegexOption.IGNORE_CASE)

// regex pattern to find message ids in a string.
private const val MATRIX_EVENT_IDENTIFIER_V3_REGEX = "\\$[A-Z0-9/+]+"
private val PATTERN_CONTAIN_MATRIX_EVENT_IDENTIFIER_V3 = MATRIX_EVENT_IDENTIFIER_V3_REGEX.toRegex(RegexOption.IGNORE_CASE)

// Ref: https://matrix.org/docs/spec/rooms/v4#event-ids
private const val MATRIX_EVENT_IDENTIFIER_V4_REGEX = "\\$[A-Z0-9\\-_]+"
private const val MATRIX_EVENT_IDENTIFIER_V4_REGEX = "\\$$OPAQUE_ID_REGEX"
private val PATTERN_CONTAIN_MATRIX_EVENT_IDENTIFIER_V4 = MATRIX_EVENT_IDENTIFIER_V4_REGEX.toRegex(RegexOption.IGNORE_CASE)
bmarty marked this conversation as resolved.
Show resolved Hide resolved

private const val MAX_IDENTIFIER_LENGTH = 255

/**
* Tells if a string is a valid user Id.
*
* @param str the string to test
* @return true if the string is a valid user id
*/
fun isUserId(str: String?): Boolean {
return str != null && str matches PATTERN_CONTAIN_MATRIX_USER_IDENTIFIER
return str != null && str matches PATTERN_CONTAIN_MATRIX_USER_IDENTIFIER && str.length <= MAX_IDENTIFIER_LENGTH
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note (not necessarily something that needs to be fixed): max identifier length is defined in utf-8 bytes, but I think this is counting the number of chars

}

/**
Expand All @@ -79,7 +79,7 @@ object MatrixPatterns {
* @return true if the string is a valid room Id
*/
fun isRoomId(str: String?): Boolean {
return str != null && str matches PATTERN_CONTAIN_MATRIX_ROOM_IDENTIFIER
return str != null && str matches PATTERN_CONTAIN_MATRIX_ROOM_IDENTIFIER && str.length <= MAX_IDENTIFIER_LENGTH
}

/**
Expand All @@ -89,7 +89,7 @@ object MatrixPatterns {
* @return true if the string is a valid room alias.
*/
fun isRoomAlias(str: String?): Boolean {
return str != null && str matches PATTERN_CONTAIN_MATRIX_ALIAS
return str != null && str matches PATTERN_CONTAIN_MATRIX_ALIAS && str.length <= MAX_IDENTIFIER_LENGTH
}

/**
Expand All @@ -101,8 +101,8 @@ object MatrixPatterns {
fun isEventId(str: String?): Boolean {
return str != null &&
(str matches PATTERN_CONTAIN_MATRIX_EVENT_IDENTIFIER ||
str matches PATTERN_CONTAIN_MATRIX_EVENT_IDENTIFIER_V3 ||
str matches PATTERN_CONTAIN_MATRIX_EVENT_IDENTIFIER_V4)
str matches PATTERN_CONTAIN_MATRIX_EVENT_IDENTIFIER_V4) &&
str.length <= MAX_IDENTIFIER_LENGTH
}

/**
Expand All @@ -118,8 +118,8 @@ object MatrixPatterns {
* Note not all cases are implemented.
*/
fun findPatterns(text: CharSequence, permalinkParser: PermalinkParser): List<MatrixPatternResult> {
val rawTextMatches = "\\S+?$DOMAIN_REGEX".toRegex(RegexOption.IGNORE_CASE).findAll(text)
val urlMatches = "\\[\\S+?\\]\\((\\S+?)\\)".toRegex(RegexOption.IGNORE_CASE).findAll(text)
val rawTextMatches = "\\S+$DOMAIN_REGEX".toRegex(RegexOption.IGNORE_CASE).findAll(text)
val urlMatches = "\\[\\S+\\]\\((\\S+)\\)".toRegex(RegexOption.IGNORE_CASE).findAll(text)
val atRoomMatches = Regex("@room").findAll(text)
return buildList {
for (match in rawTextMatches) {
Expand Down
72 changes: 71 additions & 1 deletion ...ix/api/src/test/kotlin/io/element/android/libraries/matrix/api/core/MatrixPatternsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import io.element.android.libraries.matrix.api.permalink.PermalinkParser
import org.junit.Test

class MatrixPatternsTest {
private val longLocalPart = "a".repeat(255 - ":server.com".length - 1)

@Test
fun `findPatterns - returns raw user ids`() {
val text = "A @user:server.com and @user2:server.com"
Expand Down Expand Up @@ -54,7 +56,7 @@ class MatrixPatternsTest {
}

@Test
fun `findPatterns - returns raw room event ids`() {
fun `findPatterns - returns raw event ids`() {
val text = "A \$event:server.com and \$event2:server.com"
val patterns = MatrixPatterns.findPatterns(text, aPermalinkParser())
assertThat(patterns).containsExactly(
Expand Down Expand Up @@ -90,6 +92,74 @@ class MatrixPatternsTest {
assertThat(patterns).containsExactly(MatrixPatternResult(MatrixPatternType.ROOM_ALIAS, "#room:server.com", 2, 46))
}

@Test
fun `test isRoomId`() {
assertThat(MatrixPatterns.isRoomId(null)).isFalse()
assertThat(MatrixPatterns.isRoomId("")).isFalse()
assertThat(MatrixPatterns.isRoomId("not a room id")).isFalse()
assertThat(MatrixPatterns.isRoomId(" !room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomId("!room:server.com ")).isFalse()
assertThat(MatrixPatterns.isRoomId("@room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomId("#room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomId("\$room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomId("!${longLocalPart}a:server.com")).isFalse()

assertThat(MatrixPatterns.isRoomId("!room:server.com")).isTrue()
assertThat(MatrixPatterns.isRoomId("!$longLocalPart:server.com")).isTrue()
}

@Test
fun `test isRoomAlias`() {
assertThat(MatrixPatterns.isRoomAlias(null)).isFalse()
assertThat(MatrixPatterns.isRoomAlias("")).isFalse()
assertThat(MatrixPatterns.isRoomAlias("not a room alias")).isFalse()
assertThat(MatrixPatterns.isRoomAlias(" #room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomAlias("#room:server.com ")).isFalse()
assertThat(MatrixPatterns.isRoomAlias("@room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomAlias("!room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomAlias("\$room:server.com")).isFalse()
assertThat(MatrixPatterns.isRoomAlias("#${longLocalPart}a:server.com")).isFalse()

assertThat(MatrixPatterns.isRoomAlias("#room:server.com")).isTrue()
assertThat(MatrixPatterns.isRoomAlias("#nico's-stickers:neko.dev")).isTrue()
assertThat(MatrixPatterns.isRoomAlias("#$longLocalPart:server.com")).isTrue()
}

@Test
fun `test isEventId`() {
assertThat(MatrixPatterns.isEventId(null)).isFalse()
assertThat(MatrixPatterns.isEventId("")).isFalse()
assertThat(MatrixPatterns.isEventId("not an event id")).isFalse()
assertThat(MatrixPatterns.isEventId(" \$event:server.com")).isFalse()
assertThat(MatrixPatterns.isEventId("\$event:server.com ")).isFalse()
assertThat(MatrixPatterns.isEventId("@event:server.com")).isFalse()
assertThat(MatrixPatterns.isEventId("!event:server.com")).isFalse()
assertThat(MatrixPatterns.isEventId("#event:server.com")).isFalse()
assertThat(MatrixPatterns.isEventId("$${longLocalPart}a:server.com")).isFalse()
assertThat(MatrixPatterns.isEventId("\$" + "a".repeat(255))).isFalse()

assertThat(MatrixPatterns.isEventId("\$event:server.com")).isTrue()
assertThat(MatrixPatterns.isEventId("$$longLocalPart:server.com")).isTrue()
assertThat(MatrixPatterns.isEventId("\$9BozuV4TBw6rfRW3rMEgZ5v-jNk1D6FA8Hd1OsWqT9k")).isTrue()
assertThat(MatrixPatterns.isEventId("\$" + "a".repeat(254))).isTrue()
}

@Test
fun `test isUserId`() {
assertThat(MatrixPatterns.isUserId(null)).isFalse()
assertThat(MatrixPatterns.isUserId("")).isFalse()
assertThat(MatrixPatterns.isUserId("not a user id")).isFalse()
assertThat(MatrixPatterns.isUserId(" @user:server.com")).isFalse()
assertThat(MatrixPatterns.isUserId("@user:server.com ")).isFalse()
assertThat(MatrixPatterns.isUserId("!user:server.com")).isFalse()
assertThat(MatrixPatterns.isUserId("#user:server.com")).isFalse()
assertThat(MatrixPatterns.isUserId("\$user:server.com")).isFalse()
assertThat(MatrixPatterns.isUserId("@${longLocalPart}a:server.com")).isFalse()

assertThat(MatrixPatterns.isUserId("@user:server.com")).isTrue()
assertThat(MatrixPatterns.isUserId("@$longLocalPart:server.com")).isTrue()
}

private fun aPermalinkParser(block: (String) -> PermalinkData = { PermalinkData.FallbackLink(Uri.EMPTY) }) = object : PermalinkParser {
override fun parse(uriString: String): PermalinkData {
return block(uriString)
Expand Down
Loading