Closed Bug 954728 Opened 10 years ago Closed 10 years ago

Unhandled IRC whois response messages 307 671 317

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(2 files, 2 obsolete files)

*** Original post on bio 1296 at 2012-02-28 16:35:00 UTC ***

:concrete.mozilla.org 307 testib flo :is a registered nick
:concrete.mozilla.org 671 testib flo :is using a Secure Connection

Unfortunately these are not standard messages, so I need to look into what daemons support these and if there's any conflicts with these numerics.
*** Original post on bio 1296 at 2012-02-28 16:36:51 UTC ***

Additionally:
- The current parsing of the 317 reply seems incomplete, the Mozilla server for example sends:
:concrete.mozilla.org 317 testib flo 1569 1329906345 :seconds idle, signon time

This seems to be a non-standard form of this reply that I'll need to look into.
Summary: Unhanded IRC whois messages → Unhandled IRC whois response messages 307 671 317
*** Original post on bio 1296 at 2012-05-10 09:46:43 UTC ***

The same bug was filed for Thunderbird at https://bugzilla.mozilla.org/show_bug.cgi?id=753690

An additional unhandled 378 message is mentioned there:
Warning: Unhandled IRC message: :gravel.mozilla.org 378 nONoNonO nONoNonO :is connecting from *@a168-15.amc.nl 145.117.168.15
Source File: resource:///components/irc.js
Line: 334
See Also: → 753690
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1296 as attmnt 1640 at 2012-06-20 01:10:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353397 - Flags: review?(bugzilla)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353397 [details] [diff] [review]
Patch v1

*** Original change on bio 1296 attmnt 1640 at 2012-06-20 08:30:41 UTC ***

Looks good :)

>+ * handler here should have a comment saying what server are known to support

typo: servers

>+function setWhoIs(aAccount, aMessage, aFields) {
>+  let buddyName = aAccount.normalize(aMessage.params[1], aAccount.userPrefixes);
>+  // If the buddy isn't in the list yet, add it.
>+  if (!hasOwnProperty(aAccount.whoisInformation, buddyName))
>+    aAccount.whoisInformation[buddyName] = {};
>+
>+  // Set non-normalized nickname field.
>+  aAccount.whoisInformation[buddyName]["nick"] = aMessage.params[1];
>+
>+  // Set the WHOIS fields.
>+  for (let field in aFields)
>+    aAccount.whoisInformation[buddyName][field] = aFields[field];
>+
>+  return true;
>+}

Would it make more sense to move this to the account instead? It seems aAccount wants to be 'this'.
Attachment #8353397 - Flags: review?(bugzilla) → review+
Comment on attachment 8353397 [details] [diff] [review]
Patch v1

*** Original change on bio 1296 attmnt 1640 at 2012-06-20 12:39:56 UTC ***

(In reply to comment #4)
> >+function setWhoIs(aAccount, aMessage, aFields) {
[...]
> >+}
> 
> Would it make more sense to move this to the account instead? It seems aAccount
> wants to be 'this'.

I had been wondering this too...I don't particularly like having functions on the account which take a full message object, however. But maybe this is a bit over abstracted and we should add an account method that is:
setWhois(aNick, aFields)

We would need to pass in aMessage.params[1] in most (all?) situations, but it seems more versatile. :)

