From dee4749e6d8032c1914c94fc9b85dde41233b07e Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 27 Aug 2020 10:11:40 +0100 Subject: [PATCH 1/6] [misc] re-level log: properly silence unauthorized updateClientPosition --- services/real-time/app/js/WebsocketController.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/real-time/app/js/WebsocketController.js b/services/real-time/app/js/WebsocketController.js index 7b5f2af602..38a35db8e6 100644 --- a/services/real-time/app/js/WebsocketController.js +++ b/services/real-time/app/js/WebsocketController.js @@ -356,7 +356,7 @@ module.exports = WebsocketController = { cursorData.doc_id, function (error) { if (error) { - logger.warn( + logger.info( { err: error, client_id: client.id, project_id, user_id }, "silently ignoring unauthorized updateClientPosition. Client likely hasn't called joinProject yet." ) From 1ff9c1e71b3f0704d63af8c2da4cfe6402655905 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 27 Aug 2020 10:28:44 +0100 Subject: [PATCH 2/6] [misc] add the rpc-method into the log context in Router._handleError --- services/real-time/app/js/Router.js | 1 + 1 file changed, 1 insertion(+) diff --git a/services/real-time/app/js/Router.js b/services/real-time/app/js/Router.js index ec6de5136f..1aae3ea64e 100644 --- a/services/real-time/app/js/Router.js +++ b/services/real-time/app/js/Router.js @@ -33,6 +33,7 @@ module.exports = Router = { } attrs.client_id = client.id attrs.err = error + attrs.method = method if (error.name === 'CodedError') { logger.warn(attrs, error.message) const serializedError = { message: error.message, code: error.info.code } From d2a2b9d46e83505b0e27f0a09aecd0b0495b197f Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 27 Aug 2020 10:51:34 +0100 Subject: [PATCH 3/6] [misc] add tests for web replying with a 403 for joinProject --- .../test/acceptance/js/JoinProjectTests.js | 63 +++++++++++++++++++ .../acceptance/js/helpers/MockWebServer.js | 3 + .../test/unit/js/WebApiManagerTests.js | 24 +++++++ 3 files changed, 90 insertions(+) diff --git a/services/real-time/test/acceptance/js/JoinProjectTests.js b/services/real-time/test/acceptance/js/JoinProjectTests.js index 051c33d0c7..06ab8a2c5a 100644 --- a/services/real-time/test/acceptance/js/JoinProjectTests.js +++ b/services/real-time/test/acceptance/js/JoinProjectTests.js @@ -176,6 +176,69 @@ describe('joinProject', function () { }) }) + describe('when not authorized and web replies with a 403', function () { + before(function (done) { + return async.series( + [ + (cb) => { + return FixturesManager.setUpProject( + { + project_id: 'forbidden', + privilegeLevel: 'owner', + project: { + name: 'Test Project' + } + }, + (e, { project_id, user_id }) => { + this.project_id = project_id + this.user_id = user_id + cb(e) + } + ) + }, + + (cb) => { + this.client = RealTimeClient.connect() + this.client.on('connectionAccepted', cb) + }, + + (cb) => { + this.client.emit( + 'joinProject', + { project_id: this.project_id }, + (error, project, privilegeLevel, protocolVersion) => { + this.error = error + this.project = project + this.privilegeLevel = privilegeLevel + this.protocolVersion = protocolVersion + cb() + } + ) + } + ], + done + ) + }) + + it('should return an error', function () { + this.error.message.should.equal( + 'Something went wrong in real-time service' + ) + }) + + it('should not have joined the project room', function (done) { + RealTimeClient.getConnectedClient( + this.client.socket.sessionid, + (error, client) => { + expect(Array.from(client.rooms).includes(this.project_id)).to.equal( + false + ) + done() + } + ) + }) + }) + return describe('when over rate limit', function () { before(function (done) { return async.series( diff --git a/services/real-time/test/acceptance/js/helpers/MockWebServer.js b/services/real-time/test/acceptance/js/helpers/MockWebServer.js index a2cf5af50b..8cd9d4be58 100644 --- a/services/real-time/test/acceptance/js/helpers/MockWebServer.js +++ b/services/real-time/test/acceptance/js/helpers/MockWebServer.js @@ -38,6 +38,9 @@ module.exports = MockWebServer = { joinProjectRequest(req, res, next) { const { project_id } = req.params const { user_id } = req.query + if (project_id === 'forbidden') { + return res.status(403).send() + } if (project_id === 'rate-limited') { return res.status(429).send() } else { diff --git a/services/real-time/test/unit/js/WebApiManagerTests.js b/services/real-time/test/unit/js/WebApiManagerTests.js index 4435bc14f9..c2cbd3925f 100644 --- a/services/real-time/test/unit/js/WebApiManagerTests.js +++ b/services/real-time/test/unit/js/WebApiManagerTests.js @@ -91,6 +91,30 @@ describe('WebApiManager', function () { }) }) + describe('when web replies with a 403', function () { + beforeEach(function () { + this.request.post = sinon + .stub() + .callsArgWith(1, null, { statusCode: 403 }, null) + this.WebApiManager.joinProject( + this.project_id, + this.user_id, + this.callback + ) + }) + + it('should call the callback with an error', function () { + this.callback + .calledWith( + sinon.match({ + message: 'non-success status code from web', + info: { statusCode: 403 } + }) + ) + .should.equal(true) + }) + }) + describe('with an error from web', function () { beforeEach(function () { this.request.post = sinon From 884b340c759f83a8f412abd82e902e560c56b59c Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 27 Aug 2020 10:55:13 +0100 Subject: [PATCH 4/6] [misc] re-level log: 403 from web goes to WARN and emit 'not authorized' Users will get redirected to the login page and will see a 'restricted' page after logging in again. See frontend: ConnectionManager.reportConnectionError --- services/real-time/app/js/WebApiManager.js | 3 +++ services/real-time/test/acceptance/js/JoinProjectTests.js | 4 +--- services/real-time/test/unit/js/WebApiManagerTests.js | 3 +-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/services/real-time/app/js/WebApiManager.js b/services/real-time/app/js/WebApiManager.js index de293fe015..bf399eff98 100644 --- a/services/real-time/app/js/WebApiManager.js +++ b/services/real-time/app/js/WebApiManager.js @@ -8,6 +8,7 @@ const logger = require('logger-sharelatex') const { CodedError, CorruptedJoinProjectResponseError, + NotAuthorizedError, WebApiRequestFailedError } = require('./Errors') @@ -56,6 +57,8 @@ module.exports = { 'TooManyRequests' ) ) + } else if (response.statusCode === 403) { + callback(new NotAuthorizedError()) } else { callback(new WebApiRequestFailedError(response.statusCode)) } diff --git a/services/real-time/test/acceptance/js/JoinProjectTests.js b/services/real-time/test/acceptance/js/JoinProjectTests.js index 06ab8a2c5a..4c8d698fa9 100644 --- a/services/real-time/test/acceptance/js/JoinProjectTests.js +++ b/services/real-time/test/acceptance/js/JoinProjectTests.js @@ -221,9 +221,7 @@ describe('joinProject', function () { }) it('should return an error', function () { - this.error.message.should.equal( - 'Something went wrong in real-time service' - ) + this.error.message.should.equal('not authorized') }) it('should not have joined the project room', function (done) { diff --git a/services/real-time/test/unit/js/WebApiManagerTests.js b/services/real-time/test/unit/js/WebApiManagerTests.js index c2cbd3925f..0866b2b585 100644 --- a/services/real-time/test/unit/js/WebApiManagerTests.js +++ b/services/real-time/test/unit/js/WebApiManagerTests.js @@ -107,8 +107,7 @@ describe('WebApiManager', function () { this.callback .calledWith( sinon.match({ - message: 'non-success status code from web', - info: { statusCode: 403 } + message: 'not authorized' }) ) .should.equal(true) From 55d938ba14b13f47199fba497841c871ecf78939 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 27 Aug 2020 11:39:25 +0100 Subject: [PATCH 5/6] [misc] add tests for web replying with a 404 for joinProject --- .../test/acceptance/js/JoinProjectTests.js | 61 +++++++++++++++++++ .../acceptance/js/helpers/MockWebServer.js | 3 + .../test/unit/js/WebApiManagerTests.js | 24 ++++++++ 3 files changed, 88 insertions(+) diff --git a/services/real-time/test/acceptance/js/JoinProjectTests.js b/services/real-time/test/acceptance/js/JoinProjectTests.js index 4c8d698fa9..9e554c6d1c 100644 --- a/services/real-time/test/acceptance/js/JoinProjectTests.js +++ b/services/real-time/test/acceptance/js/JoinProjectTests.js @@ -237,6 +237,67 @@ describe('joinProject', function () { }) }) + describe('when deleted and web replies with a 404', function () { + before(function (done) { + return async.series( + [ + (cb) => { + return FixturesManager.setUpProject( + { + project_id: 'not-found', + privilegeLevel: 'owner', + project: { + name: 'Test Project' + } + }, + (e, { project_id, user_id }) => { + this.project_id = project_id + this.user_id = user_id + cb(e) + } + ) + }, + + (cb) => { + this.client = RealTimeClient.connect() + this.client.on('connectionAccepted', cb) + }, + + (cb) => { + this.client.emit( + 'joinProject', + { project_id: this.project_id }, + (error, project, privilegeLevel, protocolVersion) => { + this.error = error + this.project = project + this.privilegeLevel = privilegeLevel + this.protocolVersion = protocolVersion + cb() + } + ) + } + ], + done + ) + }) + + it('should return an error', function () { + this.error.message.should.equal('Something went wrong in real-time service') + }) + + it('should not have joined the project room', function (done) { + RealTimeClient.getConnectedClient( + this.client.socket.sessionid, + (error, client) => { + expect(Array.from(client.rooms).includes(this.project_id)).to.equal( + false + ) + done() + } + ) + }) + }) + return describe('when over rate limit', function () { before(function (done) { return async.series( diff --git a/services/real-time/test/acceptance/js/helpers/MockWebServer.js b/services/real-time/test/acceptance/js/helpers/MockWebServer.js index 8cd9d4be58..2de9e61275 100644 --- a/services/real-time/test/acceptance/js/helpers/MockWebServer.js +++ b/services/real-time/test/acceptance/js/helpers/MockWebServer.js @@ -38,6 +38,9 @@ module.exports = MockWebServer = { joinProjectRequest(req, res, next) { const { project_id } = req.params const { user_id } = req.query + if (project_id === 'not-found') { + return res.status(404).send() + } if (project_id === 'forbidden') { return res.status(403).send() } diff --git a/services/real-time/test/unit/js/WebApiManagerTests.js b/services/real-time/test/unit/js/WebApiManagerTests.js index 0866b2b585..4d5834495f 100644 --- a/services/real-time/test/unit/js/WebApiManagerTests.js +++ b/services/real-time/test/unit/js/WebApiManagerTests.js @@ -114,6 +114,30 @@ describe('WebApiManager', function () { }) }) + describe('when web replies with a 404', function () { + beforeEach(function () { + this.request.post = sinon + .stub() + .callsArgWith(1, null, { statusCode: 404 }, null) + this.WebApiManager.joinProject( + this.project_id, + this.user_id, + this.callback + ) + }) + + it('should call the callback with an error', function () { + this.callback + .calledWith( + sinon.match({ + message: 'non-success status code from web', + info: { statusCode: 404 } + }) + ) + .should.equal(true) + }) + }) + describe('with an error from web', function () { beforeEach(function () { this.request.post = sinon From 4960bdd6feaf542f8b69c428f610c4a2414beb97 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 27 Aug 2020 11:47:50 +0100 Subject: [PATCH 6/6] [misc] re-level log: 404 from web -> WARN and emit 'project not found' A stale browser tab tried to join a deleted project. Emitting 'project not found'/'ProjectNotFound' will trigger a page reload in the frontend, upon web can render a 404. See frontend: ConnectionManager.joinProject callback --- services/real-time/app/js/WebApiManager.js | 2 ++ services/real-time/test/acceptance/js/JoinProjectTests.js | 2 +- services/real-time/test/unit/js/WebApiManagerTests.js | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/services/real-time/app/js/WebApiManager.js b/services/real-time/app/js/WebApiManager.js index bf399eff98..cdcbc0955f 100644 --- a/services/real-time/app/js/WebApiManager.js +++ b/services/real-time/app/js/WebApiManager.js @@ -59,6 +59,8 @@ module.exports = { ) } else if (response.statusCode === 403) { callback(new NotAuthorizedError()) + } else if (response.statusCode === 404) { + callback(new CodedError('project not found', 'ProjectNotFound')) } else { callback(new WebApiRequestFailedError(response.statusCode)) } diff --git a/services/real-time/test/acceptance/js/JoinProjectTests.js b/services/real-time/test/acceptance/js/JoinProjectTests.js index 9e554c6d1c..508e2b6614 100644 --- a/services/real-time/test/acceptance/js/JoinProjectTests.js +++ b/services/real-time/test/acceptance/js/JoinProjectTests.js @@ -282,7 +282,7 @@ describe('joinProject', function () { }) it('should return an error', function () { - this.error.message.should.equal('Something went wrong in real-time service') + this.error.code.should.equal('ProjectNotFound') }) it('should not have joined the project room', function (done) { diff --git a/services/real-time/test/unit/js/WebApiManagerTests.js b/services/real-time/test/unit/js/WebApiManagerTests.js index 4d5834495f..a366a2e75d 100644 --- a/services/real-time/test/unit/js/WebApiManagerTests.js +++ b/services/real-time/test/unit/js/WebApiManagerTests.js @@ -130,8 +130,8 @@ describe('WebApiManager', function () { this.callback .calledWith( sinon.match({ - message: 'non-success status code from web', - info: { statusCode: 404 } + message: 'project not found', + info: { code: 'ProjectNotFound' } }) ) .should.equal(true)