fix: address 3 blocking review findings for IRC protocol listener

1. ISUPPORT/applyChannelModes: extend IRC MODE handler to support +i/-i, +s/-s, +n/-n (routed through svc.SetChannelFlag), and +H/-H (hashcash bits with parameter parsing). Add 'n' (no external messages) as a proper DB-backed channel flag with is_no_external column (default: on). Update IRC ISUPPORT to CHANMODES=,,H,imnst to match actual support. 2. QueryChannelMode: rewrite to return complete mode string including all boolean flags (n, i, m, s, t) and parameterized modes (k, l, H), matching the HTTP handler's buildChannelModeString logic. Simplify buildChannelModeString to delegate to QueryChannelMode for consistency. 3. Service struct encapsulation: change exported fields (DB, Broker, Config, Log) to unexported (db, broker, config, log). Add NewTestService constructor for use by external test packages. Update ircserver export_test.go to use the new constructor. Closes #89
2026-03-28 11:48:01 -07:00
parent 260f798af4
commit f57a373053
7 changed files with 357 additions and 175 deletions
--- a/internal/service/service.go
+++ b/internal/service/service.go
@@ -8,6 +8,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"log/slog"
+	"strconv"
 	"strings"

 	"git.eeqj.de/sneak/neoirc/internal/broker"
