Repository: boardgameio/boardgame.io (ts)

Issue:
Authenticate chat messages
I didn’t have time to look at #871 before it was merged, but I see now it didn’t include authentication like we now have for all the other game master methods (partly because #853 was only merged after @TomasVidhall started work).

Should we use player credentials for authentication? This would only allow players rather than spectators to chat, but currently an unauthenticated client could post a message claiming to be an authenticated player.

Pull request:
Add authentication to chat messages
Closes #876 

Follow-up to #871 which implements the new auth logic implemented in #853 for chat messages. I think this code will make it possible for spectators to send chat messages (with no sender id though), but it will be impossible to impersonate another player in the chat.

Please feedback and improve @delucis @nicolodavis 

#### Checklist

* [x] Use a separate branch in your local repo (not `master`).
* [x] Test coverage is 100% (or you have a story for why it's ok).


Patch:
diff --git a/src/client/transport/local.ts b/src/client/transport/local.ts
index cb3efd2..40412c7 100644
--- a/src/client/transport/local.ts
+++ b/src/client/transport/local.ts
@@ -14,6 +14,7 @@ import type { TransportAPI, TransportData } from '../../master/master';
 import { Transport } from './transport';
 import type { TransportOpts, ChatCallback } from './transport';
 import type {
+  ChatMessage,
   CredentialedActionShape,
   Game,
   LogEntry,
@@ -161,8 +162,18 @@ export class LocalTransport extends Transport {
     this.isConnected = true;
   }
 
-  onChatMessage(matchID, chatMessage) {
-    this.master.onChatMessage(matchID, chatMessage);
+  /**
+   * Called when any player sends a chat message and the
+   * master broadcasts the update to other clients (including
+   * this one).
+   */
+  onChatMessage(matchID: string, chatMessage: ChatMessage) {
+    const args: Parameters<Master['onChatMessage']> = [
+      matchID,
+      chatMessage,
+      this.credentials,
+    ];
+    this.master.onChatMessage(...args);
   }
 
   /**
diff --git a/src/client/transport/socketio.ts b/src/client/transport/socketio.ts
index 2e66a1c..7862300 100644
--- a/src/client/transport/socketio.ts
+++ b/src/client/transport/socketio.ts
@@ -96,8 +96,13 @@ export class SocketIOTransport extends Transport {
     this.socket.emit('update', ...args);
   }
 
-  onChatMessage(matchID, chatMessage) {
-    this.socket.emit('chat', matchID, chatMessage);
+  onChatMessage(matchID: string, chatMessage: ChatMessage) {
+    const args: Parameters<Master['onChatMessage']> = [
+      matchID,
+      chatMessage,
+      this.credentials,
+    ];
+    this.socket.emit('chat', ...args);
   }
 
   /**
diff --git a/src/master/master.ts b/src/master/master.ts
index d7c00b5..d17d482 100644
--- a/src/master/master.ts
+++ b/src/master/master.ts
@@ -459,7 +459,30 @@ export class Master {
     }
   }
 
-  async onChatMessage(matchID, chatMessage) {
+  async onChatMessage(
+    matchID: string,
+    chatMessage: ChatMessage,
+    credentials: string | undefined
+  ): Promise<void | { error: string }> {
+    const key = matchID;
+
+    if (this.auth) {
+      const { metadata } = await (this.storageAPI as StorageAPI.Async).fetch(
+        key,
+        {
+          metadata: true,
+        }
+      );
+      const isAuthentic = await this.auth.authenticateCredentials({
+        playerID: chatMessage.sender,
+        credentials,
+        metadata,
+      });
+      if (!isAuthentic) {
+        return { error: 'unauthorized' };
+      }
+    }
+
     this.transportAPI.sendAll(() => ({
       type: 'chat',
       args: [matchID, chatMessage],
diff --git a/src/server/transport/socketio.ts b/src/server/transport/socketio.ts
index 5eff1ac..03e648c 100644
--- a/src/server/transport/socketio.ts
+++ b/src/server/transport/socketio.ts
@@ -185,15 +185,19 @@ export class SocketIO {
             );
           }
         });
-        socket.on('chat', async (matchID, chatMessage) => {
-          const master = new Master(
-            game,
-            app.context.db,
-            TransportAPI(matchID, socket, this.clientInfo, this.roomInfo),
-            app.context.auth
-          );
-          master.onChatMessage(matchID, chatMessage);
-        });
+        socket.on(
+          'chat',
+          async (...args: Parameters<Master['onChatMessage']>) => {
+            const [matchID] = args;
+            const master = new Master(
+              game,
+              app.context.db,
+              TransportAPI(matchID, socket, this.clientInfo, this.roomInfo),
+              app.context.auth
+            );
+            master.onChatMessage(...args);
+          }
+        );
       });
     }
   }


Test patch:
diff --git a/src/client/transport/local.test.ts b/src/client/transport/local.test.ts
index e59f216..436e862 100644
--- a/src/client/transport/local.test.ts
+++ b/src/client/transport/local.test.ts
@@ -15,7 +15,7 @@ import { InitializeGame } from '../../core/initialize';
 import { Client } from '../client';
 import { RandomBot } from '../../ai/random-bot';
 import { Stage } from '../../core/turn-order';
-import type { State, Store, SyncInfo } from '../../types';
+import type { ChatMessage, State, Store, SyncInfo } from '../../types';
 
 jest.useFakeTimers();
 
@@ -361,10 +361,13 @@ describe('LocalTransport', () => {
     });
 
     test('send chat-message', () => {
-      m.onChatMessage('matchID', { message: 'foo' });
-      expect(m.master.onChatMessage).lastCalledWith('matchID', {
-        message: 'foo',
-      });
+      const msg: ChatMessage = {
+        id: '0',
+        sender: '0',
+        payload: { message: 'foo' },
+      };
+      m.onChatMessage('matchID', msg);
+      expect(m.master.onChatMessage).lastCalledWith('matchID', msg, undefined);
     });
   });
 });
diff --git a/src/client/transport/socketio.test.ts b/src/client/transport/socketio.test.ts
index 12d058c..2ff2484 100644
--- a/src/client/transport/socketio.test.ts
+++ b/src/client/transport/socketio.test.ts
@@ -13,7 +13,7 @@ import { CreateGameReducer } from '../../core/reducer';
 import { InitializeGame } from '../../core/initialize';
 import * as Actions from '../../core/action-types';
 import type { Master } from '../../master/master';
-import type { State, Store } from '../../types';
+import type { ChatMessage, State, Store } from '../../types';
 
 type UpdateArgs = Parameters<Master['onUpdate']>;
 type SyncArgs = Parameters<Master['onSync']>;
@@ -204,10 +204,18 @@ describe('multiplayer', () => {
   });
 
   test('send chat-message', () => {
-    m.onChatMessage('matchID', { message: 'foo' });
-    expect(mockSocket.emit).lastCalledWith('chat', 'matchID', {
-      message: 'foo',
-    });
+    const message: ChatMessage = {
+      id: '0',
+      sender: '0',
+      payload: { message: 'foo' },
+    };
+    m.onChatMessage('matchID', message);
+    expect(mockSocket.emit).lastCalledWith(
+      'chat',
+      'matchID',
+      message,
+      m.getCredentials()
+    );
   });
 });
 
diff --git a/src/master/master.test.ts b/src/master/master.test.ts
index 8cddcfa..f8069c2 100644
--- a/src/master/master.test.ts
+++ b/src/master/master.test.ts
@@ -808,6 +808,54 @@ describe('authentication', () => {
       expect(sendAll).not.toHaveBeenCalled();
     });
   });
+
+  describe('onChatMessage', () => {
+    const chatMessage = {
+      id: 'uuid',
+      payload: { message: 'foo' },
+      sender: '0',
+    };
+
+    beforeEach(resetTestEnvironment);
+
+    test('auth failure', async () => {
+      const authenticateCredentials = () => false;
+      const master = new Master(
+        game,
+        storage,
+        TransportAPI(send, sendAll),
+        new Auth({ authenticateCredentials })
+      );
+      const ret = await master.onChatMessage(matchID, chatMessage, undefined);
+      expect(ret && ret.error).toBe('unauthorized');
+      expect(sendAll).not.toHaveBeenCalled();
+    });
+
+    test('auth success', async () => {
+      const authenticateCredentials = () => true;
+      const master = new Master(
+        game,
+        storage,
+        TransportAPI(send, sendAll),
+        new Auth({ authenticateCredentials })
+      );
+      const ret = await master.onChatMessage(matchID, chatMessage, undefined);
+      expect(ret).toBeUndefined();
+      expect(sendAll).toHaveBeenCalled();
+    });
+
+    test('default', async () => {
+      const master = new Master(
+        game,
+        storage,
+        TransportAPI(send, sendAll),
+        new Auth()
+      );
+      const ret = await master.onChatMessage(matchID, chatMessage, undefined);
+      expect(ret).toBeUndefined();
+      expect(sendAll).toHaveBeenCalled();
+    });
+  });
 });
 
 describe('redactLog', () => {
@@ -982,10 +1030,17 @@ describe('chat', () => {
   });
 
   test('Sends chat messages to all', async () => {
-    master.onChatMessage('matchID', { message: 'foo' });
+    master.onChatMessage(
+      'matchID',
+      { id: 'uuid', sender: '0', payload: { message: 'foo' } },
+      undefined
+    );
     expect(sendAllReturn('0')).toEqual({
       type: 'chat',
-      args: ['matchID', { message: 'foo' }],
+      args: [
+        'matchID',
+        { id: 'uuid', sender: '0', payload: { message: 'foo' } },
+      ],
     });
   });
 });