>diff --git a/chat/protocols/irc/ircNonStandard.jsm b/chat/protocols/irc/ircNonStandard.jsm
>+/*
>+ * This contains the implementation for non-standard extensions to IRC. Some of
>+ * these might have real documentation while others are reverse engineered. Each
>+ * handler here should have a comment saying what server are known to support
>+ * this extension.
This paragraph is pretty awful and needs to be reworded to not sound like it was written by a 5th grader. :)

>+ *
>+ * Resources for these commands include:
>+ *  http://hg.unrealircd.com/hg/unreal/raw-file/tip/include/numeric.h
This should include other resources...

>+const EXPORTED_SYMBOLS = ["ircNonStandard"];
>+
>+const {interfaces: Ci, utils: Cu} = Components;
Ci isn't used and should be removed.

>+  isEnabled: function() true,
I did wonder if we should somehow have seperate handlers for each server (i.e. Unreal, etc.) and then parse the response of 002 to see what the server is running. But it seemed better to just let everything through so we're not duplicating code for situations where multiple servers support the same extension.

>+    "317": function(aMessage) { // RPL_WHOISIDLE (Unreal)
>+      // <nick> <integer> <integer> :seconds idle, signon time
>+      // This is a non-standard extension to RPL_WHOISIDLE which includes the
>+      // sign-on time.
>+      return false;
We should probably just save the extra information (the sign-on time), even if we don't really use it.
Attachment #8353397 - Flags: review-
Attached patch Patch v2Splinter Review
*** Original post on bio 1296 as attmnt 1655 at 2012-06-21 01:58:00 UTC ***

Adds setWhois to the account object and cleans up some comments.
Attachment #8353412 - Flags: review?(florian)
Comment on attachment 8353397 [details] [diff] [review]
Patch v1

*** Original change on bio 1296 attmnt 1640 at 2012-06-21 01:58:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353397 - Attachment is obsolete: true
*** Original post on bio 1296 at 2012-06-21 11:14:48 UTC ***

Comment on attachment 8353412 [details] [diff] [review] (bio-attmnt 1655)
Patch v2

The code seems OK, although I've only skimmed it, trusting aleth's previous review.

I'm wondering about the UI though.

Am I right that this patch adds in all cases 2 lines to the tooltip:
Registered [yes/no]
Using a secure connection [yes/no]
?

Couldn't we merge these and give something more meaningful?

Identity   Connected from a secure connection and identified
           Connected from a secure connection
           Identified, but from an insecure connection
           Unknown

Maybe "Security" instead of "Identity"?

Not sure if these wordings are better, but I think we should (and can) help the user understand the information.
*** Original post on bio 1296 at 2012-06-21 11:39:33 UTC ***

Just throwing this into the mix:

Maybe we should add the "secure" information to the "Connected to" or "from" entry? i.e. something like "Connected from (IP) via SSL". (Since we use 'SSL' in the account manager it may actually be less vague than 'secure', which might puzzle a user just as much.)

There seems to be no similar place to add the 'registered' flag, but I agree something like "Using a registered nickname" might be clearer than just "Registered". ("Identified", like "secure", might be a bit vague in its meaning).
*** Original post on bio 1296 at 2012-06-21 11:58:59 UTC ***

(In reply to comment #7)
> Comment on attachment 8353412 [details] [diff] [review] (bio-attmnt 1655) [details]
> Patch v2
> 
> The code seems OK, although I've only skimmed it, trusting aleth's previous
> review.
> 
> I'm wondering about the UI though.
> 
> Am I right that this patch adds in all cases 2 lines to the tooltip:
> Registered [yes/no]
> Using a secure connection [yes/no]
> ?
No, it only adds it for those users who we receive a response for. So it actually will only ever add:
Registered yes
Using a secure connection yes

It could be modified to add the no cases fairly easily.

> Couldn't we merge these and give something more meaningful?
[...snip...]
> Not sure if these wordings are better, but I think we should (and can) help the
> user understand the information.
Fair enough. I think aleth's idea in comment 8 might make more sense though, as then you're combining all the connection information into one place.
Attached patch Patch v2.1 (obsolete) — Splinter Review
*** Original post on bio 1296 as attmnt 1660 at 2012-06-22 21:00:00 UTC ***

Better strings.
Attachment #8353417 - Flags: review?(florian)
Comment on attachment 8353412 [details] [diff] [review]
Patch v2

*** Original change on bio 1296 attmnt 1655 at 2012-06-22 21:00:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353412 - Attachment is obsolete: true
Attachment #8353412 - Flags: review?(florian)
Comment on attachment 8353417 [details] [diff] [review]
Patch v2.1

*** Original change on bio 1296 attmnt 1660 at 2012-06-22 21:03:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353417 - Flags: review?(bugzilla)
*** Original post on bio 1296 at 2012-06-22 22:19:32 UTC ***

Comment on attachment 8353417 [details] [diff] [review] (bio-attmnt 1660)
Patch v2.1

>+#    This field will contain whether the user is identified with NickServ and
>+#    connected via SSL.

It may just be me, but I find this comment clearer than the actual strings.

>+tooltip.identityValue.registered=Identified, but from an insecure connection
>+tooltip.identityValue.secure=Using a secure connection
>+tooltip.identityValue.registeredAndSecure=Using a secure connection and identified
>+tooltip.identityValue.unknown=Unknown connection

Why display "unknown connection" at all? Just leave it out of the tooltip in that case.

Are we sure that all servers who tell us whether someone is registered also tell us if they are using SSL? (since the above is asymmetric: identityValue.registered says "insecure connection" but .secure does not say "not identified")

How does connecting via SSL have anything to do with identity? It's hard to come up with something better though. If the two bools are to be paired, maybe:
"Connected with: Registered nickname"
"Connected with: SSL (Secure connection)"
"Connected with: Registered nickname via SSL"

or maybe "Security" rather than "Identity", since knowing who you are talking to is who they claim to be is part of security?

Still don't think the ideal word(s) have been found.
*** Original post on bio 1296 at 2012-06-22 22:30:10 UTC ***

(In reply to comment #11)
> Comment on attachment 8353417 [details] [diff] [review] (bio-attmnt 1660) [details]
> Patch v2.1
> 
> >+#    This field will contain whether the user is identified with NickServ and
> >+#    connected via SSL.
> 
> It may just be me, but I find this comment clearer than the actual strings.

Clearer, but also longer. ;)
It's the point of localization notes to be clearer, btw :).

> >+tooltip.identityValue.registered=Identified, but from an insecure connection
> >+tooltip.identityValue.secure=Using a secure connection
> >+tooltip.identityValue.registeredAndSecure=Using a secure connection and identified
> >+tooltip.identityValue.unknown=Unknown connection
> 
> Why display "unknown connection" at all? Just leave it out of the tooltip in
> that case.

Because users tend to not notice something that's just missing.

> How does connecting via SSL have anything to do with identity?

The question we are trying to answer by providing that information (as I understand it) is "can I trust that I'm actually talking to the person who usually comes here with that nick?".

- If you are not authenticated with nickserv, you can be anybody.
- If you are not connected through SSL, the connection can be man-in-the-middle'ed, the person actually talking could be anybody.
*** Original post on bio 1296 at 2012-06-23 11:43:51 UTC ***

Another variation:
"Identified by: Registered nickname over a secure connection"
"Identified by: Registered nickname"
"Identified by: Secure connection"
But that doesn't quite fit for the last case (just SSL), so in that case maybe instead
"Connected with: Secure connection (SSL)"
The drawback is that it might be confusing if that is then "absent" in the registered+secure case.
Attached patch Patch v2.2Splinter Review
*** Original post on bio 1296 as attmnt 1686 at 2012-06-24 14:29:00 UTC ***

This has slightly different strings which takes into account whether we're using  a secure connection and whether the other person is identified for every situation.

The unfortunate thing (and why we should be careful about wording here) is that if we don't have the information...it doesn't necessarily mean they're not using a secure connection / aren't registered. It could just be that the server doesn't support these extensions (i.e. it's unknown whether they're using them or not).
Attachment #8353443 - Flags: review?(florian)
Comment on attachment 8353417 [details] [diff] [review]
Patch v2.1

*** Original change on bio 1296 attmnt 1660 at 2012-06-24 14:29:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353417 - Attachment is obsolete: true
Attachment #8353417 - Flags: review?(florian)
Attachment #8353417 - Flags: review?(bugzilla)
*** Original post on bio 1296 at 2012-06-26 21:42:38 UTC ***

Another suggestion:
Verified identity: yes/no

(and it would be 'yes' only if the user is both identified through nickserv, and connected via SSL)

I don't know yet if I like this new idea, but it may be worth discussing it. If it turns out it isn't any better than the previous ideas, then I guess we would be better off taking a patch with a separate line for each of these 2 info (and do it soon, so that we can string freeze!).
*** Original post on bio 1296 at 2012-06-26 22:01:31 UTC ***

Another suggestion: "Verified identity: no / no (unencrypted connection) / yes (SSL and registered nick)"
Comment on attachment 8353412 [details] [diff] [review]
Patch v2

*** Original change on bio 1296 attmnt 1655 at 2012-06-26 22:52:04 UTC ***

Ok, let's take this for 1.2.
Attachment #8353412 - Attachment is obsolete: false
Attachment #8353412 - Flags: review+
Comment on attachment 8353412 [details] [diff] [review]
Patch v2

*** Original change on bio 1296 attmnt 1655 at 2012-06-26 22:53:45 UTC ***

Ah, aleth had already reviewed this, we don't need my r+ flag here (I don't want the blame if anything goes wrong :-P).
Attachment #8353412 - Flags: review+
*** Original post on bio 1296 at 2012-06-26 23:49:03 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/ea6e55dcd067

If we need to change the strings, we'll deal with it in a follow up.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Comment on attachment 8353443 [details] [diff] [review]
Patch v2.2

*** Original change on bio 1296 attmnt 1686 at 2012-06-27 12:29:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353443 - Flags: review?(florian)
You need to log in before you can comment on or make changes to this bug.