Closed Bug 955198 Opened 6 years ago Closed 6 years ago

Automatically authenticate when changing the nick to the account nickname

Categories

(Chat Core :: IRC, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1766 at 2012-11-05 17:21:00 UTC ***

Use case: We log in and our nick is blocked. When it becomes free, we /nick to our own nick. We should then handle the NickServ Identify request automatically.
Attached patch Mini-WIP (obsolete) — Splinter Review
*** Original post on bio 1766 as attmnt 2052 at 2012-11-05 17:46:00 UTC ***

It's a start... if someone wants to finish it, feel free :)
Should we always use NICKSERV IDENTIFY to reidentify, or should we reuse whatever auth method worked on login?
*** Original post on bio 1766 at 2012-11-05 19:59:15 UTC ***

(In reply to comment #1)
> It's a start... if someone wants to finish it, feel free :)
> Should we always use NICKSERV IDENTIFY to reidentify, or should we reuse
> whatever auth method worked on login?

No. SASL doesn't exist except during login. We should attempt IDENTIFY (and if that fails, NICKSERV IDENTIFY should automatically be run).

I think it would be ideal if we could add this handler to ircServices.jsm.

I also don't know why you're setting isAuthenticated to false, I don't know if changing your nick necessarily has you lose authentication (e.g. what happens if you change to another grouped nick?)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1766 as attmnt 2098 at 2012-11-18 12:20:00 UTC ***

Reauthenticates whenever the user switches to the account nick.
Attachment #8353859 - Flags: review?(clokep)
Comment on attachment 8353812 [details] [diff] [review]
Mini-WIP

*** Original change on bio 1766 attmnt 2052 at 2012-11-18 12:20:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353812 - Attachment is obsolete: true
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353859 [details] [diff] [review]
Patch

*** Original change on bio 1766 attmnt 2098 at 2012-11-18 19:34:24 UTC ***

>diff --git a/modules/ircServices.jsm b/modules/ircServices.jsm
> var ircServices = {
>   name: "IRC Services",
>   priority: ircHandlers.HIGH_PRIORITY,
>   isEnabled: function() true,
>+  sendIdentify: function(aAccount) {
Generally we use global functions when doing things like this, but I'm fine with it being part of the object.

>+    if (aAccount.imAccount.password && !aAccount.isAuthenticated)
>+      aAccount.sendMessage("IDENTIFY", aAccount.imAccount.password,
>+                           "IDENTIFY <password not logged>");
This should have { } around it, it is multiple lines.

>@@ -65,22 +70,36 @@ var ircServices = {
[...]
>+      // We may still be authenticated, but we try to authenticate with the
>+      // new nick anyway, since there is no good way to tell if we are still
>+      // authenticated.
>+      this.isAuthenticated = false;
Do I have to say it? :) delete this.isAuthenticated please.

>@@ -171,12 +190,22 @@ var servicesBase = {
>+      if (text == "You are already identified." || // Anope.
>+          text.slice(0, 30) == "You are already logged in as \x02") { // Atheme.
>+        if (!this.isAuthenticated) {
I don't like the extra indentation here, there's no need for it. Let's just add all the extra Boolean clause to the above if statement (as the first clause).

>+          // We tried to authenticate after a nick change, but were already
>+          // authenticated. Swallow the message.
How about: "Do not show the message if caused by the automatic reauthentication."
Attachment #8353859 - Flags: review?(clokep) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1766 as attmnt 2105 at 2012-11-18 19:40:00 UTC ***

Nits fixed.
Attachment #8353866 - Flags: review?(clokep)
Comment on attachment 8353859 [details] [diff] [review]
Patch

*** Original change on bio 1766 attmnt 2098 at 2012-11-18 19:40:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353859 - Attachment is obsolete: true
Comment on attachment 8353866 [details] [diff] [review]
Patch

*** Original change on bio 1766 attmnt 2105 at 2012-11-18 19:57:14 UTC ***

Looks good! This'll be great to use!
Attachment #8353866 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
Blocks: 955244
*** Original post on bio 1766 at 2012-11-22 00:40:08 UTC ***

Can't wait to use this! http://hg.instantbird.org/instantbird/rev/b13ce903a551
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.