1172360 - Handle join MUC command without passing server domain in XMPP

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

After landing of bug 1010342, we can obtain the domain if it is not passed to _parseJID in xmpp.jsm (e.g command join is used "/join room").

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - handle join without server domain

Comment 2

[aleth [:aleth]]

Comment on attachment 8626300 [details] [diff] [review]
rev 1 - handle join without server domain

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1456,5 @@
>        resource: match[3]
>      };
> +    let jid;
> +    if (!result.node && result.domain)
> +      [result.node, result.domain] = [result.domain, result.node];

Changing parseJid in this way for the convenience of /join is not a good idea. JIDs without nodes do happen (e.g. when sending/receiving service discovery messages) and we want to be sure to handle them correctly. JIDs without domains are (if I remember correctly) not allowed.

Instead, move this swap to the /join code.

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #2)
> Changing parseJid in this way for the convenience of /join is not a good
> idea. JIDs without nodes do happen (e.g. when sending/receiving service
> discovery messages) and we want to be sure to handle them correctly. JIDs
> without domains are (if I remember correctly) not allowed.
> 
> Instead, move this swap to the /join code.

This change is not only for join, but also for any case like (node/resource or node).
Because the regular expression parses node in domain and that is not correct as we do not provide "@" to be parsed as domain.

Comment 4

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #3)
> (In reply to aleth [:aleth] from comment #2)
> > Changing parseJid in this way for the convenience of /join is not a good
> > idea. JIDs without nodes do happen (e.g. when sending/receiving service
> > discovery messages) and we want to be sure to handle them correctly. JIDs
> > without domains are (if I remember correctly) not allowed.
> > 
> > Instead, move this swap to the /join code.
> 
> This change is not only for join, but also for any case like (node/resource
> or node).
> Because the regular expression parses node in domain and that is not correct
> as we do not provide "@" to be parsed as domain.

Aha! Well spotted. I didn't realize parseJid was broken.

Maybe the regex could be improved?

jid = [ localpart "@" ] domainpart [ "/" resourcepart ]

In any case, and as this is an important function, I think this is a perfect case for test-driven development. Please add an xpschell test for parseJid. You can write the test first and then change the code until your tests pass ;)

Documentation: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

The full spec is rfc 6122 btw.

Comment 5

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #4)
> > Because the regular expression parses node in domain and that is not correct
> > as we do not provide "@" to be parsed as domain.

Hmm, I'm not sure I understood you correctly here. Just to be clear, the @ is optional, the domainpart is not:
jid = [ localpart "@" ] domainpart [ "/" resourcepart ]

Comment 6

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #5)
> Hmm, I'm not sure I understood you correctly here. Just to be clear, the @
> is optional, the domainpart is not:
> jid = [ localpart "@" ] domainpart [ "/" resourcepart ]

Ah, so the regex works well for _parseJID, we need to add another parser beside join command to check if the domain part is provided and handle that otherwise.
jid = localpart [ "@" domainpart ] [ "/" resourcepart ]

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - handle join without server domain

Comment 8

[aleth [:aleth]]

Comment on attachment 8626461 [details] [diff] [review]
rev 2 - handle join without server domain

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

Don't forget to change the help string too!

It's great you've added a test despite not changing _parseJid! Please move the test to a separate bug though, as it's not related to this bug.

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +17,5 @@
>  
>  // Get account object.
>  function getAccount(aConv) getConv(aConv)._account;
>  
> +function parseJID(aJID, aMucService) {

You don't need all this duplication if instead you change parseDefaultChatName on the account.

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - handle join without server domain

Comment 10

[aleth [:aleth]]

Comment on attachment 8626732 [details] [diff] [review]
rev 3 - handle join without server domain

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

Looks good, but please fix the help string!

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - handle join without server domain

Comment 12

[aleth [:aleth]]

Comment on attachment 8626765 [details] [diff] [review]
rev 4 - handle join without server domain

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

Thanks!

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/59ae2306da56