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

RESOLVED FIXED

Status

--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: tuukka.tolvanen, Assigned: bugzilla-mozilla-20000923)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.82])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Updated

11 years ago
OS: Linux → All
Hardware: PC → All
Summary: don't save nick change if connected and server doesn't confirm it → Don't save new nickname when connected and server doesn't accept it
Version: unspecified → Trunk
(Assignee)

Comment 1

11 years ago
Created attachment 310027 [details] [diff] [review]
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.
Assignee: rginda → silver
Status: NEW → ASSIGNED
Attachment #310027 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 2

11 years ago
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

11 years ago
(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 :-(
(Assignee)

Comment 4

11 years ago
Created attachment 310998 [details] [diff] [review]
This time with conditionals

I can't follow what went on in bug 350996, but here's a version that has a condition.
Attachment #310027 - Attachment is obsolete: true
Attachment #310998 - Flags: review?(gijskruitbosch+bugs)
Attachment #310027 - Flags: review?(gijskruitbosch+bugs)

Comment 5

11 years ago
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).
Attachment #310998 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 6

11 years ago
Created attachment 318907 [details] [diff] [review]
Now with delete and == check
Attachment #310998 - Attachment is obsolete: true
Attachment #318907 - Flags: review?(gijskruitbosch+bugs)

Comment 7

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

This looks great, r=me
Attachment #318907 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 8

11 years ago
Checked in --> FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.82]
You need to log in before you can comment on or make changes to this bug.