agw/overleaf-cep
1
0
Fork 0
mirror of https://github.com/yu-i-i/overleaf-cep.git synced 2026-11-14 22:23:42 +01:00
Code Issues Packages Projects Releases Wiki Activity

[web] Password set/reset: reject current password (redux) (#8956)

Browse Source 
* [web] set-password: reject same as current password

* [web] Add 'peek' operation on tokens

This allows us to improve the UX of the reset-password form,
by not invalidating the token in the case where the new
password will be rejected by validation logic.

We give up to three attempts before invalidating the token.

* [web] Add hide-on-error feature to async forms

This allows us to hide the form elements when certain
named error conditions occur.

* [web] reset-password: handle same-password rejection

We also change the implementation to use the new
peekValueFromToken API, and to expire the token explicitely
after it has been used to set the new password.

* [web] Validate OneTimeToken when loading password reset form

* [web] Rate limit GET: /user/password/set

Now that we are peeking at OneTimeToken when accessing this page,
we add rate to the GET request, matching that of the POST request.

* [web] Tidy up pug layout and mongo query for token peeking

Co-authored-by: Mathias Jakobsen <mathias.jakobsen@overleaf.com>
GitOrigin-RevId: 835205cc7c7ebe1209ee8e5b693efeb939a3056a
This commit is contained in:
June Kelly
2022-09-28 08:06:54 +00:00
committed by Copybot
co-authored by Mathias Jakobsen
parent b5e2604041
commit 3288f87dbe
18 changed files with 568 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
services/web/test/acceptance/src/PasswordResetTests.js
+113 -2
View File
@@ -227,8 +227,119 @@ describe('PasswordReset', function () {
const auditLog = userHelper.getAuditLogWithoutNoise()
expect(auditLog.length).to.equal(1)
})
it('when the password is the same as current, should return 400 and log the change', async function () {
// send reset request
response = await userHelper.request.post('/user/password/set', {
form: {
passwordResetToken: token,
password: userHelper.getDefaultPassword(),
},
simple: false,
})
expect(response.statusCode).to.equal(400)
expect(JSON.parse(response.body).message.key).to.equal(
'password-must-be-different'
)
userHelper = await UserHelper.getUser({ email })
const auditLog = userHelper.getAuditLogWithoutNoise()
expect(auditLog.length).to.equal(1)
})
})
})
describe('multiple attempts to set the password, reaching attempt limit', async function () {
beforeEach(async function () {
response = await userHelper.request.get(
`/user/password/set?passwordResetToken=${token}&email=${email}`,
{ simple: false }
)
expect(response.statusCode).to.equal(302)
expect(response.headers.location).to.equal(
`/user/password/set${emailQuery}`
)
})
it('should allow multiple attempts with same-password error, then deny further attempts', async function () {
const sendSamePasswordRequest = async function () {
return userHelper.request.post('/user/password/set', {
form: {
passwordResetToken: token,
password: userHelper.getDefaultPassword(),
},
simple: false,
})
}
// Three attempts at setting the password, all rejected for being the same as
// the current password
const response1 = await sendSamePasswordRequest()
expect(response1.statusCode).to.equal(400)
expect(JSON.parse(response1.body).message.key).to.equal(
'password-must-be-different'
)
const response2 = await sendSamePasswordRequest()
expect(response2.statusCode).to.equal(400)
expect(JSON.parse(response2.body).message.key).to.equal(
'password-must-be-different'
)
const response3 = await sendSamePasswordRequest()
expect(response3.statusCode).to.equal(400)
expect(JSON.parse(response3.body).message.key).to.equal(
'password-must-be-different'
)
// Fourth attempt is rejected because the token has been used too many times
const response4 = await sendSamePasswordRequest()
expect(response4.statusCode).to.equal(404)
expect(JSON.parse(response4.body).message.key).to.equal('token-expired')
})
it('should allow multiple attempts with same-password error, then set the password', async function () {
const sendSamePasswordRequest = async function () {
return userHelper.request.post('/user/password/set', {
form: {
passwordResetToken: token,
password: userHelper.getDefaultPassword(),
},
simple: false,
})
}
// Two attempts at setting the password, all rejected for being the same as
// the current password
const response1 = await sendSamePasswordRequest()
expect(response1.statusCode).to.equal(400)
expect(JSON.parse(response1.body).message.key).to.equal(
'password-must-be-different'
)
const response2 = await sendSamePasswordRequest()
expect(response2.statusCode).to.equal(400)
expect(JSON.parse(response2.body).message.key).to.equal(
'password-must-be-different'
)
// Third attempt is succeeds
const response3 = await userHelper.request.post('/user/password/set', {
form: {
passwordResetToken: token,
password: 'some-new-password',
},
simple: false,
})
expect(response3.statusCode).to.equal(200)
// Check the user and audit log
userHelper = await UserHelper.getUser({ email })
user = userHelper.user
expect(user.hashedPassword).to.exist
expect(user.password).to.not.exist
const auditLog = userHelper.getAuditLogWithoutNoise()
expect(auditLog).to.exist
expect(auditLog[0]).to.exist
expect(auditLog[0].initiatorId).to.equal(null)
expect(auditLog[0].operation).to.equal('reset-password')
expect(auditLog[0].ipAddress).to.equal('127.0.0.1')
expect(auditLog[0].timestamp).to.exist
})
})
describe('without a valid token', function () {
it('no token should redirect to page to re-request reset token', async function () {
response = await userHelper.request.get(
@@ -238,7 +349,7 @@ describe('PasswordReset', function () {
expect(response.statusCode).to.equal(302)
expect(response.headers.location).to.equal('/user/password/reset')
})
it('should return 404 for invalid tokens', async function () {
it('should show error for invalid tokens and return 404 if used', async function () {
const invalidToken = 'not-real-token'
response = await userHelper.request.get(
`/user/password/set?&passwordResetToken=${invalidToken}&email=${email}`,
@@ -246,7 +357,7 @@ describe('PasswordReset', function () {
)
expect(response.statusCode).to.equal(302)
expect(response.headers.location).to.equal(
`/user/password/set${emailQuery}`
`/user/password/reset?error=token_expired`
)
// send reset request
response = await userHelper.request.post('/user/password/set', {
services/web/test/acceptance/src/SessionTests.js
+1 -1
View File
@@ -256,7 +256,7 @@ describe('Sessions', function () {
// password reset from second session, should erase two of the three sessions
next => {
this.user2.changePassword(err => next(err))
this.user2.changePassword(`password${Date.now()}`, err => next(err))
},
next => {
services/web/test/acceptance/src/helpers/User.js
+13 -5
View File
@@ -152,7 +152,9 @@ class User {
this.setExtraAttributes(user)
AuthenticationManager.setUserPasswordInV2(user, this.password, error => {
if (error != null) {
return callback(error)
if (error.name !== 'PasswordMustBeDifferentError') {
return callback(error)
}
}
this.mongoUpdate({ $set: { emails: this.emails } }, error => {
if (error != null) {
@@ -675,7 +677,7 @@ class User {
)
}
changePassword(callback) {
changePassword(newPassword, callback) {
this.getCsrfToken(error => {
if (error != null) {
return callback(error)
@@ -685,11 +687,17 @@ class User {
url: '/user/password/update',
json: {
currentPassword: this.password,
newPassword1: this.password,
newPassword2: this.password,
newPassword1: newPassword,
newPassword2: newPassword,
},
},
callback
err => {
if (err) {
return callback(err)
}
this.password = newPassword
callback()
}
)
})
}
services/web/test/unit/src/Authentication/AuthenticationManagerTests.js
+27 -6
View File
@@ -180,7 +180,8 @@ describe('AuthenticationManager', function () {
email: (this.email = 'USER@sharelatex.com'),
}
this.db.users.updateOne = sinon
this.User.findOne = sinon.stub().callsArgWith(2, null, this.user)
this.User.findOne = sinon.stub().callsArgWith(1, null, this.user)
this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false)
this.db.users.updateOne = sinon
.stub()
.callsArgWith(2, null, { modifiedCount: 1 })
@@ -603,19 +604,39 @@ describe('AuthenticationManager', function () {
describe('setUserPassword', function () {
beforeEach(function () {
this.user_id = ObjectId()
this.user = {
_id: this.user_id,
email: 'user@example.com',
}
this.password = 'banana'
this.hashedPassword = 'asdkjfa;osiuvandf'
this.salt = 'saltaasdfasdfasdf'
this.user = {
_id: this.user_id,
email: 'user@example.com',
hashedPassword: this.hashedPassword,
}
this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false)
this.bcrypt.genSalt = sinon.stub().callsArgWith(2, null, this.salt)
this.bcrypt.hash = sinon.stub().callsArgWith(2, null, this.hashedPassword)
this.User.findOne = sinon.stub().callsArgWith(2, null, this.user)
this.User.findOne = sinon.stub().callsArgWith(1, null, this.user)
this.db.users.updateOne = sinon.stub().callsArg(2)
})
describe('same as previous password', function () {
beforeEach(function () {
this.bcrypt.compare.callsArgWith(2, null, true)
})
it('should return an error', function (done) {
this.AuthenticationManager.setUserPassword(
this.user,
this.password,
err => {
expect(err).to.exist
expect(err.name).to.equal('PasswordMustBeDifferentError')
done()
}
)
})
})
describe('too long', function () {
beforeEach(function () {
this.settings.passwordStrengthOptions = {
services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js
+26
View File
@@ -39,6 +39,10 @@ describe('PasswordResetController', function () {
.stub()
.resolves({ found: true, reset: true, userID: this.user_id }),
},
getUserForPasswordResetToken: sinon
.stub()
.withArgs(this.token)
.yields(null, { _id: this.user_id }, 1),
}
this.UserSessionsManager = {
promises: {
@@ -372,6 +376,28 @@ describe('PasswordResetController', function () {
})
})
describe('with expired token in query', function () {
beforeEach(function () {
this.req.query.passwordResetToken = this.token
this.PasswordResetHandler.getUserForPasswordResetToken = sinon
.stub()
.withArgs(this.token)
.yields(null, { _id: this.user_id }, 0)
})
it('should redirect to the reset request page with an error message', function (done) {
this.res.redirect = path => {
path.should.equal('/user/password/reset?error=token_expired')
this.req.session.should.not.have.property('resetToken')
done()
}
this.res.render = (templatePath, options) => {
done('should not render')
}
this.PasswordResetController.renderSetPasswordForm(this.req, this.res)
})
})
describe('with token and email in query-string', function () {
beforeEach(function () {
this.req.query.passwordResetToken = this.token
services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js
+62 -4
View File
@@ -26,7 +26,8 @@ describe('PasswordResetHandler', function () {
this.settings = { siteUrl: 'https://www.overleaf.com' }
this.OneTimeTokenHandler = {
getNewToken: sinon.stub(),
getValueFromTokenAndExpire: sinon.stub(),
peekValueFromToken: sinon.stub(),
expireToken: sinon.stub(),
}
this.UserGetter = {
getUserByMainEmail: sinon.stub(),
@@ -188,7 +189,7 @@ describe('PasswordResetHandler', function () {
})
describe('when no data is found', function () {
beforeEach(function () {
this.OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, null)
this.OneTimeTokenHandler.peekValueFromToken.yields(null, null)
})
it('should return found == false and reset == false', function () {
@@ -210,7 +211,7 @@ describe('PasswordResetHandler', function () {
describe('when the token has a user_id and email', function () {
beforeEach(function () {
this.OneTimeTokenHandler.getValueFromTokenAndExpire
this.OneTimeTokenHandler.peekValueFromToken
.withArgs('password', this.token)
.yields(null, {
user_id: this.user._id,
@@ -219,6 +220,9 @@ describe('PasswordResetHandler', function () {
this.AuthenticationManager.promises.setUserPassword
.withArgs(this.user, this.password)
.resolves(true)
this.OneTimeTokenHandler.expireToken = sinon
.stub()
.callsArgWith(2, null)
})
describe('when no user is found with this email', function () {
@@ -238,6 +242,7 @@ describe('PasswordResetHandler', function () {
expect(err).to.not.exist
expect(found).to.be.false
expect(reset).to.be.false
expect(this.OneTimeTokenHandler.expireToken.callCount).to.equal(0)
done()
}
)
@@ -249,6 +254,7 @@ describe('PasswordResetHandler', function () {
this.UserGetter.getUserByMainEmail
.withArgs(this.email)
.yields(null, { _id: 'not-the-same', email: this.email })
this.OneTimeTokenHandler.expireToken.callsArgWith(2, null)
})
it('should return found == false and reset == false', function (done) {
@@ -261,6 +267,7 @@ describe('PasswordResetHandler', function () {
expect(err).to.not.exist
expect(found).to.be.false
expect(reset).to.be.false
expect(this.OneTimeTokenHandler.expireToken.callCount).to.equal(0)
done()
}
)
@@ -271,6 +278,9 @@ describe('PasswordResetHandler', function () {
describe('success', function () {
beforeEach(function () {
this.UserGetter.getUserByMainEmail.yields(null, this.user)
this.OneTimeTokenHandler.expireToken = sinon
.stub()
.callsArgWith(2, null)
})
it('should update the user audit log', function (done) {
@@ -308,6 +318,20 @@ describe('PasswordResetHandler', function () {
)
})
it('should expire the token', function (done) {
this.PasswordResetHandler.setNewUserPassword(
this.token,
this.password,
this.auditLog,
(_err, _result) => {
expect(this.OneTimeTokenHandler.expireToken.called).to.equal(
true
)
done()
}
)
})
describe('when logged in', function () {
beforeEach(function () {
this.auditLog.initiatorId = this.user_id
@@ -335,6 +359,30 @@ describe('PasswordResetHandler', function () {
})
describe('errors', function () {
describe('via setUserPassword', function () {
beforeEach(function () {
this.PasswordResetHandler.promises.getUserForPasswordResetToken =
sinon.stub().withArgs(this.token).resolves(this.user)
this.AuthenticationManager.promises.setUserPassword
.withArgs(this.user, this.password)
.rejects()
})
it('should return the error', function (done) {
this.PasswordResetHandler.setNewUserPassword(
this.token,
this.password,
this.auditLog,
(error, _result) => {
expect(error).to.exist
expect(
this.UserAuditLogHandler.promises.addEntry.callCount
).to.equal(1)
done()
}
)
})
})
describe('via UserAuditLogHandler', function () {
beforeEach(function () {
this.PasswordResetHandler.promises.getUserForPasswordResetToken =
@@ -367,7 +415,7 @@ describe('PasswordResetHandler', function () {
describe('when the token has a v1_user_id and email', function () {
beforeEach(function () {
this.user.overleaf = { id: 184 }
this.OneTimeTokenHandler.getValueFromTokenAndExpire
this.OneTimeTokenHandler.peekValueFromToken
.withArgs('password', this.token)
.yields(null, {
v1_user_id: this.user.overleaf.id,
@@ -376,6 +424,9 @@ describe('PasswordResetHandler', function () {
this.AuthenticationManager.promises.setUserPassword
.withArgs(this.user, this.password)
.resolves(true)
this.OneTimeTokenHandler.expireToken = sinon
.stub()
.callsArgWith(2, null)
})
describe('when no user is reset with this email', function () {
@@ -394,6 +445,9 @@ describe('PasswordResetHandler', function () {
const { reset, userId } = result
expect(err).to.not.exist
expect(reset).to.be.false
expect(this.OneTimeTokenHandler.expireToken.called).to.equal(
false
)
done()
}
)
@@ -418,6 +472,9 @@ describe('PasswordResetHandler', function () {
const { reset, userId } = result
expect(err).to.not.exist
expect(reset).to.be.false
expect(this.OneTimeTokenHandler.expireToken.called).to.equal(
false
)
done()
}
)
@@ -441,6 +498,7 @@ describe('PasswordResetHandler', function () {
expect(err).to.not.exist
expect(reset).to.be.true
expect(userId).to.equal(this.user._id)
expect(this.OneTimeTokenHandler.expireToken.called).to.equal(true)
done()
}
)
services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js
+85
View File
@@ -107,6 +107,90 @@ describe('OneTimeTokenHandler', function () {
})
})
describe('peekValueFromToken', function () {
describe('successfully', function () {
const data = 'some-mock-data'
beforeEach(function () {
this.db.tokens.findOneAndUpdate = sinon
.stub()
.yields(null, { value: { data } })
return this.OneTimeTokenHandler.peekValueFromToken(
'password',
'mock-token',
this.callback
)
})
it('should increment the peekCount', function () {
return this.db.tokens.findOneAndUpdate
.calledWith(
{
use: 'password',
token: 'mock-token',
expiresAt: { $gt: new Date() },
usedAt: { $exists: false },
peekCount: { $not: { $gte: this.OneTimeTokenHandler.MAX_PEEKS } },
},
{
$inc: { peekCount: 1 },
}
)
.should.equal(true)
})
it('should return the data', function () {
return this.callback.calledWith(null, data).should.equal(true)
})
})
describe('when a valid token is not found', function () {
beforeEach(function () {
this.db.tokens.findOneAndUpdate = sinon
.stub()
.yields(null, { value: null })
return this.OneTimeTokenHandler.peekValueFromToken(
'password',
'mock-token',
this.callback
)
})
it('should return a NotFoundError', function () {
return this.callback
.calledWith(sinon.match.instanceOf(Errors.NotFoundError))
.should.equal(true)
})
})
})
describe('expireToken', function () {
beforeEach(function () {
this.db.tokens.updateOne = sinon.stub().yields(null)
this.OneTimeTokenHandler.expireToken(
'password',
'mock-token',
this.callback
)
})
it('should expire the token', function () {
this.db.tokens.updateOne
.calledWith(
{
use: 'password',
token: 'mock-token',
},
{
$set: {
usedAt: new Date(),
},
}
)
.should.equal(true)
this.callback.calledWith(null).should.equal(true)
})
})
describe('getValueFromTokenAndExpire', function () {
describe('successfully', function () {
beforeEach(function () {
@@ -128,6 +212,7 @@ describe('OneTimeTokenHandler', function () {
token: 'mock-token',
expiresAt: { $gt: new Date() },
usedAt: { $exists: false },
peekCount: { $not: { $gte: this.OneTimeTokenHandler.MAX_PEEKS } },
},
{
$set: { usedAt: new Date() },