From 6765d033396b303de794e8891ca0dc39b54502ae Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 4 Oct 2019 10:30:24 +0100 Subject: [PATCH 1/4] Track the `isRestrictedUser` flag on clients Then, don't send new chat messages and new comments to those restricted clients. We do this because we don't want to leak private information (email addresses and names) to "restricted" users, those who have read-only access via a shared token. --- .../real-time/app/coffee/WebApiManager.coffee | 4 +- .../app/coffee/WebsocketController.coffee | 30 ++--- .../app/coffee/WebsocketLoadBalancer.coffee | 29 ++++- .../unit/coffee/WebApiManagerTests.coffee | 19 +-- .../coffee/WebsocketControllerTests.coffee | 110 +++++++++--------- .../coffee/WebsocketLoadBalancerTests.coffee | 62 +++++++++- 6 files changed, 165 insertions(+), 89 deletions(-) diff --git a/services/real-time/app/coffee/WebApiManager.coffee b/services/real-time/app/coffee/WebApiManager.coffee index 6836d5f213..ce622d496f 100644 --- a/services/real-time/app/coffee/WebApiManager.coffee +++ b/services/real-time/app/coffee/WebApiManager.coffee @@ -4,7 +4,7 @@ logger = require "logger-sharelatex" { CodedError } = require "./Errors" module.exports = WebApiManager = - joinProject: (project_id, user, callback = (error, project, privilegeLevel) ->) -> + joinProject: (project_id, user, callback = (error, project, privilegeLevel, isRestrictedUser) ->) -> user_id = user._id logger.log {project_id, user_id}, "sending join project request to web" url = "#{settings.apis.web.url}/project/#{project_id}/join" @@ -24,7 +24,7 @@ module.exports = WebApiManager = }, (error, response, data) -> return callback(error) if error? if 200 <= response.statusCode < 300 - callback null, data?.project, data?.privilegeLevel + callback null, data?.project, data?.privilegeLevel, data?.isRestrictedUser else if response.statusCode == 429 logger.log(project_id, user_id, "rate-limit hit when joining project") callback(new CodedError("rate-limit hit when joining project", "TooManyRequests")) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index 5b03f334d5..bc46ce12e0 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -13,12 +13,12 @@ module.exports = WebsocketController = # it will force a full refresh of the page. Useful for non-backwards # compatible protocol changes. Use only in extreme need. PROTOCOL_VERSION: 2 - + joinProject: (client, user, project_id, callback = (error, project, privilegeLevel, protocolVersion) ->) -> user_id = user?._id logger.log {user_id, project_id, client_id: client.id}, "user joining project" metrics.inc "editor.join-project" - WebApiManager.joinProject project_id, user, (error, project, privilegeLevel) -> + WebApiManager.joinProject project_id, user, (error, project, privilegeLevel, isRestrictedUser) -> return callback(error) if error? if !privilegeLevel or privilegeLevel == "" @@ -26,7 +26,6 @@ module.exports = WebsocketController = logger.warn {err, project_id, user_id, client_id: client.id}, "user is not authorized to join project" return callback(err) - client.set("privilege_level", privilegeLevel) client.set("user_id", user_id) client.set("project_id", project_id) @@ -37,18 +36,19 @@ module.exports = WebsocketController = client.set("connected_time", new Date()) client.set("signup_date", user?.signUpDate) client.set("login_count", user?.loginCount) - + client.set("is_restricted_user", !!(isRestrictedUser)) + RoomManager.joinProject client, project_id, (err) -> logger.log {user_id, project_id, client_id: client.id}, "user joined project" callback null, project, privilegeLevel, WebsocketController.PROTOCOL_VERSION - + # No need to block for setting the user as connected in the cursor tracking ConnectedUsersManager.updateUserPosition project_id, client.id, user, null, () -> - + # We want to flush a project if there are no more (local) connected clients # but we need to wait for the triggering client to disconnect. How long we wait # is determined by FLUSH_IF_EMPTY_DELAY. - FLUSH_IF_EMPTY_DELAY: 500 #ms + FLUSH_IF_EMPTY_DELAY: 500 #ms leaveProject: (io, client, callback = (error) ->) -> metrics.inc "editor.leave-project" Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> @@ -56,7 +56,7 @@ module.exports = WebsocketController = logger.log {project_id, user_id, client_id: client.id}, "client leaving project" WebsocketLoadBalancer.emitToRoom project_id, "clientTracking.clientDisconnected", client.id - + # bail out if the client had not managed to authenticate or join # the project. Prevents downstream errors in docupdater from # flushProjectToMongoAndDelete with null project_id. @@ -66,12 +66,12 @@ module.exports = WebsocketController = if not project_id? logger.log {user_id: user_id, client_id: client.id}, "client leaving, not in project" return callback() - + # We can do this in the background ConnectedUsersManager.markUserAsDisconnected project_id, client.id, (err) -> if err? logger.error {err, project_id, user_id, client_id: client.id}, "error marking client as disconnected" - + RoomManager.leaveProjectAndDocs(client) setTimeout () -> remainingClients = io.sockets.clients(project_id) @@ -82,14 +82,14 @@ module.exports = WebsocketController = logger.error {err, project_id, user_id, client_id: client.id}, "error flushing to doc updater after leaving project" callback() , WebsocketController.FLUSH_IF_EMPTY_DELAY - + joinDoc: (client, doc_id, fromVersion = -1, options, callback = (error, doclines, version, ops, ranges) ->) -> metrics.inc "editor.join-doc" Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> return callback(error) if error? return callback(new Error("no project_id found on client")) if !project_id? logger.log {user_id, project_id, doc_id, fromVersion, client_id: client.id}, "client joining doc" - + AuthorizationManager.assertClientCanViewProject client, (error) -> return callback(error) if error? # ensure the per-doc applied-ops channel is subscribed before sending the @@ -124,7 +124,7 @@ module.exports = WebsocketController = AuthorizationManager.addAccessToDoc client, doc_id logger.log {user_id, project_id, doc_id, fromVersion, client_id: client.id}, "client joined doc" callback null, escapedLines, version, ops, ranges - + leaveDoc: (client, doc_id, callback = (error) ->) -> metrics.inc "editor.leave-doc" Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> @@ -227,9 +227,9 @@ module.exports = WebsocketController = return callback(error) else return callback(null) - + _isCommentUpdate: (update) -> for op in update.op if !op.c? return false - return true \ No newline at end of file + return true diff --git a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee index 45a7645005..21e6235505 100644 --- a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee +++ b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee @@ -7,6 +7,8 @@ HealthCheckManager = require "./HealthCheckManager" RoomManager = require "./RoomManager" ChannelManager = require "./ChannelManager" ConnectedUsersManager = require "./ConnectedUsersManager" +Utils = require './Utils' +Async = require 'async' module.exports = WebsocketLoadBalancer = rclientPubList: RedisClientManager.createClientList(Settings.redis.pubsub) @@ -58,7 +60,7 @@ module.exports = WebsocketLoadBalancer = else if message.message is 'clientTracking.refresh' && message.room_id? clientList = io.sockets.clients(message.room_id) logger.log {channel:channel, message: message.message, room_id: message.room_id, message_id: message._id, socketIoClients: (client.id for client in clientList)}, "refreshing client list" - for client in clientList + for client in clientList ConnectedUsersManager.refreshClient(message.room_id, client.id) else if message.room_id? if message._id? && Settings.checkEventOrder @@ -69,12 +71,27 @@ module.exports = WebsocketLoadBalancer = clientList = io.sockets.clients(message.room_id) # avoid unnecessary work if no clients are connected return if clientList.length is 0 - logger.log {channel:channel, message: message.message, room_id: message.room_id, message_id: message._id, socketIoClients: (client.id for client in clientList)}, "distributing event to clients" + logger.log { + channel: channel, + message: message.message, + room_id: message.room_id, + message_id: message._id, + socketIoClients: (client.id for client in clientList) + }, "distributing event to clients" seen = {} - for client in clientList when not seen[client.id] - seen[client.id] = true - client.emit(message.message, message.payload...) + # Send the messages to clients async, don't wait for them all to finish + Async.eachSeries clientList + , (client, cb) -> + Utils.getClientAttributes client, ['is_restricted_user'], (err, {is_restricted_user}) -> + return cb(err) if err? + if !seen[client.id] + seen[client.id] = true + if !(is_restricted_user && message.message in ['new-chat-message', 'new-comment']) + client.emit(message.message, message.payload...) + cb() + , (err) -> + if err? + logger.err {err, message}, "Error sending message to clients" else if message.health_check? logger.debug {message}, "got health check message in editor events channel" HealthCheckManager.check channel, message.key - diff --git a/services/real-time/test/unit/coffee/WebApiManagerTests.coffee b/services/real-time/test/unit/coffee/WebApiManagerTests.coffee index 278585518d..872666d19b 100644 --- a/services/real-time/test/unit/coffee/WebApiManagerTests.coffee +++ b/services/real-time/test/unit/coffee/WebApiManagerTests.coffee @@ -20,17 +20,18 @@ describe 'WebApiManager', -> user: "username" pass: "password" "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } - + describe "joinProject", -> describe "successfully", -> beforeEach -> @response = { project: { name: "Test project" } - privilegeLevel: "owner" + privilegeLevel: "owner", + isRestrictedUser: true } @request.post = sinon.stub().callsArgWith(1, null, {statusCode: 200}, @response) @WebApiManager.joinProject @project_id, @user, @callback - + it "should send a request to web to join the project", -> @request.post .calledWith({ @@ -46,22 +47,22 @@ describe 'WebApiManager', -> headers: {} }) .should.equal true - - it "should return the project and privilegeLevel", -> + + it "should return the project, privilegeLevel, and restricted flag", -> @callback - .calledWith(null, @response.project, @response.privilegeLevel) + .calledWith(null, @response.project, @response.privilegeLevel, @response.isRestrictedUser) .should.equal true - + describe "with an error from web", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, {statusCode: 500}, null) @WebApiManager.joinProject @project_id, @user_id, @callback - + it "should call the callback with an error", -> @callback .calledWith(new Error("non-success code from web: 500")) .should.equal true - + describe "when the project is over its rate limit", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, {statusCode: 429}, null) diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index edb0055bf5..ab5a503716 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -37,10 +37,10 @@ describe 'WebsocketController', -> inc: sinon.stub() set: sinon.stub() "./RoomManager": @RoomManager = {} - + afterEach -> tk.reset() - + describe "joinProject", -> describe "when authorised", -> beforeEach -> @@ -53,61 +53,65 @@ describe 'WebsocketController', -> } @privilegeLevel = "owner" @ConnectedUsersManager.updateUserPosition = sinon.stub().callsArg(4) - @WebApiManager.joinProject = sinon.stub().callsArgWith(2, null, @project, @privilegeLevel) + @isRestrictedUser = true + @WebApiManager.joinProject = sinon.stub().callsArgWith(2, null, @project, @privilegeLevel, @isRestrictedUser) @RoomManager.joinProject = sinon.stub().callsArg(2) @WebsocketController.joinProject @client, @user, @project_id, @callback - + it "should load the project from web", -> @WebApiManager.joinProject .calledWith(@project_id, @user) .should.equal true - + it "should join the project room", -> @RoomManager.joinProject.calledWith(@client, @project_id).should.equal true - + it "should set the privilege level on the client", -> @client.set.calledWith("privilege_level", @privilegeLevel).should.equal true - + it "should set the user's id on the client", -> @client.set.calledWith("user_id", @user._id).should.equal true - + it "should set the user's email on the client", -> @client.set.calledWith("email", @user.email).should.equal true - + it "should set the user's first_name on the client", -> @client.set.calledWith("first_name", @user.first_name).should.equal true - + it "should set the user's last_name on the client", -> @client.set.calledWith("last_name", @user.last_name).should.equal true - + it "should set the user's sign up date on the client", -> @client.set.calledWith("signup_date", @user.signUpDate).should.equal true - + it "should set the user's login_count on the client", -> @client.set.calledWith("login_count", @user.loginCount).should.equal true - + it "should set the connected time on the client", -> @client.set.calledWith("connected_time", new Date()).should.equal true - + it "should set the project_id on the client", -> @client.set.calledWith("project_id", @project_id).should.equal true - + it "should set the project owner id on the client", -> @client.set.calledWith("owner_id", @owner_id).should.equal true - + + it "should set the is_restricted_user flag on the client", -> + @client.set.calledWith("is_restricted_user", @isRestrictedUser).should.equal true + it "should call the callback with the project, privilegeLevel and protocolVersion", -> @callback .calledWith(null, @project, @privilegeLevel, @WebsocketController.PROTOCOL_VERSION) .should.equal true - + it "should mark the user as connected in ConnectedUsersManager", -> @ConnectedUsersManager.updateUserPosition .calledWith(@project_id, @client.id, @user, null) .should.equal true - + it "should increment the join-project metric", -> @metrics.inc.calledWith("editor.join-project").should.equal true - + describe "when not authorized", -> beforeEach -> @WebApiManager.joinProject = sinon.stub().callsArgWith(2, null, null, null) @@ -138,30 +142,30 @@ describe 'WebsocketController', -> @client.params.user_id = @user_id @WebsocketController.FLUSH_IF_EMPTY_DELAY = 0 tk.reset() # Allow setTimeout to work. - + describe "when the project is empty", -> beforeEach (done) -> @clientsInRoom = [] @WebsocketController.leaveProject @io, @client, done - + it "should end clientTracking.clientDisconnected to the project room", -> @WebsocketLoadBalancer.emitToRoom .calledWith(@project_id, "clientTracking.clientDisconnected", @client.id) .should.equal true - + it "should mark the user as disconnected", -> @ConnectedUsersManager.markUserAsDisconnected .calledWith(@project_id, @client.id) .should.equal true - + it "should flush the project in the document updater", -> @DocumentUpdaterManager.flushProjectToMongoAndDelete .calledWith(@project_id) .should.equal true - + it "should increment the leave-project metric", -> @metrics.inc.calledWith("editor.leave-project").should.equal true - + it "should track the disconnection in RoomManager", -> @RoomManager.leaveProjectAndDocs .calledWith(@client) @@ -171,7 +175,7 @@ describe 'WebsocketController', -> beforeEach -> @clientsInRoom = ["mock-remaining-client"] @WebsocketController.leaveProject @io, @client - + it "should not flush the project in the document updater", -> @DocumentUpdaterManager.flushProjectToMongoAndDelete .called.should.equal false @@ -232,7 +236,7 @@ describe 'WebsocketController', -> @ops = ["mock", "ops"] @ranges = { "mock": "ranges" } @options = {} - + @client.params.project_id = @project_id @AuthorizationManager.addAccessToDoc = sinon.stub() @AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, null) @@ -275,7 +279,7 @@ describe 'WebsocketController', -> beforeEach -> @fromVersion = 40 @WebsocketController.joinDoc @client, @doc_id, @fromVersion, @options, @callback - + it "should get the document from the DocumentUpdaterManager with fromVersion", -> @DocumentUpdaterManager.getDocument .calledWith(@project_id, @doc_id, @fromVersion) @@ -285,7 +289,7 @@ describe 'WebsocketController', -> beforeEach -> @doc_lines.push ["räksmörgås"] @WebsocketController.joinDoc @client, @doc_id, -1, @options, @callback - + it "should call the callback with the escaped lines", -> escaped_lines = @callback.args[0][1] escaped_word = escaped_lines.pop() @@ -327,44 +331,44 @@ describe 'WebsocketController', -> beforeEach -> @AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, @err = new Error("not authorized")) @WebsocketController.joinDoc @client, @doc_id, -1, @options, @callback - + it "should call the callback with an error", -> @callback.calledWith(@err).should.equal true - + it "should not call the DocumentUpdaterManager", -> @DocumentUpdaterManager.getDocument.called.should.equal false - + describe "leaveDoc", -> beforeEach -> - @doc_id = "doc-id-123" + @doc_id = "doc-id-123" @client.params.project_id = @project_id @RoomManager.leaveDoc = sinon.stub() @WebsocketController.leaveDoc @client, @doc_id, @callback - + it "should remove the client from the doc_id room", -> @RoomManager.leaveDoc .calledWith(@client, @doc_id).should.equal true - + it "should call the callback", -> @callback.called.should.equal true - + it "should increment the leave-doc metric", -> @metrics.inc.calledWith("editor.leave-doc").should.equal true - + describe "getConnectedUsers", -> beforeEach -> @client.params.project_id = @project_id @users = ["mock", "users"] @WebsocketLoadBalancer.emitToRoom = sinon.stub() @ConnectedUsersManager.getConnectedUsers = sinon.stub().callsArgWith(1, null, @users) - + describe "when authorized", -> beforeEach (done) -> @AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, null) @WebsocketController.getConnectedUsers @client, (args...) => @callback(args...) done() - + it "should check that the client is authorized to view the project", -> @AuthorizationManager.assertClientCanViewProject .calledWith(@client) @@ -379,26 +383,26 @@ describe 'WebsocketController', -> @ConnectedUsersManager.getConnectedUsers .calledWith(@project_id) .should.equal true - + it "should return the users", -> @callback.calledWith(null, @users).should.equal true - + it "should increment the get-connected-users metric", -> @metrics.inc.calledWith("editor.get-connected-users").should.equal true - + describe "when not authorized", -> beforeEach -> @AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, @err = new Error("not authorized")) @WebsocketController.getConnectedUsers @client, @callback - + it "should not get the connected users for the project", -> @ConnectedUsersManager.getConnectedUsers .called .should.equal false - + it "should return an error", -> @callback.calledWith(@err).should.equal true - + describe "updateClientPosition", -> beforeEach -> @WebsocketLoadBalancer.emitToRoom = sinon.stub() @@ -422,7 +426,7 @@ describe 'WebsocketController', -> @client.get = (param, callback) => callback null, @clientParams[param] @WebsocketController.updateClientPosition @client, @update - @populatedCursorData = + @populatedCursorData = doc_id: @doc_id, id: @client.id name: "#{@first_name} #{@last_name}" @@ -462,7 +466,7 @@ describe 'WebsocketController', -> @client.get = (param, callback) => callback null, @clientParams[param] @WebsocketController.updateClientPosition @client, @update - @populatedCursorData = + @populatedCursorData = doc_id: @doc_id, id: @client.id name: "#{@first_name}" @@ -502,7 +506,7 @@ describe 'WebsocketController', -> @client.get = (param, callback) => callback null, @clientParams[param] @WebsocketController.updateClientPosition @client, @update - @populatedCursorData = + @populatedCursorData = doc_id: @doc_id, id: @client.id name: "#{@last_name}" @@ -603,7 +607,7 @@ describe 'WebsocketController', -> it "should call the callback", -> @callback.called.should.equal true - + it "should increment the doc updates", -> @metrics.inc.calledWith("editor.doc-update").should.equal true @@ -621,7 +625,7 @@ describe 'WebsocketController', -> it "should call the callback with the error", -> @callback.calledWith(@error).should.equal true - + describe "when not authorized", -> beforeEach -> @client.disconnect = sinon.stub() @@ -645,7 +649,7 @@ describe 'WebsocketController', -> @comment_update = { op: [{c: "bar", p: 132}] } @AuthorizationManager.assertClientCanEditProjectAndDoc = sinon.stub() @AuthorizationManager.assertClientCanViewProjectAndDoc = sinon.stub() - + describe "with a read-write client", -> it "should return successfully", (done) -> @AuthorizationManager.assertClientCanEditProjectAndDoc.yields(null) @@ -660,7 +664,7 @@ describe 'WebsocketController', -> @WebsocketController._assertClientCanApplyUpdate @client, @doc_id, @edit_update, (error) -> expect(error.message).to.equal "not authorized" done() - + describe "with a read-only client and a comment op", -> it "should return successfully", (done) -> @AuthorizationManager.assertClientCanEditProjectAndDoc.yields(new Error("not authorized")) @@ -668,7 +672,7 @@ describe 'WebsocketController', -> @WebsocketController._assertClientCanApplyUpdate @client, @doc_id, @comment_update, (error) -> expect(error).to.be.null done() - + describe "with a totally unauthorized client", -> it "should return an error", (done) -> @AuthorizationManager.assertClientCanEditProjectAndDoc.yields(new Error("not authorized")) diff --git a/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee b/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee index 66e950cd0e..a1906ab72c 100644 --- a/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee @@ -8,7 +8,7 @@ describe "WebsocketLoadBalancer", -> @rclient = {} @RoomEvents = {on: sinon.stub()} @WebsocketLoadBalancer = SandboxedModule.require modulePath, requires: - "./RedisClientManager": + "./RedisClientManager": createClientList: () => [] "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } "./SafeJsonParse": @SafeJsonParse = @@ -18,6 +18,12 @@ describe "WebsocketLoadBalancer", -> "./RoomManager" : @RoomManager = {eventSource: sinon.stub().returns @RoomEvents} "./ChannelManager": @ChannelManager = {publish: sinon.stub()} "./ConnectedUsersManager": @ConnectedUsersManager = {refreshClient: sinon.stub()} + "./Utils": @Utils = { + getClientAttributes: sinon.spy( + (client, _attrs, callback) -> + callback(null, {is_restricted_user: !!client.__isRestricted}) + ) + } @io = {} @WebsocketLoadBalancer.rclientPubList = [{publish: sinon.stub()}] @WebsocketLoadBalancer.rclientSubList = [{ @@ -51,7 +57,7 @@ describe "WebsocketLoadBalancer", -> @WebsocketLoadBalancer.emitToRoom .calledWith("all", @message, @payload...) .should.equal true - + describe "listenForEditorEvents", -> beforeEach -> @WebsocketLoadBalancer._processEditorEvent = sinon.stub() @@ -70,9 +76,10 @@ describe "WebsocketLoadBalancer", -> describe "_processEditorEvent", -> describe "with bad JSON", -> beforeEach -> + @isRestrictedUser = false @SafeJsonParse.parse = sinon.stub().callsArgWith 1, new Error("oops") @WebsocketLoadBalancer._processEditorEvent(@io, "editor-events", "blah") - + it "should log an error", -> @logger.error.called.should.equal true @@ -98,6 +105,54 @@ describe "WebsocketLoadBalancer", -> @emit2.calledWith(@message, @payload...).should.equal true @emit3.called.should.equal false # duplicate client should be ignored + describe "with a designated room, and restricted clients, not restricted message", -> + beforeEach -> + @io.sockets = + clients: sinon.stub().returns([ + {id: 'client-id-1', emit: @emit1 = sinon.stub()} + {id: 'client-id-2', emit: @emit2 = sinon.stub()} + {id: 'client-id-1', emit: @emit3 = sinon.stub()} # duplicate client + {id: 'client-id-4', emit: @emit4 = sinon.stub(), __isRestricted: true} + ]) + data = JSON.stringify + room_id: @room_id + message: @message + payload: @payload + @WebsocketLoadBalancer._processEditorEvent(@io, "editor-events", data) + + it "should send the message to all (unique) clients in the room", -> + @io.sockets.clients + .calledWith(@room_id) + .should.equal true + @emit1.calledWith(@message, @payload...).should.equal true + @emit2.calledWith(@message, @payload...).should.equal true + @emit3.called.should.equal false # duplicate client should be ignored + @emit4.called.should.equal true # restricted client, but should be called + + describe "with a designated room, and restricted clients, restricted message", -> + beforeEach -> + @io.sockets = + clients: sinon.stub().returns([ + {id: 'client-id-1', emit: @emit1 = sinon.stub()} + {id: 'client-id-2', emit: @emit2 = sinon.stub()} + {id: 'client-id-1', emit: @emit3 = sinon.stub()} # duplicate client + {id: 'client-id-4', emit: @emit4 = sinon.stub(), __isRestricted: true} + ]) + data = JSON.stringify + room_id: @room_id + message: @restrictedMessage = 'new-comment' + payload: @payload + @WebsocketLoadBalancer._processEditorEvent(@io, "editor-events", data) + + it "should send the message to all (unique) clients in the room, who are not restricted", -> + @io.sockets.clients + .calledWith(@room_id) + .should.equal true + @emit1.calledWith(@restrictedMessage, @payload...).should.equal true + @emit2.calledWith(@restrictedMessage, @payload...).should.equal true + @emit3.called.should.equal false # duplicate client should be ignored + @emit4.called.should.equal false # restricted client, should not be called + describe "when emitting to all", -> beforeEach -> @io.sockets = @@ -110,4 +165,3 @@ describe "WebsocketLoadBalancer", -> it "should send the message to all clients", -> @emit.calledWith(@message, @payload...).should.equal true - From df6cd4a0544520b413536d26f63fff0049317dbb Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 4 Oct 2019 13:41:49 +0100 Subject: [PATCH 2/4] Also block getConnectedUsers for restricted users. Plus refactor to use a pass list instead of a deny list. --- .../app/coffee/WebsocketController.coffee | 5 ++++- .../app/coffee/WebsocketLoadBalancer.coffee | 13 ++++++++++++- .../unit/coffee/WebsocketControllerTests.coffee | 14 ++++++++++++++ .../unit/coffee/WebsocketLoadBalancerTests.coffee | 2 +- 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index bc46ce12e0..d0ca99cc5c 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -178,8 +178,11 @@ module.exports = WebsocketController = CLIENT_REFRESH_DELAY: 1000 getConnectedUsers: (client, callback = (error, users) ->) -> metrics.inc "editor.get-connected-users" - Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> + Utils.getClientAttributes client, ["project_id", "user_id", "is_restricted_user"], (error, clientAttributes) -> return callback(error) if error? + {project_id, user_id, is_restricted_user} = clientAttributes + if is_restricted_user + return callback(null, []) return callback(new Error("no project_id found on client")) if !project_id? logger.log {user_id, project_id, client_id: client.id}, "getting connected users" AuthorizationManager.assertClientCanViewProject client, (error) -> diff --git a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee index 21e6235505..ba69d79beb 100644 --- a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee +++ b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee @@ -10,6 +10,17 @@ ConnectedUsersManager = require "./ConnectedUsersManager" Utils = require './Utils' Async = require 'async' +RESTRICTED_USER_MESSAGE_TYPE_PASS_LIST = [ + 'connectionAccepted', + 'otUpdateApplied', + 'otUpdateError', + 'joinDoc', + 'reciveNewDoc', + 'reciveNewFile', + 'reciveNewFolder', + 'removeEntity' +] + module.exports = WebsocketLoadBalancer = rclientPubList: RedisClientManager.createClientList(Settings.redis.pubsub) rclientSubList: RedisClientManager.createClientList(Settings.redis.pubsub) @@ -86,7 +97,7 @@ module.exports = WebsocketLoadBalancer = return cb(err) if err? if !seen[client.id] seen[client.id] = true - if !(is_restricted_user && message.message in ['new-chat-message', 'new-comment']) + if !(is_restricted_user && message.message not in RESTRICTED_USER_MESSAGE_TYPE_PASS_LIST) client.emit(message.message, message.payload...) cb() , (err) -> diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index ab5a503716..116485384d 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -403,6 +403,20 @@ describe 'WebsocketController', -> it "should return an error", -> @callback.calledWith(@err).should.equal true + describe "when restricted user", -> + beforeEach -> + @client.params.is_restricted_user = true + @AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, null) + @WebsocketController.getConnectedUsers @client, @callback + + it "should return an empty array of users", -> + @callback.calledWith(null, []).should.equal true + + it "should not get the connected users for the project", -> + @ConnectedUsersManager.getConnectedUsers + .called + .should.equal false + describe "updateClientPosition", -> beforeEach -> @WebsocketLoadBalancer.emitToRoom = sinon.stub() diff --git a/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee b/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee index a1906ab72c..e6fe1df8c9 100644 --- a/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee @@ -32,7 +32,7 @@ describe "WebsocketLoadBalancer", -> }] @room_id = "room-id" - @message = "message-to-editor" + @message = "otUpdateApplied" @payload = ["argument one", 42] describe "emitToRoom", -> From 06aa578bdcbc8041396a58c2d1cc59cf303ae8ea Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 11 Oct 2019 09:57:16 +0100 Subject: [PATCH 3/4] Make it an error when we get no data from joinProject --- services/real-time/app/coffee/WebApiManager.coffee | 6 +++++- .../test/unit/coffee/WebApiManagerTests.coffee | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/services/real-time/app/coffee/WebApiManager.coffee b/services/real-time/app/coffee/WebApiManager.coffee index ce622d496f..3c0551a815 100644 --- a/services/real-time/app/coffee/WebApiManager.coffee +++ b/services/real-time/app/coffee/WebApiManager.coffee @@ -24,7 +24,11 @@ module.exports = WebApiManager = }, (error, response, data) -> return callback(error) if error? if 200 <= response.statusCode < 300 - callback null, data?.project, data?.privilegeLevel, data?.isRestrictedUser + if !data? || !data?.project? + err = new Error('no data returned from joinProject request') + logger.error {err, project_id, user_id}, "error accessing web api" + return callback(err) + callback null, data.project, data.privilegeLevel, data.isRestrictedUser else if response.statusCode == 429 logger.log(project_id, user_id, "rate-limit hit when joining project") callback(new CodedError("rate-limit hit when joining project", "TooManyRequests")) diff --git a/services/real-time/test/unit/coffee/WebApiManagerTests.coffee b/services/real-time/test/unit/coffee/WebApiManagerTests.coffee index 872666d19b..a87522c0a9 100644 --- a/services/real-time/test/unit/coffee/WebApiManagerTests.coffee +++ b/services/real-time/test/unit/coffee/WebApiManagerTests.coffee @@ -63,6 +63,16 @@ describe 'WebApiManager', -> .calledWith(new Error("non-success code from web: 500")) .should.equal true + describe "with no data from web", -> + beforeEach -> + @request.post = sinon.stub().callsArgWith(1, null, {statusCode: 200}, null) + @WebApiManager.joinProject @project_id, @user_id, @callback + + it "should call the callback with an error", -> + @callback + .calledWith(new Error("no data returned from joinProject request")) + .should.equal true + describe "when the project is over its rate limit", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, {statusCode: 429}, null) From 2cc2be3d9cda293dcedfc85723f4762075b551d9 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 11 Oct 2019 10:01:21 +0100 Subject: [PATCH 4/4] send messages to clients with concurrency of 2 --- services/real-time/app/coffee/WebsocketLoadBalancer.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee index ba69d79beb..12b7ef812b 100644 --- a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee +++ b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee @@ -91,7 +91,8 @@ module.exports = WebsocketLoadBalancer = }, "distributing event to clients" seen = {} # Send the messages to clients async, don't wait for them all to finish - Async.eachSeries clientList + Async.eachLimit clientList + , 2 , (client, cb) -> Utils.getClientAttributes client, ['is_restricted_user'], (err, {is_restricted_user}) -> return cb(err) if err?