fix #649

irc/client_lookup_set.go

@@ -268,10 +268,10 @@ func NewUserMaskSet() *UserMaskSet {
 }
 
 // Add adds the given mask to this set.
-func (set *UserMaskSet) Add(mask, creatorNickmask, creatorAccount string) (added bool) {
+func (set *UserMaskSet) Add(mask, creatorNickmask, creatorAccount string) (maskAdded string, err error) {
 	casefoldedMask, err := CanonicalizeMaskWildcard(mask)
 	if err != nil {
-		return false
+		return
 	}
 
 	set.Lock()
@@ -279,8 +279,8 @@ func (set *UserMaskSet) Add(mask, creatorNickmask, creatorAccount string) (added
 		set.masks = make(map[string]MaskInfo)
 	}
 	_, present := set.masks[casefoldedMask]
-	added = !present
-	if added {
+	if !present {
+		maskAdded = casefoldedMask
 		set.masks[casefoldedMask] = MaskInfo{
 			TimeCreated:     time.Now().UTC(),
 			CreatorNickmask: creatorNickmask,
@@ -289,22 +289,23 @@ func (set *UserMaskSet) Add(mask, creatorNickmask, creatorAccount string) (added
 	}
 	set.Unlock()
 
-	if added {
+	if !present {
 		set.setRegexp()
 	}
 	return
 }
 
 // Remove removes the given mask from this set.
-func (set *UserMaskSet) Remove(mask string) (removed bool) {
-	mask, err := CanonicalizeMaskWildcard(mask)
+func (set *UserMaskSet) Remove(mask string) (maskRemoved string, err error) {
+	mask, err = CanonicalizeMaskWildcard(mask)
 	if err != nil {
-		return false
+		return
 	}
 
 	set.Lock()
-	_, removed = set.masks[mask]
+	_, removed := set.masks[mask]
 	if removed {
+		maskRemoved = mask
 		delete(set.masks, mask)
 	}
 	set.Unlock()

irc/handlers.go

@@ -1685,13 +1685,12 @@ func cmodeHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 		return false
 	}
 
-	// applied mode changes
-	applied := make(modes.ModeChanges, 0)
-
+	var changes modes.ModeChanges
 	if 1 < len(msg.Params) {
 		// parse out real mode changes
 		params := msg.Params[1:]
-		changes, unknown := modes.ParseChannelModeChanges(params...)
+		var unknown map[rune]bool
+		changes, unknown = modes.ParseChannelModeChanges(params...)
 
 		// alert for unknown mode changes
 		for char := range unknown {
@@ -1700,10 +1699,9 @@ func cmodeHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 		if len(unknown) == 1 && len(changes) == 0 {
 			return false
 		}
-
-		// apply mode changes
-		applied = channel.ApplyChannelModeChanges(client, msg.Command == "SAMODE", changes, rb)
 	}
+	// process mode changes, include list operations (an empty set of changes does a list)
+	applied := channel.ApplyChannelModeChanges(client, msg.Command == "SAMODE", changes, rb)
 
 	// save changes
 	var includeFlags uint
@@ -1719,8 +1717,8 @@ func cmodeHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 	}
 
 	// send out changes
-	prefix := client.NickMaskString()
 	if len(applied) > 0 {
+		prefix := client.NickMaskString()
 		//TODO(dan): we should change the name of String and make it return a slice here
 		args := append([]string{channel.name}, strings.Split(applied.String(), " ")...)
 		for _, member := range channel.Members() {
@@ -1735,10 +1733,6 @@ func cmodeHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 				member.Send(nil, prefix, "MODE", args...)
 			}
 		}
-	} else {
-		args := append([]string{client.nick, channel.name}, channel.modeStrings(client)...)
-		rb.Add(nil, prefix, RPL_CHANNELMODEIS, args...)
-		rb.Add(nil, client.nickMaskString, RPL_CREATIONTIME, client.nick, channel.name, strconv.FormatInt(channel.createdTime.Unix(), 10))
 	}
 	return false
 }

irc/modes.go

@@ -6,6 +6,7 @@
 package irc
 
 import (
+	"fmt"
 	"strconv"
 	"strings"
 
@@ -104,17 +105,15 @@ func ParseDefaultChannelModes(rawModes *string) modes.Modes {
 }
 
 // ApplyChannelModeChanges applies a given set of mode changes.
-func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, changes modes.ModeChanges, rb *ResponseBuffer) modes.ModeChanges {
+func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, changes modes.ModeChanges, rb *ResponseBuffer) (applied modes.ModeChanges) {
 	// so we only output one warning for each list type when full
 	listFullWarned := make(map[modes.Mode]bool)
 
 	var alreadySentPrivError bool
 
-	applied := make(modes.ModeChanges, 0)
-
-	isListOp := func(change modes.ModeChange) bool {
-		return (change.Op == modes.List) || (change.Arg == "")
-	}
+	maskOpCount := 0
+	chname := channel.Name()
+	details := client.Details()
 
 	hasPrivs := func(change modes.ModeChange) bool {
 		if isSamode {
@@ -127,18 +126,19 @@ func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, c
 				return true
 			}
 			cfarg, _ := CasefoldName(change.Arg)
-			isSelfChange := cfarg == client.NickCasefolded()
+			isSelfChange := cfarg == details.nickCasefolded
 			if change.Op == modes.Remove && isSelfChange {
 				// "There is no restriction, however, on anyone `deopping' themselves"
 				// <https://tools.ietf.org/html/rfc2812#section-3.1.5>
 				return true
 			}
 			return channelUserModeHasPrivsOver(channel.HighestUserMode(client), change.Mode)
-		case modes.BanMask:
-			// #163: allow unprivileged users to list ban masks
-			return isListOp(change) || channel.ClientIsAtLeast(client, modes.ChannelOperator)
-		default:
+		case modes.InviteMask, modes.ExceptMask:
+			// listing these requires privileges
 			return channel.ClientIsAtLeast(client, modes.ChannelOperator)
+		default:
+			// #163: allow unprivileged users to list ban masks, and any other modes
+			return change.Op == modes.List || channel.ClientIsAtLeast(client, modes.ChannelOperator)
 		}
 	}
 
@@ -146,14 +146,15 @@ func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, c
 		if !hasPrivs(change) {
 			if !alreadySentPrivError {
 				alreadySentPrivError = true
-				rb.Add(nil, client.server.name, ERR_CHANOPRIVSNEEDED, client.Nick(), channel.name, client.t("You're not a channel operator"))
+				rb.Add(nil, client.server.name, ERR_CHANOPRIVSNEEDED, details.nick, channel.name, client.t("You're not a channel operator"))
 			}
 			continue
 		}
 
 		switch change.Mode {
 		case modes.BanMask, modes.ExceptMask, modes.InviteMask:
-			if isListOp(change) {
+			maskOpCount += 1
+			if change.Op == modes.List {
 				channel.ShowMaskList(client, change.Mode, rb)
 				continue
 			}
@@ -163,20 +164,33 @@ func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, c
 			case modes.Add:
 				if channel.lists[change.Mode].Length() >= client.server.Config().Limits.ChanListModes {
 					if !listFullWarned[change.Mode] {
-						rb.Add(nil, client.server.name, ERR_BANLISTFULL, client.Nick(), channel.Name(), change.Mode.String(), client.t("Channel list is full"))
+						rb.Add(nil, client.server.name, ERR_BANLISTFULL, details.nick, chname, change.Mode.String(), client.t("Channel list is full"))
 						listFullWarned[change.Mode] = true
 					}
 					continue
 				}
 
-				details := client.Details()
-				if success := channel.lists[change.Mode].Add(mask, details.nickMask, details.accountName); success {
-					applied = append(applied, change)
+				maskAdded, err := channel.lists[change.Mode].Add(mask, details.nickMask, details.accountName)
+				if maskAdded != "" {
+					appliedChange := change
+					appliedChange.Arg = maskAdded
+					applied = append(applied, appliedChange)
+				} else if err != nil {
+					rb.Add(nil, client.server.name, ERR_INVALIDMODEPARAM, details.nick, mask, fmt.Sprintf(client.t("Invalid mode %s parameter: %s"), string(change.Mode), mask))
+				} else {
+					rb.Add(nil, client.server.name, ERR_LISTMODEALREADYSET, chname, mask, string(change.Mode), fmt.Sprintf(client.t("Channel %s list already contains %s"), chname, mask))
 				}
 
 			case modes.Remove:
-				if success := channel.lists[change.Mode].Remove(mask); success {
-					applied = append(applied, change)
+				maskRemoved, err := channel.lists[change.Mode].Remove(mask)
+				if maskRemoved != "" {
+					appliedChange := change
+					appliedChange.Arg = maskRemoved
+					applied = append(applied, appliedChange)
+				} else if err != nil {
+					rb.Add(nil, client.server.name, ERR_INVALIDMODEPARAM, details.nick, mask, fmt.Sprintf(client.t("Invalid mode %s parameter: %s"), string(change.Mode), mask))
+				} else {
+					rb.Add(nil, client.server.name, ERR_LISTMODENOTSET, chname, mask, string(change.Mode), fmt.Sprintf(client.t("Channel %s list does not contain %s"), chname, mask))
 				}
 			}
 
@@ -221,7 +235,7 @@ func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, c
 			nick := change.Arg
 			if nick == "" {
 				rb.Add(nil, client.server.name, ERR_NEEDMOREPARAMS, client.Nick(), "MODE", client.t("Not enough parameters"))
-				return nil
+				continue
 			}
 
 			change := channel.applyModeToMember(client, change.Mode, change.Op, nick, rb)
@@ -231,6 +245,13 @@ func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, c
 		}
 	}
 
+	// #649: don't send 324 RPL_CHANNELMODEIS if we were only working with mask lists
+	if len(applied) == 0 && !alreadySentPrivError && (maskOpCount == 0 || maskOpCount < len(changes)) {
+		args := append([]string{details.nick, chname}, channel.modeStrings(client)...)
+		rb.Add(nil, client.server.name, RPL_CHANNELMODEIS, args...)
+		rb.Add(nil, client.server.name, RPL_CREATIONTIME, details.nick, chname, strconv.FormatInt(channel.createdTime.Unix(), 10))
+	}
+
 	return applied
 }
 

irc/modes/modes_test.go

@@ -47,6 +47,19 @@ func TestParseChannelModeChanges(t *testing.T) {
 	if len(modes) != 1 || modes[0] != expected {
 		t.Errorf("unexpected mode change: %v", modes)
 	}
+
+	modes, unknown = ParseChannelModeChanges("+b")
+	if len(unknown) > 0 {
+		t.Errorf("unexpected unknown mode change: %v", unknown)
+	}
+	// +b with no argument becomes a list operation
+	expectedChanges := ModeChanges{{
+		Op:   List,
+		Mode: BanMask,
+	}}
+	if !reflect.DeepEqual(modes, expectedChanges) {
+		t.Errorf("unexpected mode change: %v instead of %v", modes, expectedChanges)
+	}
 }
 
 func TestSetMode(t *testing.T) {

irc/numerics.go

@@ -169,6 +169,9 @@ const (
 	RPL_YOURLANGUAGESARE          = "687"
 	ERR_CHANNAMEINUSE             = "692"
 	ERR_CANNOTRENAME              = "693"
+	ERR_INVALIDMODEPARAM          = "696"
+	ERR_LISTMODEALREADYSET        = "697"
+	ERR_LISTMODENOTSET            = "698"
 	RPL_HELPSTART                 = "704"
 	RPL_HELPTXT                   = "705"
 	RPL_ENDOFHELP                 = "706"