round 1 of follow-up for metadata (#2277)

* refactoring
* send an empty batch if necessary, as per spec
* don't broadcast no-op updates
* don't trim spaces before validating the key
* bump irctest to cover metadata
* replay existing metadata to reattaching always-on clients
* use canonicalized name everywhere
* use utils.SafeErrorParam in FAIL lines
* validate key names for sub
* fix error for METADATA CLEAR
* max-keys is enforced for channels as well
* remove unlimited configurations
* maintain the limit exactly without off-by-one cases
* add final channel registration check

irc/metadata.go

@@ -2,12 +2,17 @@ package irc
 
 import (
 	"errors"
+	"iter"
+	"maps"
 	"regexp"
-	"strings"
 
 	"github.com/ergochat/ergo/irc/caps"
 	"github.com/ergochat/ergo/irc/modes"
-	"github.com/ergochat/ergo/irc/utils"
+)
+
+const (
+	// metadata key + value need to be relayable on a single IRC RPL_KEYVALUE line
+	maxCombinedMetadataLenBytes = 350
 )
 
 var (
@@ -16,42 +21,39 @@ var (
 )
 
 type MetadataHaver = interface {
-	SetMetadata(key string, value string)
+	SetMetadata(key string, value string, limit int) (updated bool, err error)
 	GetMetadata(key string) (string, bool)
-	DeleteMetadata(key string)
+	DeleteMetadata(key string) (updated bool)
 	ListMetadata() map[string]string
 	ClearMetadata() map[string]string
 	CountMetadata() int
 }
 
-func notifySubscribers(server *Server, session *Session, target string, key string, value string) {
-	var notify utils.HashSet[*Session] = make(utils.HashSet[*Session])
-	targetChannel := server.channels.Get(target)
-	targetClient := server.clients.Get(target)
+func notifySubscribers(server *Server, session *Session, targetObj MetadataHaver, targetName, key, value string) {
+	var recipientSessions iter.Seq[*Session]
 
-	if targetClient != nil {
-		notify = targetClient.FriendsMonitors(caps.Metadata)
-		// notify clients about changes regarding themselves
-		for _, s := range targetClient.Sessions() {
-			notify.Add(s)
-		}
-	}
-	if targetChannel != nil {
-		members := targetChannel.Members()
-		for _, m := range members {
-			for _, s := range m.Sessions() {
-				if s.capabilities.Has(caps.Metadata) {
-					notify.Add(s)
-				}
-			}
+	switch target := targetObj.(type) {
+	case *Client:
+		// TODO this case is expensive and might warrant rate-limiting
+		friends := target.FriendsMonitors(caps.Metadata)
+		// broadcast metadata update to other connected sessions
+		for _, s := range target.Sessions() {
+			friends.Add(s)
 		}
+		recipientSessions = maps.Keys(friends)
+	case *Channel:
+		recipientSessions = target.sessionsWithCaps(caps.Metadata)
+	default:
+		return // impossible
 	}
 
-	// don't notify the session that made the change
-	notify.Remove(session)
+	broadcastMetadataUpdate(server, recipientSessions, session, targetName, key, value)
+}
 
-	for s := range notify {
-		if !s.isSubscribedTo(key) {
+func broadcastMetadataUpdate(server *Server, sessions iter.Seq[*Session], originator *Session, target, key, value string) {
+	for s := range sessions {
+		// don't notify the session that made the change
+		if s == originator || !s.isSubscribedTo(key) {
 			continue
 		}
 
@@ -64,42 +66,38 @@ func notifySubscribers(server *Server, session *Session, target string, key stri
 }
 
 func syncClientMetadata(server *Server, rb *ResponseBuffer, target *Client) {
-	if len(rb.session.MetadataSubscriptions()) == 0 {
-		return
-	}
-
 	batchId := rb.StartNestedBatch("metadata")
 	defer rb.EndNestedBatch(batchId)
 
+	subs := rb.session.MetadataSubscriptions()
 	values := target.ListMetadata()
 	for k, v := range values {
-		if rb.session.isSubscribedTo(k) {
+		if subs.Has(k) {
 			visibility := "*"
 			rb.Add(nil, server.name, "METADATA", target.Nick(), k, visibility, v)
 		}
 	}
 }
 
-func syncChannelMetadata(server *Server, rb *ResponseBuffer, target *Channel) {
-	if len(rb.session.MetadataSubscriptions()) == 0 {
-		return
-	}
-
+func syncChannelMetadata(server *Server, rb *ResponseBuffer, channel *Channel) {
 	batchId := rb.StartNestedBatch("metadata")
 	defer rb.EndNestedBatch(batchId)
 
-	values := target.ListMetadata()
+	subs := rb.session.MetadataSubscriptions()
+	chname := channel.Name()
+
+	values := channel.ListMetadata()
 	for k, v := range values {
-		if rb.session.isSubscribedTo(k) {
+		if subs.Has(k) {
 			visibility := "*"
-			rb.Add(nil, server.name, "METADATA", target.Name(), k, visibility, v)
+			rb.Add(nil, server.name, "METADATA", chname, k, visibility, v)
 		}
 	}
 
-	for _, client := range target.Members() {
+	for _, client := range channel.Members() {
 		values := client.ListMetadata()
 		for k, v := range values {
-			if rb.session.isSubscribedTo(k) {
+			if subs.Has(k) {
 				visibility := "*"
 				rb.Add(nil, server.name, "METADATA", client.Nick(), k, visibility, v)
 			}
@@ -110,55 +108,34 @@ func syncChannelMetadata(server *Server, rb *ResponseBuffer, target *Channel) {
 var metadataEvilCharsRegexp = regexp.MustCompile("[^A-Za-z0-9_./:-]+")
 
 func metadataKeyIsEvil(key string) bool {
-	key = strings.TrimSpace(key) // just in case
-
 	return len(key) == 0 || // key needs to contain stuff
 		key[0] == ':' || // key can't start with a colon
 		metadataEvilCharsRegexp.MatchString(key) // key can't contain the stuff it can't contain
 }
 
-func metadataCanIEditThisKey(client *Client, target string, _ string) bool {
-	if !metadataCanIEditThisTarget(client, target) { // you can't edit keys on targets you can't edit.
-		return false
-	}
-
-	// todo: we don't actually do anything regarding visibility yet so there's not much to do here
-
-	return true
+func metadataCanIEditThisKey(client *Client, targetObj MetadataHaver, key string) bool {
+	// no key-specific logic as yet
+	return metadataCanIEditThisTarget(client, targetObj)
 }
 
-func metadataCanIEditThisTarget(client *Client, target string) bool {
-	if !metadataCanISeeThisTarget(client, target) { // you can't edit what you can't see. a wise man told me this once
-		return false
+func metadataCanIEditThisTarget(client *Client, targetObj MetadataHaver) bool {
+	switch target := targetObj.(type) {
+	case *Client:
+		return client == target || client.HasRoleCapabs("metadata")
+	case *Channel:
+		return target.ClientIsAtLeast(client, modes.Operator) || client.HasRoleCapabs("metadata")
+	default:
+		return false // impossible
 	}
-
-	if client.HasRoleCapabs("sajoin") { // sajoin opers can do whatever they want
-		return true
-	}
-
-	if target == client.Nick() { // your right to swing your fist ends where my nose begins
-		return true
-	}
-
-	// if you're a channel operator, knock yourself out
-	channel := client.server.channels.Get(target)
-	if channel != nil && channel.ClientIsAtLeast(client, modes.Operator) {
-		return true
-	}
-
-	return false
 }
 
-func metadataCanISeeThisTarget(client *Client, target string) bool {
-	if client.HasRoleCapabs("sajoin") { // sajoin opers can do whatever they want
+func metadataCanISeeThisTarget(client *Client, targetObj MetadataHaver) bool {
+	switch target := targetObj.(type) {
+	case *Client:
 		return true
+	case *Channel:
+		return target.hasClient(client) || client.HasRoleCapabs("metadata")
+	default:
+		return false // impossible
 	}
-
-	// check if the user is in the channel
-	channel := client.server.channels.Get(target)
-	if channel != nil && !channel.hasClient(client) {
-		return false
-	}
-
-	return true
 }