Merge pull request #177 from overleaf/jpa-bg-issue-3291

das7pad · web-flow · commit 953c5f3ffa4e · 2020-08-12T13:04:29.000+02:00
clean up join/leave handling
diff --git a/app/js/DocumentUpdaterManager.js b/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/app/js/Router.js b/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/app/js/WebsocketController.js b/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/test/unit/js/WebsocketControllerTests.js b/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/test/unit/js/helpers/MockClient.js b/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() {}

Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,7 @@ module.exports = MockClient = class MockClient {`
`16`	`16`	`this.disconnect = sinon.stub()`
`17`	`17`	`this.id = idCounter++`
`18`	`18`	`this.publicId = idCounter++`
	`19`	`+ this.joinLeaveEpoch = 0`
`19`	`20`	`}`
`20`	`21`
`21`	`22`	`disconnect() {}`