github/overleaf
1
0
Fork 0
mirror of https://github.com/overleaf/overleaf.git synced 2025-04-15 19:37:30 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #2259 from overleaf/bg-revert-session-merge

Browse source 
Revert "Merge pull request #2249 from overleaf/bg-create-session-vali…

GitOrigin-RevId: d2114ecea0708dc109d5c9256e9dccb011a1b62c
This commit is contained in:
Brian Gough 2019-10-18 10:04:08 +01:00 • committed by sharelatex
parent bc8ccf26c7
commit e502b80116
5 changed files with 80 additions and 141 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

52
services/web/app/src/Features/Authentication/AuthenticationController.js
View file

@ -8,7 +8,6 @@ const Settings = require('settings-sharelatex')
const basicAuth = require('basic-auth-connect')
const UserHandler = require('../User/UserHandler')
const UserSessionsManager = require('../User/UserSessionsManager')
const SessionStoreManager = require('../../infrastructure/SessionStoreManager')
const Analytics = require('../Analytics/AnalyticsManager')
const passport = require('passport')
const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
@ -43,6 +42,51 @@ const AuthenticationController = (module.exports = {
cb(null, user)
},
computeSessionValidationToken(req) {
// this should be a deterministic function of the client-side sessionID,
// prepended with a version number in case we want to change it later
return 'v1:' + req.sessionID.slice(-4)
},
setSessionValidationToken(req) {
req.session.validationToken = AuthenticationController.computeSessionValidationToken(
req
)
},
checkSessionValidationToken(req) {
if (req.session) {
const token = req.session.validationToken
if (token) {
const clientToken = AuthenticationController.computeSessionValidationToken(
req
)
// Reject invalid sessions. If you change the method for computing the
// token (above) then you need to either check or ignore previous
// versions of the token.
if (token === clientToken) {
Metrics.inc('security.session', 1, { status: 'ok' })
return true
} else {
logger.error(
{
token: token,
clientToken: clientToken,
req: req,
sessionID: req.sessionID
},
'session token validation failed'
)
Metrics.inc('security.session', 1, { status: 'error' })
return false
}
} else {
Metrics.inc('security.session', 1, { status: 'missing' })
}
}
return true // fallback to allowing session
},
afterLoginSessionSetup(req, user, callback) {
if (callback == null) {
callback = function() {}
@ -64,14 +108,14 @@ const AuthenticationController = (module.exports = {
return callback(err)
}
req.sessionStore.generate(req)
// Note: the validation token is not writable, so it does not get
// transferred to the new session below.
for (let key in oldSession) {
const value = oldSession[key]
if (key !== '__tmp') {
req.session[key] = value
}
}
// generate an identifier from the sessionID
AuthenticationController.setSessionValidationToken(req)
req.session.save(function(err) {
if (err) {
logger.warn(
@ -255,7 +299,7 @@ const AuthenticationController = (module.exports = {
},
getSessionUser(req) {
if (!SessionStoreManager.checkValidationToken(req)) {
if (!AuthenticationController.checkSessionValidationToken(req)) {
return null
}
const sessionUser = _.get(req, ['session', 'user'])

4
services/web/app/src/infrastructure/Server.js
View file

@ -13,7 +13,6 @@ const Csrf = require('./Csrf')
const sessionsRedisClient = UserSessionsRedis.client()
const SessionStoreManager = require('./SessionStoreManager')
const session = require('express-session')
const RedisStore = require('connect-redis')(session)
const bodyParser = require('body-parser')
@ -112,9 +111,6 @@ webRouter.use(
})
)
// patch the session store to generate a validation token for every new session
SessionStoreManager.enableValidationToken(sessionStore)
// passport
webRouter.use(passport.initialize())
webRouter.use(passport.session())

54
services/web/app/src/infrastructure/SessionStoreManager.js
View file

@ -1,54 +0,0 @@
const Metrics = require('metrics-sharelatex')
const logger = require('logger-sharelatex')
function computeValidationToken(req) {
// this should be a deterministic function of the client-side sessionID,
// prepended with a version number in case we want to change it later
return 'v1:' + req.sessionID.slice(-4)
}
module.exports = {
enableValidationToken(sessionStore) {
// generate an identifier from the sessionID for every new session
const originalGenerate = sessionStore.generate
sessionStore.generate = function(req) {
originalGenerate(req)
// add the validation token as a property that cannot be overwritten
Object.defineProperty(req.session, 'validationToken', {
value: computeValidationToken(req),
enumerable: true,
writable: false
})
Metrics.inc('security.session', 1, { status: 'new' })
}
},
checkValidationToken(req) {
if (req.session) {
const sessionToken = req.session.validationToken
if (sessionToken) {
const clientToken = computeValidationToken(req)
// Reject invalid sessions. If you change the method for computing the
// token (above) then you need to either check or ignore previous
// versions of the token.
if (sessionToken === clientToken) {
Metrics.inc('security.session', 1, { status: 'ok' })
return true
} else {
logger.error(
{
sessionToken: sessionToken,
clientToken: clientToken
},
'session token validation failed'
)
Metrics.inc('security.session', 1, { status: 'error' })
return false
}
} else {
Metrics.inc('security.session', 1, { status: 'missing' })
}
}
return true // fallback to allowing session
}
}

41
services/web/test/unit/src/Authentication/AuthenticationControllerTests.js
View file

@ -48,9 +48,6 @@ describe('AuthenticationController', function() {
untrackSession: sinon.stub(),
revokeAllUserSessions: sinon.stub().callsArgWith(1, null)
}),
'../../infrastructure/SessionStoreManager': (this.SessionStoreManager = {
checkValidationToken: sinon.stub().returns(true)
}),
'../../infrastructure/Modules': (this.Modules = {
hooks: { fire: sinon.stub().callsArgWith(2, null, []) }
}),
@ -299,6 +296,16 @@ describe('AuthenticationController', function() {
})
})
it('should set a vaildation token on the session', function(done) {
this.call(err => {
if (err) {
return done(err)
}
expect(this.req.session).to.have.property('validationToken', 'v1:omid')
done()
})
})
describe('when req.session.save produces an error', function() {
beforeEach(function() {
this.req.session.save = sinon.stub().callsArgWith(0, new Error('woops'))
@ -323,29 +330,45 @@ describe('AuthenticationController', function() {
})
describe('getSessionUser', function() {
it('should accept a valid session', function() {
it('should accept a session without a validation token', function() {
this.req.session = {
passport: {
user: { _id: 'one' }
}
}
this.SessionStoreManager.checkValidationToken = sinon.stub().returns(true)
const user = this.AuthenticationController.getSessionUser(this.req)
expect(user).to.deep.equal({ _id: 'one' })
})
it('should reject an invalid session', function() {
it('should reject an invalid validation token', function() {
this.req.sessionID = 'nabuchodonosorroidebabylone'
this.req.session = {
validationToken: 'v1:gasp', // does not match last 4 characters of session id
passport: {
user: { _id: 'two' }
}
}
this.SessionStoreManager.checkValidationToken = sinon
.stub()
.returns(false)
const user = this.AuthenticationController.getSessionUser(this.req)
expect(user).to.be.null
})
it('should accept a valid validation token', function() {
this.req.sessionID = 'nabuchodonosorroidebabylone'
this.req.session = {
validationToken: 'v1:lone', // matches last 4 characters of session id
passport: {
user: { _id: 'three' }
}
}
const user = this.AuthenticationController.getSessionUser(this.req)
expect(user).to.deep.equal({ _id: 'three' })
})
it('should work with legacy sessions', function() {
this.req.session = { user: { _id: 'one' } }
const user = this.AuthenticationController.getSessionUser(this.req)
expect(user).to.deep.equal({ _id: 'one' })
})
})
describe('doPassportLogin', function() {

70
services/web/test/unit/src/Security/SessionStoreManagerTests.js
View file

@ -1,70 +0,0 @@
const sinon = require('sinon')
const chai = require('chai')
const { expect } = chai
const modulePath = '../../../../app/src/infrastructure/SessionStoreManager.js'
const SandboxedModule = require('sandboxed-module')
describe('SessionStoreManager', function() {
beforeEach(function() {
this.SessionStoreManager = SandboxedModule.require(modulePath, {
globals: {
console: console
},
requires: {
'metrics-sharelatex': (this.Metrics = { inc: sinon.stub() }),
'logger-sharelatex': (this.logger = {
log: sinon.stub(),
warn: sinon.stub(),
error: sinon.stub(),
err: sinon.stub()
})
}
})
this.sessionStore = {
generate: sinon.spy(req => {
req.session = {}
})
}
})
describe('enableValidationToken', function() {
beforeEach(function() {
this.originalGenerate = this.sessionStore.generate
this.SessionStoreManager.enableValidationToken(this.sessionStore)
})
it('should set up a wrapper around the generate function', function() {
expect(this.sessionStore.generate).to.not.equal(this.originalGenerate)
})
it('should add a validationToken when the generate function is called', function() {
this.req = { sessionID: '123456789' }
this.sessionStore.generate(this.req)
expect(this.req.session.validationToken).to.equal('v1:6789')
})
it('should not allow the token to be overwritten', function() {
this.req = { sessionID: '123456789' }
this.sessionStore.generate(this.req)
this.req.session.validationToken = 'try-to-overwrite-token'
expect(this.req.session.validationToken).to.equal('v1:6789')
})
})
describe('checkValidationToken', function() {
this.beforeEach(function() {
this.SessionStoreManager.enableValidationToken(this.sessionStore)
this.req = { sessionID: '123456789' }
this.sessionStore.generate(this.req)
})
it('should return true when the session id matches the validation token', function() {
const result = this.SessionStoreManager.checkValidationToken(this.req)
expect(result).to.equal(true)
})
it('should return false when the session id has changed', function() {
this.req.sessionID = 'abcdefghijklmnopqrstuvwxyz'
const result = this.SessionStoreManager.checkValidationToken(this.req)
expect(result).to.equal(false)
})
it('should return true when the session does not have a validation token', function() {
this.req = { sessionID: '123456789', session: {} }
const result = this.SessionStoreManager.checkValidationToken(this.req)
expect(result).to.equal(true)
})
})
})