diff --git a/backend/src/realtime/realtime-note/realtime-note.ts b/backend/src/realtime/realtime-note/realtime-note.ts index da1895301..0a2457370 100644 --- a/backend/src/realtime/realtime-note/realtime-note.ts +++ b/backend/src/realtime/realtime-note/realtime-note.ts @@ -52,7 +52,7 @@ export class RealtimeNote extends (EventEmitter as TypedEventEmitterConstructor< */ public addClient(client: WebsocketConnection): void { this.clients.add(client); - this.logger.debug(`User '${client.getUser().username}' connected`); + this.logger.debug(`User '${client.getUsername()}' connected`); } /** @@ -63,7 +63,7 @@ export class RealtimeNote extends (EventEmitter as TypedEventEmitterConstructor< public removeClient(client: WebsocketConnection): void { this.clients.delete(client); this.logger.debug( - `User '${client.getUser().username}' disconnected. ${ + `User '${client.getUsername()}' disconnected. ${ this.clients.size } clients left.`, ); diff --git a/backend/src/realtime/realtime-note/test-utils/mock-connection.ts b/backend/src/realtime/realtime-note/test-utils/mock-connection.ts index 53b9f62d4..86810f56e 100644 --- a/backend/src/realtime/realtime-note/test-utils/mock-connection.ts +++ b/backend/src/realtime/realtime-note/test-utils/mock-connection.ts @@ -18,5 +18,6 @@ export function mockConnection(synced: boolean): WebsocketConnection { isSynced: jest.fn(() => synced), send: jest.fn(), getUser: jest.fn(() => Mock.of({ username: 'mockedUser' })), + getUsername: jest.fn(() => 'mocked user'), }); } diff --git a/backend/src/realtime/realtime-note/websocket-connection.spec.ts b/backend/src/realtime/realtime-note/websocket-connection.spec.ts index 98c43880e..9388277af 100644 --- a/backend/src/realtime/realtime-note/websocket-connection.spec.ts +++ b/backend/src/realtime/realtime-note/websocket-connection.spec.ts @@ -192,4 +192,26 @@ describe('websocket connection', () => { expect(sut.getUser()).toBe(mockedUser); }); + + it('returns the correct username', () => { + const mockedUserWithUsername = Mock.of({ username: 'MockUser' }); + + const sut = new WebsocketConnection( + mockedWebsocket, + mockedUserWithUsername, + mockedRealtimeNote, + ); + + expect(sut.getUsername()).toBe('MockUser'); + }); + + it('returns a fallback if no username has been set', () => { + const sut = new WebsocketConnection( + mockedWebsocket, + mockedUser, + mockedRealtimeNote, + ); + + expect(sut.getUsername()).toBe('Guest'); + }); }); diff --git a/backend/src/realtime/realtime-note/websocket-connection.ts b/backend/src/realtime/realtime-note/websocket-connection.ts index dca73ff11..289bbc064 100644 --- a/backend/src/realtime/realtime-note/websocket-connection.ts +++ b/backend/src/realtime/realtime-note/websocket-connection.ts @@ -30,7 +30,7 @@ export class WebsocketConnection { */ constructor( websocket: WebSocket, - private user: User, + private user: User | null, realtimeNote: RealtimeNote, ) { const awareness = realtimeNote.getAwareness(); @@ -94,7 +94,11 @@ export class WebsocketConnection { return this.controlledAwarenessIds; } - public getUser(): User { + public getUser(): User | null { return this.user; } + + public getUsername(): string { + return this.getUser()?.username ?? 'Guest'; + } } diff --git a/backend/src/realtime/websocket/websocket.gateway.spec.ts b/backend/src/realtime/websocket/websocket.gateway.spec.ts index f0ecca79f..acd205510 100644 --- a/backend/src/realtime/websocket/websocket.gateway.spec.ts +++ b/backend/src/realtime/websocket/websocket.gateway.spec.ts @@ -3,6 +3,7 @@ * * SPDX-License-Identifier: AGPL-3.0-only */ +import { Optional } from '@mrdrogdrog/optional'; import { ConfigModule } from '@nestjs/config'; import { EventEmitterModule } from '@nestjs/event-emitter'; import { Test, TestingModule } from '@nestjs/testing'; @@ -64,7 +65,9 @@ describe('Websocket gateway', () => { const mockedValidSessionCookie = 'mockedValidSessionCookie'; const mockedSessionIdWithUser = 'mockedSessionIdWithUser'; const mockedValidUrl = 'mockedValidUrl'; + const mockedValidGuestUrl = 'mockedValidGuestUrl'; const mockedValidNoteId = 'mockedValidNoteId'; + const mockedValidGuestNoteId = 'mockedValidGuestNoteId'; let sessionExistsForUser = true; let noteExistsForNoteId = true; @@ -151,14 +154,15 @@ describe('Websocket gateway', () => { permissionsService = module.get(PermissionsService); jest - .spyOn(sessionService, 'extractVerifiedSessionIdFromRequest') - .mockImplementation((request: IncomingMessage): string => { - if (request.headers.cookie === mockedValidSessionCookie) { - return mockedSessionIdWithUser; - } else { - throw new Error('no valid session cookie found'); - } - }); + .spyOn(sessionService, 'extractSessionIdFromRequest') + .mockImplementation( + (request: IncomingMessage): Optional => + Optional.ofNullable( + request.headers?.cookie === mockedValidSessionCookie + ? mockedSessionIdWithUser + : null, + ), + ); const mockUsername = 'mockUsername'; jest @@ -184,26 +188,37 @@ describe('Websocket gateway', () => { .mockImplementation((request: IncomingMessage): string => { if (request.url === mockedValidUrl) { return mockedValidNoteId; + } else if (request.url === mockedValidGuestUrl) { + return mockedValidGuestNoteId; } else { throw new Error('no valid note id found'); } }); const mockedNote = Mock.of({ id: 4711 }); + const mockedGuestNote = Mock.of({ id: 1235 }); jest .spyOn(notesService, 'getNoteByIdOrAlias') - .mockImplementation((noteId: string) => - noteExistsForNoteId && noteId === mockedValidNoteId - ? Promise.resolve(mockedNote) - : Promise.reject('no note found'), - ); + .mockImplementation((noteId: string) => { + if (noteExistsForNoteId && noteId === mockedValidNoteId) { + return Promise.resolve(mockedNote); + } + if (noteId === mockedValidGuestNoteId) { + return Promise.resolve(mockedGuestNote); + } else { + return Promise.reject('no note found'); + } + }); jest .spyOn(permissionsService, 'mayRead') .mockImplementation( (user: User | null, note: Note): Promise => Promise.resolve( - user === mockUser && note === mockedNote && userHasReadPermissions, + (user === mockUser && + note === mockedNote && + userHasReadPermissions) || + (user === null && note === mockedGuestNote), ), ); @@ -231,6 +246,22 @@ describe('Websocket gateway', () => { addClientSpy = jest.spyOn(mockedRealtimeNote, 'addClient'); }); + it('adds a valid connection request without a session', async () => { + const request = Mock.of({ + socket: { + remoteAddress: 'mockHost', + }, + url: mockedValidGuestUrl, + headers: {}, + }); + + await expect( + gateway.handleConnection(mockedWebsocket, request), + ).resolves.not.toThrow(); + expect(addClientSpy).toHaveBeenCalledWith(mockedWebsocketConnection); + expect(mockedWebsocketCloseSpy).not.toHaveBeenCalled(); + }); + it('adds a valid connection request', async () => { const request = Mock.of({ socket: { diff --git a/backend/src/realtime/websocket/websocket.gateway.ts b/backend/src/realtime/websocket/websocket.gateway.ts index 22a92b24c..1e05b973a 100644 --- a/backend/src/realtime/websocket/websocket.gateway.ts +++ b/backend/src/realtime/websocket/websocket.gateway.ts @@ -52,10 +52,12 @@ export class WebsocketGateway implements OnGatewayConnection { extractNoteIdFromRequestUrl(request), ); + const username = user?.username ?? 'guest'; + if (!(await this.permissionsService.mayRead(user, note))) { //TODO: [mrdrogdrog] inform client about reason of disconnect. this.logger.log( - `Access denied to note '${note.id}' for user '${user.username}'`, + `Access denied to note '${note.id}' for user '${username}'`, 'handleConnection', ); clientSocket.close(); @@ -65,7 +67,7 @@ export class WebsocketGateway implements OnGatewayConnection { this.logger.debug( `New realtime connection to note '${note.id}' (${ note.publicId - }) by user '${user.username}' from ${ + }) by user '${username}' from ${ request.socket.remoteAddress ?? 'unknown' }`, ); @@ -98,11 +100,15 @@ export class WebsocketGateway implements OnGatewayConnection { */ private async findUserByRequestSession( request: IncomingMessage, - ): Promise { - const sessionId = - this.sessionService.extractVerifiedSessionIdFromRequest(request); + ): Promise { + const sessionId = this.sessionService.extractSessionIdFromRequest(request); + + if (sessionId.isEmpty()) { + return null; + } + const username = await this.sessionService.fetchUsernameForSessionId( - sessionId, + sessionId.get(), ); return await this.userService.getUserByUsername(username); } diff --git a/backend/src/session/session.service.spec.ts b/backend/src/session/session.service.spec.ts index 8e8be2f05..7e10aec5e 100644 --- a/backend/src/session/session.service.spec.ts +++ b/backend/src/session/session.service.spec.ts @@ -129,9 +129,9 @@ describe('SessionService', () => { const mockedRequest = Mock.of({ headers: {}, }); - expect(() => - sessionService.extractVerifiedSessionIdFromRequest(mockedRequest), - ).toThrow('No hedgedoc-session cookie found'); + expect( + sessionService.extractSessionIdFromRequest(mockedRequest).isEmpty(), + ).toBeTruthy(); }); it("fails if the cookie header isn't valid", () => { @@ -139,9 +139,9 @@ describe('SessionService', () => { headers: { cookie: 'no' }, }); mockParseCookieModule(`s:anyValidSessionId.validSignature`); - expect(() => - sessionService.extractVerifiedSessionIdFromRequest(mockedRequest), - ).toThrow('No hedgedoc-session cookie found'); + expect( + sessionService.extractSessionIdFromRequest(mockedRequest).isEmpty(), + ).toBeTruthy(); }); it("fails if the hedgedoc session cookie isn't marked as signed", () => { @@ -150,8 +150,10 @@ describe('SessionService', () => { }); mockParseCookieModule('sessionId.validSignature'); expect(() => - sessionService.extractVerifiedSessionIdFromRequest(mockedRequest), - ).toThrow("cookie doesn't look like a signed cookie"); + sessionService.extractSessionIdFromRequest(mockedRequest), + ).toThrow( + 'cookie "hedgedoc-session" doesn\'t look like a signed session cookie', + ); }); it("fails if the hedgedoc session cookie doesn't contain a session id", () => { @@ -160,8 +162,10 @@ describe('SessionService', () => { }); mockParseCookieModule('s:.validSignature'); expect(() => - sessionService.extractVerifiedSessionIdFromRequest(mockedRequest), - ).toThrow("cookie doesn't look like a signed cookie"); + sessionService.extractSessionIdFromRequest(mockedRequest), + ).toThrow( + 'cookie "hedgedoc-session" doesn\'t look like a signed session cookie', + ); }); it("fails if the hedgedoc session cookie doesn't contain a signature", () => { @@ -170,8 +174,10 @@ describe('SessionService', () => { }); mockParseCookieModule('s:sessionId.'); expect(() => - sessionService.extractVerifiedSessionIdFromRequest(mockedRequest), - ).toThrow("cookie doesn't look like a signed cookie"); + sessionService.extractSessionIdFromRequest(mockedRequest), + ).toThrow( + 'cookie "hedgedoc-session" doesn\'t look like a signed session cookie', + ); }); it("fails if the hedgedoc session cookie isn't signed correctly", () => { @@ -180,8 +186,8 @@ describe('SessionService', () => { }); mockParseCookieModule('s:sessionId.invalidSignature'); expect(() => - sessionService.extractVerifiedSessionIdFromRequest(mockedRequest), - ).toThrow("Signature of hedgedoc-session cookie isn't valid."); + sessionService.extractSessionIdFromRequest(mockedRequest), + ).toThrow('signature of cookie "hedgedoc-session" isn\'t valid.'); }); it('can extract a session id from a valid request', () => { @@ -190,7 +196,7 @@ describe('SessionService', () => { }); mockParseCookieModule(`s:${validSessionId}.validSignature`); expect( - sessionService.extractVerifiedSessionIdFromRequest(mockedRequest), + sessionService.extractSessionIdFromRequest(mockedRequest).get(), ).toBe(validSessionId); }); }); diff --git a/backend/src/session/session.service.ts b/backend/src/session/session.service.ts index e9515fb94..e79d4dcc2 100644 --- a/backend/src/session/session.service.ts +++ b/backend/src/session/session.service.ts @@ -70,27 +70,37 @@ export class SessionService { * Extracts the hedgedoc session cookie from the given {@link IncomingMessage request} and checks if the signature is correct. * * @param request The http request that contains a session cookie - * @return The extracted session id - * @throws Error if no session cookie was found - * @throws Error if the cookie content is malformed - * @throws Error if the cookie content isn't signed + * @return An {@link Optional optional} that either contains the extracted session id or is empty if no session cookie has been found + * @throws Error if the cookie has been found but the content is malformed + * @throws Error if the cookie has been found but the content isn't signed */ - extractVerifiedSessionIdFromRequest(request: IncomingMessage): string { - return Optional.ofNullable(request.headers.cookie) + extractSessionIdFromRequest(request: IncomingMessage): Optional { + return Optional.ofNullable(request.headers?.cookie) .map((cookieHeader) => parseCookie(cookieHeader)[HEDGEDOC_SESSION]) - .orThrow(() => new Error(`No ${HEDGEDOC_SESSION} cookie found`)) - .map((cookie) => SessionService.sessionCookieContentRegex.exec(cookie)) - .orThrow( - () => - new Error( - `${HEDGEDOC_SESSION} cookie doesn't look like a signed cookie`, - ), - ) - .guard( - (cookie) => unsign(cookie[1], this.authConfig.session.secret) !== false, - () => new Error(`Signature of ${HEDGEDOC_SESSION} cookie isn't valid.`), - ) - .map((cookie) => cookie[2]) - .get(); + .map((rawCookie) => + this.extractVerifiedSessionIdFromCookieContent(rawCookie), + ); + } + + /** + * Parses the given session cookie content and extracts the session id. + * + * @param rawCookie The cookie to parse + * @return The extracted session id + * @throws Error if the cookie has been found but the content is malformed + * @throws Error if the cookie has been found but the content isn't signed + */ + private extractVerifiedSessionIdFromCookieContent(rawCookie: string): string { + const parsedCookie = + SessionService.sessionCookieContentRegex.exec(rawCookie); + if (parsedCookie === null) { + throw new Error( + `cookie "${HEDGEDOC_SESSION}" doesn't look like a signed session cookie`, + ); + } + if (unsign(parsedCookie[1], this.authConfig.session.secret) === false) { + throw new Error(`signature of cookie "${HEDGEDOC_SESSION}" isn't valid.`); + } + return parsedCookie[2]; } }