From bfa0e7cf8930312b1fa6a0192b923e2b0a0e572b Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 8 Nov 2016 15:32:36 +0000 Subject: [PATCH 1/5] WIP: start moving web sessions to cluster --- .../AuthenticationController.coffee | 26 ++- .../Features/User/UserSessionsManager.coffee | 6 +- .../Features/User/UserSessionsRedis.coffee | 18 ++ .../app/coffee/infrastructure/Server.coffee | 7 +- services/web/config/settings.defaults.coffee | 56 +++--- services/web/npm-shrinkwrap.json | 185 ++++++++++++------ services/web/package.json | 5 +- 7 files changed, 198 insertions(+), 105 deletions(-) create mode 100644 services/web/app/coffee/Features/User/UserSessionsRedis.coffee diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index f3faad6c69..3c19f0de25 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -32,21 +32,27 @@ module.exports = AuthenticationController = afterLoginSessionSetup: (req, user, callback=(err)->) -> req.login user, (err) -> + if err? + logger.err {user_id: user._id, err}, "error from req.login" + return callback(err) # Regenerate the session to get a new sessionID (cookie value) to # protect against session fixation attacks oldSession = req.session - req.session.destroy() - req.sessionStore.generate(req) - for key, value of oldSession - req.session[key] = value - # copy to the old `session.user` location, for backward-comptability - req.session.user = req.session.passport.user - req.session.save (err) -> + req.session.destroy (err) -> if err? - logger.err {user_id: user._id}, "error saving regenerated session after login" + logger.err {user_id: user._id, err}, "error when trying to destroy old session" return callback(err) - UserSessionsManager.trackSession(user, req.sessionID, () ->) - callback(null) + req.sessionStore.generate(req) + for key, value of oldSession + req.session[key] = value + # copy to the old `session.user` location, for backward-comptability + req.session.user = req.session.passport.user + req.session.save (err) -> + if err? + logger.err {user_id: user._id}, "error saving regenerated session after login" + return callback(err) + UserSessionsManager.trackSession(user, req.sessionID, () ->) + callback(null) passportLogin: (req, res, next) -> # This function is middleware which wraps the passport.authenticate middleware, diff --git a/services/web/app/coffee/Features/User/UserSessionsManager.coffee b/services/web/app/coffee/Features/User/UserSessionsManager.coffee index 23a9bcbb36..af3efd55cd 100644 --- a/services/web/app/coffee/Features/User/UserSessionsManager.coffee +++ b/services/web/app/coffee/Features/User/UserSessionsManager.coffee @@ -3,13 +3,15 @@ redis = require('redis-sharelatex') logger = require("logger-sharelatex") Async = require('async') _ = require('underscore') +UserSessionsRedis = require('./UserSessionsRedis') -rclient = redis.createClient(Settings.redis.web) +# rclient = redis.createClient(Settings.redis.web) +rclient = UserSessionsRedis.client() module.exports = UserSessionsManager = _sessionSetKey: (user) -> - return "UserSessions:#{user._id}" + return "UserSessions:{#{user._id}}" # mimic the key used by the express sessions _sessionKey: (sessionId) -> diff --git a/services/web/app/coffee/Features/User/UserSessionsRedis.coffee b/services/web/app/coffee/Features/User/UserSessionsRedis.coffee new file mode 100644 index 0000000000..2103f09fab --- /dev/null +++ b/services/web/app/coffee/Features/User/UserSessionsRedis.coffee @@ -0,0 +1,18 @@ +Settings = require 'settings-sharelatex' +redis = require 'redis-sharelatex' +ioredis = require 'ioredis' +logger = require 'logger-sharelatex' + +module.exports = Redis = + + client: () -> + + redisSessionsSettings = Settings.redis.websessions or Settings.redis.web + + if redisSessionsSettings?.cluster? + logger.log {}, "using redis cluster for web sessions" + rclient = new ioredis.Cluster(redisSessionsSettings.cluster) + else + rclient = redis.createClient(redisSessionsSettings) + + return rclient diff --git a/services/web/app/coffee/infrastructure/Server.coffee b/services/web/app/coffee/infrastructure/Server.coffee index e19d9c5ca8..1df566b571 100644 --- a/services/web/app/coffee/infrastructure/Server.coffee +++ b/services/web/app/coffee/infrastructure/Server.coffee @@ -8,7 +8,9 @@ expressLocals = require('./ExpressLocals') Router = require('../router') metrics.inc("startup") redis = require("redis-sharelatex") -rclient = redis.createClient(Settings.redis.web) +UserSessionsRedis = require('../Features/User/UserSessionsRedis') + +sessionsRedisClient = UserSessionsRedis.client() session = require("express-session") RedisStore = require('connect-redis')(session) @@ -19,7 +21,8 @@ csrf = require('csurf') csrfProtection = csrf() cookieParser = require('cookie-parser') -sessionStore = new RedisStore(client:rclient) +# Init the session store +sessionStore = new RedisStore(client:sessionsRedisClient) passport = require('passport') LocalStrategy = require('passport-local').Strategy diff --git a/services/web/config/settings.defaults.coffee b/services/web/config/settings.defaults.coffee index daf16ae1f4..1e858253e4 100644 --- a/services/web/config/settings.defaults.coffee +++ b/services/web/config/settings.defaults.coffee @@ -38,6 +38,16 @@ module.exports = settings = port: "6379" password: "" + # websessions: + # cluster: [ + # {host: 'localhost', port: 7000} + # {host: 'localhost', port: 7001} + # {host: 'localhost', port: 7002} + # {host: 'localhost', port: 7003} + # {host: 'localhost', port: 7004} + # {host: 'localhost', port: 7005} + # ] + api: host: "localhost" port: "6379" @@ -107,8 +117,8 @@ module.exports = settings = # references: # url: "http://localhost:3040" notifications: - url: "http://localhost:3042" - + url: "http://localhost:3042" + templates: user_id: process.env.TEMPLATES_USER_ID or "5395eb7aad1f29a88756c7f2" showSocialButtons: false @@ -131,13 +141,13 @@ module.exports = settings = # this is only used if cookies are used for clsi backend #clsiCookieKey: "clsiserver" - + # Same, but with http auth credentials. httpAuthSiteUrl: 'http://#{httpAuthUser}:#{httpAuthPass}@localhost:3000' maxEntitiesPerProject: 2000 - + # Security # -------- security: @@ -166,12 +176,12 @@ module.exports = settings = price: 0 features: defaultFeatures }] - + enableSubscriptions:false # i18n # ------ - # + # i18n: subdomainLang: www: {lngCode:"en", url: siteUrl} @@ -180,7 +190,7 @@ module.exports = settings = # Spelling languages # ------------------ # - # You must have the corresponding aspell package installed to + # You must have the corresponding aspell package installed to # be able to use a language. languages: [ {name: "English", code: "en"}, @@ -228,7 +238,7 @@ module.exports = settings = # analytics: # ga: # token: "" - # + # # ShareLaTeX's help desk is provided by tenderapp.com # tenderUrl: "" # @@ -262,10 +272,10 @@ module.exports = settings = # then set this to true to allow it to correctly detect the forwarded IP # address and http/https protocol information. behindProxy: false - + # Cookie max age (in milliseconds). Set to false for a browser session. cookieSessionLength: 5 * 24 * 60 * 60 * 1000 # 5 days - + # Should we allow access to any page without logging in? This includes # public projects, /learn, /templates, about pages, etc. allowPublicAccess: if process.env["SHARELATEX_ALLOW_PUBLIC_ACCESS"] == 'true' then true else false @@ -273,10 +283,10 @@ module.exports = settings = # Use a single compile directory for all users in a project # (otherwise each user has their own directory) # disablePerUserCompiles: true - + # Maximum size of text documents in the real-time editing system. max_doc_length: 2 * 1024 * 1024 # 2mb - + # Internal configs # ---------------- path: @@ -285,11 +295,11 @@ module.exports = settings = # them to disk here). dumpFolder: Path.resolve __dirname + "/../data/dumpFolder" uploadFolder: Path.resolve __dirname + "/../data/uploads" - + # Automatic Snapshots # ------------------- automaticSnapshots: - # How long should we wait after the user last edited to + # How long should we wait after the user last edited to # take a snapshot? waitTimeAfterLastEdit: 5 * minutes # Even if edits are still taking place, this is maximum @@ -305,13 +315,13 @@ module.exports = settings = # user: "" # password: "" # projectId: "" - + appName: "ShareLaTeX (Community Edition)" adminEmail: "placeholder@example.com" nav: title: "ShareLaTeX Community Edition" - + left_footer: [{ text: "Powered by ShareLaTeX © 2016" }] @@ -377,11 +387,11 @@ module.exports = settings = "/templates/index": "/templates/" proxyUrls: {} - + reloadModuleViewsOnEachRequest: true domainLicences: [ - + ] sixpack: @@ -390,12 +400,12 @@ module.exports = settings = # ---------- - + # LDAP # ---------- # Settings below use a working LDAP test server kindly provided by forumsys.com # When testing with forumsys.com use username = einstein and password = password - + # ldap : # host: 'ldap://ldap.forumsys.com' # dn: 'uid=:userKey,dc=example,dc=com' @@ -406,13 +416,13 @@ module.exports = settings = # placeholder: 'email@example.com' # emailAtt: 'mail' # anonymous: false - # adminDN: 'cn=read-only-admin,dc=example,dc=com' + # adminDN: 'cn=read-only-admin,dc=example,dc=com' # adminPW: 'password' # starttls: true # tlsOptions: # rejectUnauthorized: false # ca: ['/etc/ldap/ca_certs.pem'] - + #templateLinks: [{ # name : "CV projects", # url : "/templates/cv" @@ -420,5 +430,3 @@ module.exports = settings = # name : "all projects", # url: "/templates/all" #}] - - diff --git a/services/web/npm-shrinkwrap.json b/services/web/npm-shrinkwrap.json index 0d3f663c33..44d69e0eaa 100644 --- a/services/web/npm-shrinkwrap.json +++ b/services/web/npm-shrinkwrap.json @@ -274,12 +274,12 @@ "dependencies": { "bytes": { "version": "2.4.0", - "from": "https://registry.npmjs.org/bytes/-/bytes-2.4.0.tgz", + "from": "bytes@2.4.0", "resolved": "https://registry.npmjs.org/bytes/-/bytes-2.4.0.tgz" }, "content-type": { "version": "1.0.2", - "from": "https://registry.npmjs.org/content-type/-/content-type-1.0.2.tgz", + "from": "content-type@~1.0.1", "resolved": "https://registry.npmjs.org/content-type/-/content-type-1.0.2.tgz" }, "debug": { @@ -328,7 +328,7 @@ }, "on-finished": { "version": "2.3.0", - "from": "https://registry.npmjs.org/on-finished/-/on-finished-2.3.0.tgz", + "from": "on-finished@~2.3.0", "resolved": "https://registry.npmjs.org/on-finished/-/on-finished-2.3.0.tgz", "dependencies": { "ee-first": { @@ -345,7 +345,7 @@ }, "raw-body": { "version": "2.1.7", - "from": "https://registry.npmjs.org/raw-body/-/raw-body-2.1.7.tgz", + "from": "raw-body@~2.1.5", "resolved": "https://registry.npmjs.org/raw-body/-/raw-body-2.1.7.tgz", "dependencies": { "unpipe": { @@ -357,7 +357,7 @@ }, "type-is": { "version": "1.6.13", - "from": "https://registry.npmjs.org/type-is/-/type-is-1.6.13.tgz", + "from": "type-is@~1.6.10", "resolved": "https://registry.npmjs.org/type-is/-/type-is-1.6.13.tgz", "dependencies": { "media-typer": { @@ -387,26 +387,41 @@ "resolved": "https://registry.npmjs.org/bufferedstream/-/bufferedstream-1.6.0.tgz" }, "connect-redis": { - "version": "2.3.0", - "from": "https://registry.npmjs.org/connect-redis/-/connect-redis-2.3.0.tgz", - "resolved": "https://registry.npmjs.org/connect-redis/-/connect-redis-2.3.0.tgz", + "version": "3.1.0", + "from": "connect-redis@3.1.0", "dependencies": { "debug": { - "version": "1.0.4", - "from": "https://registry.npmjs.org/debug/-/debug-1.0.4.tgz", - "resolved": "https://registry.npmjs.org/debug/-/debug-1.0.4.tgz", + "version": "2.3.0", + "from": "debug@^2.2.0", + "resolved": "https://registry.npmjs.org/debug/-/debug-2.3.0.tgz", "dependencies": { "ms": { - "version": "0.6.2", - "from": "https://registry.npmjs.org/ms/-/ms-0.6.2.tgz", - "resolved": "https://registry.npmjs.org/ms/-/ms-0.6.2.tgz" + "version": "0.7.2", + "from": "ms@0.7.2" } } }, "redis": { - "version": "0.12.1", - "from": "https://registry.npmjs.org/redis/-/redis-0.12.1.tgz", - "resolved": "https://registry.npmjs.org/redis/-/redis-0.12.1.tgz" + "version": "2.6.3", + "from": "redis@^2.1.0", + "resolved": "https://registry.npmjs.org/redis/-/redis-2.6.3.tgz", + "dependencies": { + "double-ended-queue": { + "version": "2.1.0-0", + "from": "double-ended-queue@^2.1.0-0", + "resolved": "https://registry.npmjs.org/double-ended-queue/-/double-ended-queue-2.1.0-0.tgz" + }, + "redis-commands": { + "version": "1.3.0", + "from": "redis-commands@^1.2.0", + "resolved": "https://registry.npmjs.org/redis-commands/-/redis-commands-1.3.0.tgz" + }, + "redis-parser": { + "version": "2.1.1", + "from": "redis-parser@^2.0.0", + "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-2.1.1.tgz" + } + } } } }, @@ -470,7 +485,7 @@ }, "json-stringify-safe": { "version": "5.0.1", - "from": "https://registry.npmjs.org/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz", + "from": "json-stringify-safe@~5.0.0", "resolved": "https://registry.npmjs.org/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz" } } @@ -616,7 +631,7 @@ }, "content-type": { "version": "1.0.2", - "from": "https://registry.npmjs.org/content-type/-/content-type-1.0.2.tgz", + "from": "content-type@~1.0.1", "resolved": "https://registry.npmjs.org/content-type/-/content-type-1.0.2.tgz" }, "cookie": { @@ -631,7 +646,7 @@ }, "debug": { "version": "2.2.0", - "from": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz", + "from": "debug@~2.2.0", "resolved": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz", "dependencies": { "ms": { @@ -685,7 +700,7 @@ }, "on-finished": { "version": "2.3.0", - "from": "https://registry.npmjs.org/on-finished/-/on-finished-2.3.0.tgz", + "from": "on-finished@~2.3.0", "resolved": "https://registry.npmjs.org/on-finished/-/on-finished-2.3.0.tgz", "dependencies": { "ee-first": { @@ -829,7 +844,7 @@ }, "type-is": { "version": "1.6.13", - "from": "https://registry.npmjs.org/type-is/-/type-is-1.6.13.tgz", + "from": "type-is@~1.6.10", "resolved": "https://registry.npmjs.org/type-is/-/type-is-1.6.13.tgz", "dependencies": { "media-typer": { @@ -864,68 +879,61 @@ } }, "express-session": { - "version": "1.11.3", - "from": "https://registry.npmjs.org/express-session/-/express-session-1.11.3.tgz", - "resolved": "https://registry.npmjs.org/express-session/-/express-session-1.11.3.tgz", + "version": "1.14.2", + "from": "express-session@1.14.2", "dependencies": { "cookie": { - "version": "0.1.3", - "from": "https://registry.npmjs.org/cookie/-/cookie-0.1.3.tgz", - "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.1.3.tgz" + "version": "0.3.1", + "from": "cookie@0.3.1" }, "cookie-signature": { "version": "1.0.6", - "from": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.0.6.tgz", - "resolved": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.0.6.tgz" + "from": "cookie-signature@1.0.6" }, "crc": { - "version": "3.3.0", - "from": "https://registry.npmjs.org/crc/-/crc-3.3.0.tgz", - "resolved": "https://registry.npmjs.org/crc/-/crc-3.3.0.tgz" + "version": "3.4.1", + "from": "crc@3.4.1" }, "debug": { "version": "2.2.0", - "from": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz", - "resolved": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz", + "from": "debug@~2.2.0", "dependencies": { "ms": { "version": "0.7.1", - "from": "https://registry.npmjs.org/ms/-/ms-0.7.1.tgz", - "resolved": "https://registry.npmjs.org/ms/-/ms-0.7.1.tgz" + "from": "ms@0.7.1" } } }, "depd": { - "version": "1.0.1", - "from": "https://registry.npmjs.org/depd/-/depd-1.0.1.tgz", - "resolved": "https://registry.npmjs.org/depd/-/depd-1.0.1.tgz" + "version": "1.1.0", + "from": "depd@~1.1.0" }, "on-headers": { "version": "1.0.1", - "from": "https://registry.npmjs.org/on-headers/-/on-headers-1.0.1.tgz", - "resolved": "https://registry.npmjs.org/on-headers/-/on-headers-1.0.1.tgz" + "from": "on-headers@~1.0.1" }, "parseurl": { "version": "1.3.1", - "from": "https://registry.npmjs.org/parseurl/-/parseurl-1.3.1.tgz", - "resolved": "https://registry.npmjs.org/parseurl/-/parseurl-1.3.1.tgz" + "from": "parseurl@~1.3.1" }, "uid-safe": { - "version": "2.0.0", - "from": "https://registry.npmjs.org/uid-safe/-/uid-safe-2.0.0.tgz", - "resolved": "https://registry.npmjs.org/uid-safe/-/uid-safe-2.0.0.tgz", + "version": "2.1.3", + "from": "uid-safe@~2.1.3", + "resolved": "https://registry.npmjs.org/uid-safe/-/uid-safe-2.1.3.tgz", "dependencies": { "base64-url": { - "version": "1.2.1", - "from": "https://registry.npmjs.org/base64-url/-/base64-url-1.2.1.tgz", - "resolved": "https://registry.npmjs.org/base64-url/-/base64-url-1.2.1.tgz" + "version": "1.3.3", + "from": "base64-url@1.3.3" + }, + "random-bytes": { + "version": "1.0.0", + "from": "random-bytes@~1.0.0" } } }, "utils-merge": { "version": "1.0.0", - "from": "https://registry.npmjs.org/utils-merge/-/utils-merge-1.0.0.tgz", - "resolved": "https://registry.npmjs.org/utils-merge/-/utils-merge-1.0.0.tgz" + "from": "utils-merge@1.0.0" } } }, @@ -1166,6 +1174,53 @@ } } }, + "ioredis": { + "version": "2.4.0", + "from": "ioredis@", + "resolved": "https://registry.npmjs.org/ioredis/-/ioredis-2.4.0.tgz", + "dependencies": { + "bluebird": { + "version": "3.4.6", + "from": "bluebird@^3.3.4", + "resolved": "https://registry.npmjs.org/bluebird/-/bluebird-3.4.6.tgz" + }, + "cluster-key-slot": { + "version": "1.0.8", + "from": "cluster-key-slot@^1.0.6", + "resolved": "https://registry.npmjs.org/cluster-key-slot/-/cluster-key-slot-1.0.8.tgz" + }, + "debug": { + "version": "2.3.0", + "from": "debug@^2.2.0", + "resolved": "https://registry.npmjs.org/debug/-/debug-2.3.0.tgz", + "dependencies": { + "ms": { + "version": "0.7.2", + "from": "ms@0.7.2" + } + } + }, + "double-ended-queue": { + "version": "2.1.0-0", + "from": "double-ended-queue@^2.1.0-0", + "resolved": "https://registry.npmjs.org/double-ended-queue/-/double-ended-queue-2.1.0-0.tgz" + }, + "flexbuffer": { + "version": "0.0.6", + "from": "flexbuffer@0.0.6" + }, + "redis-commands": { + "version": "1.3.0", + "from": "redis-commands@^1.2.0", + "resolved": "https://registry.npmjs.org/redis-commands/-/redis-commands-1.3.0.tgz" + }, + "redis-parser": { + "version": "1.3.0", + "from": "redis-parser@^1.3.0", + "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-1.3.0.tgz" + } + } + }, "jade": { "version": "1.3.1", "from": "https://registry.npmjs.org/jade/-/jade-1.3.1.tgz", @@ -1602,7 +1657,7 @@ }, "logger-sharelatex": { "version": "1.3.1", - "from": "git+https://github.com/sharelatex/logger-sharelatex.git#bf413ec621a000cf0e08c939de38d5e24541a08c", + "from": "logger-sharelatex@git+https://github.com/sharelatex/logger-sharelatex.git#bf413ec621a000cf0e08c939de38d5e24541a08c", "resolved": "git+https://github.com/sharelatex/logger-sharelatex.git#bf413ec621a000cf0e08c939de38d5e24541a08c", "dependencies": { "bunyan": { @@ -1812,7 +1867,7 @@ }, "metrics-sharelatex": { "version": "1.3.0", - "from": "git+https://github.com/sharelatex/metrics-sharelatex.git#080c4aeb696edcd5d6d86f202f2c528f0661d7a6", + "from": "metrics-sharelatex@git+https://github.com/sharelatex/metrics-sharelatex.git#080c4aeb696edcd5d6d86f202f2c528f0661d7a6", "resolved": "git+https://github.com/sharelatex/metrics-sharelatex.git#080c4aeb696edcd5d6d86f202f2c528f0661d7a6", "dependencies": { "coffee-script": { @@ -2535,32 +2590,32 @@ }, "passport-ldapauth": { "version": "0.6.0", - "from": "passport-ldapauth@*", + "from": "https://registry.npmjs.org/passport-ldapauth/-/passport-ldapauth-0.6.0.tgz", "resolved": "https://registry.npmjs.org/passport-ldapauth/-/passport-ldapauth-0.6.0.tgz", "dependencies": { "passport-strategy": { "version": "1.0.0", - "from": "passport-strategy@>=1.0.0 <2.0.0", + "from": "https://registry.npmjs.org/passport-strategy/-/passport-strategy-1.0.0.tgz", "resolved": "https://registry.npmjs.org/passport-strategy/-/passport-strategy-1.0.0.tgz" }, "ldapauth-fork": { "version": "2.5.3", - "from": "ldapauth-fork@>=2.5.0 <2.6.0", + "from": "https://registry.npmjs.org/ldapauth-fork/-/ldapauth-fork-2.5.3.tgz", "resolved": "https://registry.npmjs.org/ldapauth-fork/-/ldapauth-fork-2.5.3.tgz", "dependencies": { "bcryptjs": { "version": "2.3.0", - "from": "bcryptjs@2.3.0", + "from": "https://registry.npmjs.org/bcryptjs/-/bcryptjs-2.3.0.tgz", "resolved": "https://registry.npmjs.org/bcryptjs/-/bcryptjs-2.3.0.tgz" }, "lru-cache": { "version": "3.2.0", - "from": "lru-cache@3.2.0", + "from": "https://registry.npmjs.org/lru-cache/-/lru-cache-3.2.0.tgz", "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-3.2.0.tgz", "dependencies": { "pseudomap": { "version": "1.0.2", - "from": "pseudomap@>=1.0.1 <2.0.0", + "from": "https://registry.npmjs.org/pseudomap/-/pseudomap-1.0.2.tgz", "resolved": "https://registry.npmjs.org/pseudomap/-/pseudomap-1.0.2.tgz" } } @@ -3430,7 +3485,7 @@ }, "json-stringify-safe": { "version": "5.0.1", - "from": "https://registry.npmjs.org/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz", + "from": "json-stringify-safe@~5.0.0", "resolved": "https://registry.npmjs.org/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz" }, "mime-types": { @@ -3462,7 +3517,7 @@ }, "stringstream": { "version": "0.0.5", - "from": "https://registry.npmjs.org/stringstream/-/stringstream-0.0.5.tgz", + "from": "stringstream@~0.0.4", "resolved": "https://registry.npmjs.org/stringstream/-/stringstream-0.0.5.tgz" }, "tough-cookie": { @@ -3472,7 +3527,7 @@ }, "tunnel-agent": { "version": "0.4.3", - "from": "https://registry.npmjs.org/tunnel-agent/-/tunnel-agent-0.4.3.tgz", + "from": "tunnel-agent@~0.4.0", "resolved": "https://registry.npmjs.org/tunnel-agent/-/tunnel-agent-0.4.3.tgz" } } @@ -3558,7 +3613,7 @@ }, "depd": { "version": "1.1.0", - "from": "https://registry.npmjs.org/depd/-/depd-1.1.0.tgz", + "from": "depd@~1.1.0", "resolved": "https://registry.npmjs.org/depd/-/depd-1.1.0.tgz" }, "dottie": { @@ -3656,7 +3711,7 @@ }, "settings-sharelatex": { "version": "1.0.0", - "from": "git+https://github.com/sharelatex/settings-sharelatex.git#cbc5e41c1dbe6789721a14b3fdae05bf22546559", + "from": "settings-sharelatex@git+https://github.com/sharelatex/settings-sharelatex.git#cbc5e41c1dbe6789721a14b3fdae05bf22546559", "resolved": "git+https://github.com/sharelatex/settings-sharelatex.git#cbc5e41c1dbe6789721a14b3fdae05bf22546559", "dependencies": { "coffee-script": { diff --git a/services/web/package.json b/services/web/package.json index d302cc30d8..2b1e835c7a 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -17,17 +17,18 @@ "bcrypt": "0.8.3", "body-parser": "^1.13.1", "bufferedstream": "1.6.0", - "connect-redis": "2.3.0", + "connect-redis": "^3.1.0", "contentful": "^3.3.14", "cookie": "^0.2.3", "cookie-parser": "1.3.5", "csurf": "^1.8.3", "dateformat": "1.0.4-1.2.3", "express": "4.13.0", - "express-session": "1.11.3", + "express-session": "^1.14.2", "grunt": "^0.4.5", "heapdump": "^0.3.7", "http-proxy": "^1.8.1", + "ioredis": "^2.4.0", "jade": "~1.3.1", "ldapjs": "^1.0.0", "lodash": "^4.13.1", From a3738688625074beb3331f811197e5b8368d08fc Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 8 Nov 2016 16:00:18 +0000 Subject: [PATCH 2/5] Fix unit tests --- .../Authentication/AuthenticationControllerTests.coffee | 4 ++-- .../UnitTests/coffee/User/UserSessionsManagerTests.coffee | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index 104d2e99c3..e5a4486d4a 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -92,7 +92,7 @@ describe "AuthenticationController", -> @req.login = sinon.stub().callsArgWith(1, null) @res.json = sinon.stub() @req.session = @session = {passport: {user: @user}} - @req.session.destroy = sinon.stub() + @req.session.destroy = sinon.stub().callsArgWith(0, null) @req.session.save = sinon.stub().callsArgWith(0, null) @req.sessionStore = {generate: sinon.stub()} @passport.authenticate.callsArgWith(1, null, @user, @info) @@ -164,7 +164,7 @@ describe "AuthenticationController", -> @req.session = @session = {passport: {user: @user}} @req.session = passport: {user: {_id: "one"}} - @req.session.destroy = sinon.stub() + @req.session.destroy = sinon.stub().callsArgWith(0, null) @req.session.save = sinon.stub().callsArgWith(0, null) @req.sessionStore = {generate: sinon.stub()} @UserSessionsManager.trackSession = sinon.stub() diff --git a/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee b/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee index f9bf846e54..a9e8aa6971 100644 --- a/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee @@ -33,7 +33,7 @@ describe 'UserSessionsManager', -> @rclient.exec.callsArgWith(0, null) @redis = - createClient: () => @rclient + client: () => @rclient @logger = err: sinon.stub() error: sinon.stub() @@ -42,15 +42,15 @@ describe 'UserSessionsManager', -> redis: web: {} @UserSessionsManager = SandboxedModule.require modulePath, requires: - "redis-sharelatex": @redis "logger-sharelatex": @logger "settings-sharelatex": @settings + './UserSessionsRedis': @redis describe '_sessionSetKey', -> it 'should build the correct key', -> result = @UserSessionsManager._sessionSetKey(@user) - result.should.equal 'UserSessions:abcd' + result.should.equal 'UserSessions:{abcd}' describe '_sessionKey', -> From 5f3098df38699a5bef1b90f7fdcf00cef39baa1b Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 9 Nov 2016 11:03:03 +0000 Subject: [PATCH 3/5] Replace multi-ops with Async.series, tests passing --- .../Features/User/UserSessionsManager.coffee | 19 +++++--- .../User/UserSessionsManagerTests.coffee | 47 ++++++++++--------- .../acceptance/coffee/helpers/redis.coffee | 27 +++++++---- 3 files changed, 56 insertions(+), 37 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserSessionsManager.coffee b/services/web/app/coffee/Features/User/UserSessionsManager.coffee index af3efd55cd..1e05a576ad 100644 --- a/services/web/app/coffee/Features/User/UserSessionsManager.coffee +++ b/services/web/app/coffee/Features/User/UserSessionsManager.coffee @@ -68,7 +68,8 @@ module.exports = UserSessionsManager = if sessionKeys.length == 0 logger.log {user_id: user._id}, "no other sessions found, returning" return callback(null, []) - rclient.mget sessionKeys, (err, sessions) -> + + Async.map sessionKeys, ((k, cb) -> rclient.get(k, cb)), (err, sessions) -> if err? logger.err {user_id: user._id}, "error getting all sessions for user from redis" return callback(err) @@ -104,12 +105,18 @@ module.exports = UserSessionsManager = logger.log {user_id: user._id}, "no sessions in UserSessions set to delete, returning" return callback(null) logger.log {user_id: user._id, count: keysToDelete.length}, "deleting sessions for user" - rclient.multi() - .del(keysToDelete) - .srem(sessionSetKey, keysToDelete) - .exec (err, result) -> + + deletions = keysToDelete.map (k) -> + (cb) -> + rclient.del k, cb + + Async.series deletions, (err, _result) -> + if err? + logger.err {err, user_id: user._id, sessionSetKey}, "errror revoking all sessions for user" + return callback(err) + rclient.srem sessionSetKey, keysToDelete, (err) -> if err? - logger.err {err, user_id: user._id, sessionSetKey}, "error revoking all sessions for user" + logger.err {err, user_id: user._id, sessionSetKey}, "error removing session set for user" return callback(err) callback(null) diff --git a/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee b/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee index a9e8aa6971..8e75be83f5 100644 --- a/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee @@ -4,6 +4,7 @@ should = chai.should() expect = chai.expect modulePath = "../../../../app/js/Features/User/UserSessionsManager.js" SandboxedModule = require('sandboxed-module') +Async = require('async') describe 'UserSessionsManager', -> @@ -45,6 +46,7 @@ describe 'UserSessionsManager', -> "logger-sharelatex": @logger "settings-sharelatex": @settings './UserSessionsRedis': @redis + 'async': Async describe '_sessionSetKey', -> @@ -247,14 +249,14 @@ describe 'UserSessionsManager', -> @_checkSessions.callCount.should.equal 0 done() - ## describe 'revokeAllUserSessions', -> beforeEach -> @sessionKeys = ['sess:one', 'sess:two'] @retain = [] @rclient.smembers.callsArgWith(1, null, @sessionKeys) - @rclient.exec.callsArgWith(0, null) + @rclient.del = sinon.stub().callsArgWith(1, null) + @rclient.srem = sinon.stub().callsArgWith(2, null) @call = (callback) => @UserSessionsManager.revokeAllUserSessions @user, @retain, callback @@ -267,15 +269,14 @@ describe 'UserSessionsManager', -> it 'should call the appropriate redis methods', (done) -> @call (err) => @rclient.smembers.callCount.should.equal 1 - @rclient.multi.callCount.should.equal 1 - @rclient.del.callCount.should.equal 1 - expect(@rclient.del.firstCall.args[0]).to.deep.equal @sessionKeys + @rclient.del.callCount.should.equal 2 + expect(@rclient.del.firstCall.args[0]).to.deep.equal @sessionKeys[0] + expect(@rclient.del.secondCall.args[0]).to.deep.equal @sessionKeys[1] @rclient.srem.callCount.should.equal 1 expect(@rclient.srem.firstCall.args[1]).to.deep.equal @sessionKeys - @rclient.exec.callCount.should.equal 1 done() describe 'when a session is retained', -> @@ -284,7 +285,7 @@ describe 'UserSessionsManager', -> @sessionKeys = ['sess:one', 'sess:two', 'sess:three', 'sess:four'] @retain = ['two'] @rclient.smembers.callsArgWith(1, null, @sessionKeys) - @rclient.exec.callsArgWith(0, null) + @rclient.del = sinon.stub().callsArgWith(1, null) @call = (callback) => @UserSessionsManager.revokeAllUserSessions @user, @retain, callback @@ -297,28 +298,33 @@ describe 'UserSessionsManager', -> it 'should call the appropriate redis methods', (done) -> @call (err) => @rclient.smembers.callCount.should.equal 1 - @rclient.multi.callCount.should.equal 1 - @rclient.del.callCount.should.equal 1 + @rclient.del.callCount.should.equal @sessionKeys.length - 1 @rclient.srem.callCount.should.equal 1 - @rclient.exec.callCount.should.equal 1 done() it 'should remove all sessions except for the retained one', (done) -> @call (err) => - expect(@rclient.del.firstCall.args[0]).to.deep.equal(['sess:one', 'sess:three', 'sess:four']) + expect(@rclient.del.firstCall.args[0]).to.deep.equal('sess:one') + expect(@rclient.del.secondCall.args[0]).to.deep.equal('sess:three') + expect(@rclient.del.thirdCall.args[0]).to.deep.equal('sess:four') expect(@rclient.srem.firstCall.args[1]).to.deep.equal(['sess:one', 'sess:three', 'sess:four']) done() describe 'when rclient produces an error', -> beforeEach -> - @rclient.exec.callsArgWith(0, new Error('woops')) + @rclient.del = sinon.stub().callsArgWith(1, new Error('woops')) it 'should produce an error', (done) -> @call (err) => expect(err).to.be.instanceof Error done() + it 'should not call rclient.srem', (done) -> + @call (err) => + @rclient.srem.callCount.should.equal 0 + done() + describe 'when no user is supplied', -> beforeEach -> @@ -334,10 +340,8 @@ describe 'UserSessionsManager', -> it 'should not call the appropriate redis methods', (done) -> @call (err) => @rclient.smembers.callCount.should.equal 0 - @rclient.multi.callCount.should.equal 0 @rclient.del.callCount.should.equal 0 @rclient.srem.callCount.should.equal 0 - @rclient.exec.callCount.should.equal 0 done() describe 'when there are no keys to delete', -> @@ -354,10 +358,8 @@ describe 'UserSessionsManager', -> it 'should not do the delete operation', (done) -> @call (err) => @rclient.smembers.callCount.should.equal 1 - @rclient.multi.callCount.should.equal 0 @rclient.del.callCount.should.equal 0 @rclient.srem.callCount.should.equal 0 - @rclient.exec.callCount.should.equal 0 done() describe 'touch', -> @@ -415,7 +417,10 @@ describe 'UserSessionsManager', -> ] @exclude = ['two'] @rclient.smembers.callsArgWith(1, null, @sessionKeys) - @rclient.mget.callsArgWith(1, null, @sessions) + @rclient.get = sinon.stub() + @rclient.get.onCall(0).callsArgWith(1, null, @sessions[0]) + @rclient.get.onCall(1).callsArgWith(1, null, @sessions[1]) + @call = (callback) => @UserSessionsManager.getAllUserSessions @user, @exclude, callback @@ -437,9 +442,9 @@ describe 'UserSessionsManager', -> @rclient.smembers.callCount.should.equal 1 done() - it 'should have called rclient.mget', (done) -> + it 'should have called rclient.get', (done) -> @call (err, sessions) => - @rclient.mget.callCount.should.equal 1 + @rclient.get.callCount.should.equal @sessionKeys.length - 1 done() describe 'when there are no other sessions', -> @@ -484,10 +489,10 @@ describe 'UserSessionsManager', -> @rclient.mget.callCount.should.equal 0 done() - describe 'when mget produces an error', -> + describe 'when get produces an error', -> beforeEach -> - @rclient.mget.callsArgWith(1, new Error('woops')) + @rclient.get = sinon.stub().callsArgWith(1, new Error('woops')) it 'should produce an error', (done) -> @call (err, sessions) => diff --git a/services/web/test/acceptance/coffee/helpers/redis.coffee b/services/web/test/acceptance/coffee/helpers/redis.coffee index 2611e4fc57..dd98a39b59 100644 --- a/services/web/test/acceptance/coffee/helpers/redis.coffee +++ b/services/web/test/acceptance/coffee/helpers/redis.coffee @@ -3,25 +3,32 @@ redis = require('redis-sharelatex') logger = require("logger-sharelatex") Async = require('async') -rclient = redis.createClient(Settings.redis.web) +UserSessionsRedis = require('../../../../app/js/Features/User/UserSessionsRedis') + +# rclient = redis.createClient(Settings.redis.web) +rclient = UserSessionsRedis.client() module.exports = getUserSessions: (user, callback=(err, sessionsSet)->) -> - rclient.smembers "UserSessions:#{user._id}", (err, result) -> + rclient.smembers "UserSessions:{#{user._id}}", (err, result) -> return callback(err, result) clearUserSessions: (user, callback=(err)->) -> - sessionSetKey = "UserSessions:#{user._id}" + sessionSetKey = "UserSessions:{#{user._id}}" rclient.smembers sessionSetKey, (err, sessionKeys) -> if err return callback(err) if sessionKeys.length == 0 return callback(null) - rclient.multi() - .del(sessionKeys) - .srem(sessionSetKey, sessionKeys) - .exec (err, result) -> - if err - return callback(err) - callback(null) + actions = sessionKeys.map (k) -> + (cb) -> + rclient.del k, (err) -> + cb(err) + Async.series( + actions, (err, results) -> + rclient.srem sessionSetKey, sessionKeys, (err) -> + if err + return callback(err) + callback(null) + ) From c3860dd7a73f253415f668f947666f2311cf59d4 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 9 Nov 2016 14:36:25 +0000 Subject: [PATCH 4/5] Remove stray commented-out code, use mapSeries instead of map. --- .../web/app/coffee/Features/User/UserSessionsManager.coffee | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserSessionsManager.coffee b/services/web/app/coffee/Features/User/UserSessionsManager.coffee index 1e05a576ad..b3872ca417 100644 --- a/services/web/app/coffee/Features/User/UserSessionsManager.coffee +++ b/services/web/app/coffee/Features/User/UserSessionsManager.coffee @@ -5,7 +5,6 @@ Async = require('async') _ = require('underscore') UserSessionsRedis = require('./UserSessionsRedis') -# rclient = redis.createClient(Settings.redis.web) rclient = UserSessionsRedis.client() module.exports = UserSessionsManager = @@ -69,7 +68,7 @@ module.exports = UserSessionsManager = logger.log {user_id: user._id}, "no other sessions found, returning" return callback(null, []) - Async.map sessionKeys, ((k, cb) -> rclient.get(k, cb)), (err, sessions) -> + Async.mapSeries sessionKeys, ((k, cb) -> rclient.get(k, cb)), (err, sessions) -> if err? logger.err {user_id: user._id}, "error getting all sessions for user from redis" return callback(err) From 593e59450b63f2d605f95334d854a7b4f1611958 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 10 Nov 2016 14:31:48 +0000 Subject: [PATCH 5/5] Use different UserSessions keys for cluster vs normal redis session store --- .../Features/User/UserSessionsManager.coffee | 16 ++++++---------- .../Features/User/UserSessionsRedis.coffee | 13 ++++++++----- .../coffee/User/UserSessionsManagerTests.coffee | 11 +++-------- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserSessionsManager.coffee b/services/web/app/coffee/Features/User/UserSessionsManager.coffee index b3872ca417..78016e8a09 100644 --- a/services/web/app/coffee/Features/User/UserSessionsManager.coffee +++ b/services/web/app/coffee/Features/User/UserSessionsManager.coffee @@ -8,10 +8,6 @@ UserSessionsRedis = require('./UserSessionsRedis') rclient = UserSessionsRedis.client() module.exports = UserSessionsManager = - - _sessionSetKey: (user) -> - return "UserSessions:{#{user._id}}" - # mimic the key used by the express sessions _sessionKey: (sessionId) -> return "sess:#{sessionId}" @@ -24,7 +20,7 @@ module.exports = UserSessionsManager = logger.log {user_id: user._id}, "no sessionId to track, returning" return callback(null) logger.log {user_id: user._id, sessionId}, "onLogin handler" - sessionSetKey = UserSessionsManager._sessionSetKey(user) + sessionSetKey = UserSessionsRedis.sessionSetKey(user) value = UserSessionsManager._sessionKey sessionId rclient.multi() .sadd(sessionSetKey, value) @@ -44,7 +40,7 @@ module.exports = UserSessionsManager = logger.log {user_id: user._id}, "no sessionId to untrack, returning" return callback(null) logger.log {user_id: user._id, sessionId}, "onLogout handler" - sessionSetKey = UserSessionsManager._sessionSetKey(user) + sessionSetKey = UserSessionsRedis.sessionSetKey(user) value = UserSessionsManager._sessionKey sessionId rclient.multi() .srem(sessionSetKey, value) @@ -58,7 +54,7 @@ module.exports = UserSessionsManager = getAllUserSessions: (user, exclude, callback=(err, sessionKeys)->) -> exclude = _.map(exclude, UserSessionsManager._sessionKey) - sessionSetKey = UserSessionsManager._sessionSetKey(user) + sessionSetKey = UserSessionsRedis.sessionSetKey(user) rclient.smembers sessionSetKey, (err, sessionKeys) -> if err? logger.err user_id: user._id, "error getting all session keys for user from redis" @@ -94,7 +90,7 @@ module.exports = UserSessionsManager = logger.log {}, "no user to revoke sessions for, returning" return callback(null) logger.log {user_id: user._id}, "revoking all existing sessions for user" - sessionSetKey = UserSessionsManager._sessionSetKey(user) + sessionSetKey = UserSessionsRedis.sessionSetKey(user) rclient.smembers sessionSetKey, (err, sessionKeys) -> if err? logger.err {err, user_id: user._id, sessionSetKey}, "error getting contents of UserSessions set" @@ -123,7 +119,7 @@ module.exports = UserSessionsManager = if !user logger.log {}, "no user to touch sessions for, returning" return callback(null) - sessionSetKey = UserSessionsManager._sessionSetKey(user) + sessionSetKey = UserSessionsRedis.sessionSetKey(user) rclient.expire sessionSetKey, "#{Settings.cookieSessionLength}", (err, response) -> if err? logger.err {err, user_id: user._id}, "error while updating ttl on UserSessions set" @@ -135,7 +131,7 @@ module.exports = UserSessionsManager = logger.log {}, "no user, returning" return callback(null) logger.log {user_id: user._id}, "checking sessions for user" - sessionSetKey = UserSessionsManager._sessionSetKey(user) + sessionSetKey = UserSessionsRedis.sessionSetKey(user) rclient.smembers sessionSetKey, (err, sessionKeys) -> if err? logger.err {err, user_id: user._id, sessionSetKey}, "error getting contents of UserSessions set" diff --git a/services/web/app/coffee/Features/User/UserSessionsRedis.coffee b/services/web/app/coffee/Features/User/UserSessionsRedis.coffee index 2103f09fab..89ab7ed192 100644 --- a/services/web/app/coffee/Features/User/UserSessionsRedis.coffee +++ b/services/web/app/coffee/Features/User/UserSessionsRedis.coffee @@ -3,16 +3,19 @@ redis = require 'redis-sharelatex' ioredis = require 'ioredis' logger = require 'logger-sharelatex' +redisSessionsSettings = Settings.redis.websessions or Settings.redis.web + module.exports = Redis = - client: () -> - - redisSessionsSettings = Settings.redis.websessions or Settings.redis.web - if redisSessionsSettings?.cluster? logger.log {}, "using redis cluster for web sessions" rclient = new ioredis.Cluster(redisSessionsSettings.cluster) else rclient = redis.createClient(redisSessionsSettings) - return rclient + + sessionSetKey: (user) -> + if redisSessionsSettings?.cluster? + return "UserSessions:{#{user._id}}" + else + return "UserSessions:#{user._id}" diff --git a/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee b/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee index 8e75be83f5..b367bd893e 100644 --- a/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserSessionsManagerTests.coffee @@ -33,8 +33,9 @@ describe 'UserSessionsManager', -> @rclient.expire.returns(@rclient) @rclient.exec.callsArgWith(0, null) - @redis = + @UserSessionsRedis = client: () => @rclient + sessionSetKey: (user) => "UserSessions:{#{user._id}}" @logger = err: sinon.stub() error: sinon.stub() @@ -45,15 +46,9 @@ describe 'UserSessionsManager', -> @UserSessionsManager = SandboxedModule.require modulePath, requires: "logger-sharelatex": @logger "settings-sharelatex": @settings - './UserSessionsRedis': @redis + './UserSessionsRedis': @UserSessionsRedis 'async': Async - describe '_sessionSetKey', -> - - it 'should build the correct key', -> - result = @UserSessionsManager._sessionSetKey(@user) - result.should.equal 'UserSessions:{abcd}' - describe '_sessionKey', -> it 'should build the correct key', ->