Răsfoiți Sursa

toc: don't trigger events from toc_set_config

I discovered that toc_set_config does not need to trigger buddy
arrival and departure events for users that are added/removed
from permit/deny lists. When a user is added or removed, TOC
clients send toc_add_buddy, toc_add_deny, etc in addition to
toc_set_config.

I also surface connection errors that are raised at logout that
were previously not surfaced.
Mike 1 an în urmă
părinte
comite
0b741743ce
4 a modificat fișierele cu 43 adăugiri și 425 ștergeri
  1. 15 113
      server/toc/cmd_client.go
  2. 15 310
      server/toc/cmd_client_test.go
  3. 1 0
      server/toc/cmd_server.go
  4. 12 2
      server/toc/server.go

+ 15 - 113
server/toc/cmd_client.go

@@ -147,6 +147,7 @@ func (s OSCARProxy) RecvClientCmd(
 	toCh chan<- []byte,
 	doAsync func(f func() error),
 ) (reply string) {
+
 	cmd := payload
 	var args []byte
 	if idx := bytes.IndexByte(payload, ' '); idx > -1 {
@@ -154,7 +155,7 @@ func (s OSCARProxy) RecvClientCmd(
 	}
 
 	if s.Logger.Enabled(ctx, slog.LevelDebug) {
-		s.Logger.DebugContext(ctx, "client request", "command", args)
+		s.Logger.DebugContext(ctx, "client request", "command", cmd)
 	} else {
 		s.Logger.InfoContext(ctx, "client request", "command", cmd)
 	}
@@ -1359,123 +1360,24 @@ func (s OSCARProxy) SetCaps(ctx context.Context, me *state.Session, args []byte)
 //		- 3 - Permit Some
 //		- 4 - Deny Some
 //
+// This method doesn't attempt to validate any of the configuration--it saves
+// the config as received from the client.
+//
 // Command syntax: toc_set_config <Config Info>
 func (s OSCARProxy) SetConfig(ctx context.Context, me *state.Session, args []byte) string {
-	// replace curly braces with quotes so that the string can be properly
-	// split up by the space-delimited reader
-	for i, c := range args {
-		if c == '{' || c == '}' {
-			args[i] = '"'
-		}
-	}
-	args = bytes.TrimSpace(args)
-
-	var info string
-	if _, err := parseArgs(args, &info); err != nil {
-		return s.runtimeErr(ctx, fmt.Errorf("parseArgs: %w", err))
-	}
-
-	config := strings.Split(info, "\n")
-
-	var cfg [][2]string
-	for _, item := range config {
-		parts := strings.Split(item, " ")
-		if len(parts) != 2 {
-			s.Logger.InfoContext(ctx, "invalid config item", "item", item, "user", me.DisplayScreenName())
-			continue
-		}
-		cfg = append(cfg, [2]string{parts[0], parts[1]})
-	}
-
-	mode := wire.FeedbagPDModePermitAll
-	for _, c := range cfg {
-		if c[0] != "m" {
-			continue
-		}
-		switch c[1] {
-		case "1":
-			mode = wire.FeedbagPDModePermitAll
-		case "2":
-			mode = wire.FeedbagPDModeDenyAll
-		case "3":
-			mode = wire.FeedbagPDModePermitSome
-		case "4":
-			mode = wire.FeedbagPDModeDenySome
-		default:
-			return s.runtimeErr(ctx, fmt.Errorf("config: invalid mode `%s`", c[1]))
-		}
-	}
-
-	switch mode {
-	case wire.FeedbagPDModePermitAll:
-		snac := wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{
-			Users: []struct {
-				ScreenName string `oscar:"len_prefix=uint8"`
-			}{
-				{
-					ScreenName: me.IdentScreenName().String(),
-				},
-			},
-		}
-		if err := s.PermitDenyService.AddDenyListEntries(ctx, me, snac); err != nil {
-			return s.runtimeErr(ctx, fmt.Errorf("PermitDenyService.AddDenyListEntries: %w", err))
-		}
-	case wire.FeedbagPDModeDenyAll:
-		snac := wire.SNAC_0x09_0x05_PermitDenyAddPermListEntries{
-			Users: []struct {
-				ScreenName string `oscar:"len_prefix=uint8"`
-			}{
-				{
-					ScreenName: me.IdentScreenName().String(),
-				},
-			},
-		}
-		if err := s.PermitDenyService.AddPermListEntries(ctx, me, snac); err != nil {
-			return s.runtimeErr(ctx, fmt.Errorf("PermitDenyService.AddPermListEntrie: %w", err))
-		}
-	case wire.FeedbagPDModePermitSome:
-		snac := wire.SNAC_0x09_0x05_PermitDenyAddPermListEntries{}
-		for _, c := range cfg {
-			if c[0] != "p" {
-				continue
-			}
-			snac.Users = append(snac.Users, struct {
-				ScreenName string `oscar:"len_prefix=uint8"`
-			}{ScreenName: c[1]})
-		}
-		if err := s.PermitDenyService.AddPermListEntries(ctx, me, snac); err != nil {
-			return s.runtimeErr(ctx, fmt.Errorf("PermitDenyService.AddPermListEntrie: %w", err))
-		}
-	case wire.FeedbagPDModeDenySome:
-		snac := wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{}
-		for _, c := range cfg {
-			if c[0] != "d" {
-				continue
-			}
-			snac.Users = append(snac.Users, struct {
-				ScreenName string `oscar:"len_prefix=uint8"`
-			}{ScreenName: c[1]})
-		}
-		if err := s.PermitDenyService.AddDenyListEntries(ctx, me, snac); err != nil {
-			return s.runtimeErr(ctx, fmt.Errorf("PermitDenyService.AddDenyListEntries: %w", err))
-		}
-	}
-
-	snac := wire.SNAC_0x03_0x04_BuddyAddBuddies{}
-	for _, c := range cfg {
-		if c[0] != "b" {
-			continue
-		}
-		snac.Buddies = append(snac.Buddies, struct {
-			ScreenName string `oscar:"len_prefix=uint8"`
-		}{ScreenName: c[1]})
-	}
+	// most TOC clients don't quote the config info argument, despite what the
+	// documentation specifies. this makes the argument payload incompatible
+	// for CSV parsing. since this command takes a single argument, we can get
+	// away with trimming quotes and spaces from the byte slice before passing
+	// it to the config store.
+	args = bytes.Trim(args, "'\" ")
 
-	if err := s.BuddyService.AddBuddies(ctx, me, snac); err != nil {
-		return s.runtimeErr(ctx, fmt.Errorf("BuddyService.AddBuddies: %w", err))
+	config := string(args)
+	if config == "" {
+		return s.runtimeErr(ctx, fmt.Errorf("empty config"))
 	}
 
-	if err := s.TOCConfigStore.SetTOCConfig(me.IdentScreenName(), info); err != nil {
+	if err := s.TOCConfigStore.SetTOCConfig(me.IdentScreenName(), config); err != nil {
 		return s.runtimeErr(ctx, fmt.Errorf("TOCConfigStore.SaveTOCConfig: %w", err))
 	}
 

+ 15 - 310
server/toc/cmd_client_test.go

@@ -3213,377 +3213,82 @@ func TestOSCARProxy_RecvClientCmd_SetConfig(t *testing.T) {
 		mockParams mockParams
 	}{
 		{
-			name:     "successfully set permit all config",
+			name:     "successfully set permit all config (unquoted)",
 			me:       newTestSession("me"),
 			givenCmd: []byte("toc_set_config {m 1\ng Buddies\nb friend1\nb friend2\n}\n"),
 			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					// confusingly, setting "permit all" mode requires adding a
-					// deny list entry
-					addDenyListEntriesParams: addDenyListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "me"},
-								},
-							},
-						},
-					},
-				},
-				buddyParams: buddyParams{
-					addBuddiesParams: addBuddiesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							inBody: wire.SNAC_0x03_0x04_BuddyAddBuddies{
-								Buddies: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend1"},
-									{ScreenName: "friend2"},
-								},
-							},
-						},
-					},
-				},
 				tocConfigParams: tocConfigParams{
 					setTOCConfigParams: setTOCConfigParams{
 						{
 							user:   state.NewIdentScreenName("me"),
-							config: "m 1\ng Buddies\nb friend1\nb friend2",
+							config: "{m 1\ng Buddies\nb friend1\nb friend2\n}\n",
 						},
 					},
 				},
 			},
 		},
 		{
-			name:     "set permit all config, receive err from config store svc",
+			name:     "successfully set permit all config (double-quoted)",
 			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 1\ng Buddies\nb friend1\n}\n"),
