refactor: Harmonise Master’s auth signature with authenticateCredentials (#539)

* test(server): Add coverage for the `authenticateCredentials` wrapper

* feat(server): Don’t wrap user-provided authentication method

* refactor(master): Call auth method with (credentials, playerMetadata)

User-provided and default `auth` functions are now called with 
(credentials, playerMetadata). Checks in the default `auth` 
implementation that established whether or not we should authenticate 
have been split into a separate `doesGameRequireAuthentication` method, 
that is always used regardless of user-defined `auth` functions.

* refactor(master): Account for undefined metadata in default auth method

If a user were to dispatch an action with a non-existent playerID, we 
might plausibly get an undefined `playerMetadata` argument.

* feat(master): Skip doesGameRequireAuthentication check for custom auth

* fix(master): Safely retrieve player metadata

Account for undefined parameters when getting player metadata to pass to 
auth method

* test(master): Remove metadata from authentication test

* test(master): Improve coverage

* refactor(master): Don’t explicitly return undefined in getPlayerMetadata

* refactor(master): Rename `credentials` parameter to `actionCredentials`

* refactor(master): Tweak how the auth & shouldAuth fields are initialised

If the `auth` parameter is not passed as `true` or a custom function, 
there is no need for the `shouldAuth` function to do anything except 
always return `false`.

Co-authored-by: Nicolo John Davis <[email protected]>

Author: delucis

src/master/master.js

@@ -15,6 +15,12 @@ import * as logging from '../core/logger';
 
 const GameMetadataKey = gameID => `${gameID}:metadata`;
 
+export const getPlayerMetadata = (gameMetadata, playerID) => {
+  if (gameMetadata && gameMetadata.players) {
+    return gameMetadata.players[playerID];
+  }
+};
+
 /**
  * Redact the log.
  *
@@ -53,42 +59,27 @@ export function redactLog(log, playerID) {
 }
 
 /**
- * Verifies that the move came from a player with the
- * appropriate credentials.
+ * Verifies that the game has metadata and is using credentials.
  */
-export const isActionFromAuthenticPlayer = ({
-  action,
-  gameMetadata,
-  playerID,
-}) => {
-  if (!gameMetadata) {
-    return true;
-  }
-
-  const hasCredentials = Object.keys(gameMetadata.players).some(key => {
-    return !!(
-      gameMetadata.players[key] && gameMetadata.players[key].credentials
-    );
+export const doesGameRequireAuthentication = gameMetadata => {
+  if (!gameMetadata) return false;
+  const { players } = gameMetadata;
+  const hasCredentials = Object.keys(players).some(key => {
+    return !!(players[key] && players[key].credentials);
   });
-  if (!hasCredentials) {
-    return true;
-  }
-
-  if (!action.payload) {
-    return false;
-  }
-
-  if (!action.payload.credentials) {
-    return false;
-  }
-
-  if (
-    action.payload.credentials !== gameMetadata.players[playerID].credentials
-  ) {
-    return false;
-  }
+  return hasCredentials;
+};
 
-  return true;
+/**
+ * Verifies that the move came from a player with the correct credentials.
+ */
+export const isActionFromAuthenticPlayer = (
+  actionCredentials,
+  playerMetadata
+) => {
+  if (!actionCredentials) return false;
+  if (!playerMetadata) return false;
+  return actionCredentials === playerMetadata.credentials;
 };
 
 /**
@@ -104,11 +95,14 @@ export class Master {
     this.storageAPI = storageAPI;
     this.transportAPI = transportAPI;
     this.auth = () => true;
+    this.shouldAuth = () => false;
 
     if (auth === true) {
       this.auth = isActionFromAuthenticPlayer;
+      this.shouldAuth = doesGameRequireAuthentication;
     } else if (typeof auth === 'function') {
       this.auth = auth;
+      this.shouldAuth = () => true;
     }
   }
 
@@ -119,21 +113,19 @@ export class Master {
    */
   async onUpdate(action, stateID, gameID, playerID) {
     let isActionAuthentic;
-
+    const { credentials } = action.payload || {};
     if (this.executeSynchronously) {
       const gameMetadata = this.storageAPI.get(GameMetadataKey(gameID));
-      isActionAuthentic = this.auth({
-        action,
-        gameMetadata,
-        playerID,
-      });
+      const playerMetadata = getPlayerMetadata(gameMetadata, playerID);
+      isActionAuthentic = this.shouldAuth(gameMetadata)
+        ? this.auth(credentials, playerMetadata)
+        : true;
     } else {
       const gameMetadata = await this.storageAPI.get(GameMetadataKey(gameID));
-      isActionAuthentic = await this.auth({
-        action,
-        gameMetadata,
-        playerID,
-      });
+      const playerMetadata = getPlayerMetadata(gameMetadata, playerID);
+      isActionAuthentic = this.shouldAuth(gameMetadata)
+        ? await this.auth(credentials, playerMetadata)
+        : true;
     }
     if (!isActionAuthentic) {
       return { error: 'unauthorized action' };

src/master/master.test.js

@@ -8,7 +8,13 @@
 
 import * as ActionCreators from '../core/action-creators';
 import { InMemory } from '../server/db/inmemory';
-import { Master, redactLog, isActionFromAuthenticPlayer } from './master';
+import {
+  Master,
+  redactLog,
+  getPlayerMetadata,
+  doesGameRequireAuthentication,
+  isActionFromAuthenticPlayer,
+} from './master';
 import { error } from '../core/logger';
 
 jest.mock('../core/logger', () => ({
@@ -258,12 +264,13 @@ describe('authentication', () => {
   const send = jest.fn();
   const sendAll = jest.fn();
   const game = { seed: 0 };
+  const gameID = 'gameID';
   const action = ActionCreators.gameEvent('endTurn');
   const storage = new InMemory();
 
   beforeAll(async () => {
     const master = new Master(game, storage, TransportAPI());
-    await master.onSync('gameID', '0');
+    await master.onSync(gameID, '0');
   });
 
   test('auth failure', async () => {
@@ -274,7 +281,7 @@ describe('authentication', () => {
       TransportAPI(send, sendAll),
       isActionFromAuthenticPlayer
     );
-    await master.onUpdate(action, 0, 'gameID', '0');
+    await master.onUpdate(action, 0, gameID, '0');
     expect(sendAll).not.toHaveBeenCalled();
   });
 
@@ -286,13 +293,13 @@ describe('authentication', () => {
       TransportAPI(send, sendAll),
       isActionFromAuthenticPlayer
     );
-    await master.onUpdate(action, 0, 'gameID', '0');
+    await master.onUpdate(action, 0, gameID, '0');
     expect(sendAll).toHaveBeenCalled();
   });
 
   test('default', async () => {
     const master = new Master(game, storage, TransportAPI(send, sendAll), true);
-    await master.onUpdate(action, 0, 'gameID', '0');
+    await master.onUpdate(action, 0, gameID, '0');
     expect(sendAll).toHaveBeenCalled();
   });
 });
@@ -420,130 +427,147 @@ describe('redactLog', () => {
   });
 });
 
-describe('isActionFromAuthenticPlayer', () => {
-  let action;
-  let playerID;
-  let gameMetadata;
-
-  beforeEach(() => {
-    playerID = '0';
-
-    action = {
-      payload: {
-        credentials: 'SECRET',
-      },
-    };
+describe('getPlayerMetadata', () => {
+  describe('when metadata is not found', () => {
+    test('then playerMetadata is undefined', () => {
+      expect(getPlayerMetadata(undefined, '0')).toBeUndefined();
+    });
+  });
 
-    gameMetadata = {
-      players: {
-        '0': {
-          credentials: 'SECRET',
-        },
-      },
-    };
+  describe('when metadata does not contain players field', () => {
+    test('then playerMetadata is undefined', () => {
+      expect(getPlayerMetadata({}, '0')).toBeUndefined();
+    });
   });
 
-  describe('when game metadata is not found', () => {
-    beforeEach(() => {
-      gameMetadata = null;
+  describe('when metadata does not contain playerID', () => {
+    test('then playerMetadata is undefined', () => {
+      expect(getPlayerMetadata({ players: { '1': {} } }, '0')).toBeUndefined();
     });
+  });
 
-    test('the action is authentic', () => {
-      const result = isActionFromAuthenticPlayer({
-        action,
-        gameMetadata,
-        playerID,
-      });
+  describe('when metadata contains playerID', () => {
+    test('then playerMetadata is returned', () => {
+      const playerMetadata = { credentials: 'SECRET' };
+      const result = getPlayerMetadata(
+        { players: { '0': playerMetadata } },
+        '0'
+      );
+      expect(result).toBe(playerMetadata);
+    });
+  });
+});
 
-      expect(result).toBeTruthy();
+describe('doesGameRequireAuthentication', () => {
+  describe('when game metadata is not found', () => {
+    test('then authentication is not required', () => {
+      const result = doesGameRequireAuthentication();
+      expect(result).toBe(false);
     });
   });
 
   describe('when game has no credentials', () => {
-    beforeEach(() => {
-      gameMetadata = {
+    test('then authentication is not required', () => {
+      const gameMetadata = {
         players: {
           '0': {},
         },
       };
+      const result = doesGameRequireAuthentication(gameMetadata);
+      expect(result).toBe(false);
     });
+  });
 
-    test('then action is authentic', async () => {
-      const result = isActionFromAuthenticPlayer({
-        action,
-        gameMetadata,
-        playerID,
-      });
-
-      expect(result).toBeTruthy();
+  describe('when game has credentials', () => {
+    test('then authentication is required', () => {
+      const gameMetadata = {
+        players: {
+          '0': {
+            credentials: 'SECRET',
+          },
+        },
+      };
+      const result = doesGameRequireAuthentication(gameMetadata);
+      expect(result).toBe(true);
     });
   });
+});
+
+describe('isActionFromAuthenticPlayer', () => {
+  let action;
+  let playerID;
+  let gameMetadata;
+  let credentials;
+  let playerMetadata;
+
+  beforeEach(() => {
+    playerID = '0';
+
+    action = {
+      payload: { credentials: 'SECRET' },
+    };
+
+    gameMetadata = {
+      players: {
+        '0': { credentials: 'SECRET' },
+      },
+    };
+
+    playerMetadata = gameMetadata.players[playerID];
+    ({ credentials } = action.payload || {});
+  });
 
   describe('when game has credentials', () => {
     describe('when action contains no payload', () => {
       beforeEach(() => {
         action = {};
+        ({ credentials } = action.payload || {});
       });
 
       test('the action is not authentic', async () => {
-        const result = isActionFromAuthenticPlayer({
-          action,
-          gameMetadata,
-          playerID,
-        });
-
-        expect(result).toBeFalsy();
+        const result = isActionFromAuthenticPlayer(credentials, playerMetadata);
+        expect(result).toBe(false);
       });
     });
 
     describe('when action contains no credentials', () => {
       beforeEach(() => {
         action = {
-          payload: {
-            someStuff: 'foo',
-          },
+          payload: { someStuff: 'foo' },
         };
+        ({ credentials } = action.payload || {});
       });
 
       test('then action is not authentic', async () => {
-        const result = isActionFromAuthenticPlayer({
-          action,
-          gameMetadata,
-          playerID,
-        });
-
-        expect(result).toBeFalsy();
+        const result = isActionFromAuthenticPlayer(credentials, playerMetadata);
+        expect(result).toBe(false);
       });
     });
 
     describe('when action credentials do not match game credentials', () => {
       beforeEach(() => {
         action = {
-          payload: {
-            credentials: 'WRONG',
-          },
+          payload: { credentials: 'WRONG' },
         };
+        ({ credentials } = action.payload || {});
       });
       test('then action is not authentic', async () => {
-        const result = isActionFromAuthenticPlayer({
-          action,
-          gameMetadata,
-          playerID,
-        });
+        const result = isActionFromAuthenticPlayer(credentials, playerMetadata);
+        expect(result).toBe(false);
+      });
+    });
 
-        expect(result).toBeFalsy();
+    describe('when playerMetadata is not found', () => {
+      test('then action is not authentic', () => {
+        const result = isActionFromAuthenticPlayer(credentials);
+        expect(result).toBe(false);
       });
     });
 
     describe('when action credentials do match game credentials', () => {
       test('then action is authentic', async () => {
-        const result = isActionFromAuthenticPlayer({
-          action,
-          gameMetadata,
-          playerID,
-        });
-
-        expect(result).toBeTruthy();
+        const result = isActionFromAuthenticPlayer(credentials, playerMetadata);
+        expect(result).toBe(true);
       });
     });
   });