From 99760b6989c3b0d994ef8a4714cc2bb17266db21 Mon Sep 17 00:00:00 2001 From: Calvin Montgomery Date: Mon, 3 Oct 2016 23:12:22 -0700 Subject: [PATCH] Purge the awful refreshAccount logic User.prototype.refreshAccount was responsible for multiple race condition bugs as well as inefficient duplication of DB queries in an attempt to correct such race conditions. It has now been replaced by a more reasonable model: * Global user account information and aliases are fetched in parallel on socket connection * Channel rank is fetched when the user tries to join a channel --- package.json | 2 +- src/account.js | 102 +++++++++--------------------- src/channel/accesscontrol.js | 22 +++---- src/channel/channel.js | 51 +++++++-------- src/flags.js | 3 +- src/io/ioserver.js | 66 ++++++++++---------- src/user.js | 118 ++++++++++++----------------------- 7 files changed, 132 insertions(+), 232 deletions(-) diff --git a/package.json b/package.json index 8e108cc7..01a41d6d 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "author": "Calvin Montgomery", "name": "CyTube", "description": "Online media synchronizer and chat", - "version": "3.22.4", + "version": "3.23.0", "repository": { "url": "http://github.com/calzoneman/sync" }, diff --git a/src/account.js b/src/account.js index d8e7d1ca..559d771a 100644 --- a/src/account.js +++ b/src/account.js @@ -1,83 +1,37 @@ var db = require("./database"); var Q = require("q"); -function Account(opts) { - var defaults = { - name: "", - ip: "", - aliases: [], - globalRank: -1, - channelRank: -1, - guest: true, - profile: { - image: "", - text: "" - } - }; +const DEFAULT_PROFILE = Object.freeze({ image: '', text: '' }); - this.name = opts.name || defaults.name; - this.lowername = this.name.toLowerCase(); - this.ip = opts.ip || defaults.ip; - this.aliases = opts.aliases || defaults.aliases; - this.globalRank = "globalRank" in opts ? opts.globalRank : defaults.globalRank; - this.channelRank = "channelRank" in opts ? opts.channelRank : defaults.channelRank; - this.effectiveRank = Math.max(this.globalRank, this.channelRank); - this.guest = this.globalRank === 0; - this.profile = opts.profile || defaults.profile; +class Account { + constructor(ip, user, aliases) { + this.ip = ip; + this.user = user; + this.aliases = aliases; + this.channelRank = -1; + this.guestName = null; + + this.update(); + } + + update() { + if (this.user !== null) { + this.name = this.user.name; + this.globalRank = this.user.global_rank; + } else if (this.guestName !== null) { + this.name = this.guestName; + this.globalRank = 0; + } else { + this.name = ''; + this.globalRank = -1; + } + this.lowername = this.name.toLowerCase(); + this.effectiveRank = Math.max(this.channelRank, this.globalRank); + this.profile = (this.user === null) ? DEFAULT_PROFILE : this.user.profile; + } } -module.exports.default = function (ip) { - return new Account({ ip: ip }); -}; - -module.exports.getAccount = function (name, ip, opts, cb) { - if (!cb) { - cb = opts; - opts = {}; - } - opts.channel = opts.channel || false; - - var data = {}; - Q.nfcall(db.getAliases, ip) - .then(function (aliases) { - data.aliases = aliases; - if (name && opts.registered) { - return Q.nfcall(db.users.getUser, name); - } else if (name) { - return { global_rank: 0, profile: { text: "", image: "" } }; - } else { - return { global_rank: -1, profile: { text: "", image: "" } }; - } - }).then(function (user) { - data.globalRank = user.global_rank; - data.profile = user.profile; - if (opts.channel && opts.registered) { - return Q.nfcall(db.channels.getRank, opts.channel, name); - } else { - if (opts.registered) { - return 1; - } else if (name) { - return 0; - } else { - return -1; - } - } - }).then(function (chanRank) { - data.channelRank = chanRank; - setImmediate(function () { - cb(null, new Account({ - name: name, - ip: ip, - aliases: data.aliases, - globalRank: data.globalRank, - channelRank: data.channelRank, - profile: data.profile - })); - }); - }).catch(function (err) { - cb(err, null); - }).done(); -}; +module.exports.Account = Account; module.exports.rankForName = function (name, opts, cb) { if (!cb) { diff --git a/src/channel/accesscontrol.js b/src/channel/accesscontrol.js index 4078f713..8b0df57d 100644 --- a/src/channel/accesscontrol.js +++ b/src/channel/accesscontrol.js @@ -30,21 +30,15 @@ AccessControlModule.prototype.onUserPreJoin = function (user, data, cb) { } else { user.socket.emit("needPassword", typeof data.pw !== "undefined"); /* Option 1: log in as a moderator */ - user.waitFlag(Flags.U_LOGGED_IN, function () { - user.channel = chan; - user.refreshAccount(function (err, account) { - /* Already joined the channel by some other condition */ - if (user.is(Flags.U_IN_CHANNEL)) { - return; - } else if (user.channel === chan) { - user.channel = null; - } + user.waitFlag(Flags.U_HAS_CHANNEL_RANK, function () { + if (user.is(Flags.U_IN_CHANNEL)) { + return; + } - if (account.effectiveRank >= 2) { - cb(null, ChannelModule.PASSTHROUGH); - user.socket.emit("cancelNeedPassword"); - } - }); + if (user.account.effectiveRank >= 2) { + cb(null, ChannelModule.PASSTHROUGH); + user.socket.emit("cancelNeedPassword"); + } }); /* Option 2: Enter correct password */ diff --git a/src/channel/channel.js b/src/channel/channel.js index 073df3d0..c255f842 100644 --- a/src/channel/channel.js +++ b/src/channel/channel.js @@ -329,41 +329,33 @@ Channel.prototype.joinUser = function (user, data) { } user.channel = self; - if (self.is(Flags.C_REGISTERED)) { - user.refreshAccount(function (err, account) { - if (err) { - Logger.errlog.log("user.refreshAccount failed at Channel.joinUser"); - Logger.errlog.log(err.stack); - self.refCounter.unref("Channel::user"); - return; + user.waitFlag(Flags.U_LOGGED_IN, () => { + db.channels.getRank(self.name, user.getName(), (error, rank) => { + if (!error) { + user.setChannelRank(rank); + user.setFlag(Flags.U_HAS_CHANNEL_RANK); } - - afterAccount(); }); - } else { - afterAccount(); + }); + + if (user.socket.disconnected) { + self.refCounter.unref("Channel::user"); + return; + } else if (self.dead) { + return; } - function afterAccount() { - if (user.socket.disconnected) { + self.checkModules("onUserPreJoin", [user, data], function (err, result) { + if (result === ChannelModule.PASSTHROUGH) { + user.channel = self; + self.acceptUser(user); + } else { + user.channel = null; + user.account.channelRank = 0; + user.account.effectiveRank = user.account.globalRank; self.refCounter.unref("Channel::user"); - return; - } else if (self.dead) { - return; } - - self.checkModules("onUserPreJoin", [user, data], function (err, result) { - if (result === ChannelModule.PASSTHROUGH) { - user.channel = self; - self.acceptUser(user); - } else { - user.channel = null; - user.account.channelRank = 0; - user.account.effectiveRank = user.account.globalRank; - self.refCounter.unref("Channel::user"); - } - }); - } + }); }); }; @@ -401,7 +393,6 @@ Channel.prototype.acceptUser = function (user) { if (user.account.globalRank === 0) loginStr += " (guest)"; loginStr += " (aliases: " + user.account.aliases.join(",") + ")"; self.logger.log(loginStr); - self.sendUserJoin(self.users, user); }); diff --git a/src/flags.js b/src/flags.js index d9e2c870..615d47d9 100644 --- a/src/flags.js +++ b/src/flags.js @@ -10,5 +10,6 @@ module.exports = { U_AFK : 1 << 4, U_MUTED : 1 << 5, U_SMUTED : 1 << 6, - U_IN_CHANNEL : 1 << 7 + U_IN_CHANNEL : 1 << 7, + U_HAS_CHANNEL_RANK: 1 << 8 }; diff --git a/src/io/ioserver.js b/src/io/ioserver.js index 5afa011c..9b4132c8 100644 --- a/src/io/ioserver.js +++ b/src/io/ioserver.js @@ -7,7 +7,6 @@ var Config = require("../config"); var cookieParser = require("cookie-parser")(Config.get("http.cookie-secret")); var $util = require("../utilities"); var Flags = require("../flags"); -var Account = require("../account"); var typecheck = require("json-typecheck"); var net = require("net"); var util = require("../utilities"); @@ -16,6 +15,9 @@ var isTorExit = require("../tor").isTorExit; var session = require("../session"); import counters from '../counters'; import { verifyIPSessionCookie } from '../web/middleware/ipsessioncookie'; +import Promise from 'bluebird'; +const verifySession = Promise.promisify(session.verifySession); +const getAliases = Promise.promisify(db.getAliases); var CONNECT_RATE = { burst: 5, @@ -38,24 +40,31 @@ function parseCookies(socket, accept) { accept(null, true); } } + /** * Called before an incoming socket.io connection is accepted. */ function handleAuth(socket, accept) { - socket.user = false; - var auth = socket.request.signedCookies.auth; - if (!auth) { - return accept(null, true); + socket.user = null; + socket.aliases = []; + + const promises = []; + const auth = socket.request.signedCookies.auth; + if (auth) { + promises.push(verifySession(auth).then(user => { + socket.user = Object.assign({}, user); + }).catch(error => { + // Do nothing + })); } - session.verifySession(auth, function (err, user) { - if (!err) { - socket.user = { - name: user.name, - global_rank: user.global_rank, - registrationTime: new Date(user.time) - }; - } + promises.push(getAliases(socket._realip).then(aliases => { + socket.aliases = aliases; + }).catch(error => { + // Do nothing + })); + + Promise.all(promises).then(() => { accept(null, true); }); } @@ -234,28 +243,17 @@ function handleConnection(sock) { var user = new User(sock); if (sock.user) { user.setFlag(Flags.U_REGISTERED); - user.clearFlag(Flags.U_READY); - user.account.name = sock.user.name; - user.registrationTime = sock.user.registrationTime; - user.refreshAccount(function (err, account) { - if (err) { - user.clearFlag(Flags.U_REGISTERED); - user.setFlag(Flags.U_READY); - return; - } - - user.socket.emit("login", { - success: true, - name: user.getName(), - guest: false - }); - db.recordVisit(ip, user.getName()); - user.socket.emit("rank", user.account.effectiveRank); - user.setFlag(Flags.U_LOGGED_IN); - user.emit("login", account); - Logger.syslog.log(ip + " logged in as " + user.getName()); - user.setFlag(Flags.U_READY); + user.socket.emit("login", { + success: true, + name: user.getName(), + guest: false }); + db.recordVisit(ip, user.getName()); + user.socket.emit("rank", user.account.effectiveRank); + user.setFlag(Flags.U_LOGGED_IN); + user.emit("login", user.account); + Logger.syslog.log(ip + " logged in as " + user.getName()); + user.setFlag(Flags.U_READY); } else { user.socket.emit("rank", -1); user.setFlag(Flags.U_READY); diff --git a/src/user.js b/src/user.js index 62826b5d..9f35004e 100644 --- a/src/user.js +++ b/src/user.js @@ -16,12 +16,17 @@ function User(socket) { self.realip = socket._realip; self.displayip = socket._displayip; self.hostmask = socket._hostmask; - self.account = Account.default(self.realip); self.channel = null; self.queueLimiter = util.newRateLimiter(); self.chatLimiter = util.newRateLimiter(); self.reqPlaylistLimiter = util.newRateLimiter(); self.awaytimer = false; + if (socket.user) { + self.account = new Account.Account(self.realip, socket.user, socket.aliases); + self.registrationTime = new Date(self.account.user.time); + } else { + self.account = new Account.Account(self.realip, null, socket.aliases); + } var announcement = Server.getServer().announcement; if (announcement != null) { @@ -297,28 +302,21 @@ User.prototype.login = function (name, pw) { return; } - self.account.name = user.name; + self.account.user = user; + self.account.update(); + self.socket.emit("rank", self.account.effectiveRank); + self.emit("effectiveRankChange", self.account.effectiveRank); self.registrationTime = new Date(user.time); self.setFlag(Flags.U_REGISTERED); - self.refreshAccount(function (err, account) { - if (err) { - Logger.errlog.log("[SEVERE] getAccount failed for user " + user.name); - Logger.errlog.log(err); - self.clearFlag(Flags.U_REGISTERED); - self.clearFlag(Flags.U_LOGGING_IN); - self.account.name = ""; - return; - } - self.socket.emit("login", { - success: true, - name: user.name - }); - db.recordVisit(self.realip, self.getName()); - Logger.syslog.log(self.realip + " logged in as " + user.name); - self.setFlag(Flags.U_LOGGED_IN); - self.clearFlag(Flags.U_LOGGING_IN); - self.emit("login", self.account); + self.socket.emit("login", { + success: true, + name: user.name }); + db.recordVisit(self.realip, self.getName()); + Logger.syslog.log(self.realip + " logged in as " + user.name); + self.setFlag(Flags.U_LOGGED_IN); + self.clearFlag(Flags.U_LOGGING_IN); + self.emit("login", self.account); }); }; @@ -383,25 +381,19 @@ User.prototype.guestLogin = function (name) { // Login succeeded lastguestlogin[self.realip] = Date.now(); - self.account.name = name; - self.refreshAccount(function (err, account) { - if (err) { - Logger.errlog.log("[SEVERE] getAccount failed for guest login " + name); - Logger.errlog.log(err); - self.account.name = ""; - return; - } - - self.socket.emit("login", { - success: true, - name: name, - guest: true - }); - db.recordVisit(self.realip, self.getName()); - Logger.syslog.log(self.realip + " signed in as " + name); - self.setFlag(Flags.U_LOGGED_IN); - self.emit("login", self.account); + self.account.guestName = name; + self.account.update(); + self.socket.emit("rank", self.account.effectiveRank); + self.emit("effectiveRankChange", self.account.effectiveRank); + self.socket.emit("login", { + success: true, + name: name, + guest: true }); + db.recordVisit(self.realip, self.getName()); + Logger.syslog.log(self.realip + " signed in as " + name); + self.setFlag(Flags.U_LOGGED_IN); + self.emit("login", self.account); }); }; @@ -420,46 +412,6 @@ setInterval(function () { } }, 5 * 60 * 1000); -User.prototype.refreshAccount = function (cb) { - var name = this.account.name; - var opts = { - registered: this.is(Flags.U_REGISTERED), - channel: this.inRegisteredChannel() ? this.channel.name : false - }; - var self = this; - var old = this.account; - Account.getAccount(name, this.realip, opts, function (err, account) { - // TODO - // - // This is a hack to fix #583, an issue where racing callbacks - // from refreshAccount() can cause the user's rank to get out - // of sync. Ideally this should be removed in favor of a more - // robust way of handling updating account state, perhaps a mutex. - if (self.is(Flags.U_REGISTERED) !== opts.registered || - (self.inRegisteredChannel() && !opts.channel) || - self.account.name !== name) { - self.refreshAccount(cb); - return; - } - - if (!err) { - /* Update account if anything changed in the meantime */ - for (var key in old) { - if (self.account[key] !== old[key]) { - account[key] = self.account[key]; - } - } - self.account = account; - if (account.effectiveRank !== old.effectiveRank) { - self.socket.emit("rank", self.account.effectiveRank); - self.emit("effectiveRankChange", self.account.effectiveRank); - } - } - - process.nextTick(cb, err, account); - }); -}; - User.prototype.getFirstSeenTime = function getFirstSeenTime() { if (this.registrationTime && this.socket.ipSessionFirstSeen) { return Math.min(this.registrationTime.getTime(), this.socket.ipSessionFirstSeen.getTime()); @@ -474,4 +426,14 @@ User.prototype.getFirstSeenTime = function getFirstSeenTime() { } }; +User.prototype.setChannelRank = function setRank(rank) { + const changed = this.account.effectiveRank !== rank; + this.account.channelRank = rank; + this.account.update(); + this.socket.emit("rank", this.account.effectiveRank); + if (changed) { + this.emit("effectiveRankChange", this.account.effectiveRank); + } +}; + module.exports = User;