+			givenCmd: []byte("toc_set_config \"{m 1\ng Buddies\nb friend1\nb friend2\n}\n\""),
 			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					// confusingly, setting "permit all" mode requires adding a
-					// deny list entry
-					addDenyListEntriesParams: addDenyListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "me"},
-								},
-							},
-						},
-					},
-				},
-				buddyParams: buddyParams{
-					addBuddiesParams: addBuddiesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							inBody: wire.SNAC_0x03_0x04_BuddyAddBuddies{
-								Buddies: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend1"},
-								},
-							},
-						},
-					},
-				},
 				tocConfigParams: tocConfigParams{
 					setTOCConfigParams: setTOCConfigParams{
 						{
 							user:   state.NewIdentScreenName("me"),
-							config: "m 1\ng Buddies\nb friend1",
-							err:    io.EOF,
+							config: "{m 1\ng Buddies\nb friend1\nb friend2\n}\n",
 						},
 					},
 				},
 			},
-			wantMsg: cmdInternalSvcErr,
 		},
 		{
-			name:     "set permit all config, receive err from buddy svc",
+			name:     "successfully set permit all config (single-quoted)",
 			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 1\ng Buddies\nb friend1\n}\n"),
+			givenCmd: []byte("toc_set_config '{m 1\ng Buddies\nb friend1\nb friend2\n}\n'"),
 			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					// confusingly, setting "permit all" mode requires adding a
