Merge pull request #17810 from overleaf/dp-compormised-password-prompt

Add compromised password prompt

GitOrigin-RevId: 7910a220943fcb3aa191da6d514d5bc3ae20f5a3
This commit is contained in:
David 2024-04-18 10:58:57 +01:00 committed by Copybot
parent b2ef7a935f
commit 0cf17478fe
16 changed files with 327 additions and 126 deletions

View file

@ -178,6 +178,7 @@ const AuthenticationController = {
const redir =
AuthenticationController._getRedirectFromSession(req) || '/project'
_loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser)
const userId = user._id
UserAuditLogHandler.addEntry(
@ -241,8 +242,10 @@ const AuthenticationController = {
{ email },
password,
auditLog,
{ skipHIBPCheck: fromKnownDevice },
function (error, user) {
{
enforceHIBPCheck: !fromKnownDevice,
},
function (error, user, isPasswordReused) {
if (error != null) {
if (error instanceof ParallelLoginError) {
return done(null, false, { status: 429 })
@ -278,6 +281,16 @@ const AuthenticationController = {
status: 400,
})
} else if (user) {
if (
isPasswordReused &&
AuthenticationController._getRedirectFromSession(req) == null
) {
AuthenticationController.setRedirectInSession(
req,
'/compromised-password'
)
}
// async actions
done(null, user)
} else {

View file

@ -10,7 +10,10 @@ const {
PasswordMustBeDifferentError,
PasswordReusedError,
} = require('./AuthenticationErrors')
const { callbackify } = require('util')
const {
callbackify,
callbackifyMultiResult,
} = require('@overleaf/promise-utils')
const HaveIBeenPwned = require('./HaveIBeenPwned')
const UserAuditLogHandler = require('../User/UserAuditLogHandler')
const logger = require('@overleaf/logger')
@ -111,14 +114,14 @@ const AuthenticationManager = {
return { user, match }
},
async authenticate(query, password, auditLog, { skipHIBPCheck = false }) {
async authenticate(query, password, auditLog, { enforceHIBPCheck = true }) {
const { user, match } = await AuthenticationManager._checkUserPassword(
query,
password
)
if (!user) {
return null
return { user: null }
}
const update = { $inc: { loginEpoch: 1 } }
@ -138,7 +141,7 @@ const AuthenticationManager = {
if (!match) {
if (!auditLog) {
return null
return { user: null }
} else {
try {
await UserAuditLogHandler.promises.addEntry(
@ -154,16 +157,11 @@ const AuthenticationManager = {
'Error while adding AuditLog entry for failed-password-match'
)
}
return null
return { user: null }
}
}
await AuthenticationManager.checkRounds(user, user.hashedPassword, password)
if (skipHIBPCheck) {
HaveIBeenPwned.checkPasswordForReuseInBackground(password)
return user
}
let isPasswordReused
try {
isPasswordReused =
@ -172,11 +170,11 @@ const AuthenticationManager = {
logger.err({ err }, 'cannot check password for re-use')
}
if (isPasswordReused) {
if (isPasswordReused && enforceHIBPCheck) {
throw new PasswordReusedError()
}
return user
return { user, isPasswordReused }
},
validateEmail(email) {
@ -471,7 +469,10 @@ module.exports = {
validatePassword: AuthenticationManager.validatePassword,
getMessageForInvalidPasswordError:
AuthenticationManager.getMessageForInvalidPasswordError,
authenticate: callbackify(AuthenticationManager.authenticate),
authenticate: callbackifyMultiResult(AuthenticationManager.authenticate, [
'user',
'isPasswordReused',
]),
setUserPassword: callbackify(AuthenticationManager.setUserPassword),
checkRounds: callbackify(AuthenticationManager.checkRounds),
hashPassword: callbackify(AuthenticationManager.hashPassword),

View file

@ -70,11 +70,11 @@ async function changePassword(req, res, next) {
metrics.inc('user.password-change')
const userId = SessionManager.getLoggedInUserId(req.session)
const user = await AuthenticationManager.promises.authenticate(
const { user } = await AuthenticationManager.promises.authenticate(
{ _id: userId },
req.body.currentPassword,
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
if (!user) {
return HttpErrorHandler.badRequest(
@ -228,11 +228,11 @@ async function tryDeleteUser(req, res, next) {
return res.sendStatus(403)
}
const user = await AuthenticationManager.promises.authenticate(
const { user } = await AuthenticationManager.promises.authenticate(
{ _id: userId },
password,
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
if (!user) {
logger.err({ userId }, 'auth failed during attempt to delete account')

View file

@ -314,6 +314,10 @@ const UserPagesController = {
)
},
compromisedPasswordPage(_, res) {
res.render('user/compromised_password')
},
_restructureThirdPartyIds(user) {
// 3rd party identifiers are an array of objects
// this turn them into a single object, which

View file

@ -224,6 +224,12 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
AuthenticationController.passportLogin
)
webRouter.get(
'/compromised-password',
AuthenticationController.requireLogin(),
UserPagesController.compromisedPasswordPage
)
webRouter.get('/account-suspended', UserPagesController.accountSuspended)
if (Settings.enableLegacyLogin) {

View file

@ -33,6 +33,7 @@ html(
link(rel="preload", href=buildJsPath(currentLngCode + "-json.js"), as="script", nonce=scriptNonce)
//- Scripts
if (typeof(suppressGoogleAnalytics) == "undefined")
include _google_analytics
block meta

View file

@ -0,0 +1,15 @@
extends ../layout-marketing
block vars
- var suppressNavbar = true
- var suppressFooter = true
- var suppressGoogleAnalytics = true
block entrypointVar
- entrypoint = 'pages/compromised-password'
block append meta
block content
main.content.content-alt
#compromised-password

View file

@ -190,6 +190,7 @@
"compile_terminated_by_user": "",
"compiler": "",
"compiling": "",
"compromised_password": "",
"configure_sso": "",
"confirm": "",
"confirm_affiliation": "",
@ -480,6 +481,7 @@
"go_next_page": "",
"go_page": "",
"go_prev_page": "",
"go_to_account_settings": "",
"go_to_code_location_in_pdf": "",
"go_to_overleaf": "",
"go_to_pdf_location_in_code": "",
@ -884,6 +886,7 @@
"plan_tooltip": "",
"please_ask_the_project_owner_to_upgrade_to_track_changes": "",
"please_change_primary_to_remove": "",
"please_change_your_password_by_going_to_your_account_settings": "",
"please_check_your_inbox": "",
"please_check_your_inbox_to_confirm": "",
"please_compile_pdf_before_download": "",
@ -1566,6 +1569,7 @@
"your_git_access_tokens": "",
"your_message_to_collaborators": "",
"your_new_plan": "",
"your_password_was_detected": "",
"your_plan": "",
"your_plan_is_changing_at_term_end": "",
"your_project_exceeded_compile_timeout_limit_on_free_plan": "",

View file

@ -0,0 +1,46 @@
import useWaitForI18n from '@/shared/hooks/use-wait-for-i18n'
import { Button } from 'react-bootstrap'
import { Trans, useTranslation } from 'react-i18next'
export function CompromisedPasswordCard() {
const { t } = useTranslation()
const { isReady } = useWaitForI18n()
if (!isReady) {
return null
}
return (
<div className="compromised-password">
<div>
<h3 className="compromised-password-header">
{t('compromised_password')}
</h3>
<p>
<Trans
i18nKey="your_password_was_detected"
components={[
/* eslint-disable-next-line jsx-a11y/anchor-has-content, react/jsx-key */
<a
href="https://haveibeenpwned.com/passwords"
target="_blank"
rel="noreferrer"
/>,
]}
/>
</p>
<p>
<Trans
i18nKey="please_change_your_password_by_going_to_your_account_settings"
/* eslint-disable-next-line jsx-a11y/anchor-has-content, react/jsx-key */
components={[<a href="/user/settings" />]}
/>
</p>
</div>
<Button className="btn-primary" href="/user/settings">
{t('go_to_account_settings')}
</Button>
</div>
)
}

View file

@ -0,0 +1,12 @@
import '../marketing'
import ReactDOM from 'react-dom'
import { CompromisedPasswordCard } from '../features/compromised-password/components/compromised-password-root'
const compromisedPasswordContainer = document.getElementById(
'compromised-password'
)
if (compromisedPasswordContainer) {
ReactDOM.render(<CompromisedPasswordCard />, compromisedPasswordContainer)
}

View file

@ -154,3 +154,17 @@
margin-bottom: 100px;
}
}
.compromised-password {
max-width: 400px;
padding: 24px;
margin: 0 auto;
background: @white;
display: flex;
flex-direction: column;
gap: 16px;
.compromised-password-header {
margin-top: 0;
}
}

View file

@ -284,6 +284,7 @@
"compiling": "Compiling",
"complete": "Complete",
"compliance": "Compliance",
"compromised_password": "Compromised Password",
"configure_sso": "Configure SSO",
"configured": "Configured",
"confirm": "Confirm",
@ -1319,6 +1320,7 @@
"plans_and_pricing": "Plans and Pricing",
"please_ask_the_project_owner_to_upgrade_to_track_changes": "Please ask the project owner to upgrade to use track changes",
"please_change_primary_to_remove": "Please change your primary email in order to remove",
"please_change_your_password_by_going_to_your_account_settings": "Please change your password by going to your <0>Account Settings</0>.",
"please_check_your_inbox": "Please check your inbox",
"please_check_your_inbox_to_confirm": "Please check your email inbox to confirm your <0>__institutionName__</0> affiliation.",
"please_compile_pdf_before_download": "Please compile your project before downloading the PDF",
@ -2208,6 +2210,7 @@
"your_message_to_collaborators": "Send a message to your collaborators",
"your_new_plan": "Your new plan",
"your_password_has_been_successfully_changed": "Your password has been successfully changed",
"your_password_was_detected": "Your password was detected on a <0>public list of known compromised passwords</0>. This makes your account vulnerable.",
"your_plan": "Your plan",
"your_plan_is_changing_at_term_end": "Your plan is changing to <0>__pendingPlanName__</0> at the end of the current billing period.",
"your_project_exceeded_compile_timeout_limit_on_free_plan": "Your project exceeded the compile timeout limit on our free plan.",

View file

@ -57,6 +57,14 @@ describe('Captcha', function () {
expect(body).to.deep.equal({ redir: '/project' })
}
function expectSuccessfulLoginWithRedirectToCompromisedPasswordPage(
response,
body
) {
expect(response.statusCode).to.equal(200)
expect(body).to.deep.equal({ redir: '/compromised-password' })
}
function expectBadLogin(response, body) {
expect(response.statusCode).to.equal(401)
expect(body).to.deep.equal({
@ -226,12 +234,18 @@ describe('Captcha', function () {
})
it('should be able to skip HIBP check with deviceHistory and valid captcha', async function () {
const { response, body } = await loginWithCaptcha('valid')
expectSuccessfulLogin(response, body)
expectSuccessfulLoginWithRedirectToCompromisedPasswordPage(
response,
body
)
})
it('should be able to skip HIBP check with deviceHistory and skipped captcha', async function () {
const { response, body } = await loginWithCaptcha('')
expectSuccessfulLogin(response, body)
expectSuccessfulLoginWithRedirectToCompromisedPasswordPage(
response,
body
)
})
it('should not be able to skip HIBP check without deviceHistory', async function () {

View file

@ -16,11 +16,12 @@ describe('UserHelper', function () {
it('should create new user with default username and password', async function () {
const userHelper = await UserHelper.createUser()
userHelper.user.email.should.equal(userHelper.getDefaultEmail())
const authedUser = await AuthenticationManager.promises.authenticate(
const { user: authedUser } =
await AuthenticationManager.promises.authenticate(
{ _id: userHelper.user._id },
userHelper.getDefaultPassword(),
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
expect(authedUser).to.not.be.null
})
@ -32,11 +33,12 @@ describe('UserHelper', function () {
email: 'foo@test.com',
})
userHelper.user.email.should.equal('foo@test.com')
const authedUser = await AuthenticationManager.promises.authenticate(
const { user: authedUser } =
await AuthenticationManager.promises.authenticate(
{ _id: userHelper.user._id },
userHelper.getDefaultPassword(),
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
expect(authedUser).to.not.be.null
})
@ -48,11 +50,12 @@ describe('UserHelper', function () {
password: 'foofoofoo',
})
userHelper.user.email.should.equal(userHelper.getDefaultEmail())
const authedUser = await AuthenticationManager.promises.authenticate(
const { user: authedUser } =
await AuthenticationManager.promises.authenticate(
{ _id: userHelper.user._id },
'foofoofoo',
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
expect(authedUser).to.not.be.null
})
@ -128,11 +131,12 @@ describe('UserHelper', function () {
it('should create new user with default username and password', async function () {
const userHelper = await UserHelper.registerUser()
userHelper.user.email.should.equal(userHelper.getDefaultEmail())
const authedUser = await AuthenticationManager.promises.authenticate(
const { user: authedUser } =
await AuthenticationManager.promises.authenticate(
{ _id: userHelper.user._id },
userHelper.getDefaultPassword(),
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
expect(authedUser).to.not.be.null
})
@ -144,11 +148,12 @@ describe('UserHelper', function () {
email: 'foo2@test.com',
})
userHelper.user.email.should.equal('foo2@test.com')
const authedUser = await AuthenticationManager.promises.authenticate(
const { user: authedUser } =
await AuthenticationManager.promises.authenticate(
{ _id: userHelper.user._id },
userHelper.getDefaultPassword(),
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
expect(authedUser).to.not.be.null
})
@ -160,11 +165,12 @@ describe('UserHelper', function () {
password: 'foofoofoo',
})
userHelper.user.email.should.equal(userHelper.getDefaultEmail())
const authedUser = await AuthenticationManager.promises.authenticate(
const { user: authedUser } =
await AuthenticationManager.promises.authenticate(
{ _id: userHelper.user._id },
'foofoofoo',
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
expect(authedUser).to.not.be.null
})

View file

@ -81,12 +81,13 @@ describe('AuthenticationManager', function () {
describe('when the hashed password matches', function () {
beforeEach(async function () {
this.unencryptedPassword = 'testpassword'
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should look up the correct user in the database', function () {
@ -119,12 +120,13 @@ describe('AuthenticationManager', function () {
describe('when the encrypted passwords do not match', function () {
beforeEach(async function () {
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
'notthecorrectpassword',
null,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should persist the login failure and bump epoch', function () {
@ -159,7 +161,7 @@ describe('AuthenticationManager', function () {
{ email: this.email },
'testpassword',
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
).to.be.rejectedWith(AuthenticationErrors.ParallelLoginError)
})
@ -177,7 +179,7 @@ describe('AuthenticationManager', function () {
{ email: this.email },
'notthecorrectpassword',
null,
{ skipHIBPCheck: true }
{ enforceHIBPCheck: false }
)
).to.be.rejectedWith(AuthenticationErrors.ParallelLoginError)
})
@ -249,12 +251,13 @@ describe('AuthenticationManager', function () {
this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf'
this.bcrypt.compare = sinon.stub().resolves(true)
this.bcrypt.getRounds = sinon.stub().returns(4)
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should look up the correct user in the database', function () {
@ -280,22 +283,62 @@ describe('AuthenticationManager', function () {
})
describe('HIBP', function () {
it('should not check HIBP if not requested', function () {
this.HaveIBeenPwned.promises.checkPasswordForReuse.should.not.have
.been.called
it('should enforce HIBP if requested', async function () {
this.HaveIBeenPwned.promises.checkPasswordForReuse.resolves(true)
await expect(
this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ enforceHIBPCheck: true }
)
).to.be.rejectedWith(AuthenticationErrors.PasswordReusedError)
})
it('should check HIBP if requested', async function () {
it('should check but not enforce HIBP if not requested', async function () {
this.HaveIBeenPwned.promises.checkPasswordForReuse.resolves(true)
const { user } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ skipHIBPCheck: false }
{ enforceHIBPCheck: false }
)
this.HaveIBeenPwned.promises.checkPasswordForReuse.should.have.been.calledWith(
this.unencryptedPassword
)
expect(user).to.equal(this.user)
})
it('should report password reused when check not enforced', async function () {
this.HaveIBeenPwned.promises.checkPasswordForReuse.resolves(true)
const { isPasswordReused } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ enforceHIBPCheck: false }
)
expect(isPasswordReused).to.equal(true)
})
it('should report password not reused when check not enforced', async function () {
this.HaveIBeenPwned.promises.checkPasswordForReuse.resolves(false)
const { isPasswordReused } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ enforceHIBPCheck: false }
)
expect(isPasswordReused).to.equal(false)
})
})
})
@ -304,12 +347,13 @@ describe('AuthenticationManager', function () {
beforeEach(async function () {
this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf'
this.bcrypt.compare = sinon.stub().resolves(false)
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should not return the user', function () {
@ -323,12 +367,13 @@ describe('AuthenticationManager', function () {
this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf'
this.bcrypt.compare = sinon.stub().resolves(false)
this.auditLog = { ipAddress: 'ip', info: { method: 'foo' } }
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
this.auditLog,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should not return the user, but add entry to audit log', function () {
@ -354,12 +399,13 @@ describe('AuthenticationManager', function () {
this.AuthenticationManager.promises._setUserPasswordInMongo = sinon
.stub()
.resolves()
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should look up the correct user in the database', function () {
@ -400,12 +446,13 @@ describe('AuthenticationManager', function () {
this.AuthenticationManager.promises.setUserPassword = sinon
.stub()
.resolves()
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencryptedPassword,
null,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should not check the number of rounds', function () {
@ -433,12 +480,13 @@ describe('AuthenticationManager', function () {
this.User.findOne = sinon
.stub()
.returns({ exec: sinon.stub().resolves(null) })
this.result = await this.AuthenticationManager.promises.authenticate(
;({ user: this.result } =
await this.AuthenticationManager.promises.authenticate(
{ email: this.email },
this.unencrpytedPassword,
null,
{ skipHIBPCheck: true }
)
{ enforceHIBPCheck: false }
))
})
it('should not return a user', function () {

View file

@ -178,7 +178,9 @@ describe('UserController', function () {
this.SessionManager.getLoggedInUserId = sinon
.stub()
.returns(this.user._id)
this.AuthenticationManager.promises.authenticate.resolves(this.user)
this.AuthenticationManager.promises.authenticate.resolves({
user: this.user,
})
})
it('should send 200', function (done) {
@ -246,7 +248,9 @@ describe('UserController', function () {
describe('when authenticate does not produce a user', function () {
beforeEach(function () {
this.AuthenticationManager.promises.authenticate.resolves(null)
this.AuthenticationManager.promises.authenticate.resolves({
user: null,
})
})
it('should return 403', function (done) {
@ -696,7 +700,9 @@ describe('UserController', function () {
describe('changePassword', function () {
describe('success', function () {
beforeEach(function () {
this.AuthenticationManager.promises.authenticate.resolves(this.user)
this.AuthenticationManager.promises.authenticate.resolves({
user: this.user,
})
this.AuthenticationManager.promises.setUserPassword.resolves()
this.req.body = {
newPassword1: 'newpass',
@ -759,7 +765,7 @@ describe('UserController', function () {
describe('errors', function () {
it('should check the old password is the current one at the moment', function (done) {
this.AuthenticationManager.promises.authenticate.resolves()
this.AuthenticationManager.promises.authenticate.resolves({})
this.req.body = { currentPassword: 'oldpasshere' }
this.HttpErrorHandler.badRequest.callsFake(() => {
expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith(
@ -780,7 +786,9 @@ describe('UserController', function () {
})
it('it should not set the new password if they do not match', function (done) {
this.AuthenticationManager.promises.authenticate.resolves({})
this.AuthenticationManager.promises.authenticate.resolves({
user: this.user,
})
this.req.body = {
newPassword1: '1',
newPassword2: '2',
@ -813,7 +821,9 @@ describe('UserController', function () {
message
)
this.AuthenticationManager.promises.setUserPassword.rejects(err)
this.AuthenticationManager.promises.authenticate.resolves({})
this.AuthenticationManager.promises.authenticate.resolves({
user: this.user,
})
this.req.body = {
newPassword1: 'newpass',
newPassword2: 'newpass',
@ -831,7 +841,9 @@ describe('UserController', function () {
describe('UserAuditLogHandler error', function () {
it('should return error and not update password', function (done) {
this.UserAuditLogHandler.promises.addEntry.rejects(new Error('oops'))
this.AuthenticationManager.promises.authenticate.resolves(this.user)
this.AuthenticationManager.promises.authenticate.resolves({
user: this.user,
})
this.AuthenticationManager.promises.setUserPassword.resolves()
this.req.body = {
newPassword1: 'newpass',
@ -851,7 +863,9 @@ describe('UserController', function () {
describe('EmailHandler error', function () {
const anError = new Error('oops')
beforeEach(function () {
this.AuthenticationManager.promises.authenticate.resolves(this.user)
this.AuthenticationManager.promises.authenticate.resolves({
user: this.user,
})
this.AuthenticationManager.promises.setUserPassword.resolves()
this.req.body = {
newPassword1: 'newpass',