Merge pull request #2176 from overleaf/em-ta-json-auth

Make ensureUserCanAdminProject always raise a 403

GitOrigin-RevId: 4dd1eca1cfb171d92392bc3c8208b61cbf7c6815

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

@@ -4,6 +4,7 @@ const async = require('async')
 const logger = require('logger-sharelatex')
 const { ObjectId } = require('mongojs')
 const Errors = require('../Errors/Errors')
+const HttpErrors = require('@overleaf/o-error/http')
 const AuthenticationController = require('../Authentication/AuthenticationController')
 const TokenAccessHandler = require('../TokenAccess/TokenAccessHandler')
 
@@ -180,7 +181,7 @@ module.exports = AuthorizationMiddleware = {
             { userId, projectId },
             'denying user admin access to project'
           )
-          AuthorizationMiddleware.redirectToRestricted(req, res, next)
+          next(new HttpErrors.ForbiddenError({}))
         }
       )
     })

services/web/app/src/Features/Collaborators/CollaboratorsRouter.js

@@ -25,6 +25,7 @@ module.exports = {
 
     webRouter.delete(
       '/project/:Project_id/users/:user_id',
+      AuthenticationController.requireLogin(),
       AuthorizationMiddleware.ensureUserCanAdminProject,
       CollaboratorsController.removeUserFromProject
     )

services/web/app/src/Features/Errors/HttpErrorController.js

@@ -8,7 +8,7 @@ function renderHTMLError(statusCode, publicInfo, res) {
   if (statusCode === 404) {
     res.render('general/404', { title: 'page_not_found' })
   } else if (statusCode === 403) {
-    res.render('user/restricted')
+    res.render('user/restricted', { title: 'restricted' })
   } else if (statusCode >= 400 && statusCode < 500) {
     res.render('general/500', { title: 'Client Error' })
   } else {

services/web/app/src/router.js

@@ -308,6 +308,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
   )
   webRouter.post(
     '/project/:Project_id/settings/admin',
+    AuthenticationController.requireLogin(),
     AuthorizationMiddleware.ensureUserCanAdminProject,
     ProjectController.updateProjectAdminSettings
   )
@@ -462,11 +463,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
 
   webRouter.delete(
     '/Project/:Project_id',
+    AuthenticationController.requireLogin(),
     AuthorizationMiddleware.ensureUserCanAdminProject,
     ProjectController.deleteProject
   )
   webRouter.post(
     '/Project/:Project_id/restore',
+    AuthenticationController.requireLogin(),
     AuthorizationMiddleware.ensureUserCanAdminProject,
     ProjectController.restoreProject
   )
@@ -478,6 +481,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
 
   webRouter.post(
     '/project/:Project_id/rename',
+    AuthenticationController.requireLogin(),
     AuthorizationMiddleware.ensureUserCanAdminProject,
     ProjectController.renameProject
   )

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

@@ -1,59 +1,46 @@
-/* eslint-disable
-    camelcase,
-    max-len,
-    no-undef,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const { expect } = require('chai')
 const async = require('async')
 const User = require('./helpers/User')
 const request = require('./helpers/request')
 const settings = require('settings-sharelatex')
 
-const MockDocstoreApi = require('./helpers/MockDocstoreApi')
+require('./helpers/MockDocstoreApi')
-const MockDocUpdaterApi = require('./helpers/MockDocUpdaterApi')
+require('./helpers/MockDocUpdaterApi')
 
-const try_read_access = (user, project_id, test, callback) =>
+function tryReadAccess(user, projectId, test, callback) {
   async.series(
     [
       cb =>
-        user.request.get(`/project/${project_id}`, (error, response, body) => {
+        user.request.get(`/project/${projectId}`, (error, response, body) => {
           if (error != null) {
             return cb(error)
           }
           test(response, body)
-          return cb()
+          cb()
         }),
       cb =>
         user.request.get(
-          `/project/${project_id}/download/zip`,
+          `/project/${projectId}/download/zip`,
           (error, response, body) => {
             if (error != null) {
               return cb(error)
             }
             test(response, body)
-            return cb()
+            cb()
           }
         )
     ],
     callback
   )
+}
 
-const try_settings_write_access = (user, project_id, test, callback) =>
+function trySettingsWriteAccess(user, projectId, test, callback) {
   async.series(
     [
       cb =>
         user.request.post(
           {
-            uri: `/project/${project_id}/settings`,
+            uri: `/project/${projectId}/settings`,
             json: {
               compiler: 'latex'
             }
@@ -63,20 +50,21 @@ const try_settings_write_access = (user, project_id, test, callback) =>
               return cb(error)
             }
             test(response, body)
-            return cb()
+            cb()
           }
         )
     ],
     callback
   )
+}
 
-const try_admin_access = (user, project_id, test, callback) =>
+function tryAdminAccess(user, projectId, test, callback) {
   async.series(
     [
       cb =>
         user.request.post(
           {
-            uri: `/project/${project_id}/rename`,
+            uri: `/project/${projectId}/rename`,
             json: {
               newProjectName: 'new-name'
             }
@@ -86,13 +74,13 @@ const try_admin_access = (user, project_id, test, callback) =>
               return cb(error)
             }
             test(response, body)
-            return cb()
+            cb()
           }
         ),
       cb =>
         user.request.post(
           {
-            uri: `/project/${project_id}/settings/admin`,
+            uri: `/project/${projectId}/settings/admin`,
             json: {
               publicAccessLevel: 'private'
             }
@@ -102,26 +90,27 @@ const try_admin_access = (user, project_id, test, callback) =>
               return cb(error)
             }
             test(response, body)
-            return cb()
+            cb()
           }
         )
     ],
     callback
   )
+}
 
-const try_content_access = function(user, project_id, test, callback) {
+function tryContentAccess(user, projectId, test, callback) {
   // The real-time service calls this end point to determine the user's
   // permissions.
-  let user_id
+  let userId
   if (user.id != null) {
-    user_id = user.id
+    userId = user.id
   } else {
-    user_id = 'anonymous-user'
+    userId = 'anonymous-user'
   }
-  return request.post(
+  request.post(
     {
-      url: `/project/${project_id}/join`,
+      url: `/project/${projectId}/join`,
-      qs: { user_id },
+      qs: { user_id: userId },
       auth: {
         user: settings.apis.web.user,
         pass: settings.apis.web.pass,
@@ -135,26 +124,26 @@ const try_content_access = function(user, project_id, test, callback) {
         return callback(error)
       }
       test(response, body)
-      return callback()
+      callback()
     }
   )
 }
 
-const expect_read_access = (user, project_id, callback) =>
+function expectReadAccess(user, projectId, callback) {
   async.series(
     [
       cb =>
-        try_read_access(
+        tryReadAccess(
           user,
-          project_id,
+          projectId,
           (response, body) =>
             expect(response.statusCode).to.be.oneOf([200, 204]),
           cb
         ),
       cb =>
-        try_content_access(
+        tryContentAccess(
           user,
-          project_id,
+          projectId,
           (response, body) =>
             expect(body.privilegeLevel).to.be.oneOf([
               'owner',
@@ -166,92 +155,109 @@ const expect_read_access = (user, project_id, callback) =>
     ],
     callback
   )
+}
 
-const expect_content_write_access = (user, project_id, callback) =>
+function expectContentWriteAccess(user, projectId, callback) {
-  try_content_access(
+  tryContentAccess(
     user,
-    project_id,
+    projectId,
     (response, body) =>
       expect(body.privilegeLevel).to.be.oneOf(['owner', 'readAndWrite']),
     callback
   )
+}
 
-const expect_settings_write_access = (user, project_id, callback) =>
+function expectSettingsWriteAccess(user, projectId, callback) {
-  try_settings_write_access(
+  trySettingsWriteAccess(
     user,
-    project_id,
+    projectId,
     (response, body) => expect(response.statusCode).to.be.oneOf([200, 204]),
     callback
   )
+}
 
-const expect_admin_access = (user, project_id, callback) =>
+function expectAdminAccess(user, projectId, callback) {
-  try_admin_access(
+  tryAdminAccess(
     user,
-    project_id,
+    projectId,
     (response, body) => expect(response.statusCode).to.be.oneOf([200, 204]),
     callback
   )
+}
 
-const expect_no_read_access = (user, project_id, options, callback) =>
+function expectNoReadAccess(user, projectId, options, callback) {
   async.series(
     [
       cb =>
-        try_read_access(
+        tryReadAccess(
           user,
-          project_id,
+          projectId,
           (response, body) => {
             expect(response.statusCode).to.equal(302)
-            return expect(response.headers.location).to.match(
+            expect(response.headers.location).to.match(
               new RegExp(options.redirect_to)
             )
           },
           cb
         ),
       cb =>
-        try_content_access(
+        tryContentAccess(
           user,
-          project_id,
+          projectId,
           (response, body) => expect(body.privilegeLevel).to.be.equal(false),
           cb
         )
     ],
     callback
   )
+}
 
-const expect_no_content_write_access = (user, project_id, callback) =>
+function expectNoContentWriteAccess(user, projectId, callback) {
-  try_content_access(
+  tryContentAccess(
     user,
-    project_id,
+    projectId,
     (response, body) =>
       expect(body.privilegeLevel).to.be.oneOf([false, 'readOnly']),
     callback
   )
+}
 
-const expect_no_settings_write_access = (user, project_id, options, callback) =>
+function expectNoSettingsWriteAccess(user, projectId, options, callback) {
-  try_settings_write_access(
+  trySettingsWriteAccess(
     user,
-    project_id,
+    projectId,
     (response, body) => {
       expect(response.statusCode).to.equal(302)
-      return expect(response.headers.location).to.match(
+      expect(response.headers.location).to.match(
         new RegExp(options.redirect_to)
       )
     },
     callback
   )
+}
 
-const expect_no_admin_access = (user, project_id, options, callback) =>
+function expectNoAdminAccess(user, projectId, callback) {
-  try_admin_access(
+  tryAdminAccess(
     user,
-    project_id,
+    projectId,
     (response, body) => {
-      expect(response.statusCode).to.equal(302)
+      expect(response.statusCode).to.equal(403)
-      return expect(response.headers.location).to.match(
-        new RegExp(options.redirect_to)
-      )
     },
     callback
   )
+}
+
+function expectNoAnonymousAdminAccess(user, projectId, callback) {
+  tryAdminAccess(
+    user,
+    projectId,
+    (response, body) => {
+      expect(response.statusCode).to.equal(302)
+      expect(response.headers.location).to.match(/^\/login/)
+    },
+    callback
+  )
+}
 
 describe('Authorization', function() {
   beforeEach(function(done) {
@@ -261,18 +267,18 @@ describe('Authorization', function() {
     this.other2 = new User()
     this.anon = new User()
     this.site_admin = new User({ email: 'admin@example.com' })
-    return async.parallel(
+    async.parallel(
       [
         cb => this.owner.login(cb),
         cb => this.other1.login(cb),
         cb => this.other2.login(cb),
         cb => this.anon.getCsrfToken(cb),
         cb => {
-          return this.site_admin.login(err => {
+          this.site_admin.login(err => {
-            if (typeof error !== 'undefined' && error !== null) {
+            if (err != null) {
               return cb(err)
             }
-            return this.site_admin.ensure_admin(cb)
+            this.site_admin.ensure_admin(cb)
           })
         }
       ],
@@ -282,114 +288,97 @@ describe('Authorization', function() {
 
   describe('private project', function() {
     beforeEach(function(done) {
-      return this.owner.createProject(
+      this.owner.createProject('private-project', (error, projectId) => {
-        'private-project',
+        if (error != null) {
-        (error, project_id) => {
+          return done(error)
-          if (error != null) {
-            return done(error)
-          }
-          this.project_id = project_id
-          return done()
         }
-      )
+        this.projectId = projectId
+        done()
+      })
     })
 
     it('should allow the owner read access to it', function(done) {
-      return expect_read_access(this.owner, this.project_id, done)
+      expectReadAccess(this.owner, this.projectId, done)
     })
 
     it('should allow the owner write access to its content', function(done) {
-      return expect_content_write_access(this.owner, this.project_id, done)
+      expectContentWriteAccess(this.owner, this.projectId, done)
     })
 
     it('should allow the owner write access to its settings', function(done) {
-      return expect_settings_write_access(this.owner, this.project_id, done)
+      expectSettingsWriteAccess(this.owner, this.projectId, done)
     })
 
     it('should allow the owner admin access to it', function(done) {
-      return expect_admin_access(this.owner, this.project_id, done)
+      expectAdminAccess(this.owner, this.projectId, done)
     })
 
     it('should not allow another user read access to the project', function(done) {
-      return expect_no_read_access(
+      expectNoReadAccess(
         this.other1,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow another user write access to its content', function(done) {
-      return expect_no_content_write_access(this.other1, this.project_id, done)
+      expectNoContentWriteAccess(this.other1, this.projectId, done)
     })
 
     it('should not allow another user write access to its settings', function(done) {
-      return expect_no_settings_write_access(
+      expectNoSettingsWriteAccess(
         this.other1,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow another user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAdminAccess(this.other1, this.projectId, done)
-        this.other1,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
 
     it('should not allow anonymous user read access to it', function(done) {
-      return expect_no_read_access(
+      expectNoReadAccess(
         this.anon,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow anonymous user write access to its content', function(done) {
-      return expect_no_content_write_access(this.anon, this.project_id, done)
+      expectNoContentWriteAccess(this.anon, this.projectId, done)
     })
 
     it('should not allow anonymous user write access to its settings', function(done) {
-      return expect_no_settings_write_access(
+      expectNoSettingsWriteAccess(
         this.anon,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow anonymous user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAnonymousAdminAccess(this.anon, this.projectId, done)
-        this.anon,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
 
     it('should allow site admin users read access to it', function(done) {
-      return expect_read_access(this.site_admin, this.project_id, done)
+      expectReadAccess(this.site_admin, this.projectId, done)
     })
 
     it('should allow site admin users write access to its content', function(done) {
-      return expect_content_write_access(this.site_admin, this.project_id, done)
+      expectContentWriteAccess(this.site_admin, this.projectId, done)
     })
 
     it('should allow site admin users write access to its settings', function(done) {
-      return expect_settings_write_access(
+      expectSettingsWriteAccess(this.site_admin, this.projectId, done)
-        this.site_admin,
-        this.project_id,
-        done
-      )
     })
 
     it('should allow site admin users admin access to it', function(done) {
-      return expect_admin_access(this.site_admin, this.project_id, done)
+      expectAdminAccess(this.site_admin, this.projectId, done)
     })
   })
 
@@ -397,217 +386,178 @@ describe('Authorization', function() {
     beforeEach(function(done) {
       this.rw_user = this.other1
       this.ro_user = this.other2
-      return this.owner.createProject(
+      this.owner.createProject('private-project', (error, projectId) => {
-        'private-project',
+        if (error != null) {
-        (error, project_id) => {
+          return done(error)
-          if (error != null) {
-            return done(error)
-          }
-          this.project_id = project_id
-          return this.owner.addUserToProject(
-            this.project_id,
-            this.ro_user,
-            'readOnly',
-            error => {
-              if (error != null) {
-                return done(error)
-              }
-              return this.owner.addUserToProject(
-                this.project_id,
-                this.rw_user,
-                'readAndWrite',
-                error => {
-                  if (error != null) {
-                    return done(error)
-                  }
-                  return done()
-                }
-              )
-            }
-          )
         }
-      )
+        this.projectId = projectId
+        this.owner.addUserToProject(
+          this.projectId,
+          this.ro_user,
+          'readOnly',
+          error => {
+            if (error != null) {
+              return done(error)
+            }
+            this.owner.addUserToProject(
+              this.projectId,
+              this.rw_user,
+              'readAndWrite',
+              error => {
+                if (error != null) {
+                  return done(error)
+                }
+                done()
+              }
+            )
+          }
+        )
+      })
     })
 
     it('should allow the read-only user read access to it', function(done) {
-      return expect_read_access(this.ro_user, this.project_id, done)
+      expectReadAccess(this.ro_user, this.projectId, done)
     })
 
     it('should not allow the read-only user write access to its content', function(done) {
-      return expect_no_content_write_access(this.ro_user, this.project_id, done)
+      expectNoContentWriteAccess(this.ro_user, this.projectId, done)
     })
 
     it('should not allow the read-only user write access to its settings', function(done) {
-      return expect_no_settings_write_access(
+      expectNoSettingsWriteAccess(
         this.ro_user,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow the read-only user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAdminAccess(this.ro_user, this.projectId, done)
-        this.ro_user,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
 
     it('should allow the read-write user read access to it', function(done) {
-      return expect_read_access(this.rw_user, this.project_id, done)
+      expectReadAccess(this.rw_user, this.projectId, done)
     })
 
     it('should allow the read-write user write access to its content', function(done) {
-      return expect_content_write_access(this.rw_user, this.project_id, done)
+      expectContentWriteAccess(this.rw_user, this.projectId, done)
     })
 
     it('should allow the read-write user write access to its settings', function(done) {
-      return expect_settings_write_access(this.rw_user, this.project_id, done)
+      expectSettingsWriteAccess(this.rw_user, this.projectId, done)
     })
 
     it('should not allow the read-write user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAdminAccess(this.rw_user, this.projectId, done)
-        this.rw_user,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
   })
 
   describe('public read-write project', function() {
     beforeEach(function(done) {
-      return this.owner.createProject(
+      this.owner.createProject('public-rw-project', (error, projectId) => {
-        'public-rw-project',
+        if (error != null) {
-        (error, project_id) => {
+          return done(error)
-          if (error != null) {
-            return done(error)
-          }
-          this.project_id = project_id
-          return this.owner.makePublic(this.project_id, 'readAndWrite', done)
         }
-      )
+        this.projectId = projectId
+        this.owner.makePublic(this.projectId, 'readAndWrite', done)
+      })
     })
 
     it('should allow a user read access to it', function(done) {
-      return expect_read_access(this.other1, this.project_id, done)
+      expectReadAccess(this.other1, this.projectId, done)
     })
 
     it('should allow a user write access to its content', function(done) {
-      return expect_content_write_access(this.other1, this.project_id, done)
+      expectContentWriteAccess(this.other1, this.projectId, done)
     })
 
     it('should not allow a user write access to its settings', function(done) {
-      return expect_no_settings_write_access(
+      expectNoSettingsWriteAccess(
         this.other1,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow a user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAdminAccess(this.other1, this.projectId, done)
-        this.other1,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
 
     it('should allow an anonymous user read access to it', function(done) {
-      return expect_read_access(this.anon, this.project_id, done)
+      expectReadAccess(this.anon, this.projectId, done)
     })
 
     it('should allow an anonymous user write access to its content', function(done) {
-      return expect_content_write_access(this.anon, this.project_id, done)
+      expectContentWriteAccess(this.anon, this.projectId, done)
     })
 
     it('should not allow an anonymous user write access to its settings', function(done) {
-      return expect_no_settings_write_access(
+      expectNoSettingsWriteAccess(
         this.anon,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow an anonymous user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAnonymousAdminAccess(this.anon, this.projectId, done)
-        this.anon,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
   })
 
   describe('public read-only project', function() {
     beforeEach(function(done) {
-      return this.owner.createProject(
+      this.owner.createProject('public-ro-project', (error, projectId) => {
-        'public-ro-project',
+        if (error != null) {
-        (error, project_id) => {
+          return done(error)
-          if (error != null) {
-            return done(error)
-          }
-          this.project_id = project_id
-          return this.owner.makePublic(this.project_id, 'readOnly', done)
         }
-      )
+        this.projectId = projectId
+        this.owner.makePublic(this.projectId, 'readOnly', done)
+      })
     })
 
     it('should allow a user read access to it', function(done) {
-      return expect_read_access(this.other1, this.project_id, done)
+      expectReadAccess(this.other1, this.projectId, done)
     })
 
     it('should not allow a user write access to its content', function(done) {
-      return expect_no_content_write_access(this.other1, this.project_id, done)
+      expectNoContentWriteAccess(this.other1, this.projectId, done)
     })
 
     it('should not allow a user write access to its settings', function(done) {
-      return expect_no_settings_write_access(
+      expectNoSettingsWriteAccess(
         this.other1,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow a user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAdminAccess(this.other1, this.projectId, done)
-        this.other1,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
 
     it('should allow an anonymous user read access to it', function(done) {
-      return expect_read_access(this.anon, this.project_id, done)
+      expectReadAccess(this.anon, this.projectId, done)
     })
 
     it('should not allow an anonymous user write access to its content', function(done) {
-      return expect_no_content_write_access(this.anon, this.project_id, done)
+      expectNoContentWriteAccess(this.anon, this.projectId, done)
     })
 
     it('should not allow an anonymous user write access to its settings', function(done) {
-      return expect_no_settings_write_access(
+      expectNoSettingsWriteAccess(
         this.anon,
-        this.project_id,
+        this.projectId,
         { redirect_to: '/restricted' },
         done
       )
     })
 
     it('should not allow an anonymous user admin access to it', function(done) {
-      return expect_no_admin_access(
+      expectNoAnonymousAdminAccess(this.anon, this.projectId, done)
-        this.anon,
-        this.project_id,
-        { redirect_to: '/restricted' },
-        done
-      )
     })
   })
 })

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

@@ -1,76 +1,68 @@
-/* eslint-disable
-    camelcase,
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const sinon = require('sinon')
-const chai = require('chai')
+const { expect } = require('chai')
-const should = chai.should()
-const { expect } = chai
-const modulePath =
-  '../../../../app/src/Features/Authorization/AuthorizationMiddleware.js'
 const SandboxedModule = require('sandboxed-module')
+const HttpErrors = require('@overleaf/o-error/http')
 const Errors = require('../../../../app/src/Features/Errors/Errors.js')
 
+const MODULE_PATH =
+  '../../../../app/src/Features/Authorization/AuthorizationMiddleware.js'
+
 describe('AuthorizationMiddleware', function() {
   beforeEach(function() {
-    this.user_id = 'user-id-123'
+    this.userId = 'user-id-123'
     this.project_id = 'project-id-123'
     this.token = 'some-token'
     this.AuthenticationController = {
-      getLoggedInUserId: sinon.stub().returns(this.user_id),
+      getLoggedInUserId: sinon.stub().returns(this.userId),
       isUserLoggedIn: sinon.stub().returns(true)
     }
-    this.AuthorizationMiddleware = SandboxedModule.require(modulePath, {
+    this.AuthorizationManager = {}
+    this.TokenAccessHandler = {
+      getRequestToken: sinon.stub().returns(this.token)
+    }
+    this.ObjectId = {
+      isValid: sinon
+        .stub()
+        .withArgs(this.project_id)
+        .returns(true)
+    }
+    this.AuthorizationManager = {}
+    this.AuthorizationMiddleware = SandboxedModule.require(MODULE_PATH, {
       globals: {
         console: console
       },
       requires: {
-        './AuthorizationManager': (this.AuthorizationManager = {}),
+        './AuthorizationManager': this.AuthorizationManager,
         'logger-sharelatex': { log() {} },
         mongojs: {
-          ObjectId: (this.ObjectId = {})
+          ObjectId: this.ObjectId
         },
+        '@overleaf/o-error/http': HttpErrors,
         '../Errors/Errors': Errors,
         '../Authentication/AuthenticationController': this
           .AuthenticationController,
-        '../TokenAccess/TokenAccessHandler': (this.TokenAccessHandler = {
+        '../TokenAccess/TokenAccessHandler': this.TokenAccessHandler
-          getRequestToken: sinon.stub().returns(this.token)
-        })
       }
     })
     this.req = {}
     this.res = {}
-    this.ObjectId.isValid = sinon.stub()
+    this.next = sinon.stub()
-    this.ObjectId.isValid.withArgs(this.project_id).returns(true)
-    return (this.next = sinon.stub())
   })
 
   describe('_getUserId', function() {
     beforeEach(function() {
-      return (this.req = {})
+      this.req = {}
     })
 
     it('should get the user from session', function(done) {
       this.AuthenticationController.getLoggedInUserId = sinon
         .stub()
         .returns('1234')
-      return this.AuthorizationMiddleware._getUserId(
+      this.AuthorizationMiddleware._getUserId(this.req, (err, userId) => {
-        this.req,
+        expect(err).to.not.exist
-        (err, user_id) => {
+        expect(userId).to.equal('1234')
-          expect(err).to.not.exist
+        done()
-          expect(user_id).to.equal('1234')
+      })
-          return done()
-        }
-      )
     })
 
     it('should get oauth_user from request', function(done) {
@@ -78,14 +70,11 @@ describe('AuthorizationMiddleware', function() {
         .stub()
         .returns(null)
       this.req.oauth_user = { _id: '5678' }
-      return this.AuthorizationMiddleware._getUserId(
+      this.AuthorizationMiddleware._getUserId(this.req, (err, userId) => {
-        this.req,
+        expect(err).to.not.exist
-        (err, user_id) => {
+        expect(userId).to.equal('5678')
-          expect(err).to.not.exist
+        done()
-          expect(user_id).to.equal('5678')
+      })
-          return done()
-        }
-      )
     })
 
     it('should fall back to null', function(done) {
@@ -93,36 +82,31 @@ describe('AuthorizationMiddleware', function() {
         .stub()
         .returns(null)
       this.req.oauth_user = undefined
-      return this.AuthorizationMiddleware._getUserId(
+      this.AuthorizationMiddleware._getUserId(this.req, (err, userId) => {
-        this.req,
+        expect(err).to.not.exist
-        (err, user_id) => {
+        expect(userId).to.equal(null)
-          expect(err).to.not.exist
+        done()
-          expect(user_id).to.equal(null)
+      })
-          return done()
-        }
-      )
     })
   })
 
   const METHODS_TO_TEST = {
     ensureUserCanReadProject: 'canUserReadProject',
     ensureUserCanWriteProjectSettings: 'canUserWriteProjectSettings',
-    ensureUserCanWriteProjectContent: 'canUserWriteProjectContent',
+    ensureUserCanWriteProjectContent: 'canUserWriteProjectContent'
-    ensureUserCanAdminProject: 'canUserAdminProject'
   }
-  for (let middlewareMethod in METHODS_TO_TEST) {
+  Object.entries(METHODS_TO_TEST).forEach(
-    const managerMethod = METHODS_TO_TEST[middlewareMethod]
+    ([middlewareMethod, managerMethod]) => {
-    ;((middlewareMethod, managerMethod) =>
       describe(middlewareMethod, function() {
         beforeEach(function() {
           this.req.params = { project_id: this.project_id }
           this.AuthorizationManager[managerMethod] = sinon.stub()
-          return (this.AuthorizationMiddleware.redirectToRestricted = sinon.stub())
+          this.AuthorizationMiddleware.redirectToRestricted = sinon.stub()
         })
 
         describe('with missing project_id', function() {
           beforeEach(function() {
-            return (this.req.params = {})
+            this.req.params = {}
           })
 
           it('should return an error to next', function() {
@@ -131,21 +115,19 @@ describe('AuthorizationMiddleware', function() {
               this.res,
               this.next
             )
-            return this.next.calledWith(new Error()).should.equal(true)
+            this.next.calledWith(new Error()).should.equal(true)
           })
         })
 
         describe('with logged in user', function() {
           beforeEach(function() {
-            return this.AuthenticationController.getLoggedInUserId.returns(
+            this.AuthenticationController.getLoggedInUserId.returns(this.userId)
-              this.user_id
-            )
           })
 
           describe('when user has permission', function() {
             beforeEach(function() {
-              return this.AuthorizationManager[managerMethod]
+              this.AuthorizationManager[managerMethod]
-                .withArgs(this.user_id, this.project_id, this.token)
+                .withArgs(this.userId, this.project_id, this.token)
                 .yields(null, true)
             })
 
@@ -155,25 +137,25 @@ describe('AuthorizationMiddleware', function() {
                 this.res,
                 this.next
               )
-              return this.next.called.should.equal(true)
+              this.next.called.should.equal(true)
             })
           })
 
           describe("when user doesn't have permission", function() {
             beforeEach(function() {
-              return this.AuthorizationManager[managerMethod]
+              this.AuthorizationManager[managerMethod]
-                .withArgs(this.user_id, this.project_id, this.token)
+                .withArgs(this.userId, this.project_id, this.token)
                 .yields(null, false)
             })
 
-            it('should redirect to redirectToRestricted', function() {
+            it('should raise a 403', function() {
               this.AuthorizationMiddleware[middlewareMethod](
                 this.req,
                 this.res,
                 this.next
               )
               this.next.called.should.equal(false)
-              return this.AuthorizationMiddleware.redirectToRestricted
+              this.AuthorizationMiddleware.redirectToRestricted
                 .calledWith(this.req, this.res, this.next)
                 .should.equal(true)
             })
@@ -184,7 +166,7 @@ describe('AuthorizationMiddleware', function() {
           describe('when user has permission', function() {
             beforeEach(function() {
               this.AuthenticationController.getLoggedInUserId.returns(null)
-              return this.AuthorizationManager[managerMethod]
+              this.AuthorizationManager[managerMethod]
                 .withArgs(null, this.project_id, this.token)
                 .yields(null, true)
             })
@@ -195,14 +177,14 @@ describe('AuthorizationMiddleware', function() {
                 this.res,
                 this.next
               )
-              return this.next.called.should.equal(true)
+              this.next.called.should.equal(true)
             })
           })
 
           describe("when user doesn't have permission", function() {
             beforeEach(function() {
               this.AuthenticationController.getLoggedInUserId.returns(null)
-              return this.AuthorizationManager[managerMethod]
+              this.AuthorizationManager[managerMethod]
                 .withArgs(null, this.project_id, this.token)
                 .yields(null, false)
             })
@@ -214,7 +196,7 @@ describe('AuthorizationMiddleware', function() {
                 this.next
               )
               this.next.called.should.equal(false)
-              return this.AuthorizationMiddleware.redirectToRestricted
+              this.AuthorizationMiddleware.redirectToRestricted
                 .calledWith(this.req, this.res, this.next)
                 .should.equal(true)
             })
@@ -224,11 +206,11 @@ describe('AuthorizationMiddleware', function() {
         describe('with malformed project id', function() {
           beforeEach(function() {
             this.req.params = { project_id: 'blah' }
-            return (this.ObjectId.isValid = sinon.stub().returns(false))
+            this.ObjectId.isValid = sinon.stub().returns(false)
           })
 
           it('should return a not found error', function(done) {
-            return this.AuthorizationMiddleware[middlewareMethod](
+            this.AuthorizationMiddleware[middlewareMethod](
               this.req,
               this.res,
               error => {
@@ -238,26 +220,148 @@ describe('AuthorizationMiddleware', function() {
             )
           })
         })
-      }))(middlewareMethod, managerMethod)
+      })
-  }
+    }
+  )
 
-  describe('ensureUserIsSiteAdmin', function() {
+  describe('ensureUserCanAdminProject', function() {
     beforeEach(function() {
-      this.AuthorizationManager.isUserSiteAdmin = sinon.stub()
+      this.req.params = { project_id: this.project_id }
-      return (this.AuthorizationMiddleware.redirectToRestricted = sinon.stub())
+      this.AuthorizationManager.canUserAdminProject = sinon.stub()
+      this.AuthorizationMiddleware.redirectToRestricted = sinon.stub()
+    })
+
+    describe('with missing project_id', function() {
+      beforeEach(function() {
+        this.req.params = {}
+      })
+
+      it('should return an error to next', function() {
+        this.AuthorizationMiddleware.ensureUserCanAdminProject(
+          this.req,
+          this.res,
+          this.next
+        )
+        this.next.calledWith(new Error()).should.equal(true)
+      })
     })
 
     describe('with logged in user', function() {
       beforeEach(function() {
-        return this.AuthenticationController.getLoggedInUserId.returns(
+        this.AuthenticationController.getLoggedInUserId.returns(this.userId)
-          this.user_id
-        )
       })
 
       describe('when user has permission', function() {
         beforeEach(function() {
-          return this.AuthorizationManager.isUserSiteAdmin
+          this.AuthorizationManager.canUserAdminProject
-            .withArgs(this.user_id)
+            .withArgs(this.userId, this.project_id, this.token)
+            .yields(null, true)
+        })
+
+        it('should return next', function() {
+          this.AuthorizationMiddleware.ensureUserCanAdminProject(
+            this.req,
+            this.res,
+            this.next
+          )
+          this.next.called.should.equal(true)
+        })
+      })
+
+      describe("when user doesn't have permission", function() {
+        beforeEach(function() {
+          this.AuthorizationManager.canUserAdminProject
+            .withArgs(this.userId, this.project_id, this.token)
+            .yields(null, false)
+        })
+
+        it('should raise a 403', function(done) {
+          this.AuthorizationMiddleware.ensureUserCanAdminProject(
+            this.req,
+            this.res,
+            err => {
+              expect(err).to.be.an.instanceof(HttpErrors.ForbiddenError)
+              done()
+            }
+          )
+        })
+      })
+    })
+
+    describe('with anonymous user', function() {
+      describe('when user has permission', function() {
+        beforeEach(function() {
+          this.AuthenticationController.getLoggedInUserId.returns(null)
+          this.AuthorizationManager.canUserAdminProject
+            .withArgs(null, this.project_id, this.token)
+            .yields(null, true)
+        })
+
+        it('should return next', function() {
+          this.AuthorizationMiddleware.ensureUserCanAdminProject(
+            this.req,
+            this.res,
+            this.next
+          )
+          this.next.called.should.equal(true)
+        })
+      })
+
+      describe("when user doesn't have permission", function() {
+        beforeEach(function() {
+          this.AuthenticationController.getLoggedInUserId.returns(null)
+          this.AuthorizationManager.canUserAdminProject
+            .withArgs(null, this.project_id, this.token)
+            .yields(null, false)
+        })
+
+        it('should raise a 403', function(done) {
+          this.AuthorizationMiddleware.ensureUserCanAdminProject(
+            this.req,
+            this.res,
+            err => {
+              expect(err).to.be.an.instanceof(HttpErrors.ForbiddenError)
+              done()
+            }
+          )
+        })
+      })
+    })
+
+    describe('with malformed project id', function() {
+      beforeEach(function() {
+        this.req.params = { project_id: 'blah' }
+        this.ObjectId.isValid = sinon.stub().returns(false)
+      })
+
+      it('should return a not found error', function(done) {
+        this.AuthorizationMiddleware.ensureUserCanAdminProject(
+          this.req,
+          this.res,
+          error => {
+            error.should.be.instanceof(Errors.NotFoundError)
+            return done()
+          }
+        )
+      })
+    })
+  })
+
+  describe('ensureUserIsSiteAdmin', function() {
+    beforeEach(function() {
+      this.AuthorizationManager.isUserSiteAdmin = sinon.stub()
+      this.AuthorizationMiddleware.redirectToRestricted = sinon.stub()
+    })
+
+    describe('with logged in user', function() {
+      beforeEach(function() {
+        this.AuthenticationController.getLoggedInUserId.returns(this.userId)
+      })
+
+      describe('when user has permission', function() {
+        beforeEach(function() {
+          this.AuthorizationManager.isUserSiteAdmin
+            .withArgs(this.userId)
             .yields(null, true)
         })
 
@@ -267,14 +371,14 @@ describe('AuthorizationMiddleware', function() {
             this.res,
             this.next
           )
-          return this.next.called.should.equal(true)
+          this.next.called.should.equal(true)
         })
       })
 
       describe("when user doesn't have permission", function() {
         beforeEach(function() {
-          return this.AuthorizationManager.isUserSiteAdmin
+          this.AuthorizationManager.isUserSiteAdmin
-            .withArgs(this.user_id)
+            .withArgs(this.userId)
             .yields(null, false)
         })
 
@@ -285,7 +389,7 @@ describe('AuthorizationMiddleware', function() {
             this.next
           )
           this.next.called.should.equal(false)
-          return this.AuthorizationMiddleware.redirectToRestricted
+          this.AuthorizationMiddleware.redirectToRestricted
             .calledWith(this.req, this.res, this.next)
             .should.equal(true)
         })
@@ -296,7 +400,7 @@ describe('AuthorizationMiddleware', function() {
       describe('when user has permission', function() {
         beforeEach(function() {
           this.AuthenticationController.getLoggedInUserId.returns(null)
-          return this.AuthorizationManager.isUserSiteAdmin
+          this.AuthorizationManager.isUserSiteAdmin
             .withArgs(null)
             .yields(null, true)
         })
@@ -307,14 +411,14 @@ describe('AuthorizationMiddleware', function() {
             this.res,
             this.next
           )
-          return this.next.called.should.equal(true)
+          this.next.called.should.equal(true)
         })
       })
 
       describe("when user doesn't have permission", function() {
         beforeEach(function() {
           this.AuthenticationController.getLoggedInUserId.returns(null)
-          return this.AuthorizationManager.isUserSiteAdmin
+          this.AuthorizationManager.isUserSiteAdmin
             .withArgs(null)
             .yields(null, false)
         })
@@ -326,7 +430,7 @@ describe('AuthorizationMiddleware', function() {
             this.next
           )
           this.next.called.should.equal(false)
-          return this.AuthorizationMiddleware.redirectToRestricted
+          this.AuthorizationMiddleware.redirectToRestricted
             .calledWith(this.req, this.res, this.next)
             .should.equal(true)
         })
@@ -338,23 +442,21 @@ describe('AuthorizationMiddleware', function() {
     beforeEach(function() {
       this.AuthorizationManager.canUserReadProject = sinon.stub()
       this.AuthorizationMiddleware.redirectToRestricted = sinon.stub()
-      return (this.req.query = { project_ids: 'project1,project2' })
+      this.req.query = { project_ids: 'project1,project2' }
     })
 
     describe('with logged in user', function() {
       beforeEach(function() {
-        return this.AuthenticationController.getLoggedInUserId.returns(
+        this.AuthenticationController.getLoggedInUserId.returns(this.userId)
-          this.user_id
-        )
       })
 
       describe('when user has permission to access all projects', function() {
         beforeEach(function() {
           this.AuthorizationManager.canUserReadProject
-            .withArgs(this.user_id, 'project1', this.token)
+            .withArgs(this.userId, 'project1', this.token)
             .yields(null, true)
-          return this.AuthorizationManager.canUserReadProject
+          this.AuthorizationManager.canUserReadProject
-            .withArgs(this.user_id, 'project2', this.token)
+            .withArgs(this.userId, 'project2', this.token)
             .yields(null, true)
         })
 
@@ -364,17 +466,17 @@ describe('AuthorizationMiddleware', function() {
             this.res,
             this.next
           )
-          return this.next.called.should.equal(true)
+          this.next.called.should.equal(true)
         })
       })
 
       describe("when user doesn't have permission to access one of the projects", function() {
         beforeEach(function() {
           this.AuthorizationManager.canUserReadProject
-            .withArgs(this.user_id, 'project1', this.token)
+            .withArgs(this.userId, 'project1', this.token)
             .yields(null, true)
-          return this.AuthorizationManager.canUserReadProject
+          this.AuthorizationManager.canUserReadProject
-            .withArgs(this.user_id, 'project2', this.token)
+            .withArgs(this.userId, 'project2', this.token)
             .yields(null, false)
         })
 
@@ -385,7 +487,7 @@ describe('AuthorizationMiddleware', function() {
             this.next
           )
           this.next.called.should.equal(false)
-          return this.AuthorizationMiddleware.redirectToRestricted
+          this.AuthorizationMiddleware.redirectToRestricted
             .calledWith(this.req, this.res, this.next)
             .should.equal(true)
         })
@@ -400,7 +502,7 @@ describe('AuthorizationMiddleware', function() {
             this.AuthorizationManager.canUserReadProject
               .withArgs(null, 'project1', this.token)
               .yields(null, true)
-            return this.AuthorizationManager.canUserReadProject
+            this.AuthorizationManager.canUserReadProject
               .withArgs(null, 'project2', this.token)
               .yields(null, true)
           })
@@ -411,7 +513,7 @@ describe('AuthorizationMiddleware', function() {
               this.res,
               this.next
             )
-            return this.next.called.should.equal(true)
+            this.next.called.should.equal(true)
           })
         })
 
@@ -421,7 +523,7 @@ describe('AuthorizationMiddleware', function() {
             this.AuthorizationManager.canUserReadProject
               .withArgs(null, 'project1', this.token)
               .yields(null, true)
-            return this.AuthorizationManager.canUserReadProject
+            this.AuthorizationManager.canUserReadProject
               .withArgs(null, 'project2', this.token)
               .yields(null, false)
           })
@@ -433,7 +535,7 @@ describe('AuthorizationMiddleware', function() {
               this.next
             )
             this.next.called.should.equal(false)
-            return this.AuthorizationMiddleware.redirectToRestricted
+            this.AuthorizationMiddleware.redirectToRestricted
               .calledWith(this.req, this.res, this.next)
               .should.equal(true)
           })