diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index b14da0ee01..3fb323bdea 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -23,6 +23,7 @@ const AnalyticsRegistrationSourceHelper = require('../Analytics/AnalyticsRegistr const { acceptsJson, } = require('../../infrastructure/RequestContentTypeDetection') +const { ParallelLoginError } = require('./AuthenticationErrors') function send401WithChallenge(res) { res.setHeader('WWW-Authenticate', 'OverleafLogin') @@ -89,7 +90,13 @@ const AuthenticationController = { } else { res.status(info.status || 200) delete info.status - return res.json({ message: info }) + const body = { message: info } + const { errorReason } = info + if (errorReason) { + body.errorReason = errorReason + delete info.errorReason + } + return res.json(body) } } })(req, res, next) @@ -188,9 +195,22 @@ const AuthenticationController = { password, function (error, user) { if (error != null) { + if (error instanceof ParallelLoginError) { + return done(null, false, { status: 429 }) + } return done(error) } - if (user != null) { + if ( + user && + AuthenticationController.captchaRequiredForLogin(req, user) + ) { + done(null, false, { + text: req.i18n.translate('cannot_verify_user_not_robot'), + type: 'error', + errorReason: 'cannot_verify_user_not_robot', + status: 400, + }) + } else if (user) { // async actions done(null, user) } else { @@ -209,6 +229,30 @@ const AuthenticationController = { ) }, + captchaRequiredForLogin(req, user) { + switch (AuthenticationController.getAuditInfo(req).captcha) { + case 'disabled': + return false + case 'solved': + return false + case 'skipped': { + let required = false + if (user.lastFailedLogin) { + const requireCaptchaUntil = + user.lastFailedLogin.getTime() + + Settings.elevateAccountSecurityAfterFailedLogin + required = requireCaptchaUntil >= Date.now() + } + Metrics.inc('force_captcha_on_login', 1, { + status: required ? 'yes' : 'no', + }) + return required + } + default: + throw new Error('captcha middleware missing in handler chain') + } + }, + ipMatchCheck(req, user) { if (req.ip !== user.lastLoginIp) { NotificationsBuilder.ipMatcherAffiliation(user._id).create( diff --git a/services/web/app/src/Features/Authentication/AuthenticationErrors.js b/services/web/app/src/Features/Authentication/AuthenticationErrors.js index c2df0fcc89..fa394903b1 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationErrors.js +++ b/services/web/app/src/Features/Authentication/AuthenticationErrors.js @@ -2,8 +2,10 @@ const Errors = require('../Errors/Errors') class InvalidEmailError extends Errors.BackwardCompatibleError {} class InvalidPasswordError extends Errors.BackwardCompatibleError {} +class ParallelLoginError extends Errors.BackwardCompatibleError {} module.exports = { InvalidEmailError, InvalidPasswordError, + ParallelLoginError, } diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index afd928d666..76d41f6131 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -6,6 +6,7 @@ const EmailHelper = require('../Helpers/EmailHelper') const { InvalidEmailError, InvalidPasswordError, + ParallelLoginError, } = require('./AuthenticationErrors') const util = require('util') const HaveIBeenPwned = require('./HaveIBeenPwned') @@ -38,19 +39,36 @@ const AuthenticationManager = { if (error) { return callback(error) } + const update = { $inc: { loginEpoch: 1 } } if (!match) { - return callback(null, null) + update.$set = { lastFailedLogin: new Date() } } - AuthenticationManager.checkRounds( - user, - user.hashedPassword, - password, - function (err) { + User.updateOne( + { _id: user._id, loginEpoch: user.loginEpoch }, + update, + {}, + (err, result) => { if (err) { return callback(err) } - callback(null, user) - HaveIBeenPwned.checkPasswordForReuseInBackground(password) + if (result.nModified !== 1) { + return callback(new ParallelLoginError()) + } + if (!match) { + return callback(null, null) + } + AuthenticationManager.checkRounds( + user, + user.hashedPassword, + password, + function (err) { + if (err) { + return callback(err) + } + callback(null, user) + HaveIBeenPwned.checkPasswordForReuseInBackground(password) + } + ) } ) }) diff --git a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js b/services/web/app/src/Features/Captcha/CaptchaMiddleware.js index 9ac28521a9..3b900aa340 100644 --- a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js +++ b/services/web/app/src/Features/Captcha/CaptchaMiddleware.js @@ -6,11 +6,11 @@ const DeviceHistory = require('./DeviceHistory') const AuthenticationController = require('../Authentication/AuthenticationController') const { expressify } = require('../../util/promises') -function respondInvalidCaptcha(res) { +function respondInvalidCaptcha(req, res) { res.status(400).json({ errorReason: 'cannot_verify_user_not_robot', message: { - text: 'Sorry, we could not verify that you are not a robot. Please check that Google reCAPTCHA is not being blocked by an ad blocker or firewall.', + text: req.i18n.translate('cannot_verify_user_not_robot'), }, }) } @@ -36,24 +36,27 @@ async function canSkipCaptcha(req, res) { function validateCaptcha(action) { return expressify(async function (req, res, next) { if (!Settings.recaptcha?.siteKey || Settings.recaptcha.disabled[action]) { + if (action === 'login') { + AuthenticationController.setAuditInfo(req, { captcha: 'disabled' }) + } Metrics.inc('captcha', 1, { path: action, status: 'disabled' }) return next() } + const reCaptchaResponse = req.body['g-recaptcha-response'] if (action === 'login') { await initializeDeviceHistory(req) - if (req.deviceHistory.has(req.body?.email)) { + if (!reCaptchaResponse && req.deviceHistory.has(req.body?.email)) { // The user has previously logged in from this device, which required // solving a captcha or keeping the device history alive. - // We can skip checking the (potentially missing) captcha response. + // We can skip checking the (missing) captcha response. AuthenticationController.setAuditInfo(req, { captcha: 'skipped' }) Metrics.inc('captcha', 1, { path: action, status: 'skipped' }) return next() } } - const reCaptchaResponse = req.body['g-recaptcha-response'] if (!reCaptchaResponse) { Metrics.inc('captcha', 1, { path: action, status: 'missing' }) - return respondInvalidCaptcha(res) + return respondInvalidCaptcha(req, res) } const options = { method: 'POST', @@ -84,7 +87,7 @@ function validateCaptcha(action) { 'failed recaptcha siteverify request' ) Metrics.inc('captcha', 1, { path: action, status: 'failed' }) - return respondInvalidCaptcha(res) + return respondInvalidCaptcha(req, res) } Metrics.inc('captcha', 1, { path: action, status: 'solved' }) if (action === 'login') { @@ -95,6 +98,7 @@ function validateCaptcha(action) { } module.exports = { + respondInvalidCaptcha, validateCaptcha, canSkipCaptcha: expressify(canSkipCaptcha), } diff --git a/services/web/app/src/models/User.js b/services/web/app/src/models/User.js index 7321f6be82..15da38fc8e 100644 --- a/services/web/app/src/models/User.js +++ b/services/web/app/src/models/User.js @@ -56,7 +56,9 @@ const UserSchema = new Schema({ return new Date() }, }, + loginEpoch: { type: Number }, lastActive: { type: Date }, + lastFailedLogin: { type: Date }, lastLoggedIn: { type: Date }, lastLoginIp: { type: String, default: '' }, loginCount: { type: Number, default: 0 }, diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 8fa13b8ee4..39a6781d9f 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -106,7 +106,11 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { if (Settings.enableLegacyLogin) { AuthenticationController.addEndpointToLoginWhitelist('/login/legacy') webRouter.get('/login/legacy', UserPagesController.loginPage) - webRouter.post('/login/legacy', AuthenticationController.passportLogin) + webRouter.post( + '/login/legacy', + CaptchaMiddleware.validateCaptcha('login'), + AuthenticationController.passportLogin + ) } webRouter.get( diff --git a/services/web/config/settings.defaults.js b/services/web/config/settings.defaults.js index a7ddecac9b..f14bcaa83f 100644 --- a/services/web/config/settings.defaults.js +++ b/services/web/config/settings.defaults.js @@ -435,6 +435,10 @@ module.exports = { }, }, + elevateAccountSecurityAfterFailedLogin: + parseInt(process.env.ELEVATED_ACCOUNT_SECURITY_AFTER_FAILED_LOGIN_MS, 10) || + 24 * 60 * 60 * 1000, + deviceHistory: { cookieName: process.env.DEVICE_HISTORY_COOKIE_NAME || 'deviceHistory', entryExpiry: diff --git a/services/web/test/acceptance/src/CaptchaTests.js b/services/web/test/acceptance/src/CaptchaTests.js index 23c5d06b09..dedd4ec993 100644 --- a/services/web/test/acceptance/src/CaptchaTests.js +++ b/services/web/test/acceptance/src/CaptchaTests.js @@ -1,3 +1,4 @@ +const { db } = require('../../../app/src/infrastructure/mongodb') const { expect } = require('chai') const User = require('./helpers/User').promises @@ -9,22 +10,26 @@ describe('Captcha', function () { await user.ensureUserExists() }) - async function loginWithCaptcha(captchaResponse) { - return loginWithEmailAndCaptcha(user.email, captchaResponse) - } - - async function loginWithEmailAndCaptcha(email, captchaResponse) { + async function login(email, password, captchaResponse) { await user.getCsrfToken() return user.doRequest('POST', { url: '/login', json: { email, - password: user.password, + password, 'g-recaptcha-response': captchaResponse, }, }) } + async function loginWithCaptcha(captchaResponse) { + return login(user.email, user.password, captchaResponse) + } + + async function loginWithEmailAndCaptcha(email, captchaResponse) { + return login(email, user.password, captchaResponse) + } + async function canSkipCaptcha(email) { await user.getCsrfToken() const { response, body } = await user.doRequest('POST', { @@ -45,6 +50,16 @@ describe('Captcha', function () { expect(body).to.deep.equal({ redir: '/project' }) } + function expectBadLogin(response, body) { + expect(response.statusCode).to.equal(401) + expect(body).to.deep.equal({ + message: { + text: 'Your email or password is incorrect. Please try again', + type: 'error', + }, + }) + } + it('should reject a login without captcha response', async function () { const { response, body } = await loginWithCaptcha('') expectBadCaptchaResponse(response, body) @@ -104,6 +119,58 @@ describe('Captcha', function () { expectBadCaptchaResponse(response, body) }) + describe('login failure', function () { + beforeEach(async function () { + const { response, body } = await login( + user.email, + 'bad password', + 'valid' + ) + expectBadLogin(response, body) + }) + + it('should be able to skip captcha per device history', async function () { + expect(await canSkipCaptcha(user.email)).to.equal(true) + }) + + it('should request a captcha despite device history entry', async function () { + const { response, body } = await loginWithCaptcha('') + expectBadCaptchaResponse(response, body) + }) + + it('should accept the login with captcha', async function () { + const { response, body } = await loginWithCaptcha('valid') + expectSuccessfulLogin(response, body) + }) + + describe('when the login failure happened a long time ago', function () { + beforeEach(async function () { + db.users.updateOne( + { email: user.email }, + { + $set: { + lastFailedLogin: new Date( + Date.now() - 90 * 24 * 60 * 60 * 1000 + ), + }, + } + ) + }) + + it('should be able to skip captcha per device history', async function () { + expect(await canSkipCaptcha(user.email)).to.equal(true) + }) + it('should accept the login without captcha', async function () { + const { response, body } = await loginWithCaptcha('') + expectSuccessfulLogin(response, body) + }) + it('should accept the login with captcha', async function () { + const { response, body } = await loginWithCaptcha('valid') + expectSuccessfulLogin(response, body) + }) + }) + }) + describe('cycle history', function () { beforeEach('create and login with 10 other users', async function () { for (let i = 0; i < 10; i++) { diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index f7df3412b3..6930231fa1 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -7,6 +7,7 @@ const tk = require('timekeeper') const MockRequest = require('../helpers/MockRequest') const MockResponse = require('../helpers/MockResponse') const { ObjectId } = require('mongodb') +const AuthenticationErrors = require('../../../../app/src/Features/Authentication/AuthenticationErrors') describe('AuthenticationController', function () { beforeEach(function () { @@ -32,6 +33,7 @@ describe('AuthenticationController', function () { this.AuthenticationController = SandboxedModule.require(modulePath, { requires: { + './AuthenticationErrors': AuthenticationErrors, '../User/UserAuditLogHandler': (this.UserAuditLogHandler = { addEntry: sinon.stub().yields(null), }), @@ -63,6 +65,7 @@ describe('AuthenticationController', function () { '@overleaf/settings': (this.Settings = { siteUrl: 'http://www.foo.bar', httpAuthUsers: this.httpAuthUsers, + elevateAccountSecurityAfterFailedLogin: 90 * 24 * 60 * 60 * 1000, }), passport: (this.passport = { authenticate: sinon.stub().returns(sinon.stub()), @@ -254,7 +257,7 @@ describe('AuthenticationController', function () { this.next ) this.res.json.callCount.should.equal(1) - this.res.json.calledWith({ message: this.info }).should.equal(true) + this.res.json.should.have.been.calledWith({ message: this.info }) expect(this.res.json.lastCall.args[0].redir != null).to.equal(false) }) }) @@ -273,6 +276,7 @@ describe('AuthenticationController', function () { postLoginRedirect: '/path/to/redir/to', }, } + this.req.__authAuditInfo = { captcha: 'disabled' } this.cb = sinon.stub() }) @@ -325,22 +329,103 @@ describe('AuthenticationController', function () { .stub() .callsArgWith(2, null, this.user) this.req.sessionID = Math.random() - this.AuthenticationController.doPassportLogin( - this.req, - this.req.body.email, - this.req.body.password, - this.cb - ) }) - it('should attempt to authorise the user', function () { - this.AuthenticationManager.authenticate - .calledWith({ email: this.email.toLowerCase() }, this.password) - .should.equal(true) + describe('happy path', function () { + beforeEach(function () { + this.AuthenticationController.doPassportLogin( + this.req, + this.req.body.email, + this.req.body.password, + this.cb + ) + }) + it('should attempt to authorise the user', function () { + this.AuthenticationManager.authenticate + .calledWith({ email: this.email.toLowerCase() }, this.password) + .should.equal(true) + }) + + it("should establish the user's session", function () { + this.cb.calledWith(null, this.user).should.equal(true) + }) }) - it("should establish the user's session", function () { - this.cb.calledWith(null, this.user).should.equal(true) + describe('when authenticate flags a parallel login', function () { + beforeEach(function () { + this.AuthenticationManager.authenticate = sinon + .stub() + .callsArgWith(2, new AuthenticationErrors.ParallelLoginError()) + this.AuthenticationController.doPassportLogin( + this.req, + this.req.body.email, + this.req.body.password, + this.cb + ) + }) + + it('should send a 429', function () { + this.cb.should.have.been.calledWith(null, false, { status: 429 }) + }) + }) + + describe('with a user having a recent failed login ', function () { + beforeEach(function () { + this.user.lastFailedLogin = new Date() + }) + + describe('with captcha disabled', function () { + beforeEach(function () { + this.req.__authAuditInfo.captcha = 'disabled' + this.AuthenticationController.doPassportLogin( + this.req, + this.req.body.email, + this.req.body.password, + this.cb + ) + }) + + it('should let the user log in', function () { + this.cb.should.have.been.calledWith(null, this.user) + }) + }) + + describe('with a solved captcha', function () { + beforeEach(function () { + this.req.__authAuditInfo.captcha = 'solved' + this.AuthenticationController.doPassportLogin( + this.req, + this.req.body.email, + this.req.body.password, + this.cb + ) + }) + + it('should let the user log in', function () { + this.cb.should.have.been.calledWith(null, this.user) + }) + }) + + describe('with a skipped captcha', function () { + beforeEach(function () { + this.req.__authAuditInfo.captcha = 'skipped' + this.AuthenticationController.doPassportLogin( + this.req, + this.req.body.email, + this.req.body.password, + this.cb + ) + }) + + it('should request a captcha', function () { + this.cb.should.have.been.calledWith(null, false, { + text: 'cannot_verify_user_not_robot', + type: 'error', + errorReason: 'cannot_verify_user_not_robot', + status: 400, + }) + }) + }) }) }) diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 16f3c511d0..8907be1662 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -3,17 +3,21 @@ const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') const { ObjectId } = require('mongodb') const AuthenticationErrors = require('../../../../app/src/Features/Authentication/AuthenticationErrors') +const tk = require('timekeeper') const modulePath = '../../../../app/src/Features/Authentication/AuthenticationManager.js' describe('AuthenticationManager', function () { beforeEach(function () { + tk.freeze(Date.now()) this.settings = { security: { bcryptRounds: 4 } } this.AuthenticationManager = SandboxedModule.require(modulePath, { requires: { '../../models/User': { - User: (this.User = {}), + User: (this.User = { + updateOne: sinon.stub().callsArgWith(3, null, { nModified: 1 }), + }), }, '../../infrastructure/mongodb': { db: (this.db = { users: {} }), @@ -31,6 +35,10 @@ describe('AuthenticationManager', function () { this.callback = sinon.stub() }) + afterEach(function () { + tk.reset() + }) + describe('with real bcrypt', function () { beforeEach(function () { const bcrypt = require('bcrypt') @@ -49,13 +57,13 @@ describe('AuthenticationManager', function () { _id: 'user-id', email: (this.email = 'USER@sharelatex.com'), } + this.user.hashedPassword = this.testPassword this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) }) describe('when the hashed password matches', function () { beforeEach(function (done) { this.unencryptedPassword = 'testpassword' - this.user.hashedPassword = this.testPassword this.AuthenticationManager.authenticate( { email: this.email }, this.unencryptedPassword, @@ -70,17 +78,46 @@ describe('AuthenticationManager', function () { this.User.findOne.calledWith({ email: this.email }).should.equal(true) }) + it('should bump epoch', function () { + this.User.updateOne.should.have.been.calledWith( + { + _id: this.user._id, + loginEpoch: this.user.loginEpoch, + }, + { + $inc: { loginEpoch: 1 }, + }, + {} + ) + }) + it('should return the user', function () { this.callback.calledWith(null, this.user).should.equal(true) }) }) describe('when the encrypted passwords do not match', function () { - beforeEach(function () { + beforeEach(function (done) { this.AuthenticationManager.authenticate( { email: this.email }, 'notthecorrectpassword', - this.callback + (...args) => { + this.callback(...args) + done() + } + ) + }) + + it('should persist the login failure and bump epoch', function () { + this.User.updateOne.should.have.been.calledWith( + { + _id: this.user._id, + loginEpoch: this.user.loginEpoch, + }, + { + $inc: { loginEpoch: 1 }, + $set: { lastFailedLogin: new Date() }, + } ) }) @@ -88,6 +125,52 @@ describe('AuthenticationManager', function () { this.callback.calledWith(null, null).should.equal(true) }) }) + + describe('when another request runs in parallel', function () { + beforeEach(function () { + this.User.updateOne = sinon + .stub() + .callsArgWith(3, null, { nModified: 0 }) + }) + + describe('correct password', function () { + beforeEach(function (done) { + this.AuthenticationManager.authenticate( + { email: this.email }, + 'testpassword', + (...args) => { + this.callback(...args) + done() + } + ) + }) + + it('should return an error', function () { + this.callback.should.have.been.calledWith( + sinon.match.instanceOf(AuthenticationErrors.ParallelLoginError) + ) + }) + }) + + describe('bad password', function () { + beforeEach(function (done) { + this.User.updateOne = sinon.stub().yields(null, { nModified: 0 }) + this.AuthenticationManager.authenticate( + { email: this.email }, + 'notthecorrectpassword', + (...args) => { + this.callback(...args) + done() + } + ) + }) + it('should return an error', function () { + this.callback.should.have.been.calledWith( + sinon.match.instanceOf(AuthenticationErrors.ParallelLoginError) + ) + }) + }) + }) }) describe('setUserPasswordInV2', function () {