diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index e8cdfb5745..4e316d9423 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/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') } diff --git a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js index dde580b13a..6d396593c2 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js +++ b/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) } ) }) diff --git a/services/web/app/src/Features/Errors/HttpErrorHandler.js b/services/web/app/src/Features/Errors/HttpErrorHandler.js index 43e940b003..8da8510eb1 100644 --- a/services/web/app/src/Features/Errors/HttpErrorHandler.js +++ b/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) + } } } diff --git a/services/web/app/src/Features/Helpers/AsyncFormHelper.js b/services/web/app/src/Features/Helpers/AsyncFormHelper.js index 6d0c76851b..819ee34e1e 100644 --- a/services/web/app/src/Features/Helpers/AsyncFormHelper.js +++ b/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) diff --git a/services/web/app/src/infrastructure/RequestContentTypeDetection.js b/services/web/app/src/infrastructure/RequestContentTypeDetection.js new file mode 100644 index 0000000000..fd2b7d4855 --- /dev/null +++ b/services/web/app/src/infrastructure/RequestContentTypeDetection.js @@ -0,0 +1,5 @@ +module.exports = { + acceptsJson(req) { + return req.accepts(['html', 'json']) === 'json' + } +} diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 4994381042..31ba2494f1 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/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) } }) diff --git a/services/web/test/acceptance/src/AuthorizationTests.js b/services/web/test/acceptance/src/AuthorizationTests.js index a837112d68..2c230b614b 100644 --- a/services/web/test/acceptance/src/AuthorizationTests.js +++ b/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 ) } diff --git a/services/web/test/acceptance/src/CloseSiteTests.js b/services/web/test/acceptance/src/CloseSiteTests.js index ce61808035..33854e8b94 100644 --- a/services/web/test/acceptance/src/CloseSiteTests.js +++ b/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() }) }) }) diff --git a/services/web/test/acceptance/src/TagsTests.js b/services/web/test/acceptance/src/TagsTests.js index 5a02bfb973..cf6e4e8b28 100644 --- a/services/web/test/acceptance/src/TagsTests.js +++ b/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() }) }) diff --git a/services/web/test/acceptance/src/TokenAccessTests.js b/services/web/test/acceptance/src/TokenAccessTests.js index 63a20d0ad7..803dce63ce 100644 --- a/services/web/test/acceptance/src/TokenAccessTests.js +++ b/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 => diff --git a/services/web/test/acceptance/src/UserEmailsTests.js b/services/web/test/acceptance/src/UserEmailsTests.js index 7058cda50a..90a5b7bac9 100644 --- a/services/web/test/acceptance/src/UserEmailsTests.js +++ b/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() + } + ) + }) + }) }) diff --git a/services/web/test/acceptance/src/helpers/expectErrorResponse.js b/services/web/test/acceptance/src/helpers/expectErrorResponse.js new file mode 100644 index 0000000000..f669d7d656 --- /dev/null +++ b/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(/