issue #56 - fix chat rooms in macOS AIM 4.0.9

This commits introduces the following changes in order to fix chat rooms
in macOS client v4.0.9:

- Remove ChatNav from chat OServiceHostOnline handler response
- Change the ordering in chat ClientOnline sequence
- Fix bad channel ID in ChatChannelMsgToHost handler
- Change aim chat URL parameter sequence
- Remove reflection TLV in ChannelMsgToHost handler

foodgroup/chat.go

@@ -3,6 +3,7 @@ package foodgroup
 import (
 	"context"
 	"errors"
+	"math"
 
 	"github.com/mk6i/retro-aim-server/state"
 	"github.com/mk6i/retro-aim-server/wire"
@@ -50,6 +51,11 @@ func (s ChatService) ChannelMsgToHost(ctx context.Context, sess *state.Session,
 		},
 	}
 
+	if bodyOut.Channel == math.MaxUint16 {
+		// Fix incorrect channel bug in macOS client v4.0.9.
+		bodyOut.Channel = wire.ICBMChannelMIME
+	}
+
 	// send message to all the participants except sender
 	s.chatMessageRelayer.RelayToAllExcept(ctx, sess.ChatRoomCookie(), sess.IdentScreenName(), wire.SNACMessage{
 		Frame: frameOut,

foodgroup/chat_test.go

@@ -2,6 +2,7 @@ package foodgroup
 
 import (
 	"context"
+	"math"
 	"testing"
 
 	"github.com/mk6i/retro-aim-server/state"
@@ -105,6 +106,62 @@ func TestChatService_ChannelMsgToHost(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "send chat room message with macOS client 4.0.9 bug containing bad channel ID, expect message to " +
+				"client on MIME channel",
+			userSession: newTestSession("user_sending_chat_msg", sessOptCannedSignonTime,
+				sessOptChatRoomCookie("the-chat-cookie")),
+			inputSNAC: wire.SNACMessage{
+				Frame: wire.SNACFrame{
+					RequestID: 1234,
+				},
+				Body: wire.SNAC_0x0E_0x05_ChatChannelMsgToHost{
+					Cookie:  1234,
+					Channel: math.MaxUint16,
+					TLVRestBlock: wire.TLVRestBlock{
+						TLVList: wire.TLVList{
+							{
+								Tag:   wire.ChatTLVPublicWhisperFlag,
+								Value: []byte{},
+							},
+							{
+								Tag:   wire.ChatTLVMessageInformation,
+								Value: []byte{},
+							},
+						},
+					},
+				},
+			},
+			mockParams: mockParams{
+				chatMessageRelayerParams: chatMessageRelayerParams{
+					chatRelayToAllExceptParams: chatRelayToAllExceptParams{
+						{
+							screenName: state.NewIdentScreenName("user_sending_chat_msg"),
+							cookie:     "the-chat-cookie",
+							message: wire.SNACMessage{
+								Frame: wire.SNACFrame{
+									FoodGroup: wire.Chat,
+									SubGroup:  wire.ChatChannelMsgToClient,
+								},
+								Body: wire.SNAC_0x0E_0x06_ChatChannelMsgToClient{
+									Cookie:  1234,
+									Channel: wire.ICBMChannelMIME,
+									TLVRestBlock: wire.TLVRestBlock{
+										TLVList: wire.TLVList{
+											wire.NewTLV(wire.ChatTLVSenderInformation,
+												newTestSession("user_sending_chat_msg", sessOptCannedSignonTime).TLVUserInfo()),
+											wire.NewTLV(wire.ChatTLVPublicWhisperFlag, []byte{}),
+											wire.NewTLV(wire.ChatTLVMessageInformation, []byte{}),
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectOutput: nil,
+		},
 		{
 			name: "send chat room message, don't expect acknowledgement to sender client",
 			userSession: newTestSession("user_sending_chat_msg", sessOptCannedSignonTime,

foodgroup/icbm.go

@@ -121,16 +121,22 @@ func (s ICBMService) ChannelMsgToHost(ctx context.Context, sess *state.Session,
 		TLVRestBlock: wire.TLVRestBlock{
 			TLVList: wire.TLVList{
 				{
-					Tag:   0x0B,
+					// todo only add this TLV if the sender wants client events
+					Tag:   wire.ICBMTLVWantEvents,
 					Value: []byte{},
 				},
 			},
 		},
 	}
-	// copy over TLVs from sender SNAC to recipient SNAC verbatim. this
-	// includes ICBMTLVRequestHostAck, which is ignored by the client, as
-	// far as I can tell.
-	clientIM.AppendList(inBody.TLVRestBlock.TLVList)
+
+	for _, tlv := range inBody.TLVRestBlock.TLVList {
+		if tlv.Tag == wire.ICBMTLVRequestHostAck {
+			// Exclude this TLV, because its presence breaks chat invitations
+			// on macOS client v4.0.9.
+			continue
+		}
+		clientIM.Append(tlv)
+	}
 
 	s.messageRelayer.RelayToScreenName(ctx, recipSess.IdentScreenName(), wire.SNACMessage{
 		Frame: wire.SNACFrame{
@@ -188,6 +194,24 @@ func (s ICBMService) ClientEvent(ctx context.Context, sess *state.Session, inFra
 	}
 }
 
+func (s ICBMService) ClientErr(ctx context.Context, sess *state.Session, inFrame wire.SNACFrame, inBody wire.SNAC_0x04_0x0B_ICBMClientErr) error {
+	s.messageRelayer.RelayToScreenName(ctx, state.NewIdentScreenName(inBody.ScreenName), wire.SNACMessage{
+		Frame: wire.SNACFrame{
+			FoodGroup: wire.ICBM,
+			SubGroup:  wire.ICBMClientErr,
+			RequestID: inFrame.RequestID,
+		},
+		Body: wire.SNAC_0x04_0x0B_ICBMClientErr{
+			Cookie:     inBody.Cookie,
+			ChannelID:  inBody.ChannelID,
+			ScreenName: sess.DisplayScreenName().String(),
+			Code:       inBody.Code,
+			ErrInfo:    inBody.ErrInfo,
+		},
+	})
+	return nil
+}
+
 // EvilRequest handles user warning (a.k.a evil) notifications. It receives
 // wire.ICBMEvilRequest warning SNAC, increments the warned user's warning
 // level, and sends the warned user a notification informing them that they

foodgroup/icbm_test.go

@@ -50,6 +50,10 @@ func TestICBMService_ChannelMsgToHost(t *testing.T) {
 								Tag:   wire.ICBMTLVRequestHostAck,
 								Value: []byte{},
 							},
+							{
+								Tag:   wire.ICBMTLVData,
+								Value: []byte{1, 2, 3, 4},
+							},
 						},
 					},
 				},
@@ -68,8 +72,8 @@ func TestICBMService_ChannelMsgToHost(t *testing.T) {
 								Value: []byte{},
 							},
 							{
-								Tag:   wire.ICBMTLVRequestHostAck,
-								Value: []byte{},
+								Tag:   wire.ICBMTLVData,
+								Value: []byte{1, 2, 3, 4},
 							},
 						},
 					},
@@ -98,7 +102,12 @@ func TestICBMService_ChannelMsgToHost(t *testing.T) {
 				Body: wire.SNAC_0x04_0x06_ICBMChannelMsgToHost{
 					ScreenName: "recipient-screen-name",
 					TLVRestBlock: wire.TLVRestBlock{
-						TLVList: wire.TLVList{},
+						TLVList: wire.TLVList{
+							{
+								Tag:   wire.ICBMTLVData,
+								Value: []byte{1, 2, 3, 4},
+							},
+						},
 					},
 				},
 			},
@@ -115,6 +124,10 @@ func TestICBMService_ChannelMsgToHost(t *testing.T) {
 								Tag:   wire.ICBMTLVWantEvents,
 								Value: []byte{},
 							},
+							{
+								Tag:   wire.ICBMTLVData,
+								Value: []byte{1, 2, 3, 4},
+							},
 						},
 					},
 				},
@@ -723,3 +736,39 @@ func TestICBMService_ParameterQuery(t *testing.T) {
 
 	assert.Equal(t, want, have)
 }
+
+func TestICBMService_ClientErr(t *testing.T) {
+	sess := newTestSession("theScreenName")
+
+	inBody := wire.SNAC_0x04_0x0B_ICBMClientErr{
+		Cookie:     1234,
+		ChannelID:  wire.ICBMChannelMIME,
+		ScreenName: "recipientScreenName",
+		Code:       10,
+		ErrInfo:    []byte{1, 2, 3, 4},
+	}
+
+	expect := wire.SNACMessage{
+		Frame: wire.SNACFrame{
+			FoodGroup: wire.ICBM,
+			SubGroup:  wire.ICBMClientErr,
+			RequestID: 1234,
+		},
+		Body: wire.SNAC_0x04_0x0B_ICBMClientErr{
+			Cookie:     inBody.Cookie,
+			ChannelID:  inBody.ChannelID,
+			ScreenName: sess.DisplayScreenName().String(),
+			Code:       inBody.Code,
+			ErrInfo:    inBody.ErrInfo,
+		},
+	}
+
+	messageRelayer := newMockMessageRelayer(t)
+	messageRelayer.EXPECT().
+		RelayToScreenName(mock.Anything, state.NewIdentScreenName("recipientScreenName"), expect)
+
+	svc := NewICBMService(messageRelayer, nil, nil, nil)
+
+	err := svc.ClientErr(nil, sess, wire.SNACFrame{RequestID: 1234}, inBody)
+	assert.NoError(t, err)
+}

foodgroup/oservice.go

@@ -542,7 +542,6 @@ func NewOServiceServiceForBOS(
 				wire.Alert,
 				wire.BART,
 				wire.Buddy,
-				wire.ChatNav,
 				wire.Feedbag,
 				wire.ICBM,
 				wire.ICQ,
@@ -807,9 +806,14 @@ func (s OServiceServiceForChat) ClientOnline(ctx context.Context, _ wire.SNAC_0x
 	if err != nil {
 		return fmt.Errorf("error getting chat room: %w", err)
 	}
+
+	// Do not change the order of the following 3 methods. macOS client v4.0.9
+	// requires this exact sequence, otherwise the chat session prematurely
+	// closes seconds after users join a chat room.
+	setOnlineChatUsers(ctx, sess, s.chatMessageRelayer)
 	sendChatRoomInfoUpdate(ctx, sess, s.chatMessageRelayer, room)
 	alertUserJoined(ctx, sess, s.chatMessageRelayer)
-	setOnlineChatUsers(ctx, sess, s.chatMessageRelayer)
+
 	return nil
 }
 

foodgroup/oservice_test.go

@@ -1468,7 +1468,6 @@ func TestOServiceServiceForBOS_OServiceHostOnline(t *testing.T) {
 				wire.Alert,
 				wire.BART,
 				wire.Buddy,
-				wire.ChatNav,
 				wire.Feedbag,
 				wire.ICBM,
 				wire.ICQ,

server/http/mgmt_api_test.go

@@ -552,7 +552,7 @@ func TestPublicChatHandler_GET(t *testing.T) {
 					},
 				},
 			},
-			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","url":"aim:gochat?exchange=5\u0026roomname=chat-room-1-name","participants":[{"id":"usera","screen_name":"userA"},{"id":"userb","screen_name":"userB"}]},{"name":"chat-room-2-name","create_time":"0001-01-01T00:00:00Z","url":"aim:gochat?exchange=5\u0026roomname=chat-room-2-name","participants":[{"id":"userc","screen_name":"userC"},{"id":"userd","screen_name":"userD"}]}]`,
+			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","url":"aim:gochat?roomname=chat-room-1-name\u0026exchange=5","participants":[{"id":"usera","screen_name":"userA"},{"id":"userb","screen_name":"userB"}]},{"name":"chat-room-2-name","create_time":"0001-01-01T00:00:00Z","url":"aim:gochat?roomname=chat-room-2-name\u0026exchange=5","participants":[{"id":"userc","screen_name":"userC"},{"id":"userd","screen_name":"userD"}]}]`,
 			statusCode: http.StatusOK,
 		},
 		{
@@ -569,7 +569,7 @@ func TestPublicChatHandler_GET(t *testing.T) {
 					result: []*state.Session{},
 				},
 			},
-			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","url":"aim:gochat?exchange=5\u0026roomname=chat-room-1-name","participants":[]}]`,
+			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","url":"aim:gochat?roomname=chat-room-1-name\u0026exchange=5","participants":[]}]`,
 			statusCode: http.StatusOK,
 		},
 		{
@@ -667,7 +667,7 @@ func TestPrivateChatHandler_GET(t *testing.T) {
 					},
 				},
 			},
-			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","creator_id":"chat-room-1-creator","url":"aim:gochat?exchange=4\u0026roomname=chat-room-1-name","participants":[{"id":"usera","screen_name":"userA"},{"id":"userb","screen_name":"userB"}]},{"name":"chat-room-2-name","create_time":"0001-01-01T00:00:00Z","creator_id":"chat-room-2-creator","url":"aim:gochat?exchange=4\u0026roomname=chat-room-2-name","participants":[{"id":"userc","screen_name":"userC"},{"id":"userd","screen_name":"userD"}]}]`,
+			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","creator_id":"chat-room-1-creator","url":"aim:gochat?roomname=chat-room-1-name\u0026exchange=4","participants":[{"id":"usera","screen_name":"userA"},{"id":"userb","screen_name":"userB"}]},{"name":"chat-room-2-name","create_time":"0001-01-01T00:00:00Z","creator_id":"chat-room-2-creator","url":"aim:gochat?roomname=chat-room-2-name\u0026exchange=4","participants":[{"id":"userc","screen_name":"userC"},{"id":"userd","screen_name":"userD"}]}]`,
 			statusCode: http.StatusOK,
 		},
 		{
@@ -684,7 +684,7 @@ func TestPrivateChatHandler_GET(t *testing.T) {
 					result: []*state.Session{},
 				},
 			},
-			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","creator_id":"chat-room-1-creator","url":"aim:gochat?exchange=4\u0026roomname=chat-room-1-name","participants":[]}]`,
+			want:       `[{"name":"chat-room-1-name","create_time":"0001-01-01T00:00:00Z","creator_id":"chat-room-1-creator","url":"aim:gochat?roomname=chat-room-1-name\u0026exchange=4","participants":[]}]`,
 			statusCode: http.StatusOK,
 		},
 		{

server/oscar/handler/icbm.go

@@ -17,6 +17,7 @@ type ICBMService interface {
 	ClientEvent(ctx context.Context, sess *state.Session, inFrame wire.SNACFrame, inBody wire.SNAC_0x04_0x14_ICBMClientEvent) error
 	EvilRequest(ctx context.Context, sess *state.Session, inFrame wire.SNACFrame, inBody wire.SNAC_0x04_0x08_ICBMEvilRequest) (wire.SNACMessage, error)
 	ParameterQuery(ctx context.Context, inFrame wire.SNACFrame) wire.SNACMessage
+	ClientErr(ctx context.Context, sess *state.Session, frame wire.SNACFrame, body wire.SNAC_0x04_0x0B_ICBMClientErr) error
 }
 
 func NewICBMHandler(logger *slog.Logger, icbmService ICBMService) ICBMHandler {
@@ -75,10 +76,14 @@ func (h ICBMHandler) EvilRequest(ctx context.Context, sess *state.Session, inFra
 	return rw.SendSNAC(outSNAC.Frame, outSNAC.Body)
 }
 
-func (h ICBMHandler) ClientErr(ctx context.Context, _ *state.Session, inFrame wire.SNACFrame, r io.Reader, _ oscar.ResponseWriter) error {
+func (h ICBMHandler) ClientErr(ctx context.Context, sess *state.Session, inFrame wire.SNACFrame, r io.Reader, _ oscar.ResponseWriter) error {
 	inBody := wire.SNAC_0x04_0x0B_ICBMClientErr{}
 	h.LogRequest(ctx, inFrame, inBody)
-	return wire.UnmarshalBE(&inBody, r)
+	err := wire.UnmarshalBE(&inBody, r)
+	if err != nil {
+		return err
+	}
+	return h.ICBMService.ClientErr(ctx, sess, inFrame, inBody)
 }
 
 func (h ICBMHandler) ClientEvent(ctx context.Context, sess *state.Session, inFrame wire.SNACFrame, r io.Reader, _ oscar.ResponseWriter) error {

server/oscar/handler/icbm_test.go

@@ -82,7 +82,12 @@ func TestICBMHandler_ClientErr(t *testing.T) {
 	}
 
 	svc := newMockICBMService(t)
+	svc.EXPECT().
+		ClientErr(mock.Anything, mock.Anything, input.Frame, input.Body).
+		Return(nil)
+
 	h := NewICBMHandler(slog.Default(), svc)
+
 	responseWriter := newMockResponseWriter(t)
 
 	buf := &bytes.Buffer{}

server/oscar/handler/mock_icbm_test.go

@@ -85,6 +85,55 @@ func (_c *mockICBMService_ChannelMsgToHost_Call) RunAndReturn(run func(context.C
 	return _c
 }
 
+// ClientErr provides a mock function with given fields: ctx, sess, frame, body
+func (_m *mockICBMService) ClientErr(ctx context.Context, sess *state.Session, frame wire.SNACFrame, body wire.SNAC_0x04_0x0B_ICBMClientErr) error {
+	ret := _m.Called(ctx, sess, frame, body)
+
+	if len(ret) == 0 {
+		panic("no return value specified for ClientErr")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, *state.Session, wire.SNACFrame, wire.SNAC_0x04_0x0B_ICBMClientErr) error); ok {
+		r0 = rf(ctx, sess, frame, body)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// mockICBMService_ClientErr_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ClientErr'
+type mockICBMService_ClientErr_Call struct {
+	*mock.Call
+}
+
+// ClientErr is a helper method to define mock.On call
+//   - ctx context.Context
+//   - sess *state.Session
+//   - frame wire.SNACFrame
+//   - body wire.SNAC_0x04_0x0B_ICBMClientErr
+func (_e *mockICBMService_Expecter) ClientErr(ctx interface{}, sess interface{}, frame interface{}, body interface{}) *mockICBMService_ClientErr_Call {
+	return &mockICBMService_ClientErr_Call{Call: _e.mock.On("ClientErr", ctx, sess, frame, body)}
+}
+
+func (_c *mockICBMService_ClientErr_Call) Run(run func(ctx context.Context, sess *state.Session, frame wire.SNACFrame, body wire.SNAC_0x04_0x0B_ICBMClientErr)) *mockICBMService_ClientErr_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(*state.Session), args[2].(wire.SNACFrame), args[3].(wire.SNAC_0x04_0x0B_ICBMClientErr))
+	})
+	return _c
+}
+
+func (_c *mockICBMService_ClientErr_Call) Return(_a0 error) *mockICBMService_ClientErr_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *mockICBMService_ClientErr_Call) RunAndReturn(run func(context.Context, *state.Session, wire.SNACFrame, wire.SNAC_0x04_0x0B_ICBMClientErr) error) *mockICBMService_ClientErr_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
 // ClientEvent provides a mock function with given fields: ctx, sess, inFrame, inBody
 func (_m *mockICBMService) ClientEvent(ctx context.Context, sess *state.Session, inFrame wire.SNACFrame, inBody wire.SNAC_0x04_0x14_ICBMClientEvent) error {
 	ret := _m.Called(ctx, sess, inFrame, inBody)

server/oscar/handler/routes.go

@@ -103,6 +103,7 @@ func NewChatRouter(h Handlers) oscar.Router {
 	router.Register(wire.OService, wire.OServiceRateParamsSubAdd, h.OServiceHandler.RateParamsSubAdd)
 	router.Register(wire.OService, wire.OServiceSetUserInfoFields, h.OServiceHandler.SetUserInfoFields)
 	router.Register(wire.OService, wire.OServiceUserInfoQuery, h.OServiceHandler.UserInfoQuery)
+	router.Register(wire.OService, wire.OServiceSetPrivacyFlags, h.OServiceHandler.SetPrivacyFlags)
 
 	return router
 }
@@ -124,6 +125,7 @@ func NewChatNavRouter(h Handlers) oscar.Router {
 	router.Register(wire.OService, wire.OServiceRateParamsSubAdd, h.OServiceHandler.RateParamsSubAdd)
 	router.Register(wire.OService, wire.OServiceSetUserInfoFields, h.OServiceHandler.SetUserInfoFields)
 	router.Register(wire.OService, wire.OServiceUserInfoQuery, h.OServiceHandler.UserInfoQuery)
+	router.Register(wire.OService, wire.OServiceSetPrivacyFlags, h.OServiceHandler.SetPrivacyFlags)
 
 	return router
 }

state/chat.go

@@ -84,13 +84,13 @@ func (c ChatRoom) Cookie() string {
 
 // URL creates a URL that can be used to join a chat room.
 func (c ChatRoom) URL() *url.URL {
-	v := url.Values{}
-	v.Set("roomname", c.name)
-	v.Set("exchange", fmt.Sprintf("%d", c.exchange))
-
+	// macOS client v4.0.9 requires the `roomname` param to precede `exchange`
+	// param. Create the path using string concatenation rather than url.Values
+	// because url.Values sorts the params alphabetically.
+	opaque := fmt.Sprintf("gochat?roomname=%s&exchange=%d", url.QueryEscape(c.name), c.exchange)
 	return &url.URL{
 		Scheme: "aim",
-		Opaque: "gochat?" + v.Encode(),
+		Opaque: opaque,
 	}
 }