-					// deny list entry
-					addDenyListEntriesParams: addDenyListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "me"},
-								},
-							},
-						},
-					},
-				},
-				buddyParams: buddyParams{
-					addBuddiesParams: addBuddiesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							inBody: wire.SNAC_0x03_0x04_BuddyAddBuddies{
-								Buddies: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend1"},
-								},
-							},
-							err: io.EOF,
-						},
-					},
-				},
-			},
-			wantMsg: cmdInternalSvcErr,
-		},
-		{
-			name:     "set permit all config, receive err from permit-deny svc",
-			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 1\ng Buddies\nb friend1\nb friend2\n}\n"),
-			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					// confusingly, setting "permit all" mode requires adding a
-					// deny list entry
-					addDenyListEntriesParams: addDenyListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "me"},
-								},
-							},
-							err: io.EOF,
-						},
-					},
-				},
-			},
-			wantMsg: cmdInternalSvcErr,
-		},
-		{
-			name:     "successfully set deny all config",
-			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 2\ng Buddies\nb friend1\nb friend2\n}\n"),
-			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					// confusingly, setting "deny all" mode requires adding a
-					// permit list entry
-					addPermListEntriesParams: addPermListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x05_PermitDenyAddPermListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "me"},
-								},
-							},
-						},
-					},
-				},
-				buddyParams: buddyParams{
-					addBuddiesParams: addBuddiesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							inBody: wire.SNAC_0x03_0x04_BuddyAddBuddies{
-								Buddies: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend1"},
-									{ScreenName: "friend2"},
-								},
-							},
-						},
-					},
-				},
 				tocConfigParams: tocConfigParams{
 					setTOCConfigParams: setTOCConfigParams{
 						{
 							user:   state.NewIdentScreenName("me"),
-							config: "m 2\ng Buddies\nb friend1\nb friend2",
+							config: "{m 1\ng Buddies\nb friend1\nb friend2\n}\n",
 						},
 					},
 				},
 			},
 		},
 		{
-			name:     "set deny all config, receive err from permit-deny svc",
+			name:     "successfully set permit all config (double-quoted with spaces)",
 			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 2\ng Buddies\nb friend1\nb friend2\n}\n"),
+			givenCmd: []byte("toc_set_config \" {m 1\ng Buddies\nb friend1\nb friend2\n}\n \""),
 			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					// confusingly, setting "deny all" mode requires adding a
-					// permit list entry
-					addPermListEntriesParams: addPermListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x05_PermitDenyAddPermListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "me"},
-								},
-							},
-							err: io.EOF,
-						},
-					},
-				},
-			},
-			wantMsg: cmdInternalSvcErr,
-		},
-		{
-			name:     "successfully set permit some config",
-			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 3\np friend3\np friend4\n\ng Buddies\nb friend1\nb friend2\n}\n"),
-			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					addPermListEntriesParams: addPermListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x05_PermitDenyAddPermListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend3"},
-									{ScreenName: "friend4"},
-								},
-							},
-						},
-					},
-				},
-				buddyParams: buddyParams{
-					addBuddiesParams: addBuddiesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							inBody: wire.SNAC_0x03_0x04_BuddyAddBuddies{
-								Buddies: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend1"},
-									{ScreenName: "friend2"},
-								},
-							},
-						},
-					},
-				},
 				tocConfigParams: tocConfigParams{
 					setTOCConfigParams: setTOCConfigParams{
 						{
 							user:   state.NewIdentScreenName("me"),
-							config: "m 3\np friend3\np friend4\n\ng Buddies\nb friend1\nb friend2",
-						},
-					},
-				},
-			},
-		},
-		{
-			name:     "set permit some config, receive err from permit-deny svc",
-			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 3\np friend3\np friend4\n\ng Buddies\nb friend1\nb friend2\n}\n"),
-			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					addPermListEntriesParams: addPermListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x05_PermitDenyAddPermListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend3"},
-									{ScreenName: "friend4"},
-								},
-							},
-							err: io.EOF,
+							config: "{m 1\ng Buddies\nb friend1\nb friend2\n}\n",
 						},
 					},
 				},
 			},
