refactor: Harmonise Master’s auth signature with authenticateCredentials

[delucis]

Following on from #532, this adds a test for the wrapper used around users’ authenticateCredentials methods.

[delucis]

There’s still one line missing coverage from when the wrapper is actually called during server initialisation.

I started writing a test as follows:

test('custom credential authentication', () => {
  const game = {};
  const authenticateCredentials = jest.fn();
  const server = Server({ games: [game], authenticateCredentials });
});

But I’m not really sure what is testable here.

In fact, I’m wondering whether instead of wrapping at this point, the internal API inside Master should be refactored to match this one and make the wrapper function unnecessary. (The authenticateCredentials method is currently wrapped and passed as the SocketIO transport’s auth argument, which in turn passes it as the Master’s auth argument.)

Are there any places that currently pass a custom auth function? For example, I notice that Master passes auth a gameID option, even though that is unused by the default method. Is something using that? If not, refactoring from auth({action, gameMetadata, gameID, playerID}) to auth({credentials, playerMetadata}) might simplify things, although it would be a breaking change for anyone using Master directly and passing a custom auth function.

[nicolodavis]

I'm in favor of refactoring the signature of the auth function if it would simplify things. Feel free to do it in this PR. I've removed the redundant gameID parameter (it is not used).

[delucis]

OK, this should be ready for review. The summary:

• Users can still provide an authenticateCredentials function, which will be passed credentials (from the action payload) and playerMetadata (from the game metadata).

• Instead of wrapping that function in the server implementation, I have refactored Master to call its own default authentication check with the same signature.

• Previously, the default isActionFromAuthenticPlayer check did some work to establish whether the game required authenticating at all (i.e. is there metadata and does it contain credentials fields). To completely preserve that behaviour, I have split out that logic into a doesGameRequireAuthentication check, which decides whether or not to call auth. This is bypassed if a user passes their own auth method.

  This does mean that when a user passes their own auth method they can’t know if playerMetadata is empty because there is no game metadata or if it’s because the client sent a bad playerID (if that’s possible?), but I think that’s fine.

[nicolodavis]

Great work @delucis!

[nicolodavis]

Not super critical to test, but in the interest of keeping the test coverage at 100%, can we add a test to cover both branches here? If you run npm run test:coverage on the repo, you will notice that it complains about uncovered branches at line 115 here and line 66 in index.ts. I'm happy to also follow up with these tests if you don't have time to add them here.

[delucis]

I’m just realising that the executeSynchronously branches aren’t being tested at all in the Master tests, because they’re only used by LocalMaster in the local transport implementation. Because there’s no game metadata in the LocalMaster tests, the shouldAuth branch always returns false. I don’t think I’m familiar enough with the local transport to fix this.

Seeing this though, I’ve tweaked the way shouldAuth is initialised, because previously I had it calling doesGameRequireAuthentication even if authentication hadn’t been enabled.

[delucis]

Actually, it’s not just that there’s no metadata, LocalMaster creates an instance of Master that has auth disabled. So there’s never currently a Master that executes synchronously and uses authentication.

[nicolodavis]

OK, let's get this in first and I'll try to fix the testing coverage later.