Add authentication to chat messages #879
Conversation
There was a problem hiding this comment.
Thanks @tomasvidhall and Happy New Year! This looks good in general. I have one question about leaving the chat open to spectators — let me know what you think.
The other comments recommend some more precise typing that will help with future refactoring. Updating these might require some changes in the tests to fix some TypeScript errors — if anything isn’t clear, I’ll be happy to help.
src/master/master.ts
Outdated
| } else { | ||
| ({ metadata } = await this.storageAPI.fetch(key, { metadata: true })); | ||
| } | ||
| if (this.auth && chatMessage.sender !== undefined) { |
There was a problem hiding this comment.
I’m not 100% sure about allowing spectators to post messages without authentication, although this is an improvement over allowing them to claim to be a player. Is this a functionality you need? In general I would lean towards only permitting players to post, but am open to being more flexible if you definitely need it (or perhaps adding a server option to enable spectator chat).
Metadata is only used for authentication, which always runs asynchronously in any case. If the storage API actually is synchronous, `await` will have no effect.
|
Thanks for getting this rolling @tomasvidhall. I wanted to get this released, so I’ve merged this for now without spectator chat. Please feel free to open a PR adding that back in later if you need it! |
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
master).