Merge pull request #3053 from overleaf/jpa-spd-accepts

[misc] reland 3004: unify detection of json requests and skip issuing of redirects

GitOrigin-RevId: fa43b3b4d23deb581496ed70ae8f28b805555d64

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -17,6 +17,14 @@ const UrlHelper = require('../Helpers/UrlHelper')
 const AsyncFormHelper = require('../Helpers/AsyncFormHelper')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
 const _ = require('lodash')
+const {
+  acceptsJson
+} = require('../../infrastructure/RequestContentTypeDetection')
+
+function send401WithChallenge(res) {
+  res.setHeader('WWW-Authenticate', 'OverleafLogin')
+  res.sendStatus(401)
+}
 
 const AuthenticationController = (module.exports = {
   serializeUser(user, callback) {
@@ -213,6 +221,7 @@ const AuthenticationController = (module.exports = {
         next = function() {}
       }
       if (!AuthenticationController.isUserLoggedIn(req)) {
+        if (acceptsJson(req)) return send401WithChallenge(res)
         return AuthenticationController._redirectToLoginOrRegisterPage(req, res)
       } else {
         req.user = AuthenticationController.getSessionUser(req)
@@ -272,6 +281,7 @@ const AuthenticationController = (module.exports = {
           // need to destroy the existing session and generate a new one
           // otherwise they will already be logged in when they are redirected
           // to the login page
+          if (acceptsJson(req)) return send401WithChallenge(res)
           AuthenticationController._redirectToLoginOrRegisterPage(req, res)
         })
       } else {
@@ -303,6 +313,7 @@ const AuthenticationController = (module.exports = {
         { url: req.url },
         'user trying to access endpoint not in global whitelist'
       )
+      if (acceptsJson(req)) return send401WithChallenge(res)
       AuthenticationController.setRedirectInSession(req)
       res.redirect('/login')
     }

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

@@ -99,11 +99,7 @@ module.exports = AuthorizationMiddleware = {
             { userId, projectId },
             'denying user read access to project'
           )
-          const acceptHeader = req.headers && req.headers['accept']
-          if (acceptHeader && acceptHeader.match(/^application\/json.*$/)) {
-            return res.sendStatus(403)
-          }
-          AuthorizationMiddleware.redirectToRestricted(req, res, next)
+          HttpErrorHandler.forbidden(req, res)
         }
       )
     })
@@ -138,7 +134,7 @@ module.exports = AuthorizationMiddleware = {
             { userId, projectId },
             'denying user write access to project settings'
           )
-          AuthorizationMiddleware.redirectToRestricted(req, res, next)
+          HttpErrorHandler.forbidden(req, res)
         }
       )
     })
@@ -173,7 +169,7 @@ module.exports = AuthorizationMiddleware = {
             { userId, projectId },
             'denying user write access to project settings'
           )
-          AuthorizationMiddleware.redirectToRestricted(req, res, next)
+          HttpErrorHandler.forbidden(req, res)
         }
       )
     })

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

@@ -1,4 +1,5 @@
 const logger = require('logger-sharelatex')
+const Settings = require('settings-sharelatex')
 
 function renderJSONError(res, message, info) {
   const fullInfo = { ...info, message }
@@ -92,5 +93,21 @@ module.exports = {
       default:
         return res.send('internal server error')
     }
+  },
+
+  maintenance(req, res) {
+    res.status(503)
+    let message = `${Settings.appName} is currently down for maintenance.`
+    if (Settings.statusPageUrl) {
+      message += ` Please check https://${Settings.statusPageUrl} for updates.`
+    }
+    switch (req.accepts(['html', 'json'])) {
+      case 'html':
+        return res.render('general/closed', { title: 'maintenance' })
+      case 'json':
+        return renderJSONError(res, message, {})
+      default:
+        return res.send(message)
+    }
   }
 }

services/web/app/src/Features/Helpers/AsyncFormHelper.js