-			wantMsg: cmdInternalSvcErr,
 		},
 		{
-			name:     "successfully set deny some config",
+			name:     "set config, receive error from toc config store",
 			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 4\nd friend3\nd friend4\n\ng Buddies\nb friend1\nb friend2\n}\n"),
+			givenCmd: []byte("toc_set_config {m 1\ng Buddies\nb friend1\nb friend2\n}\n"),
 			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					addDenyListEntriesParams: addDenyListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend3"},
-									{ScreenName: "friend4"},
-								},
-							},
-						},
-					},
-				},
-				buddyParams: buddyParams{
-					addBuddiesParams: addBuddiesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							inBody: wire.SNAC_0x03_0x04_BuddyAddBuddies{
-								Buddies: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend1"},
-									{ScreenName: "friend2"},
-								},
-							},
-						},
-					},
-				},
 				tocConfigParams: tocConfigParams{
 					setTOCConfigParams: setTOCConfigParams{
 						{
 							user:   state.NewIdentScreenName("me"),
-							config: "m 4\nd friend3\nd friend4\n\ng Buddies\nb friend1\nb friend2",
-						},
-					},
-				},
-			},
-		},
-		{
-			name:     "set deny some config, receive err from permit-deny svc",
-			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 4\nd friend3\nd friend4\n\ng Buddies\nb friend1\nb friend2\n}\n"),
-			mockParams: mockParams{
-				permitDenyParams: permitDenyParams{
-					addDenyListEntriesParams: addDenyListEntriesParams{
-						{
-							me: state.NewIdentScreenName("me"),
-							body: wire.SNAC_0x09_0x07_PermitDenyAddDenyListEntries{
-								Users: []struct {
-									ScreenName string `oscar:"len_prefix=uint8"`
-								}{
-									{ScreenName: "friend3"},
-									{ScreenName: "friend4"},
-								},
-							},
-							err: io.EOF,
+							config: "{m 1\ng Buddies\nb friend1\nb friend2\n}\n",
+							err:    io.EOF,
 						},
 					},
 				},
 			},
 			wantMsg: cmdInternalSvcErr,
 		},
-		{
-			name:     "set unknown PD mode",
-			me:       newTestSession("me"),
-			givenCmd: []byte("toc_set_config {m 5\nd friend3\nd friend4\n\ng Buddies\nb friend1\nb friend2\n}\n"),
-			wantMsg:  cmdInternalSvcErr,
-		},
 		{
 			name:     "bad command",
 			givenCmd: []byte(`toc_set_config`),

+ 1 - 0
server/toc/cmd_server.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"github.com/google/uuid"
+
 	"github.com/mk6i/retro-aim-server/state"
 	"github.com/mk6i/retro-aim-server/wire"
 )

+ 12 - 2
server/toc/server.go

@@ -12,6 +12,7 @@ import (
 	"net/http"
 	"net/netip"
 	"sync"
+	"syscall"
 	"time"
 
 	"golang.org/x/sync/errgroup"
@@ -137,7 +138,9 @@ func (rt Server) Start(ctx context.Context) error {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			rt.dispatchConn(conn, ctx, httpCh)
+			if err := rt.dispatchConn(conn, ctx, httpCh); err != nil {
+				rt.Logger.ErrorContext(ctx, "client disconnected with error", "err", err.Error())
+			}
 		}()
 	}
 
@@ -162,15 +165,22 @@ func (rt Server) dispatchConn(conn net.Conn, ctx context.Context, httpCh chan ne
 		return fmt.Errorf("bufCon.Peek: %w", err)
 	}
 
+	// handle TOC/FLAP
 	if string(buf) == doFlap {
 		if err = rt.dispatchFLAP(ctx, bufCon); err != nil {
-			if !(errors.Is(err, io.EOF) || errors.Is(err, net.ErrClosed)) {
+			switch {
+			case errors.Is(err, io.EOF):
+			case errors.Is(err, net.ErrClosed):
+			case errors.Is(err, syscall.ECONNRESET):
+				return nil
+			default:
 				return fmt.Errorf("rt.dispatchFLAP: %w", err)
 			}
 		}
 		return nil
 	}
 
+	// handle TOC/HTTP
 	select {
 	case httpCh <- bufCon:
 		return nil