diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 773ccb6d16..de5bd149bf 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -201,6 +201,7 @@ const AuthenticationController = { return done(null, null, { text: req.i18n.translate('to_many_login_requests_2_mins'), type: 'error', + key: 'to-many-login-requests-2-mins', status: 429, }) } @@ -236,8 +237,8 @@ const AuthenticationController = { AuthenticationController._recordFailedLogin() logger.debug({ email }, 'failed log in') done(null, false, { - text: req.i18n.translate('email_or_password_wrong_try_again'), type: 'error', + key: 'invalid-password-retry-or-reset', status: 401, }) } diff --git a/services/web/app/src/Features/Security/RateLimiterMiddleware.js b/services/web/app/src/Features/Security/RateLimiterMiddleware.js index 85a066aa38..10b28cd61c 100644 --- a/services/web/app/src/Features/Security/RateLimiterMiddleware.js +++ b/services/web/app/src/Features/Security/RateLimiterMiddleware.js @@ -69,8 +69,13 @@ function loginRateLimit(req, res, next) { } else { logger.warn({ email }, 'rate limit exceeded') res.status(429) // Too many requests - res.write('Rate limit reached, please try again later') - res.end() + res.json({ + message: { + type: 'error', + text: req.i18n.translate('to_many_login_requests_2_mins'), + key: 'to-many-login-requests-2-mins', + }, + }) } }) } diff --git a/services/web/app/src/Features/User/UserDeleter.js b/services/web/app/src/Features/User/UserDeleter.js index 40b269d248..27019ab3b5 100644 --- a/services/web/app/src/Features/User/UserDeleter.js +++ b/services/web/app/src/Features/User/UserDeleter.js @@ -33,7 +33,7 @@ module.exports = { }, } -async function deleteUser(userId, options = {}) { +async function deleteUser(userId, options) { if (!userId) { logger.warn('user_id is null when trying to delete user') throw new Error('no user_id') diff --git a/services/web/app/src/infrastructure/Features.js b/services/web/app/src/infrastructure/Features.js index 26841c2f6b..cdaf638c3d 100644 --- a/services/web/app/src/infrastructure/Features.js +++ b/services/web/app/src/infrastructure/Features.js @@ -38,7 +38,7 @@ const Features = { return ( (Boolean(Settings.ldap) && Boolean(Settings.ldap.enable)) || (Boolean(Settings.saml) && Boolean(Settings.saml.enable)) || - Boolean(_.get(Settings, ['overleaf', 'oauth'])) + Boolean(Settings.overleaf) ) }, diff --git a/services/web/app/views/user/login.pug b/services/web/app/views/user/login.pug index 5db9274655..af0194b7af 100644 --- a/services/web/app/views/user/login.pug +++ b/services/web/app/views/user/login.pug @@ -11,6 +11,10 @@ block content form(data-ol-async-form, name="loginForm", action='/login', method="POST") input(name='_csrf', type='hidden', value=csrfToken) +formMessages() + +customFormMessage('invalid-password-retry-or-reset', 'danger') + | !{translate('email_or_password_wrong_try_again_or_reset', {}, [{ name: 'a', attrs: { href: '/user/password/reset', 'aria-describedby': 'resetPasswordDescription' } }])} + span.sr-only(id='resetPasswordDescription') + | #{translate('reset_password_link')} .form-group input.form-control( type='email', diff --git a/services/web/modules/server-ce-scripts/scripts/delete-user.js b/services/web/modules/server-ce-scripts/scripts/delete-user.js index 7abcc376c6..ff1060984a 100644 --- a/services/web/modules/server-ce-scripts/scripts/delete-user.js +++ b/services/web/modules/server-ce-scripts/scripts/delete-user.js @@ -22,7 +22,10 @@ async function main() { ) return resolve() } - UserDeleter.deleteUser(user._id, function (err) { + const options = { + ipAddress: '0.0.0.0', + } + UserDeleter.deleteUser(user._id, options, function (err) { if (err) { return reject(err) } diff --git a/services/web/modules/server-ce-scripts/test/acceptance/src/ServerCEScriptsTests.js b/services/web/modules/server-ce-scripts/test/acceptance/src/ServerCEScriptsTests.js index 146c212385..29b66bf1d8 100644 --- a/services/web/modules/server-ce-scripts/test/acceptance/src/ServerCEScriptsTests.js +++ b/services/web/modules/server-ce-scripts/test/acceptance/src/ServerCEScriptsTests.js @@ -119,6 +119,11 @@ describe('ServerCEScripts', function () { run('node modules/server-ce-scripts/scripts/delete-user --email=' + email) const dbEntry = await user.get() expect(dbEntry).to.not.exist + const softDeletedEntry = await db.deletedUsers.findOne({ + 'user.email': email, + }) + expect(softDeletedEntry).to.exist + expect(softDeletedEntry.deleterData.deleterIpAddress).to.equal('0.0.0.0') }) it('should exit with code 1 on missing email', function () { diff --git a/services/web/test/acceptance/config/settings.test.saas.js b/services/web/test/acceptance/config/settings.test.saas.js index 834bfd66d4..1b5c30fb94 100644 --- a/services/web/test/acceptance/config/settings.test.saas.js +++ b/services/web/test/acceptance/config/settings.test.saas.js @@ -49,9 +49,6 @@ const overrides = { }, }, - overleaf: { - oauth: undefined, - }, saml: undefined, // Disable contentful module. diff --git a/services/web/test/acceptance/src/AdminOnlyLoginTests.js b/services/web/test/acceptance/src/AdminOnlyLoginTests.js index 0e90b0e9a0..1a4558214b 100644 --- a/services/web/test/acceptance/src/AdminOnlyLoginTests.js +++ b/services/web/test/acceptance/src/AdminOnlyLoginTests.js @@ -31,9 +31,10 @@ describe('AdminOnlyLogin', function () { await user.login() expect.fail('expected the login request to fail') } catch (err) { - expect(err).to.match( - /login failed: status=403 body={"message":{"type":"error","text":"Admin only panel"}}/ - ) + expect(err).to.match(/login failed: status=403/) + expect(err.info.body).to.deep.equal({ + message: { type: 'error', text: 'Admin only panel' }, + }) } } diff --git a/services/web/test/acceptance/src/CaptchaTests.js b/services/web/test/acceptance/src/CaptchaTests.js index fcf05df234..cf1ab65ac4 100644 --- a/services/web/test/acceptance/src/CaptchaTests.js +++ b/services/web/test/acceptance/src/CaptchaTests.js @@ -54,8 +54,8 @@ describe('Captcha', function () { expect(response.statusCode).to.equal(401) expect(body).to.deep.equal({ message: { - text: 'Your email or password is incorrect. Please try again.', type: 'error', + key: 'invalid-password-retry-or-reset', }, }) } diff --git a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js index c97505b475..678fc68673 100644 --- a/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js +++ b/services/web/test/acceptance/src/HaveIBeenPwnedApiTests.js @@ -166,9 +166,10 @@ describe('HaveIBeenPwnedApi', function () { await user.loginWithEmailPassword(user.email, 'aLeakedPassword42') expect.fail('expected the login request to fail') } catch (err) { - expect(err).to.match( - /login failed: status=401 body={"message":{"text":"Your email or password is incorrect. Please try again.","type":"error"}}/ - ) + expect(err).to.match(/login failed: status=401/) + expect(err.info.body).to.deep.equal({ + message: { type: 'error', key: 'invalid-password-retry-or-reset' }, + }) } await letPasswordCheckRunInBackground() }) diff --git a/services/web/test/acceptance/src/ProjectInviteTests.js b/services/web/test/acceptance/src/ProjectInviteTests.js index 5a7826e026..356420606d 100644 --- a/services/web/test/acceptance/src/ProjectInviteTests.js +++ b/services/web/test/acceptance/src/ProjectInviteTests.js @@ -239,7 +239,7 @@ const expectLoginPage = (user, callback) => { tryFollowLoginLink(user, '/login', (err, response, body) => { expect(err).not.to.exist expect(response.statusCode).to.equal(200) - expect(body).to.match(/Login - .*<\/title>/) + expect(body).to.match(/<title>(Login|Log in to Overleaf) - .*<\/title>/) callback() }) } diff --git a/services/web/test/acceptance/src/RegistrationTests.js b/services/web/test/acceptance/src/RegistrationTests.js index 0934e1aa4a..309335d949 100644 --- a/services/web/test/acceptance/src/RegistrationTests.js +++ b/services/web/test/acceptance/src/RegistrationTests.js @@ -78,7 +78,7 @@ describe('Registration', function () { 'g-recaptcha-response': 'valid', }, }) - const message = body && body.message && body.message.text + const message = body && body.message && body.message.key pushInto.push(message) } } @@ -97,9 +97,7 @@ describe('Registration', function () { it('should produce the correct responses so far', function () { expect(results.length).to.equal(9) expect(results).to.deep.equal( - Array(9).fill( - 'Your email or password is incorrect. Please try again.' - ) + Array(9).fill('invalid-password-retry-or-reset') ) }) @@ -117,12 +115,8 @@ describe('Registration', function () { expect(results.length).to.equal(15) expect(results).to.deep.equal( Array(10) - .fill('Your email or password is incorrect. Please try again.') - .concat( - Array(5).fill( - 'This account has had too many login requests. Please wait 2 minutes before trying to log in again' - ) - ) + .fill('invalid-password-retry-or-reset') + .concat(Array(5).fill('to-many-login-requests-2-mins')) ) }) @@ -146,9 +140,7 @@ describe('Registration', function () { }) it('should not rate limit their request', function () { - expect(messages).to.deep.equal([ - 'Your email or password is incorrect. Please try again.', - ]) + expect(messages).to.deep.equal(['invalid-password-retry-or-reset']) }) it('should not record any further rate limited requests', async function () { @@ -195,9 +187,7 @@ describe('Registration', function () { it('should not emit any rate limited responses yet', function () { expect(results.length).to.equal(9) expect(results).to.deep.equal( - Array(9).fill( - 'Your email or password is incorrect. Please try again.' - ) + Array(9).fill('invalid-password-retry-or-reset') ) }) }) @@ -360,9 +350,13 @@ describe('Registration', function () { this.user1.addEmail(secondaryEmail, err => { expect(err).to.not.exist this.user1.loginWith(secondaryEmail, err => { - expect(err).to.match( - /login failed: status=401 body={"message":{"text":"Your email or password is incorrect. Please try again.","type":"error"}}/ - ) + expect(err).to.match(/login failed: status=401/) + expect(err.info.body).to.deep.equal({ + message: { + type: 'error', + key: 'invalid-password-retry-or-reset', + }, + }) this.user1.isLoggedIn((err, isLoggedIn) => { expect(err).to.not.exist expect(isLoggedIn).to.equal(false) @@ -402,7 +396,7 @@ describe('Registration', function () { expect(err).to.not.exist expect(body.redir != null).to.equal(false) expect(body.message != null).to.equal(true) - expect(body.message).to.have.all.keys('type', 'text') + expect(body.message).to.have.all.keys('type', 'key') expect(body.message.type).to.equal('error') cb() } diff --git a/services/web/test/acceptance/src/SettingsTests.js b/services/web/test/acceptance/src/SettingsTests.js index fd81c0a527..a0ba1ce8cf 100644 --- a/services/web/test/acceptance/src/SettingsTests.js +++ b/services/web/test/acceptance/src/SettingsTests.js @@ -11,6 +11,7 @@ const { expect } = require('chai') const async = require('async') const User = require('./helpers/User') +const Features = require('../../../app/src/infrastructure/Features') describe('SettingsPage', function () { beforeEach(function (done) { @@ -32,6 +33,10 @@ describe('SettingsPage', function () { }) it('update main email address', function (done) { + if (Features.externalAuthenticationSystemUsed()) { + this.skip() + return + } const newEmail = 'foo@bar.com' return this.user.updateSettings({ email: newEmail }, error => { expect(error).not.to.exist diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index e44973b8ae..6677c5a94e 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -156,10 +156,11 @@ class User { } if (response.statusCode !== 200) { return callback( - new Error( + new OError( `login failed: status=${ response.statusCode - } body=${JSON.stringify(body)}` + } body=${JSON.stringify(body)}`, + { response, body } ) ) } diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index a9a7aa953a..0a162be059 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -452,12 +452,12 @@ describe('AuthenticationController', function () { it('should not establish the login', function () { this.cb.callCount.should.equal(1) this.cb.calledWith(null, false) - // @res.body.should.exist - expect(this.cb.lastCall.args[2]).to.contain.all.keys(['text', 'type']) + expect(this.cb.lastCall.args[2]).to.deep.equal({ + type: 'error', + key: 'invalid-password-retry-or-reset', + status: 401, + }) }) - // message: - // text: 'Your email or password were incorrect. Please try again', - // type: 'error' it('should not setup the user data in the background', function () { this.UserHandler.setupLoginData.called.should.equal(false) diff --git a/services/web/test/unit/src/User/UserDeleterTests.js b/services/web/test/unit/src/User/UserDeleterTests.js index 1682031ea8..2550ec0f8c 100644 --- a/services/web/test/unit/src/User/UserDeleterTests.js +++ b/services/web/test/unit/src/User/UserDeleterTests.js @@ -178,40 +178,40 @@ describe('UserDeleter', function () { }) it('should find and the user in mongo by its id', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) this.UserMock.verify() }) it('should delete the user from mailchimp', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect( this.NewsletterManager.promises.unsubscribe ).to.have.been.calledWith(this.user, { delete: true }) }) it('should delete all the projects of a user', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect( this.ProjectDeleter.promises.deleteUsersProjects ).to.have.been.calledWith(this.userId) }) it("should cancel the user's subscription", async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect( this.SubscriptionHandler.promises.cancelSubscription ).to.have.been.calledWith(this.user) }) it('should delete user affiliations', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect( this.InstitutionsApi.promises.deleteAffiliations ).to.have.been.calledWith(this.userId) }) it('should fire the deleteUser hook for modules', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect(this.Modules.promises.hooks.fire).to.have.been.calledWith( 'deleteUser', this.userId @@ -219,21 +219,21 @@ describe('UserDeleter', function () { }) it('should stop the user sessions', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect( this.UserSessionsManager.promises.revokeAllUserSessions ).to.have.been.calledWith(this.userId, []) }) it('should remove user from group subscriptions', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect( this.SubscriptionUpdater.promises.removeUserFromAllGroups ).to.have.been.calledWith(this.userId) }) it('should remove user memberships', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) expect( this.UserMembershipsHandler.promises.removeUserFromAllEntities ).to.have.been.calledWith(this.userId) @@ -243,12 +243,12 @@ describe('UserDeleter', function () { this.SubscriptionLocator.promises.getUsersSubscription.rejects({ _id: 'some-subscription', }) - await expect(this.UserDeleter.promises.deleteUser(this.userId)).to - .be.rejected + await expect(this.UserDeleter.promises.deleteUser(this.userId, {})) + .to.be.rejected }) it('should create a deletedUser', async function () { - await this.UserDeleter.promises.deleteUser(this.userId) + await this.UserDeleter.promises.deleteUser(this.userId, {}) this.DeletedUserMock.verify() }) }) @@ -261,8 +261,8 @@ describe('UserDeleter', function () { }) it('should return an error and not delete the user', async function () { - await expect(this.UserDeleter.promises.deleteUser(this.userId)).to - .be.rejected + await expect(this.UserDeleter.promises.deleteUser(this.userId, {})) + .to.be.rejected this.UserMock.verify() }) }) @@ -276,7 +276,7 @@ describe('UserDeleter', function () { }) it('should delete the user', function (done) { - this.UserDeleter.deleteUser(this.userId, err => { + this.UserDeleter.deleteUser(this.userId, {}, err => { expect(err).not.to.exist this.UserMock.verify() this.DeletedUserMock.verify()