Merge pull request #358 from slingamn/resumefix.3

RESUME should end cap negotiation without requiring CAP LS

irc/client.go

@@ -374,34 +374,17 @@ func (client *Client) Ping() {
 
 }
 
-// Register sets the client details as appropriate when entering the network.
-func (client *Client) Register() {
-	client.stateMutex.Lock()
-	alreadyRegistered := client.registered
-	client.registered = true
-	client.stateMutex.Unlock()
-
-	if alreadyRegistered {
-		return
-	}
-
-	// apply resume details if we're able to.
-	client.TryResume()
-
-	// finish registration
-	client.updateNickMask()
-	client.server.monitorManager.AlertAbout(client, true)
-}
-
-// TryResume tries to resume if the client asked us to.
-func (client *Client) TryResume() {
-	if client.resumeDetails == nil {
-		return
-	}
-
+// tryResume tries to resume if the client asked us to.
+func (client *Client) tryResume() (success bool) {
 	server := client.server
 	config := server.Config()
 
+	defer func() {
+		if !success {
+			client.resumeDetails = nil
+		}
+	}()
+
 	oldnick := client.resumeDetails.OldNick
 	timestamp := client.resumeDetails.Timestamp
 	var timestampString string
@@ -412,7 +395,6 @@ func (client *Client) TryResume() {
 	oldClient := server.clients.Get(oldnick)
 	if oldClient == nil {
 		client.Send(nil, server.name, "RESUME", "ERR", oldnick, client.t("Cannot resume connection, old client not found"))
-		client.resumeDetails = nil
 		return
 	}
 	oldNick := oldClient.Nick()
@@ -421,24 +403,23 @@ func (client *Client) TryResume() {
 	resumeAllowed := config.Server.AllowPlaintextResume || (oldClient.HasMode(modes.TLS) && client.HasMode(modes.TLS))
 	if !resumeAllowed {
 		client.Send(nil, server.name, "RESUME", "ERR", oldnick, client.t("Cannot resume connection, old and new clients must have TLS"))
-		client.resumeDetails = nil
 		return
 	}
 
 	oldResumeToken := oldClient.ResumeToken()
 	if oldResumeToken == "" || !utils.SecretTokensMatch(oldResumeToken, client.resumeDetails.PresentedToken) {
 		client.Send(nil, server.name, "RESUME", "ERR", client.t("Cannot resume connection, invalid resume token"))
-		client.resumeDetails = nil
 		return
 	}
 
 	err := server.clients.Resume(client, oldClient)
 	if err != nil {
-		client.resumeDetails = nil
 		client.Send(nil, server.name, "RESUME", "ERR", client.t("Cannot resume connection"))
 		return
 	}
 
+	success = true
+
 	// this is a bit racey
 	client.resumeDetails.ResumedAt = time.Now()
 
@@ -520,13 +501,11 @@ func (client *Client) TryResume() {
 	client.Send(nil, client.server.name, "RESUME", "SUCCESS", oldNick)
 
 	// after we send the rest of the registration burst, we'll try rejoining channels
+	return
 }
 
 func (client *Client) tryResumeChannels() {
 	details := client.resumeDetails
-	if details == nil {
-		return
-	}
 
 	channels := make([]*Channel, len(details.Channels))
 	for _, name := range details.Channels {

irc/getters.go

@@ -127,6 +127,16 @@ func (client *Client) Registered() bool {
 	return client.registered
 }
 
+func (client *Client) SetRegistered() {
+	// `registered` is only written from the client's own goroutine, but may be
+	// read from other goroutines; therefore, the client's own goroutine may read
+	// the value without synchronization, but must write it with synchronization,
+	// and other goroutines must read it with synchronization
+	client.stateMutex.Lock()
+	client.registered = true
+	client.stateMutex.Unlock()
+}
+
 func (client *Client) Destroyed() bool {
 	client.stateMutex.RLock()
 	defer client.stateMutex.RUnlock()

irc/server.go

@@ -384,49 +384,58 @@ func (server *Server) generateMessageID() string {
 //
 
 func (server *Server) tryRegister(c *Client) {
-	if c.registered {
-		return
+	resumed := false
+	// try to complete registration, either via RESUME token or normally
+	if c.resumeDetails != nil {
+		if !c.tryResume() {
+			return
+		}
+		resumed = true
+	} else {
+		if c.preregNick == "" || !c.HasUsername() || c.capState == caps.NegotiatingState {
+			return
+		}
+
+		// client MUST send PASS if necessary, or authenticate with SASL if necessary,
+		// before completing the other registration commands
+		config := server.Config()
+		if !c.isAuthorized(config) {
+			c.Quit(c.t("Bad password"))
+			c.destroy(false)
+			return
+		}
+
+		rb := NewResponseBuffer(c)
+		nickAssigned := performNickChange(server, c, c, c.preregNick, rb)
+		rb.Send(true)
+		if !nickAssigned {
+			c.preregNick = ""
+			return
+		}
+
+		// check KLINEs
+		isBanned, info := server.klines.CheckMasks(c.AllNickmasks()...)
+		if isBanned {
+			c.Quit(info.BanMessage(c.t("You are banned from this server (%s)")))
+			c.destroy(false)
+			return
+		}
 	}
 
-	if c.preregNick == "" || !c.HasUsername() || c.capState == caps.NegotiatingState {
-		return
-	}
-
-	// client MUST send PASS if necessary, or authenticate with SASL if necessary,
-	// before completing the other registration commands
-	config := server.Config()
-	if !c.isAuthorized(config) {
-		c.Quit(c.t("Bad password"))
-		c.destroy(false)
-		return
-	}
-
-	rb := NewResponseBuffer(c)
-	nickAssigned := performNickChange(server, c, c, c.preregNick, rb)
-	rb.Send(true)
-	if !nickAssigned {
-		c.preregNick = ""
-		return
-	}
-
-	// check KLINEs
-	isBanned, info := server.klines.CheckMasks(c.AllNickmasks()...)
-	if isBanned {
-		c.Quit(info.BanMessage(c.t("You are banned from this server (%s)")))
-		c.destroy(false)
-		return
-	}
+	// registration has succeeded:
+	c.SetRegistered()
 
 	// count new user in statistics
 	server.stats.ChangeTotal(1)
 
+	if !resumed {
+		server.monitorManager.AlertAbout(c, true)
+	}
+
 	// continue registration
 	server.logger.Info("localconnect", fmt.Sprintf("Client connected [%s] [u:%s] [r:%s]", c.nick, c.username, c.realname))
 	server.snomasks.Send(sno.LocalConnects, fmt.Sprintf("Client connected [%s] [u:%s] [h:%s] [ip:%s] [r:%s]", c.nick, c.username, c.rawHostname, c.IPString(), c.realname))
 
-	// "register"; this includes the initial phase of session resumption
-	c.Register()
-
 	// send welcome text
 	//NOTE(dan): we specifically use the NICK here instead of the nickmask
 	// see http://modern.ircdocs.horse/#rplwelcome-001 for details on why we avoid using the nickmask
@@ -436,7 +445,7 @@ func (server *Server) tryRegister(c *Client) {
 	//TODO(dan): Look at adding last optional [<channel modes with a parameter>] parameter
 	c.Send(nil, server.name, RPL_MYINFO, c.nick, server.name, Ver, supportedUserModesString, supportedChannelModesString)
 
-	rb = NewResponseBuffer(c)
+	rb := NewResponseBuffer(c)
 	c.RplISupport(rb)
 	server.MOTD(c, rb)
 	rb.Send(true)
@@ -449,8 +458,9 @@ func (server *Server) tryRegister(c *Client) {
 		c.Notice(c.t("This server is in debug mode and is logging all user I/O. If you do not wish for everything you send to be readable by the server owner(s), please disconnect."))
 	}
 
-	// if resumed, send fake channel joins
-	c.tryResumeChannels()
+	if resumed {
+		c.tryResumeChannels()
+	}
 }
 
 // t returns the translated version of the given string, based on the languages configured by the client.