@@ -1,4 +1,6 @@
-const _ = require('lodash')
+const {
+  acceptsJson
+} = require('../../infrastructure/RequestContentTypeDetection')
 
 module.exports = {
   redirect
@@ -7,7 +9,7 @@ module.exports = {
 // redirect the request via headers or JSON response depending on the request
 // format
 function redirect(req, res, redir) {
-  if (_.get(req, ['headers', 'accept'], '').match(/^application\/json.*$/)) {
+  if (acceptsJson(req)) {
     res.json({ redir })
   } else {
     res.redirect(redir)

services/web/app/src/infrastructure/RequestContentTypeDetection.js

@@ -0,0 +1,5 @@
+module.exports = {
+  acceptsJson(req) {
+    return req.accepts(['html', 'json']) === 'json'
+  }
+}

services/web/app/src/infrastructure/Server.js

@@ -34,6 +34,7 @@ const Views = require('./Views')
 
 const ErrorController = require('../Features/Errors/ErrorController')
 const HttpErrorController = require('../Features/Errors/HttpErrorController')
+const HttpErrorHandler = require('../Features/Errors/HttpErrorHandler')
 const UserSessionsManager = require('../Features/User/UserSessionsManager')
 const AuthenticationController = require('../Features/Authentication/AuthenticationController')
 
@@ -185,8 +186,7 @@ webRouter.use(function(req, res, next) {
   ) {
     next()
   } else {
-    res.status(503)
-    res.render('general/closed', { title: 'maintenance' })
+    HttpErrorHandler.maintenance(req, res)
   }
 })
 
@@ -196,8 +196,7 @@ webRouter.use(function(req, res, next) {
   } else if (req.url.indexOf('/admin') === 0) {
     next()
   } else {
-    res.status(503)
-    res.render('general/closed', { title: 'maintenance' })
+    HttpErrorHandler.maintenance(req, res)
   }
 })
 

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

@@ -8,6 +8,8 @@ require('./helpers/MockChatApi')
 require('./helpers/MockDocstoreApi')
 require('./helpers/MockDocUpdaterApi')
 
+const expectErrorResponse = require('./helpers/expectErrorResponse')
+
 function tryReadAccess(user, projectId, test, callback) {
   async.series(
     [
@@ -190,17 +192,7 @@ function expectNoReadAccess(user, projectId, options, callback) {
   async.series(
     [
       cb =>
-        tryReadAccess(
-          user,
-          projectId,
-          (response, body) => {
-            expect(response.statusCode).to.equal(302)
-            expect(response.headers.location).to.match(
-              new RegExp(options.redirect_to)
-            )
-          },
-          cb
-        ),
+        tryReadAccess(user, projectId, expectErrorResponse.restricted.html, cb),
       cb =>
         tryContentAccess(
           user,
@@ -230,12 +222,7 @@ function expectNoSettingsWriteAccess(user, projectId, options, callback) {
   trySettingsWriteAccess(
     user,
     projectId,
-    (response, body) => {
-      expect(response.statusCode).to.equal(302)
-      expect(response.headers.location).to.match(
-        new RegExp(options.redirect_to)
-      )
-    },
+    expectErrorResponse.restricted.json,
     callback
   )
 }
@@ -255,10 +242,7 @@ function expectNoAnonymousAdminAccess(user, projectId, callback) {
   tryAdminAccess(
     user,
     projectId,
-    (response, body) => {
-      expect(response.statusCode).to.equal(302)
-      expect(response.headers.location).to.match(/^\/login/)
-    },
+    expectErrorResponse.requireLogin.json,
     callback
   )
 }

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

@@ -34,9 +34,19 @@ describe('siteIsOpen', function() {
     })
 
     it('should return maintenance page', function(done) {
-      return request.get('/login', (error, response) => {
+      request.get('/login', (error, response, body) => {
         response.statusCode.should.equal(503)
-        return done()
+        body.should.match(/is currently down for maintenance/)
+        done()
+      })
+    })
+
+    it('should return a plain text message for a json request', function(done) {
+      request.get('/some/route', { json: true }, (error, response, body) => {
+        response.statusCode.should.equal(503)
+        body.message.should.match(/maintenance/)
+        body.message.should.match(/status.overleaf.com/)
+        done()
       })
     })
   })

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

@@ -4,6 +4,8 @@ const { expect } = require('chai')
 const _ = require('lodash')
 const request = require('./helpers/request')
 
+const expectErrorResponse = require('./helpers/expectErrorResponse')
+
 const _initUser = (user, callback) => {
   async.series([cb => user.login(cb), cb => user.getCsrfToken(cb)], callback)
 }
@@ -77,8 +79,7 @@ describe('Tags', function() {
         }
         _getTags(this.user, (err, response, body) => {
           expect(err).to.not.exist
-          expect(response.statusCode).to.equal(302)
-          expect(body).to.not.exist
+          expectErrorResponse.requireLogin.json(response, body)
           done()
         })
       })

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

@@ -6,6 +6,8 @@ const request = require('./helpers/request')
 const settings = require('settings-sharelatex')
 const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs')
 
+const expectErrorResponse = require('./helpers/expectErrorResponse')
+
 const tryEditorAccess = (user, projectId, test, callback) =>
   async.series(
     [
@@ -210,10 +212,7 @@ describe('TokenAccess', function() {
             tryEditorAccess(
               this.other1,
               this.projectId,
-              (response, body) => {
-                expect(response.statusCode).to.equal(302)
-                expect(body).to.match(/.*\/restricted.*/)
-              },
+              expectErrorResponse.restricted.html,
               cb
             )
           },
@@ -267,10 +266,7 @@ describe('TokenAccess', function() {
             tryEditorAccess(
               this.other1,
               this.projectId,
-              (response, body) => {
-                expect(response.statusCode).to.equal(302)
-                expect(body).to.match(/.*\/restricted.*/)
-              },
+              expectErrorResponse.restricted.html,
               cb
             )
           },
@@ -371,10 +367,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.other1,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               ),
             // token goes nowhere
@@ -395,10 +388,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.other1,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               ),
             cb =>
@@ -450,10 +440,7 @@ describe('TokenAccess', function() {
             tryEditorAccess(
               this.anon,
               this.projectId,
-              (response, body) => {
-                expect(response.statusCode).to.equal(302)
-                expect(body).to.match(/.*\/restricted.*/)
-              },
+              expectErrorResponse.restricted.html,
               cb
             ),
           cb =>
@@ -513,10 +500,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.anon,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               ),
             // should not allow the user to access read-only token
@@ -537,10 +521,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.anon,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               ),
             // should not allow the user to join the project
@@ -595,11 +576,7 @@ describe('TokenAccess', function() {
             tryEditorAccess(
               this.other1,
               this.projectId,
-              (response, body) => {
-                expect(response.statusCode).to.equal(302)
-                expect(response.headers.location).to.match(/\/restricted.*/)
-                expect(body).to.match(/.*\/restricted.*/)
-              },
+              expectErrorResponse.restricted.html,
               cb
             ),
           cb =>
@@ -681,11 +658,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.other1,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(response.headers.location).to.match(/\/restricted.*/)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               ),
             cb => {
@@ -794,10 +767,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.other1,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                (response, body) => {},
                 cb
               )
             },
@@ -818,10 +788,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.other1,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               )
             },
@@ -876,10 +843,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.anon,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               ),
             cb =>
@@ -954,10 +918,7 @@ describe('TokenAccess', function() {
               tryEditorAccess(
                 this.anon,
                 this.projectId,
-                (response, body) => {
-                  expect(response.statusCode).to.equal(302)
-                  expect(body).to.match(/.*\/restricted.*/)
-                },
+                expectErrorResponse.restricted.html,
                 cb
               ),
             cb =>
@@ -1010,10 +971,7 @@ describe('TokenAccess', function() {
                 tryEditorAccess(
                   this.anon,
                   this.projectId,
-                  (response, body) => {
-                    expect(response.statusCode).to.equal(302)
-                    expect(body).to.match(/.*\/restricted.*/)
-                  },
+                  expectErrorResponse.restricted.html,
                   cb
                 ),
               cb =>
@@ -1032,10 +990,7 @@ describe('TokenAccess', function() {
                 tryEditorAccess(
                   this.anon,
                   this.projectId,
-                  (response, body) => {
-                    expect(response.statusCode).to.equal(302)
-                    expect(body).to.match(/.*\/restricted.*/)
-                  },
+                  expectErrorResponse.restricted.html,
                   cb
                 ),
               cb =>
@@ -1232,10 +1187,7 @@ describe('TokenAccess', function() {
             tryEditorAccess(
               this.other2,
               this.projectId,
-              (response, body) => {
-                expect(response.statusCode).to.equal(302)
-                expect(body).to.match(/.*\/restricted.*/)
-              },
+              expectErrorResponse.restricted.html,
               cb
             ),
           cb =>

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

@@ -18,6 +18,7 @@ const request = require('./helpers/request')
 const settings = require('settings-sharelatex')
 const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs')
 const MockV1Api = require('./helpers/MockV1Api')
+const expectErrorResponse = require('./helpers/expectErrorResponse')
 
 describe('UserEmails', function() {
   beforeEach(function(done) {
@@ -814,4 +815,29 @@ describe('UserEmails', function() {
       )
     })
   })
+
+  describe('when not logged in', function() {
+    beforeEach(function(done) {
+      this.anonymous = new User()
+      this.anonymous.getCsrfToken(done)
+    })
+    it('should return a plain 403 when setting the email', function(done) {
+      this.anonymous.request(
+        {
+          method: 'POST',
+          url: '/user/emails',
+          json: {
+            email: 'newly-added-email@example.com'
+          }
+        },
+        (error, response, body) => {
+          if (error) {
+            return done(error)
+          }
+          expectErrorResponse.requireLogin.json(response, body)
+          done()
+        }
+      )
+    })
+  })
 })

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

@@ -0,0 +1,22 @@
+const { expect } = require('chai')
+
+module.exports = {
+  requireLogin: {
+    json(response, body) {
+      expect(response.statusCode).to.equal(401)
+      expect(body).to.equal('Unauthorized')
+      expect(response.headers['www-authenticate']).to.equal('OverleafLogin')
+    }
+  },
+
+  restricted: {
+    html(response, body) {
+      expect(response.statusCode).to.equal(403)
+      expect(body).to.match(/<head><title>Restricted/)
+    },
+    json(response, body) {
+      expect(response.statusCode).to.equal(403)
+      expect(body).to.deep.equal({ message: 'restricted' })
+    }
+  }
+}

services/web/test/unit/src/Authentication/AuthenticationControllerTests.js

@@ -23,6 +23,12 @@ describe('AuthenticationController', function() {
         console: console
       },
       requires: {
+        '../Helpers/AsyncFormHelper': (this.AsyncFormHelper = {
+          redirect: sinon.stub()
+        }),
+        '../../infrastructure/RequestContentTypeDetection': {
+          acceptsJson: (this.acceptsJson = sinon.stub().returns(false))
+        },
         './AuthenticationManager': (this.AuthenticationManager = {}),
         '../User/UserUpdater': (this.UserUpdater = {
           updateUser: sinon.stub()
@@ -965,7 +971,7 @@ describe('AuthenticationController', function() {
       this.AuthenticationController._recordSuccessfulLogin = sinon.stub()
       this.AnalyticsManager.recordEvent = sinon.stub()
       this.AnalyticsManager.identifyUser = sinon.stub()
-      this.req.headers = { accept: 'application/json, whatever' }
+      this.acceptsJson.returns(true)
       this.res.json = sinon.stub()
       this.res.redirect = sinon.stub()
     })
@@ -1011,14 +1017,18 @@ describe('AuthenticationController', function() {
         this.res,
         this.next
       )
-      expect(this.res.json.callCount).to.equal(1)
-      expect(this.res.redirect.callCount).to.equal(0)
-      expect(this.res.json.calledWith({ redir: '/some/page' })).to.equal(true)
+      expect(
+        this.AsyncFormHelper.redirect.calledWith(
+          this.req,
+          this.res,
+          '/some/page'
+        )
+      ).to.equal(true)
     })
 
     describe('with a non-json request', function() {
       beforeEach(function() {
-        this.req.headers = {}
+        this.acceptsJson.returns(false)
         this.res.json = sinon.stub()
         this.res.redirect = sinon.stub()
       })
@@ -1030,9 +1040,13 @@ describe('AuthenticationController', function() {
           this.res,
           this.next
         )
-        expect(this.res.json.callCount).to.equal(0)
-        expect(this.res.redirect.callCount).to.equal(1)
-        expect(this.res.redirect.calledWith('/some/page')).to.equal(true)
+        expect(
+          this.AsyncFormHelper.redirect.calledWith(
+            this.req,
+            this.res,
+            '/some/page'
+          )
+        ).to.equal(true)
       })
     })
 
@@ -1072,7 +1086,7 @@ describe('AuthenticationController', function() {
           this.res,
           this.user
         )
-        expect(this.res.json.callCount).to.equal(1)
+        expect(this.AsyncFormHelper.redirect.called).to.equal(true)
       })
 
       it('stop if hook has redirected', function(done) {

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

@@ -159,8 +159,8 @@ describe('AuthorizationMiddleware', function() {
                 this.next
               )
               this.next.called.should.equal(false)
-              this.AuthorizationMiddleware.redirectToRestricted
-                .calledWith(this.req, this.res, this.next)
+              this.HttpErrorHandler.forbidden
+                .calledWith(this.req, this.res)
                 .should.equal(true)
             })
           })
@@ -200,8 +200,8 @@ describe('AuthorizationMiddleware', function() {
                 this.next
               )
               this.next.called.should.equal(false)
-              this.AuthorizationMiddleware.redirectToRestricted
-                .calledWith(this.req, this.res, this.next)
+              this.HttpErrorHandler.forbidden
+                .calledWith(this.req, this.res)
                 .should.equal(true)
             })
           })

services/web/test/unit/src/helpers/MockRequest.js

@@ -9,6 +9,7 @@ class MockRequest {
   static initClass() {
     this.prototype.session = { destroy() {} }
 
+    this.prototype.headers = {}
     this.prototype.params = {}
     this.prototype.query = {}
     this.prototype.body = {}

services/web/test/unit/src/infrastructure/RequestContentTypeDetectionTests.js

@@ -0,0 +1,47 @@
+const accepts = require('accepts')
+const { expect } = require('chai')
+const MODULE_PATH =
+  '../../../../app/src/infrastructure/RequestContentTypeDetection.js'
+const MockRequest = require('../helpers/MockRequest')
+const SandboxedModule = require('sandboxed-module')
+
+describe('RequestContentTypeDetection', function() {
+  before(function() {
+    this.RequestContentTypeDetection = SandboxedModule.require(MODULE_PATH)
+    this.req = new MockRequest()
+    this.req.accepts = function(...args) {
+      return accepts(this).type(...args)
+    }
+  })
+
+  describe('isJson=true', function() {
+    function expectJson(type) {
+      it(type, function() {
+        this.req.headers.accept = type
+        expect(this.RequestContentTypeDetection.acceptsJson(this.req)).to.equal(
+          true
+        )
+      })
+    }
+
+    expectJson('application/json')
+    expectJson('application/json, text/plain, */*')
+    expectJson('application/json, text/html, */*')
+  })
+
+  describe('isJson=false', function() {
+    function expectNonJson(type) {
+      it(type, function() {
+        this.req.headers.accept = type
+        expect(this.RequestContentTypeDetection.acceptsJson(this.req)).to.equal(
+          false
+        )
+      })
+    }
+
+    expectNonJson('*/*')
+    expectNonJson('text/html')
+    expectNonJson('text/html, application/json')
+    expectNonJson('image/png')
+  })
+})