review fixes and updates

irc/chanserv.go

@@ -52,8 +52,7 @@ For example, $bAMODE #channel +o dan$b grants the the holder of the "dan"
 account the +o operator mode every time they join #channel. To list current
 accounts and modes, use $bAMODE #channel$b. Note that users are always
 referenced by their registered account names, not their nicknames.`,
-			helpShort:    `$bAMODE$b modifies persistent mode settings for channel members.`,
+			helpShort: `$bAMODE$b modifies persistent mode settings for channel members.`,
-			authRequired: true,
 		},
 	}
 )
@@ -70,61 +69,69 @@ func csAmodeHandler(server *Server, client *Client, command, params string, rb *
 	if channel == nil {
 		csNotice(rb, client.t("Channel does not exist"))
 		return
-	}
+	} else if channel.Founder() == "" {
-
+		csNotice(rb, client.t("Channel is not registered"))
-	clientAccount := client.Account()
-	if clientAccount == "" || clientAccount != channel.Founder() {
-		csNotice(rb, client.t("You must be the channel founder to use AMODE"))
 		return
 	}
 
 	modeChanges, unknown := modes.ParseChannelModeChanges(strings.Fields(modeChange)...)
-
+	var change modes.ModeChange
 	if len(modeChanges) > 1 || len(unknown) > 0 {
 		csNotice(rb, client.t("Invalid mode change"))
 		return
+	} else if len(modeChanges) == 1 {
+		change = modeChanges[0]
+	} else {
+		change = modes.ModeChange{Op: modes.List}
 	}
 
-	if len(modeChanges) == 0 || modeChanges[0].Op == modes.List {
+	// normalize and validate the account argument
-		persistentModes := channel.AccountToUmode()
-		// sort the persistent modes in descending order of priority, i.e.,
-		// ascending order of their index in the ChannelUserModes list
-		sort.Slice(persistentModes, func(i, j int) bool {
-			index := func(modeChange modes.ModeChange) int {
-				for idx, mode := range modes.ChannelUserModes {
-					if modeChange.Mode == mode {
-						return idx
-					}
-				}
-				return len(modes.ChannelUserModes)
-			}
-			return index(persistentModes[i]) < index(persistentModes[j])
-		})
-		csNotice(rb, fmt.Sprintf(client.t("Channel %s has %d persistent modes set"), channelName, len(persistentModes)))
-		for _, modeChange := range persistentModes {
-			csNotice(rb, fmt.Sprintf(client.t("Account %s receives mode +%s"), modeChange.Arg, string(modeChange.Mode)))
-		}
-		return
-	}
-
 	accountIsValid := false
-	change := modeChanges[0]
-	// Arg is the account name, casefold it here
 	change.Arg, _ = CasefoldName(change.Arg)
-	if change.Arg != "" {
+	switch change.Op {
-		_, err := server.accounts.LoadAccount(change.Arg)
+	case modes.List:
-		accountIsValid = (err == nil)
+		accountIsValid = true
+	case modes.Add:
+		// if we're adding a mode, the account must exist
+		if change.Arg != "" {
+			_, err := server.accounts.LoadAccount(change.Arg)
+			accountIsValid = (err == nil)
+		}
+	case modes.Remove:
+		// allow removal of accounts that may have been deleted
+		accountIsValid = (change.Arg != "")
 	}
 	if !accountIsValid {
 		csNotice(rb, client.t("Account does not exist"))
 		return
 	}
-	applied := channel.ApplyAccountToUmodeChange(change)
+
-	if applied {
+	affectedModes, err := channel.ProcessAccountToUmodeChange(client, change)
-		csNotice(rb, fmt.Sprintf(client.t("Successfully set mode %s"), change.String()))
+
-		go server.channelRegistry.StoreChannel(channel, IncludeLists)
+	if err == errInsufficientPrivs {
-	} else {
+		csNotice(rb, client.t("Insufficient privileges"))
-		csNotice(rb, client.t("Change was a no-op"))
+		return
+	} else if err != nil {
+		csNotice(rb, client.t("Internal error"))
+		return
+	}
+
+	switch change.Op {
+	case modes.List:
+		// sort the persistent modes in descending order of priority
+		sort.Slice(affectedModes, func(i, j int) bool {
+			return umodeGreaterThan(affectedModes[i].Mode, affectedModes[j].Mode)
+		})
+		csNotice(rb, fmt.Sprintf(client.t("Channel %s has %d persistent modes set"), channelName, len(affectedModes)))
+		for _, modeChange := range affectedModes {
+			csNotice(rb, fmt.Sprintf(client.t("Account %s receives mode +%s"), modeChange.Arg, string(modeChange.Mode)))
+		}
+	case modes.Add, modes.Remove:
+		if len(affectedModes) > 0 {
+			csNotice(rb, fmt.Sprintf(client.t("Successfully set mode %s"), change.String()))
+		} else {
+			csNotice(rb, client.t("Change was a no-op"))
+		}
 	}
 }
 

irc/errors.go

@@ -34,6 +34,7 @@ var (
 	errNoExistingBan                  = errors.New("Ban does not exist")
 	errNoSuchChannel                  = errors.New("No such channel")
 	errRenamePrivsNeeded              = errors.New("Only chanops can rename channels")
+	errInsufficientPrivs              = errors.New("Insufficient privileges")
 	errSaslFail                       = errors.New("SASL failed")
 )
 

irc/getters.go

@@ -303,37 +303,3 @@ func (channel *Channel) Founder() string {
 	defer channel.stateMutex.RUnlock()
 	return channel.registeredFounder
 }
-
-func (channel *Channel) AccountToUmode() (result []modes.ModeChange) {
-	channel.stateMutex.RLock()
-	defer channel.stateMutex.RUnlock()
-	for account, mode := range channel.accountToUMode {
-		result = append(result, modes.ModeChange{
-			Mode: mode,
-			Arg:  account,
-			Op:   modes.Add,
-		})
-	}
-
-	return
-}
-
-func (channel *Channel) ApplyAccountToUmodeChange(change modes.ModeChange) (applied bool) {
-	channel.stateMutex.Lock()
-	defer channel.stateMutex.Unlock()
-
-	current := channel.accountToUMode[change.Arg]
-	switch change.Op {
-	case modes.Add:
-		applied = (current != change.Mode)
-		if applied {
-			channel.accountToUMode[change.Arg] = change.Mode
-		}
-	case modes.Remove:
-		applied = (current == change.Mode)
-		if applied {
-			delete(channel.accountToUMode, change.Arg)
-		}
-	}
-	return
-}

irc/modes.go

@@ -240,3 +240,79 @@ func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, c
 
 	return applied
 }
+
+// tests whether l > r, in the channel-user mode ordering (e.g., Halfop > Voice)
+func umodeGreaterThan(l modes.Mode, r modes.Mode) bool {
+	for _, mode := range modes.ChannelUserModes {
+		if l == mode && r != mode {
+			return true
+		} else if r == mode {
+			return false
+		}
+	}
+	return false
+}
+
+// ProcessAccountToUmodeChange processes Add/Remove/List operations for channel persistent usermodes.
+func (channel *Channel) ProcessAccountToUmodeChange(client *Client, change modes.ModeChange) (results []modes.ModeChange, err error) {
+	umodeGEQ := func(l modes.Mode, r modes.Mode) bool {
+		return l == r || umodeGreaterThan(l, r)
+	}
+
+	account := client.Account()
+	isOperChange := client.HasRoleCapabs("chanreg")
+
+	channel.stateMutex.Lock()
+	defer channel.stateMutex.Unlock()
+
+	clientMode := channel.accountToUMode[account]
+	targetModeNow := channel.accountToUMode[change.Arg]
+	var targetModeAfter modes.Mode
+	if change.Op == modes.Add {
+		targetModeAfter = change.Mode
+	}
+
+	// operators and founders can do anything
+	hasPrivs := isOperChange || (account != "" && account == channel.registeredFounder)
+	// halfop and up can list, and do add/removes at levels <= their own
+	if change.Op == modes.List && umodeGEQ(clientMode, modes.Halfop) {
+		hasPrivs = true
+	} else if umodeGEQ(clientMode, modes.Halfop) && umodeGEQ(clientMode, targetModeNow) && umodeGEQ(clientMode, targetModeAfter) {
+		hasPrivs = true
+	}
+	if !hasPrivs {
+		return nil, errInsufficientPrivs
+	}
+
+	switch change.Op {
+	case modes.Add:
+		if targetModeNow != targetModeAfter {
+			channel.accountToUMode[change.Arg] = change.Mode
+			go client.server.channelRegistry.StoreChannel(channel, IncludeLists)
+			return []modes.ModeChange{change}, nil
+		}
+		return nil, nil
+	case modes.Remove:
+		if targetModeNow == change.Mode {
+			delete(channel.accountToUMode, change.Arg)
+			go client.server.channelRegistry.StoreChannel(channel, IncludeLists)
+			return []modes.ModeChange{change}, nil
+		}
+		return nil, nil
+	case modes.List:
+		result := make([]modes.ModeChange, len(channel.accountToUMode))
+		pos := 0
+		for account, mode := range channel.accountToUMode {
+			result[pos] = modes.ModeChange{
+				Mode: mode,
+				Arg:  account,
+				Op:   modes.Add,
+			}
+			pos++
+		}
+		return result, nil
+	default:
+		// shouldn't happen
+		return nil, errInvalidCharacter
+	}
+}

irc/modes_test.go

@@ -36,3 +36,17 @@ func TestParseDefaultChannelModes(t *testing.T) {
 		}
 	}
 }
+
+func TestUmodeGreaterThan(t *testing.T) {
+	if !umodeGreaterThan(modes.Halfop, modes.Voice) {
+		t.Errorf("expected Halfop > Voice")
+	}
+
+	if !umodeGreaterThan(modes.Voice, modes.Mode(0)) {
+		t.Errorf("expected Voice > 0 (the zero value of modes.Mode)")
+	}
+
+	if umodeGreaterThan(modes.ChannelAdmin, modes.ChannelAdmin) {
+		t.Errorf("modes should not be greater than themselves")
+	}
+}

oragono.yaml

@@ -283,6 +283,7 @@ oper-classes:
             - "unregister"
             - "samode"
             - "vhosts"
+            - "chanreg"
 
 # ircd operators
 opers: