1657797 - Join Chat appends Matrix server name preventing us to join a different homeserver

Status: RESOLVED FIXED

(Chat Core :: Matrix, defect, P1)

Description

[Danny Colin [:sdk]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

0. Open the Chat tab
1. Connect to your Matrix account (e.g. @username:matrix.org)
2. Click on "Join Chat" button
3. Enter a room name from a different homeserver (e.g. #maildev:mozilla.org)

Actual results:

The chat join a room called #room_name:server_name:account_server_name.tld (e.g. #maildev:mozilla.org:matrix.org)

Expected results:

If a server name is specified, Join Chat shouldn't append the account server name.

Comment 1

[Patrick Cloke [:clokep]]

I think Khushil had a patch for this?

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1657797_update-join-chat-matrix-0.patch

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 9170440 [details] [diff] [review]
Bug-1657797_update-join-chat-matrix-0.patch

Review of attachment 9170440 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/matrix/matrix.jsm
@@ +812,3 @@
>      // For the format of room id and alias, see the matrix documentation:
>      // https://matrix.org/docs/spec/intro.html#room-structure
>      // https://matrix.org/docs/spec/intro.html#room-aliases

Please update these links to:

https://matrix.org/docs/spec/appendices#room-ids-and-event-ids
https://matrix.org/docs/spec/appendices#room-aliases

@@ +812,5 @@
>      // For the format of room id and alias, see the matrix documentation:
>      // https://matrix.org/docs/spec/intro.html#room-structure
>      // https://matrix.org/docs/spec/intro.html#room-aliases
> +    let roomIdOrAlias = components.getValue("roomIdOrAlias").trim();
> +    if (roomIdOrAlias.startsWith("!")) {

I'm having trouble following what the code in this if-statement is doing. Can you add a comment?

@@ +828,2 @@
>      }
> +    let domain = this._client.getDomain();

I think we can just inline this below instead of using a separate variable.

@@ +832,3 @@
>      }
>  
> +    if (!roomIdOrAlias.startsWith("@") && !roomIdOrAlias.startsWith("#")) {

I do not believe that @ can be the first character of an alias or room ID.

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1657797_update-join-chat-matrix-1.patch

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1657797_update-join-chat-matrix-2.patch

Comment 6

[Patrick Cloke [:clokep]]

(In reply to Patrick Cloke [:clokep] from comment #3)

@@ +832,3 @@

 }

• if (!roomIdOrAlias.startsWith("@") && !roomIdOrAlias.startsWith("#")) {

I do not believe that @ can be the first character of an alias or room ID.

I see that you added some comments about this, but I don't see this anywhere in the specification? In fact, when I look at room IDs I see they all start with "!". So where is this idea of it starting with an "@" coming from? I don't see that in the UI of Element or anywhere else.

Comment 7

[Khushil Mistry [:khushil324]]

So how do we add/create rooms for direct conversations? getDirectConversation function creates the direct conversation rooms when we pass the userID. If id is starting with "@", means it's a user ID.

Comment 8

[Patrick Cloke [:clokep]]

(In reply to Khushil Mistry [:khushil324] from comment #7)

So how do we add/create rooms for direct conversations? getDirectConversation function creates the direct conversation rooms when we pass the userID. If id is starting with "@", means it's a user ID.

So you're not suggesting that this is meant to be a proper ID, but just a way to start a direct conversation with someone (similar to double clicking on their name)?

Comment 9

[Khushil Mistry [:khushil324]]

Yes, right.

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 9171149 [details] [diff] [review]
Bug-1657797_update-join-chat-matrix-2.patch

Review of attachment 9171149 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I asked for a couple of comments to be clarified and one block of code to be moved. Let me know if moving it doesn't make sense for some reason!

Sorry for the delay. :)

::: chat/protocols/matrix/matrix.jsm
@@ +816,5 @@
> +    // There will be following types of ids:
> +    // !fubIsJzeAcCcjYTQvm:mozilla.org => General room id.
> +    // #maildev:mozilla.org => Group Conversation room id.
> +    // @clokep:mozilla.org => Direct Conversation room id.
> +    if (roomIdOrAlias.startsWith("!")) {

I think we should be appending the domain before this if statement (so that things like !fubIsJzeAcCcjYTQvm will work).

@@ +838,4 @@
>      }
>  
> +    // If @ or # is not provided in the id, assume it as group conversation
> +    // and append "#".

Please update this to:

> If the ID does not start with @ or #, assume it is a group conversation and append #.

@@ +841,5 @@
> +    // and append "#".
> +    if (!roomIdOrAlias.startsWith("@") && !roomIdOrAlias.startsWith("#")) {
> +      roomIdOrAlias = "#" + roomIdOrAlias;
> +    }
> +    // If it's room-id is of type direct chat, open the direct conversation.

> If the ID starts with a @, it is a direct conversation.

@@ +845,5 @@
> +    // If it's room-id is of type direct chat, open the direct conversation.
> +    if (roomIdOrAlias.startsWith("@")) {
> +      return this.getDirectConversation(roomIdOrAlias);
> +    }
> +    // If it's room-id is of type group chat, open the group conversation.

> Otherwise, it is a group conversation.

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug-1657797_update-join-chat-matrix-3.patch

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/768dcaaaefe9
Update join chat functionality in Matrix. r=clokep DONTBUILD

Comment 13

[Magnus Melin [:mkmelin]]

Uplift to 78?

Comment 14

[Khushil Mistry [:khushil324]]

Comment on attachment 9175695 [details] [diff] [review]
Bug-1657797_update-join-chat-matrix-3.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Matrix Chat experience will be improved.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): Low

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9175695 [details] [diff] [review]
Bug-1657797_update-join-chat-matrix-3.patch

[Triage Comment]
Approved for esr78

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/47cf0ae22adc