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,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"
   },

src/account.js

@@ -1,83 +1,37 @@
 var db = require("./database");
 var Q = require("q");
 
-function Account(opts) {
+const DEFAULT_PROFILE = Object.freeze({ image: '', text: '' });
-    var defaults = {
-        name: "",
-        ip: "",
-        aliases: [],
-        globalRank: -1,
-        channelRank: -1,
-        guest: true,
-        profile: {
-            image: "",
-            text: ""
-        }
-    };
 
-    this.name = opts.name || defaults.name;
+class Account {
-    this.lowername = this.name.toLowerCase();
+    constructor(ip, user, aliases) {
-    this.ip = opts.ip || defaults.ip;
+        this.ip = ip;
-    this.aliases = opts.aliases || defaults.aliases;
+        this.user = user;
-    this.globalRank = "globalRank" in opts ? opts.globalRank : defaults.globalRank;
+        this.aliases = aliases;
-    this.channelRank = "channelRank" in opts ? opts.channelRank : defaults.channelRank;
+        this.channelRank = -1;
-    this.effectiveRank = Math.max(this.globalRank, this.channelRank);
+        this.guestName = null;
-    this.guest = this.globalRank === 0;
+
-    this.profile = opts.profile || defaults.profile;
+        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) {
+module.exports.Account = Account;
-    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.rankForName = function (name, opts, cb) {
     if (!cb) {

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.waitFlag(Flags.U_HAS_CHANNEL_RANK, function () {
-                user.channel = chan;
+                if (user.is(Flags.U_IN_CHANNEL)) {
-                user.refreshAccount(function (err, account) {
+                    return;
-                    /* Already joined the channel by some other condition */
+                }
-                    if (user.is(Flags.U_IN_CHANNEL)) {
-                        return;
-                    } else if (user.channel === chan) {
-                        user.channel = null;
-                    }
 
-                    if (account.effectiveRank >= 2) {
+                if (user.account.effectiveRank >= 2) {
-                        cb(null, ChannelModule.PASSTHROUGH);
+                    cb(null, ChannelModule.PASSTHROUGH);
-                        user.socket.emit("cancelNeedPassword");
+                    user.socket.emit("cancelNeedPassword");
-                    }
+                }
-                });
             });
 
             /* Option 2: Enter correct password */

src/channel/channel.js

@@ -329,41 +329,33 @@ Channel.prototype.joinUser = function (user, data) {
         }
 
         user.channel = self;
-        if (self.is(Flags.C_REGISTERED)) {
+        user.waitFlag(Flags.U_LOGGED_IN, () => {
-            user.refreshAccount(function (err, account) {
+            db.channels.getRank(self.name, user.getName(), (error, rank) => {
-                if (err) {
+                if (!error) {
-                    Logger.errlog.log("user.refreshAccount failed at Channel.joinUser");
+                    user.setChannelRank(rank);
-                    Logger.errlog.log(err.stack);
+                    user.setFlag(Flags.U_HAS_CHANNEL_RANK);
-                    self.refCounter.unref("Channel::user");
-                    return;
                 }
-
-                afterAccount();
             });
-        } else {
+        });
-            afterAccount();
+
+        if (user.socket.disconnected) {
+            self.refCounter.unref("Channel::user");
+            return;
+        } else if (self.dead) {
+            return;
         }
 
-        function afterAccount() {
+        self.checkModules("onUserPreJoin", [user, data], function (err, result) {
-            if (user.socket.disconnected) {
+            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);
     });
 

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
 };

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;
+    socket.user = null;
-    var auth = socket.request.signedCookies.auth;
+    socket.aliases = [];
-    if (!auth) {
+
-        return accept(null, true);
+    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) {
+    promises.push(getAliases(socket._realip).then(aliases => {
-        if (!err) {
+        socket.aliases = aliases;
-            socket.user = {
+    }).catch(error => {
-                name: user.name,
+        // Do nothing
-                global_rank: user.global_rank,
+    }));
-                registrationTime: new Date(user.time)
+
-            };
+    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.socket.emit("login", {
-        user.account.name = sock.user.name;
+            success: true,
-        user.registrationTime = sock.user.registrationTime;
+            name: user.getName(),
-        user.refreshAccount(function (err, account) {
+            guest: false
-            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);
         });
+        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);

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) {
+        self.socket.emit("login", {
-            if (err) {
+            success: true,
-                Logger.errlog.log("[SEVERE] getAccount failed for user " + user.name);
+            name: 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);
         });
+        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.account.guestName = name;
-        self.refreshAccount(function (err, account) {
+        self.account.update();
-            if (err) {
+        self.socket.emit("rank", self.account.effectiveRank);
-                Logger.errlog.log("[SEVERE] getAccount failed for guest login " + name);
+        self.emit("effectiveRankChange", self.account.effectiveRank);
-                Logger.errlog.log(err);
+        self.socket.emit("login", {
-                self.account.name = "";
+            success: true,
-                return;
+            name: name,
-            }
+            guest: true
-
-            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);
         });
+        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;