From 40829e257fd292c3297f73c943a0966dfdd27ff3 Mon Sep 17 00:00:00 2001 From: Charlie DeTar Date: Tue, 1 Apr 2014 15:36:29 -0600 Subject: [PATCH] Don't set connectedParticipants from bad session Specifically: disallow setting connectedParticipants from users who have the wrong hangout-url. May be adequate to fix #280. Also, do some more not-entirely-fruitful work on addressing test inconsistency (#267). --- lib/unhangout-sockets.js | 19 ++++++--- package.json | 2 +- public/js/facilitator.js | 3 +- test/common.js | 56 +++++++++++++++------------ test/test.mock-hangout.selenium.js | 23 ++++++++--- test/test.session-joining.selenium.js | 53 ++++++++++++++++++++++--- 6 files changed, 111 insertions(+), 45 deletions(-) diff --git a/lib/unhangout-sockets.js b/lib/unhangout-sockets.js index dce8971c..f04f9a64 100644 --- a/lib/unhangout-sockets.js +++ b/lib/unhangout-sockets.js @@ -407,14 +407,21 @@ _.extend(UnhangoutSocketManager.prototype, events.EventEmitter.prototype, { }.bind(this)); - mgr.on("session/set-connected-participants", _.bind(function(socket, args) { - var session = this.ensureSocketInSession(mgr, socket, args.sessionId, - "session/set-connected-participants"); + mgr.on("session/set-connected-participants", function(socket, args) { + var route = "session/set-connected-participants"; + var session = this.ensureSocketInSession(mgr, socket, args.sessionId, route); if (session) { - session.setConnectedParticipants(args.connectedParticipants); - return session.save(); + if (session.get("hangout-url") === args["hangout-url"]) { + session.setConnectedParticipants(args.connectedParticipants); + mgr.writeAck(socket, route); + return session.save(); + } else { + mgr.writeErr(socket, route, "Not in correct hangout"); + } + } else { + mgr.writeErr(socket, route, "Not found"); } - }, this)); + }.bind(this)); mgr.on("session/set-activities", _.bind(function(socket, args) { var session = this.ensureSocketInSession(mgr, socket, args.sessionId, diff --git a/package.json b/package.json index 6b3d996b..6b6af91b 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "postinstall": "node ./bin/compile-assets.js", "start": "node ./bin/unhangout-server", "setup": "source conf.sh", - "test": "NODE_ENV=testing mocha --timeout 10000 test/test.[a-r]* && sleep 1 && NODE_ENV=testing mocha --timeout 10000 test/test.[s-z]*", + "test": "NODE_ENV=testing mocha --timeout 10000 test/test.[a-r]* && sleep 1 && NODE_ENV=testing mocha test/test.s* && sleep 1 && NODE_ENV=testing mocha test/test.[t-z]*", "forever-start": "NODE_ENV=production forever start --minUptime 1000 --spinSleepTime 1000 -a -l unhangout.log -o logs/out.log -e logs/err.log bin/unhangout-server", "forever-stop": "forever stop bin/unhangout-server", "forever-restart": "NODE_ENV=production forever restart bin/unhangout-server", diff --git a/public/js/facilitator.js b/public/js/facilitator.js index c5a4b695..ca5c38a5 100644 --- a/public/js/facilitator.js +++ b/public/js/facilitator.js @@ -146,7 +146,8 @@ var FacilitatorView = Backbone.View.extend({ session.on("change:connectedParticipants", function() { sock.sendJSON("session/set-connected-participants", { sessionId: session.id, - connectedParticipants: session.get("connectedParticipants") + connectedParticipants: session.get("connectedParticipants"), + "hangout-url": session.get("hangout-url") }); }); session.on("change:hangout-broadcast-id", function() { diff --git a/test/common.js b/test/common.js index 5851b9d0..5e8e531a 100644 --- a/test/common.js +++ b/test/common.js @@ -72,10 +72,8 @@ var buildBrowser = function(callback) { return browser.executeScript("return true;").then(cb); }; browser.waitTime = function(time) { - var waited = false; - return browser.wait(function() { - setTimeout(function() { waited = true; }, time); - return browser.then(function() { return waited; }); + return new Promise(function(resolve, reject) { + setTimeout(resolve, time); }); }; browser.waitForFunc = function(cb) { @@ -104,7 +102,11 @@ exports.getSeleniumBrowser = function(callback) { } seleniumServer = new SeleniumServer(seleniumPath, {port: 4444}); seleniumServer.start().then(function() { - buildBrowser(callback); + // Throwing in a timeout on speculation that this makes + // intermittent timeouts in beforeAll hooks less common. + setTimeout(function() { + buildBrowser(callback); + }, 2000); }); } }; @@ -213,26 +215,30 @@ exports.sockWithPromiseClose = function() { // 'userKey', and join it to the given room. Depends on `exports.server` // already being inited with users. exports.authedSock = function(userKey, room, callback) { - var newSock = exports.sockWithPromiseClose(); - var user = exports.server.db.users.findWhere({"sock-key": userKey}); - var onData = function(message) { - var msg = JSON.parse(message); - if (msg.type === "auth-ack") { - newSock.write(JSON.stringify({type: "join", args: {id: room}})); - } else if (msg.type === "join-ack") { - newSock.removeListener("data", onData); - callback && callback(newSock); - } - }; - newSock.on("data", onData); - newSock.on("error", function(msg) { - console.log("socket error", msg); - }); - newSock.once("connection", function() { - newSock.write(JSON.stringify({ - type:"auth", - args:{ key: user.getSockKey(), id: user.id } - })); + return new Promise(function(resolve, reject) { + var newSock = exports.sockWithPromiseClose(); + var user = exports.server.db.users.findWhere({"sock-key": userKey}); + var onData = function(message) { + var msg = JSON.parse(message); + if (msg.type === "auth-ack") { + newSock.write(JSON.stringify({type: "join", args: {id: room}})); + } else if (msg.type === "join-ack") { + newSock.removeListener("data", onData); + callback && callback(newSock); + resolve(newSock); + } + }; + newSock.on("data", onData); + newSock.on("error", function(msg) { + console.log("socket error", msg); + reject(new Error(msg)); + }); + newSock.once("connection", function() { + newSock.write(JSON.stringify({ + type:"auth", + args:{ key: user.getSockKey(), id: user.id } + })); + }); }); }; diff --git a/test/test.mock-hangout.selenium.js b/test/test.mock-hangout.selenium.js index e3c93ea5..77592664 100644 --- a/test/test.mock-hangout.selenium.js +++ b/test/test.mock-hangout.selenium.js @@ -32,6 +32,13 @@ describe("MOCK HANGOUT", function() { }); }); + afterEach(function(done) { + // Get a URL that won't throw modals at us. + browser.get("http://localhost:7777/public/html/test.html").then(function() { + done(); + }); + }); + it("Communicates the hangout's URL on connction.", function(done) { var u1 = common.server.db.users.at(0); browser.get("http://localhost:7777/"); @@ -56,11 +63,17 @@ describe("MOCK HANGOUT", function() { var u2 = common.server.db.users.at(1); var u3 = common.server.db.users.at(2); var u4 = common.server.db.users.at(3); - var url = "http://localhost:7777/test/hangout/" + session.id + "/"; + var baseUrl = "http://localhost:7777/test/hangout/" + session.id + "/"; + var queryUrl = baseUrl + "?mockUserIds=" + [u1.id, u2.id, u3.id].join(","); + // Set the hangout URL because connectedParticipants will be refused if + // it doesn't match the URL we get (which in the mock hangout will + // include ?mockUserIds=...). + session.set("hangout-url", queryUrl); + browser.get("http://localhost:7777/"); browser.mockAuthenticate(u1.get("sock-key")); // First, load the hangout without extra users. - browser.get(url); + browser.get(queryUrl); // Wait for iframe to load. Would be cleaner to introspect, but.. ugh. browser.waitForFunc(function() { return session.getNumConnectedParticipants() == 1; @@ -68,16 +81,14 @@ describe("MOCK HANGOUT", function() { expect(session.getNumConnectedParticipants()).to.be(1); }); // Next, load the hangout with u2 and u3 as non-app users - browser.get("http://localhost:7777/test/hangout/" + session.id + "/?mockUserIds=" + [ - u1.id, u2.id, u3.id - ].join(",")) - // Wait a longer time, for the cross-document message with participants to come through. + browser.get(queryUrl) browser.waitForFunc(function() { return session.getNumConnectedParticipants() == 3; }).then(function() { expect(_.pluck(session.get("connectedParticipants"), "id")).to.eql([ u1.id, u2.id, u3.id ]); + session.set("hangout-url", baseUrl); done(); }); }); diff --git a/test/test.session-joining.selenium.js b/test/test.session-joining.selenium.js index 2818ad7e..dd6663c7 100644 --- a/test/test.session-joining.selenium.js +++ b/test/test.session-joining.selenium.js @@ -269,15 +269,16 @@ describe("SESSION JOINING PARTICIPANT LISTS", function() { sock = thesock; }); }); - browser.waitTime(2000).then(function () { - expect(session.get("connectedParticipants").length).to.be(2); + browser.wait(function() { + return session.get("connectedParticipants").length == 2; + }).then(function() { common.restartServer(function onStopped(restart) { framedDisconnectionModalShowing(true).then(function() { restart(); }); }, function onRestarted() { framedDisconnectionModalShowing(false); - browser.waitTime(1000).then(function() { + browser.waitTime(5000).then(function() { // Refresh session from new DB. event = common.server.db.events.get(event.id); session = event.get("sessions").get(session.id); @@ -287,6 +288,7 @@ describe("SESSION JOINING PARTICIPANT LISTS", function() { }); }); }); + it("Warns you when you're in the wrong hangout", function(done) { var session = event.get("sessions").at(0); var button = "document.getElementsByTagName('iframe')[0].contentWindow" + @@ -302,14 +304,18 @@ describe("SESSION JOINING PARTICIPANT LISTS", function() { }); browser.executeScript("return " + button + ".href").then(function(href) { expect(href).to.be("http://example.com/"); + }); + // Go to a different URL that won't throw a modal dialog up. + browser.get("http://localhost:7777/").then(function() { done(); }); }); + it("Doesn't clear hangout URL immediately, but rather after a delay.", function(done) { var session = event.get("sessions").at(1); session.set("hangoutConnected", false); session.set("hangout-url", null); - common.authedSock("regular1", session.getRoomId(), function(sock) { + common.authedSock("regular1", session.getRoomId()).then(function(sock) { sock.on("data", function(message) { var msg = JSON.parse(message); if (msg.type == "session/set-hangout-url-ack") { @@ -324,9 +330,11 @@ describe("SESSION JOINING PARTICIPANT LISTS", function() { // We don't test that it actually gets invalidated here, // because the delay is LONG, and sinon doesn't play well // with asynchronous socket comms. - done(); + sock.promiseClose().then(done); } else { - done(new Error(message)); + sock.promiseClose().then(function() { + done(new Error("Unexpected message: " + message)); + }); } }); sock.write(JSON.stringify({ @@ -337,6 +345,39 @@ describe("SESSION JOINING PARTICIPANT LISTS", function() { }, })); }); + }); + it("Doesn't set connected participants if URL is invalid.", function(done) { + var session = event.get("sessions").at(1); + var participants = [{id: "p1", displayName: "P1", picture: ""}, + {id: "p2", displayName: "P2", picture: ""}, + {id: "0", displayName: "Regular1 Mock", picture: ""}]; + session.set("hangout-url", "http://example.com"); + session.set("connectedParticipants", participants); + + common.authedSock("regular1", session.getRoomId()).then(function(sock) { + sock.on("data", function(message) { + var msg = JSON.parse(message); + if (msg.type === "session/set-connected-participants-err") { + expect(msg.args).to.eql("Not in correct hangout"); + expect(session.get("connectedParticipants")).to.eql(participants); + sock.promiseClose().then(done); + } else { + sock.promiseClose().then(function() { + done(new Error("Unexpected message: " + message)); + }); + } + }); + sock.write(JSON.stringify({ + type: "session/set-connected-participants", + args: { + sessionId: session.id, + "hangout-url": "http://example2.com", + connectedParticipants: [ + {id: "0", displayName: "Regular1 Mock", picture: ""} + ] + } + })); + }); }); });