Merge pull request #2870 from overleaf/sk-restrict-chat

Block restricted users from Chat endpoints

GitOrigin-RevId: caec8fe2bc93d567dd57f32dc765bd74ba53e933

services/web/app/src/Features/Authorization/AuthorizationMiddleware.js

@@ -43,6 +43,33 @@ module.exports = AuthorizationMiddleware = {
     })
   },
 
+  blockRestrictedUserFromProject(req, res, next) {
+    AuthorizationMiddleware._getUserAndProjectId(req, function(
+      error,
+      userId,
+      projectId
+    ) {
+      if (error) {
+        return next(error)
+      }
+      const token = TokenAccessHandler.getRequestToken(req, projectId)
+      AuthorizationManager.isRestrictedUserForProject(
+        userId,
+        projectId,
+        token,
+        (err, isRestrictedUser) => {
+          if (err) {
+            return next(err)
+          }
+          if (isRestrictedUser) {
+            return res.sendStatus(403)
+          }
+          next()
+        }
+      )
+    })
+  },
+
   ensureUserCanReadProject(req, res, next) {
     AuthorizationMiddleware._getUserAndProjectId(req, function(
       error,

services/web/app/src/router.js

@@ -801,11 +801,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
 
   webRouter.get(
     '/project/:project_id/messages',
+    AuthorizationMiddleware.blockRestrictedUserFromProject,
     AuthorizationMiddleware.ensureUserCanReadProject,
     ChatController.getMessages
   )
   webRouter.post(
     '/project/:project_id/messages',
+    AuthorizationMiddleware.blockRestrictedUserFromProject,
     AuthorizationMiddleware.ensureUserCanReadProject,
     RateLimiterMiddleware.rateLimit({
       endpointName: 'send-chat-message',

services/web/test/acceptance/src/AuthorizationTests.js

@@ -4,6 +4,7 @@ const User = require('./helpers/User')
 const request = require('./helpers/request')
 const settings = require('settings-sharelatex')
 
+require('./helpers/MockChatApi')
 require('./helpers/MockDocstoreApi')
 require('./helpers/MockDocUpdaterApi')
 
@@ -262,6 +263,38 @@ function expectNoAnonymousAdminAccess(user, projectId, callback) {
   )
 }
 
+function expectChatAccess(user, projectId, callback) {
+  user.request.get(
+    {
+      url: `/project/${projectId}/messages`,
+      json: true
+    },
+    (error, response) => {
+      if (error != null) {
+        return callback(error)
+      }
+      expect(response.statusCode).to.equal(200)
+      callback()
+    }
+  )
+}
+
+function expectNoChatAccess(user, projectId, callback) {
+  user.request.get(
+    {
+      url: `/project/${projectId}/messages`,
+      json: true
+    },
+    (error, response) => {
+      if (error != null) {
+        return callback(error)
+      }
+      expect(response.statusCode).to.equal(403)
+      callback()
+    }
+  )
+}
+
 describe('Authorization', function() {
   beforeEach(function(done) {
     this.timeout(90000)
@@ -316,6 +349,10 @@ describe('Authorization', function() {
       expectAdminAccess(this.owner, this.projectId, done)
     })
 
+    it('should allow the owner user chat messages access', function(done) {
+      expectChatAccess(this.owner, this.projectId, done)
+    })
+
     it('should not allow another user read access to the project', function(done) {
       expectNoReadAccess(
         this.other1,
@@ -342,6 +379,10 @@ describe('Authorization', function() {
       expectNoAdminAccess(this.other1, this.projectId, done)
     })
 
+    it('should not allow another user chat messages access', function(done) {
+      expectNoChatAccess(this.other1, this.projectId, done)
+    })
+
     it('should not allow anonymous user read access to it', function(done) {
       expectNoReadAccess(
         this.anon,
@@ -368,6 +409,10 @@ describe('Authorization', function() {
       expectNoAnonymousAdminAccess(this.anon, this.projectId, done)
     })
 
+    it('should not allow anonymous user chat messages access', function(done) {
+      expectNoChatAccess(this.anon, this.projectId, done)
+    })
+
     it('should allow site admin users read access to it', function(done) {
       expectReadAccess(this.site_admin, this.projectId, done)
     })
@@ -422,6 +467,10 @@ describe('Authorization', function() {
       expectReadAccess(this.ro_user, this.projectId, done)
     })
 
+    it('should allow the read-only user chat messages access', function(done) {
+      expectChatAccess(this.ro_user, this.projectId, done)
+    })
+
     it('should not allow the read-only user write access to its content', function(done) {
       expectNoContentWriteAccess(this.ro_user, this.projectId, done)
     })
@@ -454,6 +503,10 @@ describe('Authorization', function() {
     it('should not allow the read-write user admin access to it', function(done) {
       expectNoAdminAccess(this.rw_user, this.projectId, done)
     })
+
+    it('should allow the read-write user chat messages access', function(done) {
+      expectChatAccess(this.rw_user, this.projectId, done)
+    })
   })
 
   describe('public read-write project', function() {
@@ -475,6 +528,10 @@ describe('Authorization', function() {
       expectContentWriteAccess(this.other1, this.projectId, done)
     })
 
+    it('should allow a user chat messages access', function(done) {
+      expectChatAccess(this.other1, this.projectId, done)
+    })
+
     it('should not allow a user write access to its settings', function(done) {
       expectNoSettingsWriteAccess(
         this.other1,
@@ -496,6 +553,10 @@ describe('Authorization', function() {
       expectContentWriteAccess(this.anon, this.projectId, done)
     })
 
+    it('should allow an anonymous user chat messages access', function(done) {
+      expectChatAccess(this.anon, this.projectId, done)
+    })
+
     it('should not allow an anonymous user write access to its settings', function(done) {
       expectNoSettingsWriteAccess(
         this.anon,
@@ -542,6 +603,11 @@ describe('Authorization', function() {
       expectNoAdminAccess(this.other1, this.projectId, done)
     })
 
+    // NOTE: legacy readOnly access does not count as 'restricted' in the new model
+    it('should allow a user chat messages access', function(done) {
+      expectChatAccess(this.other1, this.projectId, done)
+    })
+
     it('should allow an anonymous user read access to it', function(done) {
       expectReadAccess(this.anon, this.projectId, done)
     })
@@ -562,5 +628,9 @@ describe('Authorization', function() {
     it('should not allow an anonymous user admin access to it', function(done) {
       expectNoAnonymousAdminAccess(this.anon, this.projectId, done)
     })
+
+    it('should not allow an anonymous user chat messages access', function(done) {
+      expectNoChatAccess(this.anon, this.projectId, done)
+    })
   })
 })

services/web/test/acceptance/src/helpers/MockChatApi.js

@@ -0,0 +1,51 @@
+const express = require('express')
+const bodyParser = require('body-parser')
+const app = express()
+
+const projects = {}
+
+const MessageHttpController = {
+  getGlobalMessages: (req, res) => {
+    res.send(projects[req.params.project_id] || [])
+  },
+  sendGlobalMessage: (req, res) => {
+    const projectId = req.params.project_id
+    const message = {
+      id: Math.random().toString(),
+      content: req.body.content,
+      timestamp: Date.now(),
+      user_id: req.body.user_id
+    }
+    projects[projectId] = projects[projectId] || []
+    projects[projectId].push(message)
+    res.sendStatus(201).send(Object.assign({ room_id: projectId }, message))
+  }
+}
+
+const MockChatApi = {
+  run() {
+    app.use(bodyParser.json())
+
+    app.get(
+      '/project/:project_id/messages',
+      MessageHttpController.getGlobalMessages
+    )
+    app.post(
+      '/project/:project_id/messages',
+      MessageHttpController.sendGlobalMessage
+    )
+
+    app
+      .listen(3010, error => {
+        if (error) {
+          throw error
+        }
+      })
+      .on('error', error => {
+        console.error('error starting MockChatApi:', error.message)
+        return process.exit(1)
+      })
+  }
+}
+
+MockChatApi.run()

services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js

@@ -440,6 +440,55 @@ describe('AuthorizationMiddleware', function() {
     })
   })
 
+  describe('blockRestrictedUserFromProject', function() {
+    beforeEach(function() {
+      this.AuthorizationMiddleware._getUserAndProjectId = sinon
+        .stub()
+        .callsArgWith(1, null, this.userId, this.project_id)
+    })
+
+    it('should issue a 401 response for a restricted user', function(done) {
+      this.AuthorizationManager.isRestrictedUserForProject = sinon
+        .stub()
+        .callsArgWith(3, null, true)
+      this.req = {}
+      this.next = sinon.stub()
+      this.res.sendStatus = status => {
+        expect(status).to.equal(403)
+        expect(
+          this.AuthorizationManager.isRestrictedUserForProject.called
+        ).to.equal(true)
+        expect(this.next.called).to.equal(false)
+        done()
+      }
+      this.AuthorizationMiddleware.blockRestrictedUserFromProject(
+        this.req,
+        this.res,
+        this.next
+      )
+    })
+
+    it('should pass through for a regular user', function(done) {
+      this.AuthorizationManager.isRestrictedUserForProject = sinon
+        .stub()
+        .callsArgWith(3, null, false)
+      this.req = {}
+      this.res.sendStatus = sinon.stub()
+      this.next = status => {
+        expect(
+          this.AuthorizationManager.isRestrictedUserForProject.called
+        ).to.equal(true)
+        expect(this.res.sendStatus.called).to.equal(false)
+        done()
+      }
+      this.AuthorizationMiddleware.blockRestrictedUserFromProject(
+        this.req,
+        this.res,
+        this.next
+      )
+    })
+  })
+
   describe('ensureUserCanReadMultipleProjects', function() {
     beforeEach(function() {
       this.AuthorizationManager.canUserReadProject = sinon.stub()