diff --git a/hasura/metadata/databases/default/tables/public_players.yaml b/hasura/metadata/databases/default/tables/public_players.yaml index bf7416ee..c8c6bf0b 100644 --- a/hasura/metadata/databases/default/tables/public_players.yaml +++ b/hasura/metadata/databases/default/tables/public_players.yaml @@ -633,7 +633,7 @@ update_permissions: _eq: X-Hasura-User-Id comment: "" delete_permissions: - - role: match_organizer + - role: administrator permission: filter: {} comment: "" diff --git a/src/hasura/metadata-permissions.spec.ts b/src/hasura/metadata-permissions.spec.ts index 1074fd29..dd38315f 100644 --- a/src/hasura/metadata-permissions.spec.ts +++ b/src/hasura/metadata-permissions.spec.ts @@ -137,6 +137,18 @@ describe("hasura table metadata", () => { expect(problems).toEqual([]); }); + // A player row is the account itself: every match, stat and sanction hangs + // off it, so removing one is not a match organizer's call. + it("only lets an administrator delete a player", () => { + const players = tables.find(({ file }) => file === "public_players.yaml"); + + expect( + (players?.metadata?.delete_permissions ?? []).map( + (entry: { role: string }) => entry.role, + ), + ).toEqual(["administrator"]); + }); + it("never lists the same name as both a column and a computed field", () => { const problems: Array = []; diff --git a/src/matches/matches.controller.spec.ts b/src/matches/matches.controller.spec.ts index c4fe6028..4ae94785 100644 --- a/src/matches/matches.controller.spec.ts +++ b/src/matches/matches.controller.spec.ts @@ -13,25 +13,30 @@ describe("MatchesController", () => { isOrganizer: jest.Mock; rebootOnDemandServer: jest.Mock; }; + let hasura: { query: jest.Mock }; + let notifications: { send: jest.Mock }; beforeEach(() => { matchAssistant = { isOrganizer: jest.fn(), rebootOnDemandServer: jest.fn(), }; + hasura = { query: jest.fn() }; + notifications = { send: jest.fn().mockResolvedValue(undefined) }; controller = new MatchesController( {} as any, - {} as any, + hasura as any, {} as any, { - get: jest.fn(() => ({})), + get: jest.fn(() => ({ webDomain: "https://5stack.test" })), } as any, {} as any, matchAssistant as any, {} as any, {} as any, {} as any, + notifications as any, {} as any, {} as any, {} as any, @@ -52,7 +57,6 @@ describe("MatchesController", () => { {} as any, {} as any, {} as any, - {} as any ); }); @@ -82,4 +86,92 @@ describe("MatchesController", () => { expect(matchAssistant.rebootOnDemandServer).toHaveBeenCalledWith("match-1"); }); + + describe("callForOrganizer", () => { + const matchId = "00000000-0000-0000-0000-000000000001"; + + const callForOrganizer = () => + controller.callForOrganizer({ + match_id: matchId, + user: { steam_id: "76561198000000001", name: "signed-in-as" } as any, + }); + + const respond = ( + match: Record, + player: { name: string } | null = { name: "keith" }, + ) => + hasura.query.mockResolvedValue({ + matches_by_pk: match, + players_by_pk: player, + }); + + const sent = () => { + const [type, notification] = notifications.send.mock.calls[0]; + return { type, ...notification }; + }; + + beforeEach(() => { + respond({ is_in_lineup: true, requested_organizer: false }); + }); + + it("names the requester without linking them", async () => { + respond( + { is_in_lineup: true, requested_organizer: false }, + { name: "keith" }, + ); + + await callForOrganizer(); + + const notification = sent(); + expect(notification.type).toBe("MatchSupport"); + expect(notification.message).toContain( + "<b>keith</b> requested assistance in match", + ); + expect(notification.message.match(/href="([^"]+)"/)?.[1]).toBe( + `https://5stack.test/matches/${matchId}`, + ); + }); + + it("uses the current name rather than the one the session signed in with", async () => { + await callForOrganizer(); + + expect(sent().message).toContain("keith requested assistance"); + expect(sent().message).not.toContain("signed-in-as"); + }); + + it("falls back to the steam id when the player row is missing", async () => { + respond({ is_in_lineup: true, requested_organizer: false }, null); + + await callForOrganizer(); + + expect(sent().message).toContain( + "76561198000000001 requested assistance", + ); + }); + + it("titles the notification without the old typo", async () => { + await callForOrganizer(); + + expect(sent().title).toBe("Match Assistance Required"); + expect(sent().message).not.toContain("Assistanced"); + }); + + it("rejects someone who is not playing in the match", async () => { + respond({ is_in_lineup: false, requested_organizer: false }); + + await expect(callForOrganizer()).rejects.toThrow( + "only players in this match can contact support", + ); + expect(notifications.send).not.toHaveBeenCalled(); + }); + + it("does not ask twice while a request is still open", async () => { + respond({ is_in_lineup: true, requested_organizer: true }); + + await expect(callForOrganizer()).resolves.toEqual({ + success: true, + }); + expect(notifications.send).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/matches/matches.controller.ts b/src/matches/matches.controller.ts index 01a6d990..8896a78c 100644 --- a/src/matches/matches.controller.ts +++ b/src/matches/matches.controller.ts @@ -2518,18 +2518,25 @@ export class MatchesController { @HasuraAction() public async callForOrganizer(data: { user: User; match_id: string }) { - const { matches_by_pk: match } = await this.hasura.query( - { - matches_by_pk: { - __args: { - id: data.match_id, + const { matches_by_pk: match, players_by_pk: requester } = + await this.hasura.query( + { + matches_by_pk: { + __args: { + id: data.match_id, + }, + is_in_lineup: true, + requested_organizer: true, + }, + players_by_pk: { + __args: { + steam_id: data.user.steam_id, + }, + name: true, }, - is_in_lineup: true, - requested_organizer: true, }, - }, - data.user.steam_id, - ); + data.user.steam_id, + ); if (!match || match.requested_organizer) { return { @@ -2537,11 +2544,22 @@ export class MatchesController { }; } + if (!match.is_in_lineup) { + throw Error("only players in this match can contact support"); + } + + // The requester stays plain text: notificationUrl takes the first href as + // where the push lands, and that has to be the match. The name is read from + // the row because the session keeps whatever it was at sign-in. + const requesterName = NotificationsService.escapeHtml( + requester?.name ?? data.user.steam_id, + ); + void this.notifications.send( "MatchSupport", { - message: `Match Assistanced Required ${data.match_id}`, - title: "Match Assistanced Required", + message: `${requesterName} requested assistance in match ${data.match_id}`, + title: "Match Assistance Required", role: "match_organizer", entity_id: data.match_id, }, diff --git a/src/notifications/notifications.service.spec.ts b/src/notifications/notifications.service.spec.ts index 3c4f841a..e4a4256b 100644 --- a/src/notifications/notifications.service.spec.ts +++ b/src/notifications/notifications.service.spec.ts @@ -174,3 +174,82 @@ describe("CS2 build notices", () => { expect(NotificationsService.truncateDiscord("short")).toBe("short"); }); }); + +describe("NotificationsService", () => { + const webDomain = "https://5stack.test"; + let service: NotificationsService; + let postgres: { query: jest.Mock }; + let hasura: { query: jest.Mock; mutation: jest.Mock }; + let notifyPlayers: jest.SpyInstance; + + const sanction = (type: string) => ({ + sanctionId: "sanction-1", + steamId: "76561198000000001", + type, + reason: "cheating", + }); + + beforeEach(() => { + postgres = { + query: jest.fn().mockResolvedValue([{ steam_id: "76561198000000002" }]), + }; + hasura = { + query: jest.fn().mockResolvedValue({ players_by_pk: { name: "keith" } }), + mutation: jest.fn().mockResolvedValue({}), + }; + + service = new NotificationsService( + hasura as any, + postgres as any, + { log: jest.fn(), warn: jest.fn(), error: jest.fn() } as any, + { get: jest.fn(() => ({ webDomain })) } as any, + {} as any, + {} as any, + {} as any, + {} as any, + ); + + notifyPlayers = jest.spyOn(service, "notifyPlayers").mockResolvedValue(1); + }); + + describe("playerProfileLink", () => { + it("links to the absolute profile url", () => { + expect(service.playerProfileLink("76561198000000001", "keith")).toBe( + `keith`, + ); + }); + + it("escapes the name and encodes the steam id", () => { + expect( + service.playerProfileLink('1">`), + ).toBe( + `` + + `<img src=x onerror="alert('1')">`, + ); + }); + }); + + describe("notifyMatchPlayersOfSanction", () => { + it.each(["mute", "gag", "silence"])( + "keeps a %s between the player and staff", + async (type) => { + await service.notifyMatchPlayersOfSanction(sanction(type)); + + expect(postgres.query).not.toHaveBeenCalled(); + expect(notifyPlayers).not.toHaveBeenCalled(); + }, + ); + + it("tells recent team-mates about a ban", async () => { + await service.notifyMatchPlayersOfSanction(sanction("ban")); + + expect(notifyPlayers).toHaveBeenCalledTimes(1); + const [type, notification] = notifyPlayers.mock.calls[0]; + expect(type).toBe("PlayerSanctioned"); + expect(notification.steamIds).toEqual(["76561198000000002"]); + expect(notification.message).toContain( + `keith, was banned. (cheating)`, + ); + }); + }); +}); diff --git a/src/notifications/notifications.service.ts b/src/notifications/notifications.service.ts index 97d71ece..d67dbb52 100644 --- a/src/notifications/notifications.service.ts +++ b/src/notifications/notifications.service.ts @@ -114,12 +114,11 @@ export class NotificationsService { .replace(/'/g, "'"); } - private static readonly SANCTION_VERBS: Record = { - ban: "banned", - mute: "muted", - gag: "gagged", - silence: "silenced", - }; + public playerProfileLink(steamId: string, name: string): string { + return `${NotificationsService.escapeHtml(name)}`; + } async notifyMatchPlayersOfSanction(sanction: { sanctionId: string; @@ -127,6 +126,12 @@ export class NotificationsService { type: string; reason?: string | null; }): Promise { + // A mute or gag is chat moderation, and "was muted" sent to six months of + // team-mates reads as a ban to every one of them. + if (sanction.type !== "ban") { + return; + } + const recipients = await this.postgres.query>( `SELECT DISTINCT other_p.steam_id::text AS steam_id FROM public.matches m @@ -154,19 +159,12 @@ export class NotificationsService { }); const name = players_by_pk?.name ?? `Player ${sanction.steamId}`; - const verb = - NotificationsService.SANCTION_VERBS[sanction.type] ?? "sanctioned"; - const safeName = NotificationsService.escapeHtml(name); - const profileUrl = `${this.appConfig.webDomain}/players/${encodeURIComponent( - sanction.steamId, - )}`; - const reasonSuffix = - sanction.type === "ban" && sanction.reason - ? ` (${NotificationsService.escapeHtml(sanction.reason)})` - : ""; + const reasonSuffix = sanction.reason + ? ` (${NotificationsService.escapeHtml(sanction.reason)})` + : ""; const message = `A player you recently played with, ` + - `${safeName}, was ${verb}.${reasonSuffix}`; + `${this.playerProfileLink(sanction.steamId, name)}, was banned.${reasonSuffix}`; // Through notifyPlayers rather than the raw insert this used to be. Six // months of team-mates is routinely hundreds of rows and the event trigger diff --git a/src/sockets/sockets.gateway.spec.ts b/src/sockets/sockets.gateway.spec.ts new file mode 100644 index 00000000..7f19d28c --- /dev/null +++ b/src/sockets/sockets.gateway.spec.ts @@ -0,0 +1,72 @@ +import { SocketsGateway } from "./sockets.gateway"; + +describe("SocketsGateway ping", () => { + let gateway: SocketsGateway; + let sockets: { updateClient: jest.Mock }; + + const OPEN = 1; + const CLOSED = 3; + + const client = (overrides: Record = {}) => + ({ + id: "client-1", + user: undefined, + readyState: OPEN, + OPEN, + send: jest.fn(), + ...overrides, + }) as any; + + beforeEach(() => { + sockets = { updateClient: jest.fn().mockResolvedValue(undefined) }; + gateway = new SocketsGateway(sockets as any); + }); + + it("answers an anonymous client without touching its presence", async () => { + const anonymous = client(); + + await gateway.handleMessage(anonymous); + + expect(anonymous.send).toHaveBeenCalledWith( + JSON.stringify({ event: "pong" }), + ); + expect(sockets.updateClient).not.toHaveBeenCalled(); + }); + + it("answers a signed-in client and refreshes its presence", async () => { + const signedIn = client({ user: { steam_id: "76561198000000001" } }); + + await gateway.handleMessage(signedIn); + + expect(signedIn.send).toHaveBeenCalledWith( + JSON.stringify({ event: "pong" }), + ); + expect(sockets.updateClient).toHaveBeenCalledWith( + "76561198000000001", + "client-1", + ); + }); + + it("sends the pong before presence is written", async () => { + let sentBeforeUpdate = false; + const signedIn = client({ user: { steam_id: "76561198000000001" } }); + sockets.updateClient.mockImplementation(async () => { + sentBeforeUpdate = signedIn.send.mock.calls.length > 0; + }); + + await gateway.handleMessage(signedIn); + + expect(sentBeforeUpdate).toBe(true); + }); + + it("sends nothing to a socket that is no longer open", async () => { + const closing = client({ + readyState: CLOSED, + user: { steam_id: "76561198000000001" }, + }); + + await gateway.handleMessage(closing); + + expect(closing.send).not.toHaveBeenCalled(); + }); +}); diff --git a/src/sockets/sockets.gateway.ts b/src/sockets/sockets.gateway.ts index b10266f2..5a2a029d 100644 --- a/src/sockets/sockets.gateway.ts +++ b/src/sockets/sockets.gateway.ts @@ -17,6 +17,12 @@ export class SocketsGateway implements OnGatewayConnection { @SubscribeMessage("ping") public async handleMessage(client: FiveStackWebSocketClient): Promise { + // Sent here rather than returned: WsAdapter only replies once the handler + // resolves, which would hold the pong behind the redis writes below. + if (client.readyState === client.OPEN) { + client.send(JSON.stringify({ event: "pong" })); + } + if (!client.user) { return; } diff --git a/src/system/system.controller.spec.ts b/src/system/system.controller.spec.ts index a5aed324..371627aa 100644 --- a/src/system/system.controller.spec.ts +++ b/src/system/system.controller.spec.ts @@ -6,7 +6,11 @@ import { SystemController } from "./system.controller"; describe("SystemController names", () => { let controller: SystemController; let hasura: { query: jest.Mock; mutation: jest.Mock }; - let notifications: { send: jest.Mock; notifyPlayers: jest.Mock }; + let notifications: { + send: jest.Mock; + notifyPlayers: jest.Mock; + playerProfileLink: jest.Mock; + }; let player: { name: string; name_registered: boolean } | null; const user = (steamId: string, role: string | null = "user") => @@ -25,7 +29,14 @@ describe("SystemController names", () => { mutation: jest.fn(async () => ({})), }; - notifications = { send: jest.fn(), notifyPlayers: jest.fn() }; + notifications = { + send: jest.fn(), + notifyPlayers: jest.fn(), + playerProfileLink: jest.fn( + (steamId: string, name: string) => + `${name}`, + ), + }; controller = new SystemController( {} as any, @@ -105,6 +116,13 @@ describe("SystemController names", () => { expect(notifications.send).toHaveBeenCalled(); const [, notification] = notifications.send.mock.calls[0]; expect(notification.entity_id).toBe("76561198000000001"); + expect(notifications.playerProfileLink).toHaveBeenCalledWith( + "76561198000000001", + "current", + ); + expect(notification.message).toContain( + 'current', + ); }); it("ignores a steam id the caller does not own", async () => { @@ -118,6 +136,14 @@ describe("SystemController names", () => { const [, notification] = notifications.send.mock.calls[0]; expect(notification.entity_id).toBe("76561198000000001"); + expect(notifications.playerProfileLink).toHaveBeenCalledWith( + "76561198000000001", + "current", + ); + expect(notifications.playerProfileLink).not.toHaveBeenCalledWith( + "76561198000000002", + expect.anything(), + ); }); it("lets an administrator file a request for another player", async () => { @@ -129,6 +155,10 @@ describe("SystemController names", () => { const [, notification] = notifications.send.mock.calls[0]; expect(notification.entity_id).toBe("76561198000000002"); + expect(notifications.playerProfileLink).toHaveBeenCalledWith( + "76561198000000002", + "current", + ); }); it("rejects a blank name", async () => { diff --git a/src/system/system.controller.ts b/src/system/system.controller.ts index 8b27bc6e..e02b9cd0 100644 --- a/src/system/system.controller.ts +++ b/src/system/system.controller.ts @@ -313,7 +313,7 @@ export class SystemController { await this.notifications.send( "NameChangeRequest", { - message: `Player ${NotificationsService.escapeHtml(player.name)} has requested to change their name to ${NotificationsService.escapeHtml(name)}`, + message: `Player ${this.notifications.playerProfileLink(steamId, player.name)} has requested to change their name to ${NotificationsService.escapeHtml(name)}`, title: "Name Change Request", role: "administrator", entity_id: steamId, diff --git a/src/tournaments/tournaments.controller.spec.ts b/src/tournaments/tournaments.controller.spec.ts index ba80281e..47d13fe7 100644 --- a/src/tournaments/tournaments.controller.spec.ts +++ b/src/tournaments/tournaments.controller.spec.ts @@ -258,7 +258,7 @@ describe("TournamentsController registration and check-in actions", () => { tournament_id: "tournament-1", tournament_team_id: "team-1", }), - ).rejects.toThrow(/captain/i); + ).rejects.toThrow(/only the team captain or a team admin/i); }); }); diff --git a/src/tournaments/tournaments.controller.ts b/src/tournaments/tournaments.controller.ts index 1979b358..87a1c120 100644 --- a/src/tournaments/tournaments.controller.ts +++ b/src/tournaments/tournaments.controller.ts @@ -566,7 +566,9 @@ export class TournamentsController { } default: { if (!team.can_manage && !team.is_captain) { - throw Error("only the team captain can check this team in"); + throw Error( + "only the team captain or a team admin can check this team in", + ); } break; }