diff --git a/services/real-time/app/js/DocumentUpdaterManager.js b/services/real-time/app/js/DocumentUpdaterManager.js index 32533a9eaa..3fd09d7b1d 100644 --- a/services/real-time/app/js/DocumentUpdaterManager.js +++ b/services/real-time/app/js/DocumentUpdaterManager.js @@ -12,7 +12,7 @@ const rclient = require('redis-sharelatex').createClient( ) const Keys = settings.redis.documentupdater.key_schema -module.exports = { +const DocumentUpdaterManager = { getDocument(project_id, doc_id, fromVersion, callback) { const timer = new metrics.Timer('get-document') const url = `${settings.apis.documentupdater.url}/project/${project_id}/doc/${doc_id}?fromVersion=${fromVersion}` @@ -63,6 +63,11 @@ module.exports = { }) }, + checkDocument(project_id, doc_id, callback) { + // in this call fromVersion = -1 means get document without docOps + DocumentUpdaterManager.getDocument(project_id, doc_id, -1, callback) + }, + flushProjectToMongoAndDelete(project_id, callback) { // this method is called when the last connected user leaves the project logger.log({ project_id }, 'deleting project from document updater') @@ -141,3 +146,5 @@ module.exports = { }) } } + +module.exports = DocumentUpdaterManager diff --git a/services/real-time/app/js/Router.js b/services/real-time/app/js/Router.js index 0b4b3f00d7..ae10d12e2b 100644 --- a/services/real-time/app/js/Router.js +++ b/services/real-time/app/js/Router.js @@ -45,6 +45,7 @@ module.exports = Router = { } else if ( [ 'not authorized', + 'joinLeaveEpoch mismatch', 'doc updater could not load requested ops', 'no project_id found on client' ].includes(error.message) @@ -95,6 +96,8 @@ module.exports = Router = { // init client context, we may access it in Router._handleError before // setting any values client.ol_context = {} + // bail out from joinDoc when a parallel joinDoc or leaveDoc is running + client.joinLeaveEpoch = 0 if (client) { client.on('error', function (err) { diff --git a/services/real-time/app/js/WebsocketController.js b/services/real-time/app/js/WebsocketController.js index 239372eea9..8867320dbe 100644 --- a/services/real-time/app/js/WebsocketController.js +++ b/services/real-time/app/js/WebsocketController.js @@ -158,6 +158,7 @@ module.exports = WebsocketController = { return callback() } + const joinLeaveEpoch = ++client.joinLeaveEpoch metrics.inc('editor.join-doc') const { project_id, user_id, is_restricted_user } = client.ol_context if (!project_id) { @@ -168,10 +169,23 @@ module.exports = WebsocketController = { 'client joining doc' ) - AuthorizationManager.assertClientCanViewProject(client, function (error) { + WebsocketController._assertClientAuthorization(client, doc_id, function ( + error + ) { if (error) { return callback(error) } + if (client.disconnected) { + metrics.inc('editor.join-doc.disconnected', 1, { + status: 'after-client-auth-check' + }) + // the client will not read the response anyways + return callback() + } + if (joinLeaveEpoch !== client.joinLeaveEpoch) { + // another joinDoc or leaveDoc rpc overtook us + return callback(new Error('joinLeaveEpoch mismatch')) + } // ensure the per-doc applied-ops channel is subscribed before sending the // doc to the client, so that no events are missed. RoomManager.joinDoc(client, doc_id, function (error) { @@ -279,8 +293,42 @@ module.exports = WebsocketController = { }) }, + _assertClientAuthorization(client, doc_id, callback) { + // Check for project-level access first + AuthorizationManager.assertClientCanViewProject(client, function (error) { + if (error) { + return callback(error) + } + // Check for doc-level access next + AuthorizationManager.assertClientCanViewProjectAndDoc( + client, + doc_id, + function (error) { + if (error) { + // No cached access, check docupdater + const { project_id } = client.ol_context + DocumentUpdaterManager.checkDocument(project_id, doc_id, function ( + error + ) { + if (error) { + return callback(error) + } else { + // Success + AuthorizationManager.addAccessToDoc(client, doc_id, callback) + } + }) + } else { + // Access already cached + callback() + } + } + ) + }) + }, + leaveDoc(client, doc_id, callback) { // client may have disconnected, but we have to cleanup internal state. + client.joinLeaveEpoch++ metrics.inc('editor.leave-doc') const { project_id, user_id } = client.ol_context logger.log( diff --git a/services/real-time/test/unit/js/WebsocketControllerTests.js b/services/real-time/test/unit/js/WebsocketControllerTests.js index 7796ca7275..515d407cc2 100644 --- a/services/real-time/test/unit/js/WebsocketControllerTests.js +++ b/services/real-time/test/unit/js/WebsocketControllerTests.js @@ -38,6 +38,7 @@ describe('WebsocketController', function () { id: (this.client_id = 'mock-client-id-123'), publicId: `other-id-${Math.random()}`, ol_context: {}, + joinLeaveEpoch: 0, join: sinon.stub(), leave: sinon.stub() } @@ -503,10 +504,13 @@ describe('WebsocketController', function () { this.client.ol_context.project_id = this.project_id this.client.ol_context.is_restricted_user = false - this.AuthorizationManager.addAccessToDoc = sinon.stub() + this.AuthorizationManager.addAccessToDoc = sinon.stub().yields() this.AuthorizationManager.assertClientCanViewProject = sinon .stub() .callsArgWith(1, null) + this.AuthorizationManager.assertClientCanViewProjectAndDoc = sinon + .stub() + .callsArgWith(2, null) this.DocumentUpdaterManager.getDocument = sinon .stub() .callsArgWith( @@ -531,6 +535,10 @@ describe('WebsocketController', function () { ) }) + it('should inc the joinLeaveEpoch', function () { + expect(this.client.joinLeaveEpoch).to.equal(1) + }) + it('should check that the client is authorized to view the project', function () { return this.AuthorizationManager.assertClientCanViewProject .calledWith(this.client) @@ -739,6 +747,136 @@ describe('WebsocketController', function () { }) }) + describe('when the client disconnects while auth checks are running', function () { + beforeEach(function (done) { + this.AuthorizationManager.assertClientCanViewProjectAndDoc.yields( + new Error() + ) + this.DocumentUpdaterManager.checkDocument = ( + project_id, + doc_id, + cb + ) => { + this.client.disconnected = true + cb() + } + + this.WebsocketController.joinDoc( + this.client, + this.doc_id, + -1, + this.options, + (...args) => { + this.callback(...args) + done() + } + ) + }) + + it('should call the callback with no details', function () { + expect(this.callback.called).to.equal(true) + expect(this.callback.args[0]).to.deep.equal([]) + }) + + it('should increment the editor.join-doc.disconnected metric with a status', function () { + expect( + this.metrics.inc.calledWith('editor.join-doc.disconnected', 1, { + status: 'after-client-auth-check' + }) + ).to.equal(true) + }) + + it('should not get the document', function () { + expect(this.DocumentUpdaterManager.getDocument.called).to.equal(false) + }) + }) + + describe('when the client starts a parallel joinDoc request', function () { + beforeEach(function (done) { + this.AuthorizationManager.assertClientCanViewProjectAndDoc.yields( + new Error() + ) + this.DocumentUpdaterManager.checkDocument = ( + project_id, + doc_id, + cb + ) => { + this.DocumentUpdaterManager.checkDocument = sinon.stub().yields() + this.WebsocketController.joinDoc( + this.client, + this.doc_id, + -1, + {}, + () => {} + ) + cb() + } + + this.WebsocketController.joinDoc( + this.client, + this.doc_id, + -1, + this.options, + (...args) => { + this.callback(...args) + // make sure the other joinDoc request completed + setTimeout(done, 5) + } + ) + }) + + it('should call the callback with an error', function () { + expect(this.callback.called).to.equal(true) + expect(this.callback.args[0][0].message).to.equal( + 'joinLeaveEpoch mismatch' + ) + }) + + it('should get the document once (the parallel request wins)', function () { + expect(this.DocumentUpdaterManager.getDocument.callCount).to.equal(1) + }) + }) + + describe('when the client starts a parallel leaveDoc request', function () { + beforeEach(function (done) { + this.RoomManager.leaveDoc = sinon.stub() + + this.AuthorizationManager.assertClientCanViewProjectAndDoc.yields( + new Error() + ) + this.DocumentUpdaterManager.checkDocument = ( + project_id, + doc_id, + cb + ) => { + this.WebsocketController.leaveDoc(this.client, this.doc_id, () => {}) + cb() + } + + this.WebsocketController.joinDoc( + this.client, + this.doc_id, + -1, + this.options, + (...args) => { + this.callback(...args) + done() + } + ) + }) + + it('should call the callback with an error', function () { + expect(this.callback.called).to.equal(true) + expect(this.callback.args[0][0].message).to.equal( + 'joinLeaveEpoch mismatch' + ) + }) + + it('should not get the document', function () { + expect(this.DocumentUpdaterManager.getDocument.called).to.equal(false) + }) + }) + describe('when the client disconnects while RoomManager.joinDoc is running', function () { beforeEach(function () { this.RoomManager.joinDoc = (client, doc_id, cb) => { @@ -827,6 +965,10 @@ describe('WebsocketController', function () { ) }) + it('should inc the joinLeaveEpoch', function () { + expect(this.client.joinLeaveEpoch).to.equal(1) + }) + it('should remove the client from the doc_id room', function () { return this.RoomManager.leaveDoc .calledWith(this.client, this.doc_id) diff --git a/services/real-time/test/unit/js/helpers/MockClient.js b/services/real-time/test/unit/js/helpers/MockClient.js index fb90a0f7e9..61cde89ba9 100644 --- a/services/real-time/test/unit/js/helpers/MockClient.js +++ b/services/real-time/test/unit/js/helpers/MockClient.js @@ -16,6 +16,7 @@ module.exports = MockClient = class MockClient { this.disconnect = sinon.stub() this.id = idCounter++ this.publicId = idCounter++ + this.joinLeaveEpoch = 0 } disconnect() {}