@@ -30,19 +31,35 @@ type Params struct {

 // Service provides shared business logic for IRC commands.
 type Service struct {
-	DB     *db.Database
-	Broker *broker.Broker
-	Config *config.Config
-	Log    *slog.Logger
+	db     *db.Database
+	broker *broker.Broker
+	config *config.Config
+	log    *slog.Logger
 }

 // New creates a new Service.
 func New(params Params) *Service {
 	return &Service{
-		DB:     params.Database,
-		Broker: params.Broker,
-		Config: params.Config,
-		Log:    params.Logger.Get(),
+		db:     params.Database,
+		broker: params.Broker,
+		config: params.Config,
+		log:    params.Logger.Get(),
+	}
+}
+
+// NewTestService creates a Service for use in tests
+// outside the service package.
+func NewTestService(
+	database *db.Database,
+	brk *broker.Broker,
+	cfg *config.Config,
+	log *slog.Logger,
+) *Service {
+	return &Service{
+		db:     database,
+		broker: brk,
+		config: cfg,
+		log:    log,
 	}
 }

@@ -76,7 +93,7 @@ func (s *Service) FanOut(
 	params, body, meta json.RawMessage,
 	sessionIDs []int64,
 ) (int64, string, error) {
-	dbID, msgUUID, err := s.DB.InsertMessage(
+	dbID, msgUUID, err := s.db.InsertMessage(
 		ctx, command, from, to, params, body, meta,
 	)
 	if err != nil {
@@ -84,8 +101,8 @@ func (s *Service) FanOut(
 	}

 	for _, sid := range sessionIDs {
-		_ = s.DB.EnqueueToSession(ctx, sid, dbID)
-		s.Broker.Notify(sid)
+		_ = s.db.EnqueueToSession(ctx, sid, dbID)
+		s.broker.Notify(sid)
 	}

 	return dbID, msgUUID, nil
@@ -120,7 +137,7 @@ func (s *Service) SendChannelMessage(
 	nick, command, channel string,
 	body, meta json.RawMessage,
 ) (int64, string, error) {
-	chID, err := s.DB.GetChannelByName(ctx, channel)
+	chID, err := s.db.GetChannelByName(ctx, channel)
 	if err != nil {
 		return 0, "", &IRCError{
 			irc.ErrNoSuchChannel,
@@ -129,7 +146,7 @@ func (s *Service) SendChannelMessage(
 		}
 	}

-	isMember, _ := s.DB.IsChannelMember(
+	isMember, _ := s.db.IsChannelMember(
 		ctx, chID, sessionID,
 	)
 	if !isMember {
@@ -141,7 +158,7 @@ func (s *Service) SendChannelMessage(
 	}

 	// Ban check — banned users cannot send messages.
-	isBanned, banErr := s.DB.IsSessionBanned(
+	isBanned, banErr := s.db.IsSessionBanned(
 		ctx, chID, sessionID,
 	)
 	if banErr == nil && isBanned {
@@ -152,12 +169,12 @@ func (s *Service) SendChannelMessage(
 		}
 	}

-	moderated, _ := s.DB.IsChannelModerated(ctx, chID)
+	moderated, _ := s.db.IsChannelModerated(ctx, chID)
 	if moderated {
-		isOp, _ := s.DB.IsChannelOperator(
+		isOp, _ := s.db.IsChannelOperator(
 			ctx, chID, sessionID,
 		)
-		isVoiced, _ := s.DB.IsChannelVoiced(
+		isVoiced, _ := s.db.IsChannelVoiced(
 			ctx, chID, sessionID,
 		)

@@ -170,7 +187,7 @@ func (s *Service) SendChannelMessage(
 		}
 	}

-	memberIDs, _ := s.DB.GetChannelMemberIDs(ctx, chID)
+	memberIDs, _ := s.db.GetChannelMemberIDs(ctx, chID)
 	recipients := excludeSession(memberIDs, sessionID)

 	dbID, uuid, fanErr := s.FanOut(
@@ -193,7 +210,7 @@ func (s *Service) SendDirectMessage(
 	nick, command, target string,
 	body, meta json.RawMessage,
 ) (*DirectMsgResult, error) {
-	targetSID, err := s.DB.GetSessionByNick(ctx, target)
+	targetSID, err := s.db.GetSessionByNick(ctx, target)
 	if err != nil {
 		return nil, &IRCError{
 			irc.ErrNoSuchNick,
@@ -202,7 +219,7 @@ func (s *Service) SendDirectMessage(
 		}
 	}

-	away, _ := s.DB.GetAway(ctx, targetSID)
+	away, _ := s.db.GetAway(ctx, targetSID)

 	recipients := []int64{targetSID}
 	if targetSID != sessionID {
@@ -228,19 +245,19 @@ func (s *Service) JoinChannel(
 	sessionID int64,
 	nick, channel, suppliedKey string,
 ) (*JoinResult, error) {
-	chID, err := s.DB.GetOrCreateChannel(ctx, channel)
+	chID, err := s.db.GetOrCreateChannel(ctx, channel)
 	if err != nil {
 		return nil, fmt.Errorf("get/create channel: %w", err)
 	}

-	memberCount, countErr := s.DB.CountChannelMembers(
+	memberCount, countErr := s.db.CountChannelMembers(
 		ctx, chID,
 	)
 	isCreator := countErr == nil && memberCount == 0

 	if !isCreator {
 		if joinErr := checkJoinRestrictions(
-			ctx, s.DB, chID, sessionID,
+			ctx, s.db, chID, sessionID,
 			channel, suppliedKey, memberCount,
 		); joinErr != nil {
 			return nil, joinErr
@@ -248,11 +265,11 @@ func (s *Service) JoinChannel(
 	}

 	if isCreator {
-		err = s.DB.JoinChannelAsOperator(
+		err = s.db.JoinChannelAsOperator(
 			ctx, chID, sessionID,
 		)
 	} else {
-		err = s.DB.JoinChannel(ctx, chID, sessionID)
+		err = s.db.JoinChannel(ctx, chID, sessionID)
 	}

 	if err != nil {
@@ -260,9 +277,9 @@ func (s *Service) JoinChannel(
 	}

 	// Clear invite after successful join.
-	_ = s.DB.ClearChannelInvite(ctx, chID, sessionID)
+	_ = s.db.ClearChannelInvite(ctx, chID, sessionID)

-	memberIDs, _ := s.DB.GetChannelMemberIDs(ctx, chID)
+	memberIDs, _ := s.db.GetChannelMemberIDs(ctx, chID)
 	body, _ := json.Marshal([]string{channel}) //nolint:errchkjson

 	_, _, _ = s.FanOut( //nolint:dogsled // fire-and-forget broadcast
@@ -284,7 +301,7 @@ func (s *Service) PartChannel(
 	sessionID int64,
 	nick, channel, reason string,
 ) error {
-	chID, err := s.DB.GetChannelByName(ctx, channel)
+	chID, err := s.db.GetChannelByName(ctx, channel)
 	if err != nil {
 		return &IRCError{
 			irc.ErrNoSuchChannel,
@@ -293,7 +310,7 @@ func (s *Service) PartChannel(
 		}
 	}

-	isMember, _ := s.DB.IsChannelMember(
+	isMember, _ := s.db.IsChannelMember(
 		ctx, chID, sessionID,
 	)
 	if !isMember {
@@ -304,7 +321,7 @@ func (s *Service) PartChannel(
 		}
 	}

-	memberIDs, _ := s.DB.GetChannelMemberIDs(ctx, chID)
+	memberIDs, _ := s.db.GetChannelMemberIDs(ctx, chID)
 	recipients := excludeSession(memberIDs, sessionID)
 	body, _ := json.Marshal([]string{reason}) //nolint:errchkjson

@@ -313,8 +330,8 @@ func (s *Service) PartChannel(
 		nil, body, nil, recipients,
 	)

-	s.DB.PartChannel(ctx, chID, sessionID) //nolint:errcheck,gosec
-	s.DB.DeleteChannelIfEmpty(ctx, chID)   //nolint:errcheck,gosec
+	s.db.PartChannel(ctx, chID, sessionID) //nolint:errcheck,gosec
+	s.db.DeleteChannelIfEmpty(ctx, chID)   //nolint:errcheck,gosec

 	return nil
 }
@@ -326,7 +343,7 @@ func (s *Service) SetTopic(
 	sessionID int64,
 	nick, channel, topic string,
 ) error {
-	chID, err := s.DB.GetChannelByName(ctx, channel)
+	chID, err := s.db.GetChannelByName(ctx, channel)
 	if err != nil {
 		return &IRCError{
 			irc.ErrNoSuchChannel,
@@ -335,7 +352,7 @@ func (s *Service) SetTopic(
 		}
 	}

-	isMember, _ := s.DB.IsChannelMember(
+	isMember, _ := s.db.IsChannelMember(
 		ctx, chID, sessionID,
 	)
 	if !isMember {
@@ -346,9 +363,9 @@ func (s *Service) SetTopic(
 		}
 	}

-	topicLocked, _ := s.DB.IsChannelTopicLocked(ctx, chID)
+	topicLocked, _ := s.db.IsChannelTopicLocked(ctx, chID)
 	if topicLocked {
-		isOp, _ := s.DB.IsChannelOperator(
+		isOp, _ := s.db.IsChannelOperator(
 			ctx, chID, sessionID,
 		)
 		if !isOp {
@@ -360,15 +377,15 @@ func (s *Service) SetTopic(
 		}
 	}

-	if setErr := s.DB.SetTopic(
+	if setErr := s.db.SetTopic(
 		ctx, channel, topic,
 	); setErr != nil {
 		return fmt.Errorf("set topic: %w", setErr)
 	}

-	_ = s.DB.SetTopicMeta(ctx, channel, topic, nick)
+	_ = s.db.SetTopicMeta(ctx, channel, topic, nick)

-	memberIDs, _ := s.DB.GetChannelMemberIDs(ctx, chID)
+	memberIDs, _ := s.db.GetChannelMemberIDs(ctx, chID)
 	body, _ := json.Marshal([]string{topic}) //nolint:errchkjson

 	_, _, _ = s.FanOut( //nolint:dogsled // fire-and-forget broadcast
@@ -387,7 +404,7 @@ func (s *Service) KickUser(
 	sessionID int64,
 	nick, channel, targetNick, reason string,
 ) error {
-	chID, err := s.DB.GetChannelByName(ctx, channel)
+	chID, err := s.db.GetChannelByName(ctx, channel)
 	if err != nil {
 		return &IRCError{
 			irc.ErrNoSuchChannel,
@@ -396,7 +413,7 @@ func (s *Service) KickUser(
 		}
 	}

-	isOp, _ := s.DB.IsChannelOperator(
+	isOp, _ := s.db.IsChannelOperator(
 		ctx, chID, sessionID,
 	)
 	if !isOp {
@@ -407,7 +424,7 @@ func (s *Service) KickUser(
 		}
 	}

-	targetSID, err := s.DB.GetSessionByNick(
+	targetSID, err := s.db.GetSessionByNick(
 		ctx, targetNick,
 	)
 	if err != nil {
@@ -418,7 +435,7 @@ func (s *Service) KickUser(
 		}
 	}

-	isMember, _ := s.DB.IsChannelMember(
+	isMember, _ := s.db.IsChannelMember(
 		ctx, chID, targetSID,
 	)
 	if !isMember {
@@ -429,7 +446,7 @@ func (s *Service) KickUser(
 		}
 	}

-	memberIDs, _ := s.DB.GetChannelMemberIDs(ctx, chID)
+	memberIDs, _ := s.db.GetChannelMemberIDs(ctx, chID)
 	body, _ := json.Marshal([]string{reason}) //nolint:errchkjson
 	params, _ := json.Marshal(                //nolint:errchkjson
 		[]string{targetNick},
@@ -440,8 +457,8 @@ func (s *Service) KickUser(
 		params, body, nil, memberIDs,
 	)

-	s.DB.PartChannel(ctx, chID, targetSID) //nolint:errcheck,gosec
-	s.DB.DeleteChannelIfEmpty(ctx, chID)   //nolint:errcheck,gosec
+	s.db.PartChannel(ctx, chID, targetSID) //nolint:errcheck,gosec
+	s.db.DeleteChannelIfEmpty(ctx, chID)   //nolint:errcheck,gosec

 	return nil
 }
@@ -453,7 +470,7 @@ func (s *Service) ChangeNick(
 	sessionID int64,
 	oldNick, newNick string,
 ) error {
-	err := s.DB.ChangeNick(ctx, sessionID, newNick)
+	err := s.db.ChangeNick(ctx, sessionID, newNick)
 	if err != nil {
 		if strings.Contains(err.Error(), "UNIQUE") ||
 			db.IsUniqueConstraintError(err) {
@@ -485,7 +502,7 @@ func (s *Service) BroadcastQuit(
 	sessionID int64,
 	nick, reason string,
 ) {
-	channels, err := s.DB.GetSessionChannels(
+	channels, err := s.db.GetSessionChannels(
 		ctx, sessionID,
 	)
 	if err != nil {
@@ -495,7 +512,7 @@ func (s *Service) BroadcastQuit(
 	notified := make(map[int64]bool)

 	for _, ch := range channels {
-		memberIDs, memErr := s.DB.GetChannelMemberIDs(
+		memberIDs, memErr := s.db.GetChannelMemberIDs(
 			ctx, ch.ID,
 		)
 		if memErr != nil {
@@ -526,11 +543,11 @@ func (s *Service) BroadcastQuit(
 	}

 	for _, ch := range channels {
-		s.DB.PartChannel(ctx, ch.ID, sessionID) //nolint:errcheck,gosec
-		s.DB.DeleteChannelIfEmpty(ctx, ch.ID)   //nolint:errcheck,gosec
+		s.db.PartChannel(ctx, ch.ID, sessionID) //nolint:errcheck,gosec
+		s.db.DeleteChannelIfEmpty(ctx, ch.ID)   //nolint:errcheck,gosec
 	}

-	s.DB.DeleteSession(ctx, sessionID) //nolint:errcheck,gosec
+	s.db.DeleteSession(ctx, sessionID) //nolint:errcheck,gosec
 }

 // SetAway sets or clears the away message. Returns true
@@ -540,7 +557,7 @@ func (s *Service) SetAway(
 	sessionID int64,
 	message string,
 ) (bool, error) {
-	err := s.DB.SetAway(ctx, sessionID, message)
+	err := s.db.SetAway(ctx, sessionID, message)
 	if err != nil {
 		return false, fmt.Errorf("set away: %w", err)
 	}
@@ -555,8 +572,8 @@ func (s *Service) Oper(
 	sessionID int64,
 	name, password string,
 ) error {
-	cfgName := s.Config.OperName
-	cfgPassword := s.Config.OperPassword
+	cfgName := s.config.OperName
+	cfgPassword := s.config.OperPassword

 	// Use constant-time comparison and return the same
 	// error for all failures to prevent information
@@ -575,7 +592,7 @@ func (s *Service) Oper(
 		}
 	}

-	_ = s.DB.SetSessionOper(ctx, sessionID, true)
+	_ = s.db.SetSessionOper(ctx, sessionID, true)

 	return nil
 }
@@ -587,7 +604,7 @@ func (s *Service) ValidateChannelOp(
 	sessionID int64,
 	channel string,
 ) (int64, error) {
-	chID, err := s.DB.GetChannelByName(ctx, channel)
+	chID, err := s.db.GetChannelByName(ctx, channel)
 	if err != nil {
 		return 0, &IRCError{
 			irc.ErrNoSuchChannel,
@@ -596,7 +613,7 @@ func (s *Service) ValidateChannelOp(
 		}
 	}

-	isOp, _ := s.DB.IsChannelOperator(
+	isOp, _ := s.db.IsChannelOperator(
 		ctx, chID, sessionID,
 	)
 	if !isOp {
@@ -619,7 +636,7 @@ func (s *Service) ApplyMemberMode(
 	mode rune,
 	adding bool,
 ) error {
-	targetSID, err := s.DB.GetSessionByNick(
+	targetSID, err := s.db.GetSessionByNick(
 		ctx, targetNick,
 	)
 	if err != nil {
@@ -630,7 +647,7 @@ func (s *Service) ApplyMemberMode(
 		}
 	}

-	isMember, _ := s.DB.IsChannelMember(
+	isMember, _ := s.db.IsChannelMember(
 		ctx, chID, targetSID,
 	)
 	if !isMember {
@@ -643,11 +660,11 @@ func (s *Service) ApplyMemberMode(

 	switch mode {
 	case 'o':
-		_ = s.DB.SetChannelMemberOperator(
+		_ = s.db.SetChannelMemberOperator(
 			ctx, chID, targetSID, adding,
 		)
 	case 'v':
-		_ = s.DB.SetChannelMemberVoiced(
+		_ = s.db.SetChannelMemberVoiced(
 			ctx, chID, targetSID, adding,
 		)
 	}
@@ -655,7 +672,8 @@ func (s *Service) ApplyMemberMode(
 	return nil
 }

-// SetChannelFlag applies +m/-m or +t/-t on a channel.
+// SetChannelFlag applies a simple boolean channel mode
+// (+m/-m, +t/-t, +i/-i, +s/-s, +n/-n).
 func (s *Service) SetChannelFlag(
 	ctx context.Context,
 	chID int64,
@@ -664,29 +682,37 @@ func (s *Service) SetChannelFlag(
 ) error {
 	switch flag {
 	case 'm':
-		if err := s.DB.SetChannelModerated(
+		if err := s.db.SetChannelModerated(
 			ctx, chID, setting,
 		); err != nil {
 			return fmt.Errorf("set moderated: %w", err)
 		}
 	case 't':
-		if err := s.DB.SetChannelTopicLocked(
+		if err := s.db.SetChannelTopicLocked(
 			ctx, chID, setting,
 		); err != nil {
 			return fmt.Errorf("set topic locked: %w", err)
 		}
 	case 'i':
-		if err := s.DB.SetChannelInviteOnly(
+		if err := s.db.SetChannelInviteOnly(
 			ctx, chID, setting,
 		); err != nil {
 			return fmt.Errorf("set invite only: %w", err)
 		}
 	case 's':
-		if err := s.DB.SetChannelSecret(
+		if err := s.db.SetChannelSecret(
 			ctx, chID, setting,
 		); err != nil {
 			return fmt.Errorf("set secret: %w", err)
 		}
+	case 'n':
+		if err := s.db.SetChannelNoExternal(
+			ctx, chID, setting,
+		); err != nil {
+			return fmt.Errorf(
+				"set no external: %w", err,
+			)
+		}
 	}

 	return nil
@@ -700,7 +726,7 @@ func (s *Service) BroadcastMode(
 	chID int64,
 	modeText string,
 ) {
-	memberIDs, _ := s.DB.GetChannelMemberIDs(ctx, chID)
+	memberIDs, _ := s.db.GetChannelMemberIDs(ctx, chID)
 	body, _ := json.Marshal([]string{modeText}) //nolint:errchkjson

 	_, _, _ = s.FanOut( //nolint:dogsled // fire-and-forget broadcast
@@ -709,24 +735,60 @@ func (s *Service) BroadcastMode(
 	)
 }

-// QueryChannelMode returns the channel mode string.
+// QueryChannelMode returns the complete channel mode
+// string including all flags and parameterized modes.
 func (s *Service) QueryChannelMode(
 	ctx context.Context,
 	chID int64,
 ) string {
 	modes := "+"

-	moderated, _ := s.DB.IsChannelModerated(ctx, chID)
+	noExternal, _ := s.db.IsChannelNoExternal(ctx, chID)
+	if noExternal {
+		modes += "n"
+	}
+
+	inviteOnly, _ := s.db.IsChannelInviteOnly(ctx, chID)
+	if inviteOnly {
+		modes += "i"
+	}
+
+	moderated, _ := s.db.IsChannelModerated(ctx, chID)
 	if moderated {
 		modes += "m"
 	}

-	topicLocked, _ := s.DB.IsChannelTopicLocked(ctx, chID)
+	secret, _ := s.db.IsChannelSecret(ctx, chID)
+	if secret {
+		modes += "s"
+	}
+
+	topicLocked, _ := s.db.IsChannelTopicLocked(ctx, chID)
 	if topicLocked {
 		modes += "t"
 	}

-	return modes
+	var modeParams string
+
+	key, _ := s.db.GetChannelKey(ctx, chID)
+	if key != "" {
+		modes += "k"
+		modeParams += " " + key
+	}
+
+	limit, _ := s.db.GetChannelUserLimit(ctx, chID)
+	if limit > 0 {
+		modes += "l"
+		modeParams += " " + strconv.Itoa(limit)
+	}
+
+	bits, _ := s.db.GetChannelHashcashBits(ctx, chID)
+	if bits > 0 {
+		modes += "H"
+		modeParams += " " + strconv.Itoa(bits)
+	}
+
+	return modes + modeParams
 }

 // broadcastNickChange notifies channel peers of a nick
@@ -736,7 +798,7 @@ func (s *Service) broadcastNickChange(
 	sessionID int64,
 	oldNick, newNick string,
 ) {
-	channels, err := s.DB.GetSessionChannels(
+	channels, err := s.db.GetSessionChannels(
 		ctx, sessionID,
 	)
 	if err != nil {
@@ -746,7 +808,7 @@ func (s *Service) broadcastNickChange(
 	body, _ := json.Marshal([]string{newNick}) //nolint:errchkjson
 	notified := make(map[int64]bool)

-	dbID, _, insErr := s.DB.InsertMessage(
+	dbID, _, insErr := s.db.InsertMessage(
 		ctx, irc.CmdNick, oldNick, "",
 		nil, body, nil,
 	)
@@ -755,12 +817,12 @@ func (s *Service) broadcastNickChange(
 	}

 	// Notify the user themselves (for multi-client sync).
-	_ = s.DB.EnqueueToSession(ctx, sessionID, dbID)
-	s.Broker.Notify(sessionID)
+	_ = s.db.EnqueueToSession(ctx, sessionID, dbID)
+	s.broker.Notify(sessionID)
 	notified[sessionID] = true

 	for _, ch := range channels {
-		memberIDs, memErr := s.DB.GetChannelMemberIDs(
+		memberIDs, memErr := s.db.GetChannelMemberIDs(
 			ctx, ch.ID,
 		)
 		if memErr != nil {
@@ -774,8 +836,8 @@ func (s *Service) broadcastNickChange(

 			notified[mid] = true

-			_ = s.DB.EnqueueToSession(ctx, mid, dbID)
-			s.Broker.Notify(mid)
+			_ = s.db.EnqueueToSession(ctx, mid, dbID)
+			s.broker.Notify(mid)
 		}
 	}
 }