Closed Bug 1172360 Opened 5 years ago Closed 5 years ago

Handle join MUC command without passing server domain in XMPP

Categories

(Chat Core :: XMPP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: abdelrahman, Assigned: abdelrahman)

References

Details

Attachments

(1 file, 3 obsolete files)

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").
Depends on: 1010342
Assignee: nobody → a.ahmed1026
Summary: Handle in _parseJID if domain is not provided → Handle join MUC command without passing server domain in XMPP
Attachment #8626300 - Flags: review?(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.
Attachment #8626300 - Flags: review?(aleth) → review-
(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.
(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.
(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 ]
(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 ]
Attachment #8626300 - Attachment is obsolete: true
Attachment #8626461 - Flags: review?(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.
Attachment #8626461 - Flags: review?(aleth) → review-
Attachment #8626461 - Attachment is obsolete: true
Attachment #8626732 - Flags: review?(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!
Attachment #8626732 - Flags: review?(aleth) → review-
Attachment #8626732 - Attachment is obsolete: true
Comment on attachment 8626765 [details] [diff] [review]
rev 4 - handle join without server domain

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

Thanks!
Attachment #8626765 - Flags: review?(aleth) → review+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.