420635 - Don't save new nickname when connected and server doesn't accept it

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[Tuukka Tolvanen (sp3000)]

1. connect to moznet
2. /nick säyrer
-> 	===	säyrer Erroneous Nickname: Illegal characters
3. check prefs -> moznet -> general -> nickname

expected: earlier nick saved
actual: säyrer saved

An unnamed ircmonkey's valiant spoofing effort backfired when trying to connect the next time, as the mean server shunned the saved nick. Would it be possible to, if connected, defer saving the nick until the server confirms it changed? Only if the given nick matches the last-requested one on that server of course.

Comment 1

[James Ross]

Attachment: Save nickname in onNick event and when not connected

The e.server.isConnected is just for consistency with the code above, and doesn't cause any breakage - e.server.isConnect is true if the socket is connected, so doesn't break anything that's badly written (like bouncers).

I had to be careful with when to save in /nick, since if you try connecting with an invalid nickname and do /nick to specify a valid one, you do NOT get an onNick event (no NICK is sent). Once you get 001, and are thus NET_ONLINE, any changes cause onNick and so only in NET_ONLINE is the save delayed.

Comment 2

[James Ross]

This code doesn't care if you initiated the nick-change when it receives the onNick event; I'm not sure it's that important that it should. If the server renames you on its or someone else's behalf, the user is only going to use /nick afterwards anyway. Any cases it might matter?

Comment 3

[:Gijs (out for now; he/him)]

(In reply to comment #2)
> This code doesn't care if you initiated the nick-change when it receives the
> onNick event; I'm not sure it's that important that it should. If the server
> renames you on its or someone else's behalf, the user is only going to use
> /nick afterwards anyway. Any cases it might matter?
> 

I believe so. This is the whole deal about bug 350996, I believe. If we would do what this patch does, I think, we would end up setting the preferred nick and the pref to OriginalNick_, and then OriginalNick__ and so on. Doesn't sound like something we want to do to me :-(

Comment 4

[James Ross]

Attachment: This time with conditionals

I can't follow what went on in bug 350996, but here's a version that has a condition.

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 310998 [details] [diff] [review]
This time with conditionals

This still breaks if we don't get a NICK reply before disconnecting, I think? 

r=me if you check pendingNickChange for containing the same nick as the onNick reply tells us (though we should probably be careful about unicode vs. encoded nickname there :-( ). The back of my mind insists I had another case that meant we *couldn't* check the pendingNickChange value, but I don't remember it.

Furthermore, I guess it would be good to delete pendingNickChange on disconnect as well (if we don't already by some other magic I've forgotten about).

Comment 6

[James Ross]

Attachment: Now with delete and == check

Comment 7

[:Gijs (out for now; he/him)]

Comment on attachment 318907 [details] [diff] [review]
Now with delete and == check

This looks great, r=me

Comment 8

[James Ross]

Checked in --> FIXED.