From 674954f96f9b74fa93e8d1b465bffc4099ebd40a Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 19 Aug 2020 11:09:02 +0200 Subject: [PATCH] Merge pull request #3094 from overleaf/sk-restrict-admin-flag Check domain of emails on admin users GitOrigin-RevId: 75de9cff30e3c628249fcd0ea3446a33d51d39b4 --- .../AuthenticationController.js | 32 ++++++++ services/web/app/src/infrastructure/Server.js | 2 + services/web/config/settings.defaults.coffee | 1 + .../acceptance/config/settings.test.coffee | 1 + .../test/acceptance/src/AdminEmailTests.js | 57 ++++++++++++++ .../AuthenticationControllerTests.js | 78 ++++++++++++++++++- 6 files changed, 169 insertions(+), 2 deletions(-) create mode 100644 services/web/test/acceptance/src/AdminEmailTests.js diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index f41147ce9d..2b1aca3a63 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -17,6 +17,7 @@ const UrlHelper = require('../Helpers/UrlHelper') const AsyncFormHelper = require('../Helpers/AsyncFormHelper') const SudoModeHandler = require('../SudoMode/SudoModeHandler') const _ = require('lodash') +const OError = require('@overleaf/o-error') const { acceptsJson } = require('../../infrastructure/RequestContentTypeDetection') @@ -319,6 +320,37 @@ const AuthenticationController = { } }, + validateAdmin(req, res, next) { + const adminDomains = Settings.adminDomains + if ( + !adminDomains || + !(Array.isArray(adminDomains) && adminDomains.length) + ) { + return next() + } + const user = AuthenticationController.getSessionUser(req) + if (!(user && user.isAdmin)) { + return next() + } + const email = user.email + if (email == null) { + return next( + new OError('[ValidateAdmin] Admin user without email address', { + userId: user._id + }) + ) + } + if (!adminDomains.find(domain => email.endsWith(`@${domain}`))) { + return next( + new OError('[ValidateAdmin] Admin user with invalid email domain', { + email: email, + userId: user._id + }) + ) + } + return next() + }, + httpAuth: basicAuth(function(user, pass) { let expectedPassword = Settings.httpAuthUsers[user] const isValid = diff --git a/services/web/app/src/infrastructure/Server.js b/services/web/app/src/infrastructure/Server.js index 0c8d761bb8..58d8a71f18 100644 --- a/services/web/app/src/infrastructure/Server.js +++ b/services/web/app/src/infrastructure/Server.js @@ -199,6 +199,8 @@ webRouter.use(function(req, res, next) { } }) +webRouter.use(AuthenticationController.validateAdmin) + // add security headers using Helmet const noCacheMiddleware = require('nocache')() webRouter.use(function(req, res, next) { diff --git a/services/web/config/settings.defaults.coffee b/services/web/config/settings.defaults.coffee index ae79e95e97..d16100d0fc 100644 --- a/services/web/config/settings.defaults.coffee +++ b/services/web/config/settings.defaults.coffee @@ -499,6 +499,7 @@ module.exports = settings = appName: process.env['APP_NAME'] or "ShareLaTeX (Community Edition)" adminEmail: process.env['ADMIN_EMAIL'] or "placeholder@example.com" + adminDomains: JSON.parse(process.env['ADMIN_DOMAINS'] or 'null') salesEmail: process.env['SALES_EMAIL'] or "placeholder@example.com" diff --git a/services/web/test/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee index fff7a507db..76fc57912a 100644 --- a/services/web/test/acceptance/config/settings.test.coffee +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -13,6 +13,7 @@ module.exports = enableSubscriptions: true httpAuthUsers: httpAuthUsers + adminDomains: ['example.com'] apis: web: diff --git a/services/web/test/acceptance/src/AdminEmailTests.js b/services/web/test/acceptance/src/AdminEmailTests.js new file mode 100644 index 0000000000..1cfcc13c95 --- /dev/null +++ b/services/web/test/acceptance/src/AdminEmailTests.js @@ -0,0 +1,57 @@ +const { expect } = require('chai') +const async = require('async') +const User = require('./helpers/User') + +describe('AdminEmails', function() { + beforeEach(function(done) { + this.timeout(5000) + done() + }) + + describe('an admin with an invalid email address', function() { + before(function(done) { + this.badUser = new User({ email: 'alice@evil.com' }) + async.series( + [ + cb => this.badUser.ensureUserExists(cb), + cb => this.badUser.ensureAdmin(cb) + ], + done + ) + }) + + it('should block the user', function(done) { + this.badUser.login(err => { + expect(err).to.not.exist + this.badUser.getProjectListPage((err, statusCode) => { + expect(err).to.exist + done() + }) + }) + }) + }) + + describe('an admin with a valid email address', function() { + before(function(done) { + this.goodUser = new User({ email: 'alice@example.com' }) + async.series( + [ + cb => this.goodUser.ensureUserExists(cb), + cb => this.goodUser.ensureAdmin(cb) + ], + done + ) + }) + + it('should not block the user', function(done) { + this.goodUser.login(err => { + expect(err).to.not.exist + this.goodUser.getProjectListPage((err, statusCode) => { + expect(err).to.not.exist + expect(statusCode).to.equal(200) + done() + }) + }) + }) + }) +}) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 28990b910f..d6780cebdd 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -52,10 +52,10 @@ describe('AuthenticationController', function() { error: sinon.stub(), err: sinon.stub() }), - 'settings-sharelatex': { + 'settings-sharelatex': (this.Settings = { siteUrl: 'http://www.foo.bar', httpAuthUsers: this.httpAuthUsers - }, + }), passport: (this.passport = { authenticate: sinon.stub().returns(sinon.stub()) }), @@ -109,6 +109,80 @@ describe('AuthenticationController', function() { tk.reset() }) + describe('validateAdmin', function() { + beforeEach(function() { + this.Settings.adminDomains = ['good.example.com'] + this.goodAdmin = { + email: 'alice@good.example.com', + isAdmin: true + } + this.badAdmin = { + email: 'beatrice@bad.example.com', + isAdmin: true + } + this.normalUser = { + email: 'claire@whatever.example.com', + isAdmin: false + } + }) + + it('should skip when adminDomains are not configured', function(done) { + this.Settings.adminDomains = [] + this.AuthenticationController.getSessionUser = sinon + .stub() + .returns(this.normalUser) + this.AuthenticationController.validateAdmin(this.req, this.res, err => { + this.AuthenticationController.getSessionUser.called.should.equal(false) + expect(err).to.not.exist + done() + }) + }) + + it('should skip non-admin user', function(done) { + this.AuthenticationController.getSessionUser = sinon + .stub() + .returns(this.normalUser) + this.AuthenticationController.validateAdmin(this.req, this.res, err => { + this.AuthenticationController.getSessionUser.called.should.equal(true) + expect(err).to.not.exist + done() + }) + }) + + it('should permit an admin with the right doman', function(done) { + this.AuthenticationController.getSessionUser = sinon + .stub() + .returns(this.goodAdmin) + this.AuthenticationController.validateAdmin(this.req, this.res, err => { + this.AuthenticationController.getSessionUser.called.should.equal(true) + expect(err).to.not.exist + done() + }) + }) + + it('should block an admin with a missing email', function(done) { + this.AuthenticationController.getSessionUser = sinon + .stub() + .returns({ isAdmin: true }) + this.AuthenticationController.validateAdmin(this.req, this.res, err => { + this.AuthenticationController.getSessionUser.called.should.equal(true) + expect(err).to.exist + done() + }) + }) + + it('should block an admin with a bad domain', function(done) { + this.AuthenticationController.getSessionUser = sinon + .stub() + .returns(this.badAdmin) + this.AuthenticationController.validateAdmin(this.req, this.res, err => { + this.AuthenticationController.getSessionUser.called.should.equal(true) + expect(err).to.exist + done() + }) + }) + }) + describe('isUserLoggedIn', function() { beforeEach(function() { this.stub = sinon.stub(this.AuthenticationController, 'getLoggedInUserId')